[PATCH] platform: Fix platform device resource linking

[PATCH] platform: Fix platform device resource linking

[Pantelis Antoniou]

Platform device removal uncovered a number of problems with
the way resources are handled in the core platform code.

Resources now form child/parent linkages and this requires
proper linking of the resources. On top of that the OF core
directly creates it's own platform devices. Simplify things
by providing helper functions that manage the linking properly.

Two functions are provided:

platform_device_link_resources(), which links all the
linkable resources (if not already linked).

and platform_device_unlink_resources(), which unlinks all the
resources.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/base/platform.c         | 124 +++++++++++++++++++++++++++-------------
 include/linux/platform_device.h |   4 ++
 2 files changed, 87 insertions(+), 41 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index c0b8df3..dab9552 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -270,6 +270,80 @@ int platform_device_add_data(struct platform_device *pdev, const void *data,
 }
 EXPORT_SYMBOL_GPL(platform_device_add_data);
 
+static struct resource *platform_device_parent_resource(
+		struct platform_device *pdev, struct resource *r)
+{
+	unsigned long type;
+
+	if (r->parent)
+		return r->parent;
+
+	type = resource_type(r);
+	switch (type) {
+		case IORESOURCE_MEM:
+			return &iomem_resource;
+		case IORESOURCE_IO:
+			return &ioport_resource;
+		/* TODO: What about the other resources? */
+		default:
+			break;
+	}
+	pr_debug("%s: no parent for resource %p type 0x%lx\n",
+			dev_name(&pdev->dev), r, resource_type(r));
+	return NULL;
+}
+
+int platform_device_unlink_resources(struct platform_device *pdev)
+{
+	struct resource *r;
+	int i;
+
+	for (i = pdev->num_resources - 1; i >= 0; i--) {
+		r = &pdev->resource[i];
+		if (r->parent == NULL)
+			continue;
+		release_resource(r);
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(platform_device_unlink_resources);
+
+int platform_device_link_resources(struct platform_device *pdev)
+{
+	int i;
+	struct resource *p, *r;
+
+	for (i = 0; i < pdev->num_resources; i++) {
+		r = &pdev->resource[i];
+
+		if (r->name == NULL)
+			r->name = dev_name(&pdev->dev);
+
+		/* already linked */
+		if (r->parent != NULL)
+			continue;
+
+		p = platform_device_parent_resource(pdev, r);
+		if (p && insert_resource(p, r)) {
+			pr_err("%s: failed to claim resource %d\n",
+			       dev_name(&pdev->dev), i);
+			goto fail;
+		}
+	}
+
+	return 0;
+
+fail:
+	while (--i >= 0) {
+		r = &pdev->resource[i];
+		if (r->parent == NULL)
+			continue;
+		release_resource(r);
+	}
+	return -EBUSY;
+}
+EXPORT_SYMBOL_GPL(platform_device_link_resources);
+
 /**
  * platform_device_add - add a platform device to device hierarchy
  * @pdev: platform device we're adding
@@ -279,7 +353,7 @@ EXPORT_SYMBOL_GPL(platform_device_add_data);
  */
 int platform_device_add(struct platform_device *pdev)
 {
-	int i, ret;
+	int ret;
 
 	if (!pdev)
 		return -EINVAL;
@@ -311,28 +385,10 @@ int platform_device_add(struct platform_device *pdev)
 		break;
 	}
 
-	for (i = 0; i < pdev->num_resources; i++) {
-		struct resource *p, *r = &pdev->resource[i];
-
-		if (r->name == NULL)
-			r->name = dev_name(&pdev->dev);
-
-		p = r->parent;
-		if (!p) {
-			if (resource_type(r) == IORESOURCE_MEM)
-				p = &iomem_resource;
-			else if (resource_type(r) == IORESOURCE_IO)
-				p = &ioport_resource;
-		}
-
-		if (p && insert_resource(p, r)) {
-			printk(KERN_ERR
-			       "%s: failed to claim resource %d\n",
-			       dev_name(&pdev->dev), i);
-			ret = -EBUSY;
-			goto failed;
-		}
-	}
+	/* make sure the resources are linked properly */
+	ret = platform_device_link_resources(pdev);
+	if (ret != 0)
+		goto failed_res;
 
 	pr_debug("Registering platform device '%s'. Parent at %s\n",
 		 dev_name(&pdev->dev), dev_name(pdev->dev.parent));
@@ -341,20 +397,14 @@ int platform_device_add(struct platform_device *pdev)
 	if (ret == 0)
 		return ret;
 
- failed:
+	platform_device_unlink_resources(pdev);
+
+ failed_res:
 	if (pdev->id_auto) {
 		ida_simple_remove(&platform_devid_ida, pdev->id);
 		pdev->id = PLATFORM_DEVID_AUTO;
 	}
 
-	while (--i >= 0) {
-		struct resource *r = &pdev->resource[i];
-		unsigned long type = resource_type(r);
-
-		if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
-			release_resource(r);
-	}
-
  err_out:
 	return ret;
 }
@@ -370,8 +420,6 @@ EXPORT_SYMBOL_GPL(platform_device_add);
  */
 void platform_device_del(struct platform_device *pdev)
 {
-	int i;
-
 	if (pdev) {
 		device_del(&pdev->dev);
 
@@ -380,13 +428,7 @@ void platform_device_del(struct platform_device *pdev)
 			pdev->id = PLATFORM_DEVID_AUTO;
 		}
 
-		for (i = 0; i < pdev->num_resources; i++) {
-			struct resource *r = &pdev->resource[i];
-			unsigned long type = resource_type(r);
-
-			if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
-				release_resource(r);
-		}
+		platform_device_unlink_resources(pdev);
 	}
 }
 EXPORT_SYMBOL_GPL(platform_device_del);
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index a9ded9a..e48c2d5 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -293,4 +293,8 @@ extern int platform_pm_restore(struct device *dev);
 #define USE_PLATFORM_PM_SLEEP_OPS
 #endif
 
