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

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

[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.

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

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index 302018d67efa..404f3fd070a7 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -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

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

[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.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serdev/serdev-ttyport.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index 404f3fd070a7..5b09ce920117 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -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)
-- 
2.14.2

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

[Rob Herring]

On Mon, Oct 16, 2017 at 8:06 AM, Johan Hovold <johan@kernel.org> wrote:
> 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

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

[Johan Hovold]

On Tue, Oct 17, 2017 at 11:08:53AM -0500, Rob Herring wrote:
> On Mon, Oct 16, 2017 at 8:06 AM, Johan Hovold <johan@kernel.org> wrote:
> > 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

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

[Rob Herring]

On Mon, Oct 16, 2017 at 8:06 AM, Johan Hovold <johan@kernel.org> wrote:
> 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

> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/tty/serdev/serdev-ttyport.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index 302018d67efa..404f3fd070a7 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c
> @@ -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
>

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

[Greg Kroah-Hartman]

On Tue, Oct 17, 2017 at 11:07:20AM -0500, Rob Herring wrote:
> On Mon, Oct 16, 2017 at 8:06 AM, Johan Hovold <johan@kernel.org> wrote:
> > 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

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

[Rob Herring]

On Fri, Oct 20, 2017 at 7:21 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 17, 2017 at 11:07:20AM -0500, Rob Herring wrote:
>> On Mon, Oct 16, 2017 at 8:06 AM, Johan Hovold <johan@kernel.org> wrote:
>> > 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?

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 <robh@kernel.org>

Rob