Re: [PATCH] mtd: spi-nor: cadence-quadspi: make return type fit wait_for_completion_timeout

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Wed, 25 Jul 2018 06:31:37 +0000
Nicholas Mc Guire <der.herr@hofr.at> wrote:

> On Tue, Jul 24, 2018 at 10:46:26PM +0200, Boris Brezillon wrote:
> > On Sat, 21 Jul 2018 18:08:13 +0200
> > Nicholas Mc Guire <hofrat@osadl.org> wrote:
> >   
> > > wait_for_completion_timeout returns an unsigned long not int. declare a
> > > suitably type timeout and fix up assignment and check.
> > > 
> > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > > Reported-by: Vignesh R <vigneshr@ti.com>
> > > Fixes: 140623410536 ("mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller")  
> > 
> > If you don't mind, I'd like to squash all wait_for_completion_timeout()
> > fixes into a single commit.
> >  
> makes sense - no objections to that at all.


I also dropped the timeout vars and checked the return of
wait_for_completion_timeout() directly + removed the Fixes tag since
it's no longer possible to attach the fix to a single commit and also,
I'm sure the bug really existed (no overflow possible since the timeout
values are small enough to fit in a signed int).

Here is the resulting patch. Let me know if you're okay with this version.

Thanks,

Boris

--->8---
From 69edac4ef4d1c8dac519b6d12afd4097ce72a54e Mon Sep 17 00:00:00 2001
From: Nicholas Mc Guire <hofrat@osadl.org>
Date: Sat, 21 Jul 2018 16:21:51 +0200
Subject: [PATCH] mtd: spi-nor: cadence-quadspi: fix timeout handling

wait_for_completion_timeout returns an unsigned long not an int, so
let's check its return value directly instead of storing it in ret,
and avoid checking for negative values since this cannot happen.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/mtd/spi-nor/cadence-quadspi.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index c3f7aaa5d18f..7a19dae717fa 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -525,15 +525,14 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
 	       reg_base + CQSPI_REG_INDIRECTRD);
 
 	while (remaining > 0) {
-		ret = wait_for_completion_timeout(&cqspi->transfer_complete,
-						  msecs_to_jiffies
-						  (CQSPI_READ_TIMEOUT_MS));
+		if (!wait_for_completion_timeout(&cqspi->transfer_complete,
+				msecs_to_jiffies(CQSPI_READ_TIMEOUT_MS)))
+			ret = -ETIMEDOUT;
 
 		bytes_to_read = cqspi_get_rd_sram_level(cqspi);
 
-		if (!ret && bytes_to_read == 0) {
+		if (ret && bytes_to_read == 0) {
 			dev_err(nor->dev, "Indirect read timeout, no bytes\n");
-			ret = -ETIMEDOUT;
 			goto failrd;
 		}
 
@@ -649,10 +648,8 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
 		iowrite32_rep(cqspi->ahb_base, txbuf,
 			      DIV_ROUND_UP(write_bytes, 4));
 
-		ret = wait_for_completion_timeout(&cqspi->transfer_complete,
-						  msecs_to_jiffies
-						  (CQSPI_TIMEOUT_MS));
-		if (!ret) {
+		if (!wait_for_completion_timeout(&cqspi->transfer_complete,
+					msecs_to_jiffies(CQSPI_TIMEOUT_MS))) {
 			dev_err(nor->dev, "Indirect write timeout\n");
 			ret = -ETIMEDOUT;
 			goto failwr;
@@ -986,9 +983,8 @@ static int cqspi_direct_read_execute(struct spi_nor *nor, u_char *buf,
 	}
 
 	dma_async_issue_pending(cqspi->rx_chan);
-	ret = wait_for_completion_timeout(&cqspi->rx_dma_complete,
-					  msecs_to_jiffies(len));
-	if (ret <= 0) {
+	if (!wait_for_completion_timeout(&cqspi->rx_dma_complete,
+					 msecs_to_jiffies(len))) {
 		dmaengine_terminate_sync(cqspi->rx_chan);
 		dev_err(nor->dev, "DMA wait_for_completion_timeout\n");
 		ret = -ETIMEDOUT;