Re: [PATCH v6 5/6] rtc: s35390a: Add pinctrl

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 1791 bytes --]

On Tue, 2026-08-25 at 21:28 +0200, Linus Walleij wrote:
> On Mon, Aug 24, 2026 at 12:26 AM <sashiko-bot@kernel.org> wrote:
> 
> > >  config RTC_DRV_S35390A
> > >       tristate "Seiko Instruments S-35390A"
> > > +     depends on PINCTRL
> > 
> > [Severity: High]
> > Will this unconditional dependency cause the RTC driver to be silently dropped
> > on platforms without PINCTRL enabled?
> 
> Maybe just select PINCTRL then, it's a valid point.
> 
> > > +     err = pinctrl_enable(pctl);
> > > +     if (err)
> > > +             return dev_err_probe(dev, err, "pinctrl enable failed\n");
> > > +
> > > +     /* If no pinmux function is defined in DT, fallback to previous behaviour */
> > > +     fallback[0] = S35390A_FUNC_IGNORE;
> > > +     fallback[1] = S35390A_FUNC_WAKEUP;
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(s35390a_pin_groups); i++) {
> > > +             if (s35390a->pinfunction[i] == -1) {
> > 
> > [Severity: Medium]
> > Is there a data race here since the pinfunction array is accessed locklessly
> > after pinctrl_enable() has made the pinctrl device available?
> 
> That's a valid concern.
I don't think so.

`s35390a_pinctrl_set_mux` itself will take a lock for the operation.

`pinfunction[i]` is a 32-bit integer, so there should be no load
tearing.

If we take a lock here, it shouldn't change anything. In a normal
condition this shouldn't happen anyway, as `pinctrl_enable` should
block until the default state is selected. If another driver is
concurrently selecting a pinctrl state of this device while we probe
(which is the only time pinfunction should be modified here
concurrently), it is still a race condition whether the fallback
applies or not.

Thanks
- Markus Probst

> 
> Yours,
> Linus Walleij

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]