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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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.

>   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).

> +};

...

> +static DEFINE_SPINLOCK(linedisp_attachments_lock);

Why spin lock and not mutex?

...

> +/**
> + * 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.

> + * 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.

...

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

> +	timer_delete_sync(&linedisp->timer);
> +
> +	device_remove_groups(dev, linedisp_groups);
> +
> +	kfree(linedisp->map);
> +	kfree(linedisp->message);
> +	kfree(linedisp->buf);
> +}

-- 
With Best Regards,
Andy Shevchenko