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

Maciej W. Rozycki
2025-06-17T13:21:42Z

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