Strange location and name for platform devices when device-tree is used.

Strange location and name for platform devices when device-tree is used.

[NeilBrown]

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



My ARM board has a collection of "platform devices".

When I build a kernel using a board file, these platform devices are named
with exactly the names I give them, and they appear in sysfs under
   /sys/devices/platform

all as you might expect.

When I build a kernel using device-tree (and trying to follow the established
patterns for the dts file),  these platform devices appear directly in
"/sys/devices" (not in the "platform" subdirectory) and  they don't have the
names I give them, but instead a sequential number is appended.

The former makes /sys/devices look very untidy.  It can presumably be fixed by
changing of_platform_populate() to replace a parent for 'NULL' with
'platform_bus' as the patch below demonstrates.  Is there any chance that is
correct? (It seems to work, but for all I know it might break something else).


The latter is caused by of_device_make_bus_id():

	/*
	 * No BusID, use the node name and add a globally incremented
	 * counter (and pray...)
	 */
	magic = atomic_add_return(1, &bus_no_reg_magic);
	dev_set_name(dev, "%s.%d", node->name, magic - 1);


This call to prayer dates back to:

commit 9309180f11f0107c9858a61a1ac2b04518a91080
Author: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
Date:   Tue Nov 21 14:56:37 2006 +1100

    [POWERPC] powerpc: Workaround for of_platform without "reg" nor "dcr-reg"

and I wonder how relevant it still is in this context.  As platform devices
are all in the root of the device-tree and hence are siblings, they must have
unique names in the device-tree and  so the platform devices created from
them will also have unique names -- won't they?

Any help understanding and/or fixing this discrepancy greatly appreciated.

The change of name is particularly annoying to me because one of my platform
devices is a pwm_bl.c backlight.  With a boardfile I
get /sys/class/pwm_backlight.  With devicetree the best I can get
is /sys/class/pwm_backlight.23 (or similar).  It would be really nice to have
a more stable and sensible name here.

