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

[Kai-Heng Feng]

On Wed, May 31, 2017 at 1:46 PM, Stefan Bader
<stefan.bader at canonical.com> wrote:
> 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?

You are right, I didn't quite grasp the different between the
Launchpad bug description and the cover letter for Kernel Team Mailing
List.
Now I know the difference and what they are for.

>
> 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. :)

This is a much better logical grouping than the format I used. I'll
definitely use this in the future.

Thanks!

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