[PATCH 1/2] Revert "OMAP: omap_device: add omap_device_is_valid()"

[PATCH 1/2] Revert "OMAP: omap_device: add omap_device_is_valid()"

[Kevin Hilman]

From: Kevin Hilman <khilman@ti.com>

This reverts commit 0007122ad85cc36b1c18c0b59344093ca210d206.

The dereference method of checking for a valid omap_device when
wrapping a platform_device is rather unsafe and dangerous.

Instead, a better way of checking for a valid omap-device is
to use a common parent device for all omap_devices, then a check
can simply be made using the device parent.  The only user of this
API was the initial version of the runtime PM core for OMAP.  This
has now been switched to check device parent, so there are no more
users of this API.

Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 arch/arm/plat-omap/include/plat/omap_device.h |    2 --
 arch/arm/plat-omap/omap_device.c              |   20 --------------------
 2 files changed, 0 insertions(+), 22 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
index 25cd9ac..bad4c3d 100644
--- a/arch/arm/plat-omap/include/plat/omap_device.h
+++ b/arch/arm/plat-omap/include/plat/omap_device.h
@@ -62,7 +62,6 @@
  *
  */
 struct omap_device {
-	u32                             magic;
 	struct platform_device		pdev;
 	struct omap_hwmod		**hwmods;
 	struct omap_device_pm_latency	*pm_lats;
@@ -82,7 +81,6 @@ int omap_device_shutdown(struct platform_device *pdev);
 
 /* Core code interface */
 
-bool omap_device_is_valid(struct omap_device *od);
 int omap_device_count_resources(struct omap_device *od);
 int omap_device_fill_resources(struct omap_device *od, struct resource *res);
 
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index d2b1609..7f05f49 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -90,12 +90,6 @@
 #define USE_WAKEUP_LAT			0
 #define IGNORE_WAKEUP_LAT		1
 
-/*
- * OMAP_DEVICE_MAGIC: used to determine whether a struct omap_device
- * obtained via container_of() is in fact a struct omap_device
- */
-#define OMAP_DEVICE_MAGIC		 0xf00dcafe
-
 /* Private functions */
 
 /**
@@ -414,8 +408,6 @@ struct omap_device *omap_device_build_ss(const char *pdev_name, int pdev_id,
 	od->pm_lats = pm_lats;
 	od->pm_lats_cnt = pm_lats_cnt;
 
-	od->magic = OMAP_DEVICE_MAGIC;
-
 	if (is_early_device)
 		ret = omap_early_device_register(od);
 	else
@@ -627,18 +619,6 @@ int omap_device_align_pm_lat(struct platform_device *pdev,
 }
 
 /**
- * omap_device_is_valid - Check if pointer is a valid omap_device
- * @od: struct omap_device *
- *
- * Return whether struct omap_device pointer @od points to a valid
- * omap_device.
- */
-bool omap_device_is_valid(struct omap_device *od)
-{
-	return (od && od->magic == OMAP_DEVICE_MAGIC);
-}
-
-/**
  * omap_device_get_pwrdm - return the powerdomain * associated with @od
  * @od: struct omap_device *
  *
-- 
1.7.2.1

[PATCH 2/2] OMAP: omap_device: make all devices a child of a new omap_bus device

[Kevin Hilman]

From: Kevin Hilman <khilman@ti.com>

In order to help differentiate omap_devices from normal
platform_devices, make them all a parent of a new omap_bus device.

Then, in order to determine if a platform_device is also an
omap_device, checking the parent is all that is needed.

Users of this feature are the runtime PM core for OMAP, where we need
to know if a device being passed in is an omap_device or not in order
to know whether to call the omap_device API with it.

In addition, all omap_devices will now show up under /sys/devices/omap
instead of /sys/devices/platform

Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 arch/arm/plat-omap/include/plat/omap_device.h |    2 ++
 arch/arm/plat-omap/omap_device.c              |   18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
index bad4c3d..26d0c10 100644
--- a/arch/arm/plat-omap/include/plat/omap_device.h
+++ b/arch/arm/plat-omap/include/plat/omap_device.h
@@ -36,6 +36,8 @@
 
 #include <plat/omap_hwmod.h>
 
+extern struct device omap_bus;
+
 /* omap_device._state values */
 #define OMAP_DEVICE_STATE_UNKNOWN	0
 #define OMAP_DEVICE_STATE_ENABLED	1
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index 7f05f49..3e215fa 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -463,8 +463,11 @@ int omap_early_device_register(struct omap_device *od)
  */
 int omap_device_register(struct omap_device *od)
 {
+	struct platform_device *pdev = &od->pdev;
+
 	pr_debug("omap_device: %s: registering\n", od->pdev.name);
 
+	pdev->dev.parent = &omap_bus;
 	return platform_device_register(&od->pdev);
 }
 
@@ -737,3 +740,18 @@ int omap_device_enable_clocks(struct omap_device *od)
 	/* XXX pass along return value here? */
 	return 0;
 }
