| Message ID | 1588646662-25785-1-git-send-email-bbhatt@codeaurora.org |
|---|---|
| Headers | show |
| Series | Bug fixes and improved logging in MHI | expand |
On 2020-05-05 08:57, Jeffrey Hugo wrote: > On 5/4/2020 8:44 PM, Bhaumik Bhatt wrote: >> While writing any sequence or session identifiers, it is possible that >> the host could write a zero value, whereas only non-zero values should >> be supported writes to those registers. Ensure that the host does not >> write a non-zero value for them and also log them in debug messages. >> >> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> >> --- >> drivers/bus/mhi/core/boot.c | 15 +++++++-------- >> drivers/bus/mhi/core/internal.h | 2 ++ >> 2 files changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c >> index e5fcde1..7b9b561 100644 >> --- a/drivers/bus/mhi/core/boot.c >> +++ b/drivers/bus/mhi/core/boot.c >> @@ -43,10 +43,7 @@ void mhi_rddm_prepare(struct mhi_controller >> *mhi_cntrl, >> lower_32_bits(mhi_buf->dma_addr)); >> mhi_write_reg(mhi_cntrl, base, BHIE_RXVECSIZE_OFFS, >> mhi_buf->len); >> - sequence_id = prandom_u32() & BHIE_RXVECSTATUS_SEQNUM_BMSK; >> - >> - if (unlikely(!sequence_id)) >> - sequence_id = 1; >> + sequence_id = MHI_RANDOM_U32_NONZERO(BHIE_RXVECSTATUS_SEQNUM_BMSK); >> mhi_write_reg_field(mhi_cntrl, base, BHIE_RXVECDB_OFFS, >> BHIE_RXVECDB_SEQNUM_BMSK, BHIE_RXVECDB_SEQNUM_SHFT, >> @@ -189,7 +186,9 @@ static int mhi_fw_load_amss(struct mhi_controller >> *mhi_cntrl, >> return -EIO; >> } >> - dev_dbg(dev, "Starting AMSS download via BHIe\n"); >> + sequence_id = MHI_RANDOM_U32_NONZERO(BHIE_TXVECSTATUS_SEQNUM_BMSK); >> + dev_dbg(dev, "Starting AMSS download via BHIe. Sequence ID:%u\n", >> + sequence_id); >> mhi_write_reg(mhi_cntrl, base, BHIE_TXVECADDR_HIGH_OFFS, >> upper_32_bits(mhi_buf->dma_addr)); >> @@ -198,7 +197,6 @@ static int mhi_fw_load_amss(struct >> mhi_controller *mhi_cntrl, >> mhi_write_reg(mhi_cntrl, base, BHIE_TXVECSIZE_OFFS, >> mhi_buf->len); >> - sequence_id = prandom_u32() & BHIE_TXVECSTATUS_SEQNUM_BMSK; >> mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS, >> BHIE_TXVECDB_SEQNUM_BMSK, BHIE_TXVECDB_SEQNUM_SHFT, >> sequence_id); >> @@ -246,14 +244,15 @@ static int mhi_fw_load_sbl(struct mhi_controller >> *mhi_cntrl, >> goto invalid_pm_state; >> } >> - dev_dbg(dev, "Starting SBL download via BHI\n"); >> + session_id = MHI_RANDOM_U32_NONZERO(BHI_TXDB_SEQNUM_BMSK); >> + dev_dbg(dev, "Starting SBL download via BHI. Session ID:%u\n", >> + session_id); >> mhi_write_reg(mhi_cntrl, base, BHI_STATUS, 0); >> mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_HIGH, >> upper_32_bits(dma_addr)); >> mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_LOW, >> lower_32_bits(dma_addr)); >> mhi_write_reg(mhi_cntrl, base, BHI_IMGSIZE, size); >> - session_id = prandom_u32() & BHI_TXDB_SEQNUM_BMSK; >> mhi_write_reg(mhi_cntrl, base, BHI_IMGTXDB, session_id); >> read_unlock_bh(pm_lock); >> diff --git a/drivers/bus/mhi/core/internal.h >> b/drivers/bus/mhi/core/internal.h >> index 0965ca3..3205a92 100644 >> --- a/drivers/bus/mhi/core/internal.h >> +++ b/drivers/bus/mhi/core/internal.h >> @@ -452,6 +452,8 @@ enum mhi_pm_state { >> #define PRIMARY_CMD_RING 0 >> #define MHI_DEV_WAKE_DB 127 >> #define MHI_MAX_MTU 0xffff >> +#define MHI_RANDOM_U32_NONZERO(bmsk) ((prandom_u32_max(U32_MAX - 1) & >> \ >> + (bmsk)) + 1) > > I still think this is broken. I'm sorry for the back and forth. > > So, again if prandom_u32_max returns 0xFF, and bmsk is 0xF, we get 0xF > by the & operation, then we add 1, which makes the result 0x10, which > is outside of the range of bmsk, and is basically 0, assuming the > register doesn't accept values outside of the lower 4 bits. > > I think the solution should be: > prandom_u32_max(bmsk) + 1 > > If we treat bmsk like a ordinary value (say 7), then prandom_u32_max > will return a value from 0-6. Then by adding 1, we shift that range > to 1-7, which I think is exactly what we want. > > Now, this assumes that bmsk is a contiguous mask of bits from bit 0 to > N. IE 0xFF and 0x4F are valid, but 0xFB is not. Do you think that is > a valid assumption? I was under the impression that prandom_u32_max will return a value between 0 to whatever is supplied (in your example 7) and not 6. I noticed the description has the round bracket to indicate that it is excluded. If there is no need to do a bmsk - 1 then what you said makes sense. Main thing is to not go above the mask and to get a random non-zero value which fits within the mask.