Thanks,
NeilBrown

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 9b439ac63d8e..af3ef3513cb0 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -476,6 +476,9 @@ int of_platform_populate(struct device_node *root,
 	struct device_node *child;
 	int rc = 0;
 
+	if (parent == NULL)
+		parent = &platform_bus;
+
 	root = root ? of_node_get(root) : of_find_node_by_path("/");
 	if (!root)
 		return -EINVAL;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Strange location and name for platform devices when device-tree is used.

[Benjamin Herrenschmidt]

On Fri, 2013-11-01 at 14:59 +1100, NeilBrown wrote:
> and I wonder how relevant it still is in this context.  As platform devices
> are all in the root of the device-tree and hence are siblings, they must have
> unique names in the device-tree and  so the platform devices created from
> them will also have unique names -- won't they?

I agree about /sys/devices -> /sys/devices/platform, that makes more
sense

The problem with names is that we don't *know* that your devices are
at the root and unique. They don't have to be. I have platforms that
have several "chips" each containing all the same devices. They need to
be de-duped.

Maybe the right approach is to build the de-duplication in sysfs
itself ?

Cheers,
Ben.

> Any help understanding and/or fixing this discrepancy greatly appreciated.
> 
> The change of name is particularly annoying to me because one of my platform
> devices is a pwm_bl.c backlight.  With a boardfile I
> get /sys/class/pwm_backlight.  With devicetree the best I can get
> is /sys/class/pwm_backlight.23 (or similar).  It would be really nice to have
> a more stable and sensible name here.

Re: Strange location and name for platform devices when device-tree is used.

[Benjamin Herrenschmidt]

On Fri, 2013-11-01 at 15:22 +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2013-11-01 at 14:59 +1100, NeilBrown wrote:
> > and I wonder how relevant it still is in this context.  As platform devices
> > are all in the root of the device-tree and hence are siblings, they must have
> > unique names in the device-tree and  so the platform devices created from
> > them will also have unique names -- won't they?
> 
> I agree about /sys/devices -> /sys/devices/platform, that makes more
> sense
> 
> The problem with names is that we don't *know* that your devices are
> at the root and unique. They don't have to be. I have platforms that
> have several "chips" each containing all the same devices. They need to
> be de-duped.
> 
> Maybe the right approach is to build the de-duplication in sysfs
> itself ?

BTW. How come you have devices at the root of the tree without "reg" ?
It's fairly fishy ... 

The root of the tree is supposed to represent the processor address
space, and has #address-cells/#size-cells set appropriately. Any MMIO
mapped device shall thus have a "reg" property and a unit address.

Only "container" nodes (such as /cpus or /chosen) or virtual devices
(such as a node used to representing the collection of bits & pieces
that makes the audio infrastructure) and are thus not per-se MMIO mapped
entities can ommit the "reg" properties.

In the case of pwm, it looks like there's another device providing a pwm
capability, in which case your backlight would indeed be a "virtual
device" (basically non-mmio device not hanging off any bus). Or it could
have been represented as a child of pwm if that had been defined that
way, I am not familiar with the pwm bindings.

Cheers,
Ben.

> Cheers,
> Ben.
> 
> > Any help understanding and/or fixing this discrepancy greatly appreciated.
> > 
> > The change of name is particularly annoying to me because one of my platform
> > devices is a pwm_bl.c backlight.  With a boardfile I
> > get /sys/class/pwm_backlight.  With devicetree the best I can get
> > is /sys/class/pwm_backlight.23 (or similar).  It would be really nice to have
> > a more stable and sensible name here.
> 

Re: Strange location and name for platform devices when device-tree is used.

[NeilBrown]

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

On Fri, 01 Nov 2013 15:27:34 +1100 Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:

> On Fri, 2013-11-01 at 15:22 +1100, Benjamin Herrenschmidt wrote:
> > On Fri, 2013-11-01 at 14:59 +1100, NeilBrown wrote:
> > > and I wonder how relevant it still is in this context.  As platform devices
> > > are all in the root of the device-tree and hence are siblings, they must have
> > > unique names in the device-tree and  so the platform devices created from
> > > them will also have unique names -- won't they?
> > 
> > I agree about /sys/devices -> /sys/devices/platform, that makes more
> > sense
> > 
> > The problem with names is that we don't *know* that your devices are
> > at the root and unique. They don't have to be. I have platforms that
> > have several "chips" each containing all the same devices. They need to
> > be de-duped.
> > 
> > Maybe the right approach is to build the de-duplication in sysfs
> > itself ?

Do you mean we could allow multiple devices on the one bus to have the same
name, but get sysfs to notice and de-duplicate by mangling one name?  I don't
think I like that but I might have misunderstood.

On my device I seem to have some platform devices registered through
device-tree, and some registered through platform_device_add (e.g.
'alarmtimer').  Guaranteeing they remain disjoint sets if the kernel is
allowed to evolve independently of the devicetree might be tricky....
Maybe we need "/sys/devices/platform" and "/sys/devices/dt_platform" ??

Hoping someone who understands the device model better than me will help.

> 
> BTW. How come you have devices at the root of the tree without "reg" ?
> It's fairly fishy ... 
> 
> The root of the tree is supposed to represent the processor address
> space, and has #address-cells/#size-cells set appropriately. Any MMIO
> mapped device shall thus have a "reg" property and a unit address.
> 
> Only "container" nodes (such as /cpus or /chosen) or virtual devices
> (such as a node used to representing the collection of bits & pieces
> that makes the audio infrastructure) and are thus not per-se MMIO mapped
> entities can ommit the "reg" properties.
> 
> In the case of pwm, it looks like there's another device providing a pwm
> capability, in which case your backlight would indeed be a "virtual
> device" (basically non-mmio device not hanging off any bus). Or it could
> have been represented as a child of pwm if that had been defined that
> way, I am not familiar with the pwm bindings.

The  'backlight' device is a virtual device.  It uses a 'pwm' device to
provide the variable brightness to the back light.

The 'pwm' device itself is virtual, making use of a 'dmtimer' to provide the
timing...  The timer device (timer11 in omap3.dtsi) has a 'reg' property.

A random example from current mainline is 
  arch/arm/boot/dts/am335x-evmsk.dts
which has 'backlight' as a virtual device compatible with pwm-backlight.
It also has  'gpio-keys', 'gpio-leds', and 'regulator-fixed' compatible
virtual devices.  They seem fairly common.

Thanks,
NeilBrown


> 
> Cheers,
> Ben.
> 
> > Cheers,
> > Ben.
> > 
> > > Any help understanding and/or fixing this discrepancy greatly appreciated.
> > > 
> > > The change of name is particularly annoying to me because one of my platform
> > > devices is a pwm_bl.c backlight.  With a boardfile I
> > > get /sys/class/pwm_backlight.  With devicetree the best I can get
> > > is /sys/class/pwm_backlight.23 (or similar).  It would be really nice to have
> > > a more stable and sensible name here.
> > 
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Strange location and name for platform devices when device-tree is used.

[Benjamin Herrenschmidt]

On Fri, 2013-11-01 at 16:03 +1100, NeilBrown wrote:

> Do you mean we could allow multiple devices on the one bus to have the same
> name, but get sysfs to notice and de-duplicate by mangling one name?  I don't
> think I like that but I might have misunderstood.

What other option do we have ?

> On my device I seem to have some platform devices registered through
> device-tree, and some registered through platform_device_add (e.g.
> 'alarmtimer').  Guaranteeing they remain disjoint sets if the kernel is
> allowed to evolve independently of the devicetree might be tricky....
> Maybe we need "/sys/devices/platform" and "/sys/devices/dt_platform" ??

No, I think device-tree created platform devices should go
to /sys/devices/platform like the "classic" ones.

The problem is really how to deal with potential name duplication. We
could try to register, if we get -EEXIST (assuming sysfs returns the
right stuff), try again with ".1" etc...

> Hoping someone who understands the device model better than me will help.

Greg ? :-)

 .../...

