Re: [GIT PULL] ARM: SoC fixes for v5.10, part 3

Re: [GIT PULL] ARM: SoC fixes for v5.10, part 3

[Geert Uytterhoeven]

CC devicetree

On Tue, Dec 1, 2020 at 3:06 PM Arnd Bergmann <arnd@kernel.org> wrote:
> On Tue, Dec 1, 2020 at 12:39 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > So, I think we have two options. If people are willing to move to
> > "disk labels" or to patch their DTBs with mmc aliases, things can stay
> > as is. Otherwise, we can revert the async probe parts of the mmc host
> > drivers, but that would still leave us in a fragile situation.
>
> Can you reliably detect whether the mmc aliases in the dt exist?
> If that's possible, maybe the async flag could be masked out to only have
> an effect when the device number is known.

IMHO DT aliases are not a proper solution for this.

Yes, you can detect reliably if an alias exists in the DT.
The problems start when having multiple devices, some with aliases,
some without.  And when devices can appear dynamically (without
aliases, as there is no support for dynamically updating the aliases
list).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [GIT PULL] ARM: SoC fixes for v5.10, part 3

[Doug Anderson]

Hi,

On Mon, Dec 7, 2020 at 1:55 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Mon, Dec 7, 2020 at 9:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Dec 1, 2020 at 3:06 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > > On Tue, Dec 1, 2020 at 12:39 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > So, I think we have two options. If people are willing to move to
> > > > "disk labels" or to patch their DTBs with mmc aliases, things can stay
> > > > as is. Otherwise, we can revert the async probe parts of the mmc host
> > > > drivers, but that would still leave us in a fragile situation.
> > >
> > > Can you reliably detect whether the mmc aliases in the dt exist?
> > > If that's possible, maybe the async flag could be masked out to only have
> > > an effect when the device number is known.
> >
> > IMHO DT aliases are not a proper solution for this.
> >
> > Yes, you can detect reliably if an alias exists in the DT.
> > The problems start when having multiple devices, some with aliases,
> > some without.  And when devices can appear dynamically (without
> > aliases, as there is no support for dynamically updating the aliases
> > list).
>
> Actually you hit a problem earlier than that: the async probe is a
> property of the host controller driver, which may be a pci_driver,
> platform_driver, usb_driver, or anything else really. To figure out
> whether to probe it asynchronously, it would have to be the driver
> core, or each bus type that can host these to understand which
> device driver is responsible for probing an eMMC device attached
> to the host.

From what I've seen so far, my current thought on this issue is that
it's up to Ulf as the MMC maintainer what the next steps are.  For me,
at least, his argument that MMC block numbers have already shuffled
around several times in the last several years is at least some
evidence that they weren't exactly stable to begin with.  While we
could go back to the numbers that happened to be chosen as of kernel
v5.9, if someone was updating from a much older kernel then they may
have different expectations of what numbers are good / bad I think.

I will also offer one possible suggestion: what about a KConfig option
here?  In theory we could add a KConfig option like
"CONFIG_MMC_LEGACY_PROBE" or something that.  One can argue about what
the default ought to be, but maybe that would satisfy folks?  If you
were happy giving up a little bit of boot speed to get the v5.9 block
numbers then you could set this.


-Doug

Re: [GIT PULL] ARM: SoC fixes for v5.10, part 3

[Geert Uytterhoeven]

Hi Doug,

On Mon, Dec 7, 2020 at 11:15 PM Doug Anderson <dianders@chromium.org> wrote:
> On Mon, Dec 7, 2020 at 1:55 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > On Mon, Dec 7, 2020 at 9:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, Dec 1, 2020 at 3:06 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > > > On Tue, Dec 1, 2020 at 12:39 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > So, I think we have two options. If people are willing to move to
> > > > > "disk labels" or to patch their DTBs with mmc aliases, things can stay
> > > > > as is. Otherwise, we can revert the async probe parts of the mmc host
> > > > > drivers, but that would still leave us in a fragile situation.
> > > >
> > > > Can you reliably detect whether the mmc aliases in the dt exist?
> > > > If that's possible, maybe the async flag could be masked out to only have
> > > > an effect when the device number is known.
> > >
> > > IMHO DT aliases are not a proper solution for this.
> > >
> > > Yes, you can detect reliably if an alias exists in the DT.
> > > The problems start when having multiple devices, some with aliases,
> > > some without.  And when devices can appear dynamically (without
> > > aliases, as there is no support for dynamically updating the aliases
> > > list).
> >
> > Actually you hit a problem earlier than that: the async probe is a
> > property of the host controller driver, which may be a pci_driver,
> > platform_driver, usb_driver, or anything else really. To figure out
> > whether to probe it asynchronously, it would have to be the driver
> > core, or each bus type that can host these to understand which
> > device driver is responsible for probing an eMMC device attached
> > to the host.
>
> From what I've seen so far, my current thought on this issue is that
> it's up to Ulf as the MMC maintainer what the next steps are.  For me,
> at least, his argument that MMC block numbers have already shuffled
> around several times in the last several years is at least some
> evidence that they weren't exactly stable to begin with.  While we
> could go back to the numbers that happened to be chosen as of kernel
> v5.9, if someone was updating from a much older kernel then they may
> have different expectations of what numbers are good / bad I think.
>
> I will also offer one possible suggestion: what about a KConfig option
> here?  In theory we could add a KConfig option like
> "CONFIG_MMC_LEGACY_PROBE" or something that.  One can argue about what
> the default ought to be, but maybe that would satisfy folks?  If you
> were happy giving up a little bit of boot speed to get the v5.9 block
> numbers then you could set this.

