Re: [PATCH 3/6] dma: dw_dmac: check direction properly in dw_dma_cyclic_prep

Re: [PATCH 3/6] dma: dw_dmac: check direction properly in dw_dma_cyclic_prep

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

On Fri, Nov 16, 2012 at 03:59:16PM +0200, Andy Shevchenko wrote:
> dma_transfer_direction is a normal enum. It means we can't usually use the
> values as bit fields. Let's adjust this check and move it above the usage of
> the direction parameter.

why above the direction parameter ? You need to explain why ? Is it just
because 'it looks nicer' or have you found another bug triggered by
that ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/6] dma: ipu_idmac: reuse is_slave_xfer helper

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

On Fri, Nov 16, 2012 at 03:59:18PM +0200, Andy Shevchenko wrote:

-ENOCOMMITLOG ??

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/dma/ipu/ipu_idmac.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/ipu/ipu_idmac.c b/drivers/dma/ipu/ipu_idmac.c
> index 6585537..c8d0254 100644
> --- a/drivers/dma/ipu/ipu_idmac.c
> +++ b/drivers/dma/ipu/ipu_idmac.c
> @@ -1347,7 +1347,7 @@ static struct dma_async_tx_descriptor *idmac_prep_slave_sg(struct dma_chan *chan
>  	    chan->chan_id != IDMAC_IC_7)
>  		return NULL;
>  
> -	if (direction != DMA_DEV_TO_MEM && direction != DMA_MEM_TO_DEV) {
> +	if (!is_slave_xfer(direction)) {
>  		dev_err(chan->device->dev, "Invalid DMA direction %d!\n", direction);
>  		return NULL;
>  	}
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 0/6] dmaengine: introduce is_slave_xfer helper

[Andy Shevchenko]

There is a common practice to distinguish slave type transfers by checking
direction of the channel. This series introduces the check helper and its usage
across the drivers.

Andy Shevchenko (6):
  dmaengine: introduce is_slave_xfer function
  dma: at_hdmac: check direction properly for cyclic transfers
  dma: dw_dmac: check direction properly in dw_dma_cyclic_prep
  dma: ep93xx_dma: reuse is_slave_xfer helper
  dma: ipu_idmac: reuse is_slave_xfer helper
  dma: ste_dma40: reuse is_slave_xfer helper

 drivers/dma/at_hdmac.c      |   10 +++++-----
 drivers/dma/dw_dmac.c       |    5 +++--
 drivers/dma/ep93xx_dma.c    |    3 +--
 drivers/dma/ipu/ipu_idmac.c |    2 +-
 drivers/dma/ste_dma40.c     |    2 +-
 include/linux/dmaengine.h   |    5 +++++
 6 files changed, 16 insertions(+), 11 deletions(-)

-- 
1.7.10.4

[PATCH 1/6] dmaengine: introduce is_slave_xfer function

[Andy Shevchenko]

This function helps to distinguish the slave type of transfer by checking the
direction parameter.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 include/linux/dmaengine.h |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 6402b18..3e1a9026 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -618,6 +618,11 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
 			(unsigned long)config);
 }
 
+static inline bool is_slave_xfer(enum dma_transfer_direction direction)
+{
+	return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
+}
+
 static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_single(
 	struct dma_chan *chan, dma_addr_t buf, size_t len,
 	enum dma_transfer_direction dir, unsigned long flags)
-- 
1.7.10.4

[PATCH 2/6] dma: at_hdmac: check direction properly for cyclic transfers

[Andy Shevchenko]

