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 <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Russell King <[email protected]>
Cc: [email protected]
Signed-off-by: Pawel Moll <[email protected]>
---
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
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 <[email protected]>
Cc: [email protected]
Signed-off-by: Pawel Moll <[email protected]>
---
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
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 <[email protected]>
Cc: Anton Vorontsov <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Pawel Moll <[email protected]>
---
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
... describing the root of the device tree, so one can write
a platform driver initializing the platform.
Signed-off-by: Pawel Moll <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Pawel Moll <[email protected]>
---
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
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 <[email protected]>
> Cc: [email protected]
> Signed-off-by: Pawel Moll <[email protected]>
> ---
>
> 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
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 <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Pawel Moll <[email protected]>
> > ---
> >
> > 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ł
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 <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Pawel Moll <[email protected]>
> > ---
> >
> > 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
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > > Cc: [email protected]
> > > Signed-off-by: Pawel Moll <[email protected]>
> > > ---
> > >
> > > 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
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 <[email protected]>
> > > > Cc: [email protected]
> > > > Signed-off-by: Pawel Moll <[email protected]>
> > > > ---
> > > >
> > > > 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);
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 <[email protected]>
> Cc: Sascha Hauer <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: [email protected]
> Signed-off-by: Pawel Moll <[email protected]>
Acked-by: Shawn Guo <[email protected]>
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<[email protected]>
> Signed-off-by: Pawel Moll<[email protected]>
> ---
> 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
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<[email protected]>
> >Signed-off-by: Pawel Moll<[email protected]>
> >---
> > 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