| Message ID | 1614247980-1776-1-git-send-email-loic.poulain@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mhi: Fix invalid error returning in mhi_queue | expand |
On 2/25/2021 3:13 AM, Loic Poulain wrote: > mhi_queue returns an error when the doorbell is not accessible in > the current state. This can happen when the device is in non M0 > state, like M3, and needs to be waken-up prior ringing the DB. this > case is manager earlier by triggering an asynchronous M3 exit via "This case is managed"? > controller resume/suspend callbacks, that in turn will cause M0 > transition and DB update. > > So, since it's not an error but just delaying of doorbell update do not > return an error. > > That also fix a use after free error for skb case, indeed a caller "This also fixes"? > queuing skb will try to free the skb if the queueing fails, but in > that case queueing has been done. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> Is Fixes appropriate since you mention fixing a use after free? > --- > drivers/bus/mhi/core/main.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > index 7fc2482..c780234 100644 > --- a/drivers/bus/mhi/core/main.c > +++ b/drivers/bus/mhi/core/main.c > @@ -1031,12 +1031,8 @@ static int mhi_queue(struct mhi_device *mhi_dev, struct mhi_buf_info *buf_info, > if (mhi_chan->dir == DMA_TO_DEVICE) > atomic_inc(&mhi_cntrl->pending_pkts); > > - if (unlikely(!MHI_DB_ACCESS_VALID(mhi_cntrl))) { > - ret = -EIO; > - goto exit_unlock; > - } > - > - mhi_ring_chan_db(mhi_cntrl, mhi_chan); > + if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl))) > + mhi_ring_chan_db(mhi_cntrl, mhi_chan); > > exit_unlock: > read_unlock_irqrestore(&mhi_cntrl->pm_lock, flags); > Seems good to me. Feel free to add the below on the next revision - Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
On 2021-02-25 08:18 AM, Jeffrey Hugo wrote: > On 2/25/2021 3:13 AM, Loic Poulain wrote: >> mhi_queue returns an error when the doorbell is not accessible in >> the current state. This can happen when the device is in non M0 >> state, like M3, and needs to be waken-up prior ringing the DB. this >> case is manager earlier by triggering an asynchronous M3 exit via > > "This case is managed"? > >> controller resume/suspend callbacks, that in turn will cause M0 >> transition and DB update. >> >> So, since it's not an error but just delaying of doorbell update do >> not >> return an error. >> >> That also fix a use after free error for skb case, indeed a caller > > "This also fixes"? > >> queuing skb will try to free the skb if the queueing fails, but in >> that case queueing has been done. >> >> Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > Is Fixes appropriate since you mention fixing a use after free? > >> --- >> drivers/bus/mhi/core/main.c | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c >> index 7fc2482..c780234 100644 >> --- a/drivers/bus/mhi/core/main.c >> +++ b/drivers/bus/mhi/core/main.c >> @@ -1031,12 +1031,8 @@ static int mhi_queue(struct mhi_device >> *mhi_dev, struct mhi_buf_info *buf_info, >> if (mhi_chan->dir == DMA_TO_DEVICE) >> atomic_inc(&mhi_cntrl->pending_pkts); >> - if (unlikely(!MHI_DB_ACCESS_VALID(mhi_cntrl))) { >> - ret = -EIO; >> - goto exit_unlock; >> - } >> - >> - mhi_ring_chan_db(mhi_cntrl, mhi_chan); >> + if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl))) >> + mhi_ring_chan_db(mhi_cntrl, mhi_chan); >> exit_unlock: >> read_unlock_irqrestore(&mhi_cntrl->pm_lock, flags); >> > > Seems good to me. Feel free to add the below on the next revision - > Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org> Agree with Jeff. Just as a suggestion, I recommend running the commit text through grammarly or any such service once before posting it to avoid back and forth over it. Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org> Thanks, Bhaumik --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index 7fc2482..c780234 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -1031,12 +1031,8 @@ static int mhi_queue(struct mhi_device *mhi_dev, struct mhi_buf_info *buf_info, if (mhi_chan->dir == DMA_TO_DEVICE) atomic_inc(&mhi_cntrl->pending_pkts); - if (unlikely(!MHI_DB_ACCESS_VALID(mhi_cntrl))) { - ret = -EIO; - goto exit_unlock; - } - - mhi_ring_chan_db(mhi_cntrl, mhi_chan); + if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl))) + mhi_ring_chan_db(mhi_cntrl, mhi_chan); exit_unlock: read_unlock_irqrestore(&mhi_cntrl->pm_lock, flags);
mhi_queue returns an error when the doorbell is not accessible in the current state. This can happen when the device is in non M0 state, like M3, and needs to be waken-up prior ringing the DB. this case is manager earlier by triggering an asynchronous M3 exit via controller resume/suspend callbacks, that in turn will cause M0 transition and DB update. So, since it's not an error but just delaying of doorbell update do not return an error. That also fix a use after free error for skb case, indeed a caller queuing skb will try to free the skb if the queueing fails, but in that case queueing has been done. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- drivers/bus/mhi/core/main.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)