Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()

[Saravana Kannan <saravanak@google.com>]

On Tue, Jan 26, 2021 at 11:43 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Wed, Jan 27, 2021 at 1:44 AM Saravana Kannan <saravanak@google.com> wrote:
> > On Tue, Jan 26, 2021 at 12:50 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Mon, Jan 25, 2021 at 11:42 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > On Mon, Jan 25, 2021 at 11:49 AM Michael Walle <michael@walle.cc> wrote:
> > > > > Am 2021-01-21 12:01, schrieb Geert Uytterhoeven:
> > > > > > On Thu, Jan 21, 2021 at 1:05 AM Saravana Kannan <saravanak@google.com>
> > > > > > wrote:
> > > > > >> On Wed, Jan 20, 2021 at 3:53 PM Michael Walle <michael@walle.cc>
> > > > > >> wrote:
> > > > > >> > Am 2021-01-20 20:47, schrieb Saravana Kannan:
> > > > > >> > > On Wed, Jan 20, 2021 at 11:28 AM Michael Walle <michael@walle.cc>
> > > > > >> > > wrote:
> > > > > >> > >>
> > > > > >> > >> [RESEND, fat-fingered the buttons of my mail client and converted
> > > > > >> > >> all CCs to BCCs :(]
> > > > > >> > >>
> > > > > >> > >> Am 2021-01-20 20:02, schrieb Saravana Kannan:
> > > > > >> > >> > On Wed, Jan 20, 2021 at 6:24 AM Rob Herring <robh@kernel.org> wrote:
> > > > > >> > >> >>
> > > > > >> > >> >> On Wed, Jan 20, 2021 at 4:53 AM Michael Walle <michael@walle.cc>
> > > > > >> > >> >> wrote:
> > > > > >> > >> >> >
> > > > > >> > >> >> > 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).
> > >
> > > > 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).

> > of those actually fit the description above? We can probably just
> > check the DT?
>
> What do you mean by checking the DT?

I was talking about checking the DT to see if the board has very
little memory, but that's not always obvious from DT nor does it scale
with the number of instances we have. So, ignore this comment.

Anyway, time to get back to actually writing the code to deal with
this and other corner cases.

-Saravana