[PATCH 1/5] ARM: imx: Remove references to platform_bus in mxc code

[PATCH 1/5] ARM: imx: Remove references to platform_bus in mxc code

[Pawel Moll]

The bus devices created to be parents for other peripherals
were using platform_bus as a parent, not being platform
devices themselves. Remove the references, making them
virtual devices instead.

Cc: Shawn Guo <shawn.guo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Signed-off-by: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
---

This patch is a part of effort to remove references to platform_bus
and make it static.

Shawn, Sascha - could you, please, have a look if such change
is acceptable for you?

 arch/arm/mach-imx/devices/devices.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/mach-imx/devices/devices.c b/arch/arm/mach-imx/devices/devices.c
index 1b4366a..8eab544 100644
--- a/arch/arm/mach-imx/devices/devices.c
+++ b/arch/arm/mach-imx/devices/devices.c
@@ -24,12 +24,10 @@
 
 struct device mxc_aips_bus = {
 	.init_name	= "mxc_aips",
-	.parent		= &platform_bus,
 };
 
 struct device mxc_ahb_bus = {
 	.init_name	= "mxc_ahb",
-	.parent		= &platform_bus,
 };
 
 int __init mxc_device_init(void)
-- 
1.9.1

[PATCH 2/5] char: tile-srom: Remove reference to platform_bus

[Pawel Moll]

The code was creating "srom" class devices using
platform_bus as a parent. As they are not really
platform devices, make them virtual, using NULL instead.

Cc: Chris Metcalf <cmetcalf@tilera.com>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 drivers/char/tile-srom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tile-srom.c b/drivers/char/tile-srom.c
index bd37747..be88699 100644
--- a/drivers/char/tile-srom.c
+++ b/drivers/char/tile-srom.c
@@ -350,7 +350,7 @@ static int srom_setup_minor(struct srom_dev *srom, int index)
 		       SROM_PAGE_SIZE_OFF, sizeof(srom->page_size)) < 0)
 		return -EIO;
 
-	dev = device_create(srom_class, &platform_bus,
+	dev = device_create(srom_class, NULL,
 			    MKDEV(srom_major, index), srom, "%d", index);
 	return PTR_ERR_OR_ZERO(dev);
 }
-- 
1.9.1

Re: [PATCH 2/5] char: tile-srom: Remove reference to platform_bus

[Chris Metcalf]

On 7/25/2014 10:23 AM, Pawel Moll wrote:
> The code was creating "srom" class devices using
> platform_bus as a parent. As they are not really
> platform devices, make them virtual, using NULL instead.
>
> Cc: Chris Metcalf<cmetcalf-kv+TWInifGbQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Pawel Moll<pawel.moll-5wv7dgnIgG8@public.gmane.org>
> ---
>   drivers/char/tile-srom.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Can you clarify the point of this change a bit?  The SROM devices
in question are real devices (bits of silicon on the processor die), not
some kind of virtual construct.  In addition, we also have user binaries
in the wild that know to look for /sys/devices/platform/srom/ paths,
so I'm pretty reluctant to change this path without good reason.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH 2/5] char: tile-srom: Remove reference to platform_bus

[Greg Kroah-Hartman]

On Thu, Jul 31, 2014 at 04:24:37PM -0400, Chris Metcalf wrote:
> On 7/25/2014 10:23 AM, Pawel Moll wrote:
> >The code was creating "srom" class devices using
> >platform_bus as a parent. As they are not really
> >platform devices, make them virtual, using NULL instead.
> >
> >Cc: Chris Metcalf<cmetcalf@tilera.com>
> >Signed-off-by: Pawel Moll<pawel.moll@arm.com>
> >---
> >  drivers/char/tile-srom.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Can you clarify the point of this change a bit?  The SROM devices
> in question are real devices (bits of silicon on the processor die), not
> some kind of virtual construct.

Then tie them to the "real" parent device that they live on, don't try
to hang them under the platform bus where they don't belong.

> In addition, we also have user binaries in the wild that know to look
> for /sys/devices/platform/srom/ paths,

That's never a good idea, you should be iterating over your bus's
devices, to find your devices, not at a specific location within the
/sys/devices/ tree, as that is guaranteed to move around over time.
It's also why we have those symlinks and lists of devices in your bus
directory.

> so I'm pretty reluctant to change this path without good reason.

Because srom is not a platform device, so why would you put it at the
root of the platform device "tree"?

thanks,

greg kh

Re: [PATCH 2/5] char: tile-srom: Remove reference to platform_bus

[Pawel Moll]

On Thu, 2014-07-31 at 21:24 +0100, Chris Metcalf wrote:
> On 7/25/2014 10:23 AM, Pawel Moll wrote:
> > The code was creating "srom" class devices using
> > platform_bus as a parent. As they are not really
> > platform devices, make them virtual, using NULL instead.
> >
> > Cc: Chris Metcalf<cmetcalf-kv+TWInifGbQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Pawel Moll<pawel.moll-5wv7dgnIgG8@public.gmane.org>
> > ---
> >   drivers/char/tile-srom.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Can you clarify the point of this change a bit? 

Theoretically speaking there shouldn't be any need to export the
platform bus root, as all devices should be registered via the platform
API (platform_device_register & co.)

>  The SROM devices
> in question are real devices (bits of silicon on the processor die), not
> some kind of virtual construct.  

... but the driver seems to be accessing then through hypervisor calls
only? One could say that you this make them virtual ;-)

> In addition, we also have user binaries
> in the wild that know to look for /sys/devices/platform/srom/ paths,
> so I'm pretty reluctant to change this path without good reason.

So what is the srom class for then if not for device discovery? And why
do they look for them in the first place? To get relevant character
device's data, if I understand it right?

Maybe you could just register a simple "proper" platform device for all
the sroms and then hang the class devices from it? I can type some code
doing this if it sound reasonably?

Pawel

Re: [PATCH 2/5] char: tile-srom: Remove reference to platform_bus

[Chris Metcalf]

On 8/1/2014 1:21 PM, Pawel Moll wrote:
> On Thu, 2014-07-31 at 21:24 +0100, Chris Metcalf wrote:
>> On 7/25/2014 10:23 AM, Pawel Moll wrote:
>>> The code was creating "srom" class devices using
>>> platform_bus as a parent. As they are not really
>>> platform devices, make them virtual, using NULL instead.
>>>
>>> Cc: Chris Metcalf<cmetcalf-kv+TWInifGbQT0dZR+AlfA@public.gmane.org>
>>> Signed-off-by: Pawel Moll<pawel.moll-5wv7dgnIgG8@public.gmane.org>
>>> ---
>>>    drivers/char/tile-srom.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>> Can you clarify the point of this change a bit?
> Theoretically speaking there shouldn't be any need to export the
> platform bus root, as all devices should be registered via the platform
> API (platform_device_register & co.)

So, perhaps the right fix is to just use platform_device_register()
etc for this device, rather than making it virtual?

I think what I'm missing is why the platform bus isn't the right bus
for this device.  The driver-model/platform.txt doc says it's "used to
integrate peripherals on many system-on-chip processors", which is
how it's being used here.  It's directly addressable via MMIO from
the cores.

I grant you there's some confusion about our use of hypervisor calls
here, but effectively this is just a consequence of our use of the
Tilera hypervisor for kernel isolation; it's more like invoking the
BIOS on an Intel box, than it is about modern virtualization.

