linux-kernel - Re: [PATCH 29/33] serial: 8250: drop DEBUG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.21.2506171341570.37405@angie.orcam.me.uk>
Date: Tue, 17 Jun 2025 14:21:40 +0100 (BST)
From: "Maciej W. Rozycki" <macro@...am.me.uk>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
cc: "Jiri Slaby (SUSE)" <jirislaby@...nel.org>, 
    Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, 
    linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 29/33] serial: 8250: drop DEBUG_AUTOCONF() macro

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