+
+struct device omap_bus = {
+	.init_name	= "omap",
+};
+
+static int __init omap_device_init(void)
+{
+	int error = 0;
+
+	printk("%s:\n", __func__);
+	error = device_register(&omap_bus);
+
+	return error;
+}
+core_initcall(omap_device_init);
-- 
1.7.2.1

RE: [PATCH 2/2] OMAP: omap_device: make all devices a child of a new omap_bus device

[Gopinath, Thara]

>>-----Original Message-----
>>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kevin
>>Hilman
>>Sent: Wednesday, September 01, 2010 5:33 AM
>>To: linux-omap@vger.kernel.org
>>Cc: paul@pwsan.com
>>Subject: [PATCH 2/2] OMAP: omap_device: make all devices a child of a new omap_bus device
>>
>>From: Kevin Hilman <khilman@ti.com>
>>
>>In order to help differentiate omap_devices from normal
>>platform_devices, make them all a parent of a new omap_bus device.
>>
>>Then, in order to determine if a platform_device is also an
>>omap_device, checking the parent is all that is needed.
>>
>>Users of this feature are the runtime PM core for OMAP, where we need
>>to know if a device being passed in is an omap_device or not in order
>>to know whether to call the omap_device API with it.
>>
>>In addition, all omap_devices will now show up under /sys/devices/omap
>>instead of /sys/devices/platform

Hello Kevin,

Couple of minor queries/comments below.

>>
>>Signed-off-by: Kevin Hilman <khilman@ti.com>
>>---
>> arch/arm/plat-omap/include/plat/omap_device.h |    2 ++
>> arch/arm/plat-omap/omap_device.c              |   18 ++++++++++++++++++
>> 2 files changed, 20 insertions(+), 0 deletions(-)
>>
>>diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-
>>omap/include/plat/omap_device.h
>>index bad4c3d..26d0c10 100644
>>--- a/arch/arm/plat-omap/include/plat/omap_device.h
>>+++ b/arch/arm/plat-omap/include/plat/omap_device.h
>>@@ -36,6 +36,8 @@
>>
>> #include <plat/omap_hwmod.h>
>>
>>+extern struct device omap_bus;
>>+

Why is this extern declaration needed? Is it later on
to check in runtime pm code pdev->dev.parent == omap_bus??

>> /* omap_device._state values */
>> #define OMAP_DEVICE_STATE_UNKNOWN	0
>> #define OMAP_DEVICE_STATE_ENABLED	1
>>diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
>>index 7f05f49..3e215fa 100644
>>--- a/arch/arm/plat-omap/omap_device.c
>>+++ b/arch/arm/plat-omap/omap_device.c
>>@@ -463,8 +463,11 @@ int omap_early_device_register(struct omap_device *od)
>>  */
>> int omap_device_register(struct omap_device *od)
>> {
>>+	struct platform_device *pdev = &od->pdev;
>>+
>> 	pr_debug("omap_device: %s: registering\n", od->pdev.name);
>>
>>+	pdev->dev.parent = &omap_bus;

What if device_register(&omap_bus) has returned an
error? Do we still go ahead with assigning omap_bus
as parent?

>> 	return platform_device_register(&od->pdev);
>> }
>>
>>@@ -737,3 +740,18 @@ int omap_device_enable_clocks(struct omap_device *od)
>> 	/* XXX pass along return value here? */
>> 	return 0;
>> }
>>+
>>+struct device omap_bus = {
>>+	.init_name	= "omap",
>>+};
>>+
>>+static int __init omap_device_init(void)
>>+{
>>+	int error = 0;

Is the initialization to 0 needed?

>>+
>>+	printk("%s:\n", __func__);

Spurious printk???

Regards
Thara

>>+	error = device_register(&omap_bus);
>>+
>>+	return error;
>>+}
>>+core_initcall(omap_device_init);
>>--
>>1.7.2.1
>>
>>--
>>To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>>the body of a message to majordomo@vger.kernel.org
>>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] OMAP: omap_device: make all devices a child of a new omap_bus device