+/* helper functions for resource list managment */
+int platform_device_unlink_resources(struct platform_device *pdev);
+int platform_device_link_resources(struct platform_device *pdev);
+
 #endif /* _PLATFORM_DEVICE_H_ */
-- 
1.7.12

Re: [PATCH] platform: Fix platform device resource linking

[Greg Kroah-Hartman]

On Fri, Jan 04, 2013 at 12:31:10AM +0200, Pantelis Antoniou wrote:
> Platform device removal uncovered a number of problems with
> the way resources are handled in the core platform code.
> 
> Resources now form child/parent linkages and this requires
> proper linking of the resources. On top of that the OF core
> directly creates it's own platform devices. Simplify things
> by providing helper functions that manage the linking properly.
> 
> Two functions are provided:
> 
> platform_device_link_resources(), which links all the
> linkable resources (if not already linked).
> 
> and platform_device_unlink_resources(), which unlinks all the
> resources.

Who would call these functions, and why?

And why have we never seen problems with removing platform devices
previously?

thanks,

greg k-h

Re: [PATCH] platform: Fix platform device resource linking

[Pantelis Antoniou]

Hi Greg,

On Jan 4, 2013, at 12:40 AM, Greg Kroah-Hartman wrote:

> On Fri, Jan 04, 2013 at 12:31:10AM +0200, Pantelis Antoniou wrote:
>> Platform device removal uncovered a number of problems with
>> the way resources are handled in the core platform code.
>> 
>> Resources now form child/parent linkages and this requires
>> proper linking of the resources. On top of that the OF core
>> directly creates it's own platform devices. Simplify things
>> by providing helper functions that manage the linking properly.
>> 
>> Two functions are provided:
>> 
>> platform_device_link_resources(), which links all the
>> linkable resources (if not already linked).
>> 
>> and platform_device_unlink_resources(), which unlinks all the
>> resources.
> 
> Who would call these functions, and why?
> 
> And why have we never seen problems with removing platform devices
> previously?
> 

Have you tried removing devices that were created via DT and 
not using platform data?

> thanks,
> 
> greg k-h

Regards

-- Pantelis

Re: [PATCH] platform: Fix platform device resource linking

[Greg Kroah-Hartman]

