[PATCH 1/4] mtd: sh_flctl: let flctl_dma_fifo0_transfer return 0 on success

[PATCH 1/4] mtd: sh_flctl: let flctl_dma_fifo0_transfer return 0 on success

[Nicholas Mc Guire]

wait_for_completion_timeout() returns unsigned long not int so the check
for <= should be == and the type unsigned long. With that fixup the return
value of flctl_dma_fifo0_transfer can be changed to 0 for success. This
changes the failure return value for wait_for_completion_timeout to
-ETIMEDOUT and implicitly 0 on success by initializing ret to 0.
The call-sites are fixed up to check for == 0 rather than > 0 as well.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

V2: As suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
    the expected success return value is 0 so rather than just fixing up
    wait_for_completion_timeout return handling the return value of
    flctl_dma_fifo0_transfer should be adjusted.

call sites:
read_fiforeg,write_ec_fiforeg assume > 0 == success
are fixed up to check for == 0 for success
and the comment in flctl_dma_fifo0_transfe 
        /* ret > 0 is success */
        return ret;
is dropped since returning 0 on success is the expected.

Patch was compile tested with ap325rxa_defconfig (implies
CONFIG_MTD_NAND_SH_FLCTL=y)

Patch is against 4.1-rc1 (localversion-next is -next-20150501)

 drivers/mtd/nand/sh_flctl.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index c3ce81c..9b032dd 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -353,7 +353,8 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf,
 	dma_addr_t dma_addr;
 	dma_cookie_t cookie = -EINVAL;
 	uint32_t reg;
-	int ret;
+	int ret = 0;
+	unsigned long time_left;
 
 	if (dir == DMA_FROM_DEVICE) {
 		chan = flctl->chan_fifo0_rx;
@@ -388,13 +389,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:
@@ -404,7 +406,6 @@ out:
 
 	dma_unmap_single(chan->device->dev, dma_addr, len, dir);
 
-	/* ret > 0 is success */
 	return ret;
 }
 
@@ -428,7 +429,7 @@ static void read_fiforeg(struct sh_flctl *flctl, int rlen, int offset)
 
 	/* initiate DMA transfer */
 	if (flctl->chan_fifo0_rx && rlen >= 32 &&
-		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_DEV_TO_MEM) > 0)
+		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_DEV_TO_MEM) == 0)
 			goto convert;	/* DMA success */
 
 	/* do polling transfer */
@@ -487,7 +488,7 @@ static void write_ec_fiforeg(struct sh_flctl *flctl, int rlen,
 
 	/* initiate DMA transfer */
 	if (flctl->chan_fifo0_tx && rlen >= 32 &&
-		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_MEM_TO_DEV) > 0)
+		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_MEM_TO_DEV) == 0)
 			return;	/* DMA success */
 
 	/* do polling transfer */
-- 
1.7.10.4

[PATCH 2/4] mtd: sh_flctl: drop unused variable

[Nicholas Mc Guire]

shdma_tx_submit() called via dmaengine_submit() returns the assigned 
cookie but this is not used here so the variable and assignment can 
be dropped.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

V2: As pointed out by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
    the variable and assignment can be dropped but not the call to 
    dmaengine_submit(desc) - fixed up 

Patch was compile tested with ap325rxa_defconfig (implies
CONFIG_MTD_NAND_SH_FLCTL=y)

Patch is against 4.1-rc1 (localversion-next is -next-20150501)

 drivers/mtd/nand/sh_flctl.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 9b032dd..d1c46e5 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -351,7 +351,6 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf,
 	struct dma_chan *chan;
 	enum dma_transfer_direction tr_dir;
 	dma_addr_t dma_addr;
-	dma_cookie_t cookie = -EINVAL;
 	uint32_t reg;
 	int ret = 0;
 	unsigned long time_left;
@@ -377,7 +376,7 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf,
 
 		desc->callback = flctl_dma_complete;
 		desc->callback_param = flctl;
