| Message ID | 20240910045443.678145-1-luke@ljones.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: asus-wmi: don't fail if platform_profile already registered | expand |
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.
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.
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);
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); > >
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);
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(-)