The "platform_bus" (note: not platform_bus_type) only exists as an empty
directory to put platform devices into. However, it really doesn't make
sense to segregate all the platform devices into a sub directory when
typically they are memory mapped devices that doen't go through any
particular bus. Particularly on embedded type platforms the platform_bus
directory doesn't add anything.
However, this will probably just end up breaking some userspace that
depends on the /sys/devices/platform/ path to be present (no matter how
much we protest that userspace must not depend on paths in sysfs). So
while I'm seriously proposing this change, it may just be unacceptable
ABI breakage
Signed-off-by: Grant Likely <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Kay Sievers <[email protected]>
---
arch/arm/plat-mxc/devices.c | 2 --
arch/unicore32/kernel/puv3-core.c | 2 +-
arch/unicore32/kernel/puv3-nb0916.c | 6 +++---
drivers/base/platform.c | 16 +---------------
drivers/char/tile-srom.c | 2 +-
drivers/mmc/host/sdhci-pltfm.c | 6 ++----
drivers/scsi/hosts.c | 2 +-
include/linux/platform_device.h | 1 -
8 files changed, 9 insertions(+), 28 deletions(-)
diff --git a/arch/arm/plat-mxc/devices.c b/arch/arm/plat-mxc/devices.c
index 4d55a7a..7df592b 100644
--- a/arch/arm/plat-mxc/devices.c
+++ b/arch/arm/plat-mxc/devices.c
@@ -25,12 +25,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,
};
static int __init mxc_device_init(void)
diff --git a/arch/unicore32/kernel/puv3-core.c b/arch/unicore32/kernel/puv3-core.c
index 254adee..438dd2e 100644
--- a/arch/unicore32/kernel/puv3-core.c
+++ b/arch/unicore32/kernel/puv3-core.c
@@ -272,7 +272,7 @@ void __init puv3_core_init(void)
platform_device_register_simple("PKUnity-v3-UART", 1,
puv3_uart1_resources, ARRAY_SIZE(puv3_uart1_resources));
platform_device_register_simple("PKUnity-v3-AC97", -1, NULL, 0);
- platform_device_register_resndata(&platform_bus, "musb_hdrc", -1,
+ platform_device_register_resndata(NULL, "musb_hdrc", -1,
puv3_usb_resources, ARRAY_SIZE(puv3_usb_resources),
&puv3_usb_plat, sizeof(puv3_usb_plat));
}
diff --git a/arch/unicore32/kernel/puv3-nb0916.c b/arch/unicore32/kernel/puv3-nb0916.c
index 181108b..e9991b6 100644
--- a/arch/unicore32/kernel/puv3-nb0916.c
+++ b/arch/unicore32/kernel/puv3-nb0916.c
@@ -111,13 +111,13 @@ int __init mach_nb0916_init(void)
platform_device_register_simple("PKUnity-v3-I2C", -1,
puv3_i2c_resources, ARRAY_SIZE(puv3_i2c_resources));
- platform_device_register_data(&platform_bus, "pwm-backlight", -1,
+ platform_device_register_data(NULL, "pwm-backlight", -1,
&nb0916_backlight_data, sizeof(nb0916_backlight_data));
- platform_device_register_data(&platform_bus, "gpio-keys", -1,
+ platform_device_register_data(NULL, "gpio-keys", -1,
&nb0916_gpio_button_data, sizeof(nb0916_gpio_button_data));
- platform_device_register_resndata(&platform_bus, "physmap-flash", -1,
+ platform_device_register_resndata(NULL, "physmap-flash", -1,
&physmap_flash_resource, 1,
&physmap_flash_data, sizeof(physmap_flash_data));
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 72c776f..4f1c969 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -31,11 +31,6 @@ static DEFINE_IDA(platform_devid_ida);
#define to_platform_driver(drv) (container_of((drv), struct platform_driver, \
driver))
-struct device platform_bus = {
- .init_name = "platform",
-};
-EXPORT_SYMBOL_GPL(platform_bus);
-
/**
* arch_setup_pdev_archdata - Allow manipulation of archdata before its used
* @pdev: platform device
@@ -283,9 +278,6 @@ int platform_device_add(struct platform_device *pdev)
if (!pdev)
return -EINVAL;
- if (!pdev->dev.parent)
- pdev->dev.parent = &platform_bus;
-
pdev->dev.bus = &platform_bus_type;
switch (pdev->id) {
@@ -883,13 +875,7 @@ int __init platform_bus_init(void)
early_platform_cleanup();
- error = device_register(&platform_bus);
- if (error)
- return error;
- error = bus_register(&platform_bus_type);
- if (error)
- device_unregister(&platform_bus);
- return error;
+ return bus_register(&platform_bus_type);
}
#ifndef ARCH_HAS_DMA_GET_REQUIRED_MASK
diff --git a/drivers/char/tile-srom.c b/drivers/char/tile-srom.c
index 3b22a60..854ab2b 100644
--- a/drivers/char/tile-srom.c
+++ b/drivers/char/tile-srom.c
@@ -369,7 +369,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 IS_ERR(dev) ? PTR_ERR(dev) : 0;
}
diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 2716445..841425c 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -115,10 +115,8 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
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(*pltfm_host));
- else
- host = sdhci_alloc_host(&pdev->dev, sizeof(*pltfm_host));
+ host = sdhci_alloc_host(pdev->dev.parent ? pdev->dev.parent : &pdev->dev,
+ sizeof(*pltfm_host));
if (IS_ERR(host)) {
ret = PTR_ERR(host);
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 593085a..c1f9966 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -217,7 +217,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;
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 5711e95..e01efb3 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -44,7 +44,6 @@ 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 void arch_setup_pdev_archdata(struct platform_device *);
extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int);
--
1.7.10.4
On Wed, Nov 21, 2012 at 02:44:31PM +0000, Grant Likely wrote:
> The "platform_bus" (note: not platform_bus_type) only exists as an empty
> directory to put platform devices into. However, it really doesn't make
> sense to segregate all the platform devices into a sub directory when
> typically they are memory mapped devices that doen't go through any
> particular bus. Particularly on embedded type platforms the platform_bus
> directory doesn't add anything.
>
> However, this will probably just end up breaking some userspace that
> depends on the /sys/devices/platform/ path to be present (no matter how
> much we protest that userspace must not depend on paths in sysfs). So
> while I'm seriously proposing this change, it may just be unacceptable
> ABI breakage
If the devices don't show up under platform/ where are they going to be
at now, virtual/ ? That doesn't sound like a good plan, they should be
somewhere "useful".
And yes, odds are this will break userspace, but we might not know until
we try it, have you tried it on different distros to see what happens?
thanks,
greg k-h
On Wed, Nov 21, 2012 at 3:52 PM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Wed, Nov 21, 2012 at 02:44:31PM +0000, Grant Likely wrote:
>> The "platform_bus" (note: not platform_bus_type) only exists as an empty
>> directory to put platform devices into. However, it really doesn't make
>> sense to segregate all the platform devices into a sub directory when
>> typically they are memory mapped devices that doen't go through any
>> particular bus. Particularly on embedded type platforms the platform_bus
>> directory doesn't add anything.
>>
>> However, this will probably just end up breaking some userspace that
>> depends on the /sys/devices/platform/ path to be present (no matter how
>> much we protest that userspace must not depend on paths in sysfs). So
>> while I'm seriously proposing this change, it may just be unacceptable
>> ABI breakage
>
> If the devices don't show up under platform/ where are they going to be
> at now, virtual/ ? That doesn't sound like a good plan, they should be
> somewhere "useful".
Just a note to keep in mind: We usually need and want devices to have
a bus or class. Devices without a "subsystem" are invisible to udev,
and do not get proper coldplug support at bootup.
Kay
On Thu, Nov 22, 2012 at 7:17 PM, Kay Sievers <[email protected]> wrote:
> On Wed, Nov 21, 2012 at 3:52 PM, Greg Kroah-Hartman
> <[email protected]> wrote:
>> On Wed, Nov 21, 2012 at 02:44:31PM +0000, Grant Likely wrote:
>>> The "platform_bus" (note: not platform_bus_type) only exists as an empty
>>> directory to put platform devices into. However, it really doesn't make
>>> sense to segregate all the platform devices into a sub directory when
>>> typically they are memory mapped devices that doen't go through any
>>> particular bus. Particularly on embedded type platforms the platform_bus
>>> directory doesn't add anything.
>>>
>>> However, this will probably just end up breaking some userspace that
>>> depends on the /sys/devices/platform/ path to be present (no matter how
>>> much we protest that userspace must not depend on paths in sysfs). So
>>> while I'm seriously proposing this change, it may just be unacceptable
>>> ABI breakage
>>
>> If the devices don't show up under platform/ where are they going to be
>> at now, virtual/ ? That doesn't sound like a good plan, they should be
>> somewhere "useful".
>
> Just a note to keep in mind: We usually need and want devices to have
> a bus or class. Devices without a "subsystem" are invisible to udev,
> and do not get proper coldplug support at bootup.
Note: this patch is only about the "platform_bus" dummy device. It has
nothing to do with platform_bus_type.
g.
On Thu, Nov 22, 2012 at 10:20 PM, Grant Likely
<[email protected]> wrote:
> On Thu, Nov 22, 2012 at 7:17 PM, Kay Sievers <[email protected]> wrote:
>> On Wed, Nov 21, 2012 at 3:52 PM, Greg Kroah-Hartman <[email protected]> wrote:
>>> If the devices don't show up under platform/ where are they going to be
>>> at now, virtual/ ? That doesn't sound like a good plan, they should be
>>> somewhere "useful".
>>
>> Just a note to keep in mind: We usually need and want devices to have
>> a bus or class. Devices without a "subsystem" are invisible to udev,
>> and do not get proper coldplug support at bootup.
>
> Note: this patch is only about the "platform_bus" dummy device. It has
> nothing to do with platform_bus_type.
Ah, I see now.
Why do you want to remove the "platform_bus" fake-parent, entirely? I
understand and agree that drivers should not fiddle with that
directly. But I don't see a real reason why it should not be private
to the platform_bus_type. It's nothing really wrong with it, I guess.
I have on x86:
coretemp.0 -> ../../../devices/platform/coretemp.0
dock.0 -> ../../../devices/platform/dock.0
dock.1 -> ../../../devices/platform/dock.1
efifb.0 -> ../../../devices/platform/efifb.0
i8042 -> ../../../devices/platform/i8042
iTCO_wdt -> ../../../devices/pci0000:00/0000:00:1f.0/iTCO_wdt
pcspkr -> ../../../devices/platform/pcspkr
serial8250 -> ../../../devices/platform/serial8250
thinkpad_acpi -> ../../../devices/platform/thinkpad_acpi
thinkpad_hwmon -> ../../../devices/platform/thinkpad_hwmon
which look pretty reasonable in a "platform parent" instead of ending
up in virtual/.
We should probably remove the explicit assignment in the drivers like
your patch does, unexport "platform_bus" from the core, so nobody can
use it anymore in the future. The /sys/bus/platform/devices/ directory
can then be created on-demand only, with the first registration of a
device without a parent, we do that in other parts of the core
already.
Wouldn't that solve your problem and still do not touch any other
stuff visible in /sys?
Also, it seems there are devices without any subsystem in the list of
things your patch touches? That is really not how things should work.
All devices should have a bus or class, so userspace receives proper
events, and can enumerate them.
The /sys/devices/ hierarchy is not really meant to be used directly,
it can for some devices even change at runtime. It's not considered a
stable or reasonable predictable ABI, all lookups should start in the
flat device symlink lists of the class/ and bus/, and not in the
hierarchical tree in devices/.
Kay
On Wed, Nov 21, 2012 at 8:44 AM, Grant Likely <[email protected]> wrote:
> The "platform_bus" (note: not platform_bus_type) only exists as an empty
> directory to put platform devices into. However, it really doesn't make
> sense to segregate all the platform devices into a sub directory when
> typically they are memory mapped devices that doen't go through any
> particular bus. Particularly on embedded type platforms the platform_bus
> directory doesn't add anything.
>
> However, this will probably just end up breaking some userspace that
> depends on the /sys/devices/platform/ path to be present (no matter how
> much we protest that userspace must not depend on paths in sysfs). So
> while I'm seriously proposing this change, it may just be unacceptable
> ABI breakage
An old thread, but was there ever a conclusion to this? We now have a
mixture of using platform_bus as the parent or not on various ARM
platforms.
Rob
On Mon, 21 Apr 2014 16:05:29 -0500, Rob Herring <[email protected]> wrote:
> On Wed, Nov 21, 2012 at 8:44 AM, Grant Likely <[email protected]> wrote:
> > The "platform_bus" (note: not platform_bus_type) only exists as an empty
> > directory to put platform devices into. However, it really doesn't make
> > sense to segregate all the platform devices into a sub directory when
> > typically they are memory mapped devices that doen't go through any
> > particular bus. Particularly on embedded type platforms the platform_bus
> > directory doesn't add anything.
> >
> > However, this will probably just end up breaking some userspace that
> > depends on the /sys/devices/platform/ path to be present (no matter how
> > much we protest that userspace must not depend on paths in sysfs). So
> > while I'm seriously proposing this change, it may just be unacceptable
> > ABI breakage
>
> An old thread, but was there ever a conclusion to this? We now have a
> mixture of using platform_bus as the parent or not on various ARM
> platforms.
We kind of concluded in the opposite direction. Instead of removing the
/sys/device/platform directory, the drivers/of code should be changed to
use it.
The following patch is sufficient to have the same effect. It doesn't
unify the OF and non-OF paths of platform device addition, but it gets
them closer. I've been nervous about applying it because I'm concerned
about userspace breakage, but maybe it just needs to be merged and we
can quirk out systems that break.
---
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 404d1daebefa..40a85b85c932 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -175,7 +175,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
#if defined(CONFIG_MICROBLAZE)
dev->dev.dma_mask = &dev->archdata.dma_mask;
#endif
- dev->dev.parent = parent;
+ dev->dev.parent = parent ? parent : &platform_bus;
if (bus_id)
dev_set_name(&dev->dev, "%s", bus_id);
On Wed, Apr 23, 2014 at 9:05 AM, Grant Likely <[email protected]> wrote:
> On Mon, 21 Apr 2014 16:05:29 -0500, Rob Herring <[email protected]> wrote:
>> On Wed, Nov 21, 2012 at 8:44 AM, Grant Likely <[email protected]> wrote:
>> > The "platform_bus" (note: not platform_bus_type) only exists as an empty
>> > directory to put platform devices into. However, it really doesn't make
>> > sense to segregate all the platform devices into a sub directory when
>> > typically they are memory mapped devices that doen't go through any
>> > particular bus. Particularly on embedded type platforms the platform_bus
>> > directory doesn't add anything.
>> >
>> > However, this will probably just end up breaking some userspace that
>> > depends on the /sys/devices/platform/ path to be present (no matter how
>> > much we protest that userspace must not depend on paths in sysfs). So
>> > while I'm seriously proposing this change, it may just be unacceptable
>> > ABI breakage
>>
>> An old thread, but was there ever a conclusion to this? We now have a
>> mixture of using platform_bus as the parent or not on various ARM
>> platforms.
>
> We kind of concluded in the opposite direction. Instead of removing the
> /sys/device/platform directory, the drivers/of code should be changed to
> use it.
>
> The following patch is sufficient to have the same effect. It doesn't
> unify the OF and non-OF paths of platform device addition, but it gets
> them closer. I've been nervous about applying it because I'm concerned
> about userspace breakage, but maybe it just needs to be merged and we
> can quirk out systems that break.
Given that we've changed practically all device names in converting to
DT and I haven't heard of any complaints, we may be okay.
We also have some platforms (imx6 for example) setting the parent to
an soc device. I still need to understand why the soc device needs to
be the parent, but it is pointless platform variation in my book. It
would also change the paths when someone has the whim to add an soc
device.
Rob
On Wed, Apr 23, 2014 at 3:05 PM, Grant Likely <[email protected]> wrote:
> On Mon, 21 Apr 2014 16:05:29 -0500, Rob Herring <[email protected]> wrote:
>> On Wed, Nov 21, 2012 at 8:44 AM, Grant Likely <[email protected]> wrote:
>> > The "platform_bus" (note: not platform_bus_type) only exists as an empty
>> > directory to put platform devices into. However, it really doesn't make
>> > sense to segregate all the platform devices into a sub directory when
>> > typically they are memory mapped devices that doen't go through any
>> > particular bus. Particularly on embedded type platforms the platform_bus
>> > directory doesn't add anything.
>> >
>> > However, this will probably just end up breaking some userspace that
>> > depends on the /sys/devices/platform/ path to be present (no matter how
>> > much we protest that userspace must not depend on paths in sysfs). So
>> > while I'm seriously proposing this change, it may just be unacceptable
>> > ABI breakage
>>
>> An old thread, but was there ever a conclusion to this? We now have a
>> mixture of using platform_bus as the parent or not on various ARM
>> platforms.
>
> We kind of concluded in the opposite direction. Instead of removing the
> /sys/device/platform directory, the drivers/of code should be changed to
> use it.
>
> The following patch is sufficient to have the same effect. It doesn't
> unify the OF and non-OF paths of platform device addition, but it gets
> them closer. I've been nervous about applying it because I'm concerned
> about userspace breakage, but maybe it just needs to be merged and we
> can quirk out systems that break.
Ugh, no matter what we do, something is going to be inconsistent. With
this patch, the platform_devices get moved under
/sys/devices/platform, but an amba_device at the top level stays where
it is. We could move amba devices under the same hierarchy, but that
doesn't seem appropriate.
We could have a separate /sys/devices/of hierarchy for generating
devices into if this really is a problem that needs to be solved. I'm
not convinced that a real problem exists though. PowerPC has been
creating the devices directly under /sys/devices at least since
2.6.12. With the DT being hierarchical to begin with, and the dt code
preserving the hierarchy, on most platforms there won't be a lot of
things at the root.
Anyway, whatever. I don't care enough to argue anymore. Move them if
you like, but make sure the root AMBA devices get created in the same
place.
g.
>
> ---
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1daebefa..40a85b85c932 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -175,7 +175,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
> #if defined(CONFIG_MICROBLAZE)
> dev->dev.dma_mask = &dev->archdata.dma_mask;
> #endif
> - dev->dev.parent = parent;
> + dev->dev.parent = parent ? parent : &platform_bus;
>
> if (bus_id)
> dev_set_name(&dev->dev, "%s", bus_id);
>
On Wed, Apr 23, 2014 at 3:16 PM, Rob Herring <[email protected]> wrote:
> On Wed, Apr 23, 2014 at 9:05 AM, Grant Likely <[email protected]> wrote:
>> On Mon, 21 Apr 2014 16:05:29 -0500, Rob Herring <[email protected]> wrote:
>>> On Wed, Nov 21, 2012 at 8:44 AM, Grant Likely <[email protected]> wrote:
>>> > The "platform_bus" (note: not platform_bus_type) only exists as an empty
>>> > directory to put platform devices into. However, it really doesn't make
>>> > sense to segregate all the platform devices into a sub directory when
>>> > typically they are memory mapped devices that doen't go through any
>>> > particular bus. Particularly on embedded type platforms the platform_bus
>>> > directory doesn't add anything.
>>> >
>>> > However, this will probably just end up breaking some userspace that
>>> > depends on the /sys/devices/platform/ path to be present (no matter how
>>> > much we protest that userspace must not depend on paths in sysfs). So
>>> > while I'm seriously proposing this change, it may just be unacceptable
>>> > ABI breakage
>>>
>>> An old thread, but was there ever a conclusion to this? We now have a
>>> mixture of using platform_bus as the parent or not on various ARM
>>> platforms.
>>
>> We kind of concluded in the opposite direction. Instead of removing the
>> /sys/device/platform directory, the drivers/of code should be changed to
>> use it.
>>
>> The following patch is sufficient to have the same effect. It doesn't
>> unify the OF and non-OF paths of platform device addition, but it gets
>> them closer. I've been nervous about applying it because I'm concerned
>> about userspace breakage, but maybe it just needs to be merged and we
>> can quirk out systems that break.
>
> Given that we've changed practically all device names in converting to
> DT and I haven't heard of any complaints, we may be okay.
>
> We also have some platforms (imx6 for example) setting the parent to
> an soc device. I still need to understand why the soc device needs to
> be the parent, but it is pointless platform variation in my book. It
> would also change the paths when someone has the whim to add an soc
> device.
Yes, that is just dumb and should be discouraged.
g.