[PATCH] ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias

[PATCH] ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias

[Kai-Heng Feng]

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="))
 		return -ENOMEM;
 
 	len = create_of_modalias(adev, &env->buf[env->buflen - 1],
-- 
2.29.2

Re: [PATCH] ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias

[Greg Kroah-Hartman]

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

Re: [PATCH] ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias

[Kai-Heng Feng]

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

Re: [PATCH] ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias

[Greg Kroah-Hartman]

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

Re: [PATCH] ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias

[Kai-Heng Feng]

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

Re: [PATCH] ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias

[Mika Westerberg]

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.