[PATCH 0/4] serdev: fix broken lifetime assumptions

[PATCH 0/4] serdev: fix broken lifetime assumptions

[Johan Hovold]

This series fixes a number of issues with the new serdev code, which was
based on incorrect tty-port lifetime assumptions.

The first patch disables serdev support by reverting the patch which
hooked into the tty layer in a broken way that leads to crashes and
leaks when deregistering devices. This one should probably go into 4.11.

The second patch fixes a specific bug in the tty-port client
registration code, while the third patch adds a new interface for
registering serdev devices. The final patch ultimately enables serdev
again for the serial drivers.

More details can be found in the individual commit messages.

Johan


Johan Hovold (4):
  Revert "tty_port: register tty ports with serdev bus"
  serdev: fix tty-port client deregistration
  tty/serdev: add serdev registration interface
  serial: enable serdev support

 drivers/tty/serdev/serdev-ttyport.c | 21 +++++++----
 drivers/tty/serial/serial_core.c    |  4 +-
 drivers/tty/tty_port.c              | 74 ++++++++++++++++++++++++++++++++++---
 include/linux/serdev.h              |  7 +++-
 include/linux/tty.h                 |  9 +++++
 5 files changed, 99 insertions(+), 16 deletions(-)

-- 
2.12.2

[PATCH 1/4] Revert "tty_port: register tty ports with serdev bus"

[Johan Hovold]

This reverts commit 8ee3fde047589dc9c201251f07d0ca1dc776feca.

The new serdev bus hooked into the tty layer in
tty_port_register_device() by registering a serdev controller instead of
a tty device whenever a serdev client is present, and by deregistering
the controller in the tty-port destructor. This is broken in several
ways:

Firstly, it leads to a NULL-pointer dereference whenever a tty driver
later deregisters its devices as no corresponding character device will
exist.

Secondly, far from every tty driver uses tty-port refcounting (e.g.
serial core) so the serdev devices might never be deregistered or
deallocated.

Thirdly, deregistering at tty-port destruction is too late as the
underlying device and structures may be long gone by then. A port is not
released before an open tty device is closed, something which a
registered serdev client can prevent from ever happening. A driver
callback while the device is gone typically also leads to crashes.

Many tty drivers even keep their ports around until the driver is
unloaded (e.g. serial core), something which even if a late callback
never happens, leads to leaks if a device is unbound from its driver and
is later rebound.

The right solution here is to add a new tty_port_unregister_device()
helper and to never call tty_device_unregister() whenever the port has
been claimed by serdev, but since this requires modifying just about
every tty driver (and multiple subsystems) it will need to be done
incrementally.

Reverting the offending patch is the first step in fixing the broken
lifetime assumptions. A follow-up patch will add a new pair of
tty-device registration helpers, which a vetted tty driver can use to
support serdev (initially serial core). When every tty driver uses the
serdev helpers (at least for deregistration), we can add serdev
registration to tty_port_register_device() again.

Note that this also fixes another issue with serdev, which currently
allocates and registers a serdev controller for every tty device
registered using tty_port_device_register() only to immediately
deregister and deallocate it when the corresponding OF node or serdev
child node is missing. This should be addressed before enabling serdev
for hot-pluggable buses.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/tty_port.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 1d21a9c1d33e..0c880f17d27e 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -16,7 +16,6 @@
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/module.h>
-#include <linux/serdev.h>
 
 static int tty_port_default_receive_buf(struct tty_port *port,
 					const unsigned char *p,
@@ -129,15 +128,7 @@ struct device *tty_port_register_device_attr(struct tty_port *port,
 		struct device *device, 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);
-	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,
 			attr_grp);
 }
@@ -189,9 +180,6 @@ static void tty_port_destructor(struct kref *kref)
 	/* check if last port ref was dropped before tty release */
 	if (WARN_ON(port->itty))
 		return;
-
-	serdev_tty_port_unregister(port);
-
 	if (port->xmit_buf)
 		free_page((unsigned long)port->xmit_buf);
 	tty_port_destroy(port);
