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

[Miquel Raynal]

Hi Miaoqian,

linmq006@xxxxxxxxx wrote on Tue, 12 Apr 2022 16:23:24 +0800:

> 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)

  if (cond && cond && !dma_fifo_transfer())

> 	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  


Thanks,
Miquèl