mtd: rawnand: Fix return value check of wait_for_completion_timeout

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 <[email protected]>

---
change in v2:
- initialize ret to 1.

---
 drivers/mtd/nand/raw/sh_flctl.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/mtd/nand/raw/sh_flctl.c +0 -0

index b85b9c6fcc42..2373251f585b 100644

drivers/mtd/nand/raw/sh_flctl.c +2 -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,

Hi Miaoqian,

[email protected] wrote on Tue, 12 Apr 2022 06:36:52 +0000:

Quoted 14 lines

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 <[email protected]>

---
change in v2:
- initialize ret to 1.

---
 drivers/mtd/nand/raw/sh_flctl.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/mtd/nand/raw/sh_flctl.c static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf, · line 387

Quoted 10 lines

drivers/mtd/nand/raw/sh_flctl.c +1 -1

index b85b9c6fcc42..2373251f585b 100644
@@ -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

Quoted 20 lines

drivers/mtd/nand/raw/sh_flctl.c +4 -2

+	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;
 	}
 

drivers/mtd/nand/raw/sh_flctl.c static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf, · line 439

Quoted 1 line

drivers/mtd/nand/raw/sh_flctl.c +0 -0

 out:

Thanks,
Miquèl

Hi Miquel,

Thanks for your reply.

Quoted 18 lines · Miquel Raynal

Hi Miaoqian,

[email protected] wrote on Tue, 12 Apr 2022 06:36:52 +0000:

Quoted 14 lines

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 <[email protected]>

---
change in v2:
- initialize ret to 1.

---
 drivers/mtd/nand/raw/sh_flctl.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Quoted 12 lines

drivers/mtd/nand/raw/sh_flctl.c static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf, · line 387

Quoted 10 lines

drivers/mtd/nand/raw/sh_flctl.c +1 -1

index b85b9c6fcc42..2373251f585b 100644
@@ -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.

Quoted 7 lines

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,

Quoted 24 lines

Quoted 11 lines

drivers/mtd/nand/raw/sh_flctl.c +2 -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,

drivers/mtd/nand/raw/sh_flctl.c static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf, · line 439

Quoted 10 lines

drivers/mtd/nand/raw/sh_flctl.c +2 -1

 				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

Quoted 31 lines

Quoted 19 lines

Hi Miaoqian,

[email protected] wrote on Tue, 12 Apr 2022 06:36:52 +0000:

Quoted 14 lines

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 <[email protected]>

---
change in v2:
- initialize ret to 1.

---
 drivers/mtd/nand/raw/sh_flctl.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Quoted 12 lines

drivers/mtd/nand/raw/sh_flctl.c static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf, · line 387

Quoted 10 lines

drivers/mtd/nand/raw/sh_flctl.c +1 -1

index b85b9c6fcc42..2373251f585b 100644
@@ -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.

Quoted 3 lines

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)

Quoted 17 lines

And for patch v1, it will return 0  if calls wait_for_completion_timeout succeeds.

Quoted 7 lines

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)

Quoted 5 lines

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

Quoted 25 lines

What do you think?

Thanks,

Quoted 20 lines

Quoted 20 lines

drivers/mtd/nand/raw/sh_flctl.c +4 -2

+	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;
 	}
 

Quoted 3 lines

Quoted 3 lines

drivers/mtd/nand/raw/sh_flctl.c static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf, · line 439

Quoted 1 line

drivers/mtd/nand/raw/sh_flctl.c +0 -0

 out:  

Thanks,
Miquèl

Thanks,
Miquèl

Hi Miquel,

Quoted 53 lines · Miquel Raynal

Quoted 31 lines

Quoted 19 lines

Hi Miaoqian,

[email protected] wrote on Tue, 12 Apr 2022 06:36:52 +0000:

Quoted 14 lines

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 <[email protected]>

---
change in v2:
- initialize ret to 1.

---
 drivers/mtd/nand/raw/sh_flctl.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Quoted 12 lines

drivers/mtd/nand/raw/sh_flctl.c static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf, · line 387

Quoted 10 lines

drivers/mtd/nand/raw/sh_flctl.c +1 -1

index b85b9c6fcc42..2373251f585b 100644
@@ -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.

Quoted 3 lines

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)

Quoted 17 lines

And for patch v1, it will return 0  if calls wait_for_completion_timeout succeeds.

Quoted 7 lines

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)

Quoted 7 lines

Quoted 5 lines

            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.

Quoted 4 lines

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,

Quoted 31 lines

Quoted 28 lines

What do you think?

Thanks,

Quoted 23 lines

Quoted 11 lines

drivers/mtd/nand/raw/sh_flctl.c +2 -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,

drivers/mtd/nand/raw/sh_flctl.c static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf, · line 439

Quoted 10 lines

drivers/mtd/nand/raw/sh_flctl.c +2 -1

 				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

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 <[email protected]>