This is not limited to MMC.
The same is true for sdX, ethX (e* these days), ttyS*, i2cX, spiX, ...
The rule has always been to handle it by udev, disklabels, ...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [GIT PULL] ARM: SoC fixes for v5.10, part 3

[Ulf Hansson]

On Tue, 8 Dec 2020 at 08:32, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Doug,
>
> On Mon, Dec 7, 2020 at 11:15 PM Doug Anderson <dianders@chromium.org> wrote:
> > On Mon, Dec 7, 2020 at 1:55 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > > On Mon, Dec 7, 2020 at 9:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Tue, Dec 1, 2020 at 3:06 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > > > > On Tue, Dec 1, 2020 at 12:39 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > So, I think we have two options. If people are willing to move to
> > > > > > "disk labels" or to patch their DTBs with mmc aliases, things can stay
> > > > > > as is. Otherwise, we can revert the async probe parts of the mmc host
> > > > > > drivers, but that would still leave us in a fragile situation.
> > > > >
> > > > > Can you reliably detect whether the mmc aliases in the dt exist?
> > > > > If that's possible, maybe the async flag could be masked out to only have
> > > > > an effect when the device number is known.
> > > >
> > > > IMHO DT aliases are not a proper solution for this.
> > > >
> > > > Yes, you can detect reliably if an alias exists in the DT.
> > > > The problems start when having multiple devices, some with aliases,
> > > > some without.  And when devices can appear dynamically (without
> > > > aliases, as there is no support for dynamically updating the aliases
> > > > list).
> > >
> > > Actually you hit a problem earlier than that: the async probe is a
> > > property of the host controller driver, which may be a pci_driver,
> > > platform_driver, usb_driver, or anything else really. To figure out
> > > whether to probe it asynchronously, it would have to be the driver
> > > core, or each bus type that can host these to understand which
> > > device driver is responsible for probing an eMMC device attached
> > > to the host.
> >
> > From what I've seen so far, my current thought on this issue is that
> > it's up to Ulf as the MMC maintainer what the next steps are.  For me,
> > at least, his argument that MMC block numbers have already shuffled
> > around several times in the last several years is at least some
> > evidence that they weren't exactly stable to begin with.  While we
> > could go back to the numbers that happened to be chosen as of kernel
> > v5.9, if someone was updating from a much older kernel then they may
> > have different expectations of what numbers are good / bad I think.
> >
> > I will also offer one possible suggestion: what about a KConfig option
> > here?  In theory we could add a KConfig option like
> > "CONFIG_MMC_LEGACY_PROBE" or something that.  One can argue about what
> > the default ought to be, but maybe that would satisfy folks?  If you
> > were happy giving up a little bit of boot speed to get the v5.9 block
> > numbers then you could set this.
>
> This is not limited to MMC.
> The same is true for sdX, ethX (e* these days), ttyS*, i2cX, spiX, ...
> The rule has always been to handle it by udev, disklabels, ...

That's right.

Although, those rules haven't been sufficient for some cases, which
has been discussed many many times by now. I can dig out some of the
old threads from the mail archive, just tell me and will help to find
them.

For mmc we have decided to improve the situation by adding support for
aliases in DT. The support seems robust enough to me, but if you think
there are problems with it, please tell me and I am happy to help to
improve it.

In regards to adding a new Kconfig option for "legacy probe", I am
open to this if that's what people think is needed. Although, as
pointed out earlier in this thread, it won't move us into a stable
situation. The only solution to get to that point, is either to use
udev/disklabel rules or the mmc aliases in DT.

Kind regards
Uffe

Re: [GIT PULL] ARM: SoC fixes for v5.10, part 3

[Geert Uytterhoeven]

Hi Ulf,

