linux-kernel - Re: [RFC PATCH] auxdisplay: line-display: support attribute attachment to existing device

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: <CAHp75VdF=_LQbHJozUGExCmDd4UW4oJ0-deT=aEdnQjCOotsVA@mail.gmail.com>
Date: Sat, 30 Aug 2025 12:15:23 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Jean-François Lessard <jefflessard3@...il.com>
Cc: Andy Shevchenko <andy@...nel.org>, Geert Uytterhoeven <geert@...ux-m68k.org>, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] auxdisplay: line-display: support attribute
 attachment to existing device

On Fri, Aug 29, 2025 at 4:03 AM Jean-François Lessard
<jefflessard3@...il.com> wrote:
>
> Modernize the line-display core library:
> - Add the ability to attach line-display's sysfs attribute group directly
>   to an existing parent device (not only via a child device) using
>   device_add_groups(), allowing coherent integration with subsystems like
>   the LED class.
> - Implement a global linedisp_attachment mapping for unified and race-free
>   attribute access, context lookup, and cleanup, shared between
>   traditional register/unregister and new attach/detach paths.
> - Modify sysfs attribute accessors to retrieve context using a consistent
>   to_linedisp() mechanism.
> - Add a new num_chars read-only attribute reporting the number of display
>   digits to allow static non-scrolling message from userspace.
> - Ensures thread safety and proper list removal for all attachments
>   operations.

Thanks for working on this, can you split it into 5 patches?
More comments below.

> Backwards compatibility with existing users is maintained, while enabling
> uniform n-segment sysfs API and richer information for integrated drivers.

...

> -#include <linux/container_of.h>
> +#include <linux/list.h>

Keep it ordered.

...

> +static const struct device_type linedisp_type;

Why? I don't see the use of this in the nearest below blocks of code.
Can you move it closer to the first user so we can see the point?

...

> +       scoped_guard(spinlock, &linedisp_attachments_lock) {
> +               list_add(&attachment->list, &linedisp_attachments);
> +       }

{} are not needed (same rule as with for-loop of if condition with one liner).

> +       return 0;

But in this case you can simply use guard()().

...

> +static struct linedisp *delete_attachment(struct device *dev, bool owns_device)
> +{
> +       struct linedisp_attachment *attachment, *tmp;
> +       struct linedisp *linedisp = NULL;
> +
> +       scoped_guard(spinlock, &linedisp_attachments_lock) {

Use guard()() and drop NULL assignment.

> +               list_for_each_entry_safe(attachment, tmp, &linedisp_attachments, list) {
> +                       if (attachment->device == dev &&
> +                           attachment->owns_device == owns_device) {
> +                               linedisp = attachment->linedisp;
> +                               list_del(&attachment->list);
> +                               kfree(attachment);
> +                               break;
> +                       }
> +               }
> +       }
> +
> +       return linedisp;
> +}

...

> +static struct linedisp *to_linedisp(struct device *dev)

As per above.

...

>  static struct attribute *linedisp_attrs[] = {
>         &dev_attr_message.attr,
>         &dev_attr_scroll_step_ms.attr,
> +       &dev_attr_num_chars.attr,

This needs a Documentation update for a new ABI.

>         &dev_attr_map_seg7.attr,
>         &dev_attr_map_seg14.attr,
>         NULL
> };

...

> +int linedisp_attach(struct linedisp *linedisp, struct device *dev,
> +                   unsigned int num_chars, const struct linedisp_ops *ops)
> +{
> +       int err;
> +
> +       memset(linedisp, 0, sizeof(*linedisp));
> +       linedisp->ops = ops;
> +       linedisp->num_chars = num_chars;
> +       linedisp->scroll_rate = DEFAULT_SCROLL_RATE;

> +       err = -ENOMEM;
> +       linedisp->buf = kzalloc(linedisp->num_chars, GFP_KERNEL);
> +       if (!linedisp->buf)
> +               return err;

You can simply return -ENOMEM without initial assignment.

> +       /* initialise a character mapping, if required */
> +       err = linedisp_init_map(linedisp);
> +       if (err)
> +               goto out_free_buf;

The __free() can be used instead, but for uniform handling it's
probably not needed here.

> +       /* initialise a timer for scrolling the message */
> +       timer_setup(&linedisp->timer, linedisp_scroll, 0);
> +
> +       err = create_attachment(dev, linedisp, false);
> +       if (err)
> +               goto out_del_timer;
> +
> +       /* add attribute groups to target device */
> +       err = device_add_groups(dev, linedisp_groups);
> +       if (err)
> +               goto out_del_attach;
> +
> +       /* display a default message */
> +       err = linedisp_display(linedisp, LINEDISP_INIT_TEXT, -1);
> +       if (err)
> +               goto out_rem_groups;
> +
> +       return 0;
> +
> +out_rem_groups:
> +       device_remove_groups(dev, linedisp_groups);
> +out_del_attach:
> +       delete_attachment(dev, false);
> +out_del_timer:
> +       timer_delete_sync(&linedisp->timer);
> +out_free_buf:
> +       kfree(linedisp->buf);
> +       return err;
> +}

-- 
With Best Regards,
Andy Shevchenko