dw_dmac: check direction properly in dw_dma_cyclic_prep

PATCH In review Andy Shevchenko 3 participants 14 messages Nov 15, 2012 – Nov 16, 2012 dmaengine

Share & tools

Andy Shevchenko

author

2012-11-15 14:21 UTC #0 lore raw

PATCH 1/2 dw_dmac: check direction properly in dw_dma_cyclic_prep

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.

This patch introduces helper function is_slave_direction() that will be used later in other places of the code.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/dma/dw_dmac.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/dma/dw_dmac.c +8 -2

index a4a5c80..771aa15 100644
@@ -36,6 +36,11 @@
  * which does not support descriptor writeback.
  */
 
+static inline bool is_slave_direction(enum dma_transfer_direction direction)
+{
+	return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
+}
+
 static inline unsigned int dwc_get_dms(struct dw_dma_slave *slave)
 {
 	return slave ? slave->dst_master : 0;

@@ -1336,6 +1341,9 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 
 	retval = ERR_PTR(-EINVAL);
 
+	if (unlikely(!is_slave_direction(direction)))
+		goto out_err;
+
 	if (direction == DMA_MEM_TO_DEV)
 		reg_width = __ffs(sconfig->dst_addr_width);
 	else

@@ -1350,8 +1358,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

Andy Shevchenko

author

2012-11-15 14:21 UTC #1 lore raw

PATCH 2/2 dw_dmac: make usage of dw_dma_slave optional

Currently for the slave transfers the driver requires a custom slave configuration to be present. Nevertheless, in some cases we need only the request line as an additional information to the generic slave configuration. The request line is provided by slave_id parameter of the dma_slave_config structure. That's why the custom slave configuration could be optional for such cases.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/dma/dw_dmac.c |   23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

drivers/dma/dw_dmac.c +17 -6

index 771aa15..baebf81 100644
@@ -41,6 +41,11 @@ static inline bool is_slave_direction(enum dma_transfer_direction direction)
 	return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
 }
 
