[PATCH 29/33] serial: 8250: drop DEBUG_AUTOCONF() macro

Jiri Slaby (SUSE) posted 33 patches 4 months ago
[PATCH 29/33] serial: 8250: drop DEBUG_AUTOCONF() macro
Posted by Jiri Slaby (SUSE) 4 months ago
DEBUG_AUTOCONF() is always disabled (by "#if 0"), so one would need to
recompile the kernel to use it. And even if they did, they would find
out it is broken anyway:
  error: variable 'scratch' is used uninitialized whenever 'if' condition is false

Drop it.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/serial/8250/8250_port.c | 39 ++---------------------------
 1 file changed, 2 insertions(+), 37 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 6363915a1787..e93bfdac3d0e 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -38,15 +38,6 @@
 
 #include "8250.h"
 
-/*
- * Debugging.
- */
-#if 0
-#define DEBUG_AUTOCONF(fmt...)	printk(fmt)
-#else
-#define DEBUG_AUTOCONF(fmt...)	do { } while (0)
-#endif
-
 /*
  * Here we define the default xmit fifo size used for each type of UART.
  */
@@ -825,8 +816,6 @@ static void autoconfig_has_efr(struct uart_8250_port *up)
 	id3 = serial_icr_read(up, UART_ID3);
 	rev = serial_icr_read(up, UART_REV);
 
-	DEBUG_AUTOCONF("950id=%02x:%02x:%02x:%02x ", id1, id2, id3, rev);
-
 	if (id1 == 0x16 && id2 == 0xC9 &&
 	    (id3 == 0x50 || id3 == 0x52 || id3 == 0x54)) {
 		up->port.type = PORT_16C950;
@@ -850,7 +839,6 @@ static void autoconfig_has_efr(struct uart_8250_port *up)
 	 *  0x14 - XR16C854.
 	 */
 	id1 = autoconfig_read_divisor_id(up);
-	DEBUG_AUTOCONF("850id=%04x ", id1);
 
 	id2 = id1 >> 8;
 	if (id2 == 0x10 || id2 == 0x12 || id2 == 0x14) {
@@ -937,7 +925,6 @@ static void autoconfig_16550a(struct uart_8250_port *up)
 	if (serial_in(up, UART_EFR) == 0) {
 		serial_out(up, UART_EFR, 0xA8);
 		if (serial_in(up, UART_EFR) != 0) {
-			DEBUG_AUTOCONF("EFRv1 ");
 			up->port.type = PORT_16650;
 			up->capabilities |= UART_CAP_EFR | UART_CAP_SLEEP;
 		} else {
@@ -950,8 +937,6 @@ static void autoconfig_16550a(struct uart_8250_port *up)
 
 			if (status1 == UART_IIR_FIFO_ENABLED_16750)
 				up->port.type = PORT_16550A_FSL64;
-			else
-				DEBUG_AUTOCONF("Motorola 8xxx DUART ");
 		}
 		serial_out(up, UART_EFR, 0);
 		return;
@@ -963,7 +948,6 @@ static void autoconfig_16550a(struct uart_8250_port *up)
 	 */
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 	if (serial_in(up, UART_EFR) == 0 && !broken_efr(up)) {
-		DEBUG_AUTOCONF("EFRv2 ");
 		autoconfig_has_efr(up);
 		return;
 	}
@@ -1026,8 +1010,6 @@ static void autoconfig_16550a(struct uart_8250_port *up)
 
 	serial_out(up, UART_LCR, 0);
 
-	DEBUG_AUTOCONF("iir1=%d iir2=%d ", status1, status2);
-
 	if (status1 == UART_IIR_FIFO_ENABLED_16550A &&
 	    status2 == UART_IIR_FIFO_ENABLED_16750) {
 		up->port.type = PORT_16750;
@@ -1056,17 +1038,10 @@ static void autoconfig_16550a(struct uart_8250_port *up)
 			 * It's an Xscale.
 			 * We'll leave the UART_IER_UUE bit set to 1 (enabled).
 			 */
-			DEBUG_AUTOCONF("Xscale ");
 			up->port.type = PORT_XSCALE;
 			up->capabilities |= UART_CAP_UUE | UART_CAP_RTOIE;
 			return;
 		}
-	} else {
-		/*
-		 * If we got here we couldn't force the IER_UUE bit to 0.
-		 * Log it and continue.
-		 */
-		DEBUG_AUTOCONF("Couldn't force IER_UUE to 0 ");
 	}
 	serial_out(up, UART_IER, iersave);
 
@@ -1098,9 +1073,6 @@ static void autoconfig(struct uart_8250_port *up)
 	if (!port->iobase && !port->mapbase && !port->membase)
 		return;
 
-	DEBUG_AUTOCONF("%s: autoconf (0x%04lx, 0x%p): ",
-		       port->name, port->iobase, port->membase);
-
 	/*
 	 * We really do need global IRQs disabled here - we're going to
 	 * be frobbing the chips IRQ enable register to see if it exists.
@@ -1147,9 +1119,7 @@ static void autoconfig(struct uart_8250_port *up)
 			 * We failed; there's nothing here
 			 */
 			uart_port_unlock_irqrestore(port, flags);
-			DEBUG_AUTOCONF("IER test failed (%02x, %02x) ",
-				       scratch2, scratch3);
-			goto out;
+			return;
 		}
 	}
 