> The  'backlight' device is a virtual device.  It uses a 'pwm' device to
> provide the variable brightness to the back light.
> 
> The 'pwm' device itself is virtual, making use of a 'dmtimer' to provide the
> timing...  The timer device (timer11 in omap3.dtsi) has a 'reg' property.
> 
> A random example from current mainline is 
>   arch/arm/boot/dts/am335x-evmsk.dts
> which has 'backlight' as a virtual device compatible with pwm-backlight.
> It also has  'gpio-keys', 'gpio-leds', and 'regulator-fixed' compatible
> virtual devices.  They seem fairly common.

Ok.

Cheers,
Ben.

> Thanks,
> NeilBrown
> 
> 
> > 
> > Cheers,
> > Ben.
> > 
> > > Cheers,
> > > Ben.
> > > 
> > > > Any help understanding and/or fixing this discrepancy greatly appreciated.
> > > > 
> > > > The change of name is particularly annoying to me because one of my platform
> > > > devices is a pwm_bl.c backlight.  With a boardfile I
> > > > get /sys/class/pwm_backlight.  With devicetree the best I can get
> > > > is /sys/class/pwm_backlight.23 (or similar).  It would be really nice to have
> > > > a more stable and sensible name here.
> > > 
> > 
> 

Re: Strange location and name for platform devices when device-tree is used.

[Grant Likely]

On Fri, 01 Nov 2013 16:08:36 +1100, Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> On Fri, 2013-11-01 at 16:03 +1100, NeilBrown wrote:
> 
> > Do you mean we could allow multiple devices on the one bus to have the same
> > name, but get sysfs to notice and de-duplicate by mangling one name?  I don't
> > think I like that but I might have misunderstood.
> 
> What other option do we have ?
> 
> > On my device I seem to have some platform devices registered through
> > device-tree, and some registered through platform_device_add (e.g.
> > 'alarmtimer').  Guaranteeing they remain disjoint sets if the kernel is
> > allowed to evolve independently of the devicetree might be tricky....
> > Maybe we need "/sys/devices/platform" and "/sys/devices/dt_platform" ??
> 
> No, I think device-tree created platform devices should go
> to /sys/devices/platform like the "classic" ones.
> 
> The problem is really how to deal with potential name duplication. We
> could try to register, if we get -EEXIST (assuming sysfs returns the
> right stuff), try again with ".1" etc...

I'd be fine with that approach. As long as the names get created
uniquely there shouldn't be a problem.

> > Hoping someone who understands the device model better than me will help.
> 
> Greg ? :-)

There are two problems here. First, making the change moves all the DT
populated devices under the /sys/devices/platform tree, not just
platform devices.

Second, I expect there is going to be userspace breakage to move them.
I've considered moving them before, but so far have felt that being
tidier hasn't been worth the potential breakage. Userspace /shouldn't/
be relying on the node name, but we all know that userspace always does
what it should, right?

That said, I'm mostly concerned about breakage on Power machines, not on
the ARM devices in this regard. If you are convinced that I'm worrying about
nothing, then I'm fine with making the change. If anyone complains
however then it will need to be reverted.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Strange location and name for platform devices when device-tree is used.

[Benjamin Herrenschmidt]

On Fri, 2013-11-01 at 11:04 -0700, Grant Likely wrote:

> There are two problems here. First, making the change moves all the DT
> populated devices under the /sys/devices/platform tree, not just
> platform devices.

All DT populated *platform* devices. There are others that have their
own locations.

