diff mbox

dma: of-dma: return error when 'dma-cells' not found

Message ID 1362475531-32260-1-git-send-email-padma.v@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Padmavathi Venna March 5, 2013, 9:25 a.m. UTC
This patch returns error when 'dma-cells' property not found
in the corresponding device node. With out this change there
is a crash in the generic dma incompatible platforms.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
---

Based on Vinod Koul next branch.

 drivers/dma/of-dma.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

Comments

Rob Herring March 5, 2013, 9:43 a.m. UTC | #1
On 03/05/2013 03:25 AM, Padmavathi Venna wrote:
> This patch returns error when 'dma-cells' property not found
> in the corresponding device node. With out this change there
> is a crash in the generic dma incompatible platforms.
> 
> Signed-off-by: Padmavathi Venna <padma.v@samsung.com>

NAK.

#dma-cells should be optional. It is not needed for platforms supporting
memory to memory transfers only and should therefore be optional. You
cannot assume the dtb can be updated and kernel changes need to work
with old dtbs. I've submitted patches to address this and fix the crash:

https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-February/028769.html

Rob
> ---
> 
> Based on Vinod Koul next branch.
> 
>  drivers/dma/of-dma.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
> index 69d04d2..46aca0d 100644
> --- a/drivers/dma/of-dma.c
> +++ b/drivers/dma/of-dma.c
> @@ -92,6 +92,7 @@ int of_dma_controller_register(struct device_node *np,
>  				void *data)
>  {
>  	struct of_dma	*ofdma;
> +	const	 __be32 *ip;
>  	int		nbcells;
>  
>  	if (!np || !of_dma_xlate) {
> @@ -103,7 +104,12 @@ int of_dma_controller_register(struct device_node *np,
>  	if (!ofdma)
>  		return -ENOMEM;
>  
> -	nbcells = be32_to_cpup(of_get_property(np, "#dma-cells", NULL));
> +	ip = of_get_property(np, "#dma-cells", NULL);
> +	if (!ip)
> +		return -ENXIO;
> +
> +	nbcells = be32_to_cpup(ip);
> +
>  	if (!nbcells) {
>  		pr_err("%s: #dma-cells property is missing or invalid\n",
>  		       __func__);
>
padma venkat March 5, 2013, 10:04 a.m. UTC | #2
On Tue, Mar 5, 2013 at 3:13 PM, Rob Herring <robherring2@gmail.com> wrote:
> On 03/05/2013 03:25 AM, Padmavathi Venna wrote:
>> This patch returns error when 'dma-cells' property not found
>> in the corresponding device node. With out this change there
>> is a crash in the generic dma incompatible platforms.
>>
>> Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
>
> NAK.
>
> #dma-cells should be optional. It is not needed for platforms supporting
> memory to memory transfers only and should therefore be optional. You
> cannot assume the dtb can be updated and kernel changes need to work
> with old dtbs. I've submitted patches to address this and fix the crash:
>
> https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-February/028769.html
>

Okay.

Thanks
Padma

> Rob
>> ---
>>
>> Based on Vinod Koul next branch.
>>
>>  drivers/dma/of-dma.c |    8 +++++++-
>>  1 files changed, 7 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
>> index 69d04d2..46aca0d 100644
>> --- a/drivers/dma/of-dma.c
>> +++ b/drivers/dma/of-dma.c
>> @@ -92,6 +92,7 @@ int of_dma_controller_register(struct device_node *np,
>>                               void *data)
>>  {
>>       struct of_dma   *ofdma;
>> +     const    __be32 *ip;
>>       int             nbcells;
>>
>>       if (!np || !of_dma_xlate) {
>> @@ -103,7 +104,12 @@ int of_dma_controller_register(struct device_node *np,
>>       if (!ofdma)
>>               return -ENOMEM;
>>
>> -     nbcells = be32_to_cpup(of_get_property(np, "#dma-cells", NULL));
>> +     ip = of_get_property(np, "#dma-cells", NULL);
>> +     if (!ip)
>> +             return -ENXIO;
>> +
>> +     nbcells = be32_to_cpup(ip);
>> +
>>       if (!nbcells) {
>>               pr_err("%s: #dma-cells property is missing or invalid\n",
>>                      __func__);
>>
>
Arnd Bergmann March 5, 2013, 10:15 a.m. UTC | #3
On Tuesday 05 March 2013 03:43:52 Rob Herring wrote:
> On 03/05/2013 03:25 AM, Padmavathi Venna wrote:
> > This patch returns error when 'dma-cells' property not found
> > in the corresponding device node. With out this change there
> > is a crash in the generic dma incompatible platforms.
> > 
> > Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
> 
> NAK.
> 
> #dma-cells should be optional. It is not needed for platforms supporting
> memory to memory transfers only and should therefore be optional. You
> cannot assume the dtb can be updated and kernel changes need to work
> with old dtbs. I've submitted patches to address this and fix the crash:
> 
> https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-February/028769.html

Why would you call of_dma_controller_register() for a dma
engine that does not support slave channels, when that is the
only purpose of that interface?

Note that the binding defines #dma-cells as required, and it
does not make any sense otherwise.

	Arnd
Rob Herring March 5, 2013, 2:56 p.m. UTC | #4
On 03/05/2013 04:15 AM, Arnd Bergmann wrote:
> On Tuesday 05 March 2013 03:43:52 Rob Herring wrote:
>> On 03/05/2013 03:25 AM, Padmavathi Venna wrote:
>>> This patch returns error when 'dma-cells' property not found
>>> in the corresponding device node. With out this change there
>>> is a crash in the generic dma incompatible platforms.
>>>
>>> Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
>>
>> NAK.
>>
>> #dma-cells should be optional. It is not needed for platforms supporting
>> memory to memory transfers only and should therefore be optional. You
>> cannot assume the dtb can be updated and kernel changes need to work
>> with old dtbs. I've submitted patches to address this and fix the crash:
>>
>> https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-February/028769.html
> 
> Why would you call of_dma_controller_register() for a dma
> engine that does not support slave channels, when that is the
> only purpose of that interface?

Well maybe then that function should be allowed to fail without erroring
out. I just fixed it a the line that failed. Doing be32_to_cpup directly
on a function return that can be NULL is not correct either.

> Note that the binding defines #dma-cells as required, and it
> does not make any sense otherwise.

The 2nd patch I submitted changes that. It does not make sense to
require it if you have no requests and hence will never have a phandle
reference in a slave device. The simple fact is that the pl330 had an
existing binding that worked for the memory to memory only case and
kernel changes broke this. Kernel changes should not break existing
device-trees.

Rob
Arnd Bergmann March 5, 2013, 7:48 p.m. UTC | #5
On Tuesday 05 March 2013, Rob Herring wrote:
> > Why would you call of_dma_controller_register() for a dma
> > engine that does not support slave channels, when that is the
> > only purpose of that interface?
> 
> Well maybe then that function should be allowed to fail without erroring
> out. I just fixed it a the line that failed.

We have a lot of interfaces that fall back to silently doing nothing
when there is no need.

> Doing be32_to_cpup directly
> on a function return that can be NULL is not correct either.

Agreed.
 
> > Note that the binding defines #dma-cells as required, and it
> > does not make any sense otherwise.
> 
> The 2nd patch I submitted changes that. It does not make sense to
> require it if you have no requests and hence will never have a phandle
> reference in a slave device.

I still don't see why you would want to use the binding for
dma slaves to describe a dmaengine that does not have slaves.

> The simple fact is that the pl330 had an
> existing binding that worked for the memory to memory only case and
> kernel changes broke this. Kernel changes should not break existing
> device-trees.

Of course we should not break the existing device tree, but I think
it would be more sensible to change the pl330 specific binding in this
case to require the use of the generic dma slave binding only when
there are slaves connected to it.

If the #dma-cells property is absent, the pl330 driver can be changed
not to call of_dma_controller_register.

	Arnd
diff mbox

Patch

diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 69d04d2..46aca0d 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -92,6 +92,7 @@  int of_dma_controller_register(struct device_node *np,
 				void *data)
 {
 	struct of_dma	*ofdma;
+	const	 __be32 *ip;
 	int		nbcells;
 
 	if (!np || !of_dma_xlate) {
@@ -103,7 +104,12 @@  int of_dma_controller_register(struct device_node *np,
 	if (!ofdma)
 		return -ENOMEM;
 
-	nbcells = be32_to_cpup(of_get_property(np, "#dma-cells", NULL));
+	ip = of_get_property(np, "#dma-cells", NULL);
+	if (!ip)
+		return -ENXIO;
+
+	nbcells = be32_to_cpup(ip);
+
 	if (!nbcells) {
 		pr_err("%s: #dma-cells property is missing or invalid\n",
 		       __func__);