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: <CAHp75Vc3DHUJwA+S4PGfoZxGtdqVq3GTF6_BEnJFo+=sFMmfDA@mail.gmail.com>
Date: Sat, 30 Aug 2025 17:39:20 +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 Sat, Aug 30, 2025 at 4:55 PM Jean-François Lessard
<jefflessard3@...il.com> wrote:
> Le 30 août 2025 05 h 15 min 23 s HAE, Andy Shevchenko <andy.shevchenko@...il.com> a écrit :
> >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?
>
> My pleasure! Thanks for your feedback.
>
> 5 patches? I can't see how these changes could be split into 5 patches.
> Maybe the commit message is too verbose and needs to be shortened.
>
> If needed to be split into multiple patches, it could be:
> 1. Add attach/detach capability
> 2. Add num_chars sysfs attribute + ABI doc

Yeah, the commit message was a list of 5 things, hence 5 patches. I
haven't read closely to map each listed feature to the possible
change.  The description of to_linedisp() sounds like it can be split
to a separate patch. I really want to see the attachment/detachment
patch separate with the explanation "why?". I am not sure I see the
point.

> I am also thinking to add a 3rd one to pad the message when length is smaller
> than num_chars. Current behavior is to wrap around which seems weird to me.
> E.g. for 4 chars:
>
> Current behavior:
> cat "123" > message
> results in "1231" being displayed
>
> Padded behavior:
> cat "123" > message
> would result in "123<blank>" being displayed
>
> Any thoughts on that?

It's basically the question of circular vs. one time message exposure.
When it's one time, the padding makes sense, otherwise the current
(circular) behaviour seems normal to me.

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

...

> >> +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.
>
> I'll use guard()().
>
> NULL assignment was to prevent invalid pointer in case the device is not found
> in the list. But you are maybe suggesting to drop declaration of linedisp and
> then directly return the linedisp within the loop and to return NULL after the
> loop?

This won't work as you want to clean up the things.

> >> +               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;
> >> +}

The usual approach here is to use list_entry_is_head() after the loop.

  list_for_each_entry() {
    if (...)
      break;
  }

  if (list_entry_is_head(...))
    return NULL;

  linedisp = ...
  ...
  return linedisp;

...

> >> +static struct linedisp *to_linedisp(struct device *dev)
> >
> >As per above.
>
> Same.

Same.

...

> >> +       &dev_attr_num_chars.attr,
> >
> >This needs a Documentation update for a new ABI.
>
> Agreed. I think it's also worth documenting the other ABIs along the way since
> they are not documented yet. Then, what Contact should be documented?
> Is there an auxdisplay mailing list?

Your or no-one's? Is it a mandatory field? In any case the ABI must be
documented, w.o. doing that any good patch is simply NAK
automatically.

-- 
With Best Regards,
Andy Shevchenko