On Fri, Jan 04, 2013 at 12:43:46AM +0200, Pantelis Antoniou wrote:
> Hi Greg,
> 
> On Jan 4, 2013, at 12:40 AM, Greg Kroah-Hartman wrote:
> 
> > On Fri, Jan 04, 2013 at 12:31:10AM +0200, Pantelis Antoniou wrote:
> >> Platform device removal uncovered a number of problems with
> >> the way resources are handled in the core platform code.
> >> 
> >> Resources now form child/parent linkages and this requires
> >> proper linking of the resources. On top of that the OF core
> >> directly creates it's own platform devices. Simplify things
> >> by providing helper functions that manage the linking properly.
> >> 
> >> Two functions are provided:
> >> 
> >> platform_device_link_resources(), which links all the
> >> linkable resources (if not already linked).
> >> 
> >> and platform_device_unlink_resources(), which unlinks all the
> >> resources.
> > 
> > Who would call these functions, and why?
> > 
> > And why have we never seen problems with removing platform devices
> > previously?
> > 
> 
> Have you tried removing devices that were created via DT and 
> not using platform data?

Don't you think that answering two questions with another question as
something that isn't very helpful?  :)

Dropped from my queue, please resend when you can provide the needed
information.

thanks,

greg k-h

Re: [PATCH] platform: Fix platform device resource linking

[Pantelis Antoniou]

On Jan 17, 2013, at 6:31 PM, Greg Kroah-Hartman wrote:

> On Fri, Jan 04, 2013 at 12:43:46AM +0200, Pantelis Antoniou wrote:
>> Hi Greg,
>> 
>> On Jan 4, 2013, at 12:40 AM, Greg Kroah-Hartman wrote:
>> 
>>> On Fri, Jan 04, 2013 at 12:31:10AM +0200, Pantelis Antoniou wrote:
>>>> Platform device removal uncovered a number of problems with
>>>> the way resources are handled in the core platform code.
>>>> 
>>>> Resources now form child/parent linkages and this requires
>>>> proper linking of the resources. On top of that the OF core
>>>> directly creates it's own platform devices. Simplify things
>>>> by providing helper functions that manage the linking properly.
>>>> 
>>>> Two functions are provided:
>>>> 
>>>> platform_device_link_resources(), which links all the
>>>> linkable resources (if not already linked).
>>>> 
>>>> and platform_device_unlink_resources(), which unlinks all the
>>>> resources.
>>> 
>>> Who would call these functions, and why?
>>> 
>>> And why have we never seen problems with removing platform devices
>>> previously?
>>> 
>> 
>> Have you tried removing devices that were created via DT and 
>> not using platform data?
> 
> Don't you think that answering two questions with another question as
> something that isn't very helpful?  :)
> 
> Dropped from my queue, please resend when you can provide the needed
> information.
> 
> thanks,
> 

That's not very nice, but anyway...

In a nutshell, we have to exercise the platform device subsystem, in ways
that never happened before, so all sorts of weird bugs that no-one has seen
before.

In that case, the code path for creating platform devices from DT is
not the same as the one that is used when creating platform device from
a board file.

Take a look at platform_device_add() in drivers/base/platform.c and
drivers/of/device.c

platform_device_add() properly links the resources by using insert_resource(),
while of_device_add() doesn't bother with it. Now when we try to unregister
the device everything will is fine in the platform device case, since the resources
are linked properly. In the DT case we will crash spectacularly in 
__release_resource at the first line (p = &old->parent->child), since no-one bothered
to fill in the parent pointer.

That's what the patches do; first the code in platform_device_add() that perform the
resource linking is factored as a separate function (platform_device_link_resources).

The platform_device_unlink_resources() function, just makes things more clearer. 

> greg k-h

Regards

-- Pantelis

Re: [PATCH] platform: Fix platform device resource linking

[Greg Kroah-Hartman]

