[v2,1/4] serial: 8250_dw: Keep the BUSY-safe divisor hook when DLF is present

dw8250_probe() installs dw8250_set_divisor(), which brackets the
DLAB/DLL/DLH sequence with dw8250_idle_enter() and dw8250_idle_exit()
so a BUSY UART cannot drop the divisor LCR write.

dw8250_setup_port() runs afterwards and replaces that hook with the
dwlib divisor hook whenever it detects DLF. The dwlib hook programs the
divisor through serial8250_do_set_divisor() without entering idle,
leaving the LCR|DLAB write exposed to BUSY.

When termios changes while the UART is transmitting, that LCR|DLAB
write can be ignored and dw8250_check_lcr() retries it between
dw8250_idle_enter() and dw8250_idle_exit(). dw8250_idle_exit() then
restores IER while DLAB is still set, so the value lands in DLH. The
real IER stays at the zero written by dw8250_idle_enter(), and the port
gets no further TX or RX interrupts while up->ier says they're enabled.

Do not replace a divisor hook the driver has already installed. Program
DLF from the BUSY-safe hook, inside the idle section, through a dwlib
helper that checks dlf_size.

Observed on an Allwinner A733 (Radxa Cubie A7Z), where
dw8250_setup_port() mistakes the RS485 control register for DLF (see the
next patch): the first interactive tcsetattr() during console output
left hardware IER at 0 and up->ier at 0x07. The shell never printed its
prompt or read input. With this change alone the same boot reached the
prompt and echoed input, and hardware IER matched up->ier afterwards.

Fixes: a7b9ce39fbe4 ("serial: 8250_dw: Ensure BUSY is deasserted")
Cc: stable@vger.kernel.org
Signed-off-by: Vinicius Pedrosa <vinicius.eduardo.pedrosa@gmail.com>
---
 drivers/tty/serial/8250/8250_dw.c    |  2 ++
 drivers/tty/serial/8250/8250_dwlib.c | 16 ++++++++++++++--
 drivers/tty/serial/8250/8250_dwlib.h |  1 +
 3 files changed, 17 insertions(+), 2 deletions(-)
  

On Mon, Oct 19, 2026 at 05:06:10PM -0300, Vinicius Pedrosa wrote:
> dw8250_probe() installs dw8250_set_divisor(), which brackets the
> DLAB/DLL/DLH sequence with dw8250_idle_enter() and dw8250_idle_exit()
> so a BUSY UART cannot drop the divisor LCR write.
> 
> dw8250_setup_port() runs afterwards and replaces that hook with the
> dwlib divisor hook whenever it detects DLF. The dwlib hook programs the
> divisor through serial8250_do_set_divisor() without entering idle,
> leaving the LCR|DLAB write exposed to BUSY.
> 
> When termios changes while the UART is transmitting, that LCR|DLAB
> write can be ignored and dw8250_check_lcr() retries it between
> dw8250_idle_enter() and dw8250_idle_exit(). dw8250_idle_exit() then
> restores IER while DLAB is still set, so the value lands in DLH. The
> real IER stays at the zero written by dw8250_idle_enter(), and the port
> gets no further TX or RX interrupts while up->ier says they're enabled.
> 
> Do not replace a divisor hook the driver has already installed. Program
> DLF from the BUSY-safe hook, inside the idle section, through a dwlib
> helper that checks dlf_size.
> 
> Observed on an Allwinner A733 (Radxa Cubie A7Z), where
> dw8250_setup_port() mistakes the RS485 control register for DLF (see the
> next patch): the first interactive tcsetattr() during console output
> left hardware IER at 0 and up->ier at 0x07. The shell never printed its
> prompt or read input. With this change alone the same boot reached the
> prompt and echoed input, and hardware IER matched up->ier afterwards.

...

> +/* Program the fractional part of the divisor, if the UART has one */
> +void __dw8250_set_divisor(struct uart_port *p, unsigned int quot_frac)

Let's reduce amount of double underscore functions. Let's call it
dw8250_update_dfl(). Also call the parameter just 'frac'.

void dw8250_update_dfl(struct uart_port *p, unsigned int frac)

...

> +++ b/drivers/tty/serial/8250/8250_dwlib.h

>  void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, const struct ktermios *old);
>  void dw8250_setup_port(struct uart_port *p);
> +void __dw8250_set_divisor(struct uart_port *p, unsigned int quot_frac);

Put it rather after _writel_ext() implementation, because it's just a small
wrapper on top of it. Or even make it 'static inline' there.
  

