Hi Saravana,

On Wed, Jan 27, 2021 at 6:11 PM Saravana Kannan [off-list ref] wrote:

On Wed, Jan 27, 2021 at 8:56 AM Geert Uytterhoeven [off-list ref] wrote:

quoted

On Wed, Jan 27, 2021 at 5:42 PM Saravana Kannan [off-list ref] wrote:

quoted

On Tue, Jan 26, 2021 at 11:43 PM Geert Uytterhoeven
[off-list ref] wrote:

quoted

On Wed, Jan 27, 2021 at 1:44 AM Saravana Kannan [off-list ref] wrote:

quoted

On Tue, Jan 26, 2021 at 12:50 AM Geert Uytterhoeven
[off-list ref] wrote:

quoted

On Mon, Jan 25, 2021 at 11:42 PM Saravana Kannan [off-list ref] wrote:

quoted

On Mon, Jan 25, 2021 at 11:49 AM Michael Walle [off-list ref] wrote:

quoted

Am 2021-01-21 12:01, schrieb Geert Uytterhoeven:

quoted

On Thu, Jan 21, 2021 at 1:05 AM Saravana Kannan [off-list ref]
wrote:

quoted

On Wed, Jan 20, 2021 at 3:53 PM Michael Walle [off-list ref]
wrote:

quoted

Am 2021-01-20 20:47, schrieb Saravana Kannan:

quoted

On Wed, Jan 20, 2021 at 11:28 AM Michael Walle [off-list ref]
wrote:

quoted

[RESEND, fat-fingered the buttons of my mail client and converted
all CCs to BCCs :(]

Am 2021-01-20 20:02, schrieb Saravana Kannan:

quoted

On Wed, Jan 20, 2021 at 6:24 AM Rob Herring [off-list ref] wrote:

quoted

On Wed, Jan 20, 2021 at 4:53 AM Michael Walle [off-list ref]
wrote:

quoted

fw_devlink will defer the probe until all suppliers are ready. We can't
use builtin_platform_driver_probe() because it doesn't retry after probe
deferral. Convert it to builtin_platform_driver().

If builtin_platform_driver_probe() doesn't work with fw_devlink, then
shouldn't it be fixed or removed?

I was actually thinking about this too. The problem with fixing
builtin_platform_driver_probe() to behave like
builtin_platform_driver() is that these probe functions could be
marked with __init. But there are also only 20 instances of
builtin_platform_driver_probe() in the kernel:
$ git grep ^builtin_platform_driver_probe | wc -l
20

So it might be easier to just fix them to not use
builtin_platform_driver_probe().

Michael,

Any chance you'd be willing to help me by converting all these to
builtin_platform_driver() and delete builtin_platform_driver_probe()?

If it just moving the probe function to the _driver struct and
remove the __init annotations. I could look into that.

Yup. That's pretty much it AFAICT.

builtin_platform_driver_probe() also makes sure the driver doesn't ask
for async probe, etc. But I doubt anyone is actually setting async
flags and still using builtin_platform_driver_probe().

Hasn't module_platform_driver_probe() the same problem? And there
are ~80 drivers which uses that.

Yeah. The biggest problem with all of these is the __init markers.
Maybe some familiar with coccinelle can help?

And dropping them will increase memory usage.

Although I do have the changes for the builtin_platform_driver_probe()
ready, I don't think it makes much sense to send these unless we agree
on the increased memory footprint. While there are just a few
builtin_platform_driver_probe() and memory increase _might_ be
negligible, there are many more module_platform_driver_probe().

While it's good to drop code that'll not be used past kernel init, the
module_platform_driver_probe() is going even more extreme. It doesn't
even allow deferred probe (well before kernel init is done). I don't
think that behavior is right and that's why we should delete it. Also,

This construct is typically used for builtin hardware for which the
dependencies are registered very early, and thus known to probe at
first try (if present).

quoted

I doubt if any of these probe functions even take up 4KB of memory.

How many 4 KiB pages do you have in a system with 10 MiB of SRAM?
How many can you afford to waste?

There are only a few instances of this macro in the kernel. How many

$ git grep -lw builtin_platform_driver_probe | wc -l
21
$ git grep -lw module_platform_driver_probe | wc -l
86

+ the ones that haven't been converted to the above yet:

$ git grep -lw platform_driver_probe | wc -l
58

Yeah, this adds up in terms of the number of places we'd need to fix.
But thinking more about it, a couple of points:
1. Not all builtin_platform_driver_probe() are problems for
fw_devlink. So we can just fix them as we go if we need to.

2. The problem with builtin_platform_driver_probe() isn't really with
the use of __init. It's the fact that it doesn't allow deferred
probes. builtin_platform_driver_probe()/platform_driver_probe() could
still be fixed up to allow deferred probe until we get to the point
where we free the __init section (so at least till late_initcall).

That's intentional: it is used for cases that will (must) never be deferred.
That's why it's safe to use __init.

So was the usage of builtin_platform_driver_probe() wrong in the
driver Michael fixed? Because, deferring and probing again clearly
works?

It wasn't.  The regression is that the driver no longer probes at first
try, because its dependencies are now probed later.  The question is:
why are the dependencies now probed later?  How to fix that?

Also, "must never be deferred" seems like a weird condition to
enforce. I think the real "rule" is that if it defers, the platform is
not expected to work. But disallowing a probe reattempt seems weird.
What is it going to hurt if it's attempted again? At worst it fails
one more time?

"must never be deferred" is not the real condition, but "must not be
probed after __init is freed" is (one of them, there may be other, cfr.
the last paragraph below).  The simplest way to guarantee that is to
probe the driver immediately, and only once.

Also, I'd argue that all/most of the "can't defer, but I'm still a
proper struct device" cases are all just patchwork to deal with the
fact we were playing initcall chicken when there was no fw_devlink.
I'm hoping we can move people away from that mindset. And the first

I agree, partially.  Still, how come the dependencies are no longer
probed before their consumers when fw_devlinks are enabled?
I thought fw_devlinks is supposed to avoid exactly that?

step towards that would be to allow *platform_probe() to allow
deferred probes until late_initcall().

At which increase of complexity, to keep track of which drivers can and
cannot be reprobed anymore after late_initcall?
Still, many of these drivers use platform_driver_probe() early for a
reason: because they need to initialize the hardware early. Probing them
later may introduce subtle bugs.

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