> Second, I expect there is going to be userspace breakage to move them.
> I've considered moving them before, but so far have felt that being
> tidier hasn't been worth the potential breakage. Userspace /shouldn't/
> be relying on the node name, but we all know that userspace always does
> what it should, right?
> 
> That said, I'm mostly concerned about breakage on Power machines, not on
> the ARM devices in this regard. If you are convinced that I'm worrying about
> nothing, then I'm fine with making the change. If anyone complains
> however then it will need to be reverted.

I don't see an issue with power.

Cheers,
Ben.

Re: Strange location and name for platform devices when device-tree is used.

[Grant Likely]

On Sat, 02 Nov 2013 07:33:21 +1100, Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> On Fri, 2013-11-01 at 11:04 -0700, Grant Likely wrote:
> 
> > There are two problems here. First, making the change moves all the DT
> > populated devices under the /sys/devices/platform tree, not just
> > platform devices.
> 
> All DT populated *platform* devices. There are others that have their
> own locations.

They are all in the same tree! Move the platform devices and everything
else moves to because they are under them.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Strange location and name for platform devices when device-tree is used.

[Greg Kroah-Hartman]

On Fri, Nov 01, 2013 at 11:04:59AM -0700, Grant Likely wrote:
> Second, I expect there is going to be userspace breakage to move them.
> I've considered moving them before, but so far have felt that being
> tidier hasn't been worth the potential breakage. Userspace /shouldn't/
> be relying on the node name, but we all know that userspace always does
> what it should, right?

Userspace should _never_ rely on where in the /sys/devices/ tree a
device is, if so, it's _really_ broken.  They should be iterating and
getting to the device from within the bus and class section of the tree
as devices move all over the place when the box boots different times.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Strange location and name for platform devices when device-tree is used.

[Greg Kroah-Hartman]

On Fri, Nov 01, 2013 at 04:08:36PM +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2013-11-01 at 16:03 +1100, NeilBrown wrote:
> 
> > Do you mean we could allow multiple devices on the one bus to have the same
> > name, but get sysfs to notice and de-duplicate by mangling one name?  I don't
> > think I like that but I might have misunderstood.
> 
> What other option do we have ?
> 
> > On my device I seem to have some platform devices registered through
> > device-tree, and some registered through platform_device_add (e.g.
> > 'alarmtimer').  Guaranteeing they remain disjoint sets if the kernel is
> > allowed to evolve independently of the devicetree might be tricky....
> > Maybe we need "/sys/devices/platform" and "/sys/devices/dt_platform" ??
> 
> No, I think device-tree created platform devices should go
> to /sys/devices/platform like the "classic" ones.
> 
> The problem is really how to deal with potential name duplication. We
> could try to register, if we get -EEXIST (assuming sysfs returns the
> right stuff), try again with ".1" etc...

How can there be device name collisions?  All platform devices _should_
be named uniquely, if not, you have bigger problems...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Strange location and name for platform devices when device-tree is used.

[Benjamin Herrenschmidt]

On Fri, 2013-11-01 at 13:47 -0700, Greg Kroah-Hartman wrote:

> > > On my device I seem to have some platform devices registered through
> > > device-tree, and some registered through platform_device_add (e.g.
> > > 'alarmtimer').  Guaranteeing they remain disjoint sets if the kernel is
> > > allowed to evolve independently of the devicetree might be tricky....
> > > Maybe we need "/sys/devices/platform" and "/sys/devices/dt_platform" ??
> > 
> > No, I think device-tree created platform devices should go
> > to /sys/devices/platform like the "classic" ones.
> > 
> > The problem is really how to deal with potential name duplication. We
> > could try to register, if we get -EEXIST (assuming sysfs returns the
> > right stuff), try again with ".1" etc...
> 
> How can there be device name collisions?  All platform devices _should_
> be named uniquely, if not, you have bigger problems...

The problem is how to create a unique name from a platform device
created from a device-tree node.

Device tree nodes aren't necessarily uniquely named. They are unique
under a given parent but that hierarchy isn't preserved when creating
corresponding platform devices (and it would be very tricky to do so).

Currently, we simply append a number to the name when creating them,
which is obtained from a global counter.

Neil is unhappy about that because on his specific hardware, the device
has a unique name and thus we introduce a naming difference between
device-tree usage and old-style "hard coded" board file usage.

It would be nice if we could do something that only appends the "global
number" at the end of the name if the name isn't already unique. Thus my
proposal of trying first with the base name, and trying again if that
returns -EEXIST in some kind of loop.

Do you have a better idea ?

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Strange location and name for platform devices when device-tree is used.

