serdev: ttyport: enforce tty-driver open() requirement

PATCH In review Johan Hovold 3 participants 7 messages Oct 16, 2017 – Oct 20, 2017

Subsystems (2)

TTY layer and serial drivers Serial device bus

lore

Share & tools

lore

Johan Hovold

author

2017-10-16 13:06 UTC #0 lore raw

PATCH 1/2 serdev: ttyport: enforce tty-driver open() requirement

The tty-driver open routine is mandatory, but the serdev tty-port-controller implementation did not treat it as such and would instead fall back to calling tty_port_open() directly.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serdev/serdev-ttyport.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

drivers/tty/serdev/serdev-ttyport.c +10 -4

index 302018d67efa..404f3fd070a7 100644
@@ -102,10 +102,10 @@ static int ttyport_open(struct serdev_controller *ctrl)
 		return PTR_ERR(tty);
 	serport->tty = tty;
 
-	if (tty->ops->open)
-		tty->ops->open(serport->tty, NULL);
-	else
-		tty_port_open(serport->port, tty, NULL);
+	if (!tty->ops->open)
+		goto err_unlock;
+
+	tty->ops->open(serport->tty, NULL);
 
 	/* Bring the UART into a known 8 bits no parity hw fc state */
 	ktermios = tty->termios;

@@ -122,6 +122,12 @@ static int ttyport_open(struct serdev_controller *ctrl)
 
 	tty_unlock(serport->tty);
 	return 0;
+
+err_unlock:
+	tty_unlock(tty);
+	tty_release_struct(tty, serport->tty_idx);
+
+	return -ENODEV;
 }
 
 static void ttyport_close(struct serdev_controller *ctrl)

Signature · 1 line

2.14.2

Johan Hovold

author

2017-10-16 13:06 UTC #1 lore raw

PATCH 2/2 serdev: ttyport: add missing open() error handling

Add missing error handling for tty-driver open() which may fail (e.g. if resource allocation fails or if a port is being disconnected).

Note that close() must be called also in case of failed open() and that the operation sanity check is amended to catch buggy drivers.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serdev/serdev-ttyport.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

drivers/tty/serdev/serdev-ttyport.c +10 -3

index 404f3fd070a7..5b09ce920117 100644
@@ -96,16 +96,21 @@ static int ttyport_open(struct serdev_controller *ctrl)
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
 	struct tty_struct *tty;
 	struct ktermios ktermios;
+	int ret;
 
 	tty = tty_init_dev(serport->tty_drv, serport->tty_idx);
 	if (IS_ERR(tty))
 		return PTR_ERR(tty);
 	serport->tty = tty;
 
-	if (!tty->ops->open)
+	if (!tty->ops->open || !tty->ops->close) {
+		ret = -ENODEV;
 		goto err_unlock;
+	}
 
-	tty->ops->open(serport->tty, NULL);
+	ret = tty->ops->open(serport->tty, NULL);
+	if (ret)
+		goto err_close;
 
 	/* Bring the UART into a known 8 bits no parity hw fc state */
 	ktermios = tty->termios;

@@ -123,11 +128,13 @@ static int ttyport_open(struct serdev_controller *ctrl)
 	tty_unlock(serport->tty);
 	return 0;
 
+err_close:
+	tty->ops->close(tty, NULL);
 err_unlock:
 	tty_unlock(tty);
 	tty_release_struct(tty, serport->tty_idx);
 
-	return -ENODEV;
+	return ret;
 }
 
 static void ttyport_close(struct serdev_controller *ctrl)

Signature · 1 line

2.14.2

Rob Herring

2017-10-17 16:08 UTC #2 lore raw

Re: [PATCH 2/2] serdev: ttyport: add missing open() error handling

Quoted 5 lines · Johan Hovold

Add missing error handling for tty-driver open() which may fail (e.g. if resource allocation fails or if a port is being disconnected).
Note that close() must be called also in case of failed open() and that the operation sanity check is amended to catch buggy drivers.

