[PATCH] arm: edma: Fix xbar mapping

[PATCH] arm: edma: Fix xbar mapping

[Thomas Gleixner]

Subject: arm: edma: Fix xbar mapping
From: Thomas Gleixner <tglx@linutronix.de>
Date: Sun, 13 Apr 2014 20:44:46 +0200

This is another great example of trainwreck engineering:

commit 2646a0e529 (ARM: edma: Add EDMA crossbar event mux support)
added support for using EDMA on peripherals which have no direct EDMA
event mapping.

The code compiles and does not explode in your face, but that's it.

1) Reading an u16 array from an u32 device tree array simply does not
   work. Even if the function is named "edma_of_read_u32_to_s16_array".

   It merily calls of_property_read_u16_array. So the resulting 16bit
   array will have every other entry = 0.

2) The DT entry for the xbar registers related to xbar has length 0x10
   instead of the real length: 0xfd0 - 0xf90 = 0x40.

   Not a real problem as it does not cross a page boundary, but
   wrong nevertheless.

3) But none of this matters as the mapping never happens:

   After reading nonsense edma_of_read_u32_to_s16_array() invalidates
   the first array entry pair, so nobody can ever notice the
   braindamage by immediate explosion.

Seems the QA criteria for this code was solely not to explode when
someone adds edma-xbar-event-map entries to the DT. Goal achieved,
congratulations!

Not really helpful if someone wants to use edma on a device which
requires a xbar mapping.

Fix the issues by:

- annotating the device tree entry with "/bits/ 16" as documented in
  the of_property_read_u16_array kernel doc

- make the size of the xbar register mapping correct

- invalidating the end of the array and not the start

This convoluted mess wants to be completely rewritten as there is no
point to keep the xbar_chan array memory and the iomapping of the xbar
regs around forever. Marking the xbar mapped channels as used should
be done right there.

But that's a different issue and this patch is small enough to make it
work and allows a simple backport for stable.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 Documentation/devicetree/bindings/dma/ti-edma.txt |    4 -
 arch/arm/boot/dts/am33xx.dtsi                     |    2 
 arch/arm/common/edma.c                            |   48 ++++++----------------
 3 files changed, 18 insertions(+), 36 deletions(-)

Index: linux-2.6/Documentation/devicetree/bindings/dma/ti-edma.txt
===================================================================
--- linux-2.6.orig/Documentation/devicetree/bindings/dma/ti-edma.txt
+++ linux-2.6/Documentation/devicetree/bindings/dma/ti-edma.txt
@@ -29,6 +29,6 @@ edma: edma at 49000000 {
 	dma-channels = <64>;
 	ti,edma-regions = <4>;
 	ti,edma-slots = <256>;
-	ti,edma-xbar-event-map = <1 12
-				  2 13>;
+	ti,edma-xbar-event-map = /bits/ 16 <1 12
+				  	    2 13>;
 };
Index: linux-2.6/arch/arm/boot/dts/am33xx.dtsi
===================================================================
--- linux-2.6.orig/arch/arm/boot/dts/am33xx.dtsi
+++ linux-2.6/arch/arm/boot/dts/am33xx.dtsi
@@ -144,7 +144,7 @@
 			compatible = "ti,edma3";
 			ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";
 			reg =	<0x49000000 0x10000>,
-				<0x44e10f90 0x10>;
+				<0x44e10f90 0x40>;
 			interrupts = <12 13 14>;
 			#dma-cells = <1>;
 			dma-channels = <64>;
Index: linux-2.6/arch/arm/common/edma.c
===================================================================
--- linux-2.6.orig/arch/arm/common/edma.c
+++ linux-2.6/arch/arm/common/edma.c
@@ -1423,55 +1423,38 @@ EXPORT_SYMBOL(edma_clear_event);
 
 #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_DMADEVICES)
 
