Re: [PATCH] i2c: Fix modalias for ACPI enumerated I2C devices

Re: [PATCH] i2c: Fix modalias for ACPI enumerated I2C devices

[Grant Likely]

On Wed, Oct 16, 2013 at 12:47 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Tuesday, October 15, 2013 04:31:43 PM Greg Kroah-Hartman wrote:
>> On Tue, Oct 15, 2013 at 11:24:01PM +0200, Rafael J. Wysocki wrote:
>> > On Tuesday, October 15, 2013 01:48:29 PM Greg Kroah-Hartman wrote:
>> > > On Tue, Oct 15, 2013 at 10:37:02PM +0200, Rafael J. Wysocki wrote:
>> > > > On Tuesday, October 15, 2013 07:44:44 PM Zhang Rui wrote:
>> > > > > On Mon, 2013-10-14 at 20:47 +0800, Zhang Rui wrote:
>> > > > > > On Mon, 2013-10-14 at 14:18 +0300, Jarkko Nikula wrote:
>> > > > > > > On 10/14/2013 12:23 PM, Zhang Rui wrote:
>> > > > > > > > Hi,
>> > > > > > > >
>> > > > > > > > On Thu, 2013-10-10 at 17:17 +0300, Jarkko Nikula wrote:
>> > > > > > > >> There is a minor fault about ACPI enumerated I2C devices with their modalias
>> > > > > > > >> attribute. Now modalias is set by device instance not by hardware ID.
>> > > > > > > >> For example "i2c:INTABCD:00", "i2c:INTABCD:01" etc.
>> > > > > > > >>
>> > > > > > > >> This means each device instance gets different modalias which does match
>> > > > > > > >> with generated modules.alias. Currently this is not problem as matching can
>> > > > > > > >> happen also with "acpi:INTABCD" modalias.
>> > > > > > > >>
>> > > > > > > > IMO, this is not the proper fix for the modalias problem because ACPI
>> > > > > > > > enumerated I2C device may have compatible ids.
>> > > > > > > > Instead, we should export all the compatible ids as the modules alias of
>> > > > > > > > the ACPI enumerated I2C device.
>> > > > > > > >
>> > > > > > > > can you please take a look at the patch I sent out earlier?
>> > > > > > > > https://patchwork.kernel.org/patch/3034991/
>> > > > > > > > https://patchwork.kernel.org/patch/3035041/
>> > > > > > > > https://patchwork.kernel.org/patch/3035021/
>> > > > > > > I see. This makes sense as it avoids that same device has two different
>> > > > > > > modaliases from both acpi and other subsystem.
>> > > > > > >
>> > > > > > > How about modalias nodes in sysfs, should they also reflect what is
>> > > > > > > matching uvent?
>> > > > > > >
>> > > > > > good catch, will fix "modalias" as well in next version.
>> > > > >
>> > > > > Hi,
>> > > > >
>> > > > > I have a question about the device "uevent" and "modalias" sysfs
>> > > > > attributes.
>> > > > > what is the relationship between these two?
>> > > > > Am I right to say that, if there is the "MODALIAS" field in uevent file,
>> > > > > this field must be consistent with the content in "modalias" attribute?
>> > >
>> > > Well, if it isn't, it's pretty pointless, right?
>> > >
>> > > > > I checked the code in drivers/base/platform.c,
>> > > > > static ssize_t modalias_show(struct device *dev, struct device_attribute
>> > > > > *a,
>> > > > >                              char *buf)
>> > > > > {
>> > > > >         struct platform_device  *pdev = to_platform_device(dev);
>> > > > >         int len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
>> > > > >
>> > > > >         return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
>> > > > > }
>> > > > >
>> > > > > static int platform_uevent(struct device *dev, struct kobj_uevent_env
>> > > > > *env)
>> > > > > {
>> > > > >         struct platform_device  *pdev = to_platform_device(dev);
>> > > > >         int rc;
>> > > > >
>> > > > >         /* Some devices have extra OF data and an OF-style MODALIAS */
>> > > > >         rc = of_device_uevent_modalias(dev, env);
>> > > > >         if (rc != -ENODEV)
>> > > > >                 return rc;
>> > > > >
>> > > > >         add_uevent_var(env, "MODALIAS=%s%s", PLATFORM_MODULE_PREFIX,
>> > > > >                         pdev->name);
>> > > > >         return 0;
>> > > > > }
>> > > > >
>> > > > > This means that the OF-style MODALIAS is not shown in "modalias" sysfs
>> > > > > attribute.
>> > > > > is this a bug?
>> > > >
>> > > > I would consider that as a bug, but I'm not sure what the recommended practice
>> > > > is.  Greg?
>> > >
>> > > I have no idea how the OF stuff is working, and honestly, I really have
>> > > no wish to ever know anything about it.  Especially when it comes to
>> > > platform devices/drivers, something that I personally hate and wish
>> > > would be deleted.

