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

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

[Jarkko Nikula]

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.

Fix this by using ACPI hardware ID.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Generated on top of v3.12-rc4-29-g0e7a3ed.
---
 drivers/i2c/i2c-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 29d3f04..6dd0c53 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1111,7 +1111,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 	if (ret < 0 || !info.addr)
 		return AE_OK;
 
-	strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
+	strlcpy(info.type, acpi_device_hid(adev), sizeof(info.type));
 	if (!i2c_new_device(adapter, &info)) {
 		dev_err(&adapter->dev,
 			"failed to add I2C device %s from ACPI\n",
-- 
1.8.4.rc3

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

[Mika Westerberg]

+Rafael

On Thu, Oct 10, 2013 at 05:17:49PM +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.

I think that this is intentional. We don't want that the i2c modalias
matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI
IDs that are added to the driver to match with the ACPI device.

> 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.
> 
> Fix this by using ACPI hardware ID.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> Generated on top of v3.12-rc4-29-g0e7a3ed.
> ---
>  drivers/i2c/i2c-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 29d3f04..6dd0c53 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1111,7 +1111,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
>  	if (ret < 0 || !info.addr)
>  		return AE_OK;
>  
> -	strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
> +	strlcpy(info.type, acpi_device_hid(adev), sizeof(info.type));
>  	if (!i2c_new_device(adapter, &info)) {
>  		dev_err(&adapter->dev,
>  			"failed to add I2C device %s from ACPI\n",
> -- 
> 1.8.4.rc3

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

[Rafael J. Wysocki]

On Friday, October 11, 2013 05:49:46 PM Mika Westerberg wrote:
> +Rafael
> 
> On Thu, Oct 10, 2013 at 05:17:49PM +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.
> 
> I think that this is intentional. We don't want that the i2c modalias
> matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI
> IDs that are added to the driver to match with the ACPI device.

Well, I'm not really sure this was intentional, but I wonder how other bus
types work in that respect?

> > 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.
> > 
> > Fix this by using ACPI hardware ID.
> > 
> > Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > Cc: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> > Generated on top of v3.12-rc4-29-g0e7a3ed.
> > ---
> >  drivers/i2c/i2c-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index 29d3f04..6dd0c53 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -1111,7 +1111,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
> >  	if (ret < 0 || !info.addr)
> >  		return AE_OK;
> >  
> > -	strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
> > +	strlcpy(info.type, acpi_device_hid(adev), sizeof(info.type));
> >  	if (!i2c_new_device(adapter, &info)) {
> >  		dev_err(&adapter->dev,
> >  			"failed to add I2C device %s from ACPI\n",
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Mika Westerberg]

On Sat, Oct 12, 2013 at 12:16:02AM +0200, Rafael J. Wysocki wrote:
> > I think that this is intentional. We don't want that the i2c modalias
> > matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI
> > IDs that are added to the driver to match with the ACPI device.
> 
> Well, I'm not really sure this was intentional, but I wonder how other bus
> types work in that respect?

We have the same for platform bus, if that's what you are asking.

It probably doesn't hurt to have this patch applied but it might cause
inadvertent match if for some reason there is an I2C client driver that
happens to have INTABCD I2C id in its list.

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

[Rafael J. Wysocki]

On Saturday, October 12, 2013 08:04:13 AM Mika Westerberg wrote:
> On Sat, Oct 12, 2013 at 12:16:02AM +0200, Rafael J. Wysocki wrote:
> > > I think that this is intentional. We don't want that the i2c modalias
> > > matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI
> > > IDs that are added to the driver to match with the ACPI device.
> > 
> > Well, I'm not really sure this was intentional, but I wonder how other bus
> > types work in that respect?
> 
> We have the same for platform bus, if that's what you are asking.
> 
> It probably doesn't hurt to have this patch applied but it might cause
> inadvertent match if for some reason there is an I2C client driver that
> happens to have INTABCD I2C id in its list.

Well, if they have that id in their lists, they are supposed to be able to
handle this device, aren't they?  What other reason may be there for them
to put that id into their lists?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Mika Westerberg]

On Sat, Oct 12, 2013 at 03:45:15PM +0200, Rafael J. Wysocki wrote:
> On Saturday, October 12, 2013 08:04:13 AM Mika Westerberg wrote:
> > On Sat, Oct 12, 2013 at 12:16:02AM +0200, Rafael J. Wysocki wrote:
> > > > I think that this is intentional. We don't want that the i2c modalias
> > > > matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI
> > > > IDs that are added to the driver to match with the ACPI device.
> > > 
> > > Well, I'm not really sure this was intentional, but I wonder how other bus
> > > types work in that respect?
> > 
> > We have the same for platform bus, if that's what you are asking.
> > 
> > It probably doesn't hurt to have this patch applied but it might cause
> > inadvertent match if for some reason there is an I2C client driver that
> > happens to have INTABCD I2C id in its list.
> 
> Well, if they have that id in their lists, they are supposed to be able to
> handle this device, aren't they?  What other reason may be there for them
> to put that id into their lists?

If we have two ACPI enumerated devices, they have following modalias:

  i2c-device0:	i2c:INTABCD:00
		acpi:INTABCD

  i2c-device1:	i2c:INTABCD:01
  		acpi:INTABCD

Likelihood that some random I2C driver has INTABCD:00 or INTABCD:01 ids in
their list is minimal. However, when you turn it to this:


  i2c-device0:	i2c:INTABCD
		acpi:INTABCD

  i2c-device1:	i2c:INTABCD
  		acpi:INTABCD

It might be possible that we get a match that isn't supposed to happen.
Well, OK it is pretty remote but anyway :-)

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

[Jarkko Nikula]

On 10/12/2013 07:18 PM, Mika Westerberg wrote:
> If we have two ACPI enumerated devices, they have following modalias:
>
>    i2c-device0:	i2c:INTABCD:00
> 		acpi:INTABCD
>
>    i2c-device1:	i2c:INTABCD:01
>    		acpi:INTABCD
>
> Likelihood that some random I2C driver has INTABCD:00 or INTABCD:01 ids in
> their list is minimal. However, when you turn it to this:
>
>
>    i2c-device0:	i2c:INTABCD
> 		acpi:INTABCD
>
>    i2c-device1:	i2c:INTABCD
>    		acpi:INTABCD
>
> It might be possible that we get a match that isn't supposed to happen.
> Well, OK it is pretty remote but anyway :-)
Well, name conflicts could occur of course but still I don't think we 
should generate illegal or wrong modaliases. I'm not an udev expert but 
I suppose trying to load nonexisting drivers (i2c_INTABCD:xy) could slow 
booting a little and perhaps pollute needlessly error log compared to if 
it can see that driver is already loaded or tries to load the same 
driver again.

I don't think name conflicts can pose too big risk as they are trivial 
to fix in sources and can be queued to stable too.

-- 
Jarkko

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

[Mika Westerberg]

On Mon, Oct 14, 2013 at 09:45:53AM +0300, Jarkko Nikula wrote:
> On 10/12/2013 07:18 PM, Mika Westerberg wrote:
> >If we have two ACPI enumerated devices, they have following modalias:
> >
> >   i2c-device0:	i2c:INTABCD:00
> >		acpi:INTABCD
> >
> >   i2c-device1:	i2c:INTABCD:01
> >   		acpi:INTABCD
> >
> >Likelihood that some random I2C driver has INTABCD:00 or INTABCD:01 ids in
> >their list is minimal. However, when you turn it to this:
> >
> >
> >   i2c-device0:	i2c:INTABCD
> >		acpi:INTABCD
> >
> >   i2c-device1:	i2c:INTABCD
> >   		acpi:INTABCD
> >
> >It might be possible that we get a match that isn't supposed to happen.
> >Well, OK it is pretty remote but anyway :-)
> Well, name conflicts could occur of course but still I don't think
> we should generate illegal or wrong modaliases. I'm not an udev
> expert but I suppose trying to load nonexisting drivers
> (i2c_INTABCD:xy) could slow booting a little and perhaps pollute
> needlessly error log compared to if it can see that driver is
> already loaded or tries to load the same driver again.
> 
> I don't think name conflicts can pose too big risk as they are
> trivial to fix in sources and can be queued to stable too.

OK, you got me convinced :-)

