diff mbox series

[1/1,SRU,OEM-OSP1-B] UBUNTU: SAUCE: platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

Message ID 20200507102524.10261-2-koba.ko@canonical.com
State Rejected
Headers show
Series UBUNTU: SAUCE: don't register platform::micmute if the related tokens don't exist. | expand

Commit Message

Koba Ko May 7, 2020, 10:25 a.m. UTC 
From: Koba Ko <koba.ko@canonical.com>

BugLink: https://bugs.launchpad.net/bugs/1877275

Error messge is issued,
"platform::micmute: Setting an LED's brightness failed (-19)",
Even the device isn't presented.

Get the related tokens of SMBIOS, GLOBAL_MIC_MUTE_DISABLE/ENABLE.
If one of two tokens doesn't exist, don't register platform::micmute.

Signed-off-by: Koba Ko <koba.ko@canonical.com>
---
 drivers/platform/x86/dell-laptop.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Sultan Alsawaf May 13, 2020, 5:47 p.m. UTC | #1 
Hi,

This change breaks the error path in this function because it can perform an
unregister on the led classdev when it is not registered. It looks like you
realized this is a problem with your LKML submission, so you should update this
sauce patch accordingly. Also, you should implement the other comments that the
reviewers on LKML left for you [1].

Thanks,
Sultan

[1] https://patchwork.kernel.org/patch/11533437

On Thu, May 07, 2020 at 06:25:24PM +0800, koba.ko@canonical.com wrote:
> From: Koba Ko <koba.ko@canonical.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1877275
> 
> Error messge is issued,
> "platform::micmute: Setting an LED's brightness failed (-19)",
> Even the device isn't presented.
> 
> Get the related tokens of SMBIOS, GLOBAL_MIC_MUTE_DISABLE/ENABLE.
> If one of two tokens doesn't exist, don't register platform::micmute.
> 
> Signed-off-by: Koba Ko <koba.ko@canonical.com>
> ---
>  drivers/platform/x86/dell-laptop.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 1e46022fb2c5..afc1ded83e56 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -2208,10 +2208,13 @@ static int __init dell_init(void)
>  
>  	dell_laptop_register_notifier(&dell_laptop_notifier);
>  
> -	micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> -	ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
> -	if (ret < 0)
> -		goto fail_led;
> +	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> +	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> +		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> +		ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
> +		if (ret < 0)
> +			goto fail_led;
> +	}
>  
>  	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>  		return 0;
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Koba Ko May 14, 2020, 12:21 a.m. UTC | #2 
On Thu, May 14, 2020 at 1:48 AM Sultan Alsawaf <sultan.alsawaf@canonical.com>
wrote:

> Hi,
>
> This change breaks the error path in this function because it can perform
> an
> unregister on the led classdev when it is not registered. It looks like you
> realized this is a problem with your LKML submission, so you should update
> this
> sauce patch accordingly. Also, you should implement the other comments
> that the
> reviewers on LKML left for you [1].
>
> Thanks,
> Sultan
>
> [1] https://patchwork.kernel.org/patch/11533437

Hi Sultan,
As per the message[A], it's not a strong requirement.
How do you think!?
[A]https://www.spinics.net/lists/platform-driver-x86/msg21661.html

>


