[v1] platform/x86: asus-wmi: don't fail if platform_profile already registered

[PATCH] platform/x86: asus-wmi: don't fail if platform_profile already registered

Posted by Luke D. Jones 1 year, 5 months ago

On some newer laptops ASUS laptops SPS support is advertised but not
actually used, causing the AMD driver to register as a platform_profile
handler.

If this happens then the asus_wmi driver would error with -EEXIST when
trying to register its own handler leaving the user with a possibly
unusable system. This is especially true for laptops with an MCU that emit
a stream of HID packets, some of which can be misinterpreted as shutdown
signals.

We can safely continue loading the driver instead of bombing out.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index fbb3345cc65a..d53c4aff519f 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3876,8 +3876,13 @@ static int platform_profile_setup(struct asus_wmi *asus)
 		asus->platform_profile_handler.choices);
 
 	err = platform_profile_register(&asus->platform_profile_handler);
-	if (err)
+	if (err == -EEXIST) {
+		pr_warn("%s, a platform_profile handler is already registered\n", __func__);
+		return 0;
+	} else if (err) {
+		pr_err("%s, failed at platform_profile_register: %d\n", __func__, err);
 		return err;
+	}
 
 	asus->platform_profile_support = true;
 	return 0;
@@ -4752,7 +4757,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 		goto fail_fan_boost_mode;
 
 	err = platform_profile_setup(asus);
-	if (err)
+	if (err && err != -EEXIST)
 		goto fail_platform_profile_setup;
 
 	err = asus_wmi_sysfs_init(asus->platform_device);
-- 
2.46.0

Re: [PATCH] platform/x86: asus-wmi: don't fail if platform_profile already registered

Posted by Hans de Goede 1 year, 4 months ago

Hi,

On 9/10/24 6:54 AM, Luke D. Jones wrote:
> On some newer laptops ASUS laptops SPS support is advertised but not
> actually used, causing the AMD driver to register as a platform_profile
> handler.
> 
> If this happens then the asus_wmi driver would error with -EEXIST when
> trying to register its own handler leaving the user with a possibly
> unusable system. This is especially true for laptops with an MCU that emit
> a stream of HID packets, some of which can be misinterpreted as shutdown
> signals.
> 
> We can safely continue loading the driver instead of bombing out.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>

Thank you for your patch. I've applied this now, dropping the second
unnecessary chunk manually so there is no need to send out a new version.

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> ---
>  drivers/platform/x86/asus-wmi.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index fbb3345cc65a..d53c4aff519f 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -3876,8 +3876,13 @@ static int platform_profile_setup(struct asus_wmi *asus)
>  		asus->platform_profile_handler.choices);
>  
>  	err = platform_profile_register(&asus->platform_profile_handler);
> -	if (err)
> +	if (err == -EEXIST) {
> +		pr_warn("%s, a platform_profile handler is already registered\n", __func__);
> +		return 0;
> +	} else if (err) {
> +		pr_err("%s, failed at platform_profile_register: %d\n", __func__, err);
>  		return err;
> +	}
>  
>  	asus->platform_profile_support = true;
>  	return 0;
> @@ -4752,7 +4757,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>  		goto fail_fan_boost_mode;
>  
>  	err = platform_profile_setup(asus);
> -	if (err)
> +	if (err && err != -EEXIST)
>  		goto fail_platform_profile_setup;
>  
>  	err = asus_wmi_sysfs_init(asus->platform_device);

Re: [PATCH] platform/x86: asus-wmi: don't fail if platform_profile already registered

Posted by Luke Jones 1 year, 4 months ago

On Thu, 12 Sep 2024, at 12:32 AM, Hans de Goede wrote:
> Hi,
> 
> On 9/10/24 6:54 AM, Luke D. Jones wrote:
> > On some newer laptops ASUS laptops SPS support is advertised but not
> > actually used, causing the AMD driver to register as a platform_profile
> > handler.
> > 
> > If this happens then the asus_wmi driver would error with -EEXIST when
> > trying to register its own handler leaving the user with a possibly
> > unusable system. This is especially true for laptops with an MCU that emit
> > a stream of HID packets, some of which can be misinterpreted as shutdown
> > signals.
> > 
> > We can safely continue loading the driver instead of bombing out.
> > 
> > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> 
> Thank you for your patch. I've applied this now, dropping the second
> unnecessary chunk manually so there is no need to send out a new version.
> 
> Thank you for your patch, I've applied this patch to my review-hans 
> branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> 
> Once I've run some tests on this branch the patches there will be
> added to the platform-drivers-x86/for-next branch and eventually
> will be included in the pdx86 pull-request to Linus for the next
> merge-window.
> 
> Regards,
> 
> Hans


Okay Thanks Hans,