<digress>Greg, I've heard you say that a lot, but regardless of what
platform devices/drivers were originally designed for, it is pretty
much exactly what we need for non-discoverable memory mapped busses.
I've yet to heard a viable alternative proposed. I've heard the
proposal of creating new bus types and new driver binding to that bus
type for each variant of a non-discoverable memory mapped bus, but I
think it is a non-starter. There are too many combinations. What
/might/ work to replace the platform_bus_type would be to have a
mechanism for drivers to transparently bind to multiple bus types, but
then I suspect that it will end up looking an awful lot like the
existing platform_bus_type.

Probably worth discussing over beer next week</digress>

>> > >
>> > > So go ask the OF maintainers/developers, this is their domain :)
>> >
>> > Well, OK.  Whom in particular?
>>
>> The "OPEN FIRMWARE AND FLATTENED DEVICE TREE" entries in MAINTAINERS?
>
> Hmm, that looks like Grant and Rob Herring.
>
> Grant, Rob, do we want the OF-style MODALIAS to be shown in "modalias" sysfs
> attributes of platform devices?

I've not actually had to deal with modutils and modalias on any of the
platforms I've worked with so I've only looked at it superficially.
Yes, that does look like a bug, but Ben would probably have a much
better idea [cc'd].

g.

Re: [PATCH] i2c: Fix modalias for ACPI enumerated I2C devices

[Greg Kroah-Hartman]

On Wed, Oct 16, 2013 at 01:04:02AM +0100, Grant Likely wrote:
> On Wed, Oct 16, 2013 at 12:47 AM, Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org> wrote:
> > On Tuesday, October 15, 2013 04:31:43 PM Greg Kroah-Hartman wrote:
> >> On Tue, Oct 15, 2013 at 11:24:01PM +0200, Rafael J. Wysocki wrote:
> >> > On Tuesday, October 15, 2013 01:48:29 PM Greg Kroah-Hartman wrote:
> >> > > On Tue, Oct 15, 2013 at 10:37:02PM +0200, Rafael J. Wysocki wrote:
> >> > > > On Tuesday, October 15, 2013 07:44:44 PM Zhang Rui wrote:
> >> > > > > On Mon, 2013-10-14 at 20:47 +0800, Zhang Rui wrote:
> >> > > > > > On Mon, 2013-10-14 at 14:18 +0300, Jarkko Nikula wrote:
> >> > > > > > > On 10/14/2013 12:23 PM, Zhang Rui wrote:
> >> > > > > > > > Hi,
> >> > > > > > > >
> >> > > > > > > > On Thu, 2013-10-10 at 17:17 +0300, Jarkko Nikula wrote:
> >> > > > > > > >> There is a minor fault about ACPI enumerated I2C devices with their modalias
> >> > > > > > > >> attribute. Now modalias is set by device instance not by hardware ID.
> >> > > > > > > >> For example "i2c:INTABCD:00", "i2c:INTABCD:01" etc.
> >> > > > > > > >>
> >> > > > > > > >> This means each device instance gets different modalias which does match
> >> > > > > > > >> with generated modules.alias. Currently this is not problem as matching can
> >> > > > > > > >> happen also with "acpi:INTABCD" modalias.
> >> > > > > > > >>
> >> > > > > > > > IMO, this is not the proper fix for the modalias problem because ACPI
> >> > > > > > > > enumerated I2C device may have compatible ids.
> >> > > > > > > > Instead, we should export all the compatible ids as the modules alias of
> >> > > > > > > > the ACPI enumerated I2C device.
> >> > > > > > > >
> >> > > > > > > > can you please take a look at the patch I sent out earlier?
> >> > > > > > > > https://patchwork.kernel.org/patch/3034991/
> >> > > > > > > > https://patchwork.kernel.org/patch/3035041/
> >> > > > > > > > https://patchwork.kernel.org/patch/3035021/
> >> > > > > > > I see. This makes sense as it avoids that same device has two different
> >> > > > > > > modaliases from both acpi and other subsystem.
> >> > > > > > >
> >> > > > > > > How about modalias nodes in sysfs, should they also reflect what is
> >> > > > > > > matching uvent?
> >> > > > > > >
> >> > > > > > good catch, will fix "modalias" as well in next version.
> >> > > > >
> >> > > > > Hi,
> >> > > > >
> >> > > > > I have a question about the device "uevent" and "modalias" sysfs
> >> > > > > attributes.
> >> > > > > what is the relationship between these two?
> >> > > > > Am I right to say that, if there is the "MODALIAS" field in uevent file,
> >> > > > > this field must be consistent with the content in "modalias" attribute?
> >> > >
> >> > > Well, if it isn't, it's pretty pointless, right?
> >> > >
> >> > > > > I checked the code in drivers/base/platform.c,
> >> > > > > static ssize_t modalias_show(struct device *dev, struct device_attribute
> >> > > > > *a,
> >> > > > >                              char *buf)
> >> > > > > {
> >> > > > >         struct platform_device  *pdev = to_platform_device(dev);
> >> > > > >         int len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
> >> > > > >
> >> > > > >         return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
> >> > > > > }
> >> > > > >
> >> > > > > static int platform_uevent(struct device *dev, struct kobj_uevent_env
> >> > > > > *env)
> >> > > > > {
> >> > > > >         struct platform_device  *pdev = to_platform_device(dev);
> >> > > > >         int rc;
> >> > > > >
> >> > > > >         /* Some devices have extra OF data and an OF-style MODALIAS */
> >> > > > >         rc = of_device_uevent_modalias(dev, env);
> >> > > > >         if (rc != -ENODEV)
> >> > > > >                 return rc;
> >> > > > >
> >> > > > >         add_uevent_var(env, "MODALIAS=%s%s", PLATFORM_MODULE_PREFIX,
> >> > > > >                         pdev->name);
> >> > > > >         return 0;
> >> > > > > }
> >> > > > >
> >> > > > > This means that the OF-style MODALIAS is not shown in "modalias" sysfs
> >> > > > > attribute.
> >> > > > > is this a bug?
> >> > > >
> >> > > > I would consider that as a bug, but I'm not sure what the recommended practice
> >> > > > is.  Greg?
> >> > >
> >> > > I have no idea how the OF stuff is working, and honestly, I really have
> >> > > no wish to ever know anything about it.  Especially when it comes to
> >> > > platform devices/drivers, something that I personally hate and wish
> >> > > would be deleted.
> 
> <digress>Greg, I've heard you say that a lot, but regardless of what
> platform devices/drivers were originally designed for, it is pretty
> much exactly what we need for non-discoverable memory mapped busses.
> I've yet to heard a viable alternative proposed. I've heard the
> proposal of creating new bus types and new driver binding to that bus
> type for each variant of a non-discoverable memory mapped bus, but I
> think it is a non-starter. There are too many combinations. What
> /might/ work to replace the platform_bus_type would be to have a
> mechanism for drivers to transparently bind to multiple bus types, but
> then I suspect that it will end up looking an awful lot like the
> existing platform_bus_type.
> 
> Probably worth discussing over beer next week</digress>

I think we have a whole session about this at the ARM summit next week,
and if someone wants to bring beer into it, that will make me happy!

Re: [PATCH] i2c: Fix modalias for ACPI enumerated I2C devices

[Jarkko Nikula]

On 10/16/2013 03:04 AM, Grant Likely wrote:
> On Wed, Oct 16, 2013 at 12:47 AM, Rafael J. Wysocki<rjw@sisk.pl>  wrote:
>> On Tuesday, October 15, 2013 04:31:43 PM Greg Kroah-Hartman wrote:
>>> On Tue, Oct 15, 2013 at 11:24:01PM +0200, Rafael J. Wysocki wrote:
>>>> On Tuesday, October 15, 2013 01:48:29 PM Greg Kroah-Hartman wrote:
>>>>> On Tue, Oct 15, 2013 at 10:37:02PM +0200, Rafael J. Wysocki wrote:
>>>>>> On Tuesday, October 15, 2013 07:44:44 PM Zhang Rui wrote:
>>>>>>> I have a question about the device "uevent" and "modalias" sysfs
>>>>>>> attributes.
>>>>>>> what is the relationship between these two?
>>>>>>> Am I right to say that, if there is the "MODALIAS" field in uevent file,
>>>>>>> this field must be consistent with the content in "modalias" attribute?
>>>>> Well, if it isn't, it's pretty pointless, right?
>>>>>>> static int platform_uevent(struct device *dev, struct kobj_uevent_env
>>>>>>> *env)
>>>>>>> {
>>>>>>>          struct platform_device  *pdev = to_platform_device(dev);
>>>>>>>          int rc;
>>>>>>>
>>>>>>>          /* Some devices have extra OF data and an OF-style MODALIAS */
>>>>>>>          rc = of_device_uevent_modalias(dev, env);
>>>>>>>          if (rc != -ENODEV)
>>>>>>>                  return rc;
>>>>>>>
>>>>>>>          add_uevent_var(env, "MODALIAS=%s%s", PLATFORM_MODULE_PREFIX,
>>>>>>>                          pdev->name);
>>>>>>>          return 0;
>>>>>>> }
>>>>>>>
>>>>>>> This means that the OF-style MODALIAS is not shown in "modalias" sysfs
>>>>>>> attribute.
>>>>>>> is this a bug?
Here is an example from one DT based system:

cat /sys/bus/platform/devices/48070000.i2c/uevent
DRIVER=omap_i2c
OF_NAME=i2c
OF_FULLNAME=/ocp/i2c@48070000
OF_COMPATIBLE_0=ti,omap4-i2c
OF_COMPATIBLE_N=1
MODALIAS=of:Ni2cT<NULL>Cti,omap4-i2c

cat /sys/bus/platform/devices/48070000.i2c/modalias
platform:48070000.i2c

And a device on that I2C bus:

cat /sys/bus/platform/devices/rtc.11/uevent
DRIVER=twl_rtc
OF_NAME=rtc
OF_FULLNAME=/ocp/i2c@48070000/twl@48/rtc
OF_COMPATIBLE_0=ti,twl4030-rtc
OF_COMPATIBLE_N=1
MODALIAS=of:NrtcT<NULL>Cti,twl4030-rtc

cat /sys/bus/platform/devices/rtc.11/modalias
platform:rtc.11

Unfortunately I cannot debug above example further at the moment is 
there failing or needless modprobe calls. Maybe device tree experts know 
better?

-- 
Jarkko