-static int edma_of_read_u32_to_s16_array(const struct device_node *np,
-					 const char *propname, s16 *out_values,
-					 size_t sz)
+static int edma_xbar_event_map(struct device *dev, struct device_node *node,
+			       struct edma_soc_info *pdata, size_t sz)
 {
-	int ret;
-
-	ret = of_property_read_u16_array(np, propname, out_values, sz);
-	if (ret)
-		return ret;
-
-	/* Terminate it */
-	*out_values++ = -1;
-	*out_values++ = -1;
-
-	return 0;
-}
-
-static int edma_xbar_event_map(struct device *dev,
-			       struct device_node *node,
-			       struct edma_soc_info *pdata, int len)
-{
-	int ret, i;
+	const char pname[] = "ti,edma-xbar-event-map";
 	struct resource res;
 	void __iomem *xbar;
-	const s16 (*xbar_chans)[2];
+	s16 (*xbar_chans)[2];
+	size_t nelm = sz / sizeof(s16);
 	u32 shift, offset, mux;
+	int ret, i;
 
-	xbar_chans = devm_kzalloc(dev,
-				  len/sizeof(s16) + 2*sizeof(s16),
-				  GFP_KERNEL);
+	xbar_chans = devm_kzalloc(dev, (nelm + 2) * sizeof(s16), GFP_KERNEL);
 	if (!xbar_chans)
 		return -ENOMEM;
 
 	ret = of_address_to_resource(node, 1, &res);
 	if (ret)
-		return -EIO;
+		return -ENOMEM;
 
 	xbar = devm_ioremap(dev, res.start, resource_size(&res));
 	if (!xbar)
 		return -ENOMEM;
 
-	ret = edma_of_read_u32_to_s16_array(node,
-					    "ti,edma-xbar-event-map",
-					    (s16 *)xbar_chans,
-					    len/sizeof(u32));
+	ret = of_property_read_u16_array(node, pname, (u16 *)xbar_chans, nelm);
 	if (ret)
 		return -EIO;
 
-	for (i = 0; xbar_chans[i][0] != -1; i++) {
+	/* Invalidate last entry for the other user of this mess */
+	nelm >>= 1;
+	xbar_chans[nelm][0] = xbar_chans[nelm][1] = -1;
+
+	for (i = 0; i < nelm; i++) {
 		shift = (xbar_chans[i][1] & 0x03) << 3;
 		offset = xbar_chans[i][1] & 0xfffffffc;
 		mux = readl(xbar + offset);
@@ -1480,8 +1463,7 @@ static int edma_xbar_event_map(struct de
 		writel(mux, (xbar + offset));
 	}
 
-	pdata->xbar_chans = xbar_chans;
-
+	pdata->xbar_chans = (const s16 (*)[2]) xbar_chans;
 	return 0;
 }
 

[PATCH] arm: edma: Fix xbar mapping

[Sekhar Nori]

+ Matt with current e-mail address.

On Monday 14 April 2014 02:18 AM, Thomas Gleixner wrote:
> Subject: arm: edma: Fix xbar mapping
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Sun, 13 Apr 2014 20:44:46 +0200
> 
> This is another great example of trainwreck engineering:
> 
> commit 2646a0e529 (ARM: edma: Add EDMA crossbar event mux support)
> added support for using EDMA on peripherals which have no direct EDMA
> event mapping.

I committed it, so the blame goes to me (at least in part).

> 
> The code compiles and does not explode in your face, but that's it.
> 
> 1) Reading an u16 array from an u32 device tree array simply does not
>    work. Even if the function is named "edma_of_read_u32_to_s16_array".
> 
>    It merily calls of_property_read_u16_array. So the resulting 16bit
>    array will have every other entry = 0.
> 
> 2) The DT entry for the xbar registers related to xbar has length 0x10
>    instead of the real length: 0xfd0 - 0xf90 = 0x40.
> 
>    Not a real problem as it does not cross a page boundary, but
>    wrong nevertheless.
> 
> 3) But none of this matters as the mapping never happens:
> 
>    After reading nonsense edma_of_read_u32_to_s16_array() invalidates
>    the first array entry pair, so nobody can ever notice the
>    braindamage by immediate explosion.
> 
> Seems the QA criteria for this code was solely not to explode when
> someone adds edma-xbar-event-map entries to the DT. Goal achieved,
> congratulations!
> 
> Not really helpful if someone wants to use edma on a device which
> requires a xbar mapping.
> 
> Fix the issues by:
> 
> - annotating the device tree entry with "/bits/ 16" as documented in
>   the of_property_read_u16_array kernel doc
> 
> - make the size of the xbar register mapping correct
> 
> - invalidating the end of the array and not the start
> 
> This convoluted mess wants to be completely rewritten as there is no
> point to keep the xbar_chan array memory and the iomapping of the xbar
> regs around forever. Marking the xbar mapped channels as used should
> be done right there.
> 
> But that's a different issue and this patch is small enough to make it
> work and allows a simple backport for stable.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

This time, I tested this patch and FWIW you can add:

Acked-by: Sekhar Nori <nsekhar@ti.com>

Thanks,
Sekhar

