lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 6 Apr 2019 16:15:03 +0200
From:   Jacek Anaszewski <jacek.anaszewski@...il.com>
To:     Pavel Machek <pavel@....cz>
Cc:     Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Guenter Roeck <groeck@...gle.com>,
        Dmitry Torokhov <dtor@...gle.com>,
        Nick Crews <ncrews@...omium.org>,
        Benson Leung <bleung@...omium.org>, linux-leds@...r.kernel.org,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        linux-rtc@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Duncan Laurie <dlaurie@...omium.org>,
        Simon Glass <sjg@...gle.com>
Subject: Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard
 backlight name

Hi Pavel,

On 4/6/19 11:53 AM, Pavel Machek wrote:
> Hi!
> 
>> The patch set introduces also a set of predefined LED_FUNCTION
>> names to be used in DT bindings. This along with the removal
>> of devicename section from LED naming pattern will help to keep
>> LED sysfs interface more uniform and not varying depending on
>> underlaying hardware driving the LEDs.
>>
>> Regarding the problem discussed in this thread - I would not necessarily
>> go for "platform" in place of devicename LED name section in the
>> cros_kbd_led_backlight driver. If we change it (should we at all - it is
>> already in 5.0 AFAICS?), then I would opt for dropping the "chromeos:"
>> part. It believe it will be possible to retrieve this name with
>> get_led_device_info.sh script. It would be good exercise to check
>> it out.
> 
> I am not sure about existing driver. Important thing for me is that
> new drivers use consistent naming.
> 
>> In cases like above:
>>
>>      keyboardist::kbd_backlight
>>      tclnumpad::kbd_backlight
>>
>> we could do with the following:
>>
>>      :kbd-backlight
>>      :numpad-backlight
>>
>> I used hyphens instead of underscores since we will have this convention
>> in the LED_FUNCTION names, which is more common for Device Tree, and
>> some of existing LED triggers.
> 
> Existing userspace already searches for *:kbd_backlight", AFAICT, so
> we probably want to keep the "_".

OK, but it should be an exception but not a rule.
This "kbd-*" naming is used in input and tty subsystems which register
keyboard triggers with this style:

~/kernel$ git grep  ".*[\":]kbd-" -- "*.c"
drivers/input/input-leds.c:     [LED_NUML]      = { "numlock", 
VT_TRIGGER("kbd-numlock") },
drivers/input/input-leds.c:     [LED_CAPSL]     = { "capslock", 
VT_TRIGGER("kbd-capslock") },
drivers/input/input-leds.c:     [LED_SCROLLL]   = { "scrolllock", 
VT_TRIGGER("kbd-scrolllock") },
drivers/input/input-leds.c:     [LED_KANA]      = { "kana", 
VT_TRIGGER("kbd-kanalock") },
drivers/tty/vt/keyboard.c:      KBD_LED_TRIGGER(VC_SCROLLOCK, 
"kbd-scrolllock"),
drivers/tty/vt/keyboard.c:      KBD_LED_TRIGGER(VC_NUMLOCK, 
"kbd-numlock"),
drivers/tty/vt/keyboard.c:      KBD_LED_TRIGGER(VC_CAPSLOCK, 
"kbd-capslock"),
drivers/tty/vt/keyboard.c:      KBD_LED_TRIGGER(VC_KANALOCK, 
"kbd-kanalock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_SHIFTLOCK, 
"kbd-shiftlock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_ALTGRLOCK, 
"kbd-altgrlock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_CTRLLOCK, 
"kbd-ctrllock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_ALTLOCK, 
"kbd-altlock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_SHIFTLLOCK, 
"kbd-shiftllock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_SHIFTRLOCK, 
"kbd-shiftrlock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_CTRLLLOCK, 
"kbd-ctrlllock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_CTRLRLOCK, 
"kbd-ctrlrlock"),

"kbd_" naming is used only in case of backlight LEDs:

~/kernel$ git grep  ".*[\":]kbd_" -- "*.c"
drivers/hid/hid-asus.c: drvdata->kbd_backlight->cdev.name = 
"asus::kbd_backlight";
drivers/hid/hid-google-hammer.c:        kbd_backlight->cdev.name = 
"hammer::kbd_backlight";
drivers/hwmon/applesmc.c:       .name                   = 
"smc::kbd_backlight",
drivers/input/misc/ims-pcu.c:            "pcu%d::kbd_backlight", 
pcu->device_no);
drivers/platform/chrome/cros_kbd_led_backlight.c:       cdev->name = 
"chromeos::kbd_backlight";
drivers/platform/x86/asus-laptop.c:             cdev->name = 
"asus::kbd_backlight";
drivers/platform/x86/asus-wmi.c:                asus->kbd_led.name = 
"asus::kbd_backlight";
drivers/platform/x86/dell-laptop.c:     .name           = 
"dell::kbd_backlight",
drivers/platform/x86/samsung-laptop.c:          samsung->kbd_led.name = 
"samsung::kbd_backlight";
drivers/platform/x86/sony-laptop.c:     kbdbl_ctl->mode_attr.attr.name = 
"kbd_backlight";
drivers/platform/x86/sony-laptop.c: 
kbdbl_ctl->timeout_attr.attr.name = "kbd_backlight_timeout";
drivers/platform/x86/thinkpad_acpi.c:           .name           = 
"tpacpi::kbd_backlight",
drivers/platform/x86/toshiba_acpi.c:            dev->kbd_led.name = 
"toshiba::kbd_backlight";

With LEDs in platform drivers is that problem that we have the name
compiled into the kernel. Maybe to make it more flexible we could
use kernel config to choose between new "kbd-" and legacy "kbd_"
naming.

> I don't care much if we use "platform:" or no prefix at all for
> backlight of internal keyboard, as long as it is consistent across all
> devices.
> 
> We certainly want to use some prefix (probably inputX:) for backlight
> on USB keyboards.

For LEDs exposed through the input-LED bridge my get_led_device_info.sh
script nicely reports:

associated input node: input1

It just does:

readlink input1\:\:numlock/device

which prints: "../../input1 "

And I believe that for backlight LEDs exposed by input
subsystem it should be similarly since the input device
related to USB keyboard is a child of USB device:

/sys/class/leds# readlink input1::numlock
../../devices/pci0000:00/0000:00:14.0/usb2/2-4/2-4:1.0/0003:1C4F:0002.0002/input/input1/input1::numlock

-- 
Best regards,
Jacek Anaszewski

Powered by blists - more mailing lists