Comments

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(-)

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

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?

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 :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

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.

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

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@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",
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

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?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html