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

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
	Ferry Toth <ftoth@exalondelft.nl>
Subject: Re: [PATCH v1 1/1] Revert "pinctrl: avoid unsafe code pattern in find_pinctrl()"
Date: Tue, 17 Oct 2023 14:43:01 -0700	[thread overview]
Message-ID: <ZS7_5VGvRnw99gzd@google.com> (raw)
In-Reply-To: <ZS7kY/+80Be4geGM@smile.fi.intel.com>

Hi Andy,

On Tue, Oct 17, 2023 at 10:45:39PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 17, 2023 at 08:59:05PM +0200, Linus Walleij wrote:
> > On Tue, Oct 17, 2023 at 8:34 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Oct 17, 2023 at 08:18:23PM +0200, Linus Walleij wrote:
> > 
> > > > 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.
> > 
> > Actually, I don't agree with that, just relaying it. I would prefer that we
> > solve exactly the problem that we are facing here: some random unrelated
> > code or similar affecting enumeration order of mmc devices.

Sorry, but the era of static configuration where one has a well defined
order in which things are probed and numbered has long gone. The right
answer is either device aliases that provides stable numbering on a
board that is not dependent on scheduler behavior, mutexes
implementation (how they deal with writer starvation, etc),
kernel/driver/subsystem linking order and myriad other things, or
mounting by UUID. The kernel does not provide any guarantees on the
stability of device probe and instantiation order.

If you think about it it is the same issue as legacy GPIO numbering.
It was convenient some time ago, but now it is no longer suitable or
sufficient and could change when kernel is uprevved.

> > 
> > It's not the first time it happens to me, I have several devices that change
> > this enumeration order depending on whether an SD card is plugged
> > in or not, and in a *BIG* way: the boot partition on the soldered eMMC
> > changes enumeration depending on whether an SD card is inserted
> > or not, and that has never been fixed (because above).
> 
> This is not the problem I have. I haven't added any SD card, hardware
> configuration is the same. The solely difference in the whole setup is
> this revert applied or not.

Yes, I guess there is a contention on this mutex and the fact that we
are now taking it once and not twice makes difference in which probes
happen. If you look at the logs, you will see that even before the patch
controllers did not enumerate on the order of PCI functions:

[   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

So you have mmc2 instantiated before mmc1 even before the patch. This
happens because we now have

		.probe_type = PROBE_PREFER_ASYNCHRONOUS,

in sdhci_driver structure in drivers/mmc/host/sdhci-pci-core.c. It just
happened that even with asynchronous probing your storage did end up on
mmc0 originally and you were happy.

I wonder, could you please post entire dmesg for your system?

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

Rob might not want them, but they are the reality and are present for
multiple classes of devices and I believe are here to stay.

> > >
> > > > 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?
> > 
> > Because device properties are not device tree, they are just some
> > Linux thing so we can do whatever we want. Just checking if
> > Dmitry has some idea that would solve this for good, he usually
> > replies quickly.
> 
> OK.

I think the right answer is "fix the userspace" really in this case. We
could also try extend of_alias_get_id() to see if we could pass some
preferred numbering on x86. But this will again be fragile if the
knowledge resides in the driver and is not tied to a particular board
(as it is in DT case): there could be multiple controllers, things will
be shifting board to board...

Thanks.

-- 
Dmitry