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

Message ID 1406298233-27876-2-git-send-email-pawel.moll@arm.com
State New
Headers show

Commit Message

Pawel Moll July 25, 2014, 2:23 p.m.
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(-)

Comments

Chris Metcalf July 31, 2014, 8:24 p.m. | #1
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.  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.
Greg Kroah-Hartman July 31, 2014, 9:32 p.m. | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pawel Moll Aug. 1, 2014, 5:21 p.m. | #3
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@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? 

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Chris Metcalf Aug. 5, 2014, 8:08 p.m. | #4
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@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?
> 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.
Greg Kroah-Hartman Aug. 5, 2014, 11:06 p.m. | #5
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@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?
> >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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch hide | download patch | download mbox

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);
 }