dma_transfer_direction is a normal enum. It means we can't usually use the
values as bit fields. Let's adjust this check and move it above the usage of
the direction parameter.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/dma/at_hdmac.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
index 13a02f4..997ae54 100644
--- a/drivers/dma/at_hdmac.c
+++ b/drivers/dma/at_hdmac.c
@@ -778,7 +778,7 @@ err:
  */
 static int
 atc_dma_cyclic_check_values(unsigned int reg_width, dma_addr_t buf_addr,
-		size_t period_len, enum dma_transfer_direction direction)
+		size_t period_len)
 {
 	if (period_len > (ATC_BTSIZE_MAX << reg_width))
 		goto err_out;
@@ -786,8 +786,6 @@ atc_dma_cyclic_check_values(unsigned int reg_width, dma_addr_t buf_addr,
 		goto err_out;
 	if (unlikely(buf_addr & ((1 << reg_width) - 1)))
 		goto err_out;
-	if (unlikely(!(direction & (DMA_DEV_TO_MEM | DMA_MEM_TO_DEV))))
-		goto err_out;
 
 	return 0;
 
@@ -886,14 +884,16 @@ atc_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
 		return NULL;
 	}
 
+	if (unlikely(!is_slave_xfer(direction)))
+		goto err_out;
+
 	if (sconfig->direction == DMA_MEM_TO_DEV)
 		reg_width = convert_buswidth(sconfig->dst_addr_width);
 	else
 		reg_width = convert_buswidth(sconfig->src_addr_width);
 
 	/* Check for too big/unaligned periods and unaligned DMA buffer */
-	if (atc_dma_cyclic_check_values(reg_width, buf_addr,
-					period_len, direction))
+	if (atc_dma_cyclic_check_values(reg_width, buf_addr, period_len))
 		goto err_out;
 
 	/* build cyclic linked list */
-- 
1.7.10.4

[PATCH 3/6] dma: dw_dmac: check direction properly in dw_dma_cyclic_prep

[Andy Shevchenko]

dma_transfer_direction is a normal enum. It means we can't usually use the
values as bit fields. Let's adjust this check and move it above the usage of
the direction parameter.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index a4a5c80..5e2c4dc 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1336,6 +1336,9 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 
 	retval = ERR_PTR(-EINVAL);
 
+	if (unlikely(!is_slave_xfer(direction)))
+		goto out_err;
+
 	if (direction == DMA_MEM_TO_DEV)
 		reg_width = __ffs(sconfig->dst_addr_width);
 	else
@@ -1350,8 +1353,6 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 		goto out_err;
 	if (unlikely(buf_addr & ((1 << reg_width) - 1)))
 		goto out_err;
-	if (unlikely(!(direction & (DMA_MEM_TO_DEV | DMA_DEV_TO_MEM))))
-		goto out_err;
 
 	retval = ERR_PTR(-ENOMEM);
 
-- 
1.7.10.4

[PATCH 4/6] dma: ep93xx_dma: reuse is_slave_xfer helper

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/dma/ep93xx_dma.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
index bcfde40..7c26354 100644
--- a/drivers/dma/ep93xx_dma.c
+++ b/drivers/dma/ep93xx_dma.c
@@ -903,8 +903,7 @@ static int ep93xx_dma_alloc_chan_resources(struct dma_chan *chan)
 			switch (data->port) {
 			case EP93XX_DMA_SSP:
 			case EP93XX_DMA_IDE:
-				if (data->direction != DMA_MEM_TO_DEV &&
-				    data->direction != DMA_DEV_TO_MEM)
+				if (!is_slave_xfter(data->direction))
 					return -EINVAL;
 				break;
 			default:
-- 
1.7.10.4

[PATCH 5/6] dma: ipu_idmac: reuse is_slave_xfer helper

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/dma/ipu/ipu_idmac.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/ipu/ipu_idmac.c b/drivers/dma/ipu/ipu_idmac.c
index 6585537..c8d0254 100644
--- a/drivers/dma/ipu/ipu_idmac.c
+++ b/drivers/dma/ipu/ipu_idmac.c
@@ -1347,7 +1347,7 @@ static struct dma_async_tx_descriptor *idmac_prep_slave_sg(struct dma_chan *chan
 	    chan->chan_id != IDMAC_IC_7)
 		return NULL;
 
-	if (direction != DMA_DEV_TO_MEM && direction != DMA_MEM_TO_DEV) {
+	if (!is_slave_xfer(direction)) {
 		dev_err(chan->device->dev, "Invalid DMA direction %d!\n", direction);
 		return NULL;
 	}
-- 
1.7.10.4

[PATCH 6/6] dma: ste_dma40: reuse is_slave_xfer helper

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/dma/ste_dma40.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
index 23c5573..9f4fdf4 100644
--- a/drivers/dma/ste_dma40.c
+++ b/drivers/dma/ste_dma40.c
@@ -2337,7 +2337,7 @@ static struct dma_async_tx_descriptor *d40_prep_slave_sg(struct dma_chan *chan,
 							 unsigned long dma_flags,
 							 void *context)
 {
-	if (direction != DMA_DEV_TO_MEM && direction != DMA_MEM_TO_DEV)
+	if (!is_slave_xfer(direction))
 		return NULL;
 
 	return d40_prep_sg(chan, sgl, sgl, sg_len, direction, dma_flags);
-- 
1.7.10.4

Re: [PATCH 1/6] dmaengine: introduce is_slave_xfer function

[Viresh Kumar]

On 16 November 2012 19:29, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> This function helps to distinguish the slave type of transfer by checking the
> direction parameter.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> ---
>  include/linux/dmaengine.h |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 6402b18..3e1a9026 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -618,6 +618,11 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
>                         (unsigned long)config);
>  }
>
> +static inline bool is_slave_xfer(enum dma_transfer_direction direction)
> +{
> +       return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
> +}

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

Though i still have vote to include DMA_DEV_TO_DEV in thist list :)

Re: [PATCH 2/6] dma: at_hdmac: check direction properly for cyclic transfers

[Viresh Kumar]

On 16 November 2012 19:29, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> dma_transfer_direction is a normal enum. It means we can't usually use the
> values as bit fields. Let's adjust this check and move it above the usage of
> the direction parameter.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 3/6] dma: dw_dmac: check direction properly in dw_dma_cyclic_prep