On Mon, Oct 19, 2026 at 05:06:10PM -0300, Vinicius Pedrosa wrote:

...

> @@ -218,7 +228,9 @@ void dw8250_setup_port(struct uart_port *p)
>  	if (reg) {
>  		pd->dlf_size = fls(reg);
>  		p->get_divisor = dw8250_get_divisor;

In the case of existing _set_divisor, do we want to keep the possible set
_get_divisor as well? Shouldn't we introduce a respective _get_divisor in 8250_dw.c?

> -		p->set_divisor = dw8250_set_divisor;
> +		/* Keep a hook the driver has already installed */
> +		if (!p->set_divisor)
> +			p->set_divisor = dw8250_set_divisor;
>  	}

On top of my previous reply, why is this hunk needed?

...

But now I see that we have same function name in two files. Can we rename those
in dwlib to, for example,
dw8250_get_divisor --> dw8250_get_dfl_divisor
dw8250_set_divisor --> dw8250_set_dfl_divisor

?
  

Hi Andy,

On Tue, 20 Oct 2026, Andy Shevchenko wrote:

> > +		/* Keep a hook the driver has already installed */
> > +		if (!p->set_divisor)
> > +			p->set_divisor = dw8250_set_divisor;
>
> On top of my previous reply, why is this hunk needed?

dw8250_probe() installs the BUSY-safe dw8250_set_divisor() before
dw8250_setup_port() runs. Without the !p->set_divisor check, detecting
DLF replaces it with the dwlib setter, which is the BUSY problem
described in the commit message.

> In the case of existing _set_divisor, do we want to keep the possible set
> _get_divisor as well? Shouldn't we introduce a respective _get_divisor in
> 8250_dw.c?

8250_dw.c doesn't install a get_divisor, so there's no existing getter
to preserve. The dwlib getter is still needed when DLF is present,
because it computes the fractional part used by the preserved BUSY-safe
setter.

> But now I see that we have same function name in two files. Can we rename
> those in dwlib to, for example,
> dw8250_get_divisor --> dw8250_get_dfl_divisor
> dw8250_set_divisor --> dw8250_set_dfl_divisor

I'll also clean up the naming in v3: dw8250_update_dlf() with a frac
parameter, static inline after dw8250_writel_ext(), and the dwlib-local
helpers renamed to dw8250_get_dlf_divisor() and
dw8250_set_dlf_divisor().

Thanks,
Vinicius
  

On Tue, Oct 20, 2026 at 07:47:53AM -0300, Vinicius Pedrosa wrote:
> On Tue, 20 Oct 2026, Andy Shevchenko wrote:

...

> > > +		/* Keep a hook the driver has already installed */
> > > +		if (!p->set_divisor)
> > > +			p->set_divisor = dw8250_set_divisor;
> >
> > On top of my previous reply, why is this hunk needed?
> 
> dw8250_probe() installs the BUSY-safe dw8250_set_divisor() before
> dw8250_setup_port() runs. Without the !p->set_divisor check, detecting
> DLF replaces it with the dwlib setter, which is the BUSY problem
> described in the commit message.
> 
> > In the case of existing _set_divisor, do we want to keep the possible set
> > _get_divisor as well? Shouldn't we introduce a respective _get_divisor in
> > 8250_dw.c?
> 
> 8250_dw.c doesn't install a get_divisor, so there's no existing getter
> to preserve.

Right.

> The dwlib getter is still needed when DLF is present,
> because it computes the fractional part used by the preserved BUSY-safe
> setter.

Right, but have you read my second question in the above paragraph?

> > But now I see that we have same function name in two files. Can we rename
> > those in dwlib to, for example,
> > dw8250_get_divisor --> dw8250_get_dfl_divisor
> > dw8250_set_divisor --> dw8250_set_dfl_divisor
> 
> I'll also clean up the naming in v3: dw8250_update_dlf() with a frac
> parameter, static inline after dw8250_writel_ext(), and the dwlib-local
> helpers renamed to dw8250_get_dlf_divisor() and
> dw8250_set_dlf_divisor().
  

Hi Andy,

On Wed, 21 Oct 2026, Andy Shevchenko wrote:

> Right, but have you read my second question in the above paragraph?

Sorry, I missed the second part of your question.

If I understand you correctly, since 8250_dw.c already has the BUSY-safe
set_divisor, you're suggesting that it should have the matching
get_divisor as well, instead of pairing it with the dwlib getter.