The current tilegx series hardware always contains this device, so
there's no FDT-like model for discovering it dynamically; we just always
enable it on tilegx hardware.

>> In addition, we also have user binaries
>> in the wild that know to look for /sys/devices/platform/srom/ paths,
>> so I'm pretty reluctant to change this path without good reason.
> So what is the srom class for then if not for device discovery? And why
> do they look for them in the first place? To get relevant character
> device's data, if I understand it right?
>
> Maybe you could just register a simple "proper" platform device for all
> the sroms and then hang the class devices from it? I can type some code
> doing this if it sound reasonably?

I'm not sure exactly what you mean by device discovery here.  The
subdirectories under /sys/devices/platform/srom/ correspond to partitions
in the SPI-ROM, which are software constructs created by the Tilera hypervisor.
By default we have three, where the first holds boot data that the chip
can use to boot out of hardware, and the other two are smaller partitions
for boot- and user-specific data.  We use the /sys files primarily to get the
page size and sector size for the sroms, and also export other interesting
information like the total size of the particular srom device.

Thank you for volunteering to write a bit of code; if that's the best
way to clarify this for us, fantastic, or else pointing us at existing
good practices or documentation would be great too.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH 2/5] char: tile-srom: Remove reference to platform_bus

[Greg Kroah-Hartman]

On Tue, Aug 05, 2014 at 04:08:40PM -0400, Chris Metcalf wrote:
> On 8/1/2014 1:21 PM, Pawel Moll wrote:
> >On Thu, 2014-07-31 at 21:24 +0100, Chris Metcalf wrote:
> >>On 7/25/2014 10:23 AM, Pawel Moll wrote:
> >>>The code was creating "srom" class devices using
> >>>platform_bus as a parent. As they are not really
> >>>platform devices, make them virtual, using NULL instead.
> >>>
> >>>Cc: Chris Metcalf<cmetcalf-kv+TWInifGbQT0dZR+AlfA@public.gmane.org>
> >>>Signed-off-by: Pawel Moll<pawel.moll-5wv7dgnIgG8@public.gmane.org>
> >>>---
> >>>   drivers/char/tile-srom.c | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>Can you clarify the point of this change a bit?
> >Theoretically speaking there shouldn't be any need to export the
> >platform bus root, as all devices should be registered via the platform
> >API (platform_device_register & co.)
> 
> So, perhaps the right fix is to just use platform_device_register()
> etc for this device, rather than making it virtual?

Sure, that's fine with me if you want to make it a platform device, but
note that a platform device doesn't have a minor associated with it, you
will have to still have your existing srom_class and the rest.  Right
now you aren't creating any real "devices" in the kernel, other than the
one you use for your minor number, which is not a "real" device.

struct srom_dev should be a platform device and then this will get
"easier" overall, sorry I missed that when the original code was
submitted.

thanks,

greg k-h

Re: [PATCH 2/5] char: tile-srom: Remove reference to platform_bus

[Pawel Moll]

On Tue, 2014-08-05 at 21:08 +0100, Chris Metcalf wrote:
> >> In addition, we also have user binaries
> >> in the wild that know to look for /sys/devices/platform/srom/ paths,
> >> so I'm pretty reluctant to change this path without good reason.
> > So what is the srom class for then if not for device discovery? And why
> > do they look for them in the first place? To get relevant character
> > device's data, if I understand it right?
> >
> > Maybe you could just register a simple "proper" platform device for all
> > the sroms and then hang the class devices from it? I can type some code
> > doing this if it sound reasonably?
> 
> I'm not sure exactly what you mean by device discovery here.  



> The
> subdirectories under /sys/devices/platform/srom/ correspond to partitions
> in the SPI-ROM, which are software constructs created by the Tilera hypervisor.
> By default we have three, where the first holds boot data that the chip
> can use to boot out of hardware, and the other two are smaller partitions
> for boot- and user-specific data.  We use the /sys files primarily to get the
> page size and sector size for the sroms, and also export other interesting
> information like the total size of the particular srom device.
> 
> Thank you for volunteering to write a bit of code; if that's the best
> way to clarify this for us, fantastic, or else pointing us at existing
> good practices or documentation would be great too.

I was thinking about something like the following (warning, untested)

8<-------------------------------------------
From c53f0a2492d6cd38d1f82d57916a6528b071e8a8 Mon Sep 17 00:00:00 2001
From: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
Date: Fri, 8 Aug 2014 16:32:58 +0100
Subject: [PATCH] char: tile-srom: Add real platform bus parent

Add a real platform bus device as a parent for
the srom class devices, to prevent non-platform
devices hanging from the bus root.

Signed-off-by: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
---
 drivers/char/tile-srom.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tile-srom.c b/drivers/char/tile-srom.c
index bd37747..7fb0fd5 100644
--- a/drivers/char/tile-srom.c
+++ b/drivers/char/tile-srom.c
@@ -76,6 +76,7 @@ MODULE_LICENSE("GPL");
 
 static int srom_devs;			/* Number of SROM partitions */
 static struct cdev srom_cdev;
+static struct platform_device *srom_parent;
 static struct class *srom_class;
 static struct srom_dev *srom_devices;
 
@@ -350,7 +351,7 @@ static int srom_setup_minor(struct srom_dev *srom, int index)
 		       SROM_PAGE_SIZE_OFF, sizeof(srom->page_size)) < 0)
 		return -EIO;
 
-	dev = device_create(srom_class, &platform_bus,
+	dev = device_create(srom_class, srom_parent,
 			    MKDEV(srom_major, index), srom, "%d", index);
 	return PTR_ERR_OR_ZERO(dev);
 }
@@ -415,6 +416,13 @@ static int srom_init(void)
 	if (result < 0)
 		goto fail_chrdev;
 
+	/* Create a parent device */
+	srom_parent = platform_device_register_simple("srom", -1, NULL, 0);
+	if (IS_ERR(srom_parent)) {
+		result = PTR_ERR(srom_parent);
+		goto fail_pdev;
+	}
+
 	/* Create a sysfs class. */
 	srom_class = class_create(THIS_MODULE, "srom");
 	if (IS_ERR(srom_class)) {
@@ -438,6 +446,8 @@ fail_class:
 		device_destroy(srom_class, MKDEV(srom_major, i));
 	class_destroy(srom_class);
 fail_cdev:
+	platform_device_unregister(srom_parent);
+fail_pdev:
 	cdev_del(&srom_cdev);
 fail_chrdev:
 	unregister_chrdev_region(dev, srom_devs);
@@ -454,6 +464,7 @@ static void srom_cleanup(void)
 		device_destroy(srom_class, MKDEV(srom_major, i));
 	class_destroy(srom_class);
 	cdev_del(&srom_cdev);
+	platform_device_unregister(srom_parent);
 	unregister_chrdev_region(MKDEV(srom_major, 0), srom_devs);
 	kfree(srom_devices);
 }
-- 
1.9.1
8<-------------------------------------------

Would that work for you? Note that it will move the srom class devices
one level deeper in /sys/devices/... hierarchy.

Paweł

Re: [PATCH 2/5] char: tile-srom: Remove reference to platform_bus

[Pawel Moll]