No further objections from me.

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

[Jarkko Nikula]

On 10/12/2013 08:04 AM, Mika Westerberg wrote:
> On Sat, Oct 12, 2013 at 12:16:02AM +0200, Rafael J. Wysocki wrote:
>>> I think that this is intentional. We don't want that the i2c modalias
>>> matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI
>>> IDs that are added to the driver to match with the ACPI device.
>> Well, I'm not really sure this was intentional, but I wonder how other bus
>> types work in that respect?
> We have the same for platform bus, if that's what you are asking.
>
Do we? I don't recall seeing per device modaliases on other platforms on 
their platform buses.

And actually I don't see that happening in drivers/base/platform.c: 
platform_uevent() either where just pdev->name is used but not pdev->id 
(which is used with pdev->name for dev_set_name()).

This makes me thinking that perhaps "pdevinfo.name = 
dev_name(&adev->dev);" in drivers/acpi/acpi_platform.c: 
acpi_create_platform_device() should be fixed too as now modalias for 
ACPI registered platform devices differ from platform devices that are 
registered in other subsystems (e.g. regulatory, pcspkr, alarmtimer, etc 
devices)?

I can send a patch for that.

-- 
Jarkko

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

[Mika Westerberg]

On Mon, Oct 14, 2013 at 09:34:39AM +0300, Jarkko Nikula wrote:
> On 10/12/2013 08:04 AM, Mika Westerberg wrote:
> >On Sat, Oct 12, 2013 at 12:16:02AM +0200, Rafael J. Wysocki wrote:
> >>>I think that this is intentional. We don't want that the i2c modalias
> >>>matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI
> >>>IDs that are added to the driver to match with the ACPI device.
> >>Well, I'm not really sure this was intentional, but I wonder how other bus
> >>types work in that respect?
> >We have the same for platform bus, if that's what you are asking.
> >
> Do we? I don't recall seeing per device modaliases on other
> platforms on their platform buses.

