drivers/tty/serial/serial_base.h | 1 + drivers/tty/serial/serial_base_bus.c | 28 +++++++++++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-)
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(-)
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);
}
--
2.41.0On Thu, Aug 10, 2023 at 09:57:34AM +0300, Tony Lindgren wrote: > 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. ... > + unsigned int min = 0, max = ~0U; Shouldn't this be int? The max IIRC will be INT_MAX with this anyway. -- With Best Regards, Andy Shevchenko
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). -- With Best Regards, Andy Shevchenko
* 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 :) Regards, Tony
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. -- With Best Regards, Andy Shevchenko
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 */ -- With Best Regards, Andy Shevchenko
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. -- With Best Regards, Andy Shevchenko
* 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
On Aug 10, 2023 at 09:57:34 +0300, Tony Lindgren wrote:
> 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(-)
>
> 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;
LGTM!
Reviewed-by: Dhruva Gole <d-gole@ti.com>
[...]
--
Best regards,
Dhruva Gole <d-gole@ti.com>
On Aug 10, 2023 at 09:57:34 +0300, Tony Lindgren wrote:
> 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>
> ---
Please can you provide the base commit? I am unable to cleanly apply
this patch on commit: 21ef7b1e17d0 (tag: next-20230809, linux-next/master) Add linux-next specific files for 20230809
> drivers/tty/serial/serial_base.h | 1 +
> drivers/tty/serial/serial_base_bus.c | 28 +++++++++++++++++++++++++++-
> 2 files changed, 28 insertions(+), 1 deletion(-)
>
[...]
>
> 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);
> }
>
> --
> 2.41.0
>
This is the issue I faced:
➜ patch -p1 < patches/20230810_tony_serial_core_fix_serial_core_port_id_including_multiport_devices.mbx
patching file drivers/tty/serial/serial_base.h
patching file drivers/tty/serial/serial_base_bus.c
Hunk #3 FAILED at 145.
1 out of 4 hunks FAILED -- saving rejects to file
drivers/tty/serial/serial_base_bus.c.rej
Perhaps this patch needs a rebase?
--
Best regards,
Dhruva Gole <d-gole@ti.com>
* Dhruva Gole <d-gole@ti.com> [230810 08:36]: > Please can you provide the base commit? I am unable to cleanly apply > this patch on commit: 21ef7b1e17d0 (tag: next-20230809, linux-next/master) Add linux-next specific files for 20230809 This is against the current tty tree tty-linus branch, not yet in next. Regards, Tony
On Thu, Aug 10, 2023 at 11:49:33AM +0300, Tony Lindgren wrote: > * Dhruva Gole <d-gole@ti.com> [230810 08:36]: > > Please can you provide the base commit? I am unable to cleanly apply > > this patch on commit: 21ef7b1e17d0 (tag: next-20230809, linux-next/master) Add linux-next specific files for 20230809 > > This is against the current tty tree tty-linus branch, not yet in next. What he is suggesting is you using --base when formatting your patch. Then you don't need to repeat this to every reviewer/tester/etc :-) -- With Best Regards, Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.