@@ -1171,9 +1141,7 @@ static void autoconfig(struct uart_8250_port *up)
 		serial8250_out_MCR(up, save_mcr);
 		if (status1 != (UART_MSR_DCD | UART_MSR_CTS)) {
 			uart_port_unlock_irqrestore(port, flags);
-			DEBUG_AUTOCONF("LOOP test failed (%02x) ",
-				       status1);
-			goto out;
+			return;
 		}
 	}
 
@@ -1241,9 +1209,6 @@ static void autoconfig(struct uart_8250_port *up)
 		dev_warn(port->dev, "detected caps %08x should be %08x\n",
 			 old_capabilities, up->capabilities);
 	}
-out:
-	DEBUG_AUTOCONF("iir=%d ", scratch);
-	DEBUG_AUTOCONF("type=%s\n", uart_config[port->type].name);
 }
 
 static void autoconfig_irq(struct uart_8250_port *up)
-- 
2.49.0
Re: [PATCH 29/33] serial: 8250: drop DEBUG_AUTOCONF() macro
Posted by Maciej W. Rozycki 3 months, 3 weeks ago
On Wed, 11 Jun 2025, Jiri Slaby (SUSE) wrote:

> DEBUG_AUTOCONF() is always disabled (by "#if 0"), so one would need to
> recompile the kernel to use it. And even if they did, they would find
> out it is broken anyway:
>   error: variable 'scratch' is used uninitialized whenever 'if' condition is false

 This is removing useful debugging aids.

 The issue with compilation is related to commit 3398cc4f2b15 ("serial: 
8250: Add IIR FIFOs enabled field properly"), which removed the assignment 
of IIR to `scratch' (although a path did exist before it that bypassed the 
assignment anyway), and can be trivially fixed by bringing the assignment 
back and moving the debug statement next to it.

 I agree that "#if 0" isn't very useful as it requires patching the source 
to activate; changing it to "#ifdef DEBUG" would make more sense nowadays.

  Maciej
Re: [PATCH 29/33] serial: 8250: drop DEBUG_AUTOCONF() macro
Posted by Greg Kroah-Hartman 3 months, 3 weeks ago
On Tue, Jun 17, 2025 at 12:32:48PM +0100, Maciej W. Rozycki wrote:
> On Wed, 11 Jun 2025, Jiri Slaby (SUSE) wrote:
> 
> > DEBUG_AUTOCONF() is always disabled (by "#if 0"), so one would need to
> > recompile the kernel to use it. And even if they did, they would find
> > out it is broken anyway:
> >   error: variable 'scratch' is used uninitialized whenever 'if' condition is false
> 
>  This is removing useful debugging aids.

How can it be "useful" if it's broken and no one has ever reported that?

>  The issue with compilation is related to commit 3398cc4f2b15 ("serial: 
> 8250: Add IIR FIFOs enabled field properly"), which removed the assignment 
> of IIR to `scratch' (although a path did exist before it that bypassed the 
> assignment anyway), and can be trivially fixed by bringing the assignment 
> back and moving the debug statement next to it.

So it's been broken for over 2 years and no one has asked for it to be
fixed?

>  I agree that "#if 0" isn't very useful as it requires patching the source 
> to activate; changing it to "#ifdef DEBUG" would make more sense nowadays.

No, dynamic debugging is the proper solution, not build-time stuff.  If
you really need/want this, add it back in that way, not this old-style
"let's rebuild the whole kernel" type of thing.  This isn't the 1990's
anymore :)

thanks,

greg k-h
Re: [PATCH 29/33] serial: 8250: drop DEBUG_AUTOCONF() macro
Posted by Maciej W. Rozycki 3 months, 3 weeks ago
On Tue, 17 Jun 2025, Greg Kroah-Hartman wrote:

