linux-kernel - Re: [PATCH v3 3/3] leds: tlc5925: Add support for non blocking operations

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: <CAHp75VftbVOwPFra83T-k5d1qu3NnD_sDHYxiiSEDDrW3NObNQ@mail.gmail.com>
Date:   Thu, 9 Jun 2022 18:43:04 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Jean-Jacques Hiblot <jjhiblot@...phandler.com>
Cc:     Pavel Machek <pavel@....cz>, krzk+dt@...nel.org,
        Rob Herring <robh+dt@...nel.org>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 3/3] leds: tlc5925: Add support for non blocking operations

On Thu, Jun 9, 2022 at 6:29 PM Jean-Jacques Hiblot
<jjhiblot@...phandler.com> wrote:
>
> Settings multiple LEDs in a row can be a slow operation because of the
> time required to acquire the bus and prepare the transfer.
> And, in most cases, it is not required that the operation is synchronous.
>
> Implementing the non-blocking brightness_set() for such cases.
> A work queue is used to perform the actual SPI transfer.
>
> The blocking method is still available in case someone needs to perform
> this operation synchronously (ie by calling led_set_brightness_sync()).

i.e.

> +#define BITS_PER_ATOMIC (sizeof(atomic_t) * 8)

We have BITS_PER_TYPE(). Use it directly in the code, no need for a
whole new macro.

...

> +static int xmit(struct tlc5925_leds_priv *priv)
> +{
> +       int i;
> +
> +       spin_lock(&priv->lock);

This can't be called during IRQ?

> +       for (i = 0; i < priv->max_state / (sizeof(atomic_t) * 8) ; i++)

BITS_PER_TYPE() ?

> +               priv->spi_buffer[i] = atomic_read(&priv->state[i]);
> +       spin_unlock(&priv->lock);
> +
> +       return spi_write(priv->spi, priv->spi_buffer, priv->max_num_leds / 8);
> +}

...

> +static void xmit_work(struct work_struct *ws)
> +{
> +       struct tlc5925_leds_priv *priv =
> +               container_of(ws, struct tlc5925_leds_priv, xmit_work);

One line?

Missed blank line here.

> +       xmit(priv);
> +};

...

>         if (brightness)
> -               priv->state[index / 8] |= (1 << (index % 8));
> +               atomic_or(1 << (index % BITS_PER_ATOMIC),
> +                         &priv->state[index / BITS_PER_ATOMIC]);
>         else
> -               priv->state[index / 8] &= ~(1 << (index % 8));
> -       spin_unlock(&priv->lock);
> +               atomic_and(~(1 << (index % BITS_PER_ATOMIC)),
> +                          &priv->state[index / BITS_PER_ATOMIC]);

The whole bunch looks like reinventing the bitmap / bitops.
Use unsigned long (or DECLARE_BITMAP() if it can be higher than 32)
for state and set_bit() / clear_bit() / assign_bit() that are atomic.

...

> +       if (brightness)
> +               atomic_or(1 << (index % BITS_PER_ATOMIC),
> +                         &priv->state[index / BITS_PER_ATOMIC]);
> +       else
> +               atomic_and(~(1 << (index % BITS_PER_ATOMIC)),
> +                          &priv->state[index / BITS_PER_ATOMIC]);

assign_bit()

...

> +       // Allocate the buffer used to hold the state of each LED
> +       priv->max_state = round_up(max_num_leds, BITS_PER_ATOMIC);
> +       priv->state = devm_kzalloc(dev,
> +                                  priv->max_state / 8,
> +                                  GFP_KERNEL);
>         if (!priv->state)
>                 return -ENOMEM;

devm_bitmap_zalloc() ?

...

> +       // Allocate a second buffer for the communication on the SPI bus
> +       priv->spi_buffer = devm_kzalloc(dev,
> +                                  priv->max_state / 8,
> +                                  GFP_KERNEL);

Not sure I understand the output, but perhaps here the BITS_TO_BYTES()
should be used.

-- 
With Best Regards,
Andy Shevchenko