[Greg Kroah-Hartman]

On Sat, Nov 02, 2013 at 10:09:59AM +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2013-11-01 at 13:47 -0700, Greg Kroah-Hartman wrote:
> 
> > > > On my device I seem to have some platform devices registered through
> > > > device-tree, and some registered through platform_device_add (e.g.
> > > > 'alarmtimer').  Guaranteeing they remain disjoint sets if the kernel is
> > > > allowed to evolve independently of the devicetree might be tricky....
> > > > Maybe we need "/sys/devices/platform" and "/sys/devices/dt_platform" ??
> > > 
> > > No, I think device-tree created platform devices should go
> > > to /sys/devices/platform like the "classic" ones.
> > > 
> > > The problem is really how to deal with potential name duplication. We
> > > could try to register, if we get -EEXIST (assuming sysfs returns the
> > > right stuff), try again with ".1" etc...
> > 
> > How can there be device name collisions?  All platform devices _should_
> > be named uniquely, if not, you have bigger problems...
> 
> The problem is how to create a unique name from a platform device
> created from a device-tree node.

I have no idea, and frankly, I don't care, I leave that to the
device-tree maintainers, as they are the ones that are using platform
devices for this type of kernel interaction.

So go blame them for this, not me, remember, I'm the one who _hates_
platform devices and wish they had never been created...

> Device tree nodes aren't necessarily uniquely named. They are unique
> under a given parent but that hierarchy isn't preserved when creating
> corresponding platform devices (and it would be very tricky to do so).
> 
> Currently, we simply append a number to the name when creating them,
> which is obtained from a global counter.

That's the best solution, you can't rely on device ids being the same
for any type of bus, that's just life.

> Neil is unhappy about that because on his specific hardware, the device
> has a unique name and thus we introduce a naming difference between
> device-tree usage and old-style "hard coded" board file usage.
> 
> It would be nice if we could do something that only appends the "global
> number" at the end of the name if the name isn't already unique. Thus my
> proposal of trying first with the base name, and trying again if that
> returns -EEXIST in some kind of loop.

Just loop through all the platform devices before registering it to
determine if you need to do this, the platform code can do this just
fine.  If you try to register a duplicate name with the driver core,
odds are it will complain loudly, so don't do that.

good luck,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Strange location and name for platform devices when device-tree is used.

[Benjamin Herrenschmidt]

On Sat, 2013-11-02 at 08:58 -0700, Greg Kroah-Hartman wrote:
> Just loop through all the platform devices before registering it to
> determine if you need to do this, the platform code can do this just
> fine.  If you try to register a duplicate name with the driver core,
> odds are it will complain loudly, so don't do that.

But that loop + registration is racy ... oh well, we might do something
better with Neil's idea of labels instead.

Ben.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Strange location and name for platform devices when device-tree is used.

[Greg Kroah-Hartman]

On Sun, Nov 03, 2013 at 07:22:10AM +1100, Benjamin Herrenschmidt wrote:
> On Sat, 2013-11-02 at 08:58 -0700, Greg Kroah-Hartman wrote:
> > Just loop through all the platform devices before registering it to
> > determine if you need to do this, the platform code can do this just
> > fine.  If you try to register a duplicate name with the driver core,
> > odds are it will complain loudly, so don't do that.
> 
> But that loop + registration is racy ... oh well, we might do something
> better with Neil's idea of labels instead.

How is it racy?  Only one platform device should be allowed to be
registered at a time, there is a per-bus lock that should be used to
enforce this, right?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Strange location and name for platform devices when device-tree is used.

[Benjamin Herrenschmidt]

On Sat, 2013-11-02 at 13:40 -0700, Greg Kroah-Hartman wrote:
> On Sun, Nov 03, 2013 at 07:22:10AM +1100, Benjamin Herrenschmidt wrote:
> > On Sat, 2013-11-02 at 08:58 -0700, Greg Kroah-Hartman wrote:
> > > Just loop through all the platform devices before registering it to
> > > determine if you need to do this, the platform code can do this just
> > > fine.  If you try to register a duplicate name with the driver core,
> > > odds are it will complain loudly, so don't do that.
> > 
> > But that loop + registration is racy ... oh well, we might do something
> > better with Neil's idea of labels instead.
> 
> How is it racy?  Only one platform device should be allowed to be
> registered at a time, there is a per-bus lock that should be used to
> enforce this, right?

No, platform devices can be registered from different "sources" and
possibly from different threads. There's no fundamental reason why the
registration would have to be serialized accross all possible entities
capable of crating them (though it generally is at the moment).