On Thu, Jan 17, 2013 at 06:50:19PM +0200, Pantelis Antoniou wrote:
> 
> On Jan 17, 2013, at 6:31 PM, Greg Kroah-Hartman wrote:
> 
> > On Fri, Jan 04, 2013 at 12:43:46AM +0200, Pantelis Antoniou wrote:
> >> Hi Greg,
> >> 
> >> On Jan 4, 2013, at 12:40 AM, Greg Kroah-Hartman wrote:
> >> 
> >>> On Fri, Jan 04, 2013 at 12:31:10AM +0200, Pantelis Antoniou wrote:
> >>>> Platform device removal uncovered a number of problems with
> >>>> the way resources are handled in the core platform code.
> >>>> 
> >>>> Resources now form child/parent linkages and this requires
> >>>> proper linking of the resources. On top of that the OF core
> >>>> directly creates it's own platform devices. Simplify things
> >>>> by providing helper functions that manage the linking properly.
> >>>> 
> >>>> Two functions are provided:
> >>>> 
> >>>> platform_device_link_resources(), which links all the
> >>>> linkable resources (if not already linked).
> >>>> 
> >>>> and platform_device_unlink_resources(), which unlinks all the
> >>>> resources.
> >>> 
> >>> Who would call these functions, and why?
> >>> 
> >>> And why have we never seen problems with removing platform devices
> >>> previously?
> >>> 
> >> 
> >> Have you tried removing devices that were created via DT and 
> >> not using platform data?
> > 
> > Don't you think that answering two questions with another question as
> > something that isn't very helpful?  :)
> > 
> > Dropped from my queue, please resend when you can provide the needed
> > information.
> > 
> > thanks,
> > 
> 
> That's not very nice, but anyway...

What would you have me do if you were in my shoes?

> In a nutshell, we have to exercise the platform device subsystem, in ways
> that never happened before, so all sorts of weird bugs that no-one has seen
> before.

Why do you have to do this?  What are you doing that is so different
from everyone else?  What drivers are you using that trigger this type
of thing?

> In that case, the code path for creating platform devices from DT is
> not the same as the one that is used when creating platform device from
> a board file.

Why not?

> Take a look at platform_device_add() in drivers/base/platform.c and
> drivers/of/device.c
> 
> platform_device_add() properly links the resources by using insert_resource(),
> while of_device_add() doesn't bother with it. Now when we try to unregister
> the device everything will is fine in the platform device case, since the resources
> are linked properly. In the DT case we will crash spectacularly in 
> __release_resource at the first line (p = &old->parent->child), since no-one bothered
> to fill in the parent pointer.

So, isn't that a bug in the DT case?  A device always has to have a
parent, as you have found out.  Hm, maybe not "root" devices, but you
don't have many of those, right?

> That's what the patches do; first the code in platform_device_add() that perform the
> resource linking is factored as a separate function (platform_device_link_resources).
> 
> The platform_device_unlink_resources() function, just makes things more clearer. 

But you added a new function that no one calls, which is what I am
objecting to.

thanks,

greg k-h

Re: [PATCH] platform: Fix platform device resource linking

[Pantelis Antoniou]

