| Message ID | 20210119081513.300938-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias | expand |
On Tue, Jan 19, 2021 at 04:15:13PM +0800, Kai-Heng Feng wrote: > Commit 8765c5ba1949 ("ACPI / scan: Rework modalias creation when > "compatible" is present") may create two "MODALIAS=" in uevent file if > conditions are met. > > This breaks systemd-udevd, which assumes each "key" in uevent file is > unique. The internal implementation of systemd-udevd overwrites the > first MODALIAS with the second one, so its kmod rule doesn't load driver > for the first MODALIAS. > > Right now it doesn't seem to have any user relies on the second > MODALIAS, so change it to OF_MODALIAS to workaround the issue. > > Reference: https://github.com/systemd/systemd/pull/18163 > Fixes: 8765c5ba1949 ("ACPI / scan: Rework modalias creation when "compatible" is present") > Cc: AceLan Kao <acelan.kao@canonical.com> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>, > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/acpi/device_sysfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c > index 96869f1538b9..c92b671cb816 100644 > --- a/drivers/acpi/device_sysfs.c > +++ b/drivers/acpi/device_sysfs.c > @@ -260,7 +260,7 @@ int __acpi_device_uevent_modalias(struct acpi_device *adev, > if (!adev->data.of_compatible) > return 0; > > - if (len > 0 && add_uevent_var(env, "MODALIAS=")) > + if (len > 0 && add_uevent_var(env, "OF_MODALIAS=")) Who will use OF_MODALIAS and where have you documented it? thanks, greg k-h
On Tue, Jan 19, 2021 at 4:27 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Jan 19, 2021 at 04:15:13PM +0800, Kai-Heng Feng wrote: > > Commit 8765c5ba1949 ("ACPI / scan: Rework modalias creation when > > "compatible" is present") may create two "MODALIAS=" in uevent file if > > conditions are met. > > > > This breaks systemd-udevd, which assumes each "key" in uevent file is > > unique. The internal implementation of systemd-udevd overwrites the > > first MODALIAS with the second one, so its kmod rule doesn't load driver > > for the first MODALIAS. > > > > Right now it doesn't seem to have any user relies on the second > > MODALIAS, so change it to OF_MODALIAS to workaround the issue. > > > > Reference: https://github.com/systemd/systemd/pull/18163 > > Fixes: 8765c5ba1949 ("ACPI / scan: Rework modalias creation when "compatible" is present") > > Cc: AceLan Kao <acelan.kao@canonical.com> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>, > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > drivers/acpi/device_sysfs.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c > > index 96869f1538b9..c92b671cb816 100644 > > --- a/drivers/acpi/device_sysfs.c > > +++ b/drivers/acpi/device_sysfs.c > > @@ -260,7 +260,7 @@ int __acpi_device_uevent_modalias(struct acpi_device *adev, > > if (!adev->data.of_compatible) > > return 0; > > > > - if (len > 0 && add_uevent_var(env, "MODALIAS=")) > > + if (len > 0 && add_uevent_var(env, "OF_MODALIAS=")) > > Who will use OF_MODALIAS and where have you documented it? After this lands in mainline, I'll modify the pull request for systemd to add a new rule for OF_MODALIAS. I'll modify the comment on the function to document the change. Kai-Heng > > thanks, > > greg k-h
On Tue, Jan 19, 2021 at 04:41:48PM +0800, Kai-Heng Feng wrote: > On Tue, Jan 19, 2021 at 4:27 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Tue, Jan 19, 2021 at 04:15:13PM +0800, Kai-Heng Feng wrote: ... > > Who will use OF_MODALIAS and where have you documented it? > > After this lands in mainline, I'll modify the pull request for systemd > to add a new rule for OF_MODALIAS. > I'll modify the comment on the function to document the change. I'm wondering why to have two fixes in two places instead of fixing udev to understand multiple MODALIAS= events?
On Tue, Jan 19, 2021 at 11:41:59AM +0200, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 04:41:48PM +0800, Kai-Heng Feng wrote: > > On Tue, Jan 19, 2021 at 4:27 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > On Tue, Jan 19, 2021 at 04:15:13PM +0800, Kai-Heng Feng wrote: > > ... > > > > Who will use OF_MODALIAS and where have you documented it? > > > > After this lands in mainline, I'll modify the pull request for systemd > > to add a new rule for OF_MODALIAS. > > I'll modify the comment on the function to document the change. > > I'm wondering why to have two fixes in two places instead of fixing udev to > understand multiple MODALIAS= events? It's not a matter of multiple events, it's a single event with a key/value pair with duplicate keys and different values. What is this event with different values supposed to be doing in userspace? Do you want multiple invocations of `modprobe` or something else? Usually a "device" only has a single "signature" that modprobe uses to look up the correct module for. Modules can support any number of device signatures, but traditionally it is odd to think that a device itself can be supported by multiple modules, which is what you are saying is happening here. So what should userspace do with this, and why does a device need to have multiple module alias signatures? thanks, greg k-h
On Tue, Jan 19, 2021 at 6:34 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Jan 19, 2021 at 11:41:59AM +0200, Andy Shevchenko wrote: > > On Tue, Jan 19, 2021 at 04:41:48PM +0800, Kai-Heng Feng wrote: > > > On Tue, Jan 19, 2021 at 4:27 PM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Jan 19, 2021 at 04:15:13PM +0800, Kai-Heng Feng wrote: > > > > ... > > > > > > Who will use OF_MODALIAS and where have you documented it? > > > > > > After this lands in mainline, I'll modify the pull request for systemd > > > to add a new rule for OF_MODALIAS. > > > I'll modify the comment on the function to document the change. > > > > I'm wondering why to have two fixes in two places instead of fixing udev to > > understand multiple MODALIAS= events? > > It's not a matter of multiple events, it's a single event with a > key/value pair with duplicate keys and different values. > > What is this event with different values supposed to be doing in > userspace? Do you want multiple invocations of `modprobe` or something > else? > > Usually a "device" only has a single "signature" that modprobe uses to > look up the correct module for. Modules can support any number of > device signatures, but traditionally it is odd to think that a device > itself can be supported by multiple modules, which is what you are > saying is happening here. > > So what should userspace do with this, and why does a device need to > have multiple module alias signatures? From the original use case [1], I think the "compatible" modalias should be enough. Andy and Mika, what do you think? Can we remove the ACPI modalias for this case? [1] https://lwn.net/Articles/612062/ Kai-Heng > > thanks, > > greg k-h
On Thu, Jan 21, 2021 at 02:22:43PM +0800, Kai-Heng Feng wrote: > On Tue, Jan 19, 2021 at 6:34 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Jan 19, 2021 at 11:41:59AM +0200, Andy Shevchenko wrote: > > > On Tue, Jan 19, 2021 at 04:41:48PM +0800, Kai-Heng Feng wrote: > > > > On Tue, Jan 19, 2021 at 4:27 PM Greg Kroah-Hartman > > > > <gregkh@linuxfoundation.org> wrote: > > > > > On Tue, Jan 19, 2021 at 04:15:13PM +0800, Kai-Heng Feng wrote: > > > > > > ... > > > > > > > > Who will use OF_MODALIAS and where have you documented it? > > > > > > > > After this lands in mainline, I'll modify the pull request for systemd > > > > to add a new rule for OF_MODALIAS. > > > > I'll modify the comment on the function to document the change. > > > > > > I'm wondering why to have two fixes in two places instead of fixing udev to > > > understand multiple MODALIAS= events? > > > > It's not a matter of multiple events, it's a single event with a > > key/value pair with duplicate keys and different values. > > > > What is this event with different values supposed to be doing in > > userspace? Do you want multiple invocations of `modprobe` or something > > else? > > > > Usually a "device" only has a single "signature" that modprobe uses to > > look up the correct module for. Modules can support any number of > > device signatures, but traditionally it is odd to think that a device > > itself can be supported by multiple modules, which is what you are > > saying is happening here. > > > > So what should userspace do with this, and why does a device need to > > have multiple module alias signatures? > > >From the original use case [1], I think the "compatible" modalias > should be enough. > Andy and Mika, what do you think? Can we remove the ACPI modalias for this case? Yes, I think that should work. After all we want the match to happen through the DT compatible string if the property is present, not through ACPI IDs.
diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c index 96869f1538b9..c92b671cb816 100644 --- a/drivers/acpi/device_sysfs.c +++ b/drivers/acpi/device_sysfs.c @@ -260,7 +260,7 @@ int __acpi_device_uevent_modalias(struct acpi_device *adev, if (!adev->data.of_compatible) return 0; - if (len > 0 && add_uevent_var(env, "MODALIAS=")) + if (len > 0 && add_uevent_var(env, "OF_MODALIAS=")) return -ENOMEM; len = create_of_modalias(adev, &env->buf[env->buflen - 1],
Commit 8765c5ba1949 ("ACPI / scan: Rework modalias creation when "compatible" is present") may create two "MODALIAS=" in uevent file if conditions are met. This breaks systemd-udevd, which assumes each "key" in uevent file is unique. The internal implementation of systemd-udevd overwrites the first MODALIAS with the second one, so its kmod rule doesn't load driver for the first MODALIAS. Right now it doesn't seem to have any user relies on the second MODALIAS, so change it to OF_MODALIAS to workaround the issue. Reference: https://github.com/systemd/systemd/pull/18163 Fixes: 8765c5ba1949 ("ACPI / scan: Rework modalias creation when "compatible" is present") Cc: AceLan Kao <acelan.kao@canonical.com> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Cc: Mika Westerberg <mika.westerberg@linux.intel.com>, Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/acpi/device_sysfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)