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

From: Crescent CY Hsieh
Date: Fri May 31 2024 - 04:58:54 EST


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