2023-10-24 11:36:56

by Tony Lindgren

[permalink] [raw]
Subject: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

Let's move tty and serdev controller to be children of the serial core port
device. This way the runtime PM usage count of a child device propagates
to the serial hardware device.

The tty and serdev devices are associated with a specific serial port of
a serial hardware controller device, and we now have serial core hierarchy
of controllers and ports.

The tty device moves happily with just a change of the parent device.
The serdev device init needs some changes to separate the serial hardware
controller device from the parent device.

Suggested-by: Johan Hovold <[email protected]>
Cc: Maximilian Luz <[email protected]>
Cc: Rob Herring <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---

AFAIK no urgent need to for these, will repost after the merge window

---
drivers/tty/serdev/core.c | 15 +++++++++------
drivers/tty/serdev/serdev-ttyport.c | 3 ++-
drivers/tty/serial/serial_core.c | 3 ++-
drivers/tty/tty_port.c | 16 +++++++++-------
include/linux/serdev.h | 8 +++++++-
include/linux/tty_port.h | 4 ++--
6 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -476,6 +476,7 @@ EXPORT_SYMBOL_GPL(serdev_device_alloc);

/**
* serdev_controller_alloc() - Allocate a new serdev controller
+ * @host: serial port hardware controller device
* @parent: parent device
* @size: size of private data
*
@@ -484,8 +485,9 @@ EXPORT_SYMBOL_GPL(serdev_device_alloc);
* The allocated private data region may be accessed via
* serdev_controller_get_drvdata()
*/
-struct serdev_controller *serdev_controller_alloc(struct device *parent,
- size_t size)
+struct serdev_controller *serdev_controller_alloc(struct device *host,
+ struct device *parent,
+ size_t size)
{
struct serdev_controller *ctrl;
int id;
@@ -510,7 +512,8 @@ struct serdev_controller *serdev_controller_alloc(struct device *parent,
ctrl->dev.type = &serdev_ctrl_type;
ctrl->dev.bus = &serdev_bus_type;
ctrl->dev.parent = parent;
- ctrl->dev.of_node = parent->of_node;
+ ctrl->host = host;
+ ctrl->dev.of_node = host->of_node;
serdev_controller_set_drvdata(ctrl, &ctrl[1]);

dev_set_name(&ctrl->dev, "serial%d", id);
@@ -673,7 +676,7 @@ static int acpi_serdev_check_resources(struct serdev_controller *ctrl,
acpi_get_parent(adev->handle, &lookup.controller_handle);

/* Make sure controller and ResourceSource handle match */
- if (ACPI_HANDLE(ctrl->dev.parent) != lookup.controller_handle)
+ if (ACPI_HANDLE(ctrl->host) != lookup.controller_handle)
return -ENODEV;

return 0;
@@ -738,7 +741,7 @@ static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
bool skip;
int ret;

- if (!has_acpi_companion(ctrl->dev.parent))
+ if (!has_acpi_companion(ctrl->host))
return -ENODEV;

/*
@@ -747,7 +750,7 @@ static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
* succeed in this case, so that the proper serdev devices can be
* added "manually" later.
*/
- ret = acpi_quirk_skip_serdev_enumeration(ctrl->dev.parent, &skip);
+ ret = acpi_quirk_skip_serdev_enumeration(ctrl->host, &skip);
if (ret)
return ret;
if (skip)
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -274,6 +274,7 @@ static const struct serdev_controller_ops ctrl_ops = {
};

struct device *serdev_tty_port_register(struct tty_port *port,
+ struct device *host,
struct device *parent,
struct tty_driver *drv, int idx)
{
@@ -284,7 +285,7 @@ struct device *serdev_tty_port_register(struct tty_port *port,
if (!port || !drv || !parent)
return ERR_PTR(-ENODEV);

- ctrl = serdev_controller_alloc(parent, sizeof(struct serport));
+ ctrl = serdev_controller_alloc(host, parent, sizeof(struct serport));
if (!ctrl)
return ERR_PTR(-ENOMEM);
serport = serdev_controller_get_drvdata(ctrl);
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3153,7 +3153,8 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
* setserial to be used to alter this port's parameters.
*/
tty_dev = tty_port_register_device_attr_serdev(port, drv->tty_driver,
- uport->line, uport->dev, port, uport->tty_groups);
+ uport->line, uport->dev, &uport->port_dev->dev, port,
+ uport->tty_groups);
if (!IS_ERR(tty_dev)) {
device_set_wakeup_capable(tty_dev, 1);
} else {
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -171,7 +171,8 @@ EXPORT_SYMBOL_GPL(tty_port_register_device_attr);
* @port: tty_port of the device
* @driver: tty_driver for this device
* @index: index of the tty
- * @device: parent if exists, otherwise NULL
+ * @host: serial port hardware device
+ * @parent: parent if exists, otherwise NULL
* @drvdata: driver data for the device
* @attr_grp: attribute group for the device
*
@@ -180,20 +181,20 @@ EXPORT_SYMBOL_GPL(tty_port_register_device_attr);
*/
struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
struct tty_driver *driver, unsigned index,
- struct device *device, void *drvdata,
+ struct device *host, struct device *parent, void *drvdata,
const struct attribute_group **attr_grp)
{
struct device *dev;

tty_port_link_device(port, driver, index);

- dev = serdev_tty_port_register(port, device, driver, index);
+ dev = serdev_tty_port_register(port, host, parent, driver, index);
if (PTR_ERR(dev) != -ENODEV) {
/* Skip creating cdev if we registered a serdev device */
return dev;
}

- return tty_register_device_attr(driver, index, device, drvdata,
+ return tty_register_device_attr(driver, index, parent, drvdata,
attr_grp);
}
EXPORT_SYMBOL_GPL(tty_port_register_device_attr_serdev);
@@ -203,17 +204,18 @@ EXPORT_SYMBOL_GPL(tty_port_register_device_attr_serdev);
* @port: tty_port of the device
* @driver: tty_driver for this device
* @index: index of the tty
- * @device: parent if exists, otherwise NULL
+ * @host: serial port hardware controller device
+ * @parent: parent if exists, otherwise NULL
*
* Register a serdev or tty device depending on if the parent device has any
* defined serdev clients or not.
*/
struct device *tty_port_register_device_serdev(struct tty_port *port,
struct tty_driver *driver, unsigned index,
- struct device *device)
+ struct device *host, struct device *parent)
{
return tty_port_register_device_attr_serdev(port, driver, index,
- device, NULL, NULL);
+ host, parent, NULL, NULL);
}
EXPORT_SYMBOL_GPL(tty_port_register_device_serdev);

diff --git a/include/linux/serdev.h b/include/linux/serdev.h
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -99,12 +99,14 @@ struct serdev_controller_ops {
/**
* struct serdev_controller - interface to the serdev controller
* @dev: Driver model representation of the device.
+ * @host: Serial port hardware controller device
* @nr: number identifier for this controller/bus.
* @serdev: Pointer to slave device for this controller.
* @ops: Controller operations.
*/
struct serdev_controller {
struct device dev;
+ struct device *host;
unsigned int nr;
struct serdev_device *serdev;
const struct serdev_controller_ops *ops;
@@ -167,7 +169,9 @@ struct serdev_device *serdev_device_alloc(struct serdev_controller *);
int serdev_device_add(struct serdev_device *);
void serdev_device_remove(struct serdev_device *);

-struct serdev_controller *serdev_controller_alloc(struct device *, size_t);
+struct serdev_controller *serdev_controller_alloc(struct device *host,
+ struct device *parent,
+ size_t);
int serdev_controller_add(struct serdev_controller *);
void serdev_controller_remove(struct serdev_controller *);

@@ -311,11 +315,13 @@ struct tty_driver;

#ifdef CONFIG_SERIAL_DEV_CTRL_TTYPORT
struct device *serdev_tty_port_register(struct tty_port *port,
+ struct device *host,
struct device *parent,
struct tty_driver *drv, int idx);
int serdev_tty_port_unregister(struct tty_port *port);
#else
static inline struct device *serdev_tty_port_register(struct tty_port *port,
+ struct device *host,
struct device *parent,
struct tty_driver *drv, int idx)
{
diff --git a/include/linux/tty_port.h b/include/linux/tty_port.h
--- a/include/linux/tty_port.h
+++ b/include/linux/tty_port.h
@@ -149,10 +149,10 @@ struct device *tty_port_register_device_attr(struct tty_port *port,
const struct attribute_group **attr_grp);
struct device *tty_port_register_device_serdev(struct tty_port *port,
struct tty_driver *driver, unsigned index,
- struct device *device);
+ struct device *host, struct device *parent);
struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
struct tty_driver *driver, unsigned index,
- struct device *device, void *drvdata,
+ struct device *host, struct device *parent, void *drvdata,
const struct attribute_group **attr_grp);
void tty_port_unregister_device(struct tty_port *port,
struct tty_driver *driver, unsigned index);

base-commit: 6f699743aebf07538e506a46c5965eb8bdd2c716
--
2.42.0


2023-10-24 11:37:09

by Tony Lindgren

[permalink] [raw]
Subject: [RFC PATCH 2/2] serial: core: Revert checks for tx runtime PM state

This reverts commit 81a61051e0ce5fd7e09225c0d5985da08c7954a7.

With tty and serdev controller moved to be children of the serial core
port device, runtime PM usage count of the serdev controller now
propagates to the serial hardware controller parent device as expected.

Cc: Maximilian Luz <[email protected]>
Cc: Rob Herring <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
drivers/tty/serial/serial_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -156,7 +156,7 @@ static void __uart_start(struct uart_state *state)
* enabled, serial_port_runtime_resume() calls start_tx() again
* after enabling the device.
*/
- if (!pm_runtime_enabled(port->dev) || pm_runtime_active(port->dev))
+ if (pm_runtime_active(&port_dev->dev))
port->ops->start_tx(port);
pm_runtime_mark_last_busy(&port_dev->dev);
pm_runtime_put_autosuspend(&port_dev->dev);
--
2.42.0

2023-10-24 11:52:30

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

On Tue, Oct 24, 2023 at 02:36:18PM +0300, Tony Lindgren wrote:
> Let's move tty and serdev controller to be children of the serial core port
> device. This way the runtime PM usage count of a child device propagates
> to the serial hardware device.
>
> The tty and serdev devices are associated with a specific serial port of
> a serial hardware controller device, and we now have serial core hierarchy
> of controllers and ports.
>
> The tty device moves happily with just a change of the parent device.
> The serdev device init needs some changes to separate the serial hardware
> controller device from the parent device.
>

What does this change the sysfs tree to look like?

No objection from me, just curious.

thanks,

greg k-h

2023-10-24 12:17:52

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

On Tue, Oct 24, 2023 at 02:36:18PM +0300, Tony Lindgren wrote:
> Let's move tty and serdev controller to be children of the serial core port
> device. This way the runtime PM usage count of a child device propagates
> to the serial hardware device.
>
> The tty and serdev devices are associated with a specific serial port of
> a serial hardware controller device, and we now have serial core hierarchy
> of controllers and ports.
>
> The tty device moves happily with just a change of the parent device.
> The serdev device init needs some changes to separate the serial hardware
> controller device from the parent device.

...

> - ctrl->dev.of_node = parent->of_node;
> + ctrl->dev.of_node = host->of_node;

Even above should have been using device_set_node(&ctrl->dev, dev_fwnode(host)).

...

> /* Make sure controller and ResourceSource handle match */
> - if (ACPI_HANDLE(ctrl->dev.parent) != lookup.controller_handle)
> + if (ACPI_HANDLE(ctrl->host) != lookup.controller_handle)

This can be changed to use device_match_acpi_handle().

> return -ENODEV;

...

> - if (!has_acpi_companion(ctrl->dev.parent))
> + if (!has_acpi_companion(ctrl->host))

I prefer is_acpi_device_node(dev_fwnode(...)) check, but here seems no other
use for fwnode (haven't checked the full context, though).

> return -ENODEV;

--
With Best Regards,
Andy Shevchenko


2023-10-24 12:30:02

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

* Greg Kroah-Hartman <[email protected]> [231024 11:52]:
> What does this change the sysfs tree to look like?

On x86 qemu for the ttys:

# find /sys -name tty
/sys/class/tty
/sys/class/tty/tty
/sys/devices/pnp0/00:04/00:04:0/00:04:0.0/tty
/sys/devices/platform/serial8250/serial8250:0/serial8250:0.3/tty
/sys/devices/platform/serial8250/serial8250:0/serial8250:0.1/tty
/sys/devices/platform/serial8250/serial8250:0/serial8250:0.2/tty
/sys/devices/virtual/tty
/sys/devices/virtual/tty/tty

Regards,

Tony

2023-10-24 12:43:05

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

On Tue, Oct 24, 2023 at 03:29:55PM +0300, Tony Lindgren wrote:
> * Greg Kroah-Hartman <[email protected]> [231024 11:52]:
> > What does this change the sysfs tree to look like?
>
> On x86 qemu for the ttys:
>
> # find /sys -name tty
> /sys/class/tty
> /sys/class/tty/tty
> /sys/devices/pnp0/00:04/00:04:0/00:04:0.0/tty
> /sys/devices/platform/serial8250/serial8250:0/serial8250:0.3/tty
> /sys/devices/platform/serial8250/serial8250:0/serial8250:0.1/tty
> /sys/devices/platform/serial8250/serial8250:0/serial8250:0.2/tty
> /sys/devices/virtual/tty
> /sys/devices/virtual/tty/tty

I believe the question was how serdev device will move it's location
in the hierarchy.

--
With Best Regards,
Andy Shevchenko


2023-10-24 12:44:23

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

On Tue, Oct 24, 2023 at 03:17:26PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 24, 2023 at 02:36:18PM +0300, Tony Lindgren wrote:

...

> > - ctrl->dev.of_node = parent->of_node;
> > + ctrl->dev.of_node = host->of_node;
>
> Even above should have been using device_set_node(&ctrl->dev, dev_fwnode(host)).

...

> > /* Make sure controller and ResourceSource handle match */
> > - if (ACPI_HANDLE(ctrl->dev.parent) != lookup.controller_handle)
> > + if (ACPI_HANDLE(ctrl->host) != lookup.controller_handle)
>
> This can be changed to use device_match_acpi_handle().
>
> > return -ENODEV;

...

> > - if (!has_acpi_companion(ctrl->dev.parent))
> > + if (!has_acpi_companion(ctrl->host))
>
> I prefer is_acpi_device_node(dev_fwnode(...)) check, but here seems no other
> use for fwnode (haven't checked the full context, though).
>
> > return -ENODEV;

...

I have just sent a little series based on these comments.

--
With Best Regards,
Andy Shevchenko


2023-10-24 12:46:21

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

* Andy Shevchenko <[email protected]> [231024 12:42]:
> On Tue, Oct 24, 2023 at 03:29:55PM +0300, Tony Lindgren wrote:
> > * Greg Kroah-Hartman <[email protected]> [231024 11:52]:
> > > What does this change the sysfs tree to look like?
> >
> > On x86 qemu for the ttys:
> >
> > # find /sys -name tty
> > /sys/class/tty
> > /sys/class/tty/tty
> > /sys/devices/pnp0/00:04/00:04:0/00:04:0.0/tty
> > /sys/devices/platform/serial8250/serial8250:0/serial8250:0.3/tty
> > /sys/devices/platform/serial8250/serial8250:0/serial8250:0.1/tty
> > /sys/devices/platform/serial8250/serial8250:0/serial8250:0.2/tty
> > /sys/devices/virtual/tty
> > /sys/devices/virtual/tty/tty
>
> I believe the question was how serdev device will move it's location
> in the hierarchy.

If used, a serdev device serial0 will appear instead of the tty in
the same location.

Regards,

Tony

2023-10-24 12:48:50

by Johan Hovold

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

On Tue, Oct 24, 2023 at 02:36:18PM +0300, Tony Lindgren wrote:
> Let's move tty and serdev controller to be children of the serial core port
> device. This way the runtime PM usage count of a child device propagates
> to the serial hardware device.

> The tty device moves happily with just a change of the parent device.

> @@ -3153,7 +3153,8 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
> * setserial to be used to alter this port's parameters.
> */
> tty_dev = tty_port_register_device_attr_serdev(port, drv->tty_driver,
> - uport->line, uport->dev, port, uport->tty_groups);
> + uport->line, uport->dev, &uport->port_dev->dev, port,
> + uport->tty_groups);
> if (!IS_ERR(tty_dev)) {
> device_set_wakeup_capable(tty_dev, 1);
> } else {

> struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
> struct tty_driver *driver, unsigned index,
> - struct device *device, void *drvdata,
> + struct device *host, struct device *parent, void *drvdata,
> const struct attribute_group **attr_grp)
> {
> struct device *dev;
>
> tty_port_link_device(port, driver, index);
>
> - dev = serdev_tty_port_register(port, device, driver, index);
> + dev = serdev_tty_port_register(port, host, parent, driver, index);
> if (PTR_ERR(dev) != -ENODEV) {
> /* Skip creating cdev if we registered a serdev device */
> return dev;
> }
>
> - return tty_register_device_attr(driver, index, device, drvdata,
> + return tty_register_device_attr(driver, index, parent, drvdata,
> attr_grp);
> }

Looks like this patch breaks the wakeup-irq hack in uart_suspend_port():

tty_dev = device_find_child(uport->dev, &match, serial_match_port);
if (tty_dev && device_may_wakeup(tty_dev)) {
enable_irq_wake(uport->irq);
put_device(tty_dev);
mutex_unlock(&port->mutex);
return 0;
}

There may be more of these hard-coded assumptions, this one I happened
to be aware of.

Johan

2023-10-24 13:39:14

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

* Johan Hovold <[email protected]> [231024 12:48]:
> Looks like this patch breaks the wakeup-irq hack in uart_suspend_port():
>
> tty_dev = device_find_child(uport->dev, &match, serial_match_port);
> if (tty_dev && device_may_wakeup(tty_dev)) {
> enable_irq_wake(uport->irq);
> put_device(tty_dev);
> mutex_unlock(&port->mutex);
> return 0;
> }
>
> There may be more of these hard-coded assumptions, this one I happened
> to be aware of.

OK thanks I'll take a look.

Regards,

Tony

2023-10-24 14:02:01

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

On Tue, Oct 24, 2023 at 03:29:55PM +0300, Tony Lindgren wrote:
> * Greg Kroah-Hartman <[email protected]> [231024 11:52]:
> > What does this change the sysfs tree to look like?
>
> On x86 qemu for the ttys:
>
> # find /sys -name tty
> /sys/class/tty
> /sys/class/tty/tty
> /sys/devices/pnp0/00:04/00:04:0/00:04:0.0/tty
> /sys/devices/platform/serial8250/serial8250:0/serial8250:0.3/tty
> /sys/devices/platform/serial8250/serial8250:0/serial8250:0.1/tty
> /sys/devices/platform/serial8250/serial8250:0/serial8250:0.2/tty
> /sys/devices/virtual/tty
> /sys/devices/virtual/tty/tty

A diff of before vs. after would make more sense for those of us who
don't have your same system configuration :)

thanks,

greg k-h

2023-10-25 06:52:05

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

* Greg Kroah-Hartman <[email protected]> [231024 14:01]:
> On Tue, Oct 24, 2023 at 03:29:55PM +0300, Tony Lindgren wrote:
> > * Greg Kroah-Hartman <[email protected]> [231024 11:52]:
> > > What does this change the sysfs tree to look like?
> >
> > On x86 qemu for the ttys:
> >
> > # find /sys -name tty
> > /sys/class/tty
> > /sys/class/tty/tty
> > /sys/devices/pnp0/00:04/00:04:0/00:04:0.0/tty
> > /sys/devices/platform/serial8250/serial8250:0/serial8250:0.3/tty
> > /sys/devices/platform/serial8250/serial8250:0/serial8250:0.1/tty
> > /sys/devices/platform/serial8250/serial8250:0/serial8250:0.2/tty
> > /sys/devices/virtual/tty
> > /sys/devices/virtual/tty/tty
>
> A diff of before vs. after would make more sense for those of us who
> don't have your same system configuration :)

Here's the diff of the same command before and after:

--- /tmp/before 2023-10-25 09:45:12.197283690 +0300
+++ /tmp/after 2023-10-25 09:43:30.681797899 +0300
@@ -1,7 +1,9 @@
# find /sys -name tty
/sys/class/tty
/sys/class/tty/tty
-/sys/devices/pnp0/00:04/tty
-/sys/devices/platform/serial8250/tty
+/sys/devices/pnp0/00:04/00:04:0/00:04:0.0/tty
+/sys/devices/platform/serial8250/serial8250:0/serial8250:0.3/tty
+/sys/devices/platform/serial8250/serial8250:0/serial8250:0.1/tty
+/sys/devices/platform/serial8250/serial8250:0/serial8250:0.2/tty
/sys/devices/virtual/tty
/sys/devices/virtual/tty/tty

There are multiple ports claimed by serial8250. So I think the new sysfs
output is correct showing more ttys. If there's some reason why serial8250
should only have one tty and this output is not correct let me know too..

Regards,

Tony

2023-10-25 06:53:21

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

* Andy Shevchenko <[email protected]> [231024 12:43]:
> I have just sent a little series based on these comments.

OK sounds good to me thanks.

Tony

2023-10-25 07:34:12

by Johan Hovold

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

On Wed, Oct 25, 2023 at 09:51:52AM +0300, Tony Lindgren wrote:
> * Greg Kroah-Hartman <[email protected]> [231024 14:01]:

> > > > What does this change the sysfs tree to look like?

> Here's the diff of the same command before and after:
>
> --- /tmp/before 2023-10-25 09:45:12.197283690 +0300
> +++ /tmp/after 2023-10-25 09:43:30.681797899 +0300
> @@ -1,7 +1,9 @@
> # find /sys -name tty
> /sys/class/tty
> /sys/class/tty/tty
> -/sys/devices/pnp0/00:04/tty
> -/sys/devices/platform/serial8250/tty
> +/sys/devices/pnp0/00:04/00:04:0/00:04:0.0/tty
> +/sys/devices/platform/serial8250/serial8250:0/serial8250:0.3/tty
> +/sys/devices/platform/serial8250/serial8250:0/serial8250:0.1/tty
> +/sys/devices/platform/serial8250/serial8250:0/serial8250:0.2/tty
> /sys/devices/virtual/tty
> /sys/devices/virtual/tty/tty

Your diff is missing the actual tty devices. 'tty' is just the class
directory.

And can you post the equivalent diff for serdev as well for completeness?

> There are multiple ports claimed by serial8250. So I think the new sysfs
> output is correct showing more ttys. If there's some reason why serial8250
> should only have one tty and this output is not correct let me know too..

There should not be more class devices, you've just moved them and thus
there are more class directories (with one device per directory).

Johan

2023-10-25 08:24:50

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] serial: core: Move tty and serdev to be children of serial core port device

* Johan Hovold <[email protected]> [231025 07:32]:
> Your diff is missing the actual tty devices. 'tty' is just the class
> directory.

Ah right, that explains :) The find must be for ttyS* in this case,
here's the diff for qemu x86 for command:

# find /sys -name ttyS*

--- /tmp/before 2023-10-25 10:50:29.870083012 +0300
+++ /tmp/after 2023-10-25 10:52:52.770393075 +0300
@@ -3,7 +3,7 @@
/sys/class/tty/ttyS0
/sys/class/tty/ttyS3
/sys/class/tty/ttyS1
-/sys/devices/pnp0/00:04/tty/ttyS0
-/sys/devices/platform/serial8250/tty/ttyS2
-/sys/devices/platform/serial8250/tty/ttyS3
-/sys/devices/platform/serial8250/tty/ttyS1
+/sys/devices/pnp0/00:04/00:04:0/00:04:0.0/tty/ttyS0
+/sys/devices/platform/serial8250/serial8250:0/serial8250:0.3/tty/ttyS3
+/sys/devices/platform/serial8250/serial8250:0/serial8250:0.1/tty/ttyS1
+/sys/devices/platform/serial8250/serial8250:0/serial8250:0.2/tty/ttyS2

> And can you post the equivalent diff for serdev as well for completeness?

I don't have an x86 or arm64 testcase for serdev, but here's a armv7 wlcore
hci-uart serdev diff for command:

# find /sys -name ttyS* -o -name serial0

--- /tmp/before 2023-10-25 08:23:15.468382112 +0000
+++ /tmp/after 2023-10-25 08:23:15.468382112 +0000
@@ -1,10 +1,9 @@
-# find /sys -name ttyS* -o -name serial0
-/sys/devices/platform/ocp/48000000.interconnect/48000000.interconnect:segment@0/4806e050.target-module/4806e000.serial/serial0
-/sys/devices/platform/ocp/48000000.interconnect/48000000.interconnect:segment@0/48020050.target-module/48020000.serial/tty/ttyS2
-/sys/devices/platform/ocp/48000000.interconnect/48000000.interconnect:segment@0/4806a050.target-module/4806a000.serial/tty/ttyS0
-/sys/devices/platform/ocp/48000000.interconnect/48000000.interconnect:segment@0/4806c050.target-module/4806c000.serial/tty/ttyS1
-/sys/devices/platform/serial8250/tty/ttyS4
-/sys/devices/platform/serial8250/tty/ttyS5
+/sys/devices/platform/ocp/48000000.interconnect/48000000.interconnect:segment@0/4806e050.target-module/4806e000.serial/4806e000.serial:0/4806e000.serial:0.0/serial0
+/sys/devices/platform/ocp/48000000.interconnect/48000000.interconnect:segment@0/48020050.target-module/48020000.serial/48020000.serial:0/48020000.serial:0.0/tty/ttyS2
+/sys/devices/platform/ocp/48000000.interconnect/48000000.interconnect:segment@0/4806a050.target-module/4806a000.serial/4806a000.serial:0/4806a000.serial:0.0/tty/ttyS0
+/sys/devices/platform/ocp/48000000.interconnect/48000000.interconnect:segment@0/4806c050.target-module/4806c000.serial/4806c000.serial:0/4806c000.serial:0.0/tty/ttyS1
+/sys/devices/platform/serial8250/serial8250:0/serial8250:0.5/tty/ttyS5
+/sys/devices/platform/serial8250/serial8250:0/serial8250:0.4/tty/ttyS4
/sys/class/tty/ttyS4
/sys/class/tty/ttyS2
/sys/class/tty/ttyS0

> > There are multiple ports claimed by serial8250. So I think the new sysfs
> > output is correct showing more ttys. If there's some reason why serial8250
> > should only have one tty and this output is not correct let me know too..
>
> There should not be more class devices, you've just moved them and thus
> there are more class directories (with one device per directory).

OK makes sense.

Thanks,

Tony