ARM: omap_hwmod: Fix up resource names when booted with devicetree

PATCH New Peter Ujfalusi 4 participants 9 messages Aug 23, 2012 – Sep 6, 2012

Subsystems (2)

Arm (32-bit) Arm sub-architectures

lore

Share & tools

lore

Peter Ujfalusi

author

2012-08-23 13:54 UTC #0 lore raw

PATCH 1/1 ARM: omap_hwmod: Fix up resource names when booted with devicetree

When booted with some resource will have their name set to NULL. This can cause later kernel crash since this is not expected by the platform code.

When we boot without DT the devices are created with platform_device_add() which itself fixes up the missing resource names:
if (r->name == NULL)

	r->name = dev_name(&pdev->dev);

The of core also fixes up the resource names when taking the information from DT data - in __of_address_to_resource():
r->name = name ? name : dev->full_name;

When we boot OMAP with devicetree: of will create the devices based on the DT data so the resource names are guarantied to be not NULL. Since we have the 'ti,hwmod' tag, we remove the of created resources from the device and re-create them based on hwmod data. If the hwmod data does not specify a name for a resource it will be NULL.
This can cause kernel crash if the driver uses platform_get_resource_byname() to get any resource.

Signed-off-by Peter Ujfalusi <[email protected]>

---
 arch/arm/plat-omap/omap_device.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

arch/arm/plat-omap/omap_device.c +8 -0

index c490240..ff57b5a 100644
@@ -370,6 +370,14 @@ static int omap_device_build_from_dt(struct platform_device *pdev)
 		goto odbfd_exit1;
 	}
 
+	/* Fix up missing resource names */
+	for (i = 0; i < pdev->num_resources; i++) {
+		struct resource *r = &pdev->resource[i];
+
+		if (r->name == NULL)
+			r->name = dev_name(&pdev->dev);
+	}
+
 	if (of_get_property(node, "ti,no_idle_on_suspend", NULL))
 		omap_device_disable_idle_on_suspend(pdev);

Signature · 1 line

1.7.8.6

Paul Walmsley

2012-08-24 15:38 UTC #1 lore raw

PATCH 1/1 ARM: omap_hwmod: Fix up resource names when booted with devicetree

Hello P?ter,

Quoted 12 lines · Peter Ujfalusi

When booted with some resource will have their name set to NULL. This can cause later kernel crash since this is not expected by the platform code.
When we boot without DT the devices are created with platform_device_add() which itself fixes up the missing resource names:
if (r->name == NULL)
	r->name = dev_name(&pdev->dev);
The of core also fixes up the resource names when taking the information from DT data - in __of_address_to_resource():
r->name = name ? name : dev->full_name;

Quoted 7 lines

When we boot OMAP with devicetree: of will create the devices based on the DT data so the resource names are guarantied to be not NULL. Since we have the 'ti,hwmod' tag, we remove the of created resources from the device and re-create them based on hwmod data. If the hwmod data does not specify a name for a resource it will be NULL.
This can cause kernel crash if the driver uses platform_get_resource_byname() to get any resource.

Do we need both this one and your '[PATCH] driver core: Check if r->name is valid in platform_get_resource_byname()' ? Or would that second patch be enough? Is the crash happening in the platform_get_resource_byname() iterator?

- Paul

Peter Ujfalusi

author

2012-08-24 16:20 UTC #2 lore raw

PATCH 1/1 ARM: omap_hwmod: Fix up resource names when booted with devicetree

Hi Paul,

Quoted 4 lines · Paul Walmsley

Do we need both this one and your '[PATCH] driver core: Check if r->name is valid in platform_get_resource_byname()' ? Or would that second patch be enough? Is the crash happening in the platform_get_resource_byname() iterator?

The crash happens in platform_get_resource_byname(). What I see as a problem that when we boot without DT the r->name is configured for the hwmods. If we boot with DT we discard the resources created by the OF (which also have the r->name configured). We replace the resources from hwmods but we do not fix up the resource names (which is done in other cases).
I have sent the patch for the drivers core as well since I think it is a good practice anyway to check for NULL pointer before strcmp().