> > > DEBUG_AUTOCONF() is always disabled (by "#if 0"), so one would need to
> > > recompile the kernel to use it. And even if they did, they would find
> > > out it is broken anyway:
> > >   error: variable 'scratch' is used uninitialized whenever 'if' condition is false
> > 
> >  This is removing useful debugging aids.
> 
> How can it be "useful" if it's broken and no one has ever reported that?

 It's broken in a trivial way and would be fixed by a competent developer 
in no time.  If no one has reported the breakage, it means no one has used 
this code in a way that would trigger it, e.g. -Wno-error in effect would 
mask the compilation issue.  I'm fairly sure I used this code while making 
changes to the OxSemi Tornado backend a couple of years ago.

> >  The issue with compilation is related to commit 3398cc4f2b15 ("serial: 
> > 8250: Add IIR FIFOs enabled field properly"), which removed the assignment 
> > of IIR to `scratch' (although a path did exist before it that bypassed the 
> > assignment anyway), and can be trivially fixed by bringing the assignment 
> > back and moving the debug statement next to it.
> 
> So it's been broken for over 2 years and no one has asked for it to be
> fixed?

 Well, what can I say beyond the obvious?  That debugging a mature driver 
doesn't happen all the time?  This would typically happen when adding a 
new chip-specific backend, and I don't think new variants of 8250-style 
serial ports appear that often nowadays.

 You can argue one can insert these debug statements back if they need it, 
but someone already made this effort years ago, so why waste it?  To save 
a handful of source lines?  It doesn't seem a good justification to me.

> >  I agree that "#if 0" isn't very useful as it requires patching the source 
> > to activate; changing it to "#ifdef DEBUG" would make more sense nowadays.
> 
> No, dynamic debugging is the proper solution, not build-time stuff.  If
> you really need/want this, add it back in that way, not this old-style
> "let's rebuild the whole kernel" type of thing.  This isn't the 1990's
> anymore :)

 There's no need to rebuild everything, handing CFLAGS_8250_port.o=-DDEBUG 
to `make' only causes the named object to be recompiled.  I use it all the 
time, also to pass other compilation flags if needed (call me outdated if 
you prefer).

 Any kind of run-time selectable debugging would bloat the kernel binary 
unnecessarily for everyone, for the corner case of driver development or 
debugging.  Unless made optional at configuration or build time, but then 
we're back to the "1990's solution" with little to no gain over the local 
CFLAGS override.

 And if working on a piece of code, then rebuilding it sooner or later 
seems inevitable anyway, so what's the deal?

  Maciej
Re: [PATCH 29/33] serial: 8250: drop DEBUG_AUTOCONF() macro
Posted by Ilpo Järvinen 4 months ago
On Wed, 11 Jun 2025, Jiri Slaby (SUSE) wrote:

> DEBUG_AUTOCONF() is always disabled (by "#if 0"), so one would need to
> recompile the kernel to use it. And even if they did, they would find
> out it is broken anyway:
>   error: variable 'scratch' is used uninitialized whenever 'if' condition is false
> 
> Drop it.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> ---
>  drivers/tty/serial/8250/8250_port.c | 39 ++---------------------------
>  1 file changed, 2 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 6363915a1787..e93bfdac3d0e 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -38,15 +38,6 @@
>  
>  #include "8250.h"
>  
> -/*
> - * Debugging.
> - */
> -#if 0
> -#define DEBUG_AUTOCONF(fmt...)	printk(fmt)
> -#else
> -#define DEBUG_AUTOCONF(fmt...)	do { } while (0)
> -#endif
> -
>  /*
>   * Here we define the default xmit fifo size used for each type of UART.
>   */
> @@ -825,8 +816,6 @@ static void autoconfig_has_efr(struct uart_8250_port *up)
>  	id3 = serial_icr_read(up, UART_ID3);
>  	rev = serial_icr_read(up, UART_REV);
>  
> -	DEBUG_AUTOCONF("950id=%02x:%02x:%02x:%02x ", id1, id2, id3, rev);
> -
>  	if (id1 == 0x16 && id2 == 0xC9 &&
>  	    (id3 == 0x50 || id3 == 0x52 || id3 == 0x54)) {
>  		up->port.type = PORT_16C950;
> @@ -850,7 +839,6 @@ static void autoconfig_has_efr(struct uart_8250_port *up)
>  	 *  0x14 - XR16C854.
>  	 */
>  	id1 = autoconfig_read_divisor_id(up);
> -	DEBUG_AUTOCONF("850id=%04x ", id1);
>  
>  	id2 = id1 >> 8;
>  	if (id2 == 0x10 || id2 == 0x12 || id2 == 0x14) {
> @@ -937,7 +925,6 @@ static void autoconfig_16550a(struct uart_8250_port *up)
>  	if (serial_in(up, UART_EFR) == 0) {
>  		serial_out(up, UART_EFR, 0xA8);
>  		if (serial_in(up, UART_EFR) != 0) {
> -			DEBUG_AUTOCONF("EFRv1 ");
>  			up->port.type = PORT_16650;
>  			up->capabilities |= UART_CAP_EFR | UART_CAP_SLEEP;
>  		} else {
> @@ -950,8 +937,6 @@ static void autoconfig_16550a(struct uart_8250_port *up)
>  
>  			if (status1 == UART_IIR_FIFO_ENABLED_16750)
>  				up->port.type = PORT_16550A_FSL64;
> -			else
> -				DEBUG_AUTOCONF("Motorola 8xxx DUART ");
>  		}
>  		serial_out(up, UART_EFR, 0);
>  		return;
> @@ -963,7 +948,6 @@ static void autoconfig_16550a(struct uart_8250_port *up)
>  	 */
>  	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>  	if (serial_in(up, UART_EFR) == 0 && !broken_efr(up)) {
> -		DEBUG_AUTOCONF("EFRv2 ");
>  		autoconfig_has_efr(up);
>  		return;
>  	}
> @@ -1026,8 +1010,6 @@ static void autoconfig_16550a(struct uart_8250_port *up)
>  
>  	serial_out(up, UART_LCR, 0);
>  
> -	DEBUG_AUTOCONF("iir1=%d iir2=%d ", status1, status2);
> -
>  	if (status1 == UART_IIR_FIFO_ENABLED_16550A &&
>  	    status2 == UART_IIR_FIFO_ENABLED_16750) {
>  		up->port.type = PORT_16750;
> @@ -1056,17 +1038,10 @@ static void autoconfig_16550a(struct uart_8250_port *up)
>  			 * It's an Xscale.
>  			 * We'll leave the UART_IER_UUE bit set to 1 (enabled).
>  			 */
> -			DEBUG_AUTOCONF("Xscale ");
>  			up->port.type = PORT_XSCALE;
>  			up->capabilities |= UART_CAP_UUE | UART_CAP_RTOIE;
>  			return;
>  		}
> -	} else {
> -		/*
> -		 * If we got here we couldn't force the IER_UUE bit to 0.
> -		 * Log it and continue.
> -		 */
> -		DEBUG_AUTOCONF("Couldn't force IER_UUE to 0 ");
>  	}
>  	serial_out(up, UART_IER, iersave);
>  
> @@ -1098,9 +1073,6 @@ static void autoconfig(struct uart_8250_port *up)
>  	if (!port->iobase && !port->mapbase && !port->membase)
>  		return;
>  
> -	DEBUG_AUTOCONF("%s: autoconf (0x%04lx, 0x%p): ",
> -		       port->name, port->iobase, port->membase);
> -
>  	/*
>  	 * We really do need global IRQs disabled here - we're going to
>  	 * be frobbing the chips IRQ enable register to see if it exists.
> @@ -1147,9 +1119,7 @@ static void autoconfig(struct uart_8250_port *up)
>  			 * We failed; there's nothing here
>  			 */
>  			uart_port_unlock_irqrestore(port, flags);
> -			DEBUG_AUTOCONF("IER test failed (%02x, %02x) ",
> -				       scratch2, scratch3);
> -			goto out;
> +			return;
>  		}
>  	}
>  
> @@ -1171,9 +1141,7 @@ static void autoconfig(struct uart_8250_port *up)
>  		serial8250_out_MCR(up, save_mcr);
>  		if (status1 != (UART_MSR_DCD | UART_MSR_CTS)) {
>  			uart_port_unlock_irqrestore(port, flags);
> -			DEBUG_AUTOCONF("LOOP test failed (%02x) ",
> -				       status1);
> -			goto out;
> +			return;
>  		}
>  	}
>  
> @@ -1241,9 +1209,6 @@ static void autoconfig(struct uart_8250_port *up)
>  		dev_warn(port->dev, "detected caps %08x should be %08x\n",
>  			 old_capabilities, up->capabilities);
>  	}
> -out:
> -	DEBUG_AUTOCONF("iir=%d ", scratch);
> -	DEBUG_AUTOCONF("type=%s\n", uart_config[port->type].name);
>  }
>  
>  static void autoconfig_irq(struct uart_8250_port *up)
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.