Re: [PATCH v7 1/2] serial: core: fix baud rate fallback in uart_get_baud_rate()

[Ilpo Järvinen]

On Wed, 7 Oct 2026, Hugo Villeneuve wrote:

> Hi Ilpo,
> 
> On Wed, 7 Oct 2026 13:38:55 +0300 (EEST)
> Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
> 
> > On Tue, 6 Oct 2026, Hugo Villeneuve wrote:
> > 
> > > On Thu, 1 Oct 2026 12:46:05 +0300 (EEST)
> > > Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
> > > 
> > > > On Thu, 1 Oct 2026, Greg Kroah-Hartman wrote:
> > > > 
> > > > > On Wed, Sep 30, 2026 at 12:59:00PM +0000, Hui Peng wrote:
> > > > > > When uart_get_baud_rate() is called with a baud rate exceeding the port's
> > > > > > maximum supported speed (port->uartclk / 16), it clips baud to [min,
> > > > > > max - 1] and encodes it into termios via tty_termios_encode_baud_rate().
> > > > > > 
> > > > > > However, because the loop bound is for (try = 0; try < 2; try++), the
> > > > > > loop terminates immediately after try == 1 without re-evaluating
> > > > > 
> > > > > But try == 1 should keep the loop going as it is < 2, right?  What am I
> > > > > missing here?  Do I need more coffee?
> > > > >
> > > > > > baud = tty_termios_baud_rate(termios) for the clipped rate, hitting
> > > > > > WARN_ON(1) and returning 0, which then triggers a fatal divide-by-zero
> > > > > > (Oops: divide error) in uart_get_divisor():
> > > > > > 
> > > > > >   WARNING: drivers/tty/serial/serial_core.c:548 at uart_get_baud_rate+0x136/0x260
> > > > > >   [ ... ]
> > > > > >   divide error: 0000 [#1] PREEMPT SMP KASAN
> > > > > > 
> > > > > > Increase the retry count in uart_get_baud_rate() from 2 to 3 iterations so
> > > > > > that clipped baud rates are re-evaluated in the third iteration.
> > > > > 
> > > > > What is the magic 2 here, and why turning it into a magic 3 somehow fix
> > > > > things?
> > > > 
> > > > Hi Greg & Hui,
> > > > 
> > > > First of all, I'm withdrawing my Reviewed-by from this!!!
> > > > 
> > > > Lets hope the submitter can finally get his/her act together and not make 
> > > > unlisted changes between versions or send non-sense.
> > > > 
> > > > While the code change is still fine, it seems the submitter (or more 
> > > > likely AI) has changed the changelog from what I read when I reviewed 
> > > > this. And the new one is way worse than it used to be so not being able 
> > > > to follow what's going on is very understandable given the lackluster 
> > > > explanation that remains.
> > > > 
> > > > 
> > > > What you're missing is that the baud returns happens within the loop, so:
> > > > 
> > > > try == 0: use new, if baud is out of bound and old is available, switch to 
> > > >           old
> > > > try == 1: if old is also out of bounds, there's the last resort rule 
> > > >           towards the end of the loop which is applied forcing baud to the 
> > > >           accetable range.
> > > 
> > > Hi all,
> > > is it at all possible that old is out of bounds in the first place?
> > 
> > Apparently it is, given the WARNING above (unless that too is fabricate 
> > by LLM which seems well within possiblities given the track record of this 
> > particular submitter).
> > 
> > > If yes, is it something that should be fixed?
> > 
> > I agree.
> > 
> > I think the scenario here is (but take it with grain of salt, as the 
> > information seems to be constantly changing thanks to LLM/submitter 
> > failing to keep one's act to together) [1]:
> > 
> > Tested in QEMU against Linux 7.3.0-rc3 by setting /dev/ttyS1 to B115200,
> > lowering baud_base to 9600 (max = 9600) via TIOCSSERIAL, and calling
> > tcsetattr() with B57600 (old = B115200), reproducing the WARNING and
> > Oops: divide error in uart_get_divisor() on the unfixed kernel and
> > verifying clean execution with 0 warnings/faults with the fix applied.
> > 
> > So when rate gets lowered, it should alter the termios to prevent what is 
> > here seem as "old" having an invalid value.
> 
> That makes sense...
> 
> > 
> > [1] https://lore.kernel.org/linux-serial/20260930041216.155911-3-benquike@xxxxxxxxx/
> > 
> > > > try == 2: loop exits => WARN_ON(1) triggers.
> > > > 
> > > > What we'd want to happen with try == 2, is for it to use the return baud 
> > > > which is within the loop body.
> > > 
> > > Then should the "return 0" statement be modified to "return baud", and
> > > possibly the WARN_ON() removed? Then you wouldn't need to increase
> > > max try?
> > 
> > IMO, the entire loop constructs feels somewhat artificial in this case so 
> > I'd prefer to kill the loop entirely. But such refactoring is not going to 
> > be a minimal fix to the issue.
> 
> Yes, when I updated uart_get_baud_rate() a few months ago, the loop
> also felt a little bit weird, but I did not remove it because I assumed
> it was working ok if we assumed, like I did, that the old baud rate was
> always valid.
> 
> I tried in the past to simplify it, make it more logical and
> understandable by normal humans :) But I always end up with
> something not so simple and obvious because of all the special cases
> (B0, spd_* flags, etc).

Trying to all that in the same function surely gets problematic and 
repetivive. But how about adding another function/helper that is just 
called multiple times to avoid duplicating them?

> I am still trying though, maybe I will have better luck this time :)
> 
> 
> 

-- 
 i.