On Fri, 2014-08-08 at 17:34 +0100, Pawel Moll wrote:
> On Tue, 2014-08-05 at 21:08 +0100, Chris Metcalf wrote:
> > >> In addition, we also have user binaries
> > >> in the wild that know to look for /sys/devices/platform/srom/ paths,
> > >> so I'm pretty reluctant to change this path without good reason.
> > > So what is the srom class for then if not for device discovery? And why
> > > do they look for them in the first place? To get relevant character
> > > device's data, if I understand it right?
> > >
> > > Maybe you could just register a simple "proper" platform device for all
> > > the sroms and then hang the class devices from it? I can type some code
> > > doing this if it sound reasonably?
> > 
> > I'm not sure exactly what you mean by device discovery here.  

(sorry, sent too early...)

By "device discovery" I meant the way you find the way in your devices
in /sysfs. You seem to be traversing /sys/devices/... tree, while you've
got almost direct access to them through /sys/class/srom and you can (I
believe, correct me if I'm wrong, Greg) rely on this path being stable.

Paweł

Re: [PATCH 2/5] char: tile-srom: Remove reference to platform_bus

[Chris Metcalf]

Thanks for the code and comments. I'm out on vacation till mid next week but I'll look at this when I get back.
Chris

> On Aug 8, 2014, at 12:35 PM, "Pawel Moll" <pawel.moll-5wv7dgnIgG8@public.gmane.org> wrote:
> 
> On Tue, 2014-08-05 at 21:08 +0100, Chris Metcalf wrote:
>>>> In addition, we also have user binaries
>>>> in the wild that know to look for /sys/devices/platform/srom/ paths,
>>>> so I'm pretty reluctant to change this path without good reason.
>>> So what is the srom class for then if not for device discovery? And why
>>> do they look for them in the first place? To get relevant character
>>> device's data, if I understand it right?
>>> 
>>> Maybe you could just register a simple "proper" platform device for all
>>> the sroms and then hang the class devices from it? I can type some code
>>> doing this if it sound reasonably?
>> 
>> I'm not sure exactly what you mean by device discovery here.  
> 
> 
> 
>> The
>> subdirectories under /sys/devices/platform/srom/ correspond to partitions
>> in the SPI-ROM, which are software constructs created by the Tilera hypervisor.
>> By default we have three, where the first holds boot data that the chip
>> can use to boot out of hardware, and the other two are smaller partitions
>> for boot- and user-specific data.  We use the /sys files primarily to get the
>> page size and sector size for the sroms, and also export other interesting
>> information like the total size of the particular srom device.
>> 
>> Thank you for volunteering to write a bit of code; if that's the best
>> way to clarify this for us, fantastic, or else pointing us at existing
>> good practices or documentation would be great too.
> 
> I was thinking about something like the following (warning, untested)
> 
> 8<-------------------------------------------
> From c53f0a2492d6cd38d1f82d57916a6528b071e8a8 Mon Sep 17 00:00:00 2001
> From: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> Date: Fri, 8 Aug 2014 16:32:58 +0100
> Subject: [PATCH] char: tile-srom: Add real platform bus parent
> 
> Add a real platform bus device as a parent for
> the srom class devices, to prevent non-platform
> devices hanging from the bus root.
> 
> Signed-off-by: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> ---
> drivers/char/tile-srom.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tile-srom.c b/drivers/char/tile-srom.c
> index bd37747..7fb0fd5 100644
> --- a/drivers/char/tile-srom.c
> +++ b/drivers/char/tile-srom.c
> @@ -76,6 +76,7 @@ MODULE_LICENSE("GPL");
> 
> static int srom_devs;            /* Number of SROM partitions */
> static struct cdev srom_cdev;
> +static struct platform_device *srom_parent;
> static struct class *srom_class;
> static struct srom_dev *srom_devices;
> 
> @@ -350,7 +351,7 @@ static int srom_setup_minor(struct srom_dev *srom, int index)
>               SROM_PAGE_SIZE_OFF, sizeof(srom->page_size)) < 0)
>        return -EIO;
> 
> -    dev = device_create(srom_class, &platform_bus,
> +    dev = device_create(srom_class, srom_parent,
>                MKDEV(srom_major, index), srom, "%d", index);
>    return PTR_ERR_OR_ZERO(dev);
> }
> @@ -415,6 +416,13 @@ static int srom_init(void)
>    if (result < 0)
>        goto fail_chrdev;
> 
> +    /* Create a parent device */
> +    srom_parent = platform_device_register_simple("srom", -1, NULL, 0);
> +    if (IS_ERR(srom_parent)) {
> +        result = PTR_ERR(srom_parent);
> +        goto fail_pdev;
> +    }
> +
>    /* Create a sysfs class. */
>    srom_class = class_create(THIS_MODULE, "srom");
>    if (IS_ERR(srom_class)) {
> @@ -438,6 +446,8 @@ fail_class:
>        device_destroy(srom_class, MKDEV(srom_major, i));
>    class_destroy(srom_class);
> fail_cdev:
> +    platform_device_unregister(srom_parent);
> +fail_pdev:
>    cdev_del(&srom_cdev);
> fail_chrdev:
>    unregister_chrdev_region(dev, srom_devs);
> @@ -454,6 +464,7 @@ static void srom_cleanup(void)
>        device_destroy(srom_class, MKDEV(srom_major, i));
>    class_destroy(srom_class);
>    cdev_del(&srom_cdev);
> +    platform_device_unregister(srom_parent);
>    unregister_chrdev_region(MKDEV(srom_major, 0), srom_devs);
>    kfree(srom_devices);
> }
> -- 
> 1.9.1
> 8<-------------------------------------------
> 
> Would that work for you? Note that it will move the srom class devices
> one level deeper in /sys/devices/... hierarchy.
> 
> Paweł
> 

Re: [PATCH 2/5] char: tile-srom: Remove reference to platform_bus

[Chris Metcalf]

(Resending with text/plain.)

First, sorry for the delayed response, with summer vacation and then
trying to catch up.  :-)

On 8/8/2014 12:34 PM, Pawel Moll wrote:
> On Tue, 2014-08-05 at 21:08 +0100, Chris Metcalf wrote:
>>>> In addition, we also have user binaries
>>>> in the wild that know to look for /sys/devices/platform/srom/ paths,
>>>> so I'm pretty reluctant to change this path without good reason.
>>> So what is the srom class for then if not for device discovery? And why
>>> do they look for them in the first place? To get relevant character
>>> device's data, if I understand it right?
>>>
>>> Maybe you could just register a simple "proper" platform device for all
>>> the sroms and then hang the class devices from it? I can type some code
>>> doing this if it sound reasonably?
>> By "device discovery" I meant the way you find the way in your devices
>> in /sysfs. You seem to be traversing /sys/devices/... tree, while you've
>> got almost direct access to them through /sys/class/srom and you can (I
>> believe, correct me if I'm wrong, Greg) rely on this path being stable.

Yes, this is an excellent point.  I will change the user tool to use
/sys/class instead and then it will work with the existing kernel as well
as with future kernels that incorporate your suggested change.

>> The
>> subdirectories under /sys/devices/platform/srom/ correspond to partitions
>> in the SPI-ROM, which are software constructs created by the Tilera hypervisor.
>> By default we have three, where the first holds boot data that the chip
>> can use to boot out of hardware, and the other two are smaller partitions
>> for boot- and user-specific data.  We use the /sys files primarily to get the
>> page size and sector size for the sroms, and also export other interesting
>> information like the total size of the particular srom device.
>>
>> Thank you for volunteering to write a bit of code; if that's the best
>> way to clarify this for us, fantastic, or else pointing us at existing
>> good practices or documentation would be great too.
> [...]
> @@ -350,7 +351,7 @@ static int srom_setup_minor(struct srom_dev *srom, int index)
>  		       SROM_PAGE_SIZE_OFF, sizeof(srom->page_size)) < 0)
>  		return -EIO;
>
> -	dev = device_create(srom_class, &platform_bus,
> +	dev = device_create(srom_class, srom_parent,
>  			    MKDEV(srom_major, index), srom, "%d", index);
>  	return PTR_ERR_OR_ZERO(dev);
>  }

