dell-laptop and separate AC timeouts on some Dell systems

dell-laptop and separate AC timeouts on some Dell systems

[Mario.Limonciello]

Pali,

For some time there have been folks reporting some issues where keyboard backlight setting is failing on various Dell systems (https://github.com/dell/libsmbios/issues/3).  I've been circling around internally to find out what's going on.
This affects a number of systems from the last year or so.

What is happening is that some platforms support an alternate keyboard timeout while on AC.  The old "timeout" value is treated as just a battery timeout and the separate value is the AC timeout.
Unfortunately if the AC timeout is /not/ set in the timeout setting/update request, this will fail.

As a result I prototyped a few changes for this at the libsmbios branch here: https://github.com/dell/libsmbios/commits/fix-g8-keyboard-backlight
It's not yet requested for merging because I still don't know why the request to supported features returns "Always On" still (that shouldn't be supported).

As far as I can tell this doesn't really map well to how the keyboard backlight driver in the kernel works today, so I wanted to raise this to you to discuss what the best way to handle it is.
One of these systems can be detected by the presence of the token 0x451.  When that is found, the additional timeout unit and value should be sent with the requests.

Can you let me know your thoughts?

Thanks,

Re: dell-laptop and separate AC timeouts on some Dell systems

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2184 bytes --]

Hi!

On Friday 07 April 2017 21:33:53 Mario.Limonciello@dell.com wrote:
> Pali,
> 
> For some time there have been folks reporting some issues where
> keyboard backlight setting is failing on various Dell systems
> (https://github.com/dell/libsmbios/issues/3).  I've been circling
> around internally to find out what's going on. This affects a number
> of systems from the last year or so.

Great!

> What is happening is that some platforms support an alternate
> keyboard timeout while on AC.  The old "timeout" value is treated as
> just a battery timeout and the separate value is the AC timeout.
> Unfortunately if the AC timeout is /not/ set in the timeout
> setting/update request, this will fail.

So in case notebook is running on AC and we want to change timeout for 
keyboard backlight, we need to tell smbios two timeouts, one for battery 
and one for AC?

> As a result I prototyped a few changes for this at the libsmbios
> branch here:
> https://github.com/dell/libsmbios/commits/fix-g8-keyboard-backlight

Can you update also documentation which is at the end of file? It would 
help for kernel implementation.

> It's not yet requested for merging because I still don't know why
> the request to supported features returns "Always On" still (that
> shouldn't be supported).

It looks like that "Always On" mode is reported by smbios by supported, 
but setting it will cause failure. Any idea where is a problem? Probably 
Dell firmware bug? Or needs to be that mode handled specially?

> As far as I can tell this doesn't really map well to how the keyboard
> backlight driver in the kernel works today, so I wanted to raise
> this to you to discuss what the best way to handle it is. One of
> these systems can be detected by the presence of the token 0x451. 
> When that is found, the additional timeout unit and value should be
> sent with the requests.
> 
> Can you let me know your thoughts?

I think that extending struct kbd_state should work.

There are already functions dell_send_intensity() and 
dell_get_intensity() which handle state based on battery/AC mode.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: dell-laptop and separate AC timeouts on some Dell systems

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Friday, April 7, 2017 4:55 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: platform-driver-x86@vger.kernel.org
> Subject: Re: dell-laptop and separate AC timeouts on some Dell systems
> 
> Hi!
> 
> On Friday 07 April 2017 21:33:53 Mario.Limonciello@dell.com wrote:
> > Pali,
> >
> > For some time there have been folks reporting some issues where
> > keyboard backlight setting is failing on various Dell systems
> > (https://github.com/dell/libsmbios/issues/3).  I've been circling
> > around internally to find out what's going on. This affects a number
> > of systems from the last year or so.
> 
> Great!
> 
> > What is happening is that some platforms support an alternate keyboard
> > timeout while on AC.  The old "timeout" value is treated as just a
> > battery timeout and the separate value is the AC timeout.
> > Unfortunately if the AC timeout is /not/ set in the timeout
> > setting/update request, this will fail.
> 
> So in case notebook is running on AC and we want to change timeout for
> keyboard backlight, we need to tell smbios two timeouts, one for battery and
> one for AC?

Yes, that's correct.  The reason for the failures so far has been that the field for 
AC has not been getting sent.  Since the buffer is zero'ed out before starting
the undefined value "0" for AC timeout is sent to EC and rejected.

> 
> > As a result I prototyped a few changes for this at the libsmbios
> > branch here:
> > https://github.com/dell/libsmbios/commits/fix-g8-keyboard-backlight
> 
> Can you update also documentation which is at the end of file? It would help
> for kernel implementation.
>
OK, just added more on this.
 
> > It's not yet requested for merging because I still don't know why the
> > request to supported features returns "Always On" still (that
> > shouldn't be supported).
> 
> It looks like that "Always On" mode is reported by smbios by supported, but
> setting it will cause failure. Any idea where is a problem? Probably Dell
> firmware bug? Or needs to be that mode handled specially?

It's a behavior outside of spec, but I'm still clarifying if it's intended and spec
wasn't updated.

Continue to ignore this bit if it's set unless I hear otherwise.

> 
> > As far as I can tell this doesn't really map well to how the keyboard
> > backlight driver in the kernel works today, so I wanted to raise this
> > to you to discuss what the best way to handle it is. One of these
> > systems can be detected by the presence of the token 0x451.
> > When that is found, the additional timeout unit and value should be
> > sent with the requests.
> >
> > Can you let me know your thoughts?
> 
> I think that extending struct kbd_state should work.
> 
> There are already functions dell_send_intensity() and
> dell_get_intensity() which handle state based on battery/AC mode.
The difference is that intensity it's obvious that it changes from 50 to
100 percent.  With different timeouts, shouldn't this be more complex?

Would you just keep two different nodes as accessible to userspace?

Re: dell-laptop and separate AC timeouts on some Dell systems

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 4448 bytes --]

On Saturday 08 April 2017 00:56:24 Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Friday, April 7, 2017 4:55 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: platform-driver-x86@vger.kernel.org
> > Subject: Re: dell-laptop and separate AC timeouts on some Dell
> > systems
> > 
> > Hi!
> > 
> > On Friday 07 April 2017 21:33:53 Mario.Limonciello@dell.com wrote:
> > > Pali,
> > > 
> > > For some time there have been folks reporting some issues where
> > > keyboard backlight setting is failing on various Dell systems
> > > (https://github.com/dell/libsmbios/issues/3).  I've been circling
> > > around internally to find out what's going on. This affects a
> > > number of systems from the last year or so.
> > 
> > Great!
> > 
> > > What is happening is that some platforms support an alternate
> > > keyboard timeout while on AC.  The old "timeout" value is
> > > treated as just a battery timeout and the separate value is the
> > > AC timeout. Unfortunately if the AC timeout is /not/ set in the
> > > timeout setting/update request, this will fail.
> > 
> > So in case notebook is running on AC and we want to change timeout
> > for keyboard backlight, we need to tell smbios two timeouts, one
> > for battery and one for AC?
> 
> Yes, that's correct.  The reason for the failures so far has been
> that the field for AC has not been getting sent.  Since the buffer
> is zero'ed out before starting the undefined value "0" for AC
> timeout is sent to EC and rejected.
> 
> > > As a result I prototyped a few changes for this at the libsmbios
> > > branch here:
> > > https://github.com/dell/libsmbios/commits/fix-g8-keyboard-backlig
> > > ht
> > 
> > Can you update also documentation which is at the end of file? It
> > would help for kernel implementation.
> 
> OK, just added more on this.
> 
> > > It's not yet requested for merging because I still don't know why
> > > the request to supported features returns "Always On" still
> > > (that shouldn't be supported).
> > 
> > It looks like that "Always On" mode is reported by smbios by
> > supported, but setting it will cause failure. Any idea where is a
> > problem? Probably Dell firmware bug? Or needs to be that mode
> > handled specially?
> 
> It's a behavior outside of spec, but I'm still clarifying if it's
> intended and spec wasn't updated.
> 
> Continue to ignore this bit if it's set unless I hear otherwise.
> 
> > > As far as I can tell this doesn't really map well to how the
> > > keyboard backlight driver in the kernel works today, so I wanted
> > > to raise this to you to discuss what the best way to handle it
> > > is. One of these systems can be detected by the presence of the
> > > token 0x451. When that is found, the additional timeout unit and
> > > value should be sent with the requests.
> > > 
> > > Can you let me know your thoughts?
> > 
> > I think that extending struct kbd_state should work.
> > 
> > There are already functions dell_send_intensity() and
> > dell_get_intensity() which handle state based on battery/AC mode.
> 
> The difference is that intensity it's obvious that it changes from 50
> to 100 percent.  With different timeouts, shouldn't this be more
> complex?

Currently for keyboard backlight settings is used procedure:
get current status --> update --> set new status.

So we just need to have two timeouts in struct kbd_state and get/set 
functions needs to handle it. I do not thing this change would be 
complex, it should be straightforward.

> Would you just keep two different nodes as accessible to userspace?

And this is another question... For a first step we should just fix 
timeout sysfs node to work. And probably the best fix would be that this 
node will manage timeout which belongs to current AC or battery state. 
Like for dell_get_intensity(). When notebook is running on battery we 
will export timeout configured for battery and when on AC we will export 
timeout configured for AC. Changing timeout via sysfs just affect only 
one (current) configuration (ac or battery).

IIRC we do not provide a sysfs node for changing AC display panel 
brightness level when notebook is running on battery and vice versa. So 
do we need it for keyboard backlight timeout at all?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: dell-laptop and separate AC timeouts on some Dell systems

[Mario.Limonciello]

> >
> > The difference is that intensity it's obvious that it changes from 50
> > to 100 percent.  With different timeouts, shouldn't this be more
> > complex?
> 
> Currently for keyboard backlight settings is used procedure:
> get current status --> update --> set new status.
> 
> So we just need to have two timeouts in struct kbd_state and get/set functions
> needs to handle it. I do not thing this change would be complex, it should be
> straightforward.

Yeah I figured this part was straightforward, it was the second part that was
more complex :)

> 
> > Would you just keep two different nodes as accessible to userspace?
> 
> And this is another question... For a first step we should just fix timeout sysfs
> node to work. And probably the best fix would be that this node will manage
> timeout which belongs to current AC or battery state.

So the easiest way to implement this would be to save/restore the values from the
cbArg3 that are not currently saved without to regard to whether the platform
supports a dedicated AC timeout.

If it doesn't support a dedicated AC timeout this will be a no-op, but if it does it
will fix this problem.

> Like for dell_get_intensity(). When notebook is running on battery we will
> export timeout configured for battery and when on AC we will export timeout
> configured for AC. Changing timeout via sysfs just affect only one (current)
> configuration (ac or battery).
> 

So basically the value in sysfs would change when you added/removed the AC
adapter?  That might be a little bit confusing to a user in my opinion.

> IIRC we do not provide a sysfs node for changing AC display panel brightness
> level when notebook is running on battery and vice versa. So do we need it for
> keyboard backlight timeout at all?
> 

How is that handled then?  The node dynamically changes values based on if
a power adapter is plugged in?

Re: dell-laptop and separate AC timeouts on some Dell systems

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2438 bytes --]

On Saturday 08 April 2017 04:09:11 Mario.Limonciello@dell.com wrote:
> > > The difference is that intensity it's obvious that it changes
> > > from 50 to 100 percent.  With different timeouts, shouldn't this
> > > be more complex?
> > 
> > Currently for keyboard backlight settings is used procedure:
> > get current status --> update --> set new status.
> > 
> > So we just need to have two timeouts in struct kbd_state and
> > get/set functions needs to handle it. I do not thing this change
> > would be complex, it should be straightforward.
> 
> Yeah I figured this part was straightforward, it was the second part
> that was more complex :)
> 
> > > Would you just keep two different nodes as accessible to
> > > userspace?
> > 
> > And this is another question... For a first step we should just fix
> > timeout sysfs node to work. And probably the best fix would be
> > that this node will manage timeout which belongs to current AC or
> > battery state.
> 
> So the easiest way to implement this would be to save/restore the
> values from the cbArg3 that are not currently saved without to
> regard to whether the platform supports a dedicated AC timeout.
> 
> If it doesn't support a dedicated AC timeout this will be a no-op,
> but if it does it will fix this problem.

We can detect at module load time if notebook is new or old and based on 
this fact we can fill or not cbArg3.

> > Like for dell_get_intensity(). When notebook is running on battery
> > we will export timeout configured for battery and when on AC we
> > will export timeout configured for AC. Changing timeout via sysfs
> > just affect only one (current) configuration (ac or battery).
> 
> So basically the value in sysfs would change when you added/removed
> the AC adapter?  That might be a little bit confusing to a user in
> my opinion.
> 
> > IIRC we do not provide a sysfs node for changing AC display panel
> > brightness level when notebook is running on battery and vice
> > versa. So do we need it for keyboard backlight timeout at all?
> 
> How is that handled then?  The node dynamically changes values based
> on if a power adapter is plugged in?

Node has read and store functions. Those function check if AC is 
connected and based on this state variant for AC or battery is called.

There is no need to active monitoring plug of power adapter.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]