-		cookie = dmaengine_submit(desc);
+		dmaengine_submit(desc);
 
 		dma_async_issue_pending(chan);
 	} else {
-- 
1.7.10.4

[PATCH 3/4] mtd: sh_flctl: fix function parameter alignment

[Nicholas Mc Guire]

CodingStyle fix only - align function parameters to opening (.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Patch was compile tested with ap325rxa_defconfig (implies
CONFIG_MTD_NAND_SH_FLCTL=y)

Patch is against 4.1-rc1 (localversion-next is -next-20150501)

 drivers/mtd/nand/sh_flctl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index d1c46e5..ffda530 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -390,7 +390,7 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf,
 
 	time_left =
 	wait_for_completion_timeout(&flctl->dma_complete,
-				msecs_to_jiffies(3000));
+				    msecs_to_jiffies(3000));
 
 	if (time_left == 0) {
 		dmaengine_terminate_all(chan);
-- 
1.7.10.4

[PATCH 4/4] mtd: sh_flctl: fix wrapped condition alignment

[Nicholas Mc Guire]

CodingStyle fix only - align function parameters to opening (.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Patch was compile tested with ap325rxa_defconfig (implies
CONFIG_MTD_NAND_SH_FLCTL=y)

Patch is against 4.1-rc1 (localversion-next is -next-20150501)

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

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index ffda530..2078c4d 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -428,8 +428,8 @@ static void read_fiforeg(struct sh_flctl *flctl, int rlen, int offset)
 
 	/* initiate DMA transfer */
 	if (flctl->chan_fifo0_rx && rlen >= 32 &&
-		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_DEV_TO_MEM) == 0)
-			goto convert;	/* DMA success */
+	    flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_DEV_TO_MEM) == 0)
+		goto convert;	/* DMA success */
 
 	/* do polling transfer */
 	for (i = 0; i < len_4align; i++) {
@@ -487,8 +487,8 @@ static void write_ec_fiforeg(struct sh_flctl *flctl, int rlen,
 
 	/* initiate DMA transfer */
 	if (flctl->chan_fifo0_tx && rlen >= 32 &&
-		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_MEM_TO_DEV) == 0)
-			return;	/* DMA success */
+	    flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_MEM_TO_DEV) == 0)
+		return;	/* DMA success */
 
 	/* do polling transfer */
 	for (i = 0; i < len_4align; i++) {
-- 
1.7.10.4

Re: [PATCH 2/4] mtd: sh_flctl: drop unused variable

[Laurent Pinchart]

Hi Nicholas,

Thank you for the patch.

On Saturday 02 May 2015 09:57:08 Nicholas Mc Guire wrote:
> shdma_tx_submit() called via dmaengine_submit() returns the assigned
> cookie but this is not used here so the variable and assignment can
> be dropped.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>

I would rephrase the commit message to avoid mentioning shdma_tx_submit() as 
that's not relevant. Something like "dmaengine_submit() returns the assigned
cookie but this is not used here so the variable and assignment can be 
dropped."

With that fixed,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> V2: As pointed out by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>     the variable and assignment can be dropped but not the call to
>     dmaengine_submit(desc) - fixed up
> 
> Patch was compile tested with ap325rxa_defconfig (implies
> CONFIG_MTD_NAND_SH_FLCTL=y)
> 
> Patch is against 4.1-rc1 (localversion-next is -next-20150501)
> 
>  drivers/mtd/nand/sh_flctl.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index 9b032dd..d1c46e5 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -351,7 +351,6 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl
> *flctl, unsigned long *buf, struct dma_chan *chan;
>  	enum dma_transfer_direction tr_dir;
>  	dma_addr_t dma_addr;
> -	dma_cookie_t cookie = -EINVAL;
>  	uint32_t reg;
>  	int ret = 0;
>  	unsigned long time_left;
> @@ -377,7 +376,7 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl
> *flctl, unsigned long *buf,
> 
>  		desc->callback = flctl_dma_complete;
>  		desc->callback_param = flctl;
> -		cookie = dmaengine_submit(desc);
> +		dmaengine_submit(desc);
> 
>  		dma_async_issue_pending(chan);
>  	} else {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] mtd: sh_flctl: let flctl_dma_fifo0_transfer return 0 on success

[Laurent Pinchart]

Hi Nicholas,

Thank you for the patch.

On Saturday 02 May 2015 09:57:07 Nicholas Mc Guire wrote:
> wait_for_completion_timeout() returns unsigned long not int so the check
> for <= should be == and the type unsigned long. With that fixup the return
> value of flctl_dma_fifo0_transfer can be changed to 0 for success. This
> changes the failure return value for wait_for_completion_timeout to
> -ETIMEDOUT and implicitly 0 on success by initializing ret to 0.
> The call-sites are fixed up to check for == 0 rather than > 0 as well.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>

I would squash patches 3/4 and 4/4 into this one, there isn't much point in 
having indentation fixes as separate patches when this one already touches the 
same lines of code. Then,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> V2: As suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>     the expected success return value is 0 so rather than just fixing up
>     wait_for_completion_timeout return handling the return value of
>     flctl_dma_fifo0_transfer should be adjusted.
> 
> call sites:
> read_fiforeg,write_ec_fiforeg assume > 0 == success
> are fixed up to check for == 0 for success
> and the comment in flctl_dma_fifo0_transfe
>         /* ret > 0 is success */
>         return ret;
> is dropped since returning 0 on success is the expected.
> 
> Patch was compile tested with ap325rxa_defconfig (implies
> CONFIG_MTD_NAND_SH_FLCTL=y)
> 
> Patch is against 4.1-rc1 (localversion-next is -next-20150501)
> 
>  drivers/mtd/nand/sh_flctl.c |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index c3ce81c..9b032dd 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -353,7 +353,8 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl
> *flctl, unsigned long *buf, dma_addr_t dma_addr;
>  	dma_cookie_t cookie = -EINVAL;
>  	uint32_t reg;
> -	int ret;
> +	int ret = 0;
> +	unsigned long time_left;
> 
>  	if (dir == DMA_FROM_DEVICE) {
>  		chan = flctl->chan_fifo0_rx;
> @@ -388,13 +389,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:
> @@ -404,7 +406,6 @@ out:
> 
>  	dma_unmap_single(chan->device->dev, dma_addr, len, dir);
> 
> -	/* ret > 0 is success */
>  	return ret;
>  }
> 
> @@ -428,7 +429,7 @@ static void read_fiforeg(struct sh_flctl *flctl, int
> rlen, int offset)
> 
>  	/* initiate DMA transfer */
>  	if (flctl->chan_fifo0_rx && rlen >= 32 &&
> -		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_DEV_TO_MEM) > 0)
> +		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_DEV_TO_MEM) == 0)
>  			goto convert;	/* DMA success */
> 
>  	/* do polling transfer */
> @@ -487,7 +488,7 @@ static void write_ec_fiforeg(struct sh_flctl *flctl, int
> rlen,
> 
>  	/* initiate DMA transfer */
>  	if (flctl->chan_fifo0_tx && rlen >= 32 &&
> -		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_MEM_TO_DEV) > 0)
> +		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_MEM_TO_DEV) == 0)
>  			return;	/* DMA success */
> 
>  	/* do polling transfer */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] mtd: sh_flctl: drop unused variable

[Vinod Koul]

On Sun, May 03, 2015 at 10:33:43PM +0300, Laurent Pinchart wrote:
> Hi Nicholas,
> 
> Thank you for the patch.
> 
> On Saturday 02 May 2015 09:57:08 Nicholas Mc Guire wrote:
> > shdma_tx_submit() called via dmaengine_submit() returns the assigned
> > cookie but this is not used here so the variable and assignment can
> > be dropped.
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> 
> I would rephrase the commit message to avoid mentioning shdma_tx_submit() as 
> that's not relevant. Something like "dmaengine_submit() returns the assigned
> cookie but this is not used here so the variable and assignment can be 
> dropped."
And I am bit surrised about taht. Ideally the driver should use the cookie
to check the status later on...

