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] [day] [month] [year] [list]

Message-ID: <82528887-a6e9-f7cc-26f3-6a0932d967c9@linux.ibm.com>
Date:   Fri, 1 Apr 2022 09:17:24 -0500
From:   Eddie James <eajames@...ux.ibm.com>
To:     Patrick Williams <patrick@...cx.xyz>
Cc:     pavel@....cz, openbmc@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
        joel@....id.au
Subject: Re: [PATCH] leds: pca955x: Add HW blink support


On 3/31/22 10:39, Patrick Williams wrote:
> 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.


Yep. I see your point, it could be problematic.


>
> 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?


OK, I like this idea, I'll go ahead and implement it. Thanks for the 
suggestion!


Eddie


>
> 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
>