Re: [PATCH v2] mtd: rawnand: Fix return value check of wait_for_completion_timeout

[Miaoqian Lin]

Hi Miquel,

On 2022/4/12 15:48, Miquel Raynal wrote:
>
>>> Hi Miaoqian,
>>>
>>> linmq006@xxxxxxxxx wrote on Tue, 12 Apr 2022 06:36:52 +0000:
>>>  
>>>> wait_for_completion_timeout() returns unsigned long not int.
>>>> It returns 0 if timed out, and positive if completed.
>>>> The check for <= 0 is ambiguous and should be == 0 here
>>>> indicating timeout which is the only error case.
>>>>
>>>> Fixes: 83738d87e3a0 ("mtd: sh_flctl: Add DMA capabilty")
>>>> Signed-off-by: Miaoqian Lin <linmq006@xxxxxxxxx>
>>>> ---
>>>> change in v2:
>>>> - initialize ret to 1.
>>>> ---
>>>>  drivers/mtd/nand/raw/sh_flctl.c | 8 +++++---
>>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/sh_flctl.c b/drivers/mtd/nand/raw/sh_flctl.c
>>>> index b85b9c6fcc42..2373251f585b 100644
>>>> --- a/drivers/mtd/nand/raw/sh_flctl.c
>>>> +++ b/drivers/mtd/nand/raw/sh_flctl.c
>>>> @@ -384,7 +384,8 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf,
>>>>  	dma_addr_t dma_addr;
>>>>  	dma_cookie_t cookie;
>>>>  	uint32_t reg;
>>>> -	int ret;
>>>> +	int ret = 1;  
>>> Does not look right. I know this function returns > 0 on positive
>>> outcomes but this does not make any sense in the first place.  
>> Yes, I made a mistake, Now I realize that in v2, it will return 1 in error path
>>
>> when DMA submit failed.
> Not 1, but a proper error code please (-ETIMEDOUT, -EINVAL, whatever)
>
>> And for patch v1, it will return 0  if calls wait_for_completion_timeout succeeds.
>>
>>> This function is static and only called twice, please turn it into
>>> something like:
>>>
>>> if (dma_fifo_transfer())
>>> 	error
>>> else
>>> 	ok  
>> So I want to keep ret>0 means success.
>>
>> Or could I set ret > 0 after in wait_for_completion_timeout() success path?
>>
>> like:
>>
>>     if(time_left == 0)
>>
>>             ret = -ETIMEDOUT;
>>
>>     else
>>
>>             ret = 1;
> You can initialize ret to zero at to top. So that anything != 0 is an
> error (like a lot of functions in the kernel). 

Thanks for your advice, I will do this.
> And use:
>
> 	if (dma_fifo_transfer())
> 		error();
I think keeping the original condition structure is better,
something like:

if (dma_fifo_transfer()==0)
	succeed();

In this way, only minor changes is needed——only need to update the symbol in condition.
Otherwise It needs to restructure the code and be more complicated.


Thanks,

>> What do you think?
>>
>>
>> Thanks,
>>
>>>> +	unsigned long time_left;
>>>>  
>>>>  	if (dir == DMA_FROM_DEVICE) {
>>>>  		chan = flctl->chan_fifo0_rx;
>>>> @@ -425,13 +426,14 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf,
>>>>  		goto out;
>>>>  	}
>>>>  
>>>> -	ret =
>>>> +	time_left =
>>>>  	wait_for_completion_timeout(&flctl->dma_complete,
>>>>  				msecs_to_jiffies(3000));
>>>>  
>>>> -	if (ret <= 0) {
>>>> +	if (time_left == 0) {
>>>>  		dmaengine_terminate_all(chan);
>>>>  		dev_err(&flctl->pdev->dev, "wait_for_completion_timeout\n");
>>>> +		ret = -ETIMEDOUT;
>>>>  	}
>>>>  
>>>>  out:  
>>> Thanks,
>>> Miquèl  
>
> Thanks,
> Miquèl