-- 
~Vinod
> 
> With that fixed,
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > ---
> > 
> > V2: As pointed out by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >     the variable and assignment can be dropped but not the call to
> >     dmaengine_submit(desc) - fixed up
> > 
> > Patch was compile tested with ap325rxa_defconfig (implies
> > CONFIG_MTD_NAND_SH_FLCTL=y)
> > 
> > Patch is against 4.1-rc1 (localversion-next is -next-20150501)
> > 
> >  drivers/mtd/nand/sh_flctl.c |    3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> > index 9b032dd..d1c46e5 100644
> > --- a/drivers/mtd/nand/sh_flctl.c
> > +++ b/drivers/mtd/nand/sh_flctl.c
> > @@ -351,7 +351,6 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl
> > *flctl, unsigned long *buf, struct dma_chan *chan;
> >  	enum dma_transfer_direction tr_dir;
> >  	dma_addr_t dma_addr;
> > -	dma_cookie_t cookie = -EINVAL;
> >  	uint32_t reg;
> >  	int ret = 0;
> >  	unsigned long time_left;
> > @@ -377,7 +376,7 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl
> > *flctl, unsigned long *buf,
> > 
> >  		desc->callback = flctl_dma_complete;
> >  		desc->callback_param = flctl;
> > -		cookie = dmaengine_submit(desc);
> > +		dmaengine_submit(desc);
> > 
> >  		dma_async_issue_pending(chan);
> >  	} else {
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 

Re: [PATCH 4/4] mtd: sh_flctl: fix wrapped condition alignment

[Vinod Koul]

On Sat, May 02, 2015 at 09:57:10AM +0200, Nicholas Mc Guire wrote:
> CodingStyle fix only - align function parameters to opening (.
> 
This doesnt look any better to me...

-- 
~Vinod

> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
> 
> Patch was compile tested with ap325rxa_defconfig (implies
> CONFIG_MTD_NAND_SH_FLCTL=y)
> 
> Patch is against 4.1-rc1 (localversion-next is -next-20150501)
> 
>  drivers/mtd/nand/sh_flctl.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index ffda530..2078c4d 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -428,8 +428,8 @@ static void read_fiforeg(struct sh_flctl *flctl, int rlen, int offset)
>  
>  	/* initiate DMA transfer */
>  	if (flctl->chan_fifo0_rx && rlen >= 32 &&
> -		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_DEV_TO_MEM) == 0)
> -			goto convert;	/* DMA success */
> +	    flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_DEV_TO_MEM) == 0)
> +		goto convert;	/* DMA success */
>  
>  	/* do polling transfer */
>  	for (i = 0; i < len_4align; i++) {
> @@ -487,8 +487,8 @@ static void write_ec_fiforeg(struct sh_flctl *flctl, int rlen,
>  
>  	/* initiate DMA transfer */
>  	if (flctl->chan_fifo0_tx && rlen >= 32 &&
> -		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_MEM_TO_DEV) == 0)
> -			return;	/* DMA success */
> +	    flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_MEM_TO_DEV) == 0)
> +		return;	/* DMA success */
>  
>  	/* do polling transfer */
>  	for (i = 0; i < len_4align; i++) {
> -- 
> 1.7.10.4
> 

-- 

Re: [PATCH 4/4] mtd: sh_flctl: fix wrapped condition alignment

[Nicholas Mc Guire]

On Mon, 04 May 2015, Vinod Koul wrote:

> On Sat, May 02, 2015 at 09:57:10AM +0200, Nicholas Mc Guire wrote:
> > CodingStyle fix only - align function parameters to opening (.
> > 
> This doesnt look any better to me...
>
True it makes little difference when looking at these few lines
I guess though the issue is consistency.
 
thx!
hofrat

Re: [PATCH 2/4] mtd: sh_flctl: drop unused variable

[Nicholas Mc Guire]

On Mon, 04 May 2015, Vinod Koul wrote:

> On Sun, May 03, 2015 at 10:33:43PM +0300, Laurent Pinchart wrote:
> > Hi Nicholas,
> > 
> > Thank you for the patch.
> > 
> > On Saturday 02 May 2015 09:57:08 Nicholas Mc Guire wrote:
> > > shdma_tx_submit() called via dmaengine_submit() returns the assigned
> > > cookie but this is not used here so the variable and assignment can
> > > be dropped.
> > > 
> > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > 
> > I would rephrase the commit message to avoid mentioning shdma_tx_submit() as 
> > that's not relevant. Something like "dmaengine_submit() returns the assigned
> > cookie but this is not used here so the variable and assignment can be 
> > dropped."
> And I am bit surrised about taht. Ideally the driver should use the cookie
> to check the status later on...
>
looking at other drivers it seems like the drivers should call 
dma_submit_error(cookie); on the received cookie - which does:
  return cookie < 0 ? cookie : 0;
but doing that after dmaengine_submit() which actually already queued the
this request in shdma_base.cc:shdma_tx_submit() might not be that helpful 
and looking at dma_cookie_assign() I do not see how the condition that 
dma_submit_error is checking for ever could occur as it can't go below 
cookie = DMA_MIN_COOKIE which is defined to 1 (include/linux/dmaengine.h)

As other drivers seem to not be doing more with the returned cookie than 
calling dma_submit_error() on it this seems ok here ...but I'm not that
deep into this - my starting point was a simple API inconsisteny in the
use of wait_for_completion_timeout() :)

thx!
hofrat

Re: [PATCH 4/4] mtd: sh_flctl: fix wrapped condition alignment

[Joe Perches]

On Mon, 2015-05-04 at 10:51 +0530, Vinod Koul wrote:
> On Sat, May 02, 2015 at 09:57:10AM +0200, Nicholas Mc Guire wrote:
> > CodingStyle fix only - align function parameters to opening (.
> > 
> This doesnt look any better to me...

The goto and return statements were overly indented

> > diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
[]
> > @@ -428,8 +428,8 @@ static void read_fiforeg(struct sh_flctl *flctl, int rlen, int offset)
> >  
> >  	/* initiate DMA transfer */
> >  	if (flctl->chan_fifo0_rx && rlen >= 32 &&
> > -		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_DEV_TO_MEM) == 0)
> > -			goto convert;	/* DMA success */
> > +	    flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_DEV_TO_MEM) == 0)
> > +		goto convert;	/* DMA success */
> >  
> >  	/* do polling transfer */
> >  	for (i = 0; i < len_4align; i++) {
> > @@ -487,8 +487,8 @@ static void write_ec_fiforeg(struct sh_flctl *flctl, int rlen,
> >  
> >  	/* initiate DMA transfer */
> >  	if (flctl->chan_fifo0_tx && rlen >= 32 &&
> > -		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_MEM_TO_DEV) == 0)
> > -			return;	/* DMA success */
> > +	    flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_MEM_TO_DEV) == 0)
> > +		return;	/* DMA success */