Either is good, but IMHO we should fix this in omap_hwmod (at least).

Signature · 1 line

P?ter

Benoit Cousson

2012-09-05 14:27 UTC #3 lore raw

PATCH 1/1 ARM: omap_hwmod: Fix up resource names when booted with devicetree

Hi Paul,

Quoted 16 lines · Peter Ujfalusi

Hi Paul,
Quoted 4 lines · Paul Walmsley
Do we need both this one and your '[PATCH] driver core: Check if r->name is valid in platform_get_resource_byname()' ? Or would that second patch be enough? Is the crash happening in the platform_get_resource_byname() iterator?
The crash happens in platform_get_resource_byname(). What I see as a problem that when we boot without DT the r->name is configured for the hwmods. If we boot with DT we discard the resources created by the OF (which also have the r->name configured). We replace the resources from hwmods but we do not fix up the resource names (which is done in other cases).
I have sent the patch for the drivers core as well since I think it is a good practice anyway to check for NULL pointer before strcmp().

Quoted 1 line

Either is good, but IMHO we should fix this in omap_hwmod (at least).

Yes, clearly we do have a corner case today due to the mix of hwmod / DTS resource management.
- Legacy boot + hwmod resources is OK
- DTS boot with DTS resources will be OK
- DTS boot with hwmod resources is not OK since the resource name will not be populated in the case resources are not named :-(

If you are OK, I'll take that patch along with Vaibhav one to handle properly the resources from DTS long with the DTS patches I'm queuing for 3.7.

Thanks,
Benoit

Benoit Cousson

2012-09-05 14:46 UTC #4 lore raw

PATCH 1/1 ARM: omap_hwmod: Fix up resource names when booted with devicetree

Peter,

I will just slighty change the subject since it is a omap_device.c patch and not an omap_hwmod.c one.

ARM: OMAP: omap_device: Fix up resource names when booted with devicetree

Regards,
Benoit

Quoted 35 lines · Benoit Cousson

Hi Paul,
Quoted 16 lines · Peter Ujfalusi
Hi Paul,
Quoted 4 lines · Paul Walmsley
Do we need both this one and your '[PATCH] driver core: Check if r->name is valid in platform_get_resource_byname()' ? Or would that second patch be enough? Is the crash happening in the platform_get_resource_byname() iterator?
The crash happens in platform_get_resource_byname(). What I see as a problem that when we boot without DT the r->name is configured for the hwmods. If we boot with DT we discard the resources created by the OF (which also have the r->name configured). We replace the resources from hwmods but we do not fix up the resource names (which is done in other cases).
I have sent the patch for the drivers core as well since I think it is a good practice anyway to check for NULL pointer before strcmp().
Quoted 1 line
Either is good, but IMHO we should fix this in omap_hwmod (at least).
Yes, clearly we do have a corner case today due to the mix of hwmod / DTS resource management.
- Legacy boot + hwmod resources is OK
- DTS boot with DTS resources will be OK
- DTS boot with hwmod resources is not OK since the resource name will not be populated in the case resources are not named :-(
If you are OK, I'll take that patch along with Vaibhav one to handle properly the resources from DTS long with the DTS patches I'm queuing for 3.7.
Thanks,
Benoit

Benoit Cousson

2012-09-05 14:46 UTC #5 lore raw

PATCH 1/1 ARM: omap_hwmod: Fix up resource names when booted with devicetree

In reply to Benoit Cousson

Peter,

I will just slighty change the subject since it is an omap_device.c patch and not an omap_hwmod.c one.

ARM: OMAP: omap_device: Fix up resource names when booted with devicetree

Regards,
Benoit

Quoted 35 lines · Benoit Cousson

Hi Paul,
Quoted 16 lines · Peter Ujfalusi
Hi Paul,
Quoted 4 lines · Paul Walmsley
Do we need both this one and your '[PATCH] driver core: Check if r->name is valid in platform_get_resource_byname()' ? Or would that second patch be enough? Is the crash happening in the platform_get_resource_byname() iterator?
The crash happens in platform_get_resource_byname(). What I see as a problem that when we boot without DT the r->name is configured for the hwmods. If we boot with DT we discard the resources created by the OF (which also have the r->name configured). We replace the resources from hwmods but we do not fix up the resource names (which is done in other cases).
I have sent the patch for the drivers core as well since I think it is a good practice anyway to check for NULL pointer before strcmp().
Quoted 1 line
Either is good, but IMHO we should fix this in omap_hwmod (at least).
Yes, clearly we do have a corner case today due to the mix of hwmod / DTS resource management.
- Legacy boot + hwmod resources is OK
- DTS boot with DTS resources will be OK
- DTS boot with hwmod resources is not OK since the resource name will not be populated in the case resources are not named :-(
If you are OK, I'll take that patch along with Vaibhav one to handle properly the resources from DTS long with the DTS patches I'm queuing for 3.7.
Thanks,
Benoit

Vaibhav Hiremath

2012-09-05 20:05 UTC #6 lore raw

PATCH 1/1 ARM: omap_hwmod: Fix up resource names when booted with devicetree

In reply to Benoit Cousson

Quoted 28 lines · Benoit Cousson

Hi Paul,
Quoted 16 lines · Peter Ujfalusi
Hi Paul,
Quoted 4 lines · Paul Walmsley
Do we need both this one and your '[PATCH] driver core: Check if r->name is valid in platform_get_resource_byname()' ? Or would that second patch be enough? Is the crash happening in the platform_get_resource_byname() iterator?
The crash happens in platform_get_resource_byname(). What I see as a problem that when we boot without DT the r->name is configured for the hwmods. If we boot with DT we discard the resources created by the OF (which also have the r->name configured). We replace the resources from hwmods but we do not fix up the resource names (which is done in other cases).
I have sent the patch for the drivers core as well since I think it is a good practice anyway to check for NULL pointer before strcmp().
Quoted 1 line
Either is good, but IMHO we should fix this in omap_hwmod (at least).
Yes, clearly we do have a corner case today due to the mix of hwmod / DTS resource management.
- Legacy boot + hwmod resources is OK
- DTS boot with DTS resources will be OK
- DTS boot with hwmod resources is not OK since the resource name will not be populated in the case resources are not named :-(

Quoted 4 lines

If you are OK, I'll take that patch along with Vaibhav one to handle properly the resources from DTS long with the DTS patches I'm queuing for 3.7.

Benoit,
Note that this patch will add device name to all resources, since none of the resource will have name property and may break drivers; especially after my patch which respects DT resources.

Also I just read the thread you started sometime back on _byname api, not sure that was the conclusion, but it seems like it is not recommended to use _byname. The driver should be fixed while converting to DT.

http://lists.infradead.org/pipermail/linux-arm-kernel/2011-August/060861.html

Thanks,
Vaibhav

Quoted 8 lines

Thanks,
Benoit
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Benoit Cousson

2012-09-06 07:37 UTC #7 lore raw

PATCH 1/1 ARM: omap_hwmod: Fix up resource names when booted with devicetree

Hi Vaibhav,

Quoted 34 lines · Vaibhav Hiremath

Quoted 28 lines · Benoit Cousson
Hi Paul,
Quoted 16 lines · Peter Ujfalusi
Hi Paul,
Quoted 4 lines · Paul Walmsley
Do we need both this one and your '[PATCH] driver core: Check if r->name is valid in platform_get_resource_byname()' ? Or would that second patch be enough? Is the crash happening in the platform_get_resource_byname() iterator?
The crash happens in platform_get_resource_byname(). What I see as a problem that when we boot without DT the r->name is configured for the hwmods. If we boot with DT we discard the resources created by the OF (which also have the r->name configured). We replace the resources from hwmods but we do not fix up the resource names (which is done in other cases).
I have sent the patch for the drivers core as well since I think it is a good practice anyway to check for NULL pointer before strcmp().
Quoted 1 line
Either is good, but IMHO we should fix this in omap_hwmod (at least).
Yes, clearly we do have a corner case today due to the mix of hwmod / DTS resource management.
- Legacy boot + hwmod resources is OK
- DTS boot with DTS resources will be OK
- DTS boot with hwmod resources is not OK since the resource name will not be populated in the case resources are not named :-(
Quoted 4 lines
If you are OK, I'll take that patch along with Vaibhav one to handle properly the resources from DTS long with the DTS patches I'm queuing for 3.7.

Quoted 4 lines

Benoit,
Note that this patch will add device name to all resources, since none of the resource will have name property and may break drivers; especially after my patch which respects DT resources.

So you do have to add the name in the DTS if the driver is expecting that.

Quoted 4 lines

Also I just read the thread you started sometime back on _byname api, not sure that was the conclusion, but it seems like it is not recommended to use _byname. The driver should be fixed while converting to DT.

No the DT name for reg and interrupt was accepted and Peter is taking advantage on that. The only missing binding is the DMA now.

Here is what Peter did.

compatible = "ti,omap4-mcpdm";
reg = <0x40132000 0x7f>, /* MPU private access */
<0x49032000 0x7f>; /* L3 Interconnect */
+ reg-names = "mpu", "dma";
interrupts = <0 112 0x4>; interrupt-parent = <&gic>;
ti,hwmods = "mcpdm";

Regards,
Benoit

Hiremath, Vaibhav

2012-09-06 08:28 UTC #8 lore raw

PATCH 1/1 ARM: omap_hwmod: Fix up resource names when booted with devicetree

Quoted 55 lines · Benoit

Hi Vaibhav,
Quoted 34 lines · Vaibhav Hiremath
Quoted 28 lines · Benoit Cousson
Hi Paul,
Quoted 16 lines · Peter Ujfalusi
Hi Paul,
Quoted 4 lines · Paul Walmsley
Do we need both this one and your '[PATCH] driver core: Check if r->name is valid in platform_get_resource_byname()' ? Or would that second patch be enough? Is the crash happening in the platform_get_resource_byname() iterator?
The crash happens in platform_get_resource_byname(). What I see as a problem that when we boot without DT the r->name is configured for the hwmods. If we boot with DT we discard the resources created by the OF (which also have the r->name configured). We replace the resources from hwmods but we do not fix up the resource names (which is done in other cases).
I have sent the patch for the drivers core as well since I think it is a good practice anyway to check for NULL pointer before strcmp().
Quoted 1 line
Either is good, but IMHO we should fix this in omap_hwmod (at least).
Yes, clearly we do have a corner case today due to the mix of hwmod / DTS resource management.
- Legacy boot + hwmod resources is OK
- DTS boot with DTS resources will be OK
- DTS boot with hwmod resources is not OK since the resource name will not be populated in the case resources are not named :-(
Quoted 4 lines
If you are OK, I'll take that patch along with Vaibhav one to handle properly the resources from DTS long with the DTS patches I'm queuing for 3.7.
Quoted 4 lines
Benoit,
Note that this patch will add device name to all resources, since none of the resource will have name property and may break drivers; especially after my patch which respects DT resources.
So you do have to add the name in the DTS if the driver is expecting that.
Quoted 4 lines
Also I just read the thread you started sometime back on _byname api, not sure that was the conclusion, but it seems like it is not recommended to use _byname. The driver should be fixed while converting to DT.
No the DT name for reg and interrupt was accepted and Peter is taking advantage on that. The only missing binding is the DMA now.
Here is what Peter did.

Review

Quoted 7 lines

(quoted diff) +1 -0

 			compatible = "ti,omap4-mcpdm";
 			reg = <0x40132000 0x7f>, /* MPU private access */
 			      <0x49032000 0x7f>; /* L3 Interconnect */
+			reg-names = "mpu", "dma";
 			interrupts = <0 112 0x4>;
 			interrupt-parent = <&gic>;
 			ti,hwmods = "mcpdm";

Thanks for sharing this and now I found it in Documentation/devicetree/bindings/resource-names.txt

In that case, we do not have any issues and you can merge this patch.

Thanks,
Vaibhav

Quoted 5 lines

Regards,
Benoit