On Mon, Dec 14, 2020 at 9:23 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Tue, 8 Dec 2020 at 08:32, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, Dec 7, 2020 at 11:15 PM Doug Anderson <dianders@chromium.org> wrote:
> > > On Mon, Dec 7, 2020 at 1:55 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > > > On Mon, Dec 7, 2020 at 9:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Tue, Dec 1, 2020 at 3:06 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > > > > > On Tue, Dec 1, 2020 at 12:39 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > > So, I think we have two options. If people are willing to move to
> > > > > > > "disk labels" or to patch their DTBs with mmc aliases, things can stay
> > > > > > > as is. Otherwise, we can revert the async probe parts of the mmc host
> > > > > > > drivers, but that would still leave us in a fragile situation.
> > > > > >
> > > > > > Can you reliably detect whether the mmc aliases in the dt exist?
> > > > > > If that's possible, maybe the async flag could be masked out to only have
> > > > > > an effect when the device number is known.
> > > > >
> > > > > IMHO DT aliases are not a proper solution for this.
> > > > >
> > > > > Yes, you can detect reliably if an alias exists in the DT.
> > > > > The problems start when having multiple devices, some with aliases,
> > > > > some without.  And when devices can appear dynamically (without
> > > > > aliases, as there is no support for dynamically updating the aliases
> > > > > list).
> > > >
> > > > Actually you hit a problem earlier than that: the async probe is a
> > > > property of the host controller driver, which may be a pci_driver,
> > > > platform_driver, usb_driver, or anything else really. To figure out
> > > > whether to probe it asynchronously, it would have to be the driver
> > > > core, or each bus type that can host these to understand which
> > > > device driver is responsible for probing an eMMC device attached
> > > > to the host.
> > >
> > > From what I've seen so far, my current thought on this issue is that
> > > it's up to Ulf as the MMC maintainer what the next steps are.  For me,
> > > at least, his argument that MMC block numbers have already shuffled
> > > around several times in the last several years is at least some
> > > evidence that they weren't exactly stable to begin with.  While we
> > > could go back to the numbers that happened to be chosen as of kernel
> > > v5.9, if someone was updating from a much older kernel then they may
> > > have different expectations of what numbers are good / bad I think.
> > >
> > > I will also offer one possible suggestion: what about a KConfig option
> > > here?  In theory we could add a KConfig option like
> > > "CONFIG_MMC_LEGACY_PROBE" or something that.  One can argue about what
> > > the default ought to be, but maybe that would satisfy folks?  If you
> > > were happy giving up a little bit of boot speed to get the v5.9 block
> > > numbers then you could set this.
> >
> > This is not limited to MMC.
> > The same is true for sdX, ethX (e* these days), ttyS*, i2cX, spiX, ...
> > The rule has always been to handle it by udev, disklabels, ...
>
> That's right.
>
> Although, those rules haven't been sufficient for some cases, which
> has been discussed many many times by now. I can dig out some of the
> old threads from the mail archive, just tell me and will help to find
> them.
>
> For mmc we have decided to improve the situation by adding support for
> aliases in DT. The support seems robust enough to me, but if you think
> there are problems with it, please tell me and I am happy to help to
> improve it.

DT rule #1: DT is hardware description (device naming is software
policy).

The only generally accepted aliases are "serial0" (the first serial
port, as such labeled on the board, to be used for the console;  there
may be more labeled ports, and thus more "serialX" aliases), and
"ethernet0" (the first Ethernet port, so U-Boot knows to which port to
add an appropriate "local-mac-address" property, when booting over the
network).  So yeah, you can claim the first SD card slot is labeled as
such, and thus deserves an alias.  Then the issue is what you do with
the remaining slots, which can be added either statically or
dynamically.  And what if for some reason the labeled MMC slot is not
probed first...

