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 <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
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.0
* Dhruva Gole <[email protected]> [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 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 <[email protected]>
> Signed-off-by: Tony Lindgren <[email protected]>
> ---
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 <[email protected]>
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 <[email protected]>
> Signed-off-by: Tony Lindgren <[email protected]>
> ---
> 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 <[email protected]>
[...]
--
Best regards,
Dhruva Gole <[email protected]>
On Thu, Aug 10, 2023 at 11:49:33AM +0300, Tony Lindgren wrote:
> * Dhruva Gole <[email protected]> [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
On 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 <[email protected]> [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 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 <[email protected]> [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
On Fri, Aug 11, 2023 at 08:11:21AM +0300, Tony Lindgren wrote:
> * Andy Shevchenko <[email protected]> [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
* Andy Shevchenko <[email protected]> [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 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 <[email protected]> [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