Re: [PATCH 4/4] Revert "drivers: convert sbd_duart.map_guard from atomic_t to refcount_t"

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Apr 27, 2026 at 03:13:55PM +0100, Maciej W. Rozycki wrote:
> On Sun, 26 Apr 2026, Greg Kroah-Hartman wrote:
> 
> > > Revert commit 22a33651a56f ("drivers: convert sbd_duart.map_guard from
> > > atomic_t to refcount_t"), which broke perfectly valid code:
> > > 
> > >   ------------[ cut here ]------------
> > >   WARNING: CPU: 1 PID: 1 at lib/refcount.c:114 sbd_request_port+0x54/0x140
> > >   refcount_t: increment on 0; use-after-free.
> > >   CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.11.0-rc2+ #34
> > >   Stack : 0000000014001fe0 0000000000000000 ffffffff80830000 0000000000000000
> > >           ffffffff8127bc7a ffffffff8016fe08 ffffffff808d0000 ffffffff808d0000
> > >           ffffffff807aa828 ffffffff80822337 ffffffff808ce188 a8000001860b0000
> > >           0000000000000001 0000000000000001 00000000000001c8 ffffffff808a3090
> > >           00000000000000bb ffffffff801b09d4 a80000018609bb68 ffffffff801231cc
> > >           ffffffff812a0000 ffffffff80171388 0000000000001000 ffffffff807aa828
> > >           0000000000000001 0000000000000001 0000000000000000 0000000000000000
> > >           0000000000000000 a80000018609bab0 0000000000000000 ffffffff803c47cc
> > >           0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > >           ffffffff807cb648 ffffffff8010bff8 0000000014001fe1 ffffffff803c47cc
> > >           ...
> > >   Call Trace:
> > >   [<ffffffff8010bff8>] show_stack+0x28/0x88
> > >   [<ffffffff803c47cc>] dump_stack+0x8c/0xc0
> > >   [<ffffffff801aff5c>] __warn+0xe0/0x114
> > >   [<ffffffff801233f0>] warn_slowpath_fmt+0x40/0x50
> > >   [<ffffffff80455bcc>] sbd_request_port+0x54/0x140
> > >   [<ffffffff804563a4>] sbd_config_port+0x2c/0x68
> > >   ---[ end trace f666d696412caa3e ]---
> > > 
> > > (report at the offending commit) -- sbd_request_port() is called twice
> > > per DUART instance, to reserve a resource holding the control register
> > > block shared between the two channels, so there's no slightest chance
> > > for an overflow.  Also this doesn't stop the driver from working and
> > > it's just the reservation that is missing as a result, i.e.:
> > > 
> > > 10060100-100601ff : sb1250-duart
> > > 10060200-100602ff : sb1250-duart
> > > 
> > > as from the offending change, vs:
> > > 
> > > 10060100-100601ff : sb1250-duart
> > > 10060200-100602ff : sb1250-duart
> > > 10060300-100603ff : sb1250-duart
> > > 
> > > beforehand, which is surely why the breakage has gone so long unnoticed.
> > > 
> > > "If it ain't broke, don't fix it," so just revert the broken commit.
> > 
> > How about fix this up to work properly with a refcount?  having "open
> > coded" atomic variables like this is ripe for problems, like it seems
> > this driver is abusing.
> 
>  Clearly refcount has odd semantics for this use case, as the failed 
> attempt to "fix" this code has shown.
> 
>  The natural values for `map_guard' are 0 and 1 (FALSE and TRUE), for the 
> resource not taken and taken respectively, however refcount code complains 
> about this arrangement as indicated by the report quoted.
> 
>  I suppose I can bend backwards and adopt other values, which I'll have to 
> figure out from the API somehow, but it's not clear to me how it would 
> cause less confusion than original straightforward code, the whole point 
> of which is to prevent the resource from being requested again for the 
> second port in a DUART.
> 
>  Or I could use an ordinary variable, possibly of the `bool' type, guarded 
> by a spinlock, but that would be even more of an overkill IMO.

No, that would be best because using an atomic is the same end result,
but it confuses us humans who are the ones responsible for maintaining
the code over time.

So a bool would be great, thanks!

greg k-h