[-- Attachment #1: Type: text/plain, Size: 4724 bytes --]

Hi Greg,

On Jan 17, 2013, at 7:07 PM, Greg Kroah-Hartman wrote:

> On Thu, Jan 17, 2013 at 06:50:19PM +0200, Pantelis Antoniou wrote:
>> 
>> On Jan 17, 2013, at 6:31 PM, Greg Kroah-Hartman wrote:
>> 
>>> On Fri, Jan 04, 2013 at 12:43:46AM +0200, Pantelis Antoniou wrote:
>>>> Hi Greg,
>>>> 
>>>> On Jan 4, 2013, at 12:40 AM, Greg Kroah-Hartman wrote:
>>>> 
>>>>> On Fri, Jan 04, 2013 at 12:31:10AM +0200, Pantelis Antoniou wrote:
>>>>>> Platform device removal uncovered a number of problems with
>>>>>> the way resources are handled in the core platform code.
>>>>>> 
>>>>>> Resources now form child/parent linkages and this requires
>>>>>> proper linking of the resources. On top of that the OF core
>>>>>> directly creates it's own platform devices. Simplify things
>>>>>> by providing helper functions that manage the linking properly.
>>>>>> 
>>>>>> Two functions are provided:
>>>>>> 
>>>>>> platform_device_link_resources(), which links all the
>>>>>> linkable resources (if not already linked).
>>>>>> 
>>>>>> and platform_device_unlink_resources(), which unlinks all the
>>>>>> resources.
>>>>> 
>>>>> Who would call these functions, and why?
>>>>> 
>>>>> And why have we never seen problems with removing platform devices
>>>>> previously?
>>>>> 
>>>> 
>>>> Have you tried removing devices that were created via DT and 
>>>> not using platform data?
>>> 
>>> Don't you think that answering two questions with another question as
>>> something that isn't very helpful?  :)
>>> 
>>> Dropped from my queue, please resend when you can provide the needed
>>> information.
>>> 
>>> thanks,
>>> 
>> 
>> That's not very nice, but anyway...
> 
> What would you have me do if you were in my shoes?
> 

Point.

>> In a nutshell, we have to exercise the platform device subsystem, in ways
>> that never happened before, so all sorts of weird bugs that no-one has seen
>> before.
> 
> Why do you have to do this?  What are you doing that is so different
> from everyone else?  What drivers are you using that trigger this type
> of thing?
> 

This is all part of a larger patchset; I guess you weren't directly CCed.
The name of the patchset is 'Introducing Device Tree Overlays' and is a
method of changing the live device tree and have the changes reflected to
the kernel's state.

As I mentioned earlier, device tree platform devices were never removed
up until now; the DT statically described the hardware of a board and there
wasn't any way to remove a device.

As part of the Device Tree Overlay functionality, an overlay should be possible
to be removed. The crash happens when a platform device created by DT
is removed. 

>> In that case, the code path for creating platform devices from DT is
>> not the same as the one that is used when creating platform device from
>> a board file.
> 
> Why not?
> 

Because while DT creates platform devices, it doesn't use the platform device
methods to do so, rather than builds the platform device itself. This is 
something that was overlooked.

>> Take a look at platform_device_add() in drivers/base/platform.c and
>> drivers/of/device.c
>> 
>> platform_device_add() properly links the resources by using insert_resource(),
>> while of_device_add() doesn't bother with it. Now when we try to unregister
>> the device everything will is fine in the platform device case, since the resources
>> are linked properly. In the DT case we will crash spectacularly in 
>> __release_resource at the first line (p = &old->parent->child), since no-one bothered
>> to fill in the parent pointer.
> 
> So, isn't that a bug in the DT case?  A device always has to have a
> parent, as you have found out.  Hm, maybe not "root" devices, but you
> don't have many of those, right?
> 

It's not about a device parent, it's about a resource parent. In general resource
handling in the DT world is a big WIP. One step to that direction is to have the
resources properly linked as the rest of the kernel code expects.

>> That's what the patches do; first the code in platform_device_add() that perform the
>> resource linking is factored as a separate function (platform_device_link_resources).
>> 
>> The platform_device_unlink_resources() function, just makes things more clearer. 
> 
> But you added a new function that no one calls, which is what I am
> objecting to.
> 

Looking at my mailer, it looks like the patch that uses this got dropped since it
is such a small patch.

That is my mistake and apologize for the severe confusion.

The patch in question is attached; I will sent it along by itself too.

> thanks,
> 
> greg k-h

Regards

-- Pantelis


[-- Attachment #2: 0001-Link-platform-device-resources-properly.patch --]
[-- Type: application/octet-stream, Size: 1023 bytes --]

From a17f5ea9601938d492cd7d236826d9a6dba6d79d Mon Sep 17 00:00:00 2001
From: Pantelis Antoniou <panto@antoniou-consulting.com>
Date: Fri, 28 Dec 2012 11:39:29 +0200
Subject: [PATCH] Link platform device resources properly.

The resources of the platform devices created by the OF core were
not properly linked. Make sure that they are, so that we don't get
any crashes when trying to remove the device.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 4c74e4f..d75fcaf 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -62,6 +62,9 @@ int of_device_add(struct platform_device *ofdev)
 	if (!ofdev->dev.parent)
 		set_dev_node(&ofdev->dev, of_node_to_nid(ofdev->dev.of_node));
 
+	/* make sure we add the resources to the appropriate lists */
+	platform_device_link_resources(ofdev);
+
 	return device_add(&ofdev->dev);
 }
 
-- 
1.7.12

Re: [PATCH] platform: Fix platform device resource linking

[Greg Kroah-Hartman]

On Thu, Jan 17, 2013 at 07:27:21PM +0200, Pantelis Antoniou wrote:
> >> In a nutshell, we have to exercise the platform device subsystem, in ways
> >> that never happened before, so all sorts of weird bugs that no-one has seen
> >> before.
> > 
> > Why do you have to do this?  What are you doing that is so different
> > from everyone else?  What drivers are you using that trigger this type
> > of thing?
> > 
> 
> This is all part of a larger patchset; I guess you weren't directly CCed.
> The name of the patchset is 'Introducing Device Tree Overlays' and is a
> method of changing the live device tree and have the changes reflected to
> the kernel's state.

Ok, no wonder I was confused :)

