Re: [PATCH 4/5] auxdisplay: linedisp: support attribute attachment to auxdisplay devices

["Jean-François Lessard" <jefflessard3@gmail.com>]

Le 2 septembre 2025 06 h 18 min 24 s HAE, Andy Shevchenko <andriy.shevchenko@intel.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);
>> +}
>