-- 
2.12.2

Re: [PATCH 1/4] Revert "tty_port: register tty ports with serdev bus"

[Johan Hovold]

On Tue, Apr 11, 2017 at 08:53:32PM -0500, Rob Herring wrote:
> On Tue, Apr 11, 2017 at 12:07 PM, Johan Hovold <johan@kernel.org> wrote:
> > This reverts commit 8ee3fde047589dc9c201251f07d0ca1dc776feca.
> >
> > The new serdev bus hooked into the tty layer in
> > tty_port_register_device() by registering a serdev controller instead of
> > a tty device whenever a serdev client is present, and by deregistering
> > the controller in the tty-port destructor. This is broken in several
> > ways:
> 
> Just curious, how are you testing this? greybus?

By unbinding a platform device from its serial driver, and by using some
instrumentation code in USB serial.

> > Firstly, it leads to a NULL-pointer dereference whenever a tty driver
> > later deregisters its devices as no corresponding character device will
> > exist.
> >
> > Secondly, far from every tty driver uses tty-port refcounting (e.g.
> > serial core) so the serdev devices might never be deregistered or
> > deallocated.
> >
> > Thirdly, deregistering at tty-port destruction is too late as the
> > underlying device and structures may be long gone by then. A port is not
> > released before an open tty device is closed, something which a
> > registered serdev client can prevent from ever happening. A driver
> > callback while the device is gone typically also leads to crashes.
> >
> > Many tty drivers even keep their ports around until the driver is
> > unloaded (e.g. serial core), something which even if a late callback
> > never happens, leads to leaks if a device is unbound from its driver and
> > is later rebound.
> 
> Isn't this something we want to fix? i.e. everything done in
> probe/release rather than in init/exit.

It keeps some buffers allocated for a while longer than necessary, sure.
But there's quite a few drivers to change and for (non-hotpluggable)
serial drivers it doesn't make that much of difference as I assume
unbinding is mostly done for development purposes.

But we definitely need to make sure all drivers deregisters their
tty/serdev devices before starting to release resources.

> > The right solution here is to add a new tty_port_unregister_device()
> > helper and to never call tty_device_unregister() whenever the port has
> > been claimed by serdev, but since this requires modifying just about
> > every tty driver (and multiple subsystems) it will need to be done
> > incrementally.
> >
> > Reverting the offending patch is the first step in fixing the broken
> > lifetime assumptions. A follow-up patch will add a new pair of
> > tty-device registration helpers, which a vetted tty driver can use to
> > support serdev (initially serial core). When every tty driver uses the
> > serdev helpers (at least for deregistration), we can add serdev
> > registration to tty_port_register_device() again.
> 
> Reverting for 4.11 or not probably isn't that important. There aren't
> any users. I guess if you have a DT with a device added, then you
> could still have some problems.

True, but it only takes a DT child node (with a compatible string) to
trigger, and it's also the overhead added for all users of
tty_port_register_device() (e.g. cdc-acm) mentioned below. And we'd also
avoid enabling something in 4.11 which is then again disabled in 4.12
(for most tty drivers).

> > Note that this also fixes another issue with serdev, which currently
> > allocates and registers a serdev controller for every tty device
> > registered using tty_port_device_register() only to immediately
> > deregister and deallocate it when the corresponding OF node or serdev
> > child node is missing. This should be addressed before enabling serdev
> > for hot-pluggable buses.
> 
> Yeah, hot-plugging is definitely not supported ATM. Though folks are
> already asking about it. We need to figure out how to switch between a
> cdev and serdev.

Yeah, we need to give hotplugging some thought so we don't paint
ourselves into a corner here.

> Generally, the series looks good to me. Thanks for finding and fixing
> these issues. I'll give it a spin soon.

Thanks,
Johan

[PATCH 2/4] serdev: fix tty-port client deregistration

[Johan Hovold]

