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

[Hugo Villeneuve]

Hi Ilpo,

On Wed, 7 Oct 2026 23:09:26 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:

> 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?

That is exactly what I have been trying all day :) I implemented a
uart_validate_baud_rate() helper. Still testing all corner cases...


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


-- 
Hugo Villeneuve