The description of commit 7678f4c20fa7670f ("serial: sh-sci: Add support
for dynamic instances") mentions some background and remaining
issues w.r.t. aliases.

> In regards to adding a new Kconfig option for "legacy probe", I am
> open to this if that's what people think is needed. Although, as
> pointed out earlier in this thread, it won't move us into a stable
> situation. The only solution to get to that point, is either to use
> udev/disklabel rules or the mmc aliases in DT.

IMHO that will be a fragile solution, too: over time, it may become
harder and harder to maintain the original probe order.

And if it will be an option, it means there will be two code paths to
maintain, increasing the burden.



Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [GIT PULL] ARM: SoC fixes for v5.10, part 3

[Ulf Hansson]

On Tue, 15 Dec 2020 at 09:19, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Mon, Dec 14, 2020 at 9:23 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Tue, 8 Dec 2020 at 08:32, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Mon, Dec 7, 2020 at 11:15 PM Doug Anderson <dianders@chromium.org> wrote:
> > > > On Mon, Dec 7, 2020 at 1:55 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > > > > On Mon, Dec 7, 2020 at 9:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > On Tue, Dec 1, 2020 at 3:06 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > > > > > > On Tue, Dec 1, 2020 at 12:39 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > > > So, I think we have two options. If people are willing to move to
> > > > > > > > "disk labels" or to patch their DTBs with mmc aliases, things can stay
> > > > > > > > as is. Otherwise, we can revert the async probe parts of the mmc host
> > > > > > > > drivers, but that would still leave us in a fragile situation.
> > > > > > >
> > > > > > > Can you reliably detect whether the mmc aliases in the dt exist?
> > > > > > > If that's possible, maybe the async flag could be masked out to only have
> > > > > > > an effect when the device number is known.
> > > > > >
> > > > > > IMHO DT aliases are not a proper solution for this.
> > > > > >
> > > > > > Yes, you can detect reliably if an alias exists in the DT.
> > > > > > The problems start when having multiple devices, some with aliases,
> > > > > > some without.  And when devices can appear dynamically (without
> > > > > > aliases, as there is no support for dynamically updating the aliases
> > > > > > list).
> > > > >
> > > > > Actually you hit a problem earlier than that: the async probe is a
> > > > > property of the host controller driver, which may be a pci_driver,
> > > > > platform_driver, usb_driver, or anything else really. To figure out
> > > > > whether to probe it asynchronously, it would have to be the driver
> > > > > core, or each bus type that can host these to understand which
> > > > > device driver is responsible for probing an eMMC device attached
> > > > > to the host.
> > > >
> > > > From what I've seen so far, my current thought on this issue is that
> > > > it's up to Ulf as the MMC maintainer what the next steps are.  For me,
> > > > at least, his argument that MMC block numbers have already shuffled
> > > > around several times in the last several years is at least some
> > > > evidence that they weren't exactly stable to begin with.  While we
> > > > could go back to the numbers that happened to be chosen as of kernel
> > > > v5.9, if someone was updating from a much older kernel then they may
> > > > have different expectations of what numbers are good / bad I think.
> > > >
> > > > I will also offer one possible suggestion: what about a KConfig option
> > > > here?  In theory we could add a KConfig option like
> > > > "CONFIG_MMC_LEGACY_PROBE" or something that.  One can argue about what
> > > > the default ought to be, but maybe that would satisfy folks?  If you
> > > > were happy giving up a little bit of boot speed to get the v5.9 block
> > > > numbers then you could set this.
> > >
> > > This is not limited to MMC.
> > > The same is true for sdX, ethX (e* these days), ttyS*, i2cX, spiX, ...
> > > The rule has always been to handle it by udev, disklabels, ...
> >
> > That's right.
> >
> > Although, those rules haven't been sufficient for some cases, which
> > has been discussed many many times by now. I can dig out some of the
> > old threads from the mail archive, just tell me and will help to find
> > them.
> >
> > For mmc we have decided to improve the situation by adding support for
> > aliases in DT. The support seems robust enough to me, but if you think
> > there are problems with it, please tell me and I am happy to help to
> > improve it.
>
> DT rule #1: DT is hardware description (device naming is software
> policy).

I do admit, the use of aliases in DT, for *all* cases including mmc is
a bit of a stretch. Anyway, it's there.

>
> The only generally accepted aliases are "serial0" (the first serial
> port, as such labeled on the board, to be used for the console;  there
> may be more labeled ports, and thus more "serialX" aliases), and
> "ethernet0" (the first Ethernet port, so U-Boot knows to which port to
> add an appropriate "local-mac-address" property, when booting over the
> network).  So yeah, you can claim the first SD card slot is labeled as
> such, and thus deserves an alias.  Then the issue is what you do with
> the remaining slots, which can be added either statically or
> dynamically.  And what if for some reason the labeled MMC slot is not
> probed first...

The probe order shouldn't matter. By pre-parsing all mmc aliases (for
all slots) existing in the DTB we can keep track of reserved ids. In
this way, for the non-alias case, we can dynamically select a
non-reserved id.

Although, perhaps more improvements can be made.

>
> The description of commit 7678f4c20fa7670f ("serial: sh-sci: Add support
> for dynamic instances") mentions some background and remaining
> issues w.r.t. aliases.
>
> > In regards to adding a new Kconfig option for "legacy probe", I am
> > open to this if that's what people think is needed. Although, as
> > pointed out earlier in this thread, it won't move us into a stable
> > situation. The only solution to get to that point, is either to use
> > udev/disklabel rules or the mmc aliases in DT.
>
> IMHO that will be a fragile solution, too: over time, it may become
> harder and harder to maintain the original probe order.
>
> And if it will be an option, it means there will be two code paths to
> maintain, increasing the burden.

Alright, sounds like we can conclude that the Kconfig option doesn't
seem like a good way forward.

Kind regards
Uffe