The dwlib getter would stay for the other users of dw8250_setup_port(),
like 8250_lpss. Is that the structure you had in mind?

Thanks,
Vinicius
  

On Wed, Oct 21, 2026 at 07:00:49AM -0300, Vinicius Pedrosa wrote:
> On Wed, 21 Oct 2026, Andy Shevchenko wrote:
> 
> > Right, but have you read my second question in the above paragraph?
> 
> Sorry, I missed the second part of your question.
> 
> If I understand you correctly, since 8250_dw.c already has the BUSY-safe
> set_divisor, you're suggesting that it should have the matching
> get_divisor as well, instead of pairing it with the dwlib getter.

Exactly!

> The dwlib getter would stay for the other users of dw8250_setup_port(),
> like 8250_lpss. Is that the structure you had in mind?

8250_lpss doesn't redefine either of the _get/_set_divisor, so it won't be
affected. But I think that 8250_lpss actually lacks of busy support that's
why it uses simpler version of all this. Maybe we can refactor that (in
the future?) so that we will have dwlib to provide the IDLE version of them
for all and depending on the existence of busy functionality degrade to
the same as just current DFL helpers.

Can you check that approach?

Ah, dw8250_idle_*() are already BUSY-functionality aware. So, what we need
is to provide dw8250_idlej_*() callbacks via struct dw8250_port_data and
make current 8250_dw::dw8250_set_divisor() to be part of dwlib.

Patch 1 (dwlib): Add callback definitions for dw8250_idle_*() to struct dw8250_port_data
Patch 2 (dw): Assign new callbacks and use them in dw8250_set_divisor()
Patch 3 (lpss): Introduce stubs and assign new callbacks to them
Patch 4 (dwlib): Use dw8250_idle_*() in dw8250_set_divisor()
Patch 5 (dw): Kill now duplicate code pieces

(Some of the patches might be combined, but I think the above split is not bad.)

Would it work?
  

Hi Andy,

On Wed, 21 Oct 2026, Andy Shevchenko wrote:

> Ah, dw8250_idle_*() are already BUSY-functionality aware. So, what we need
> is to provide dw8250_idlej_*() callbacks via struct dw8250_port_data and
> make current 8250_dw::dw8250_set_divisor() to be part of dwlib.
>
> Patch 1 (dwlib): Add callback definitions for dw8250_idle_*() to struct dw8250_port_data
> Patch 2 (dw): Assign new callbacks and use them in dw8250_set_divisor()
> Patch 3 (lpss): Introduce stubs and assign new callbacks to them
> Patch 4 (dwlib): Use dw8250_idle_*() in dw8250_set_divisor()
> Patch 5 (dw): Kill now duplicate code pieces
>
> Would it work?

Yes, I think the approach works. Two details from checking the call
paths.

dw8250_setup_port() is skipped for Octeon (skip_autocfg), so 8250_dw
would still have to install the setter from probe.

For LPSS, I wonder if the idle callbacks could simply be optional.
8250_dw would provide them and LPSS could leave them NULL, avoiding
stubs and a separate LPSS patch. The common setter in dwlib would use
them when present.

I'd still prefer to keep patch 1 as the small fix with the stable tag
and do the refactoring as a separate follow-up series. Would that work
for you?

I don't have LPSS hardware, but I can build-test the LPSS side.

Thanks,
Vinicius
  

On Wed, Oct 21, 2026 at 12:37:32PM -0300, Vinicius Pedrosa wrote:
> On Wed, 21 Oct 2026, Andy Shevchenko wrote:
> 
> > Ah, dw8250_idle_*() are already BUSY-functionality aware. So, what we need
> > is to provide dw8250_idle_*() callbacks via struct dw8250_port_data and
> > make current 8250_dw::dw8250_set_divisor() to be part of dwlib.
> >
> > Patch 1 (dwlib): Add callback definitions for dw8250_idle_*() to struct dw8250_port_data
> > Patch 2 (dw): Assign new callbacks and use them in dw8250_set_divisor()
> > Patch 3 (lpss): Introduce stubs and assign new callbacks to them
> > Patch 4 (dwlib): Use dw8250_idle_*() in dw8250_set_divisor()
> > Patch 5 (dw): Kill now duplicate code pieces
> >
> > Would it work?
> 
> Yes, I think the approach works. Two details from checking the call
> paths.
> 
> dw8250_setup_port() is skipped for Octeon (skip_autocfg), so 8250_dw
> would still have to install the setter from probe.