[PATCH] arm: edma: Fix xbar mapping

[Thomas Gleixner]

On Mon, 14 Apr 2014, Sekhar Nori wrote:
> > Fix the issues by:
> > 
> > - annotating the device tree entry with "/bits/ 16" as documented in
> >   the of_property_read_u16_array kernel doc
> > 
> > - make the size of the xbar register mapping correct
> > 
> > - invalidating the end of the array and not the start
> > 
> > This convoluted mess wants to be completely rewritten as there is no
> > point to keep the xbar_chan array memory and the iomapping of the xbar
> > regs around forever. Marking the xbar mapped channels as used should
> > be done right there.
> > 
> > But that's a different issue and this patch is small enough to make it
> > work and allows a simple backport for stable.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> This time, I tested this patch and FWIW you can add:
> 
> Acked-by: Sekhar Nori <nsekhar@ti.com>

Can someone pick this up please?

[PATCH] arm: edma: Fix xbar mapping

[Vinod Koul]

On Mon, Apr 28, 2014 at 08:32:55PM +0200, Thomas Gleixner wrote:
> On Mon, 14 Apr 2014, Sekhar Nori wrote:
> > > Fix the issues by:
> > > 
> > > - annotating the device tree entry with "/bits/ 16" as documented in
> > >   the of_property_read_u16_array kernel doc
> > > 
> > > - make the size of the xbar register mapping correct
> > > 
> > > - invalidating the end of the array and not the start
> > > 
> > > This convoluted mess wants to be completely rewritten as there is no
> > > point to keep the xbar_chan array memory and the iomapping of the xbar
> > > regs around forever. Marking the xbar mapped channels as used should
> > > be done right there.
> > > 
> > > But that's a different issue and this patch is small enough to make it
> > > work and allows a simple backport for stable.
> > > 
> > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > 
> > This time, I tested this patch and FWIW you can add:
> > 
> > Acked-by: Sekhar Nori <nsekhar@ti.com>
> 
> Can someone pick this up please?
Shouldnt this go thru ARM tree?

-- 
~Vinod

[PATCH] arm: edma: Fix xbar mapping

[Sekhar Nori]

On Tuesday 29 April 2014 11:37 AM, Vinod Koul wrote:
> On Mon, Apr 28, 2014 at 08:32:55PM +0200, Thomas Gleixner wrote:
>> On Mon, 14 Apr 2014, Sekhar Nori wrote:
>>>> Fix the issues by:
>>>>
>>>> - annotating the device tree entry with "/bits/ 16" as documented in
>>>>   the of_property_read_u16_array kernel doc
>>>>
>>>> - make the size of the xbar register mapping correct
>>>>
>>>> - invalidating the end of the array and not the start
>>>>
>>>> This convoluted mess wants to be completely rewritten as there is no
>>>> point to keep the xbar_chan array memory and the iomapping of the xbar
>>>> regs around forever. Marking the xbar mapped channels as used should
>>>> be done right there.
>>>>
>>>> But that's a different issue and this patch is small enough to make it
>>>> work and allows a simple backport for stable.
>>>>
>>>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>>>
>>> This time, I tested this patch and FWIW you can add:
>>>
>>> Acked-by: Sekhar Nori <nsekhar@ti.com>
>>
>> Can someone pick this up please?
> Shouldnt this go thru ARM tree?

I will send a pull request to ARM-SoC.

Thanks,
Sekhar

[PATCH] arm: edma: Fix xbar mapping

[Olof Johansson]

On Mon, Apr 14, 2014 at 7:14 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> + Matt with current e-mail address.
>
> On Monday 14 April 2014 02:18 AM, Thomas Gleixner wrote:
>> Subject: arm: edma: Fix xbar mapping
>> From: Thomas Gleixner <tglx@linutronix.de>
>> Date: Sun, 13 Apr 2014 20:44:46 +0200
>>
>> This is another great example of trainwreck engineering:
>>
>> commit 2646a0e529 (ARM: edma: Add EDMA crossbar event mux support)
>> added support for using EDMA on peripherals which have no direct EDMA
>> event mapping.
>
> I committed it, so the blame goes to me (at least in part).

Another part of the problem is that it's a feature without in-tree
users -- there's not a single DT that defines the nodes (and thus can
be used to test it). Sure, there are no strict requirements to have
all bindings implemented on a platform but it sure does help catch
problems if you can test without applying out-of-tree patches to make
a feature work.

(I just merged the pull request from Sekhar, I had missed it before.
Should make -rc6).


-Olof