Re: [PATCH] mhi: Fix invalid error returning in mhi_queue

[Jeffrey Hugo <jhugo@codeaurora.org>]

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>

-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.