Re: [PATCH 2/4] mtd: sh_flctl: drop unused variable

[Laurent Pinchart]

Hi Nicholas,

On Monday 04 May 2015 08:03:46 Nicholas Mc Guire wrote:
> On Mon, 04 May 2015, Vinod Koul wrote:
> > On Sun, May 03, 2015 at 10:33:43PM +0300, Laurent Pinchart wrote:
> > > On Saturday 02 May 2015 09:57:08 Nicholas Mc Guire wrote:
> > > > shdma_tx_submit() called via dmaengine_submit() returns the assigned
> > > > cookie but this is not used here so the variable and assignment can
> > > > be dropped.
> > > > 
> > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > > 
> > > I would rephrase the commit message to avoid mentioning
> > > shdma_tx_submit() as that's not relevant. Something like
> > > "dmaengine_submit() returns the assigned cookie but this is not used
> > > here so the variable and assignment can be dropped."
> > 
> > And I am bit surrised about taht. Ideally the driver should use the cookie
> > to check the status later on...
> 
> looking at other drivers it seems like the drivers should call
> dma_submit_error(cookie); on the received cookie - which does:
>   return cookie < 0 ? cookie : 0;
> but doing that after dmaengine_submit() which actually already queued the
> this request in shdma_base.cc:shdma_tx_submit()