[Kevin Hilman]

"Gopinath, Thara" <thara@ti.com> writes:

>>>-----Original Message-----
>>>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kevin
>>>Hilman
>>>Sent: Wednesday, September 01, 2010 5:33 AM
>>>To: linux-omap@vger.kernel.org
>>>Cc: paul@pwsan.com
>>>Subject: [PATCH 2/2] OMAP: omap_device: make all devices a child of a new omap_bus device
>>>
>>>From: Kevin Hilman <khilman@ti.com>
>>>
>>>In order to help differentiate omap_devices from normal
>>>platform_devices, make them all a parent of a new omap_bus device.
>>>
>>>Then, in order to determine if a platform_device is also an
>>>omap_device, checking the parent is all that is needed.
>>>
>>>Users of this feature are the runtime PM core for OMAP, where we need
>>>to know if a device being passed in is an omap_device or not in order
>>>to know whether to call the omap_device API with it.
>>>
>>>In addition, all omap_devices will now show up under /sys/devices/omap
>>>instead of /sys/devices/platform
>
> Hello Kevin,
>
> Couple of minor queries/comments below.

Hi Thara, 

Thanks for the review.

>>>
>>>Signed-off-by: Kevin Hilman <khilman@ti.com>
>>>---
>>> arch/arm/plat-omap/include/plat/omap_device.h |    2 ++
>>> arch/arm/plat-omap/omap_device.c              |   18 ++++++++++++++++++
>>> 2 files changed, 20 insertions(+), 0 deletions(-)
>>>
>>>diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-
>>>omap/include/plat/omap_device.h
>>>index bad4c3d..26d0c10 100644
>>>--- a/arch/arm/plat-omap/include/plat/omap_device.h
>>>+++ b/arch/arm/plat-omap/include/plat/omap_device.h
>>>@@ -36,6 +36,8 @@
>>>
>>> #include <plat/omap_hwmod.h>
>>>
>>>+extern struct device omap_bus;
>>>+
>
> Why is this extern declaration needed? Is it later on
> to check in runtime pm code pdev->dev.parent == omap_bus??

Yes.

>>> /* omap_device._state values */
>>> #define OMAP_DEVICE_STATE_UNKNOWN	0
>>> #define OMAP_DEVICE_STATE_ENABLED	1
>>>diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
>>>index 7f05f49..3e215fa 100644
>>>--- a/arch/arm/plat-omap/omap_device.c
>>>+++ b/arch/arm/plat-omap/omap_device.c
>>>@@ -463,8 +463,11 @@ int omap_early_device_register(struct omap_device *od)
>>>  */
>>> int omap_device_register(struct omap_device *od)
>>> {
>>>+	struct platform_device *pdev = &od->pdev;
>>>+
>>> 	pr_debug("omap_device: %s: registering\n", od->pdev.name);
>>>
>>>+	pdev->dev.parent = &omap_bus;
>
> What if device_register(&omap_bus) has returned an
> error? Do we still go ahead with assigning omap_bus
> as parent?

Good point.  I followed the lead of the platform_bus here which does the
same thing, and bascially assumes that device_register() does not fail.

I'll take a look to see if this could be handled cleaner.  Thanks.

>>> 	return platform_device_register(&od->pdev);
>>> }
>>>
>>>@@ -737,3 +740,18 @@ int omap_device_enable_clocks(struct omap_device *od)
>>> 	/* XXX pass along return value here? */
>>> 	return 0;
>>> }
>>>+
>>>+struct device omap_bus = {
>>>+	.init_name	= "omap",
>>>+};
>>>+
>>>+static int __init omap_device_init(void)
>>>+{
>>>+	int error = 0;
>
> Is the initialization to 0 needed?

Nope

>>>+
>>>+	printk("%s:\n", __func__);
>
> Spurious printk???

Yup.

Oops, I cleaned those two up locally but sent the wrong version.

Thanks for catching,

Kevin