There's no global mutex used by every piece of code in the kernel that
might create/register a platform device.

However, let's leave that aside for now and go back to the original
issue because that's probably worth discussing a bit more.

Normally, a device "name" in sysfs shouldn't be relevant to user space,
I think we agree. Devices get identified by function (class typically)
and additional functional attributes associated with them.

For platform devices, things get a bit more murky though, ie, let's say
we have a backlight device, that's good ... but nothing tells us what
specific display it controls (there could be more than one in a system).

The device-tree typically provide such cross linking, but sysfs doesn't.

My understanding, and Neil correct me if I'm wrong, is that it's become
a case where userspace has started relying on the "name" of the platform
device to identify it's specific function within a class. Is that
correct ? IE. Which screen this backlight is associated to isn't an
explicit information provided somewhere in sysfs but something intuited
by user space tools based on the platform device name.

Greg, I'm not saying that's a good thing btw :-) I'm just trying to get
a grasp of the exact scope of the problem.

Now, there's a way for userspace to sort that out using device-tree of
course, they can parse /proc/device-tree, find the linkage, and related
the device-tree nodes to the corresponding platform devices (are we
creating the devspec files btw ?)

But that only works for DT-created platform devices, not the hand-made
ones.

Another option would be for classes that are meaningless "seldomly" such
as a backlight control (ie, it's always attached to *something*, a
screen for example, etc...) to associate a separate name which isn't the
device name in sysfs but a "label" as Neil proposed. In this case we'd
need to add an attribute but I would object to this being specific to
pwm_bl, instead of should generally be an attribute of all backlight
devices.

This could then be class specific... but should it be ? it might
actually be a useful thing to generally be able to label devices,
especially in the embedded world.

But that's just one option ... the point remains, people seem to be
relying on platform device names in sysfs and yes, that's not right, but
we need to understand why they do it and what's the right way to do to
replace that practice. 

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Strange location and name for platform devices when device-tree is used.

[NeilBrown]

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

On Sat, 2 Nov 2013 08:58:24 -0700 Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:


> So go blame them for this, not me, remember, I'm the one who _hates_
> platform devices and wish they had never been created...

Have you ever written up why you hate them?  I did a bit of googling and
couldn't find anything obvious.

Is it the whole concept, or the particular details that you don't like?