That's a very odd pattern. Wouldn't a must_check annotation be enough?

Rob

Johan Hovold

author

2017-10-18 07:01 UTC #3 lore raw

Re: [PATCH 2/2] serdev: ttyport: add missing open() error handling

Quoted 8 lines · Rob Herring

Quoted 5 lines · Johan Hovold
Add missing error handling for tty-driver open() which may fail (e.g. if resource allocation fails or if a port is being disconnected).
Note that close() must be called also in case of failed open() and that the operation sanity check is amended to catch buggy drivers.
That's a very odd pattern. Wouldn't a must_check annotation be enough?

An odd pattern indeed, but that's how the TTY drivers are implemented and the interface documented (i.e. that close() will be called also after failed open()).

Not sure how a must_check could help here.

Johan

Rob Herring

2017-10-17 16:07 UTC #4 lore raw

In reply to Johan Hovold

Quoted 3 lines · Johan Hovold

The tty-driver open routine is mandatory, but the serdev tty-port-controller implementation did not treat it as such and would instead fall back to calling tty_port_open() directly.

The idea was to eventually get rid of the tty_struct dependency and only depend on tty_port. That's very invasive though and needs various pieces of tty_struct to move into tty_port.

Of course, tty_port_open itself would have to change as well, so this change doesn't really matter.

Rob

Quoted 40 lines

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serdev/serdev-ttyport.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

drivers/tty/serdev/serdev-ttyport.c +10 -5

index 302018d67efa..404f3fd070a7 100644
@@ -102,10 +102,10 @@ static int ttyport_open(struct serdev_controller *ctrl)
                return PTR_ERR(tty);
        serport->tty = tty;
 
-       if (tty->ops->open)
-               tty->ops->open(serport->tty, NULL);
-       else
-               tty_port_open(serport->port, tty, NULL);
+       if (!tty->ops->open)
+               goto err_unlock;
+
+       tty->ops->open(serport->tty, NULL);
 
        /* Bring the UART into a known 8 bits no parity hw fc state */
        ktermios = tty->termios;

@@ -122,6 +122,12 @@ static int ttyport_open(struct serdev_controller *ctrl)
 
        tty_unlock(serport->tty);
        return 0;
+
+err_unlock:
+       tty_unlock(tty);
+       tty_release_struct(tty, serport->tty_idx);
+
+       return -ENODEV;
 }
 
 static void ttyport_close(struct serdev_controller *ctrl)
--

2.14.2

Greg Kroah-Hartman

maintainer

2017-10-20 12:21 UTC #5 lore raw

Quoted 11 lines · Rob Herring

Quoted 3 lines · Johan Hovold
The tty-driver open routine is mandatory, but the serdev tty-port-controller implementation did not treat it as such and would instead fall back to calling tty_port_open() directly.
The idea was to eventually get rid of the tty_struct dependency and only depend on tty_port. That's very invasive though and needs various pieces of tty_struct to move into tty_port.
Of course, tty_port_open itself would have to change as well, so this change doesn't really matter.

So are you acking these patches?

confused,

greg k-h

Rob Herring

Acked-by

2017-10-20 21:43 UTC #6 lore raw

Quoted 6 lines · Greg Kroah-Hartman

Quoted 5 lines · Rob Herring
Quoted 3 lines · Johan Hovold
The tty-driver open routine is mandatory, but the serdev tty-port-controller implementation did not treat it as such and would instead fall back to calling tty_port_open() directly.

Quoted 8 lines

Quoted 6 lines
The idea was to eventually get rid of the tty_struct dependency and only depend on tty_port. That's very invasive though and needs various pieces of tty_struct to move into tty_port.
Of course, tty_port_open itself would have to change as well, so this change doesn't really matter.
So are you acking these patches?

Yeah, I guess. I was mainly trying to give some background on why it was done the way it was.

For both:

Acked-by Rob Herring <[email protected]>

Rob