Re: [PATCH] tty: serial: 8250: Passing attr_group to universal driver

[Crescent CY Hsieh <crescentcy.hsieh@moxa.com>]

On Thu, May 30, 2024 at 02:45:02PM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 30, 2024 at 05:44:57PM +0800, CrescentCY Hsieh wrote:
> > Many low-level drivers in Linux kernel register their serial ports with
> > the help of universal driver (8250_core, 8250_port).
> > 
> > There is an attribute group called `serial8250_dev_attr_group` within
> > `8250_port.c` to handle the `rx_trig_bytes` attribute:
> > https://lore.kernel.org/all/20140716011931.31474.68825.stgit@yuno-kbuild.novalocal/
> > 
> > However, if a low-level driver has some HW specifications that need to
> > be set or retrieved using an attr_group, the universal driver
> > (8250_port) would overwrite the low-level driver's attr_group.
> > 
> > This patch allows the low-level driver's attr_group to be passed to the
> > universal driver (8250_port) and combines the two attr_groups. This
> > ensures that the corresponding system file will only be created if the
> > device is registered by such a low-level driver.
> 
> Great!  But is this needed now by any in-kernel drivers, or is this only
> needed by things that are not in our tree?
> 
> If in our tree, what driver(s) does this fix up?  If none, then for
> obvious reasons, we can't take this change.

Currently, no in-kernel drivers utilize this, but I can add a patch in
v2 to demonstrate how this patch would work.

> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index 893bc493f662..ddfa8b59e562 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -3135,9 +3135,31 @@ static struct attribute_group serial8250_dev_attr_group = {
> >  static void register_dev_spec_attr_grp(struct uart_8250_port *up)
> >  {
> >  	const struct serial8250_config *conf_type = &uart_config[up->port.type];
> > +	struct attribute **upp_attrs = NULL;
> > +	int upp_attrs_num = 0, i;
> >  
> > -	if (conf_type->rxtrig_bytes[0])
> > -		up->port.attr_group = &serial8250_dev_attr_group;
> > +	up->port.attr_group_allocated = false;
> > +
> > +	if (up->port.attr_group) {
> > +		upp_attrs = up->port.attr_group->attrs;
> > +
> > +		while (upp_attrs[upp_attrs_num])
> > +			upp_attrs_num++;
> > +
> > +		up->port.attr_group = kcalloc(1, sizeof(struct attribute_group), GFP_KERNEL);
> > +		up->port.attr_group->attrs = kcalloc(upp_attrs_num + 2, sizeof(struct attribute *), GFP_KERNEL);
> > +
> > +		for (i = 0; i < upp_attrs_num; ++i)
> > +			up->port.attr_group->attrs[i] = upp_attrs[i];
> > +
> > +		if (conf_type->rxtrig_bytes[0])
> > +			up->port.attr_group->attrs[upp_attrs_num] = &dev_attr_rx_trig_bytes.attr;
> > +
> > +		up->port.attr_group_allocated = true;
> 
> This feels odd, why is this all dynamically allocated?  You want to add
> another group to the existing group?

Yes, this approach aims to add the `attr_group` which declared in the
low-level driver to the existing one in universal driver (8250_port.c),
so that it will have the attributes from both low-level and universal
drivers. This ensures that other device nodes utilizing the universal
driver will not create the unsupported system file(s).

Sincerely,
Crescent Hsieh