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

[Bhaumik Bhatt <bbhatt@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