[PATCH] Revert "tty: fix NULL pointer issue when tty_port ops is not set"

[PATCH] Revert "tty: fix NULL pointer issue when tty_port ops is not set"

[Johan Hovold]

This reverts commit f4e68d58cf2b20a581759bbc7228052534652673.

TTY drivers using the tty-port abstraction all provide a pointer to a
set of port operations, which specifically cannot be NULL (or we'd find
out at first attempt to open a port).

Revert the recent commit which added unnecessary NULL-checks and whose
commit message indicated that it was fixing a real problem, which it did
not.

Note that even the two tty drivers for virtual devices currently
providing an empty set of operations probably should be implementing at
least some of the callbacks.

Cc: Fabien Dessenne <fabien.dessenne@st.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---

This reverts a commit that got into -rc3 despite not really fixing
anything.

Johan




 drivers/tty/tty_port.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index a9e12b3bc31d..044c3cbdcfa4 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -325,7 +325,7 @@ static void tty_port_shutdown(struct tty_port *port, struct tty_struct *tty)
 		if (tty && C_HUPCL(tty))
 			tty_port_lower_dtr_rts(port);
 
-		if (port->ops && port->ops->shutdown)
+		if (port->ops->shutdown)
 			port->ops->shutdown(port);
 	}
 out:
@@ -398,7 +398,7 @@ EXPORT_SYMBOL_GPL(tty_port_tty_wakeup);
  */
 int tty_port_carrier_raised(struct tty_port *port)
 {
-	if (!port->ops || !port->ops->carrier_raised)
+	if (port->ops->carrier_raised == NULL)
 		return 1;
 	return port->ops->carrier_raised(port);
 }
@@ -414,7 +414,7 @@ EXPORT_SYMBOL(tty_port_carrier_raised);
  */
 void tty_port_raise_dtr_rts(struct tty_port *port)
 {
-	if (port->ops && port->ops->dtr_rts)
+	if (port->ops->dtr_rts)
 		port->ops->dtr_rts(port, 1);
 }
 EXPORT_SYMBOL(tty_port_raise_dtr_rts);
@@ -429,7 +429,7 @@ EXPORT_SYMBOL(tty_port_raise_dtr_rts);
  */
 void tty_port_lower_dtr_rts(struct tty_port *port)
 {
-	if (port->ops && port->ops->dtr_rts)
+	if (port->ops->dtr_rts)
 		port->ops->dtr_rts(port, 0);
 }
 EXPORT_SYMBOL(tty_port_lower_dtr_rts);
@@ -684,7 +684,7 @@ int tty_port_open(struct tty_port *port, struct tty_struct *tty,
 
 	if (!tty_port_initialized(port)) {
 		clear_bit(TTY_IO_ERROR, &tty->flags);
-		if (port->ops && port->ops->activate) {
+		if (port->ops->activate) {
 			int retval = port->ops->activate(port, tty);
 			if (retval) {
 				mutex_unlock(&port->mutex);
-- 
2.21.0

Re: [PATCH] Revert "tty: fix NULL pointer issue when tty_port ops is not set"

[Greg Kroah-Hartman]

On Wed, Apr 03, 2019 at 09:40:53AM +0200, Johan Hovold wrote:
> This reverts commit f4e68d58cf2b20a581759bbc7228052534652673.
> 
> TTY drivers using the tty-port abstraction all provide a pointer to a
> set of port operations, which specifically cannot be NULL (or we'd find
> out at first attempt to open a port).
> 
> Revert the recent commit which added unnecessary NULL-checks and whose
> commit message indicated that it was fixing a real problem, which it did
> not.
> 
> Note that even the two tty drivers for virtual devices currently
> providing an empty set of operations probably should be implementing at
> least some of the callbacks.

This was a "future fix" for a driver that is under review for 5.2, see
the email thread:
	Subject: [PATCH 0/2] TTY: add rpmsg tty driver

It didn't want/need the port pointer, if that is incorrect, that's fine,
I'll gladly revert this, but as-is this patch isn't hurting anything,
right?

thanks,

greg k-h

Re: [PATCH] Revert "tty: fix NULL pointer issue when tty_port ops is not set"

[Johan Hovold]

On Wed, Apr 03, 2019 at 10:06:49AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 03, 2019 at 09:40:53AM +0200, Johan Hovold wrote:
> > This reverts commit f4e68d58cf2b20a581759bbc7228052534652673.
> > 
> > TTY drivers using the tty-port abstraction all provide a pointer to a
> > set of port operations, which specifically cannot be NULL (or we'd find
> > out at first attempt to open a port).
> > 
> > Revert the recent commit which added unnecessary NULL-checks and whose
> > commit message indicated that it was fixing a real problem, which it did
> > not.
> > 
> > Note that even the two tty drivers for virtual devices currently
> > providing an empty set of operations probably should be implementing at
> > least some of the callbacks.
> 
> This was a "future fix" for a driver that is under review for 5.2, see
> the email thread:
> 	Subject: [PATCH 0/2] TTY: add rpmsg tty driver
>
> It didn't want/need the port pointer, if that is incorrect, that's fine,
> I'll gladly revert this, but as-is this patch isn't hurting anything,
> right?

No, it only adds the unnecessary NULL checks and some confusion by
claiming to "fix an issue".

The rpmsg tty driver not having to do anything at activate/shutdown
(first open, final close) looks a bit suspicious, but in any case I
think it's the outliers that should continue providing an empty set of
callbacks instead of sprinkling conditionals in core.

Johan