[Viresh Kumar]

On 16 November 2012 19:29, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> dma_transfer_direction is a normal enum. It means we can't usually use the
> values as bit fields. Let's adjust this check and move it above the usage of
> the direction parameter.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 4/6] dma: ep93xx_dma: reuse is_slave_xfer helper

[Viresh Kumar]

On 16 November 2012 19:29, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 5/6] dma: ipu_idmac: reuse is_slave_xfer helper

[Viresh Kumar]

On 16 November 2012 19:29, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Apart from missing commit log:

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 6/6] dma: ste_dma40: reuse is_slave_xfer helper

[Viresh Kumar]

On 16 November 2012 19:29, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>

Missing log :)

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 1/6] dmaengine: introduce is_slave_xfer function

[Mika Westerberg]

On Fri, Nov 16, 2012 at 03:59:14PM +0200, Andy Shevchenko wrote:
> This function helps to distinguish the slave type of transfer by checking the
> direction parameter.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> ---
>  include/linux/dmaengine.h |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 6402b18..3e1a9026 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -618,6 +618,11 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
>  			(unsigned long)config);
>  }
>  
> +static inline bool is_slave_xfer(enum dma_transfer_direction direction)
> +{
> +	return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
> +}

I wonder if the above function need some prefix like
dmaengine_is_slave_xfer()?

Otherwise looks fine to me.

Re: [PATCH 1/6] dmaengine: introduce is_slave_xfer function

[Andy Shevchenko]

On Fri, Nov 16, 2012 at 4:21 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, Nov 16, 2012 at 03:59:14PM +0200, Andy Shevchenko wrote:
>> This function helps to distinguish the slave type of transfer by checking the
>> direction parameter.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>  include/linux/dmaengine.h |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index 6402b18..3e1a9026 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -618,6 +618,11 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
>>                       (unsigned long)config);
>>  }
>>
>> +static inline bool is_slave_xfer(enum dma_transfer_direction direction)
>> +{
>> +     return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
>> +}
>
> I wonder if the above function need some prefix like
> dmaengine_is_slave_xfer()?

is_dma_slave_xfer seems more consistent with other helper functions.

But is_slave_xfer is vacant anyway. I like shorter names.

>
> Otherwise looks fine to me.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] dmaengine: introduce is_slave_xfer function

[Andy Shevchenko]

On Fri, Nov 16, 2012 at 4:03 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 16 November 2012 19:29, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> This function helps to distinguish the slave type of transfer by checking the
>> direction parameter.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>  include/linux/dmaengine.h |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index 6402b18..3e1a9026 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -618,6 +618,11 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
>>                         (unsigned long)config);
>>  }
>>
>> +static inline bool is_slave_xfer(enum dma_transfer_direction direction)
>> +{
>> +       return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
>> +}
>
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> Though i still have vote to include DMA_DEV_TO_DEV in thist list :)

I propose to do this later when we have a real user of it. Otherwise
it requires to do additional (sometimes useless) checks.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/6] dma: dw_dmac: check direction properly in dw_dma_cyclic_prep

[Andy Shevchenko]

On Fri, Nov 16, 2012 at 3:56 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Fri, Nov 16, 2012 at 03:59:16PM +0200, Andy Shevchenko wrote:
>> dma_transfer_direction is a normal enum. It means we can't usually use the
>> values as bit fields. Let's adjust this check and move it above the usage of
>> the direction parameter.
>
> why above the direction parameter ? You need to explain why ? Is it just
> because 'it looks nicer' or have you found another bug triggered by
> that ?

Many of current users do something like this:

if (dir == MEM_TO_DEV)
 { ... }
*else*
 { ... }

that potentially could trigger the bug.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/6] dma: ipu_idmac: reuse is_slave_xfer helper

[Andy Shevchenko]

On Fri, Nov 16, 2012 at 3:56 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Fri, Nov 16, 2012 at 03:59:18PM +0200, Andy Shevchenko wrote:
>
> -ENOCOMMITLOG ??

I don't see much sense to add anything here, but I will do for the patch v2.

>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> ---
>>  drivers/dma/ipu/ipu_idmac.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/ipu/ipu_idmac.c b/drivers/dma/ipu/ipu_idmac.c
>> index 6585537..c8d0254 100644
>> --- a/drivers/dma/ipu/ipu_idmac.c
>> +++ b/drivers/dma/ipu/ipu_idmac.c
>> @@ -1347,7 +1347,7 @@ static struct dma_async_tx_descriptor *idmac_prep_slave_sg(struct dma_chan *chan
>>           chan->chan_id != IDMAC_IC_7)
>>               return NULL;
>>
>> -     if (direction != DMA_DEV_TO_MEM && direction != DMA_MEM_TO_DEV) {
>> +     if (!is_slave_xfer(direction)) {
>>               dev_err(chan->device->dev, "Invalid DMA direction %d!\n", direction);
>>               return NULL;
>>       }
>> --
>> 1.7.10.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
> --
> balbi



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] dmaengine: introduce is_slave_xfer function

