| Message ID | 20230913231829.192842-1-admin@dennisbonke.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | platform/x86: thinkpad_acpi: Take mutex in hotkey_resume | expand |
Hi Dennis, thanks for the fix! On 2023-09-14 01:18:29+0200, admin@dennisbonke.com wrote: > From: Dennis Bonke <admin@dennisbonke.com> > > hotkey_status_{set,get} expect the hotkey_mutex to be held. > It seems like it was missed here and that gives warnings while resuming. Which kind of warnings? If it's from lockdep then it's triggered by hotkey_mask_set() and the commit message is a bit off. Also then the patch needs: Fixes: 38831eaf7d4c ("platform/x86: thinkpad_acpi: use lockdep annotations") Cc: stable@vger.kernel.org With those: Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> > > Signed-off-by: Dennis Bonke <admin@dennisbonke.com> > --- > drivers/platform/x86/thinkpad_acpi.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index d70c89d32534..de5859a5eb0d 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -4116,9 +4116,11 @@ static void hotkey_resume(void) > { > tpacpi_disable_brightness_delay(); > > + mutex_lock(&hotkey_mutex) > if (hotkey_status_set(true) < 0 || > hotkey_mask_set(hotkey_acpi_mask) < 0) > pr_err("error while attempting to reset the event firmware interface\n"); > + mutex_unlock(&hotkey_mutex); > > tpacpi_send_radiosw_update(); > tpacpi_input_send_tabletsw(); > -- > 2.40.1 >
On Thu, 2023-09-14 at 01:33 +0200, Thomas Weißschuh wrote: > Hi Dennis, > > thanks for the fix! Hello Thomas, Thank you for the quick review! I apologize for the messy V2 that I seem to have posted. It's my first time working with a mailing list and it seems that something went wrong. > > On 2023-09-14 01:18:29+0200, admin@dennisbonke.com wrote: > > From: Dennis Bonke <admin@dennisbonke.com> > > > > hotkey_status_{set,get} expect the hotkey_mutex to be held. > > It seems like it was missed here and that gives warnings while resuming. > > Which kind of warnings? > > If it's from lockdep then it's triggered by hotkey_mask_set() and the > commit message is a bit off. It is indeed from lockdep. I've changed the commit message to reflect your comment. > > Also then the patch needs: > > Fixes: 38831eaf7d4c ("platform/x86: thinkpad_acpi: use lockdep annotations") > Cc: stable@vger.kernel.org > > With those: > > Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> About those tags, do I add them to the patch? Just double checking before I accidentally CC the stable list with an incorrect patch. > > > > > Signed-off-by: Dennis Bonke <admin@dennisbonke.com> > > --- > > drivers/platform/x86/thinkpad_acpi.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > > index d70c89d32534..de5859a5eb0d 100644 > > --- a/drivers/platform/x86/thinkpad_acpi.c > > +++ b/drivers/platform/x86/thinkpad_acpi.c > > @@ -4116,9 +4116,11 @@ static void hotkey_resume(void) > > { > > tpacpi_disable_brightness_delay(); > > > > + mutex_lock(&hotkey_mutex) > > if (hotkey_status_set(true) < 0 || > > hotkey_mask_set(hotkey_acpi_mask) < 0) > > pr_err("error while attempting to reset the event firmware interface\n"); > > + mutex_unlock(&hotkey_mutex); > > > > tpacpi_send_radiosw_update(); > > tpacpi_input_send_tabletsw(); > > -- > > 2.40.1 > >
Hi Dennis, On 2023-09-14 02:36:44+0200, Dennis Bonke (admin) wrote: > On Thu, 2023-09-14 at 01:33 +0200, Thomas Weißschuh wrote: > > thanks for the fix! > Hello Thomas, > > Thank you for the quick review! The least I can do if somebody else has to clean up my mess :-) > I apologize for the messy V2 that I seem to have posted. > It's my first time working with a mailing list and it seems that something went wrong. No need to apologize. Some more notes: You should also CC LKML (linux-kernel@vger.kernel.org) and the subsystems maintainers as per MAINTAINERS / get_maintainers.pl on all your patches. And now that I have worked with you on a patch also CC me on new revisions. I can also recommend the usage of the "b4"[0] tool to prepare your patches. It takes care of some of the chores. [0] https://b4.docs.kernel.org/en/latest/ Some more comments below. > > > > On 2023-09-14 01:18:29+0200, admin@dennisbonke.com wrote: > > > From: Dennis Bonke <admin@dennisbonke.com> > > > > > > hotkey_status_{set,get} expect the hotkey_mutex to be held. > > > It seems like it was missed here and that gives warnings while resuming. > > > > Which kind of warnings? > > > > If it's from lockdep then it's triggered by hotkey_mask_set() and the > > commit message is a bit off. > It is indeed from lockdep. I've changed the commit message to reflect your comment. Thanks! > > > > Also then the patch needs: > > > > Fixes: 38831eaf7d4c ("platform/x86: thinkpad_acpi: use lockdep annotations") > > Cc: stable@vger.kernel.org > > > > With those: > > > > Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> > About those tags, do I add them to the patch? Just double checking > before I accidentally CC the stable list with an incorrect patch. Yes, please add them to the patch. The CC stable will only have any effect after your patch is in Linus' tree at which point multiple people will have looked at it. If an incorrect patch makes it that far it's not your fault. > > > > > > Signed-off-by: Dennis Bonke <admin@dennisbonke.com> > > > --- > > > drivers/platform/x86/thinkpad_acpi.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > > > index d70c89d32534..de5859a5eb0d 100644 > > > --- a/drivers/platform/x86/thinkpad_acpi.c > > > +++ b/drivers/platform/x86/thinkpad_acpi.c > > > @@ -4116,9 +4116,11 @@ static void hotkey_resume(void) > > > { > > > tpacpi_disable_brightness_delay(); > > > > > > + mutex_lock(&hotkey_mutex) > > > if (hotkey_status_set(true) < 0 || > > > hotkey_mask_set(hotkey_acpi_mask) < 0) > > > pr_err("error while attempting to reset the event firmware interface\n"); > > > + mutex_unlock(&hotkey_mutex); > > > > > > tpacpi_send_radiosw_update(); > > > tpacpi_input_send_tabletsw(); > > > -- > > > 2.40.1 > > > > Thomas
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index d70c89d32534..de5859a5eb0d 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -4116,9 +4116,11 @@ static void hotkey_resume(void) { tpacpi_disable_brightness_delay(); + mutex_lock(&hotkey_mutex) if (hotkey_status_set(true) < 0 || hotkey_mask_set(hotkey_acpi_mask) < 0) pr_err("error while attempting to reset the event firmware interface\n"); + mutex_unlock(&hotkey_mutex); tpacpi_send_radiosw_update(); tpacpi_input_send_tabletsw();