The second argument should be &srom_parent.dev though, I think.  Right?

> Would that work for you? Note that it will move the srom class devices
> one level deeper in /sys/devices/... hierarchy.

Yes, that seems slightly unfortunately but not too problematic.  If the
consensus is that this is the way to go, I can certainly take this change
into the Tile tree.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH 2/5] char: tile-srom: Remove reference to platform_bus

[Pawel Moll]

On Fri, 2014-08-29 at 19:43 +0100, Chris Metcalf wrote:
> >> Thank you for volunteering to write a bit of code; if that's the best
> >> way to clarify this for us, fantastic, or else pointing us at existing
> >> good practices or documentation would be great too.
> > [...]
> > @@ -350,7 +351,7 @@ static int srom_setup_minor(struct srom_dev *srom, int index)
> >  		       SROM_PAGE_SIZE_OFF, sizeof(srom->page_size)) < 0)
> >  		return -EIO;
> >
> > -	dev = device_create(srom_class, &platform_bus,
> > +	dev = device_create(srom_class, srom_parent,
> >  			    MKDEV(srom_major, index), srom, "%d", index);
> >  	return PTR_ERR_OR_ZERO(dev);
> >  }
> 
> The second argument should be &srom_parent.dev though, I think.  Right?

Yes, sure - as I said, I haven't really tested this code, sorry!


> If the
> consensus is that this is the way to go, I can certainly take this change
> into the Tile tree.

That would be cool, and left us only with the scsi/DMA as the last user
of platform_bus. But this is a completely different story ;-)

Paweł

Re: [PATCH 2/5] char: tile-srom: Remove reference to platform_bus

[Chris Metcalf]

On 9/1/2014 8:27 AM, Pawel Moll wrote:
> On Fri, 2014-08-29 at 19:43 +0100, Chris Metcalf wrote:
>> If the
>> consensus is that this is the way to go, I can certainly take this change
>> into the Tile tree.
> That would be cool, and left us only with the scsi/DMA as the last user
> of platform_bus. But this is a completely different story ;-)

OK, sounds good.  It's in the tile tree at linux-next now.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

[PATCH 3/5] mmc: sdhci-pltfm: Do not use parent as the host's device

[Pawel Moll]

