Received-SPF: pass (google.com: domain of linux-kernel+bounces-195379-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 30 May 2024 14:45:02 +0200
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: CrescentCY Hsieh <crescentcy.hsieh@moxa.com>
Cc: Jiri Slaby <jirislaby@kernel.org>, linux-kernel@vger.kernel.org,
	linux-serial@vger.kernel.org
Subject: Re: [PATCH] tty: serial: 8250: Passing attr_group to universal driver
Message-ID: <2024053013-clapping-germless-d50d@gregkh>
References: <20240530094457.1974-1-crescentcy.hsieh@moxa.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240530094457.1974-1-crescentcy.hsieh@moxa.com>

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