The port client data must be set when registering the serdev controller
or client deregistration will fail (and the serdev devices are left
registered and allocated) if the port was never opened in between.

Make sure to clear the port client data on any probe errors to avoid a
use-after-free when the client is later deregistered unconditionally
(e.g. in a tty-port deregistration helper).

Also move port client operation initialisation to registration. Note
that the client ops must be restored on failed probe.

Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serdev/serdev-ttyport.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index d05393594f15..06e17faa6dfb 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -101,9 +101,6 @@ static int ttyport_open(struct serdev_controller *ctrl)
 		return PTR_ERR(tty);
 	serport->tty = tty;
 
-	serport->port->client_ops = &client_ops;
-	serport->port->client_data = ctrl;
-
 	if (tty->ops->open)
 		tty->ops->open(serport->tty, NULL);
 	else
@@ -181,6 +178,7 @@ struct device *serdev_tty_port_register(struct tty_port *port,
 					struct device *parent,
 					struct tty_driver *drv, int idx)
 {
+	const struct tty_port_client_operations *old_ops;
 	struct serdev_controller *ctrl;
 	struct serport *serport;
 	int ret;
@@ -199,15 +197,22 @@ struct device *serdev_tty_port_register(struct tty_port *port,
 
 	ctrl->ops = &ctrl_ops;
 
+	old_ops = port->client_ops;
+	port->client_ops = &client_ops;
+	port->client_data = ctrl;
+
 	ret = serdev_controller_add(ctrl);
 	if (ret)
-		goto err_controller_put;
+		goto err_reset_data;
 
 	dev_info(&ctrl->dev, "tty port %s%d registered\n", drv->name, idx);
 	return &ctrl->dev;
 
-err_controller_put:
+err_reset_data:
+	port->client_data = NULL;
+	port->client_ops = old_ops;
 	serdev_controller_put(ctrl);
+
 	return ERR_PTR(ret);
 }
 
-- 
2.12.2

[PATCH 3/4] tty/serdev: add serdev registration interface

[Johan Hovold]

Add a new interface for registering a serdev controller and clients, and
a helper function to deregister serdev devices (or a tty device) that
were previously registered using the new interface.

Once every driver currently using the tty_port_register_device() helpers
have been vetted and converted to use the new serdev registration
interface (at least for deregistration), we can move serdev registration
to the current helpers and get rid of the serdev-specific functions.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serdev/serdev-ttyport.c |  6 ++-
 drivers/tty/tty_port.c              | 76 +++++++++++++++++++++++++++++++++++++
 include/linux/serdev.h              |  7 +++-
 include/linux/tty.h                 |  9 +++++
 4 files changed, 94 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index 06e17faa6dfb..8f9d7a0c27d1 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -216,16 +216,18 @@ struct device *serdev_tty_port_register(struct tty_port *port,
 	return ERR_PTR(ret);
 }
 
-void serdev_tty_port_unregister(struct tty_port *port)
+int serdev_tty_port_unregister(struct tty_port *port)
 {
 	struct serdev_controller *ctrl = port->client_data;
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
 
 	if (!serport)
-		return;
+		return -ENODEV;
 
 	serdev_controller_remove(ctrl);
 	port->client_ops = NULL;
 	port->client_data = NULL;
 	serdev_controller_put(ctrl);
+
+	return 0;
 }
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 0c880f17d27e..2b4c62189346 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -16,6 +16,7 @@
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/serdev.h>
 
 static int tty_port_default_receive_buf(struct tty_port *port,
 					const unsigned char *p,
@@ -134,6 +135,81 @@ struct device *tty_port_register_device_attr(struct tty_port *port,
 }
 EXPORT_SYMBOL_GPL(tty_port_register_device_attr);
 
+/**
+ * tty_port_register_device_attr_serdev - register tty or serdev device
+ * @port: tty_port of the device
+ * @driver: tty_driver for this device
+ * @index: index of the tty
+ * @device: parent if exists, otherwise NULL
+ * @drvdata: driver data for the device
+ * @attr_grp: attribute group for the device
+ *
+ * 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_attr_serdev(struct tty_port *port,
+		struct tty_driver *driver, unsigned index,
+		struct device *device, 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);
+	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,
+			attr_grp);
+}
+EXPORT_SYMBOL_GPL(tty_port_register_device_attr_serdev);
+
+/**
+ * tty_port_register_device_serdev - register tty or serdev device
+ * @port: tty_port of the device
+ * @driver: tty_driver for this device
+ * @index: index of the tty
+ * @device: 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)
+{
+	return tty_port_register_device_attr_serdev(port, driver, index,
+			device, NULL, NULL);
+}
+EXPORT_SYMBOL_GPL(tty_port_register_device_serdev);
+
+
+/**
+ * tty_port_unregister_device - deregister a tty or serdev device
+ * @port: tty_port of the device
+ * @driver: tty_driver for this device
+ * @index: index of the tty
+ *
+ * If a tty or serdev device is registered with a call to
+ * tty_port_register_device_serdev() then this function must be called when
+ * the device is gone.
+ */
+void tty_port_unregister_device(struct tty_port *port,
+		struct tty_driver *driver, unsigned index)
+{
+	int ret;
+
+	ret = serdev_tty_port_unregister(port);
+	if (ret == 0)
+		return;
+
+	tty_unregister_device(driver, index);
+}
+EXPORT_SYMBOL_GPL(tty_port_unregister_device);
+
 int tty_port_alloc_xmit_buf(struct tty_port *port)
 {
 	/* We may sleep in get_zeroed_page() */
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 9519da6253a8..d612953c63c2 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -248,7 +248,7 @@ struct tty_driver;
 struct device *serdev_tty_port_register(struct tty_port *port,
 					struct device *parent,
 					struct tty_driver *drv, int idx);
-void serdev_tty_port_unregister(struct tty_port *port);
+int serdev_tty_port_unregister(struct tty_port *port);
 #else
 static inline struct device *serdev_tty_port_register(struct tty_port *port,
 					   struct device *parent,
@@ -256,7 +256,10 @@ static inline struct device *serdev_tty_port_register(struct tty_port *port,
 {
 	return ERR_PTR(-ENODEV);
 }
-static inline void serdev_tty_port_unregister(struct tty_port *port) {}
+static inline int serdev_tty_port_unregister(struct tty_port *port)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_SERIAL_DEV_CTRL_TTYPORT */
 
 #endif /*_LINUX_SERDEV_H */
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1017e904c0a3..0a3eeca4761a 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -555,6 +555,15 @@ extern struct device *tty_port_register_device_attr(struct tty_port *port,
 		struct tty_driver *driver, unsigned index,
 		struct device *device, void *drvdata,
 		const struct attribute_group **attr_grp);
+extern struct device *tty_port_register_device_serdev(struct tty_port *port,
+		struct tty_driver *driver, unsigned index,
+		struct device *device);
+extern struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
+		struct tty_driver *driver, unsigned index,
+		struct device *device, void *drvdata,
+		const struct attribute_group **attr_grp);
+extern void tty_port_unregister_device(struct tty_port *port,
+		struct tty_driver *driver, unsigned index);
 extern int tty_port_alloc_xmit_buf(struct tty_port *port);
 extern void tty_port_free_xmit_buf(struct tty_port *port);
 extern void tty_port_destroy(struct tty_port *port);
-- 
2.12.2

Re: [PATCH 3/4] tty/serdev: add serdev registration interface

[Rob Herring]

On Tue, Apr 11, 2017 at 12:07 PM, Johan Hovold <johan@kernel.org> wrote:
> Add a new interface for registering a serdev controller and clients, and
> a helper function to deregister serdev devices (or a tty device) that
> were previously registered using the new interface.
>
> Once every driver currently using the tty_port_register_device() helpers
> have been vetted and converted to use the new serdev registration
> interface (at least for deregistration), we can move serdev registration
> to the current helpers and get rid of the serdev-specific functions.

I don't really think this is necessary. While in theory any tty port
can work with serdev, the reality is it only ever going to be used
with a few. There's about 31 possible drivers. Of those, there's a
fair number that an attached device is not going to make sense (e.g.
ISDN CAPI, rfcomm?, goldfish, etc.). Second, right now serdev only
works with DT binding. There are only 3 drivers supporting DT:
serial_core, goldfish, and ehv_bytechan. Likely drivers I'm aware of
to use serdev in addition to serial_core are USB serial and greybus.
But for those, we currently only could support them if the whole bus
topology is described in DT. Otherwise, we first have to figure out
how to apply DT overlays to arbitrary devices not described in DT.

OTOH, we are at least explicit with what tty ports support serdev.

Rob

Re: [PATCH 3/4] tty/serdev: add serdev registration interface

[Johan Hovold]

On Thu, Apr 20, 2017 at 12:52:50PM -0500, Rob Herring wrote:
> On Tue, Apr 11, 2017 at 12:07 PM, Johan Hovold <johan@kernel.org> wrote:
> > Add a new interface for registering a serdev controller and clients, and
> > a helper function to deregister serdev devices (or a tty device) that
> > were previously registered using the new interface.
> >
> > Once every driver currently using the tty_port_register_device() helpers
> > have been vetted and converted to use the new serdev registration
> > interface (at least for deregistration), we can move serdev registration
> > to the current helpers and get rid of the serdev-specific functions.
> 
> I don't really think this is necessary. While in theory any tty port
> can work with serdev, the reality is it only ever going to be used
> with a few. There's about 31 possible drivers. Of those, there's a
> fair number that an attached device is not going to make sense (e.g.
> ISDN CAPI, rfcomm?, goldfish, etc.). Second, right now serdev only
> works with DT binding. There are only 3 drivers supporting DT:
> serial_core, goldfish, and ehv_bytechan.

There are also about ten PCI-based drivers (e.g. rocket.c), which, if
I'm not mistaken, could have an associated DT-node already today. And so
could the SPI-based ifx6x60 driver (modem).

> Likely drivers I'm aware of to use serdev in addition to serial_core
> are USB serial and greybus.  But for those, we currently only could
> support them if the whole bus topology is described in DT. Otherwise,
> we first have to figure out how to apply DT overlays to arbitrary
> devices not described in DT.

There's also cdc-acm and possibly fw-serial (in staging).

And while our current USB device-tree implementation is limited to
describing USB devices, extending this to interfaces, which our USB
drivers bind to, should be easily done (I'm looking into it now).

There's also a comment in serdev about adding support for "ACPI and
platform" descriptions, and you mentioned being able to switch between
cdev and serdev as a possible future extension.

The point is that serdev currently hooks into the tty layer through a
generic function, and if and how a client can be described is just
an implementation detail. Rather than using such a large hammer, it
seems to me that enabling serdev on a per-driver basis after making sure
that nothing breaks (e.g. resources are released on deregistration) is
preferred.

> OTOH, we are at least explicit with what tty ports support serdev.

That would be another benefit.

Johan

Re: [PATCH 3/4] tty/serdev: add serdev registration interface

[Greg Kroah-Hartman]

On Tue, Apr 11, 2017 at 07:07:30PM +0200, Johan Hovold wrote:
> Add a new interface for registering a serdev controller and clients, and
> a helper function to deregister serdev devices (or a tty device) that
> were previously registered using the new interface.
> 
> Once every driver currently using the tty_port_register_device() helpers
> have been vetted and converted to use the new serdev registration
> interface (at least for deregistration), we can move serdev registration
> to the current helpers and get rid of the serdev-specific functions.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  drivers/tty/serdev/serdev-ttyport.c |  6 ++-
>  drivers/tty/tty_port.c              | 76 +++++++++++++++++++++++++++++++++++++
>  include/linux/serdev.h              |  7 +++-
>  include/linux/tty.h                 |  9 +++++
>  4 files changed, 94 insertions(+), 4 deletions(-)

This patch doesn't apply, and should it really be a 4.12-final patch?

I've dropped this one, and the 4th as well, from my queue.

thanks,

greg k-h

Re: [PATCH 3/4] tty/serdev: add serdev registration interface

[Johan Hovold]

On Thu, May 18, 2017 at 04:43:42PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 11, 2017 at 07:07:30PM +0200, Johan Hovold wrote:
> > Add a new interface for registering a serdev controller and clients, and
> > a helper function to deregister serdev devices (or a tty device) that
> > were previously registered using the new interface.
> > 
> > Once every driver currently using the tty_port_register_device() helpers
> > have been vetted and converted to use the new serdev registration
> > interface (at least for deregistration), we can move serdev registration
> > to the current helpers and get rid of the serdev-specific functions.
> > 
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> >  drivers/tty/serdev/serdev-ttyport.c |  6 ++-
> >  drivers/tty/tty_port.c              | 76 +++++++++++++++++++++++++++++++++++++
> >  include/linux/serdev.h              |  7 +++-
> >  include/linux/tty.h                 |  9 +++++
> >  4 files changed, 94 insertions(+), 4 deletions(-)
> 
> This patch doesn't apply

Probably because it was generated against v4.11-rc which did not have
the write_buf wrapper which you just reverted (after which the patch
seems to apply just fine again).

I'll submit a rebased v2 of the two remaining patches anyway.

> and should it really be a 4.12-final patch?

I think so. serdev was enabled in 4.11 by hooking into the tty layer at
the wrong place, which is what this series addresses. I had hoped to get
at least the revert-patch into 4.11-final, and the rest into 4.12, to
avoid disabling something which had once been enabled.

The remaining two patches only re-enables serdev for a subset of tty
drivers (for which it is safe to do so) by using the new tty-port
registration functions in serial-core only.

In effect we'd up with serdev disabled in 4.11 and enabled for
serial-core in 4.12 (which is the first release that has driver using
serdev).

Does that make sense?

Thanks,
Johan

Re: [PATCH 3/4] tty/serdev: add serdev registration interface

[Greg Kroah-Hartman]

On Thu, May 18, 2017 at 05:31:01PM +0200, Johan Hovold wrote:
> On Thu, May 18, 2017 at 04:43:42PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 11, 2017 at 07:07:30PM +0200, Johan Hovold wrote:
> > > Add a new interface for registering a serdev controller and clients, and
> > > a helper function to deregister serdev devices (or a tty device) that
> > > were previously registered using the new interface.
> > > 
> > > Once every driver currently using the tty_port_register_device() helpers
> > > have been vetted and converted to use the new serdev registration
> > > interface (at least for deregistration), we can move serdev registration
> > > to the current helpers and get rid of the serdev-specific functions.
> > > 
> > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > ---
> > >  drivers/tty/serdev/serdev-ttyport.c |  6 ++-
> > >  drivers/tty/tty_port.c              | 76 +++++++++++++++++++++++++++++++++++++
> > >  include/linux/serdev.h              |  7 +++-
> > >  include/linux/tty.h                 |  9 +++++
> > >  4 files changed, 94 insertions(+), 4 deletions(-)
> > 
> > This patch doesn't apply
> 
> Probably because it was generated against v4.11-rc which did not have
> the write_buf wrapper which you just reverted (after which the patch
> seems to apply just fine again).
> 
> I'll submit a rebased v2 of the two remaining patches anyway.
> 
> > and should it really be a 4.12-final patch?
> 
> I think so. serdev was enabled in 4.11 by hooking into the tty layer at
> the wrong place, which is what this series addresses. I had hoped to get
> at least the revert-patch into 4.11-final, and the rest into 4.12, to
> avoid disabling something which had once been enabled.
> 
> The remaining two patches only re-enables serdev for a subset of tty
> drivers (for which it is safe to do so) by using the new tty-port
> registration functions in serial-core only.
> 
> In effect we'd up with serdev disabled in 4.11 and enabled for
> serial-core in 4.12 (which is the first release that has driver using
> serdev).
> 
> Does that make sense?

Ok, fair enough, now queued up, thanks.

greg k-h

[PATCH 4/4] serial: enable serdev support

[Johan Hovold]

Enable serdev support by using the new device-registration helpers.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 3fe56894974a..9f5eb0ced359 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2785,7 +2785,7 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 	 * Register the port whether it's detected or not.  This allows
 	 * setserial to be used to alter this port's parameters.
 	 */
-	tty_dev = tty_port_register_device_attr(port, drv->tty_driver,
+	tty_dev = tty_port_register_device_attr_serdev(port, drv->tty_driver,
 			uport->line, uport->dev, port, uport->tty_groups);
 	if (likely(!IS_ERR(tty_dev))) {
 		device_set_wakeup_capable(tty_dev, 1);
@@ -2848,7 +2848,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
 	/*
 	 * Remove the devices from the tty layer
 	 */
-	tty_unregister_device(drv->tty_driver, uport->line);
+	tty_port_unregister_device(port, drv->tty_driver, uport->line);
 
 	tty = tty_port_tty_get(port);
 	if (tty) {
-- 
2.12.2

Re: [PATCH 0/4] serdev: fix broken lifetime assumptions

[Johan Hovold]

On Tue, Apr 11, 2017 at 07:07:27PM +0200, Johan Hovold wrote:
> This series fixes a number of issues with the new serdev code, which was
> based on incorrect tty-port lifetime assumptions.
> 
> The first patch disables serdev support by reverting the patch which
> hooked into the tty layer in a broken way that leads to crashes and
> leaks when deregistering devices. This one should probably go into 4.11.
> 
> The second patch fixes a specific bug in the tty-port client
> registration code, while the third patch adds a new interface for
> registering serdev devices. The final patch ultimately enables serdev
> again for the serial drivers.
> 
> More details can be found in the individual commit messages.
> 
> Johan
> 
> 
> Johan Hovold (4):
>   Revert "tty_port: register tty ports with serdev bus"
>   serdev: fix tty-port client deregistration
>   tty/serdev: add serdev registration interface
>   serial: enable serdev support

Rob, did you have any further comments to this series?

Greg, I think these should go into 4.12-rc2, and the first patch should
probably also have a stable tag now as serdev resource-management is
clearly broken in 4.11.

Johan

Re: [PATCH 0/4] serdev: fix broken lifetime assumptions

[Rob Herring]

On Wed, May 17, 2017 at 5:39 AM, Johan Hovold <johan@kernel.org> wrote:
> On Tue, Apr 11, 2017 at 07:07:27PM +0200, Johan Hovold wrote:
>> This series fixes a number of issues with the new serdev code, which was
>> based on incorrect tty-port lifetime assumptions.
>>
>> The first patch disables serdev support by reverting the patch which
>> hooked into the tty layer in a broken way that leads to crashes and
>> leaks when deregistering devices. This one should probably go into 4.11.
>>
>> The second patch fixes a specific bug in the tty-port client
>> registration code, while the third patch adds a new interface for
>> registering serdev devices. The final patch ultimately enables serdev
>> again for the serial drivers.
>>
>> More details can be found in the individual commit messages.
>>
>> Johan
>>
>>
>> Johan Hovold (4):
>>   Revert "tty_port: register tty ports with serdev bus"
>>   serdev: fix tty-port client deregistration
>>   tty/serdev: add serdev registration interface
>>   serial: enable serdev support
>
> Rob, did you have any further comments to this series?

No. For the series,

Reviewed-by: Rob Herring <robh@kernel.org>

> Greg, I think these should go into 4.12-rc2, and the first patch should
> probably also have a stable tag now as serdev resource-management is
> clearly broken in 4.11.
>
> Johan