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

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

[Nicholas Mc Guire]

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

Given that CQSPI_TIMEOUT_MS is < INT_MAX the type conversion is actually safe
here but it is cleaner to use proper types.

Patch was compile tested with: socfpga_defconfig (implies
CONFIG_SPI_CADENCE_QUADSPI=y)

Patch is against 4.18-rc5 (localversion-next is next-20180720)

 drivers/mtd/spi-nor/cadence-quadspi.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index d7e10b3..ce5f840 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -622,6 +622,7 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
 	unsigned int remaining = n_tx;
 	unsigned int write_bytes;
 	int ret;
+	unsigned long timeout;
 
 	writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
 	writel(remaining, reg_base + CQSPI_REG_INDIRECTWRBYTES);
@@ -649,10 +650,10 @@ 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) {
+		timeout = wait_for_completion_timeout(&cqspi->transfer_complete,
+						      msecs_to_jiffies
+						      (CQSPI_TIMEOUT_MS));
+		if (!timeout) {
 			dev_err(nor->dev, "Indirect write timeout\n");
 			ret = -ETIMEDOUT;
 			goto failwr;
-- 
2.1.4

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

[Boris Brezillon]

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.

Thanks,

Boris

> ---
> 
> Given that CQSPI_TIMEOUT_MS is < INT_MAX the type conversion is actually safe
> here but it is cleaner to use proper types.
> 
> Patch was compile tested with: socfpga_defconfig (implies
> CONFIG_SPI_CADENCE_QUADSPI=y)
> 
> Patch is against 4.18-rc5 (localversion-next is next-20180720)
> 
>  drivers/mtd/spi-nor/cadence-quadspi.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
> index d7e10b3..ce5f840 100644
> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> @@ -622,6 +622,7 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
>  	unsigned int remaining = n_tx;
>  	unsigned int write_bytes;
>  	int ret;
> +	unsigned long timeout;
>  
>  	writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
>  	writel(remaining, reg_base + CQSPI_REG_INDIRECTWRBYTES);
> @@ -649,10 +650,10 @@ 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) {
> +		timeout = wait_for_completion_timeout(&cqspi->transfer_complete,
> +						      msecs_to_jiffies
> +						      (CQSPI_TIMEOUT_MS));
> +		if (!timeout) {
>  			dev_err(nor->dev, "Indirect write timeout\n");
>  			ret = -ETIMEDOUT;
>  			goto failwr;

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

[Nicholas Mc Guire]

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.

thx!
hofrat 

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

[Boris Brezillon]

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;

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

[Nicholas Mc Guire]

On Thu, Jul 26, 2018 at 10:09:28PM +0200, Boris Brezillon wrote:
> 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.
>

looks good to me - quit a bit simpler than what I did - thanks !

thx!
hofrat
 
> 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;

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

[Boris Brezillon]

On Fri, 27 Jul 2018 05:21:49 +0000
Nicholas Mc Guire <der.herr@hofr.at> wrote:

> On Thu, Jul 26, 2018 at 10:09:28PM +0200, Boris Brezillon wrote:
> > 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.
> >  
> 
> looks good to me - quit a bit simpler than what I did - thanks !
> 
> thx!
> hofrat
>  
> > 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>

Applied this version.

Thanks,

Boris

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