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