+static inline bool dwc_is_slave(struct dma_slave_config *sconfig)
+{
+	return is_slave_direction(sconfig->direction);
+}
+
 static inline unsigned int dwc_get_dms(struct dw_dma_slave *slave)
 {
 	return slave ? slave->dst_master : 0;

@@ -55,11 +60,12 @@ static inline unsigned int dwc_get_sms(struct dw_dma_slave *slave)
 		struct dw_dma_slave *__slave = (_chan->private);	\
 		struct dw_dma_chan *_dwc = to_dw_dma_chan(_chan);	\
 		struct dma_slave_config	*_sconfig = &_dwc->dma_sconfig;	\
+		bool _is_slave = dwc_is_slave(_sconfig);	\
 		int _dms = dwc_get_dms(__slave);		\
 		int _sms = dwc_get_sms(__slave);		\
-		u8 _smsize = __slave ? _sconfig->src_maxburst :	\
+		u8 _smsize = _is_slave ? _sconfig->src_maxburst :	\
 			DW_DMA_MSIZE_16;			\
-		u8 _dmsize = __slave ? _sconfig->dst_maxburst :	\
+		u8 _dmsize = _is_slave ? _sconfig->dst_maxburst :	\
 			DW_DMA_MSIZE_16;			\
 								\
 		(DWC_CTLL_DST_MSIZE(_dmsize)			\

@@ -330,7 +336,7 @@ dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
 	list_splice_init(&desc->tx_list, &dwc->free_list);
 	list_move(&desc->desc_node, &dwc->free_list);
 
-	if (!dwc->chan.private) {
+	if (!dwc_is_slave(&dwc->dma_sconfig)) {
 		struct device *parent = chan2parent(&dwc->chan);
 		if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
 			if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)

@@ -809,9 +815,11 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 
 	dev_vdbg(chan2dev(chan), "%s\n", __func__);
 
-	if (unlikely(!dws || !sg_len))
+	if (unlikely(!is_slave_direction(direction) || !sg_len))
 		return NULL;
 
+	sconfig->direction = direction;
+
 	prev = first = NULL;
 
 	switch (direction) {

@@ -983,8 +991,8 @@ set_runtime_config(struct dma_chan *chan, struct dma_slave_config *sconfig)
 {
 	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
 
-	/* Check if it is chan is configured for slave transfers */
-	if (!chan->private)
+	/* Check if chan will be configured for slave transfers */
+	if (!dwc_is_slave(sconfig))
 		return -EINVAL;
 
 	memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig));

@@ -1344,6 +1352,8 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 	if (unlikely(!is_slave_direction(direction)))
 		goto out_err;
 
+	sconfig->direction = direction;
+
 	if (direction == DMA_MEM_TO_DEV)
 		reg_width = __ffs(sconfig->dst_addr_width);
 	else

@@ -1718,6 +1728,7 @@ static int __devinit dw_probe(struct platform_device *pdev)
 		channel_clear_bit(dw, CH_EN, dwc->mask);
 
 		dwc->dw = dw;
+		dwc->dma_sconfig.direction = DMA_TRANS_NONE;
 
 		/* hardware configuration */
 		if (autocfg) {

Signature · 1 line

1.7.10.4

viresh kumar

2012-11-15 14:58 UTC #2 lore raw

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

drivers/dma/dw_dmac.c static inline bool is_slave_direction(enum dma_transfer_direction direction) · line 47

Quoted 6 lines · Andy Shevchenko

drivers/dma/dw_dmac.c +4 -0

 
+static inline bool dwc_is_slave(struct dma_slave_config *sconfig)
+{
+       return is_slave_direction(sconfig->direction);
+}

I will not buy this one. Why hide the real implementation, call is_slave_direction()
directly.

drivers/dma/dw_dmac.c static int __devinit dw_probe(struct platform_device *pdev) · line 1728

Quoted 6 lines

drivers/dma/dw_dmac.c +3 -0

@@ -1344,6 +1352,8 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
+       sconfig->direction = direction;
+

@@ -1718,6 +1728,7 @@ static int __devinit dw_probe(struct platform_device *pdev)
+               dwc->dma_sconfig.direction = DMA_TRANS_NONE;

Why do you need above changes??

--
viresh

Andy Shevchenko

author

2012-11-15 15:21 UTC #3 lore raw

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

Quoted 4 lines · viresh kumar

Quoted 1 line · Andy Shevchenko
drivers/dma/dw_dmac.c +0 -0

Quoted 8 lines

drivers/dma/dw_dmac.c static inline bool is_slave_direction(enum dma_transfer_direction direction) · line 47
Quoted 4 lines
drivers/dma/dw_dmac.c +4 -0
+static inline bool dwc_is_slave(struct dma_slave_config *sconfig)
+{
+       return is_slave_direction(sconfig->direction);
+}
I will not buy this one. Why hide the real implementation, call is_slave_direction()
directly.

There is no strong reason to keep it so.

Quoted 8 lines

drivers/dma/dw_dmac.c static int __devinit dw_probe(struct platform_device *pdev) · line 1728
Quoted 6 lines
drivers/dma/dw_dmac.c +3 -0
@@ -1344,6 +1352,8 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
+       sconfig->direction = direction;
+
 
@@ -1718,6 +1728,7 @@ static int __devinit dw_probe(struct platform_device *pdev)
+               dwc->dma_sconfig.direction = DMA_TRANS_NONE;
Why do you need above changes??

This one is not needed indeed. But we have to look after default (0) enum value.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Viresh Kumar

2012-11-15 15:24 UTC #4 lore raw

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

Quoted 20 lines · Andy Shevchenko

Quoted 4 lines · viresh kumar
Quoted 1 line · Andy Shevchenko
drivers/dma/dw_dmac.c +0 -0
Quoted 8 lines
drivers/dma/dw_dmac.c static inline bool is_slave_direction(enum dma_transfer_direction direction) · line 47
Quoted 4 lines
drivers/dma/dw_dmac.c +4 -0
+static inline bool dwc_is_slave(struct dma_slave_config *sconfig)
+{
+       return is_slave_direction(sconfig->direction);
+}
I will not buy this one. Why hide the real implementation, call is_slave_direction()
directly.
There is no strong reason to keep it so.
Quoted 4 lines
Quoted 3 lines
drivers/dma/dw_dmac.c +2 -0
@@ -1344,6 +1352,8 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
+       sconfig->direction = direction;
+

Quoted 7 lines

Quoted 4 lines
drivers/dma/dw_dmac.c static int __devinit dw_probe(struct platform_device *pdev) · line 1728
Quoted 2 lines
drivers/dma/dw_dmac.c +1 -0
@@ -1718,6 +1728,7 @@ static int __devinit dw_probe(struct platform_device *pdev)
+               dwc->dma_sconfig.direction = DMA_TRANS_NONE;
Why do you need above changes??
This one is not needed indeed. But we have to look after default (0) enum value.

I am asking about all lines where you are doing sconfig->direction = foo;

Andy Shevchenko

author

2012-11-15 15:28 UTC #5 lore raw

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

Quoted 27 lines · Viresh Kumar

Quoted 25 lines · Andy Shevchenko
Quoted 4 lines · viresh kumar
Quoted 1 line · Andy Shevchenko
drivers/dma/dw_dmac.c +0 -0
Quoted 8 lines
drivers/dma/dw_dmac.c static inline bool is_slave_direction(enum dma_transfer_direction direction) · line 47
Quoted 4 lines
drivers/dma/dw_dmac.c +4 -0
+static inline bool dwc_is_slave(struct dma_slave_config *sconfig)
+{
+       return is_slave_direction(sconfig->direction);
+}
I will not buy this one. Why hide the real implementation, call is_slave_direction()
directly.
There is no strong reason to keep it so.
Quoted 8 lines
drivers/dma/dw_dmac.c static int __devinit dw_probe(struct platform_device *pdev) · line 1728
Quoted 6 lines
drivers/dma/dw_dmac.c +3 -0
@@ -1344,6 +1352,8 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
+       sconfig->direction = direction;
+
 
@@ -1718,6 +1728,7 @@ static int __devinit dw_probe(struct platform_device *pdev)
+               dwc->dma_sconfig.direction = DMA_TRANS_NONE;
Why do you need above changes??

Quoted 4 lines

Quoted 2 lines
This one is not needed indeed. But we have to look after default (0) enum value.
I am asking about all lines where you are doing sconfig->direction = foo;

Well, the prep_* should assign the value due to changes of check in the dwc_descriptor_complete. Otherwise we will potentially skip some important piece of code.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Viresh Kumar

2012-11-15 15:38 UTC #6 lore raw

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

Quoted 3 lines · Andy Shevchenko

Well, the prep_* should assign the value due to changes of check in the dwc_descriptor_complete. Otherwise we will potentially skip some important piece of code.

What i meant to say was, set_runtime_config() must have already done this part.

--
viresh

Andy Shevchenko

2012-11-15 17:58 UTC #7 lore raw

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

Quoted 7 lines · Viresh Kumar

Quoted 3 lines · Andy Shevchenko
Well, the prep_* should assign the value due to changes of check in the dwc_descriptor_complete. Otherwise we will potentially skip some important piece of code.
What i meant to say was, set_runtime_config() must have already done this part.

On one hand it is true. On the other - *_prep* functions use explicitly passed parameter. I doubt there is a consistency between value in slave config passed via dwc_control and value passed as explicit function parameter.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Viresh Kumar

2012-11-16 04:29 UTC #8 lore raw

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

Quoted 7 lines · Andy Shevchenko

Quoted 6 lines · Viresh Kumar
Quoted 3 lines · Andy Shevchenko
Well, the prep_* should assign the value due to changes of check in the dwc_descriptor_complete. Otherwise we will potentially skip some important piece of code.

Quoted 6 lines

Quoted 1 line
What i meant to say was, set_runtime_config() must have already done this part.
On one hand it is true. On the other - *_prep* functions use explicitly passed parameter. I doubt there is a consistency between value in slave config passed via dwc_control and value passed as explicit function parameter.

I believe it should be consistent.

@Vinod: Why have we duplicated direction? Once in prep_* and then in slave_config?

--
viresh

Andy Shevchenko

author

2012-11-16 12:38 UTC #9 lore raw

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

Quoted 15 lines · Viresh Kumar

Quoted 7 lines · Andy Shevchenko
Quoted 6 lines · Viresh Kumar
Quoted 3 lines · Andy Shevchenko
Well, the prep_* should assign the value due to changes of check in the dwc_descriptor_complete. Otherwise we will potentially skip some important piece of code.
Quoted 6 lines
Quoted 1 line
What i meant to say was, set_runtime_config() must have already done this part.
On one hand it is true. On the other - *_prep* functions use explicitly passed parameter. I doubt there is a consistency between value in slave config passed via dwc_control and value passed as explicit function parameter.

Quoted 4 lines

I believe it should be consistent.
@Vinod: Why have we duplicated direction? Once in prep_* and then in slave_config?

Documentation and TODO list say that slave_config.direction is kinda deprecated.

Quoted 3 lines

--
viresh

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

viresh kumar

2012-11-15 14:35 UTC #10 lore raw

In reply to Andy Shevchenko

drivers/dma/dw_dmac.c line 42

Quoted 6 lines · Andy Shevchenko

drivers/dma/dw_dmac.c +4 -0

 
+static inline bool is_slave_direction(enum dma_transfer_direction direction)
+{
+       return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
+}

I would add this in dmaengine.h and will also include check for DMA_DEV_TO_DEV.

viresh kumar

2012-11-15 14:58 UTC #11 lore raw

Quoted 10 lines · viresh kumar

drivers/dma/dw_dmac.c line 42
Quoted 6 lines · Andy Shevchenko
drivers/dma/dw_dmac.c +4 -0
 
+static inline bool is_slave_direction(enum dma_transfer_direction direction)
+{
+       return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
+}
I would add this in dmaengine.h and will also include check for DMA_DEV_TO_DEV.

Also, what about is_slave_xfer() name?

Andy Shevchenko

author

2012-11-15 15:17 UTC #12 lore raw

Quoted 5 lines · viresh kumar

Quoted 4 lines · viresh kumar
Quoted 1 line · Andy Shevchenko
drivers/dma/dw_dmac.c +0 -0

Quoted 8 lines

Quoted 6 lines
drivers/dma/dw_dmac.c line 42
Quoted 4 lines
drivers/dma/dw_dmac.c +4 -0
+static inline bool is_slave_direction(enum dma_transfer_direction direction)
+{
+       return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
+}
I would add this in dmaengine.h and will also include check for DMA_DEV_TO_DEV.
Also, what about is_slave_xfer() name?

No objections.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Andy Shevchenko

author

2012-11-16 12:37 UTC #13 lore raw

In reply to viresh kumar

Quoted 10 lines · viresh kumar

drivers/dma/dw_dmac.c line 42
Quoted 6 lines · Andy Shevchenko
drivers/dma/dw_dmac.c +4 -0
 
+static inline bool is_slave_direction(enum dma_transfer_direction direction)
+{
+       return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
+}
I would add this in dmaengine.h and will also include check for DMA_DEV_TO_DEV.

I briefly checked the other dma drivers and have noticed that dev-2-dev is not common type of transfer. So, I prefer to keep only those two in the check function.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy