Re: [PATCH v1 1/1] Revert "pinctrl: avoid unsafe code pattern in find_pinctrl()"

[Andy Shevchenko]

On Tue, Oct 17, 2023 at 09:34:34PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 17, 2023 at 08:18:23PM +0200, Linus Walleij wrote:
> > On Tue, Oct 17, 2023 at 4:18 PM Andy Shevchenko
> > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > 
> > > The commit breaks MMC enumeration on the Intel Merrifield
> > > plaform.
> > 
> > The enumeration works, just that the probe order is different, right?
> > 
> > > Before:
> > > [   36.439057] mmc0: SDHCI controller on PCI [0000:00:01.0] using ADMA
> > > [   36.450924] mmc2: SDHCI controller on PCI [0000:00:01.3] using ADMA
> > > [   36.459355] mmc1: SDHCI controller on PCI [0000:00:01.2] using ADMA
> > > [   36.706399] mmc0: new DDR MMC card at address 0001
> > > [   37.058972] mmc2: new ultra high speed DDR50 SDIO card at address 0001
> > > [   37.278977] mmcblk0: mmc0:0001 H4G1d 3.64 GiB
> > > [   37.297300]  mmcblk0: p1 p2 p3 p4 p5 p6 p7 p8 p9 p10
> > >
> > > After:
> > > [   36.436704] mmc2: SDHCI controller on PCI [0000:00:01.3] using ADMA
> > > [   36.436720] mmc1: SDHCI controller on PCI [0000:00:01.0] using ADMA
> > > [   36.463685] mmc0: SDHCI controller on PCI [0000:00:01.2] using ADMA
> > > [   36.720627] mmc1: new DDR MMC card at address 0001
> > > [   37.068181] mmc2: new ultra high speed DDR50 SDIO card at address 0001
> > > [   37.279998] mmcblk1: mmc1:0001 H4G1d 3.64 GiB
> > > [   37.302670]  mmcblk1: p1 p2 p3 p4 p5 p6 p7 p8 p9 p10
> > >
> > > This reverts commit c153a4edff6ab01370fcac8e46f9c89cca1060c2.
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > 
> > Relying on this probe order or whatever it is causing one or the other
> > to be enumerated first seems very fragile, I think this condition can be
> > caused by other much more random things in the probe path as well,
> > so it would be great if we could just hammer this down for good, as
> > it is apparently ABI.

And as I mentioned in the reply to the patch, I have 100% reproducibility
of the issue, I never have "random" or arbitrary numbers. While it might
be fragile, it very well works reliably for _years_.

> > In the past some file system developers have told us (Ulf will know)
> > that we can't rely on the block device enumeration to identify
> > devices, and requires that we use things such as sysfs or the
> > UUID volume label in ext4 to identify storage.
> 
> While I technically might agree with you, this was working for everybody
> since day 1 of support of Intel Merrifield added (circa v4.8), now _user
> space_ is broken.
> 
> Note, I'm having _simple_ setup, no fancy UDEV or DBUS there, and I want
> my scripts simply continue working. As I mentioned, this is Buildroot
> + Busybox which I haven't touched in the area of how they treat MMC
> devices in _user space_.
> 
> Since we are at rc6 I prefer to get this reverted first and next cycle we can
> discuss better solutions. I'm all for testing any.
> 
> > That said, device trees are full of stuff like this:
> > 
> >         aliases {
> >                 serial0 = &uart_AO;
> >                 mmc0 = &sd_card_slot;
> >                 mmc1 = &sdhc;
> >         };
> 
> And Rob, AFAIU, is against aliases.
> 
> > Notice how this enumeration gets defined by the aliases.
> > 
> > Can you do the same with device properties? (If anyone can
> > answer that question it's Dmitry!)
> 
> No, and why should we?

-- 
With Best Regards,
Andy Shevchenko