| 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
| ||
|
Message-ID: <f07313a1-c609-f760-9578-4d6889fd89d9@kaod.org>
Date: Fri, 1 Sep 2017 19:06:30 +0200
From: Cédric Le Goater <clg@...d.org>
To: Andrew Jeffery <andrew@...id.au>, linux-leds@...r.kernel.org
Cc: rpurdie@...ys.net, jacek.anaszewski@...il.com, pavel@....cz,
linux-kernel@...r.kernel.org, bjwyman@...il.com,
openbmc@...ts.ozlabs.org
Subject: Re: [PATCH v2] leds: pca955x: Don't invert requested value in
pca955x_gpio_set_value()
On 09/01/2017 07:38 AM, Andrew Jeffery wrote:
> The PCA9552 lines can be used either for driving LEDs or as GPIOs. The
> manual states that for LEDs, the operation is open-drain:
>
> The LSn LED select registers determine the source of the LED data.
>
> 00 = output is set LOW (LED on)
> 01 = output is set high-impedance (LED off; default)
> 10 = output blinks at PWM0 rate
> 11 = output blinks at PWM1 rate
>
> For GPIOs it suggests a pull-up so that the open-case drives the line
> high:
>
> For use as output, connect external pull-up resistor to the pin
> and size it according to the DC recommended operating
> characteristics. LED output pin is HIGH when the output is
> programmed as high-impedance, and LOW when the output is
> programmed LOW through the ‘LED selector’ register. The output
> can be pulse-width controlled when PWM0 or PWM1 are used.
>
> Now, I have a hardware design that uses the LED controller to control
> LEDs. However, for $reasons, we're using the leds-gpio driver to drive
> the them. The reasons are here are a tangent but lead to the discovery
> of the inversion, which manifested as the LEDs being set to full
> brightness at boot when we expected them to be off.
>
> As we're driving the LEDs through leds-gpio, this means wending our way
> through the gpiochip abstractions. So with that in mind we need to
> describe an active-low GPIO configuration to drive the LEDs as though
> they were GPIOs.
>
> The set() gpiochip callback in leds-pca955x does the following:
>
> ...
> if (val)
> pca955x_led_set(&led->led_cdev, LED_FULL);
> else
> pca955x_led_set(&led->led_cdev, LED_OFF);
> ...
>
> Where LED_FULL = 255. pca955x_led_set() in turn does:
>
> ...
> switch (value) {
> case LED_FULL:
> ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);
> break;
> ...
>
> Where PCA955X_LS_LED_ON is defined as:
>
> #define PCA955X_LS_LED_ON 0x0 /* Output LOW */
>
> So here we have some type confusion: We've crossed domains from GPIO
> behaviour to LED behaviour without accounting for possible inversions
> in the process.
>
> Stepping back to leds-gpio for a moment, during probe() we call
> create_gpio_led(), which eventually executes:
>
> if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP) {
> state = gpiod_get_value_cansleep(led_dat->gpiod);
> if (state < 0)
> return state;
> } else {
> state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
> }
> ...
> ret = gpiod_direction_output(led_dat->gpiod, state);
>
> In the devicetree the GPIO is annotated as active-low, and
> gpiod_get_value_cansleep() handles this for us:
>
> int gpiod_get_value_cansleep(const struct gpio_desc *desc)
> {
> int value;
>
> might_sleep_if(extra_checks);
> VALIDATE_DESC(desc);
> value = _gpiod_get_raw_value(desc);
> if (value < 0)
> return value;
>
> if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
> value = !value;
>
> return value;
> }
>
> _gpiod_get_raw_value() in turn calls through the get() callback for the
> gpiochip implementation, so returning to our get() implementation in
> leds-pca955x we find we extract the raw value from hardware:
>
> static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
> {
> struct pca955x *pca955x = gpiochip_get_data(gc);
> struct pca955x_led *led = &pca955x->leds[offset];
> u8 reg = pca955x_read_input(pca955x->client, led->led_num / 8);
>
> return !!(reg & (1 << (led->led_num % 8)));
> }
>
> This behaviour is not symmetric with that of set(), where the val is
> inverted by the driver.
>
> Closing the loop on the GPIO_ACTIVE_LOW inversions,
> gpiod_direction_output(), like gpiod_get_value_cansleep(), handles it
> for us:
>
> int gpiod_direction_output(struct gpio_desc *desc, int value)
> {
> VALIDATE_DESC(desc);
> if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
> value = !value;
> else
> value = !!value;
> return _gpiod_direction_output_raw(desc, value);
> }
>
> All-in-all, with a value of 'keep' for default-state property in a
> leds-gpio child node, the current state of the hardware will in-fact be
> inverted; precisely the opposite of what was intended.
>
> Rework leds-pca955x so that we avoid the incorrect inversion and clarify
> the semantics with respect to GPIO.
>
> Signed-off-by: Andrew Jeffery <andrew@...id.au>
Reviewed-by: Cédric Le Goater <clg@...d.org>
Thanks for digging into the code, that was a lot of inversions.
C.
> ---
>
> I've rebased on top of "1591caf2d5ea leds: pca955x: check for I2C errors" and
> resolved the conflicts.
>
> drivers/leds/leds-pca955x.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
> index 905729191d3e..6dcd2d7cc6a4 100644
> --- a/drivers/leds/leds-pca955x.c
> +++ b/drivers/leds/leds-pca955x.c
> @@ -61,6 +61,9 @@
> #define PCA955X_LS_BLINK0 0x2 /* Blink at PWM0 rate */
> #define PCA955X_LS_BLINK1 0x3 /* Blink at PWM1 rate */
>
> +#define PCA955X_GPIO_HIGH LED_OFF
> +#define PCA955X_GPIO_LOW LED_FULL
> +
> enum pca955x_type {
> pca9550,
> pca9551,
> @@ -329,9 +332,9 @@ static int pca955x_set_value(struct gpio_chip *gc, unsigned int offset,
> struct pca955x_led *led = &pca955x->leds[offset];
>
> if (val)
> - return pca955x_led_set(&led->led_cdev, LED_FULL);
> - else
> - return pca955x_led_set(&led->led_cdev, LED_OFF);
> + return pca955x_led_set(&led->led_cdev, PCA955X_GPIO_HIGH);
> +
> + return pca955x_led_set(&led->led_cdev, PCA955X_GPIO_LOW);
> }
>
> static void pca955x_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
>
Powered by blists - more mailing lists