The code selecting a device for the sdhci host has been
continuously tweaked (4b711cb13843f5082e82970dd1e8031383134a65
"mmc: sdhci-pltfm: Add structure for host-specific data" and
a4d2177f00a5252d825236c5124bc1e9918bdb41 "mmc: sdhci-pltfm: dt
device does not pass parent to sdhci_alloc_host") while there
does not seem to be any reason to use platform device's parent
in the first place.

The comment saying "Some PCI-based MFD need the parent here"
seem to refer to Timberdale FPGA driver (the only MFD driver
registering SDHCI cell, drivers/mfd/timberdale.c) but again,
the only situation when parent device matter is runtime PM,
which is not implemented for Timberdale.

Cc: Chris Ball <chris@printf.net>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: linux-mmc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---

This patch is a part of effort to remove references to platform_bus
and make it static.

Chris, Anton, Ulf - could you please advise if the assumptions
above are correct or if I'm completely wrong? Do you know what
where the real reasons to use parent originally? The PCI comment
seems like a red herring to me...

 drivers/mmc/host/sdhci-pltfm.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 7e834fb..4996112 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -136,13 +136,8 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 	if (resource_size(iomem) < 0x100)
 		dev_err(&pdev->dev, "Invalid iomem size!\n");
 
-	/* Some PCI-based MFD need the parent here */
-	if (pdev->dev.parent != &platform_bus && !np)
-		host = sdhci_alloc_host(pdev->dev.parent,
-			sizeof(struct sdhci_pltfm_host) + priv_size);
-	else
-		host = sdhci_alloc_host(&pdev->dev,
-			sizeof(struct sdhci_pltfm_host) + priv_size);
+	host = sdhci_alloc_host(&pdev->dev,
+		sizeof(struct sdhci_pltfm_host) + priv_size);
 
 	if (IS_ERR(host)) {
 		ret = PTR_ERR(host);
-- 
1.9.1

Re: [PATCH 3/5] mmc: sdhci-pltfm: Do not use parent as the host's device

[Pawel Moll]

On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
> The code selecting a device for the sdhci host has been
> continuously tweaked (4b711cb13843f5082e82970dd1e8031383134a65
> "mmc: sdhci-pltfm: Add structure for host-specific data" and
> a4d2177f00a5252d825236c5124bc1e9918bdb41 "mmc: sdhci-pltfm: dt
> device does not pass parent to sdhci_alloc_host") while there
> does not seem to be any reason to use platform device's parent
> in the first place.
> 
> The comment saying "Some PCI-based MFD need the parent here"
> seem to refer to Timberdale FPGA driver (the only MFD driver
> registering SDHCI cell, drivers/mfd/timberdale.c) but again,
> the only situation when parent device matter is runtime PM,
> which is not implemented for Timberdale.
> 
> Cc: Chris Ball <chris@printf.net>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-mmc@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
> 
> This patch is a part of effort to remove references to platform_bus
> and make it static.
> 
> Chris, Anton, Ulf - could you please advise if the assumptions
> above are correct or if I'm completely wrong? Do you know what
> where the real reasons to use parent originally? The PCI comment
> seems like a red herring to me...

Can I take the silence as a suggestion that the change looks ok-ish for
you?

Paweł

Re: [PATCH 3/5] mmc: sdhci-pltfm: Do not use parent as the host's device

[Ulf Hansson]

On 8 August 2014 18:36, Pawel Moll <pawel.moll@arm.com> wrote:
> On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
>> The code selecting a device for the sdhci host has been
>> continuously tweaked (4b711cb13843f5082e82970dd1e8031383134a65
>> "mmc: sdhci-pltfm: Add structure for host-specific data" and
>> a4d2177f00a5252d825236c5124bc1e9918bdb41 "mmc: sdhci-pltfm: dt
>> device does not pass parent to sdhci_alloc_host") while there
>> does not seem to be any reason to use platform device's parent
>> in the first place.
>>
>> The comment saying "Some PCI-based MFD need the parent here"
>> seem to refer to Timberdale FPGA driver (the only MFD driver
>> registering SDHCI cell, drivers/mfd/timberdale.c) but again,
>> the only situation when parent device matter is runtime PM,
>> which is not implemented for Timberdale.
>>
>> Cc: Chris Ball <chris@printf.net>
>> Cc: Anton Vorontsov <anton@enomsg.org>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: linux-mmc@vger.kernel.org
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
>> ---
>>
>> This patch is a part of effort to remove references to platform_bus
>> and make it static.
>>
>> Chris, Anton, Ulf - could you please advise if the assumptions
>> above are correct or if I'm completely wrong? Do you know what
>> where the real reasons to use parent originally? The PCI comment
>> seems like a red herring to me...
>
> Can I take the silence as a suggestion that the change looks ok-ish for
> you?

Sorry for the delay. I suppose this make sense, but I really don't
know for sure.

I guess we need some testing in linux-next, to get some confidence.

Kind regards
Uffe

>
> Paweł
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] mmc: sdhci-pltfm: Do not use parent as the host's device

[Pawel Moll]

On Mon, 2014-08-11 at 10:07 +0100, Ulf Hansson wrote:
> On 8 August 2014 18:36, Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
> >> The code selecting a device for the sdhci host has been
> >> continuously tweaked (4b711cb13843f5082e82970dd1e8031383134a65
> >> "mmc: sdhci-pltfm: Add structure for host-specific data" and
> >> a4d2177f00a5252d825236c5124bc1e9918bdb41 "mmc: sdhci-pltfm: dt
> >> device does not pass parent to sdhci_alloc_host") while there
> >> does not seem to be any reason to use platform device's parent
> >> in the first place.
> >>
> >> The comment saying "Some PCI-based MFD need the parent here"
> >> seem to refer to Timberdale FPGA driver (the only MFD driver
> >> registering SDHCI cell, drivers/mfd/timberdale.c) but again,
> >> the only situation when parent device matter is runtime PM,
> >> which is not implemented for Timberdale.
> >>
> >> Cc: Chris Ball <chris-OsFVWbfNK3isTnJN9+BGXg@public.gmane.org>
> >> Cc: Anton Vorontsov <anton-9xeibp6oKSgdnm+yROfE0A@public.gmane.org>
> >> Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> Cc: linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> Cc: linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> >> Signed-off-by: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> >> ---
> >>
> >> This patch is a part of effort to remove references to platform_bus
> >> and make it static.
> >>
> >> Chris, Anton, Ulf - could you please advise if the assumptions
> >> above are correct or if I'm completely wrong? Do you know what
> >> where the real reasons to use parent originally? The PCI comment
> >> seems like a red herring to me...
> >
> > Can I take the silence as a suggestion that the change looks ok-ish for
> > you?
> 
> Sorry for the delay. I suppose this make sense, but I really don't
> know for sure.
> 
> I guess we need some testing in linux-next, to get some confidence.

Would you take it into -next then? Unless I'm completely wrong there
should be no impact on any in-tree driver...

Cheers!

Pawel

Re: [PATCH 3/5] mmc: sdhci-pltfm: Do not use parent as the host's device

[Ulf Hansson]

On 11 August 2014 11:15, Pawel Moll <pawel.moll@arm.com> wrote:
> On Mon, 2014-08-11 at 10:07 +0100, Ulf Hansson wrote:
>> On 8 August 2014 18:36, Pawel Moll <pawel.moll@arm.com> wrote:
>> > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
>> >> The code selecting a device for the sdhci host has been
>> >> continuously tweaked (4b711cb13843f5082e82970dd1e8031383134a65
>> >> "mmc: sdhci-pltfm: Add structure for host-specific data" and
>> >> a4d2177f00a5252d825236c5124bc1e9918bdb41 "mmc: sdhci-pltfm: dt
>> >> device does not pass parent to sdhci_alloc_host") while there
>> >> does not seem to be any reason to use platform device's parent
>> >> in the first place.
>> >>
>> >> The comment saying "Some PCI-based MFD need the parent here"
>> >> seem to refer to Timberdale FPGA driver (the only MFD driver
>> >> registering SDHCI cell, drivers/mfd/timberdale.c) but again,
>> >> the only situation when parent device matter is runtime PM,
>> >> which is not implemented for Timberdale.
>> >>
>> >> Cc: Chris Ball <chris@printf.net>
>> >> Cc: Anton Vorontsov <anton@enomsg.org>
>> >> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> >> Cc: linux-mmc@vger.kernel.org
>> >> Cc: linuxppc-dev@lists.ozlabs.org
>> >> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
>> >> ---
>> >>
>> >> This patch is a part of effort to remove references to platform_bus
>> >> and make it static.
>> >>
>> >> Chris, Anton, Ulf - could you please advise if the assumptions
>> >> above are correct or if I'm completely wrong? Do you know what
>> >> where the real reasons to use parent originally? The PCI comment
>> >> seems like a red herring to me...
>> >
>> > Can I take the silence as a suggestion that the change looks ok-ish for
>> > you?
>>
>> Sorry for the delay. I suppose this make sense, but I really don't
>> know for sure.
>>
>> I guess we need some testing in linux-next, to get some confidence.
>
> Would you take it into -next then? Unless I'm completely wrong there
> should be no impact on any in-tree driver...

I will take it; though I think it's best to queue it for 3.18 to get
some more testing.

Kind regards
Uffe

Re: [PATCH 3/5] mmc: sdhci-pltfm: Do not use parent as the host's device

[Ulf Hansson]

On 11 August 2014 11:32, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 11 August 2014 11:15, Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org> wrote:
>> On Mon, 2014-08-11 at 10:07 +0100, Ulf Hansson wrote:
>>> On 8 August 2014 18:36, Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org> wrote:
>>> > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
>>> >> The code selecting a device for the sdhci host has been
>>> >> continuously tweaked (4b711cb13843f5082e82970dd1e8031383134a65
>>> >> "mmc: sdhci-pltfm: Add structure for host-specific data" and
>>> >> a4d2177f00a5252d825236c5124bc1e9918bdb41 "mmc: sdhci-pltfm: dt
>>> >> device does not pass parent to sdhci_alloc_host") while there
>>> >> does not seem to be any reason to use platform device's parent
>>> >> in the first place.
>>> >>
>>> >> The comment saying "Some PCI-based MFD need the parent here"
>>> >> seem to refer to Timberdale FPGA driver (the only MFD driver
>>> >> registering SDHCI cell, drivers/mfd/timberdale.c) but again,
>>> >> the only situation when parent device matter is runtime PM,
>>> >> which is not implemented for Timberdale.
>>> >>
>>> >> Cc: Chris Ball <chris-OsFVWbfNK3isTnJN9+BGXg@public.gmane.org>
>>> >> Cc: Anton Vorontsov <anton-9xeibp6oKSgdnm+yROfE0A@public.gmane.org>
>>> >> Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> >> Cc: linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> >> Cc: linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
>>> >> Signed-off-by: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
>>> >> ---
>>> >>
>>> >> This patch is a part of effort to remove references to platform_bus
>>> >> and make it static.
>>> >>
>>> >> Chris, Anton, Ulf - could you please advise if the assumptions
>>> >> above are correct or if I'm completely wrong? Do you know what
>>> >> where the real reasons to use parent originally? The PCI comment
>>> >> seems like a red herring to me...
>>> >
>>> > Can I take the silence as a suggestion that the change looks ok-ish for
>>> > you?
>>>
>>> Sorry for the delay. I suppose this make sense, but I really don't
>>> know for sure.
>>>
>>> I guess we need some testing in linux-next, to get some confidence.
>>
>> Would you take it into -next then? Unless I'm completely wrong there
>> should be no impact on any in-tree driver...
>
> I will take it; though I think it's best to queue it for 3.18 to get
> some more testing.

This patch causes a compiler warning, could you please fix it.

Kind regards
Uffe

Re: [PATCH 3/5] mmc: sdhci-pltfm: Do not use parent as the host's device

[Russell King - ARM Linux]

On Mon, Aug 11, 2014 at 10:15:50AM +0100, Pawel Moll wrote:
> On Mon, 2014-08-11 at 10:07 +0100, Ulf Hansson wrote:
> > Sorry for the delay. I suppose this make sense, but I really don't
> > know for sure.
> > 
> > I guess we need some testing in linux-next, to get some confidence.
> 
> Would you take it into -next then? Unless I'm completely wrong there
> should be no impact on any in-tree driver...

But not until 3.17-rc1 has been released.  linux-next should not receive
new material other than bug fixes while a merge window is open.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[Pawel Moll]

The host devices without a parent were "forcefully adopted"
by platform bus. This patch removes this assignment. In
effect the dev_dev may be NULL now, which means ISA.

Cc: James E.J. Bottomley <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---

This patch is a part of effort to remove references to platform_bus
and make it static.

James, could you please have a look and advice if the change is
correct? Would you happen to know the "real reasons" behind
using the root platform_bus device a parent?

 drivers/scsi/hosts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 3cbb57a..0c7389f 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -218,7 +218,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 		goto fail;
 
 	if (!shost->shost_gendev.parent)
-		shost->shost_gendev.parent = dev ? dev : &platform_bus;
+		shost->shost_gendev.parent = dev;
 	if (!dma_dev)
 		dma_dev = shost->shost_gendev.parent;
 
-- 
1.9.1

Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[James Bottomley]

On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
> The host devices without a parent were "forcefully adopted"
> by platform bus. This patch removes this assignment. In
> effect the dev_dev may be NULL now, which means ISA.
> 
> Cc: James E.J. Bottomley <JBottomley@parallels.com>
> Cc: linux-scsi@vger.kernel.org
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
> 
> This patch is a part of effort to remove references to platform_bus
> and make it static.
> 
> James, could you please have a look and advice if the change is
> correct? Would you happen to know the "real reasons" behind
> using the root platform_bus device a parent?

Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic
in the DMA transfers if it is.  A lot of the legacy ISA device on x86
and I thought some ARM SOC devices don't pass in the parent device, so
we hang them off a known parent.

You can grep for it; these are the devices that will begin to panic if
you apply this patch:

arch/ia64/hp/sim/simscsi.c:     error = scsi_add_host(host, NULL);
drivers/scsi/a2091.c:   error = scsi_add_host(instance, NULL);
drivers/scsi/a3000.c:   error = scsi_add_host(instance, NULL);
drivers/scsi/aha152x.c: if( scsi_add_host(shpnt, NULL) ) {
drivers/scsi/gdth.c:    error = scsi_add_host(shp, NULL);
drivers/scsi/gdth.c:    error = scsi_add_host(shp, NULL);
drivers/scsi/gvp11.c:   error = scsi_add_host(instance, NULL);
drivers/scsi/imm.c:     err = scsi_add_host(host, NULL);
drivers/scsi/pcmcia/fdomain_stub.c:    if (scsi_add_host(host, NULL))
drivers/scsi/pcmcia/nsp_cs.c:   ret = scsi_add_host (host, NULL);
drivers/scsi/pcmcia/qlogic_stub.c:      if (scsi_add_host(shost, NULL))
drivers/scsi/pcmcia/sym53c500_cs.c:     if (scsi_add_host(host, NULL))
drivers/scsi/ppa.c:     err = scsi_add_host(host, NULL);
drivers/scsi/qlogicfas.c:       if (scsi_add_host(hreg, NULL))
drivers/scsi/scsi_module.c:             error = scsi_add_host(shost, NULL);
drivers/scsi/sgiwd93.c: err = scsi_add_host(host, NULL);

Note I've picked up scsi_module, so anything that uses the SCSI module
interface also has this problem.

James

Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[Pawel Moll]

On Fri, 2014-07-25 at 15:46 +0100, James Bottomley wrote:
> On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
> > The host devices without a parent were "forcefully adopted"
> > by platform bus. This patch removes this assignment. In
> > effect the dev_dev may be NULL now, which means ISA.
> > 
> > Cc: James E.J. Bottomley <JBottomley@parallels.com>
> > Cc: linux-scsi@vger.kernel.org
> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > ---
> > 
> > This patch is a part of effort to remove references to platform_bus
> > and make it static.
> > 
> > James, could you please have a look and advice if the change is
> > correct? Would you happen to know the "real reasons" behind
> > using the root platform_bus device a parent?
> 
> Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic
> in the DMA transfers if it is.  

That's what I though at the beginning as well, but then I crawled
through get_dma_ops(struct device *dev) and it seems that on most
architectures (all but two, if I remember correctly) it will return a
default set of DMA ops if the dev == NULL, eg.

static inline struct dma_map_ops *get_dma_ops(struct device *dev)
{
#ifndef CONFIG_X86_DEV_DMA_OPS
        return dma_ops;
#else
        if (unlikely(!dev) || !dev->archdata.dma_ops)
                return dma_ops;
        else
                return dev->archdata.dma_ops;
#endif 
}

in arch/x86/include/asm/dma-mapping.h. Now, there seem to be only a
handful of places where dev_dma is dereferenced: scsi_dma_map and
scsi_dma_unmap in drivers/scsi/scsi_lib_dma.c, where all the calls seem
to be "NULL resistant" and __scsi_alloc_queue in __scsi_alloc_queue
which will oops as you said.

Anyway, if you are saying that dev_dma must not be NULL at any
circumstances, I'll either have to find some kind of replacement for
platform_bus or convince Greg that platform_bus must not be made
static ;-)

> A lot of the legacy ISA device on x86
> and I thought some ARM SOC devices don't pass in the parent device, so
> we hang them off a known parent.

That's another thing I'm not sure - once assigned, is the dma_dev
related to the parent in any way? Even more - is the shost_gendev.parent
used at all? Doesn't seem to be. If it's only about a place in the
device model hierarchy, leaving parent as NULL will make such device a
virtual one, which it probably should be...

Paweł


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[Greg Kroah-Hartman]

On Fri, Jul 25, 2014 at 07:46:56AM -0700, James Bottomley wrote:
> On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
> > The host devices without a parent were "forcefully adopted"
> > by platform bus. This patch removes this assignment. In
> > effect the dev_dev may be NULL now, which means ISA.
> > 
> > Cc: James E.J. Bottomley <JBottomley@parallels.com>
> > Cc: linux-scsi@vger.kernel.org
> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > ---
> > 
> > This patch is a part of effort to remove references to platform_bus
> > and make it static.
> > 
> > James, could you please have a look and advice if the change is
> > correct? Would you happen to know the "real reasons" behind
> > using the root platform_bus device a parent?
> 
> Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic
> in the DMA transfers if it is.  A lot of the legacy ISA device on x86
> and I thought some ARM SOC devices don't pass in the parent device, so
> we hang them off a known parent.

The "generic" platform bus device is not a "known parent".  I don't
understand the difference between just setting the parent to be NULL,
which will then have a "proper" parent pointer filled in by the driver
core when the device is registered, or faking it out here.  What is the
difference?

In the end, the device always ends up with a parent pointer, right?

thanks,

greg k-h

Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[James Bottomley]

On Sat, 2014-07-26 at 13:11 -0700, Greg Kroah-Hartman wrote:
> On Fri, Jul 25, 2014 at 07:46:56AM -0700, James Bottomley wrote:
> > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
> > > The host devices without a parent were "forcefully adopted"
> > > by platform bus. This patch removes this assignment. In
> > > effect the dev_dev may be NULL now, which means ISA.
> > > 
> > > Cc: James E.J. Bottomley <JBottomley@parallels.com>
> > > Cc: linux-scsi@vger.kernel.org
> > > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > > ---
> > > 
> > > This patch is a part of effort to remove references to platform_bus
> > > and make it static.
> > > 
> > > James, could you please have a look and advice if the change is
> > > correct? Would you happen to know the "real reasons" behind
> > > using the root platform_bus device a parent?
> > 
> > Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic
> > in the DMA transfers if it is.  A lot of the legacy ISA device on x86
> > and I thought some ARM SOC devices don't pass in the parent device, so
> > we hang them off a known parent.
> 
> The "generic" platform bus device is not a "known parent".  I don't
> understand the difference between just setting the parent to be NULL,
> which will then have a "proper" parent pointer filled in by the driver
> core when the device is registered, or faking it out here.  What is the
> difference?

If you set the parent to NULL, the host template dma_dev will end up
NULL as well and that will trigger a NULL deref panic in the dma segment
routines.

If you want to remove platform_bus, we have to have a well known device
to set dma_dev to at scsi_host_add time.

> In the end, the device always ends up with a parent pointer, right?

The parent pointer isn't the problem ... assigning the correct dma
device is.

James

Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[Greg Kroah-Hartman]

On Sun, Jul 27, 2014 at 07:52:57AM +0400, James Bottomley wrote:
> On Sat, 2014-07-26 at 13:11 -0700, Greg Kroah-Hartman wrote:
> > On Fri, Jul 25, 2014 at 07:46:56AM -0700, James Bottomley wrote:
> > > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
> > > > The host devices without a parent were "forcefully adopted"
> > > > by platform bus. This patch removes this assignment. In
> > > > effect the dev_dev may be NULL now, which means ISA.
> > > > 
> > > > Cc: James E.J. Bottomley <JBottomley@parallels.com>
> > > > Cc: linux-scsi@vger.kernel.org
> > > > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > > > ---
> > > > 
> > > > This patch is a part of effort to remove references to platform_bus
> > > > and make it static.
> > > > 
> > > > James, could you please have a look and advice if the change is
> > > > correct? Would you happen to know the "real reasons" behind
> > > > using the root platform_bus device a parent?
> > > 
> > > Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic
> > > in the DMA transfers if it is.  A lot of the legacy ISA device on x86
> > > and I thought some ARM SOC devices don't pass in the parent device, so
> > > we hang them off a known parent.
> > 
> > The "generic" platform bus device is not a "known parent".  I don't
> > understand the difference between just setting the parent to be NULL,
> > which will then have a "proper" parent pointer filled in by the driver
> > core when the device is registered, or faking it out here.  What is the
> > difference?
> 
> If you set the parent to NULL, the host template dma_dev will end up
> NULL as well and that will trigger a NULL deref panic in the dma segment
> routines.
>
> If you want to remove platform_bus, we have to have a well known device
> to set dma_dev to at scsi_host_add time.

Why not set the dma_dev after you call device_add()?  That way you will
pick up the right parent no matter what.

> > In the end, the device always ends up with a parent pointer, right?
> 
> The parent pointer isn't the problem ... assigning the correct dma
> device is.

Ah, ok, it's a scsi core thing, not a driver core thing, that's less
confusing now.  For a "fallback" of a platform device, if you switch the
lines around you should be fine, something like this patch perhaps:

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 3cbb57a8b846..d8d3b294f5bc 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -218,16 +218,16 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 		goto fail;
 
 	if (!shost->shost_gendev.parent)
-		shost->shost_gendev.parent = dev ? dev : &platform_bus;
-	if (!dma_dev)
-		dma_dev = shost->shost_gendev.parent;
-
-	shost->dma_dev = dma_dev;
+		shost->shost_gendev.parent = dev;
 
 	error = device_add(&shost->shost_gendev);
 	if (error)
 		goto out;
 
+	if (!dma_dev)
+		dma_dev = shost->shost_gendev.parent;
+	shost->dma_dev = dma_dev;
+
 	pm_runtime_set_active(&shost->shost_gendev);
 	pm_runtime_enable(&shost->shost_gendev);
 	device_enable_async_suspend(&shost->shost_gendev);

Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[Pawel Moll]

On Sun, 2014-07-27 at 16:07 +0100, Greg Kroah-Hartman wrote:
> Ah, ok, it's a scsi core thing, not a driver core thing, that's less
> confusing now.  For a "fallback" of a platform device, if you switch the
> lines around you should be fine, something like this patch perhaps:
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 3cbb57a8b846..d8d3b294f5bc 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -218,16 +218,16 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>  		goto fail;
>  
>  	if (!shost->shost_gendev.parent)
> -		shost->shost_gendev.parent = dev ? dev : &platform_bus;
> -	if (!dma_dev)
> -		dma_dev = shost->shost_gendev.parent;
> -
> -	shost->dma_dev = dma_dev;
> +		shost->shost_gendev.parent = dev;
>  
>  	error = device_add(&shost->shost_gendev);
>  	if (error)
>  		goto out;
>  
> +	if (!dma_dev)
> +		dma_dev = shost->shost_gendev.parent;
> +	shost->dma_dev = dma_dev;
> +
>  	pm_runtime_set_active(&shost->shost_gendev);
>  	pm_runtime_enable(&shost->shost_gendev);
>  	device_enable_async_suspend(&shost->shost_gendev);

But this will still make shost->dma_dev == NULL in the cases James
quoted:

On Fri, 2014-07-25 at 15:46 +0100, James Bottomley wrote:
> Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic
> in the DMA transfers if it is.  A lot of the legacy ISA device on x86
> and I thought some ARM SOC devices don't pass in the parent device, so
> we hang them off a known parent.
> 
> You can grep for it; these are the devices that will begin to panic if
> you apply this patch:
> 
> arch/ia64/hp/sim/simscsi.c:     error = scsi_add_host(host, NULL);
> drivers/scsi/a2091.c:   error = scsi_add_host(instance, NULL);
> drivers/scsi/a3000.c:   error = scsi_add_host(instance, NULL);
> drivers/scsi/aha152x.c: if( scsi_add_host(shpnt, NULL) ) {
> drivers/scsi/gdth.c:    error = scsi_add_host(shp, NULL);
> drivers/scsi/gdth.c:    error = scsi_add_host(shp, NULL);
> drivers/scsi/gvp11.c:   error = scsi_add_host(instance, NULL);
> drivers/scsi/imm.c:     err = scsi_add_host(host, NULL);
> drivers/scsi/pcmcia/fdomain_stub.c:    if (scsi_add_host(host, NULL))
> drivers/scsi/pcmcia/nsp_cs.c:   ret = scsi_add_host (host, NULL);
> drivers/scsi/pcmcia/qlogic_stub.c:      if (scsi_add_host(shost, NULL))
> drivers/scsi/pcmcia/sym53c500_cs.c:     if (scsi_add_host(host, NULL))
> drivers/scsi/ppa.c:     err = scsi_add_host(host, NULL);
> drivers/scsi/qlogicfas.c:       if (scsi_add_host(hreg, NULL))
> drivers/scsi/scsi_module.c:             error = scsi_add_host(shost, NULL);
> drivers/scsi/sgiwd93.c: err = scsi_add_host(host, NULL);

Maybe the DMA API should be coping with NULL device? It seems to handle
it in a half of the methods and fails in the other half...

Pawel

[PATCH 5/5] platform: Make platform_bus device a platform device

[Pawel Moll]

... describing the root of the device tree, so one can write
a platform driver initializing the platform.

Signed-off-by: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
---
 drivers/base/platform.c         | 20 ++++++++++++++------
 include/linux/platform_device.h |  2 +-
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index eee48c4..9caffa7 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -30,8 +30,8 @@
 /* For automatically allocated device IDs */
 static DEFINE_IDA(platform_devid_ida);
 
-struct device platform_bus = {
-	.init_name	= "platform",
+struct platform_device platform_bus = {
+	.name	= "platform",
 };
 EXPORT_SYMBOL_GPL(platform_bus);
 
@@ -300,7 +300,7 @@ int platform_device_add(struct platform_device *pdev)
 		return -EINVAL;
 
 	if (!pdev->dev.parent)
-		pdev->dev.parent = &platform_bus;
+		pdev->dev.parent = &platform_bus.dev;
 
 	pdev->dev.bus = &platform_bus_type;
 
@@ -946,12 +946,20 @@ int __init platform_bus_init(void)
 
 	early_platform_cleanup();
 
-	error = device_register(&platform_bus);
+	dev_set_name(&platform_bus.dev, "%s", platform_bus.name);
+	error = device_register(&platform_bus.dev);
 	if (error)
 		return error;
 	error =  bus_register(&platform_bus_type);
-	if (error)
-		device_unregister(&platform_bus);
+	if (!error) {
+#ifdef CONFIG_OF
+		platform_bus.dev.of_node = of_allnodes;
+#endif
+		platform_bus.dev.bus = &platform_bus_type;
+		bus_add_device(&platform_bus.dev);
+	} else {
+		device_unregister(&platform_bus.dev);
+	}
 	return error;
 }
 
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 16f6654..a99032a 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -44,7 +44,7 @@ extern int platform_device_register(struct platform_device *);
 extern void platform_device_unregister(struct platform_device *);
 
 extern struct bus_type platform_bus_type;
-extern struct device platform_bus;
+extern struct platform_device platform_bus;
 
 extern void arch_setup_pdev_archdata(struct platform_device *);
 extern struct resource *platform_get_resource(struct platform_device *,
-- 
1.9.1

Re: [PATCH 5/5] platform: Make platform_bus device a platform device

[Greg Kroah-Hartman]

On Fri, Jul 25, 2014 at 03:23:53PM +0100, Pawel Moll wrote:
> ... describing the root of the device tree, so one can write
> a platform driver initializing the platform.

Wait, what do you mean by "one can write a platform driver initializing
the platform"?  I don't understand your end goal here...

thanks,

greg k-h

Re: [PATCH 5/5] platform: Make platform_bus device a platform device

[Pawel Moll]

On Sat, 2014-07-26 at 21:12 +0100, Greg Kroah-Hartman wrote:
> On Fri, Jul 25, 2014 at 03:23:53PM +0100, Pawel Moll wrote:
> > ... describing the root of the device tree, so one can write
> > a platform driver initializing the platform.
> 
> Wait, what do you mean by "one can write a platform driver initializing
> the platform"?  I don't understand your end goal here...

Bad wording, sorry. The goal is to have a platform driver (as in
platform bus) that will initialize my platform (as in: board, machine,
hardware). My platform (as in: the board) will be represented by the
root platform bus device (as in: the bus ;-) with compatible value
matching the one passed in the device tree's root.

The tree:

8<----------------------------
/ {
	compatible = "my,board";
}
8<----------------------------

The driver:

8<----------------------------
static struct of_device_id my_board_match[] = {
        { .compatible = "my,board", },
        {},
};

static struct platform_driver my_board_driver = {
        .driver = {
                .name = "my_board",
                .owner = THIS_MODULE,
                .of_match_table = of_match_ptr(my_board_match),
        },
        .probe = my_board_probe,
        .remove = my_board_remove,
};
module_platform_driver(my_board_driver);
8<----------------------------

I'll work on better commit message for the next spin.

Paweł

Re: [PATCH 5/5] platform: Make platform_bus device a platform device

[Greg Kroah-Hartman]

On Fri, Jul 25, 2014 at 03:23:53PM +0100, Pawel Moll wrote:
> ... describing the root of the device tree, so one can write
> a platform driver initializing the platform.
> 
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
>  drivers/base/platform.c         | 20 ++++++++++++++------
>  include/linux/platform_device.h |  2 +-
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index eee48c4..9caffa7 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -30,8 +30,8 @@
>  /* For automatically allocated device IDs */
>  static DEFINE_IDA(platform_devid_ida);
>  
> -struct device platform_bus = {
> -	.init_name	= "platform",
> +struct platform_device platform_bus = {
> +	.name	= "platform",
>  };
>  EXPORT_SYMBOL_GPL(platform_bus);
>  
> @@ -300,7 +300,7 @@ int platform_device_add(struct platform_device *pdev)
>  		return -EINVAL;
>  
>  	if (!pdev->dev.parent)
> -		pdev->dev.parent = &platform_bus;
> +		pdev->dev.parent = &platform_bus.dev;
>  
>  	pdev->dev.bus = &platform_bus_type;
>  
> @@ -946,12 +946,20 @@ int __init platform_bus_init(void)
>  
>  	early_platform_cleanup();
>  
> -	error = device_register(&platform_bus);
> +	dev_set_name(&platform_bus.dev, "%s", platform_bus.name);
> +	error = device_register(&platform_bus.dev);
>  	if (error)
>  		return error;
>  	error =  bus_register(&platform_bus_type);
> -	if (error)
> -		device_unregister(&platform_bus);
> +	if (!error) {
> +#ifdef CONFIG_OF
> +		platform_bus.dev.of_node = of_allnodes;
> +#endif

Why are you doing this?  The original code didn't do it and all was
fine, right?  What changes here?

thanks,

greg k-h

Re: [PATCH 5/5] platform: Make platform_bus device a platform device

[Pawel Moll]

On Sat, 2014-07-26 at 21:13 +0100, Greg Kroah-Hartman wrote:
> > @@ -946,12 +946,20 @@ int __init platform_bus_init(void)
> >  
> >  	early_platform_cleanup();
> >  
> > -	error = device_register(&platform_bus);
> > +	dev_set_name(&platform_bus.dev, "%s", platform_bus.name);
> > +	error = device_register(&platform_bus.dev);
> >  	if (error)
> >  		return error;
> >  	error =  bus_register(&platform_bus_type);
> > -	if (error)
> > -		device_unregister(&platform_bus);
> > +	if (!error) {
> > +#ifdef CONFIG_OF
> > +		platform_bus.dev.of_node = of_allnodes;
> > +#endif
> 
> Why are you doing this?  The original code didn't do it and all was
> fine, right?  What changes here?

You mean the #ifdef? It wasn't there, but Olof figured out that it
breaks !CONFIG_OF builds:

http://article.gmane.org/gmane.linux.ports.tegra/18473

as of_allnodes is only defined when CONFIG_OF. I had a choice of
#ifdefing the assignment above or providing a dummy symbol. The latter
doesn't seem sensibly, as there should be no other users for it (the
symbol).

Pawel

Re: [PATCH 1/5] ARM: imx: Remove references to platform_bus in mxc code

[Shawn Guo]

On Fri, Jul 25, 2014 at 03:23:49PM +0100, Pawel Moll wrote:
> The bus devices created to be parents for other peripherals
> were using platform_bus as a parent, not being platform
> devices themselves. Remove the references, making them
> virtual devices instead.
> 
> Cc: Shawn Guo <shawn.guo@freescale.com>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>

Acked-by: Shawn Guo <shawn.guo@freescale.com>