There are serial_in/out() helpers to be used instead of direct
p->serial_in/out(). Use them in various 8250 drivers.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c   | 17 +++++++++--------
 drivers/tty/serial/8250/8250_fsl.c  |  8 ++++----
 drivers/tty/serial/8250/8250_omap.c |  2 +-
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index af24ec25d976..d0abbdd630ac 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -120,16 +120,17 @@ static void dw8250_force_idle(struct uart_port *p)
 	 * enabled.
 	 */
 	if (up->fcr & UART_FCR_ENABLE_FIFO) {
-		lsr = p->serial_in(p, UART_LSR);
+		lsr = serial_in(up, UART_LSR);
 		if (!(lsr & UART_LSR_DR))
 			return;
 	}
 
-	(void)p->serial_in(p, UART_RX);
+	serial_in(up, UART_RX);
 }
 
 static void dw8250_check_lcr(struct uart_port *p, int offset, int value)
 {
+	struct uart_8250_port *up = up_to_u8250p(p);
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
 	void __iomem *addr = p->membase + (offset << p->regshift);
 	int tries = 1000;
@@ -139,7 +140,7 @@ static void dw8250_check_lcr(struct uart_port *p, int offset, int value)
 
 	/* Make sure LCR write wasn't ignored */
 	while (tries--) {
-		unsigned int lcr = p->serial_in(p, offset);
+		unsigned int lcr = serial_in(up, offset);
 
 		if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
 			return;
@@ -260,7 +261,7 @@ static int dw8250_handle_irq(struct uart_port *p)
 {
 	struct uart_8250_port *up = up_to_u8250p(p);
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
-	unsigned int iir = p->serial_in(p, UART_IIR);
+	unsigned int iir = serial_in(up, UART_IIR);
 	bool rx_timeout = (iir & 0x3f) == UART_IIR_RX_TIMEOUT;
 	unsigned int quirks = d->pdata->quirks;
 	unsigned int status;
@@ -281,7 +282,7 @@ static int dw8250_handle_irq(struct uart_port *p)
 		status = serial_lsr_in(up);
 
 		if (!(status & (UART_LSR_DR | UART_LSR_BI)))
-			(void) p->serial_in(p, UART_RX);
+			serial_in(up, UART_RX);
 
 		uart_port_unlock_irqrestore(p, flags);
 	}
@@ -303,7 +304,7 @@ static int dw8250_handle_irq(struct uart_port *p)
 
 	if ((iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
 		/* Clear the USR */
-		(void)p->serial_in(p, d->pdata->usr_reg);
+		serial_in(up, d->pdata->usr_reg);
 
 		return 1;
 	}
@@ -390,7 +391,7 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
 static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios)
 {
 	struct uart_8250_port *up = up_to_u8250p(p);
-	unsigned int mcr = p->serial_in(p, UART_MCR);
+	unsigned int mcr = serial_in(up, UART_MCR);
 
 	if (up->capabilities & UART_CAP_IRDA) {
 		if (termios->c_line == N_IRDA)
@@ -398,7 +399,7 @@ static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios)
 		else
 			mcr &= ~DW_UART_MCR_SIRE;
 
-		p->serial_out(p, UART_MCR, mcr);
+		serial_out(up, UART_MCR, mcr);
 	}
 	serial8250_do_set_ldisc(p, termios);
 }
diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
index 1b7bd55619c6..df8f846f3d54 100644
--- a/drivers/tty/serial/8250/8250_fsl.c
+++ b/drivers/tty/serial/8250/8250_fsl.c
@@ -32,7 +32,7 @@ int fsl8250_handle_irq(struct uart_port *port)
 
 	uart_port_lock_irqsave(&up->port, &flags);
 
-	iir = port->serial_in(port, UART_IIR);
+	iir = serial_in(up, UART_IIR);
 	if (iir & UART_IIR_NO_INT) {
 		uart_port_unlock_irqrestore(&up->port, flags);
 		return 0;
@@ -54,12 +54,12 @@ int fsl8250_handle_irq(struct uart_port *port)
 	if (unlikely((iir & UART_IIR_ID) == UART_IIR_RLSI &&
 		     (up->lsr_saved_flags & UART_LSR_BI))) {
 		up->lsr_saved_flags &= ~UART_LSR_BI;
-		port->serial_in(port, UART_RX);
+		serial_in(up, UART_RX);
 		uart_port_unlock_irqrestore(&up->port, flags);
 		return 1;
 	}
 
-	lsr = orig_lsr = up->port.serial_in(&up->port, UART_LSR);
+	lsr = orig_lsr = serial_in(up, UART_LSR);
 
 	/* Process incoming characters first */
 	if ((lsr & (UART_LSR_DR | UART_LSR_BI)) &&
@@ -71,7 +71,7 @@ int fsl8250_handle_irq(struct uart_port *port)
 	if ((orig_lsr & UART_LSR_OE) && (up->overrun_backoff_time_ms > 0)) {
 		unsigned long delay;
 
-		up->ier = port->serial_in(port, UART_IER);
+		up->ier = serial_in(up, UART_IER);
 		if (up->ier & (UART_IER_RLSI | UART_IER_RDI)) {
 			port->ops->stop_rx(port);
 		} else {
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index c2b75e3f106d..16610782258e 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -692,7 +692,7 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 
 		/* Synchronize UART_IER access against the console. */
 		uart_port_lock(port);
-		up->ier = port->serial_in(port, UART_IER);
+		up->ier = serial_in(up, UART_IER);
 		if (up->ier & (UART_IER_RLSI | UART_IER_RDI)) {
 			port->ops->stop_rx(port);
 		} else {
-- 
2.48.1

On Thu, Feb 20, 2025 at 12:16:03PM +0100, Jiri Slaby (SUSE) wrote:
> There are serial_in/out() helpers to be used instead of direct
> p->serial_in/out(). Use them in various 8250 drivers.

Okay, I have checked all calls, and I think to avoid possible issues
we need to add a few comments here and there, see below.

But first of all, can we actually use serial_port_in()/serial_port_out()
in the cases where we have already a pointer to uart_port?

...

Here we should add a comment like

/*
 * This function is being called as part of the uart_port::serial_out()
 * routine. Hence it must not call serial_out() on itself against
 * the modified registers here, i.e. LCR.
 */

> static void dw8250_force_idle(struct uart_port *p)
> {
> 	struct uart_8250_port *up = up_to_u8250p(p);
> 	unsigned int lsr;

Here we should add a comment like

	/*
	 * The following call currently performs serial_out()
	 * against the FCR register. Because it differs to LCR
	 * there will be no dead loop, but if it ever gets
	 * modified, we might need a new custom version that
	 * avoids infinite recursion.
	 */

> 	serial8250_clear_and_reinit_fifos(up);

...

On top of this function we need to add the same comment as on top of
dw8250_force_idle() above.

>  static void dw8250_check_lcr(struct uart_port *p, int offset, int value)

...

The rest is legit as we shouldn't use those in the ->serial_in()/->serial_out().

-- 
With Best Regards,
Andy Shevchenko

On 20. 02. 25, 16:23, Andy Shevchenko wrote:
> On Thu, Feb 20, 2025 at 12:16:03PM +0100, Jiri Slaby (SUSE) wrote:
>> There are serial_in/out() helpers to be used instead of direct
>> p->serial_in/out(). Use them in various 8250 drivers.
> 
> Okay, I have checked all calls, and I think to avoid possible issues
> we need to add a few comments here and there, see below.
> 
> But first of all, can we actually use serial_port_in()/serial_port_out()
> in the cases where we have already a pointer to uart_port?

Yes, this makes sense -- that'd be real 1:1 change.

PS: omap8250_irq() is funny. It uses all serial_port_in(), serial_in(), 
and port->serial_in() :D.

PS2: the naming of serial_in/out() is unfortunate. I'd expect 
serial8250_in/out().

thanks,
-- 
js
suse labs

On Thu, Feb 20, 2025 at 12:16:03PM +0100, Jiri Slaby (SUSE) wrote:
> There are serial_in/out() helpers to be used instead of direct
> p->serial_in/out(). Use them in various 8250 drivers.

Is this just a mechanical (compile-only) conversion?
IIRC 8250 DW code is twisted in some cases and it might
be a possibility of dead loops, but I don't remember it
by heart and haven't checked myself before writing this
reply.

TL;DR: this needs a thorough review.

-- 
With Best Regards,
Andy Shevchenko

On Thu, Feb 20, 2025 at 02:27:41PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 20, 2025 at 12:16:03PM +0100, Jiri Slaby (SUSE) wrote:
> > There are serial_in/out() helpers to be used instead of direct
> > p->serial_in/out(). Use them in various 8250 drivers.
> 
> Is this just a mechanical (compile-only) conversion?
> IIRC 8250 DW code is twisted in some cases and it might
> be a possibility of dead loops, but I don't remember it
> by heart and haven't checked myself before writing this
> reply.
> 
> TL;DR: this needs a thorough review.

Wonderful, are you going to do it?  :)

On Thu, Feb 20, 2025 at 03:39:40PM +0100, Greg KH wrote:
> On Thu, Feb 20, 2025 at 02:27:41PM +0200, Andy Shevchenko wrote:
> > On Thu, Feb 20, 2025 at 12:16:03PM +0100, Jiri Slaby (SUSE) wrote:
> > > There are serial_in/out() helpers to be used instead of direct
> > > p->serial_in/out(). Use them in various 8250 drivers.
> > 
> > Is this just a mechanical (compile-only) conversion?
> > IIRC 8250 DW code is twisted in some cases and it might
> > be a possibility of dead loops, but I don't remember it
> > by heart and haven't checked myself before writing this
> > reply.
> > 
> > TL;DR: this needs a thorough review.
> 
> Wonderful, are you going to do it?  :)

Why not. Just give me some time.

-- 
With Best Regards,
Andy Shevchenko

On Thu, Feb 20, 2025 at 04:57:02PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 20, 2025 at 03:39:40PM +0100, Greg KH wrote:
> > On Thu, Feb 20, 2025 at 02:27:41PM +0200, Andy Shevchenko wrote:
> > > On Thu, Feb 20, 2025 at 12:16:03PM +0100, Jiri Slaby (SUSE) wrote:
> > > > There are serial_in/out() helpers to be used instead of direct
> > > > p->serial_in/out(). Use them in various 8250 drivers.
> > > 
> > > Is this just a mechanical (compile-only) conversion?
> > > IIRC 8250 DW code is twisted in some cases and it might
> > > be a possibility of dead loops, but I don't remember it
> > > by heart and haven't checked myself before writing this
> > > reply.
> > > 
> > > TL;DR: this needs a thorough review.
> > 
> > Wonderful, are you going to do it?  :)
> 
> Why not. Just give me some time.

Just done it, I think we need small improvements as I suggested in the separate
reply. Otherwise I like the idea to unify this.

-- 
With Best Regards,
Andy Shevchenko