Don't take shdma into account. There's no guarantee that the DMA engine will 
be an SH DMAC on all platforms where the flctl driver will be used. 
Furthermore, the shdma implementation might change in the future. You should 
consider the DMA engine API only and comply with its requirements.

> might not be that helpful
> and looking at dma_cookie_assign() I do not see how the condition that
> dma_submit_error is checking for ever could occur as it can't go below
> cookie = DMA_MIN_COOKIE which is defined to 1 (include/linux/dmaengine.h)
> 
> As other drivers seem to not be doing more with the returned cookie than
> calling dma_submit_error() on it this seems ok here ...but I'm not that
> deep into this - my starting point was a simple API inconsisteny in the
> use of wait_for_completion_timeout() :)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] mtd: sh_flctl: drop unused variable

[Nicholas Mc Guire]

On Mon, 04 May 2015, Laurent Pinchart wrote:

> Hi Nicholas,
> 
> On Monday 04 May 2015 08:03:46 Nicholas Mc Guire wrote:
> > On Mon, 04 May 2015, Vinod Koul wrote:
> > > On Sun, May 03, 2015 at 10:33:43PM +0300, Laurent Pinchart wrote:
> > > > On Saturday 02 May 2015 09:57:08 Nicholas Mc Guire wrote:
> > > > > shdma_tx_submit() called via dmaengine_submit() returns the assigned
> > > > > cookie but this is not used here so the variable and assignment can
> > > > > be dropped.
> > > > > 
> > > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > > > 
> > > > I would rephrase the commit message to avoid mentioning
> > > > shdma_tx_submit() as that's not relevant. Something like
> > > > "dmaengine_submit() returns the assigned cookie but this is not used
> > > > here so the variable and assignment can be dropped."
> > > 
> > > And I am bit surrised about taht. Ideally the driver should use the cookie
> > > to check the status later on...
> > 
> > looking at other drivers it seems like the drivers should call
> > dma_submit_error(cookie); on the received cookie - which does:
> >   return cookie < 0 ? cookie : 0;
> > but doing that after dmaengine_submit() which actually already queued the
> > this request in shdma_base.cc:shdma_tx_submit()
> 
> Don't take shdma into account. There's no guarantee that the DMA engine will 
> be an SH DMAC on all platforms where the flctl driver will be used. 
> Furthermore, the shdma implementation might change in the future. You should 
> consider the DMA engine API only and comply with its requirements.
>
ok - trying to find out what the requirements regarding checking actually are
- looking through Documentation/dmaengine/client.txt

   Interface:
        dma_cookie_t dmaengine_submit(struct dma_async_tx_descriptor *desc)     

   This returns a cookie can be used to check the progress of DMA engine
   activity via other DMA engine calls not covered in this document.

the client.txt later points to include/linux/dmaengine.h - so there it seems
that it should be calling dma_submit_error(cookei) in any case and return
error if < 0. basically (as a few other drivers do)

        ret = dma_submit_error(cookie);                                         
        if (ret) {
                dev_err(&flctl->pdev->dev, "dma_submit_error\n");
                return ret;
        }

e.g. like in drivers/crypto/qce/dma.c:qce_dma_prep_sg() - most simply do
        cookie = dmaengine_submit(desc);                                        
        dma_async_issue_pending(channel);
or
        dmaengine_submit(desc);                                        
        dma_async_issue_pending(channel);

so all of these cases would need to be cleaned up ?
provided this is the correct interpretation of the dmaengine interfacer 
requirements this probably is best done with a spatch.

The spatch scanner (which might not yet be exhaustive/correct) used here is:

<snip check_dmaengine_submit_return.cocci>
virtual context
virtual patch
virtual org
virtual report

@has_cookie@
identifier f;
typedef dma_cookie_t;
idexpression dma_cookie_t cookie;
position p;
@@

f(...){
<+...
  cookie = dmaengine_submit@p(...);
  ... when != dma_submit_error(cookie)
  dma_async_issue_pending(...);
...+>
}