[Mika Westerberg]

On Fri, Nov 16, 2012 at 05:26:07PM +0200, Andy Shevchenko wrote:
> > I wonder if the above function need some prefix like
> > dmaengine_is_slave_xfer()?
> 
> is_dma_slave_xfer seems more consistent with other helper functions.
> 
> But is_slave_xfer is vacant anyway. I like shorter names.

Fair enough.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 4/6] dma: ep93xx_dma: reuse is_slave_xfer helper

[Mika Westerberg]

On Fri, Nov 16, 2012 at 03:59:17PM +0200, Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

> ---
>  drivers/dma/ep93xx_dma.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
> index bcfde40..7c26354 100644
> --- a/drivers/dma/ep93xx_dma.c
> +++ b/drivers/dma/ep93xx_dma.c
> @@ -903,8 +903,7 @@ static int ep93xx_dma_alloc_chan_resources(struct dma_chan *chan)
>  			switch (data->port) {
>  			case EP93XX_DMA_SSP:
>  			case EP93XX_DMA_IDE:
> -				if (data->direction != DMA_MEM_TO_DEV &&
> -				    data->direction != DMA_DEV_TO_MEM)
> +				if (!is_slave_xfter(data->direction))
>  					return -EINVAL;
>  				break;
>  			default:
> -- 
> 1.7.10.4

Re: [PATCH 2/6] dma: at_hdmac: check direction properly for cyclic transfers

[Nicolas Ferre]

On 11/16/2012 02:59 PM, Andy Shevchenko :
> dma_transfer_direction is a normal enum. It means we can't usually use the
> values as bit fields. Let's adjust this check and move it above the usage of
> the direction parameter.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>

Whatever name of function is:

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

> ---
>  drivers/dma/at_hdmac.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index 13a02f4..997ae54 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -778,7 +778,7 @@ err:
>   */
>  static int
>  atc_dma_cyclic_check_values(unsigned int reg_width, dma_addr_t buf_addr,
> -		size_t period_len, enum dma_transfer_direction direction)
> +		size_t period_len)
>  {
>  	if (period_len > (ATC_BTSIZE_MAX << reg_width))
>  		goto err_out;
> @@ -786,8 +786,6 @@ atc_dma_cyclic_check_values(unsigned int reg_width, dma_addr_t buf_addr,
>  		goto err_out;
>  	if (unlikely(buf_addr & ((1 << reg_width) - 1)))
>  		goto err_out;
> -	if (unlikely(!(direction & (DMA_DEV_TO_MEM | DMA_MEM_TO_DEV))))
> -		goto err_out;
>  
>  	return 0;
>  
> @@ -886,14 +884,16 @@ atc_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
>  		return NULL;
>  	}
>  
> +	if (unlikely(!is_slave_xfer(direction)))
> +		goto err_out;
> +
>  	if (sconfig->direction == DMA_MEM_TO_DEV)
>  		reg_width = convert_buswidth(sconfig->dst_addr_width);
>  	else
>  		reg_width = convert_buswidth(sconfig->src_addr_width);
>  
>  	/* Check for too big/unaligned periods and unaligned DMA buffer */
> -	if (atc_dma_cyclic_check_values(reg_width, buf_addr,
> -					period_len, direction))
> +	if (atc_dma_cyclic_check_values(reg_width, buf_addr, period_len))
>  		goto err_out;
>  
>  	/* build cyclic linked list */
> 


-- 
Nicolas Ferre

Re: [PATCH 3/6] dma: dw_dmac: check direction properly in dw_dma_cyclic_prep

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

Hi,

On Fri, Nov 16, 2012 at 05:30:21PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 16, 2012 at 3:56 PM, Felipe Balbi <balbi@ti.com> wrote:
> > On Fri, Nov 16, 2012 at 03:59:16PM +0200, Andy Shevchenko wrote:
> >> dma_transfer_direction is a normal enum. It means we can't usually use the
> >> values as bit fields. Let's adjust this check and move it above the usage of
> >> the direction parameter.
> >
> > why above the direction parameter ? You need to explain why ? Is it just
> > because 'it looks nicer' or have you found another bug triggered by
> > that ?
> 
> Many of current users do something like this:
> 
> if (dir == MEM_TO_DEV)
>  { ... }
> *else*
>  { ... }
> 
> that potentially could trigger the bug.

looks like this deserves to go into commit log ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]