| Message ID | 1614336782-5809-1-git-send-email-loic.poulain@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0ecc1c70dcd32c0f081b173a1a5d89952686f271 |
| Headers | show |
| Series | [v2] mhi: Fix invalid error returning in mhi_queue | expand |
On Fri, Feb 26, 2021 at 11:53:02AM +0100, 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 managed 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, there > is no reason to return an error. > > This also fixes 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. > > Fixes: a8f75cb348fd ("mhi: core: Factorize mhi queuing") > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org> > Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Thanks, Mani > --- > v2: - Fix/reword commit message > - Add Fixes tag > > 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); > -- > 2.7.4 >
Hi Loic, On 2/26/21 2:53 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 managed 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, there > is no reason to return an error. > > This also fixes 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. > > Fixes: a8f75cb348fd ("mhi: core: Factorize mhi queuing") > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org> > Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > --- > v2: - Fix/reword commit message > - Add Fixes tag > > 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); Caller of all of the ring db APIs mhi_ring_chan/er/cmd_db are checking this if condition "if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)))" every where in the code. Does it make sense to move this check inside the APIs itself, as a clean up. > > exit_unlock: > read_unlock_irqrestore(&mhi_cntrl->pm_lock, flags); > Thanks, Hemant
On Fri, Feb 26, 2021 at 11:53:02AM +0100, 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 managed 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, there > is no reason to return an error. > > This also fixes 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. > > Fixes: a8f75cb348fd ("mhi: core: Factorize mhi queuing") > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org> > Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org> Applied to mhi-next! Thanks, Mani > --- > v2: - Fix/reword commit message > - Add Fixes tag > > 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); > -- > 2.7.4 >
Hello: This patch was applied to qcom/linux.git (refs/heads/for-next): On Fri, 26 Feb 2021 11:53:02 +0100 you 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 managed earlier by triggering an asynchronous M3 exit via > controller resume/suspend callbacks, that in turn will cause M0 > transition and DB update. > > [...] Here is the summary with links: - [v2] mhi: Fix invalid error returning in mhi_queue https://git.kernel.org/qcom/c/0ecc1c70dcd3 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
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 managed 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, there is no reason to return an error. This also fixes 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. Fixes: a8f75cb348fd ("mhi: core: Factorize mhi queuing") Signed-off-by: Loic Poulain <loic.poulain@linaro.org> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org> Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org> --- v2: - Fix/reword commit message - Add Fixes tag drivers/bus/mhi/core/main.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)