[SRU] [Xenial] [PATCH 0/1] Fix keyboard led sysfs cannot control its brightness

[Stefan Bader]

On 31.05.2017 06:43, Kai-Heng Feng wrote:
> On Tue, May 30, 2017 at 8:33 PM, Stefan Bader
> <stefan.bader at canonical.com> wrote:
>> On 25.05.2017 05:29, Kai-Heng Feng wrote:
>>> BugLink: https://bugs.launchpad.net/bugs/1693126
>>>
>>> Impact:
>>> On some new dell laptops, the keyboard led backlight sysfs cannot control its brightness:
>>>  dell_laptop: Setting old previous keyboard state failed
>>>  leds dell::kbd_backlight: Setting an LED's brightness failed (-6)
>>>
>>> Fix:
>>> Commit 9216e0dcb5533a999d544d0af8661118e0588e1d can fix the issue:
>>>  This patch adds support for retrieving current timeout AC values and also
>>>  updating them. Current timeout value in sysfs is displayed based on current
>>>  AC status, like current display brightness value.
>>>
>>> The patch uses dell_smbios_find_token() which is a refactored version of
>>> find_token_location(). I use the latter one to avoid unnecessary patch series.
>>>
>>> Test:
>>> Keyboad backlight now can be correctly controlled by sysfs
>>> "dell:kbd_backlight/brightness" on Dell Precision 3520.
>>>
>>> Pali Rohár (1):
>>>   platform/x86: dell-laptop: Add keyboard backlight timeout AC settings
>>>
>>>  drivers/platform/x86/dell-laptop.c | 59 ++++++++++++++++++++++++++++++++++----
>>>  1 file changed, 53 insertions(+), 6 deletions(-)
>>>
>>
>> This change is relate to "Support new chassis type for dell-laptop", so it would
>> be better to group both patches together. It is also a bit weird that this one
>> is only asked for Xenial and the other one for Yakkety and Zesty, too.
> 
> dell-laptop on Yakkey and Zesty had some function name changing and
> refactoring, but not on Xenial. So unfortunately the Xenial one needs
> some extra care.

Right, but still the two patches (or three) are a kind of unit. Sending them
individually can cause problems. Just as an example, I think I saw the second
part of the fix (submitted for Y/Z) getting two acks (all 3 submissions are not
grouped together because other mails were sent in between). So it could happen
that for some reason only that part makes it into the next cycle. If that
partial fix were released the bots would claim the whole bug being fixed but of
course that would not be the truth.

So I would suggest to have (this was asked for by Colin on a different part of
the fix) the SRU template as part of the bug reports description (Impact, fix,
and testcase). That part is about risk: like how bad is the impact of the
current situation, how intrusive is the fix, could this break other machines, is
this easy to verify?

The additional info in the cover email is somewhat similar but more for the
reviewers, explaining the patches. Like why are there multiple variants of the
same. Or roughly what needed to be done for the backport. Anything that helps
somebody who has not worked on the bug before to understand how this resulted in
those patches submitted.

So what I would have done is to take the 3 patch files and name them in a way
they can co-exist. Like (but use whatever suits you):

0001-<foo>.patch
0002a-<bar>.patch
0002b-<bar>.patch

then add a 0000-cover for the introductional email. And then modify the
"Subject" lines in those to end up with a submission:

[SRU X/Y/Z 0/2] Fix dell ... (the cover email)
+- [PATCH 1/2 X/Y/Z] <patch 1>
+- [PATCH 2/2 X] <patch 2>
+- [PATCH 2/2 Y/Z] <patch 2>

That way it is very clear that you need both patches on X/Y/Z and that the
second part is different for X and Y/Z. And using git send-email automatically
makes the patches follow-ups on the cover email and any mail client that does
threaded views groups things together. Actually you also win because you only
need one cover email. :)

-Stefan


> 
>>
>> -Stefan
>>
>>
>> --
>> kernel-team mailing list
>> kernel-team at lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20170531/aef2374f/attachment.sig>