How about cc:ing me on the next round of these patches, all of the,
which will give me the proper background as to what is going on?

> >> In that case, the code path for creating platform devices from DT is
> >> not the same as the one that is used when creating platform device from
> >> a board file.
> > 
> > Why not?
> > 
> 
> Because while DT creates platform devices, it doesn't use the platform device
> methods to do so, rather than builds the platform device itself. This is 
> something that was overlooked.

Can't this be fixed?  What does the platform device core need to do to
resolve this?

thanks,

greg k-h

Re: [PATCH] platform: Fix platform device resource linking

[Pantelis Antoniou]

Hi Greg,

On Jan 18, 2013, at 5:00 AM, Greg Kroah-Hartman wrote:

> On Thu, Jan 17, 2013 at 07:27:21PM +0200, Pantelis Antoniou wrote:
>>>> In a nutshell, we have to exercise the platform device subsystem, in ways
>>>> that never happened before, so all sorts of weird bugs that no-one has seen
>>>> before.
>>> 
>>> Why do you have to do this?  What are you doing that is so different
>>> from everyone else?  What drivers are you using that trigger this type
>>> of thing?
>>> 
>> 
>> This is all part of a larger patchset; I guess you weren't directly CCed.
>> The name of the patchset is 'Introducing Device Tree Overlays' and is a
>> method of changing the live device tree and have the changes reflected to
>> the kernel's state.
> 
> Ok, no wonder I was confused :)
> 
> How about cc:ing me on the next round of these patches, all of the,
> which will give me the proper background as to what is going on?
> 

Will do. I'm still waiting for some feedback from the DT maintainers, but
I will make sure that you will be CCed on the next revision.

You can of course take a look at it and comment on the current version too. 
  
>>>> In that case, the code path for creating platform devices from DT is
>>>> not the same as the one that is used when creating platform device from
>>>> a board file.
>>> 
>>> Why not?
>>> 
>> 
>> Because while DT creates platform devices, it doesn't use the platform device
>> methods to do so, rather than builds the platform device itself. This is 
>> something that was overlooked.
> 
> Can't this be fixed?  What does the platform device core need to do to
> resolve this?
> 

Hmm, due to historical reasons the two ways of creating platform devices
have diverged. The core of the issue is that while OF creates platform devices
it does so in it's own way.

Take a look at of_device_add()/platform_device_add(), 
of_device_register()/platform_device_register() for example.

It is pretty obvious some bits in platform_device_* are more recent and are missing
in of_device_*.

It might make sense for the of_device_* functions that are duplicating 
platform_device_* functions to be removed, and their functionality to 
be subsumed by platform_device_*, possibly by calling some helper functions
in drivers/of/ when of_node is not NULL. The of_device_* functions can be 
replaced by a direct call to platform_device_* via a define (until all of
the callers get converted).

The problem with doing anything like this would be that a whole bunch of
devices/arches depend on DT, and if anything breaks there will be a lot of
angry people with pitchforks after the culprit.

So without the full force of a core maintainer behind such a move, people
are reluctant to do so.   

> thanks,
> 
> greg k-h

Regards

-- Pantelis

Re: [PATCH] platform: Fix platform device resource linking

