| Message ID | 20220412063703.8537-1-linmq006@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] mtd: rawnand: Fix return value check of wait_for_completion_timeout | expand |
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. This function is static and only called twice, please turn it into something like: if (dma_fifo_transfer()) error else ok > + 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
Hi Miquel, Thanks for your reply. On 2022/4/12 15:06, 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. 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; 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
Hi Miaoqian, linmq006@gmail.com wrote on Tue, 12 Apr 2022 15:42:02 +0800: > Hi Miquel, > > Thanks for your reply. > > On 2022/4/12 15:06, 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). And use: if (dma_fifo_transfer()) error(); > > 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
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) 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
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
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; + 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:
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(-)