Brightness notification does not work until the user writes to
hotkey_mask attribute. That's because the polling thread will only run
if hotkey_user_mask is set and someone is reading the input device or if
hotkey_driver_mask is set. In this second case, this condition is not
tested after the mask is changed, because the brightness and volume
drivers are started after the hotkey drivers.
This fix test for the polling condition that ends up starting the
polling thread after hotkey_driver_mask is set in brightness and volume
init functions.
Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]>
---
drivers/platform/x86/thinkpad_acpi.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index e67e4fe..d69749d 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -6272,6 +6272,11 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
tpacpi_hotkey_driver_mask_set(hotkey_driver_mask
| TP_ACPI_HKEY_BRGHTUP_MASK
| TP_ACPI_HKEY_BRGHTDWN_MASK);;
+
+#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
+ hotkey_poll_setup(true);
+#endif
+
return 0;
}
@@ -6903,6 +6908,10 @@ static int __init volume_init(struct ibm_init_struct *iibm)
| TP_ACPI_HKEY_VOLDWN_MASK
| TP_ACPI_HKEY_MUTE_MASK);
+#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
+ hotkey_poll_setup(true);
+#endif
+
return 0;
}
--
1.6.6.1
On Sun, 7 Feb 2010 16:32:07 -0200
Thadeu Lima de Souza Cascardo <[email protected]> wrote:
> Brightness notification does not work until the user writes to
> hotkey_mask attribute. That's because the polling thread will only run
> if hotkey_user_mask is set and someone is reading the input device or if
> hotkey_driver_mask is set. In this second case, this condition is not
> tested after the mask is changed, because the brightness and volume
> drivers are started after the hotkey drivers.
>
> This fix test for the polling condition that ends up starting the
> polling thread after hotkey_driver_mask is set in brightness and volume
> init functions.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 9 +++++++++
> 1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index e67e4fe..d69749d 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6272,6 +6272,11 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
> tpacpi_hotkey_driver_mask_set(hotkey_driver_mask
> | TP_ACPI_HKEY_BRGHTUP_MASK
> | TP_ACPI_HKEY_BRGHTDWN_MASK);;
> +
> +#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> + hotkey_poll_setup(true);
> +#endif
> +
> return 0;
> }
>
> @@ -6903,6 +6908,10 @@ static int __init volume_init(struct ibm_init_struct *iibm)
> | TP_ACPI_HKEY_VOLDWN_MASK
> | TP_ACPI_HKEY_MUTE_MASK);
>
> +#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> + hotkey_poll_setup(true);
> +#endif
> +
> return 0;
> }
Something like this is needed, methinks:
- Avoid ifdefs
- Make sure that hotkey_mutex is held when calling hotkey_poll_setup:
use hotkey_poll_setup_safe. Doesn't matter much in __init code.
--- a/drivers/platform/x86/thinkpad_acpi.c~thinkpad-acpi-setup-hotkey-polling-after-changing-hotkey_driver_mask-fix
+++ a/drivers/platform/x86/thinkpad_acpi.c
@@ -2597,6 +2597,10 @@ static void hotkey_poll_set_freq(unsigne
#else /* CONFIG_THINKPAD_ACPI_HOTKEY_POLL */
+static void hotkey_poll_setup(bool __unused)
+{
+}
+
static void hotkey_poll_setup_safe(bool __unused)
{
}
@@ -2694,9 +2698,7 @@ static ssize_t hotkey_mask_store(struct
res = hotkey_user_mask_set(t);
-#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
hotkey_poll_setup(true);
-#endif
mutex_unlock(&hotkey_mutex);
@@ -6273,9 +6275,7 @@ static int __init brightness_init(struct
| TP_ACPI_HKEY_BRGHTUP_MASK
| TP_ACPI_HKEY_BRGHTDWN_MASK);;
-#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
- hotkey_poll_setup(true);
-#endif
+ hotkey_poll_setup_safe(true);
return 0;
}
_
On Mon, Feb 08, 2010 at 03:50:59PM -0800, Andrew Morton wrote:
> On Sun, 7 Feb 2010 16:32:07 -0200
> Thadeu Lima de Souza Cascardo <[email protected]> wrote:
>
> > Brightness notification does not work until the user writes to
> > hotkey_mask attribute. That's because the polling thread will only run
> > if hotkey_user_mask is set and someone is reading the input device or if
> > hotkey_driver_mask is set. In this second case, this condition is not
> > tested after the mask is changed, because the brightness and volume
> > drivers are started after the hotkey drivers.
> >
> > This fix test for the polling condition that ends up starting the
> > polling thread after hotkey_driver_mask is set in brightness and volume
> > init functions.
> >
> > Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]>
> > ---
> > drivers/platform/x86/thinkpad_acpi.c | 9 +++++++++
> > 1 files changed, 9 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> > index e67e4fe..d69749d 100644
> > --- a/drivers/platform/x86/thinkpad_acpi.c
> > +++ b/drivers/platform/x86/thinkpad_acpi.c
> > @@ -6272,6 +6272,11 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
> > tpacpi_hotkey_driver_mask_set(hotkey_driver_mask
> > | TP_ACPI_HKEY_BRGHTUP_MASK
> > | TP_ACPI_HKEY_BRGHTDWN_MASK);;
> > +
> > +#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> > + hotkey_poll_setup(true);
> > +#endif
> > +
> > return 0;
> > }
> >
> > @@ -6903,6 +6908,10 @@ static int __init volume_init(struct ibm_init_struct *iibm)
> > | TP_ACPI_HKEY_VOLDWN_MASK
> > | TP_ACPI_HKEY_MUTE_MASK);
> >
> > +#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> > + hotkey_poll_setup(true);
> > +#endif
> > +
> > return 0;
> > }
>
> Something like this is needed, methinks:
>
> - Avoid ifdefs
>
> - Make sure that hotkey_mutex is held when calling hotkey_poll_setup:
> use hotkey_poll_setup_safe. Doesn't matter much in __init code.
>
> --- a/drivers/platform/x86/thinkpad_acpi.c~thinkpad-acpi-setup-hotkey-polling-after-changing-hotkey_driver_mask-fix
> +++ a/drivers/platform/x86/thinkpad_acpi.c
> @@ -2597,6 +2597,10 @@ static void hotkey_poll_set_freq(unsigne
>
> #else /* CONFIG_THINKPAD_ACPI_HOTKEY_POLL */
>
> +static void hotkey_poll_setup(bool __unused)
> +{
> +}
> +
> static void hotkey_poll_setup_safe(bool __unused)
> {
> }
> @@ -2694,9 +2698,7 @@ static ssize_t hotkey_mask_store(struct
>
> res = hotkey_user_mask_set(t);
>
> -#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> hotkey_poll_setup(true);
> -#endif
>
> mutex_unlock(&hotkey_mutex);
>
> @@ -6273,9 +6275,7 @@ static int __init brightness_init(struct
> | TP_ACPI_HKEY_BRGHTUP_MASK
> | TP_ACPI_HKEY_BRGHTDWN_MASK);;
>
> -#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> - hotkey_poll_setup(true);
> -#endif
> + hotkey_poll_setup_safe(true);
>
> return 0;
> }
> _
>
How about the added call to volume_init?
Regards,
Cascardo.
On Mon, 8 Feb 2010 22:08:10 -0200
Thadeu Lima de Souza Cascardo <[email protected]> wrote:
>
> ...
>
> > --- a/drivers/platform/x86/thinkpad_acpi.c~thinkpad-acpi-setup-hotkey-polling-after-changing-hotkey_driver_mask-fix
> > +++ a/drivers/platform/x86/thinkpad_acpi.c
> > @@ -2597,6 +2597,10 @@ static void hotkey_poll_set_freq(unsigne
> >
> > #else /* CONFIG_THINKPAD_ACPI_HOTKEY_POLL */
> >
> > +static void hotkey_poll_setup(bool __unused)
> > +{
> > +}
> > +
> > static void hotkey_poll_setup_safe(bool __unused)
> > {
> > }
> > @@ -2694,9 +2698,7 @@ static ssize_t hotkey_mask_store(struct
> >
> > res = hotkey_user_mask_set(t);
> >
> > -#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> > hotkey_poll_setup(true);
> > -#endif
> >
> > mutex_unlock(&hotkey_mutex);
> >
> > @@ -6273,9 +6275,7 @@ static int __init brightness_init(struct
> > | TP_ACPI_HKEY_BRGHTUP_MASK
> > | TP_ACPI_HKEY_BRGHTDWN_MASK);;
> >
> > -#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> > - hotkey_poll_setup(true);
> > -#endif
> > + hotkey_poll_setup_safe(true);
> >
> > return 0;
> > }
> > _
> >
>
> How about the added call to volume_init?
>
I don't know what you mean.
On Mon, Feb 08, 2010 at 04:21:53PM -0800, Andrew Morton wrote:
> On Mon, 8 Feb 2010 22:08:10 -0200
> Thadeu Lima de Souza Cascardo <[email protected]> wrote:
>
> >
> > ...
> >
> > > --- a/drivers/platform/x86/thinkpad_acpi.c~thinkpad-acpi-setup-hotkey-polling-after-changing-hotkey_driver_mask-fix
> > > +++ a/drivers/platform/x86/thinkpad_acpi.c
> > > @@ -2597,6 +2597,10 @@ static void hotkey_poll_set_freq(unsigne
> > >
> > > #else /* CONFIG_THINKPAD_ACPI_HOTKEY_POLL */
> > >
> > > +static void hotkey_poll_setup(bool __unused)
> > > +{
> > > +}
> > > +
> > > static void hotkey_poll_setup_safe(bool __unused)
> > > {
> > > }
> > > @@ -2694,9 +2698,7 @@ static ssize_t hotkey_mask_store(struct
> > >
> > > res = hotkey_user_mask_set(t);
> > >
> > > -#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> > > hotkey_poll_setup(true);
> > > -#endif
> > >
> > > mutex_unlock(&hotkey_mutex);
> > >
> > > @@ -6273,9 +6275,7 @@ static int __init brightness_init(struct
> > > | TP_ACPI_HKEY_BRGHTUP_MASK
> > > | TP_ACPI_HKEY_BRGHTDWN_MASK);;
> > >
> > > -#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> > > - hotkey_poll_setup(true);
> > > -#endif
> > > + hotkey_poll_setup_safe(true);
> > >
> > > return 0;
> > > }
> > > _
> > >
> >
> > How about the added call to volume_init?
> >
>
> I don't know what you mean.
>
You've fixed the hotkey_poll_setup to a hotkey_poll_setup_safe that has
just been added by a patch of mine to the function brightness_init. The
same has not been done to the call that has been added to the function
volume_init.
On Sun, 07 Feb 2010, Thadeu Lima de Souza Cascardo wrote:
> Brightness notification does not work until the user writes to
> hotkey_mask attribute. That's because the polling thread will only run
Hmm, maybe this has something to do with it...
http://bugzilla.kernel.org/show_bug.cgi?id=15118
> if hotkey_user_mask is set and someone is reading the input device or if
> hotkey_driver_mask is set. In this second case, this condition is not
> tested after the mask is changed, because the brightness and volume
> drivers are started after the hotkey drivers.
>
> This fix test for the polling condition that ends up starting the
> polling thread after hotkey_driver_mask is set in brightness and volume
> init functions.
tpacpi_hotkey_driver_mask_set() has to do whatever is required to get the
events to work.
I am looking into this problem now.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Mon, 08 Feb 2010, Thadeu Lima de Souza Cascardo wrote:
> You've fixed the hotkey_poll_setup to a hotkey_poll_setup_safe that has
> just been added by a patch of mine to the function brightness_init. The
> same has not been done to the call that has been added to the function
> volume_init.
Andrew said it didn't matter for the __init functions in thinkpad-acpi, and
he is correct.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
Thadeu, thanks for the detailed analysis of the problem.
Please test the commit below, it should fix things cleanly. I am quite
tired right now, so I might have missed some details, but it survived a fast
testing and compiled without warnings both with and without poll support
compiled in.
Tested in a T43 by crippling the input device open handle to not start the
poller, and by crippling hotkey_mask support to force the driver to think it
needs to default to poll mode. Both volume and brightness reporting worked
fine in the test.
If it does fixes the issues you observed, I will add the tested-by, and send
it to Len.
commit 8e05920a6cb236b21f31391b4479ec29ce65ccdf
Author: Henrique de Moraes Holschuh <[email protected]>
Date: Mon Feb 8 22:40:28 2010 -0200
thinkpad-acpi: make driver events work in NVRAM poll mode
Thadeu Lima de Souza Cascardo reports this:
Brightness notification does not work until the user writes to
hotkey_mask attribute. That's because the polling thread will only run
if hotkey_user_mask is set and someone is reading the input device or
if hotkey_driver_mask is set. In this second case, this condition is
not tested after the mask is changed, because the brightness and
volume drivers are started after the hotkey drivers.
Fix tpacpi_hotkey_driver_mask_set() to call hotkey_poll_setup(), so
that the poller kthread will be started when needed.
Reported-by: Thadeu Lima de Souza Cascardo <[email protected]>
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Andrew Morton <[email protected]>
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index d12b61b..09c1fe6 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -2086,6 +2086,7 @@ static struct attribute_set *hotkey_dev_attributes;
static void tpacpi_driver_event(const unsigned int hkey_event);
static void hotkey_driver_event(const unsigned int scancode);
+static void hotkey_poll_setup(const bool may_warn);
/* HKEY.MHKG() return bits */
#define TP_HOTKEY_TABLET_MASK (1 << 3)
@@ -2268,6 +2269,8 @@ static int tpacpi_hotkey_driver_mask_set(const u32 mask)
rc = hotkey_mask_set((hotkey_acpi_mask | hotkey_driver_mask) &
~hotkey_source_mask);
+ hotkey_poll_setup(true);
+
mutex_unlock(&hotkey_mutex);
return rc;
@@ -2552,7 +2555,7 @@ static void hotkey_poll_stop_sync(void)
}
/* call with hotkey_mutex held */
-static void hotkey_poll_setup(bool may_warn)
+static void hotkey_poll_setup(const bool may_warn)
{
const u32 poll_driver_mask = hotkey_driver_mask & hotkey_source_mask;
const u32 poll_user_mask = hotkey_user_mask & hotkey_source_mask;
@@ -2583,7 +2586,7 @@ static void hotkey_poll_setup(bool may_warn)
}
}
-static void hotkey_poll_setup_safe(bool may_warn)
+static void hotkey_poll_setup_safe(const bool may_warn)
{
mutex_lock(&hotkey_mutex);
hotkey_poll_setup(may_warn);
@@ -2601,7 +2604,11 @@ static void hotkey_poll_set_freq(unsigned int freq)
#else /* CONFIG_THINKPAD_ACPI_HOTKEY_POLL */
-static void hotkey_poll_setup_safe(bool __unused)
+static void hotkey_poll_setup(const bool __unused)
+{
+}
+
+static void hotkey_poll_setup_safe(const bool __unused)
{
}
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Mon, Feb 08, 2010 at 11:37:36PM -0200, Henrique de Moraes Holschuh wrote:
> Thadeu, thanks for the detailed analysis of the problem.
>
> Please test the commit below, it should fix things cleanly. I am quite
> tired right now, so I might have missed some details, but it survived a fast
> testing and compiled without warnings both with and without poll support
> compiled in.
>
> Tested in a T43 by crippling the input device open handle to not start the
> poller, and by crippling hotkey_mask support to force the driver to think it
> needs to default to poll mode. Both volume and brightness reporting worked
> fine in the test.
>
> If it does fixes the issues you observed, I will add the tested-by, and send
> it to Len.
>
Sorry for the delay. Takes a while to build a kernel in my old notebook
with a not-so-large config.
It works nice for me. This was a fix that has crossed my mind, but since
other init was doing the other way (hotkey_init), I did prefer that too.
Anyway, your way is cleaner and the right way. :-)
Tested-by: Thadeu Lima de Souza Cascardo <[email protected]>
Thanks and my best regards,
Cascardo.
>
> commit 8e05920a6cb236b21f31391b4479ec29ce65ccdf
> Author: Henrique de Moraes Holschuh <[email protected]>
> Date: Mon Feb 8 22:40:28 2010 -0200
>
> thinkpad-acpi: make driver events work in NVRAM poll mode
>
> Thadeu Lima de Souza Cascardo reports this:
>
> Brightness notification does not work until the user writes to
> hotkey_mask attribute. That's because the polling thread will only run
> if hotkey_user_mask is set and someone is reading the input device or
> if hotkey_driver_mask is set. In this second case, this condition is
> not tested after the mask is changed, because the brightness and
> volume drivers are started after the hotkey drivers.
>
> Fix tpacpi_hotkey_driver_mask_set() to call hotkey_poll_setup(), so
> that the poller kthread will be started when needed.
>
> Reported-by: Thadeu Lima de Souza Cascardo <[email protected]>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Andrew Morton <[email protected]>
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index d12b61b..09c1fe6 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -2086,6 +2086,7 @@ static struct attribute_set *hotkey_dev_attributes;
>
> static void tpacpi_driver_event(const unsigned int hkey_event);
> static void hotkey_driver_event(const unsigned int scancode);
> +static void hotkey_poll_setup(const bool may_warn);
>
> /* HKEY.MHKG() return bits */
> #define TP_HOTKEY_TABLET_MASK (1 << 3)
> @@ -2268,6 +2269,8 @@ static int tpacpi_hotkey_driver_mask_set(const u32 mask)
>
> rc = hotkey_mask_set((hotkey_acpi_mask | hotkey_driver_mask) &
> ~hotkey_source_mask);
> + hotkey_poll_setup(true);
> +
> mutex_unlock(&hotkey_mutex);
>
> return rc;
> @@ -2552,7 +2555,7 @@ static void hotkey_poll_stop_sync(void)
> }
>
> /* call with hotkey_mutex held */
> -static void hotkey_poll_setup(bool may_warn)
> +static void hotkey_poll_setup(const bool may_warn)
> {
> const u32 poll_driver_mask = hotkey_driver_mask & hotkey_source_mask;
> const u32 poll_user_mask = hotkey_user_mask & hotkey_source_mask;
> @@ -2583,7 +2586,7 @@ static void hotkey_poll_setup(bool may_warn)
> }
> }
>
> -static void hotkey_poll_setup_safe(bool may_warn)
> +static void hotkey_poll_setup_safe(const bool may_warn)
> {
> mutex_lock(&hotkey_mutex);
> hotkey_poll_setup(may_warn);
> @@ -2601,7 +2604,11 @@ static void hotkey_poll_set_freq(unsigned int freq)
>
> #else /* CONFIG_THINKPAD_ACPI_HOTKEY_POLL */
>
> -static void hotkey_poll_setup_safe(bool __unused)
> +static void hotkey_poll_setup(const bool __unused)
> +{
> +}
> +
> +static void hotkey_poll_setup_safe(const bool __unused)
> {
> }
>
>
> --
> "One disk to rule them all, One disk to find them. One disk to bring
> them all and in the darkness grind them. In the Land of Redmond
> where the shadows lie." -- The Silicon Valley Tarot
> Henrique Holschuh
On Tue, 09 Feb 2010, Thadeu Lima de Souza Cascardo wrote:
> Sorry for the delay. Takes a while to build a kernel in my old notebook
> with a not-so-large config.
Heh, I know how you feel. Maybe you can use distcc to offload the worst of
the work to a more powerful box?
> It works nice for me. This was a fix that has crossed my mind, but since
> other init was doing the other way (hotkey_init), I did prefer that too.
> Anyway, your way is cleaner and the right way. :-)
hotkey_* kinda "owns" the event interface, so it is allowed some liberties I
try to avoid in the other parts of the driver :)
> Tested-by: Thadeu Lima de Souza Cascardo <[email protected]>
Thank you. I will forward it to Len.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh