| Message ID | 20230810065737.47294-1-tony@atomide.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: core: Fix serial core port id, including multiport devices | expand |
On Fri, Aug 11, 2023 at 08:11:21AM +0300, Tony Lindgren wrote: > * Andy Shevchenko <andriy.shevchenko@intel.com> [230810 15:26]: > > On Thu, Aug 10, 2023 at 06:24:13PM +0300, Andy Shevchenko wrote: > > > On Thu, Aug 10, 2023 at 09:57:34AM +0300, Tony Lindgren wrote: ... > > > > + unsigned int min = 0, max = ~0U; > > > > > > Shouldn't this be int? The max IIRC will be INT_MAX with this anyway. > > > > Ah, and then you can supply is as 0 (IIRC). > > The returned id will be INT_MAX, but idr.h uses unsigned int: > > int ida_alloc_range(struct ida *, unsigned int min, unsigned int max, gfp_t); > > If there's some reason to limit max id we can do it, otherwise it's just > a don't care flag. > > Please clarify if I'm not following what you are suggesting :) ... max = 0; Will have the same effect with more explicit intention "use whatever maximum is default". With ~0U I would expect to see something bigger than INT_MAX, but it won't ever appear.
On Fri, Aug 11, 2023 at 12:35:56PM +0300, Andy Shevchenko wrote: > On Fri, Aug 11, 2023 at 12:35:01PM +0300, Andy Shevchenko wrote: > > On Fri, Aug 11, 2023 at 08:11:21AM +0300, Tony Lindgren wrote: > > > * Andy Shevchenko <andriy.shevchenko@intel.com> [230810 15:26]: > > > > On Thu, Aug 10, 2023 at 06:24:13PM +0300, Andy Shevchenko wrote: > > > > > On Thu, Aug 10, 2023 at 09:57:34AM +0300, Tony Lindgren wrote: ... > > > > > > + unsigned int min = 0, max = ~0U; > > > > > > > > > > Shouldn't this be int? The max IIRC will be INT_MAX with this anyway. > > > > > > > > Ah, and then you can supply is as 0 (IIRC). > > > > > > The returned id will be INT_MAX, but idr.h uses unsigned int: > > > > > > int ida_alloc_range(struct ida *, unsigned int min, unsigned int max, gfp_t); > > > > > > If there's some reason to limit max id we can do it, otherwise it's just > > > a don't care flag. > > > > > > Please clarify if I'm not following what you are suggesting :) > > > > ... max = 0; > > > > Will have the same effect with more explicit intention "use whatever maximum is > > default". With ~0U I would expect to see something bigger than INT_MAX, but it > > won't ever appear. > > You can put a comment on top > > /* Use 0 for max to apply IDA defaults */ Hmm... Looking into the implementation code it seems better to have /* Use -1 for max to apply IDA defaults */ int min = 0, max = -1; And supply like that.
* Andy Shevchenko <andriy.shevchenko@intel.com> [230811 09:40]: > Hmm... Looking into the implementation code it seems better to have > > /* Use -1 for max to apply IDA defaults */ > int min = 0, max = -1; > > And supply like that. OK let's use that, will send out v2. Thanks, Tony
diff --git a/drivers/tty/serial/serial_base.h b/drivers/tty/serial/serial_base.h --- a/drivers/tty/serial/serial_base.h +++ b/drivers/tty/serial/serial_base.h @@ -16,6 +16,7 @@ struct device; struct serial_ctrl_device { struct device dev; + struct ida port_ida; }; struct serial_port_device { diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c --- a/drivers/tty/serial/serial_base_bus.c +++ b/drivers/tty/serial/serial_base_bus.c @@ -10,6 +10,7 @@ #include <linux/container_of.h> #include <linux/device.h> +#include <linux/idr.h> #include <linux/module.h> #include <linux/serial_core.h> #include <linux/slab.h> @@ -112,6 +113,8 @@ struct serial_ctrl_device *serial_base_ctrl_add(struct uart_port *port, if (!ctrl_dev) return ERR_PTR(-ENOMEM); + ida_init(&ctrl_dev->port_ida); + err = serial_base_device_init(port, &ctrl_dev->dev, parent, &serial_ctrl_type, serial_base_ctrl_release, @@ -142,16 +145,31 @@ struct serial_port_device *serial_base_port_add(struct uart_port *port, struct serial_ctrl_device *ctrl_dev) { struct serial_port_device *port_dev; + unsigned int min = 0, max = ~0U; int err; port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL); if (!port_dev) return ERR_PTR(-ENOMEM); + /* Device driver specified port_id vs automatic assignment? */ + if (port->port_id) { + min = port->port_id; + max = port->port_id; + } + + err = ida_alloc_range(&ctrl_dev->port_ida, min, max, GFP_KERNEL); + if (err < 0) { + kfree(port_dev); + return ERR_PTR(err); + } + + port->port_id = err; + err = serial_base_device_init(port, &port_dev->dev, &ctrl_dev->dev, &serial_port_type, serial_base_port_release, - port->ctrl_id, port->line); + port->ctrl_id, port->port_id); if (err) goto err_put_device; @@ -165,16 +183,24 @@ struct serial_port_device *serial_base_port_add(struct uart_port *port, err_put_device: put_device(&port_dev->dev); + ida_free(&ctrl_dev->port_ida, port->port_id); return ERR_PTR(err); } void serial_base_port_device_remove(struct serial_port_device *port_dev) { + struct serial_ctrl_device *ctrl_dev; + struct device *parent; + if (!port_dev) return; + parent = port_dev->dev.parent; + ctrl_dev = to_serial_base_ctrl_device(parent); + device_del(&port_dev->dev); + ida_free(&ctrl_dev->port_ida, port_dev->port->port_id); put_device(&port_dev->dev); }
We want to fix the serial core port DEVNAME to use a port id of the hardware specific controller port instance instead of the port->line. For example, the 8250 driver sets up a number of serial8250 ports initially that can be inherited by the hardware specific driver. At that the port->line no longer decribes the port's relation to the serial core controller instance. Let's fix the issue by assigning port->port_id for each serial core controller port instance. Fixes: 7d695d83767c ("serial: core: Fix serial_base_match() after fixing controller port name") Tested-by: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/tty/serial/serial_base.h | 1 + drivers/tty/serial/serial_base_bus.c | 28 +++++++++++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-)