Received-SPF: pass (google.com: domain of linux-kernel+bounces-196525-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 31 May 2024 16:58:29 +0800
From: Crescent CY Hsieh <crescentcy.hsieh@moxa.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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: <ZlmRNVONKs3MWnM8@localhost.localdomain>
References: <20240530094457.1974-1-crescentcy.hsieh@moxa.com>
 <2024053013-clapping-germless-d50d@gregkh>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2024053013-clapping-germless-d50d@gregkh>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?uAkmSjlEnt/MqzNV//NenoxFsJLR0GIGA5fSObSm+yBHGZHyDqy5Fs/xe74Q?=
 =?us-ascii?Q?lne4cYl+dKJBc4Ra3jNXgMCq7vKrbblUPo0llAqwhQd1od9SzhFK2hxdxjV4?=
 =?us-ascii?Q?BrD47Gct/hbqBSLQvG+lhqVQtChdu5j+2leQIHhBn8+QJ0QXtMWMabmswKwe?=
 =?us-ascii?Q?FBbcjQMCshAWHwaxNC9Ueo6CpIzyhtYnr0melp3pcQuOLLtQc3wzfn13HEiC?=
 =?us-ascii?Q?2x+E8w1AdcJkyGJTuZu4cbJsre0EM0yU/fPKDHnxhFM5uz1UwUfYVp0GH6gf?=
 =?us-ascii?Q?gWyRtgOWTI2IYYBK9BeRJXR9eypJAXhriUZfae09uMsZB+2wJIuRxars9/Es?=
 =?us-ascii?Q?ZnRPcgFxdo+eWUf8tg3vqQ/P4pjl1J7LGwSGH5eFY0BYGu4o01mGYhK7HVZn?=
 =?us-ascii?Q?qHezYdcwie5gIWv0Fu9DFgQZta4O1Exe/LuZ7WIiekCheUJ0X4NVSnegmdOm?=
 =?us-ascii?Q?JV+GJxZdD+QRiS4xk3QVNtZoZC4pEMITc6fXtzDnFpHeN06olw22kJ5ZbiGe?=
 =?us-ascii?Q?dQ68lPwpOsZVaLvvLsagC16ExBLH1vUDUqlx7xzRR+cbxsHcAQpn1T/hIH4o?=
 =?us-ascii?Q?CqUvmJtucPqw9BXBiQTG5HqkibUe+y0UtcbHYqLga0cRxeXbx0pmgEvLIAhV?=
 =?us-ascii?Q?rctw0ka32sReCwnWmHC/21OXs+Yc3qr2OXrNdpew4+2jAbdPs2ifJfHBoRYS?=
 =?us-ascii?Q?vVNp6rR99FIoHRh41H0bWCdh7EMfjk+Ar0cpQ32N3JSHhYP/QRwg6EK6e5yu?=
 =?us-ascii?Q?5N9U/7NFUFw+G+xN+FRlWo/fa+P7cyeRgr5D/gczuewufDT9PVcMfkOZBPac?=
 =?us-ascii?Q?qEkikD+BvqxjeYIv9rCnSxkiT6sGw8/w+wYwfavGfeIXFIV+6RQrBINxAyQW?=
 =?us-ascii?Q?LMJECRdGMdf5z9l2FsNt52JKJLECoDumPOZh3xNnlGpCIC4ka66RmQUvPXrk?=
 =?us-ascii?Q?UvZ/ZfVUfhYxIWTtPoWWZKoR+px5AmRGjvmybfGiGa6i84fZ7h1cWw09NzUB?=
 =?us-ascii?Q?230ZXq0cp5YfndXsmFMCAcq0Yokh2vOBfEiOBEFkk+ABK0MGxSIfZKmTU7uI?=
 =?us-ascii?Q?3HanGxPDTpUDWBnKc9rwY5x6V5papXTfD9iio6LQa+EYvL0J7uzzdBu4wZpM?=
 =?us-ascii?Q?nHENozz9XCKpDiXFrtiDFZbTpYXdrgAqGwFrWzdbddYufq6Hw4zEnHscaBLr?=
 =?us-ascii?Q?sNpe8wTaYBNQ94JvLV0GrYSrdWo0qZSLPcH7v623qySPnEc53z/cIUDw/xtO?=
 =?us-ascii?Q?MU0h3hVnW1xhFav20VXycHHmyIB688s12bkj4FLZ0BYtKL5kxoTQd/rTrpEW?=
 =?us-ascii?Q?JlNsi0lEDcAl/NIMlays+Le1J1/XkNX1/okQ7I4VyLq9dnr5iPsWmGs8wo98?=
 =?us-ascii?Q?o2bui7zuBQzqpvWAKQtfAzncwEy84waw7bhqwH78ZmPXPjG0fuAJXp7kqiiq?=
 =?us-ascii?Q?bpWCmHc8wxe0L/tnuaH/x1FEjcuzdS7hiCi73aBpHFaDd/fIh8U8x4YHDcTM?=
 =?us-ascii?Q?zVWQNOsOOQY+8uJ95PrCC2XJfPBqCQkYsHT5mGGzkBouTzrCreV20148z+O6?=
 =?us-ascii?Q?pTSimUdpE1RptOqGUm42Opn/nibKkKYNTV8NFZdwnk53C4pU3xCSh6kzVQ1/?=
 =?us-ascii?Q?7Q=3D=3D?=
X-OriginatorOrg: moxa.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 777b5077-3ba6-4b15-e785-08dc814fd9cc
X-MS-Exchange-CrossTenant-AuthSource: PUZPR01MB5405.apcprd01.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2024 08:58:33.8716
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5571c7d4-286b-47f6-9dd5-0aa688773c8e
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: CKB5X3iM7zh++F86CVw4zv1/RDYsrCIuuMgEntbtZXKD+Lh28QSKdT11ufk03V97qt4YsOyFykc8aLKv6KsVsD2VHHNNsQS/pWPW856uwwQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: TYZPR01MB4260

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