I mean for platform devices enumerated from ACPI.

> And actually I don't see that happening in drivers/base/platform.c:
> platform_uevent() either where just pdev->name is used but not
> pdev->id (which is used with pdev->name for dev_set_name()).
> 
> This makes me thinking that perhaps "pdevinfo.name =
> dev_name(&adev->dev);" in drivers/acpi/acpi_platform.c:
> acpi_create_platform_device() should be fixed too as now modalias
> for ACPI registered platform devices differ from platform devices
> that are registered in other subsystems (e.g. regulatory, pcspkr,
> alarmtimer, etc devices)?

Well, if you think that it doesn't hit us back later if we get a match that
isn't supposed to happen.

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

[Zhang Rui]

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/

thanks,
rui

> Fix this by using ACPI hardware ID.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>


> ---
> Generated on top of v3.12-rc4-29-g0e7a3ed.
> ---
>  drivers/i2c/i2c-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 29d3f04..6dd0c53 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1111,7 +1111,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
>  	if (ret < 0 || !info.addr)
>  		return AE_OK;
>  
> -	strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
> +	strlcpy(info.type, acpi_device_hid(adev), sizeof(info.type));
>  	if (!i2c_new_device(adapter, &info)) {
>  		dev_err(&adapter->dev,
>  			"failed to add I2C device %s from ACPI\n",

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

[Jarkko Nikula]

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?

-- 
Jarkko

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

[Zhang Rui]

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.

thanks,
rui

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

[Zhang Rui]

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?

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?

thanks,
rui

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

[Rafael J. Wysocki]

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?
> 
> 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 speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Greg Kroah-Hartman]

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.

So go ask the OF maintainers/developers, this is their domain :)

sorry,

greg k-h

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

[Rafael J. Wysocki]

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.
> 
> So go ask the OF maintainers/developers, this is their domain :)

Well, OK.  Whom in particular?

Rafael

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

[Greg Kroah-Hartman]

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.
> > 
> > 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?

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

[Rafael J. Wysocki]

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.
> > > 
> > > 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?

Rafael

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

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

[Rafael J. Wysocki]

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.
> 
> So go ask the OF maintainers/developers, this is their domain :)

So Rui, let's consider that as a bug and please prepare a patch to fix it.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.