linux-kernel - Re: [PATCH] leds: pca955x: Add HW blink support

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]

Message-ID: <YkXLG++LWdQWCxQF@heinlein>
Date:   Thu, 31 Mar 2022 10:39:07 -0500
From:   Patrick Williams <patrick@...cx.xyz>
To:     Eddie James <eajames@...ux.ibm.com>
Cc:     linux-leds@...r.kernel.org, openbmc@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org, pavel@....cz, joel@....id.au
Subject: Re: [PATCH] leds: pca955x: Add HW blink support

On Wed, Mar 30, 2022 at 03:33:18PM -0500, Eddie James wrote:
> Support blinking using the PCA955x chip. Use PWM0 for blinking
> instead of LED_HALF brightness. Since there is only one frequency
> and brightness register for any blinking LED, all blinked LEDs on
> the chip will have the same frequency and brightness.

The current implementation uses the PWM to control the "brightness"
with PWM0 being assigned 50% and PWM1 being configured as a single value
that isn't ON, OFF, or 50%.  I suspect that most users of these 955x
chips care either about brightness or blinking but not both, but it is
possible I am wrong.  It would be nice if we could use PWM1 as another
hardware blink control when it hasn't been used for brightness, but that
would require some additional state tracking I think.

I like that we can now use the hardware to control blink rate, rather
than doing it in software, and, I really like that in theory if N LEDs on
the device are all blinking at the same rate they will actually turn on and
off at the same exact moment because it is done in hardware.  I am really
concerned about this proposed change and the way it will change current
behavior though.

It is not uncommon in a BMC design to use one of these 955x chips to control
8 or 16 different LEDs reflecting the state of the system and at
different blink rates.  An example LED policy might be that you have 1 LED
for "power status" and another LED for "system identify + health status".
When the system is powered off the "power status" LED flashes at a slow rate
and when the system is powered on it goes on solid.  When the system is healthy
the "health status" is on, when it is unhealthy it blinks slowly, and when the
system is "identified" it blinks fast.

My point of the above is that there are certainly system policies where
you'd want to flash two different LEDs at two different rates.  In
today's implementation of this driver those both turn into
software-emulated blinking by the kernel.  With your proposal we lose
this ability and instead whichever LED is configured second will affect
all other blinking LEDs.

It looks like in led-core.c led_blink_setup that if the device
`blink_set` returns an error then software blinking is the fallback.  Is
it possible for us to have this driver keep track of how many LEDs are
in blink state (and which speeds are allocated) and get led-core to
fallback to software blinking if we are unable to satisfy the new blink
rate without affecting an existing LED blink rate?

Looking at the tree it seems bcm6328 does what I am suggesting already
but I don't see any other drivers that obviously do.  The PCA955x is
pretty widely used in BMC implementations:

    $ git grep -l pca955 arch/arm/boot/dts/aspeed* | wc -l
    13

-- 
Patrick Williams

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)