> 
> 
> 
> > ---
> >  drivers/platform/x86/asus-wmi.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > index fbb3345cc65a..d53c4aff519f 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -3876,8 +3876,13 @@ static int platform_profile_setup(struct asus_wmi *asus)
> >  asus->platform_profile_handler.choices);
> >  
> >  err = platform_profile_register(&asus->platform_profile_handler);
> > - if (err)
> > + if (err == -EEXIST) {
> > + pr_warn("%s, a platform_profile handler is already registered\n", __func__);
> > + return 0;
> > + } else if (err) {
> > + pr_err("%s, failed at platform_profile_register: %d\n", __func__, err);
> >  return err;
> > + }
> >  
> >  asus->platform_profile_support = true;
> >  return 0;
> > @@ -4752,7 +4757,7 @@ static int asus_wmi_add(struct platform_device *pdev)
> >  goto fail_fan_boost_mode;
> >  
> >  err = platform_profile_setup(asus);
> > - if (err)
> > + if (err && err != -EEXIST)
> >  goto fail_platform_profile_setup;
> >  
> >  err = asus_wmi_sysfs_init(asus->platform_device);
> 
>

Re: [PATCH] platform/x86: asus-wmi: don't fail if platform_profile already registered

Posted by Ilpo Järvinen 1 year, 5 months ago

On Tue, 10 Sep 2024, Luke D. Jones wrote:

> On some newer laptops ASUS laptops SPS support is advertised but not
> actually used, causing the AMD driver to register as a platform_profile
> handler.
> 
> If this happens then the asus_wmi driver would error with -EEXIST when
> trying to register its own handler leaving the user with a possibly
> unusable system. This is especially true for laptops with an MCU that emit
> a stream of HID packets, some of which can be misinterpreted as shutdown
> signals.
> 
> We can safely continue loading the driver instead of bombing out.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index fbb3345cc65a..d53c4aff519f 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -3876,8 +3876,13 @@ static int platform_profile_setup(struct asus_wmi *asus)
>  		asus->platform_profile_handler.choices);
>  
>  	err = platform_profile_register(&asus->platform_profile_handler);
> -	if (err)
> +	if (err == -EEXIST) {
> +		pr_warn("%s, a platform_profile handler is already registered\n", __func__);
> +		return 0;
> +	} else if (err) {
> +		pr_err("%s, failed at platform_profile_register: %d\n", __func__, err);

Don't print __func__ in user visible warn/error/info messages but use 
plain English please.

>  		return err;
> +	}
>  
>  	asus->platform_profile_support = true;
>  	return 0;
> @@ -4752,7 +4757,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>  		goto fail_fan_boost_mode;
>  
>  	err = platform_profile_setup(asus);
> -	if (err)
> +	if (err && err != -EEXIST)

Hi Luke,

Hans had a comment about this line against the previous version.

Also, this patch has entirely lost the version information. It's v3 now I 
think.


-- 
 i.

Re: [PATCH] platform/x86: asus-wmi: don't fail if platform_profile already registered

Posted by Luke Jones 1 year, 5 months ago

On Tue, 10 Sep 2024, at 9:39 PM, Ilpo Järvinen wrote:
> On Tue, 10 Sep 2024, Luke D. Jones wrote:
> 
> > On some newer laptops ASUS laptops SPS support is advertised but not
> > actually used, causing the AMD driver to register as a platform_profile
> > handler.
> > 
> > If this happens then the asus_wmi driver would error with -EEXIST when
> > trying to register its own handler leaving the user with a possibly
> > unusable system. This is especially true for laptops with an MCU that emit
> > a stream of HID packets, some of which can be misinterpreted as shutdown
> > signals.
> > 
> > We can safely continue loading the driver instead of bombing out.
> > 
> > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > ---
> >  drivers/platform/x86/asus-wmi.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > index fbb3345cc65a..d53c4aff519f 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -3876,8 +3876,13 @@ static int platform_profile_setup(struct asus_wmi *asus)
> >  asus->platform_profile_handler.choices);
> >  
> >  err = platform_profile_register(&asus->platform_profile_handler);
> > - if (err)
> > + if (err == -EEXIST) {
> > + pr_warn("%s, a platform_profile handler is already registered\n", __func__);
> > + return 0;
> > + } else if (err) {
> > + pr_err("%s, failed at platform_profile_register: %d\n", __func__, err);
> 
> Don't print __func__ in user visible warn/error/info messages but use 
> plain English please.

Ack. I've done this a few times now. Sorry I missed this one.

> 
> >  return err;
> > + }
> >  
> >  asus->platform_profile_support = true;
> >  return 0;
> > @@ -4752,7 +4757,7 @@ static int asus_wmi_add(struct platform_device *pdev)
> >  goto fail_fan_boost_mode;
> >  
> >  err = platform_profile_setup(asus);
> > - if (err)
> > + if (err && err != -EEXIST)
> 
> Hi Luke,
> 
> Hans had a comment about this line against the previous version.
> 
> Also, this patch has entirely lost the version information. It's v3 now I 
> think.

Uh.... sorry, I seem to have lost it in the depths of my inbox even with filters and searching. I'll look it up on lore and ensure everything is correct next go round.

Cheers,
Luke.