[Greg Kroah-Hartman]

On Fri, Jan 18, 2013 at 11:05:14AM +0200, Pantelis Antoniou wrote:
> It might make sense for the of_device_* functions that are duplicating 
> platform_device_* functions to be removed, and their functionality to 
> be subsumed by platform_device_*, possibly by calling some helper functions
> in drivers/of/ when of_node is not NULL. The of_device_* functions can be 
> replaced by a direct call to platform_device_* via a define (until all of
> the callers get converted).

That sounds reasonable.

> The problem with doing anything like this would be that a whole bunch of
> devices/arches depend on DT, and if anything breaks there will be a lot of
> angry people with pitchforks after the culprit.

That's nothing new, we are totally used to that happening :)

> So without the full force of a core maintainer behind such a move, people
> are reluctant to do so.   

Send patches if you want to do this, no need for the maintainer to do it
(hint, I will not as I don't even have a system that this type of code
runs on.)

thanks,

greg k-h

Re: [PATCH] platform: Fix platform device resource linking

[Pantelis Antoniou]

Hi Greg,

On Jan 18, 2013, at 9:47 PM, Greg Kroah-Hartman wrote:

> On Fri, Jan 18, 2013 at 11:05:14AM +0200, Pantelis Antoniou wrote:
>> It might make sense for the of_device_* functions that are duplicating 
>> platform_device_* functions to be removed, and their functionality to 
>> be subsumed by platform_device_*, possibly by calling some helper functions
>> in drivers/of/ when of_node is not NULL. The of_device_* functions can be 
>> replaced by a direct call to platform_device_* via a define (until all of
>> the callers get converted).
> 
> That sounds reasonable.
> 
>> The problem with doing anything like this would be that a whole bunch of
>> devices/arches depend on DT, and if anything breaks there will be a lot of
>> angry people with pitchforks after the culprit.
> 
> That's nothing new, we are totally used to that happening :)
> 

I'm fearful of the angry mob with torches and pitchforks after me, if something
gets broken :)

>> So without the full force of a core maintainer behind such a move, people
>> are reluctant to do so.   
> 
> Send patches if you want to do this, no need for the maintainer to do it
> (hint, I will not as I don't even have a system that this type of code
> runs on.)
> 

OK, maybe next week, when I think of this a little bit more.

> thanks,
> 
> greg k-h

Regards

-- Pantelis

Re: [PATCH] platform: Fix platform device resource linking

[Grant Likely]

On Fri, 18 Jan 2013 11:05:14 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> Hi Greg,
> 
> On Jan 18, 2013, at 5:00 AM, Greg Kroah-Hartman wrote:
> 
> > On Thu, Jan 17, 2013 at 07:27:21PM +0200, Pantelis Antoniou wrote:
> >>>> In a nutshell, we have to exercise the platform device subsystem, in ways
> >>>> that never happened before, so all sorts of weird bugs that no-one has seen
> >>>> before.
> >>> 
> >>> Why do you have to do this?  What are you doing that is so different
> >>> from everyone else?  What drivers are you using that trigger this type
> >>> of thing?
> >>> 
> >> 
> >> This is all part of a larger patchset; I guess you weren't directly CCed.
> >> The name of the patchset is 'Introducing Device Tree Overlays' and is a
> >> method of changing the live device tree and have the changes reflected to
> >> the kernel's state.
> > 
> > Ok, no wonder I was confused :)
> > 
> > How about cc:ing me on the next round of these patches, all of the,
> > which will give me the proper background as to what is going on?
> > 
> 
> Will do. I'm still waiting for some feedback from the DT maintainers, but
> I will make sure that you will be CCed on the next revision.
> 
> You can of course take a look at it and comment on the current version too. 
>   
> >>>> In that case, the code path for creating platform devices from DT is
> >>>> not the same as the one that is used when creating platform device from
> >>>> a board file.
> >>> 
> >>> Why not?
> >>> 
> >> 
> >> Because while DT creates platform devices, it doesn't use the platform device
> >> methods to do so, rather than builds the platform device itself. This is 
> >> something that was overlooked.
> > 
> > Can't this be fixed?  What does the platform device core need to do to
> > resolve this?
> > 
> 
> Hmm, due to historical reasons the two ways of creating platform devices
> have diverged. The core of the issue is that while OF creates platform devices
> it does so in it's own way.

