Skip to contentSkip to conversation

dmaengine: introduce is_slave_xfer helper

PATCH Superseded Andy Shevchenko 6 participants 24 messages Nov 16, 2012

Subsystems (3)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover dmaengine: introduce is_slave_xfer helper

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(-)
Signature · 1 line
1.7.10.4
#1 lore raw

PATCH 1/6 dmaengine: introduce is_slave_xfer function

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

Cc Nicolas Ferre <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Guennadi Liakhovetski <[email protected]>
Cc Linus Walleij <[email protected]>
---
 include/linux/dmaengine.h |    5 +++++
 1 file changed, 5 insertions(+)
include/linux/dmaengine.h +5 -0
index 6402b18..3e1a9026 100644
@@ -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)
Signature · 1 line
1.7.10.4
Reviewed-by on v1 1/6
#2 lore raw

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

Quoted 17 lines · Andy Shevchenko

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

Cc Nicolas Ferre <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Guennadi Liakhovetski <[email protected]>
Cc Linus Walleij <[email protected]>
---
 include/linux/dmaengine.h |    5 +++++
 1 file changed, 5 insertions(+)
include/linux/dmaengine.h +0 -0
index 6402b18..3e1a9026 100644

include/linux/dmaengine.h static inline int dmaengine_slave_config(struct dma_chan *chan, · line 624

Quoted 7 lines
include/linux/dmaengine.h +4 -0
@@ -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);
+}

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

#3 lore raw

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

Quoted 22 lines · Viresh Kumar
Quoted 20 lines · Andy Shevchenko

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

Cc Nicolas Ferre <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Guennadi Liakhovetski <[email protected]>
Cc Linus Walleij <[email protected]>
---
 include/linux/dmaengine.h |    5 +++++
 1 file changed, 5 insertions(+)
include/linux/dmaengine.h +0 -0
index 6402b18..3e1a9026 100644
@@ -618,6 +618,11 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
                        (unsigned long)config);
 }
 
Quoted 8 lines

include/linux/dmaengine.h static inline int dmaengine_slave_config(struct dma_chan *chan, · line 624

Quoted 4 lines
include/linux/dmaengine.h +4 -0
+static inline bool is_slave_xfer(enum dma_transfer_direction direction)
+{
+       return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
+}

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.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#4 lore raw

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

In reply to Andy Shevchenko
Quoted 17 lines · Andy Shevchenko

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

Cc Nicolas Ferre <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Guennadi Liakhovetski <[email protected]>
Cc Linus Walleij <[email protected]>
---
 include/linux/dmaengine.h |    5 +++++
 1 file changed, 5 insertions(+)
include/linux/dmaengine.h +0 -0
index 6402b18..3e1a9026 100644

include/linux/dmaengine.h static inline int dmaengine_slave_config(struct dma_chan *chan, · line 624

Quoted 7 lines
include/linux/dmaengine.h +4 -0
@@ -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.

#5 lore raw

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

Quoted 21 lines · Mika Westerberg
Quoted 20 lines · Andy Shevchenko

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

Cc Nicolas Ferre <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Guennadi Liakhovetski <[email protected]>
Cc Linus Walleij <[email protected]>
---
 include/linux/dmaengine.h |    5 +++++
 1 file changed, 5 insertions(+)
include/linux/dmaengine.h +0 -0
index 6402b18..3e1a9026 100644
@@ -618,6 +618,11 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
                      (unsigned long)config);
 }
 
Quoted 7 lines

include/linux/dmaengine.h static inline int dmaengine_slave_config(struct dma_chan *chan, · line 624

Quoted 4 lines
include/linux/dmaengine.h +4 -0
+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.

Quoted 2 lines

Otherwise looks fine to me.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
Reviewed-by
#6 lore raw

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

Quoted 6 lines · Andy Shevchenko
Quoted 2 lines

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.

#7 lore raw

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

In reply to 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.

---
 drivers/dma/at_hdmac.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/dma/at_hdmac.c +5 -5
index 13a02f4..997ae54 100644
@@ -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 */
Signature · 1 line
1.7.10.4
Acked-by on v1 2/6
#9 lore raw

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

In reply to Andy Shevchenko

On 11/16/2012 02:59 PM, Andy Shevchenko :

Quoted 6 lines

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.

Whatever name of function is:

Quoted 46 lines
---
 drivers/dma/at_hdmac.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/dma/at_hdmac.c +5 -5
index 13a02f4..997ae54 100644
@@ -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 */
Signature · 1 line
Nicolas Ferre
#10 lore raw

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

In reply to 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.

---
 drivers/dma/dw_dmac.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/dma/dw_dmac.c +3 -2
index a4a5c80..5e2c4dc 100644
@@ -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);
 
Signature · 1 line
1.7.10.4
#11 lore raw

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

Quoted 3 lines · 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.

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 ?

Signature · 1 line
balbi
#12 lore raw

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

Quoted 8 lines · Felipe Balbi
Quoted 3 lines · 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.

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.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#13 lore raw

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

Hi,

Quoted 10 lines · Andy Shevchenko
Quoted 8 lines · Felipe Balbi
Quoted 3 lines · 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.

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 ?

Quoted 8 lines

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

Signature · 1 line
balbi
#15 lore raw

PATCH 4/6 dma: ep93xx_dma: reuse is_slave_xfer helper

In reply to Andy Shevchenko
---
 drivers/dma/ep93xx_dma.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/dma/ep93xx_dma.c +1 -2
index bcfde40..7c26354 100644
@@ -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:
Signature · 1 line
1.7.10.4
Acked-by on v1 4/6
#17 lore raw

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

In reply to Andy Shevchenko
Quoted 2 lines · Andy Shevchenko
Quoted 20 lines
---
 drivers/dma/ep93xx_dma.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/dma/ep93xx_dma.c +1 -2
index bcfde40..7c26354 100644
@@ -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:
Signature · 1 line
1.7.10.4
#18 lore raw

PATCH 5/6 dma: ipu_idmac: reuse is_slave_xfer helper

In reply to Andy Shevchenko
Cc Guennadi Liakhovetski <[email protected]>
---
 drivers/dma/ipu/ipu_idmac.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/dma/ipu/ipu_idmac.c +1 -1
index 6585537..c8d0254 100644
@@ -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;
 	}
Signature · 1 line
1.7.10.4
#19 lore raw

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

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

-ENOCOMMITLOG ??

Quoted 27 lines
Cc Guennadi Liakhovetski <[email protected]>
---
 drivers/dma/ipu/ipu_idmac.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/dma/ipu/ipu_idmac.c +1 -1
index 6585537..c8d0254 100644
@@ -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;
 	}
Signature · 7 lines
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Signature · 1 line
balbi
#20 lore raw

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

Quoted 3 lines · Felipe Balbi

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.

Quoted 31 lines
Quoted 23 lines
Cc Guennadi Liakhovetski <[email protected]>
---
 drivers/dma/ipu/ipu_idmac.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/dma/ipu/ipu_idmac.c +1 -2
index 6585537..c8d0254 100644
@@ -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

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
balbi

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#22 lore raw

PATCH 6/6 dma: ste_dma40: reuse is_slave_xfer helper

In reply to Andy Shevchenko
---
 drivers/dma/ste_dma40.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/dma/ste_dma40.c +1 -1
index 23c5573..9f4fdf4 100644
@@ -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);
Signature · 1 line
1.7.10.4