(I find the concept seems to make perfect sense, though there are a lot of
details that I don't know).

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Strange location and name for platform devices when device-tree is used.

[Benjamin Herrenschmidt]

On Fri, 2013-11-01 at 13:47 -0700, Greg Kroah-Hartman wrote:

> > > On my device I seem to have some platform devices registered through
> > > device-tree, and some registered through platform_device_add (e.g.
> > > 'alarmtimer').  Guaranteeing they remain disjoint sets if the kernel is
> > > allowed to evolve independently of the devicetree might be tricky....
> > > Maybe we need "/sys/devices/platform" and "/sys/devices/dt_platform" ??
> > 
> > No, I think device-tree created platform devices should go
> > to /sys/devices/platform like the "classic" ones.
> > 
> > The problem is really how to deal with potential name duplication. We
> > could try to register, if we get -EEXIST (assuming sysfs returns the
> > right stuff), try again with ".1" etc...
> 
> How can there be device name collisions?  All platform devices _should_
> be named uniquely, if not, you have bigger problems...

The problem is how to create a unique name for a platform device created
from a device-tree node.

Device tree nodes aren't necessarily uniquely named. They are unique
under a given parent but that hierarchy isn't preserved when creating
corresponding platform devices (and it would be very tricky to do so).

Currently, we simply append a number to the name when creating them,
which is obtained from a global counter.

Neil is unhappy about that because on his specific hardware, the device
has a unique name and thus we introduce a naming difference between
device-tree usage and old-style "hard coded" board file usage.

It would be nice if we could do something that only appends the "global
number" at the end of the name if the name isn't already unique. Thus my
proposal of trying first with the base name, and trying again if that
returns -EEXIST in some kind of loop.

Do you have a better idea ?

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Strange location and name for platform devices when device-tree is used.

[NeilBrown]

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

On Sat, 02 Nov 2013 10:10:25 +1100 Benjamin Herrenschmidt
<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:

> On Fri, 2013-11-01 at 13:47 -0700, Greg Kroah-Hartman wrote:
> 
> > > > On my device I seem to have some platform devices registered through
> > > > device-tree, and some registered through platform_device_add (e.g.
> > > > 'alarmtimer').  Guaranteeing they remain disjoint sets if the kernel is
> > > > allowed to evolve independently of the devicetree might be tricky....
> > > > Maybe we need "/sys/devices/platform" and "/sys/devices/dt_platform" ??
> > > 
> > > No, I think device-tree created platform devices should go
> > > to /sys/devices/platform like the "classic" ones.
> > > 
> > > The problem is really how to deal with potential name duplication. We
> > > could try to register, if we get -EEXIST (assuming sysfs returns the
> > > right stuff), try again with ".1" etc...
> > 
> > How can there be device name collisions?  All platform devices _should_
> > be named uniquely, if not, you have bigger problems...
> 
> The problem is how to create a unique name for a platform device created
> from a device-tree node.
> 
> Device tree nodes aren't necessarily uniquely named. They are unique
> under a given parent but that hierarchy isn't preserved when creating
> corresponding platform devices (and it would be very tricky to do so).
> 
> Currently, we simply append a number to the name when creating them,
> which is obtained from a global counter.
> 
> Neil is unhappy about that because on his specific hardware, the device
> has a unique name and thus we introduce a naming difference between
> device-tree usage and old-style "hard coded" board file usage.

It occurs to me that a different approach could solve my problem.

My problem stems from the fact that the name of the device on the
platform-bus is used as the name of the device in the "backlight" class.

As Greg writes elsewhere, depending on names with /sys/devices is not
supported - we need to accept that bus-names might change.
However names in class devices tend to be a lot more stable.
Several devices allow these to be explicitly set.
 leds have 'label'
 regulators has "regulator-name"
 gpio-keys has 'label'.

I could just teach pwm_bl to allow a 'label' property which would be used in
place of the platform-bus device name when creating the class/backlight
device.

The maxim "you cannot trust names to remain stable in /sys/devices" can
justify both the movement of platform devices into /sys/devices/platform, and
the use of "label" rather than the device-name for creating the class device.

Does that sound convincing?

Thanks,
NeilBrown

> 
> It would be nice if we could do something that only appends the "global
> number" at the end of the name if the name isn't already unique. Thus my
> proposal of trying first with the base name, and trying again if that
> returns -EEXIST in some kind of loop.
> 
> Do you have a better idea ?
> 
> Cheers,
> Ben.
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Strange location and name for platform devices when device-tree is used.

[Thierry Reding]

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

On Sat, Nov 02, 2013 at 10:45:05AM +1100, NeilBrown wrote:
> On Sat, 02 Nov 2013 10:10:25 +1100 Benjamin Herrenschmidt
> <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> 
> > On Fri, 2013-11-01 at 13:47 -0700, Greg Kroah-Hartman wrote:
> > 
> > > > > On my device I seem to have some platform devices registered through
> > > > > device-tree, and some registered through platform_device_add (e.g.
> > > > > 'alarmtimer').  Guaranteeing they remain disjoint sets if the kernel is
> > > > > allowed to evolve independently of the devicetree might be tricky....
> > > > > Maybe we need "/sys/devices/platform" and "/sys/devices/dt_platform" ??
> > > > 
> > > > No, I think device-tree created platform devices should go
> > > > to /sys/devices/platform like the "classic" ones.
> > > > 
> > > > The problem is really how to deal with potential name duplication. We
> > > > could try to register, if we get -EEXIST (assuming sysfs returns the
> > > > right stuff), try again with ".1" etc...
> > > 
> > > How can there be device name collisions?  All platform devices _should_
> > > be named uniquely, if not, you have bigger problems...
> > 
> > The problem is how to create a unique name for a platform device created
> > from a device-tree node.
> > 
> > Device tree nodes aren't necessarily uniquely named. They are unique
> > under a given parent but that hierarchy isn't preserved when creating
> > corresponding platform devices (and it would be very tricky to do so).
> > 
> > Currently, we simply append a number to the name when creating them,
> > which is obtained from a global counter.
> > 
> > Neil is unhappy about that because on his specific hardware, the device
> > has a unique name and thus we introduce a naming difference between
> > device-tree usage and old-style "hard coded" board file usage.
> 
> It occurs to me that a different approach could solve my problem.
> 
> My problem stems from the fact that the name of the device on the
> platform-bus is used as the name of the device in the "backlight" class.
> 
> As Greg writes elsewhere, depending on names with /sys/devices is not
> supported - we need to accept that bus-names might change.
> However names in class devices tend to be a lot more stable.
> Several devices allow these to be explicitly set.
>  leds have 'label'
>  regulators has "regulator-name"
>  gpio-keys has 'label'.
> 
> I could just teach pwm_bl to allow a 'label' property which would be used in
> place of the platform-bus device name when creating the class/backlight
> device.
> 
> The maxim "you cannot trust names to remain stable in /sys/devices" can
> justify both the movement of platform devices into /sys/devices/platform, and
> the use of "label" rather than the device-name for creating the class device.
> 
> Does that sound convincing?

I see how this is a general problem, but since you've brought up the
topic of backlights, let me reply to that.

The correct thing, in my opinion, to do here would be to associate the
backlight with whatever device it is attached to. Typically this would
be a display. fbdev has some support for that already, and if I read the
code correctly then it automatically associates any backlight device
with any fbdev device (which obviously breaks once you start having more
of each, but that's probably not a typical use-case).

In my experience associating a backlight with its display and letting
the display driver control the backlight is the only sane solution. On
one hand it makes the association explicit, so userspace doesn't have to
hardcode any sysfs filenames to access the backlight. Furthermore only
the display driver knows when it's safe to turn on and off the backlight
since it controls the flow of pixels to the display.

Currently the Linux backlight framework turns on the backlight right
when it is registered (or more precisely that's what each individual
driver does). That means depending on the probe order the backlight will
light up before there's any meaningful content on the display. In many
cases that may turn out to be just black, but in other cases it may be
random content.

However, I'm not aware of any way to control the backlight from an fbdev
device in userspace. fbdev is also mostly considered obsolete these days
and I've been working on integrating backlight and panels with DRM for
quite a while now. That's also still missing a way to control the
backlight brightness via DRM IOCTLs, but it's on my TODO.

That probably doesn't help with the overall situation, but at least I
think it's what we should be doing for backlight devices.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Strange location and name for platform devices when device-tree is used.

[Grant Likely]

On Sat, 2 Nov 2013 10:45:05 +1100, NeilBrown <neilb@suse.de> wrote:
> On Sat, 02 Nov 2013 10:10:25 +1100 Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> 
> > On Fri, 2013-11-01 at 13:47 -0700, Greg Kroah-Hartman wrote:
> > 
> > > > > On my device I seem to have some platform devices registered through
> > > > > device-tree, and some registered through platform_device_add (e.g.
> > > > > 'alarmtimer').  Guaranteeing they remain disjoint sets if the kernel is
> > > > > allowed to evolve independently of the devicetree might be tricky....
> > > > > Maybe we need "/sys/devices/platform" and "/sys/devices/dt_platform" ??
> > > > 
> > > > No, I think device-tree created platform devices should go
> > > > to /sys/devices/platform like the "classic" ones.
> > > > 
> > > > The problem is really how to deal with potential name duplication. We
> > > > could try to register, if we get -EEXIST (assuming sysfs returns the
> > > > right stuff), try again with ".1" etc...
> > > 
> > > How can there be device name collisions?  All platform devices _should_
> > > be named uniquely, if not, you have bigger problems...
> > 
> > The problem is how to create a unique name for a platform device created
> > from a device-tree node.
> > 
> > Device tree nodes aren't necessarily uniquely named. They are unique
> > under a given parent but that hierarchy isn't preserved when creating
> > corresponding platform devices (and it would be very tricky to do so).
> > 
> > Currently, we simply append a number to the name when creating them,
> > which is obtained from a global counter.
> > 
> > Neil is unhappy about that because on his specific hardware, the device
> > has a unique name and thus we introduce a naming difference between
> > device-tree usage and old-style "hard coded" board file usage.
> 
> It occurs to me that a different approach could solve my problem.
> 
> My problem stems from the fact that the name of the device on the
> platform-bus is used as the name of the device in the "backlight" class.
> 
> As Greg writes elsewhere, depending on names with /sys/devices is not
> supported - we need to accept that bus-names might change.
> However names in class devices tend to be a lot more stable.
> Several devices allow these to be explicitly set.
>  leds have 'label'
>  regulators has "regulator-name"
>  gpio-keys has 'label'.
> 
> I could just teach pwm_bl to allow a 'label' property which would be used in
> place of the platform-bus device name when creating the class/backlight
> device.
> 
> The maxim "you cannot trust names to remain stable in /sys/devices" can
> justify both the movement of platform devices into /sys/devices/platform, and
> the use of "label" rather than the device-name for creating the class device.
> 
> Does that sound convincing?

The problem with a property in the node in this case is that there is no
guarantee of uniqueness. An alternative however would be to look for an
alias in /aliases. Those are unique.

g.