No, just pass that as a 'bool autoconf' to it, and assign those callbacks
there, while calling dw8250_setup_port() unconditionally from _probe().

It might require another patch, though (to make it a parameter first).

> For LPSS, I wonder if the idle callbacks could simply be optional.
> 8250_dw would provide them and LPSS could leave them NULL, avoiding
> stubs and a separate LPSS patch. The common setter in dwlib would use
> them when present.

I think it would be less flexible, also it will add an additional branch
(actually two) each time the set_divisor is called (not that it's critical
but just wanted to mentioned that).

> I'd still prefer to keep patch 1 as the small fix with the stable tag
> and do the refactoring as a separate follow-up series. Would that work
> for you?

I dunno, I prefer just a longer solution, but I don't know how big this issue
is that we need to backport in that way (and not taking the whole series as a
fix).

> I don't have LPSS hardware, but I can build-test the LPSS side.

I have Intel Galileo board that uses 8250_lpss.
  

Hi Andy,

On Wed, 21 Oct 2026, Andy Shevchenko wrote:

> No, just pass that as a 'bool autoconf' to it, and assign those callbacks
> there, while calling dw8250_setup_port() unconditionally from _probe().

I think the autoconf approach works for Octeon as long as everything
dw8250_setup_port() probes today stays under it, including the RS485
detection and UART_CAP_NOTEMT, not just DLF/CPR. With autoconf=false it
would only install the common divisor setter.

> I think it would be less flexible, also it will add an additional branch
> (actually two) each time the set_divisor is called (not that it's critical
> but just wanted to mentioned that).

The idle callbacks have to come from the drivers, since the
dw8250_idle_*() functions are static in 8250_dw.c. Stubs for LPSS are
fine with me.

> I dunno, I prefer just a longer solution, but I don't know how big this issue
> is that we need to backport in that way (and not taking the whole series as a
> fix).

On the severity: the IER restore comes from a7b9ce39fbe4. On the A733 I
saw hardware IER left at 0 and the console stop taking TX/RX
interrupts. I haven't tested this on hardware with a real DLF.

Given that, how about keeping the small fix as patch 1 with the stable
tag, and putting the refactoring after it in the same series? That would
keep the fix independently backportable if needed, while mainline would
still end up with the structure you proposed.

> I have Intel Galileo board that uses 8250_lpss.

The Galileo should be useful for checking the LPSS path.

Thanks,
Vinicius
  

On Wed, Oct 21, 2026 at 02:04:03PM -0300, Vinicius Pedrosa wrote:
> On Wed, 21 Oct 2026, Andy Shevchenko wrote:

...

> > I dunno, I prefer just a longer solution, but I don't know how big this issue
> > is that we need to backport in that way (and not taking the whole series as a
> > fix).
> 
> On the severity: the IER restore comes from a7b9ce39fbe4. On the A733 I
> saw hardware IER left at 0 and the console stop taking TX/RX
> interrupts. I haven't tested this on hardware with a real DLF.

Yes. but is it only a single device/platform affected or a big class of the
devices? I assume only narrow part of them which unfortunately use DFL register
offset for something else in their RTL.

> Given that, how about keeping the small fix as patch 1 with the stable
> tag, and putting the refactoring after it in the same series? That would
> keep the fix independently backportable if needed, while mainline would
> still end up with the structure you proposed.

It all depends on the outcome of the above discussion. Maybe others can share
their opinions, but I lean towards a series-as-a-fix rather than a bit ugly fix
+ proper refactoring on top.

The whole approach doesn't seem to me like a pile of changes. The deduplication
patch can be deferred for later actually.
  

Hi Andy,

On Wed, 21 Oct 2026, Andy Shevchenko wrote:

> Yes. but is it only a single device/platform affected or a big class of the
> devices? I assume only narrow part of them which unfortunately use DFL register
> offset for something else in their RTL.

The A733 is the only case I've seen, and the false DLF is what exposed
it. But I don't think the BUSY problem depends on the DLF offset being
reused: with a real DLF on a UART that isn't 16550-compatible,
dw8250_setup_port() also replaces the BUSY-safe setter with the dwlib
one, so the same path exists. I haven't confirmed that on hardware.

With patch 2 the A733 no longer takes that path.

> It all depends on the outcome of the above discussion. Maybe others can share
> their opinions, but I lean towards a series-as-a-fix rather than a bit ugly fix
> + proper refactoring on top.

I'm fine with a series-as-a-fix, and with deferring the deduplication
patch.

Thanks,
Vinicius