Messages in this thread Patch in this message | | | Date | Tue, 12 Apr 2022 10:28:32 +0200 | From | Miquel Raynal <> | Subject | Re: [PATCH v2] mtd: rawnand: Fix return value check of wait_for_completion_timeout |
| |
Hi Miaoqian,
linmq006@gmail.com wrote on Tue, 12 Apr 2022 16:23:24 +0800:
> Hi Miquel, > > On 2022/4/12 15:48, Miquel Raynal wrote: > > > >>> Hi Miaoqian, > >>> > >>> linmq006@gmail.com 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@gmail.com> > >>>> --- > >>>> 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
| |