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

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

[CrescentCY Hsieh]

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.

Signed-off-by: CrescentCY Hsieh <crescentcy.hsieh@moxa.com>
---
 drivers/tty/serial/8250/8250_core.c |  9 +++++++++
 drivers/tty/serial/8250/8250_port.c | 26 ++++++++++++++++++++++++--
 include/linux/serial_core.h         |  1 +
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 43824a174a51..01d04f9d5192 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1130,6 +1130,8 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
 			uart->port.pm = up->port.pm;
 		if (up->port.handle_break)
 			uart->port.handle_break = up->port.handle_break;
+		if (up->port.attr_group)
+			uart->port.attr_group = up->port.attr_group;
 		if (up->dl_read)
 			uart->dl_read = up->dl_read;
 		if (up->dl_write)
@@ -1210,6 +1212,13 @@ void serial8250_unregister_port(int line)
 		uart->port.type = PORT_UNKNOWN;
 		uart->port.dev = &serial8250_isa_devs->dev;
 		uart->port.port_id = line;
+
+		if (uart->port.attr_group_allocated) {
+			kfree(uart->port.attr_group->attrs);
+			kfree(uart->port.attr_group);
+			uart->port.attr_group_allocated = false;
+		}
+		uart->port.attr_group = NULL;
 		uart->capabilities = 0;
 		serial8250_init_port(uart);
 		serial8250_apply_quirks(uart);
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;
+	} else {
+		if (conf_type->rxtrig_bytes[0])
+			up->port.attr_group = &serial8250_dev_attr_group;
+	}
 }
 
 static void serial8250_config_port(struct uart_port *port, int flags)
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 8cb65f50e830..3212d64c32c6 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -581,6 +581,7 @@ struct uart_port {
 	unsigned char		console_reinit;
 	const char		*name;			/* port name */
 	struct attribute_group	*attr_group;		/* port specific attributes */
+	bool			attr_group_allocated;	/* whether attr_group is dynamic allocated */
 	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
 	struct serial_rs485     rs485;
 	struct serial_rs485	rs485_supported;	/* Supported mask for serial_rs485 */
-- 
2.34.1

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

[Greg Kroah-Hartman]

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.

> 
> Signed-off-by: CrescentCY Hsieh <crescentcy.hsieh@moxa.com>
> ---
>  drivers/tty/serial/8250/8250_core.c |  9 +++++++++
>  drivers/tty/serial/8250/8250_port.c | 26 ++++++++++++++++++++++++--
>  include/linux/serial_core.h         |  1 +
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 43824a174a51..01d04f9d5192 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1130,6 +1130,8 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
>  			uart->port.pm = up->port.pm;
>  		if (up->port.handle_break)
>  			uart->port.handle_break = up->port.handle_break;
> +		if (up->port.attr_group)
> +			uart->port.attr_group = up->port.attr_group;
>  		if (up->dl_read)
>  			uart->dl_read = up->dl_read;
>  		if (up->dl_write)
> @@ -1210,6 +1212,13 @@ void serial8250_unregister_port(int line)
>  		uart->port.type = PORT_UNKNOWN;
>  		uart->port.dev = &serial8250_isa_devs->dev;
>  		uart->port.port_id = line;
> +
> +		if (uart->port.attr_group_allocated) {
> +			kfree(uart->port.attr_group->attrs);
> +			kfree(uart->port.attr_group);
> +			uart->port.attr_group_allocated = false;

Why is this needed to be set to false now, how can it matter anymore?

> +		}
> +		uart->port.attr_group = NULL;
>  		uart->capabilities = 0;
>  		serial8250_init_port(uart);
>  		serial8250_apply_quirks(uart);
> 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?


> +	} else {
> +		if (conf_type->rxtrig_bytes[0])
> +			up->port.attr_group = &serial8250_dev_attr_group;
> +	}
>  }
>  
>  static void serial8250_config_port(struct uart_port *port, int flags)
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 8cb65f50e830..3212d64c32c6 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -581,6 +581,7 @@ struct uart_port {
>  	unsigned char		console_reinit;
>  	const char		*name;			/* port name */
>  	struct attribute_group	*attr_group;		/* port specific attributes */
> +	bool			attr_group_allocated;	/* whether attr_group is dynamic allocated */

Any way to do this without this variable?

thanks,

greg k-h

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

[Crescent CY Hsieh]

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

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

[Greg Kroah-Hartman]

On Fri, May 31, 2024 at 04:58:29PM +0800, Crescent CY Hsieh wrote:
> 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.

It would be a requirement if this is to even be considered, you know
that, why submit this without that?

But step back, why would we want a serial port driver to have the
ability to have different attributes than the normal ones?  What
attribute would it want to have at this layer that would be unique to
the driver that is controlling it?

That is almost never a good idea, it only can cause confusion and
complex userspace issues when a device type can have different types of
attributes just because of something "below" it controlling it.  So in
this case, why not create a proper child device of it's own specific
type  with those unique attributes instead?

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

What files would be "unsupported" here?

I think we need a real example to understand what you are attempting to
do here.  serial ports should usually never have unique sysfs files as
the serial core code should all control them properly and provide the
needed interfaces to the device.  If not, then why not add it to the
generic serial port type so that all can use them?

thanks,

greg k-h