It's actually the other way around. The DT code path used to be a
completely separate of_platform_bus_type that didn't share any code with
platform_bus_type. So in fact, the code patches have converged instead
of diverged.

When I merged the paths there were some breakages that prevented me from
using platform_device_add() directly. Most of those are now gone and

I've got a patch in my tree which makes the OF code use
platform_device_add(). That makes this patch series unnecessary. The
patch is currently in linux-next. Assuming I don't run into any major
problems it will be merged in v3.9

> The problem with doing anything like this would be that a whole bunch of
> devices/arches depend on DT, and if anything breaks there will be a lot of
> angry people with pitchforks after the culprit.

Pitchforks? pish. It's the torches that are dangerous.

g.

Re: [PATCH] platform: Fix platform device resource linking

[Pantelis Antoniou]

Hi Grant,

On Feb 9, 2013, at 12:02 AM, Grant Likely wrote:

> On Fri, 18 Jan 2013 11:05:14 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>> Hi Greg,
>> 
>> On Jan 18, 2013, at 5:00 AM, Greg Kroah-Hartman wrote:
>> 
>>> On Thu, Jan 17, 2013 at 07:27:21PM +0200, Pantelis Antoniou wrote:
>>>>>> In a nutshell, we have to exercise the platform device subsystem, in ways
>>>>>> that never happened before, so all sorts of weird bugs that no-one has seen
>>>>>> before.
>>>>> 
>>>>> Why do you have to do this?  What are you doing that is so different
>>>>> from everyone else?  What drivers are you using that trigger this type
>>>>> of thing?
>>>>> 
>>>> 
>>>> This is all part of a larger patchset; I guess you weren't directly CCed.
>>>> The name of the patchset is 'Introducing Device Tree Overlays' and is a
>>>> method of changing the live device tree and have the changes reflected to
>>>> the kernel's state.
>>> 
>>> Ok, no wonder I was confused :)
>>> 
>>> How about cc:ing me on the next round of these patches, all of the,
>>> which will give me the proper background as to what is going on?
>>> 
>> 
>> Will do. I'm still waiting for some feedback from the DT maintainers, but
>> I will make sure that you will be CCed on the next revision.
>> 
>> You can of course take a look at it and comment on the current version too. 
>> 
>>>>>> In that case, the code path for creating platform devices from DT is
>>>>>> not the same as the one that is used when creating platform device from
>>>>>> a board file.
>>>>> 
>>>>> Why not?
>>>>> 
>>>> 
>>>> Because while DT creates platform devices, it doesn't use the platform device
>>>> methods to do so, rather than builds the platform device itself. This is 
>>>> something that was overlooked.
>>> 
>>> Can't this be fixed?  What does the platform device core need to do to
>>> resolve this?
>>> 
>> 
>> Hmm, due to historical reasons the two ways of creating platform devices
>> have diverged. The core of the issue is that while OF creates platform devices
>> it does so in it's own way.
> 
> It's actually the other way around. The DT code path used to be a
> completely separate of_platform_bus_type that didn't share any code with
> platform_bus_type. So in fact, the code patches have converged instead
> of diverged.
> 
> When I merged the paths there were some breakages that prevented me from
> using platform_device_add() directly. Most of those are now gone and
> 
> I've got a patch in my tree which makes the OF code use
> platform_device_add(). That makes this patch series unnecessary. The
> patch is currently in linux-next. Assuming I don't run into any major
> problems it will be merged in v3.9
> 

I'm fine with this, as long as nothing breaks. I'll wait until it lands in
mainline to test it.

>> The problem with doing anything like this would be that a whole bunch of
>> devices/arches depend on DT, and if anything breaks there will be a lot of
>> angry people with pitchforks after the culprit.
> 
> Pitchforks? pish. It's the torches that are dangerous.
> 

Fire bad. Got it.

> g.

What about the other (more substantial) patches that are in limbo? Any word
on them?

Regards

-- Pantelis