> On Thu, May 07, 2020 at 06:25:24PM +0800, koba.ko@canonical.com wrote:
> > From: Koba Ko <koba.ko@canonical.com>
> >
> > BugLink: https://bugs.launchpad.net/bugs/1877275
> >
> > Error messge is issued,
> > "platform::micmute: Setting an LED's brightness failed (-19)",
> > Even the device isn't presented.
> >
> > Get the related tokens of SMBIOS, GLOBAL_MIC_MUTE_DISABLE/ENABLE.
> > If one of two tokens doesn't exist, don't register platform::micmute.
> >
> > Signed-off-by: Koba Ko <koba.ko@canonical.com>
> > ---
> >  drivers/platform/x86/dell-laptop.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell-laptop.c
> b/drivers/platform/x86/dell-laptop.c
> > index 1e46022fb2c5..afc1ded83e56 100644
> > --- a/drivers/platform/x86/dell-laptop.c
> > +++ b/drivers/platform/x86/dell-laptop.c
> > @@ -2208,10 +2208,13 @@ static int __init dell_init(void)
> >
> >       dell_laptop_register_notifier(&dell_laptop_notifier);
> >
> > -     micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > -     ret = led_classdev_register(&platform_device->dev,
> &micmute_led_cdev);
> > -     if (ret < 0)
> > -             goto fail_led;
> > +     if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> > +         dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> > +             micmute_led_cdev.brightness =
> ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > +             ret = led_classdev_register(&platform_device->dev,
> &micmute_led_cdev);
> > +             if (ret < 0)
> > +                     goto fail_led;
> > +     }
> >
> >       if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
> >               return 0;
> > --
> > 2.17.1
> >
> >
> > --
> > kernel-team mailing list
> > kernel-team@lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
Sultan Alsawaf May 14, 2020, 6:38 p.m. UTC | #3 
On Thu, May 14, 2020 at 08:21:27AM +0800, Koba Ko wrote:
> On Thu, May 14, 2020 at 1:48 AM Sultan Alsawaf <sultan.alsawaf@canonical.com>
> wrote:
> 
> > Hi,
> >
> > This change breaks the error path in this function because it can perform
> > an
> > unregister on the led classdev when it is not registered. It looks like you
> > realized this is a problem with your LKML submission, so you should update
> > this
> > sauce patch accordingly. Also, you should implement the other comments
> > that the
> > reviewers on LKML left for you [1].
> >
> > Thanks,
> > Sultan
> >
> > [1] https://patchwork.kernel.org/patch/11533437
> 
> Hi Sultan,
> As per the message[A], it's not a strong requirement.
> How do you think!?
> [A]https://www.spinics.net/lists/platform-driver-x86/msg21661.html

That looks correct to me, but that commit is only in Linux 5.4+. The bionic
linux-oem-osp1 kernel is based on 5.0, so it needs to have a check before
calling led_classdev_unregister().

I'll ack the oem-5.6 version you submitted once the commit message in that one
is cleaned up. You only need to remove these two lines from that commit message:
> (cherry picked from commit f0a165c6311a4b8c7b45bafcb4806f105f0e7aa1)
> Signed-off-by: Koba Ko <koba.ko@canonical.com>

You need to remove the "cherry picked from" because that isn't an upstream
cherry pick, and you need to remove your duplicate Signed-off-by. Then it'll
look good to me.

Sultan
Koba Ko May 15, 2020, 1:17 a.m. UTC | #4 
hi Sultan
Have updated v2 patch, please check [1].
[1] https://lists.ubuntu.com/archives/kernel-team/2020-May/109911.html

For OSP1, will modify it further.

Thanks
Koba Ko

On Fri, May 15, 2020 at 2:39 AM Sultan Alsawaf <sultan.alsawaf@canonical.com>
wrote:

> On Thu, May 14, 2020 at 08:21:27AM +0800, Koba Ko wrote:
> > On Thu, May 14, 2020 at 1:48 AM Sultan Alsawaf <
> sultan.alsawaf@canonical.com>
> > wrote:
> >
> > > Hi,
> > >
> > > This change breaks the error path in this function because it can
> perform
> > > an
> > > unregister on the led classdev when it is not registered. It looks
> like you
> > > realized this is a problem with your LKML submission, so you should
> update
> > > this
> > > sauce patch accordingly. Also, you should implement the other comments
> > > that the
> > > reviewers on LKML left for you [1].
> > >
> > > Thanks,
> > > Sultan
> > >
> > > [1] https://patchwork.kernel.org/patch/11533437
> >
> > Hi Sultan,
> > As per the message[A], it's not a strong requirement.
> > How do you think!?
> > [A]https://www.spinics.net/lists/platform-driver-x86/msg21661.html
>
> That looks correct to me, but that commit is only in Linux 5.4+. The bionic
> linux-oem-osp1 kernel is based on 5.0, so it needs to have a check before
> calling led_classdev_unregister().
>
> I'll ack the oem-5.6 version you submitted once the commit message in that
> one
> is cleaned up. You only need to remove these two lines from that commit
> message:
> > (cherry picked from commit f0a165c6311a4b8c7b45bafcb4806f105f0e7aa1)
> > Signed-off-by: Koba Ko <koba.ko@canonical.com>
>
> You need to remove the "cherry picked from" because that isn't an upstream
> cherry pick, and you need to remove your duplicate Signed-off-by. Then
> it'll
> look good to me.
>
> Sultan
>
diff mbox series

Patch

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 1e46022fb2c5..afc1ded83e56 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -2208,10 +2208,13 @@  static int __init dell_init(void)
 
 	dell_laptop_register_notifier(&dell_laptop_notifier);
 
-	micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
-	ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
-	if (ret < 0)
-		goto fail_led;
+	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
+	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
+		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
+		ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
+		if (ret < 0)
+			goto fail_led;
+	}
 
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
 		return 0;