@script:python@
p << has_cookie.p;
fn << has_cookie.f;
@@

print "%s:%s:%s WARNING unchecked dmaengine_submit" % (p[0].file,fn,p[0].line)

@no_cookie@
identifier f;
position p,p1;
@@

f@p(...){
<+...
  dmaengine_submit@p1(...);
  ... when != dma_submit_error(...)
  dma_async_issue_pending(...);
...+>
}

@script:python@
p << no_cookie.p;
fn << no_cookie.f;
@@

print "%s:%s:%s WARNING unchecked dmaengine_submit" % (p[0].file,fn,p[0].line)
<snip>

run: make coccicheck COCCI=check_dmaengine_submit_return.cocci MODE=report

The list of cases found is currently 54 cases:

./sound/soc/sh/siu_pcm.c:siu_pcm_rd_set:192 WARNING unchecked dmaengine_submit
./sound/soc/sh/siu_pcm.c:siu_pcm_wr_set:142 WARNING unchecked dmaengine_submit
./drivers/soc/tegra/fuse/fuse-tegra20.c:tegra20_fuse_readl:57 WARNING unchecked dmaengine_submit
./drivers/crypto/omap-aes.c:omap_aes_crypt_dma:416 WARNING unchecked dmaengine_submit
./drivers/crypto/atmel-tdes.c:atmel_tdes_crypt_dma:433 WARNING unchecked dmaengine_submit
./drivers/crypto/ux500/cryp/cryp_core.c:cryp_set_dma_transfer:596 WARNING unchecked dmaengine_submit
./drivers/crypto/ux500/hash/hash_core.c:hash_set_dma_transfer:194 WARNING unchecked dmaengine_submit
./drivers/crypto/omap-sham.c:omap_sham_xmit_dma:552 WARNING unchecked dmaengine_submit
./drivers/crypto/img-hash.c:img_hash_xmit_dma:221 WARNING unchecked dmaengine_submit
./drivers/crypto/atmel-sha.c:atmel_sha_xmit_dma:425 WARNING unchecked dmaengine_submit
./drivers/crypto/atmel-aes.c:atmel_aes_crypt_dma:310 WARNING unchecked dmaengine_submit
./drivers/crypto/omap-des.c:omap_des_crypt_dma:400 WARNING unchecked dmaengine_submit
./drivers/mtd/nand/gpmi-nand/gpmi-nand.c:start_dma_without_bch_irq:445 WARNING unchecked dmaengine_submit
./drivers/mtd/nand/omap2.c:omap_nand_dma_transfer:458 WARNING unchecked dmaengine_submit
./drivers/mtd/nand/lpc32xx_mlc.c:lpc32xx_xmit_dma:389 WARNING unchecked dmaengine_submit
./drivers/mtd/nand/sh_flctl.c:flctl_dma_fifo0_transfer:380 WARNING unchecked dmaengine_submit
./drivers/mtd/nand/lpc32xx_slc.c:lpc32xx_xmit_dma:425 WARNING unchecked dmaengine_submit
./drivers/mmc/host/s3cmci.c:s3cmci_prepare_dma:1086 WARNING unchecked dmaengine_submit
./drivers/mmc/host/mmci.c:mmci_dma_start_data:648 WARNING unchecked dmaengine_submit
./drivers/mmc/host/jz4740_mmc.c:jz4740_mmc_start_dma_transfer:270 WARNING unchecked dmaengine_submit
./drivers/mmc/host/sh_mmcif.c:sh_mmcif_start_dma_rx:311 WARNING unchecked dmaengine_submit
./drivers/mmc/host/sh_mmcif.c:sh_mmcif_start_dma_tx:360 WARNING unchecked dmaengine_submit
./drivers/mmc/host/atmel-mci.c:atmci_submit_data_dma:1088 WARNING unchecked dmaengine_submit
./drivers/mmc/host/moxart-mmc.c:moxart_transfer_dma:257 WARNING unchecked dmaengine_submit
./drivers/mmc/host/mxs-mmc.c:mxs_mmc_ac:293 WARNING unchecked dmaengine_submit
./drivers/mmc/host/mxs-mmc.c:mxs_mmc_adtc:351 WARNING unchecked dmaengine_submit
./drivers/mmc/host/mxs-mmc.c:mxs_mmc_bc:259 WARNING unchecked dmaengine_submit
./drivers/mmc/host/mxcmmc.c:mxcmci_setup_data:301 WARNING unchecked dmaengine_submit
./drivers/mmc/host/davinci_mmc.c:mmc_davinci_send_dma_request:418 WARNING unchecked dmaengine_submit
./drivers/i2c/busses/i2c-mxs.c:mxs_i2c_dma_setup_xfer:177 WARNING unchecked dmaengine_submit
./drivers/i2c/busses/i2c-at91.c:at91_twi_read_data_dma:315 WARNING unchecked dmaengine_submit
./drivers/i2c/busses/i2c-at91.c:at91_twi_write_data_dma:221 WARNING unchecked dmaengine_submit
./drivers/spi/spi-sirf.c:spi_sirfsoc_dma_transfer:337 WARNING unchecked dmaengine_submit
./drivers/spi/spi-imx.c:spi_imx_dma_transfer:893 WARNING unchecked dmaengine_submit
./drivers/spi/spi-img-spfi.c:img_spfi_start_dma:309 WARNING unchecked dmaengine_submit
./drivers/spi/spi-pl022.c:configure_dma:933 WARNING unchecked dmaengine_submit
./drivers/spi/spi-s3c64xx.c:prepare_dma:276 WARNING unchecked dmaengine_submit
./drivers/spi/spi-tegra114.c:tegra_spi_start_rx_dma:454 WARNING unchecked dmaengine_submit
./drivers/spi/spi-tegra114.c:tegra_spi_start_tx_dma:435 WARNING unchecked dmaengine_submit
./drivers/spi/spi-tegra20-slink.c:tegra_slink_start_rx_dma:463 WARNING unchecked dmaengine_submit
./drivers/spi/spi-tegra20-slink.c:tegra_slink_start_tx_dma:444 WARNING unchecked dmaengine_submit
./drivers/spi/spi-dw-mid.c:mid_spi_dma_transfer:248 WARNING unchecked dmaengine_submit
./drivers/spi/spi-mxs.c:mxs_spi_txrx_dma:172 WARNING unchecked dmaengine_submit
./drivers/spi/spi-davinci.c:davinci_spi_bufs:584 WARNING unchecked dmaengine_submit
./drivers/spi/spi-ep93xx.c:ep93xx_spi_dma_transfer:555 WARNING unchecked dmaengine_submit
./drivers/spi/spi-rockchip.c:rockchip_spi_prepare_dma:436 WARNING unchecked dmaengine_submit
./drivers/spi/spi-omap2-mcspi.c:omap2_mcspi_rx_dma:428 WARNING unchecked dmaengine_submit
./drivers/spi/spi-omap2-mcspi.c:omap2_mcspi_tx_dma:390 WARNING unchecked dmaengine_submit
./drivers/ata/sata_dwc_460ex.c:sata_dwc_bmdma_start_by_tag:965 WARNING unchecked dmaengine_submit
./drivers/tty/serial/imx.c:imx_dma_tx:514 WARNING unchecked dmaengine_submit
./drivers/tty/serial/imx.c:start_rx_dma:938 WARNING unchecked dmaengine_submit
./drivers/tty/serial/sirfsoc_uart.c:sirfsoc_uart_tx_with_dma:199 WARNING unchecked dmaengine_submit
./drivers/tty/serial/mxs-auart.c:mxs_auart_dma_prep_rx:551 WARNING unchecked dmaengine_submit
./drivers/tty/serial/mxs-auart.c:mxs_auart_dma_tx:224 WARNING unchecked dmaengine_submit


Pleas let me know if the prposed addition of dma_submit_error(cookie) is 
correct then I'll give it a shot at a clean semantic patch to clean it all up 
at once. 

thx!
hofrat