[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9223FFA5-2B0F-4A74-AFE2-CB7C9703CFAB@gmail.com>
Date: Tue, 02 Sep 2025 13:37:52 -0400
From: Jean-François Lessard <jefflessard3@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
CC: Andy Shevchenko <andy@...nel.org>, Geert Uytterhoeven <geert@...ux-m68k.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] auxdisplay: linedisp: support attribute attachment to auxdisplay devices
Le 2 septembre 2025 06 h 18 min 24 s HAE, Andy Shevchenko <andriy.shevchenko@...el.com> a écrit :
>On Sun, Aug 31, 2025 at 10:00:28PM -0400, Jean-François Lessard wrote:
>> Enable linedisp library integration into existing kernel devices (like LED
>> class) to provide a uniform 7-segment userspace API without creating
>> separate child devices, meeting the consistent interface while maintaining
>> coherent device hierarchies.
>>
>> This allows uniform 7-segment API across all drivers while solving device
>> proliferation and fragmented userspace interfaces.
>>
>> The provided attributes appear in two locations depending on usage:
>
>You wanted to say "...in one of the two possible..."?
>Otherwise it looks like undesired side effect of the change.
>
Yes, your wording is much clearer. I will rephrase:
The provided attributes appear in one of the two locations depending on usage:
>> 1. On linedisp.N child devices (legacy linedisp_register())
>> 2. On the parent auxdisplay device (new linedisp_attach())
>> Functionality is identical in both modes.
>>
>> Existing consumers of linedisp_register() are unaffected. The new API
>> enables drivers like TM16XX to integrate 7-segment display functionality
>> seamlessly within their LED class device hierarchy.
>
>...
>
>> +struct linedisp_attachment {
>> + struct list_head list;
>> + struct device *device;
>> + struct linedisp *linedisp;
>
>> + bool owns_device; /* true for child device mode, false for attached mode */
>
>I would rename this to
>
> bool attached; // with inverted logic
>
>or
> bool mode; // with "default" (false) mode to be actually legacy one
>
>(so in both cases I think we want false for the legacy mode).
>
Understood. I will rename, invert logic and also document as kernel-doc instead
of inline comment.
>> +};
>
>...
>
>> +static DEFINE_SPINLOCK(linedisp_attachments_lock);
>
>Why spin lock and not mutex?
>
The attachment list operations are extremely lightweight (just adding/removing
list entries), making spinlock the optimal choice because:
- Very short critical sections: Only list traversal and pointer assignments;
avoids context switching overhead for brief operations
- No sleeping operations: No memory allocation or I/O within locked sections
- Future-proof atomic context safety: Can be safely called from interrupt
handlers if needed
>...
>
>> +/**
>> + * linedisp_attach - attach a character line display
>> + * @linedisp: pointer to character line display structure
>> + * @dev: pointer of the device to attach to
>> + * @num_chars: the number of characters that can be displayed
>> + * @ops: character line display operations
>> + *
>
>Can you add a description here, please? Important note that it should be freed
>by the respective API afterwards.
>
Yes, I will clarify that attach/register operations must be freed using
their corresponding detach/unregister operations.
>> + * Return: zero on success, else a negative error code.
>> + */
>
>...
>
>> + /* 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;
>
>Can this be racy with user space? Once we publish attributes, the new
>message can be already issued and here it puts another message.
>OTOH this is done before device_add(), so the attributes are not visible
>until the device is added.
>
This concern is perfectly valid. linedisp_attach() can and will be called after
device_add() which can be racy. In fact, current linedisp_attach() replicates
the linedisp_register() logic which is also racy since it displays initial
message after device_add(). It needs to be fixed in both attach/register cases
by first initializing the display message before calling
device_add_groups()/device_add().
>...
>
>> +void linedisp_detach(struct device *dev)
>> +{
>
>> + struct linedisp *linedisp = delete_attachment(dev, false);
>> +
>> + if (!linedisp)
>> + return;
>
>Please, rewrite as
>
> struct linedisp *linedisp;
>
> linedisp = delete_attachment(dev, false);
> if (!linedisp)
> return;
>
Acknowledged. I will separate assignment from declaration.
>> + timer_delete_sync(&linedisp->timer);
>> +
>> + device_remove_groups(dev, linedisp_groups);
>> +
>> + kfree(linedisp->map);
>> + kfree(linedisp->message);
>> + kfree(linedisp->buf);
>> +}
>
Powered by blists - more mailing lists