diff mbox series

PCI: dwc: layerscape: convert to builtin_platform_driver()

Message ID 20210120105246.23218-1-michael@walle.cc
State New
Headers show
Series PCI: dwc: layerscape: convert to builtin_platform_driver() | expand

Checks

Context Check Description
snowpatch_ozlabs/needsstable fail
snowpatch_ozlabs/checkpatch warning total: 0 errors, 1 warnings, 0 checks, 20 lines checked
snowpatch_ozlabs/build-pmac32 success Build succeeded
snowpatch_ozlabs/build-ppc64e success Build succeeded
snowpatch_ozlabs/build-ppc64be success Build succeeded
snowpatch_ozlabs/build-ppc64le success Build succeeded
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (41d8cb7ece7c81e4eb897ed7ec7d3c3d72fd0af4)

Commit Message

Michael Walle Jan. 20, 2021, 10:52 a.m. UTC
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().

Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pci/controller/dwc/pci-layerscape.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Rob Herring Jan. 20, 2021, 2:23 p.m. UTC | #1
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? Then we're not fixing drivers later
when folks start caring about deferred probe and devlink.

I'd really prefer if we convert this to a module instead. (And all the
other PCI host drivers.)

> Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")

This happened!?

> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/pci/controller/dwc/pci-layerscape.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
> index 44ad34cdc3bc..5b9c625df7b8 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> @@ -232,7 +232,7 @@ static const struct of_device_id ls_pcie_of_match[] = {
>         { },
>  };
>
> -static int __init ls_pcie_probe(struct platform_device *pdev)
> +static int ls_pcie_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
>         struct dw_pcie *pci;
> @@ -271,10 +271,11 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
>  }
>
>  static struct platform_driver ls_pcie_driver = {
> +       .probe = ls_pcie_probe,
>         .driver = {
>                 .name = "layerscape-pcie",
>                 .of_match_table = ls_pcie_of_match,
>                 .suppress_bind_attrs = true,
>         },
>  };
> -builtin_platform_driver_probe(ls_pcie_driver, ls_pcie_probe);
> +builtin_platform_driver(ls_pcie_driver);
> --
> 2.20.1
>
Michael Walle Jan. 20, 2021, 2:34 p.m. UTC | #2
Am 2021-01-20 15:23, schrieb Rob Herring:
> 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? Then we're not fixing drivers later
> when folks start caring about deferred probe and devlink.
> 
> I'd really prefer if we convert this to a module instead. (And all the
> other PCI host drivers.)

I briefly looked into adding a remove() op. But I don't know what will
have to be undo of the ls_pcie_host_init(). That would be up to the
maintainers of this driver.

_But_ I'd really like to see PCI working on my board again ;) At
least until the end of this development cycle.

-michael

>> Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
> 
> This happened!?
> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/pci/controller/dwc/pci-layerscape.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/pci/controller/dwc/pci-layerscape.c 
>> b/drivers/pci/controller/dwc/pci-layerscape.c
>> index 44ad34cdc3bc..5b9c625df7b8 100644
>> --- a/drivers/pci/controller/dwc/pci-layerscape.c
>> +++ b/drivers/pci/controller/dwc/pci-layerscape.c
>> @@ -232,7 +232,7 @@ static const struct of_device_id 
>> ls_pcie_of_match[] = {
>>         { },
>>  };
>> 
>> -static int __init ls_pcie_probe(struct platform_device *pdev)
>> +static int ls_pcie_probe(struct platform_device *pdev)
>>  {
>>         struct device *dev = &pdev->dev;
>>         struct dw_pcie *pci;
>> @@ -271,10 +271,11 @@ static int __init ls_pcie_probe(struct 
>> platform_device *pdev)
>>  }
>> 
>>  static struct platform_driver ls_pcie_driver = {
>> +       .probe = ls_pcie_probe,
>>         .driver = {
>>                 .name = "layerscape-pcie",
>>                 .of_match_table = ls_pcie_of_match,
>>                 .suppress_bind_attrs = true,
>>         },
>>  };
>> -builtin_platform_driver_probe(ls_pcie_driver, ls_pcie_probe);
>> +builtin_platform_driver(ls_pcie_driver);
>> --
>> 2.20.1
>>
Greg KH Jan. 20, 2021, 3:50 p.m. UTC | #3
On Wed, Jan 20, 2021 at 08:23:59AM -0600, Rob Herring 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? Then we're not fixing drivers later
> when folks start caring about deferred probe and devlink.
> 
> I'd really prefer if we convert this to a module instead. (And all the
> other PCI host drivers.)
> 
> > Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
> 
> This happened!?

It's in linux-next in my tree, but is looking like it might be reverted
soon.  But finding these issues is good.

thanks,

greg k-h
Saravana Kannan Jan. 20, 2021, 7:02 p.m. UTC | #4
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()?

-Saravana

> Then we're not fixing drivers later
> when folks start caring about deferred probe and devlink.
>
> I'd really prefer if we convert this to a module instead. (And all the
> other PCI host drivers.)
>
> > Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
>
> This happened!?
>
> > Signed-off-by: Michael Walle <michael@walle.cc>
> > ---
> >  drivers/pci/controller/dwc/pci-layerscape.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
> > index 44ad34cdc3bc..5b9c625df7b8 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> > @@ -232,7 +232,7 @@ static const struct of_device_id ls_pcie_of_match[] = {
> >         { },
> >  };
> >
> > -static int __init ls_pcie_probe(struct platform_device *pdev)
> > +static int ls_pcie_probe(struct platform_device *pdev)
> >  {
> >         struct device *dev = &pdev->dev;
> >         struct dw_pcie *pci;
> > @@ -271,10 +271,11 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
> >  }
> >
> >  static struct platform_driver ls_pcie_driver = {
> > +       .probe = ls_pcie_probe,
> >         .driver = {
> >                 .name = "layerscape-pcie",
> >                 .of_match_table = ls_pcie_of_match,
> >                 .suppress_bind_attrs = true,
> >         },
> >  };
> > -builtin_platform_driver_probe(ls_pcie_driver, ls_pcie_probe);
> > +builtin_platform_driver(ls_pcie_driver);
> > --
> > 2.20.1
> >
Michael Walle Jan. 20, 2021, 7:25 p.m. UTC | #5
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.

-michael
Michael Walle Jan. 20, 2021, 7:28 p.m. UTC | #6
[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.

-michael
Saravana Kannan Jan. 20, 2021, 7:47 p.m. UTC | #7
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().

-Saravana
Saravana Kannan Jan. 20, 2021, 7:47 p.m. UTC | #8
On Wed, Jan 20, 2021 at 11:47 AM Saravana Kannan <saravanak@google.com> wrote:
>
> 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().
>

And thanks for agreeing to help!

-Saravana
Michael Walle Jan. 20, 2021, 11:53 p.m. UTC | #9
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.

-michael
Saravana Kannan Jan. 20, 2021, 11:58 p.m. UTC | #10
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?

-Saravana
Geert Uytterhoeven Jan. 21, 2021, 11:01 a.m. UTC | #11
Hi Saravana,

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.

Gr{oetje,eeting}s,

                        Geert
Lorenzo Pieralisi Jan. 25, 2021, 4:50 p.m. UTC | #12
On Wed, Jan 20, 2021 at 08:28:36PM +0100, Michael Walle 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.

Can I drop this patch then ?

Thanks,
Lorenzo
Saravana Kannan Jan. 25, 2021, 6:58 p.m. UTC | #13
On Mon, Jan 25, 2021 at 8:50 AM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Wed, Jan 20, 2021 at 08:28:36PM +0100, Michael Walle 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.
>
> Can I drop this patch then ?

No, please pick it up. Michael and I were talking about doing similar
changes for other drivers.

-Saravana
Michael Walle Jan. 25, 2021, 7:44 p.m. UTC | #14
Am 2021-01-25 19:58, schrieb Saravana Kannan:
> On Mon, Jan 25, 2021 at 8:50 AM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
>> 
>> On Wed, Jan 20, 2021 at 08:28:36PM +0100, Michael Walle 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.
>> 
>> Can I drop this patch then ?
> 
> No, please pick it up. Michael and I were talking about doing similar
> changes for other drivers.

Yes please, I was just about to answer, but Saravana beat me.

-michael
Michael Walle Jan. 25, 2021, 7:49 p.m. UTC | #15
Am 2021-01-21 12:01, schrieb Geert Uytterhoeven:
> Hi Saravana,
> 
> 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().

-michael
Saravana Kannan Jan. 25, 2021, 10:41 p.m. UTC | #16
On Mon, Jan 25, 2021 at 11:49 AM Michael Walle <michael@walle.cc> wrote:
>
> Am 2021-01-21 12:01, schrieb Geert Uytterhoeven:
> > Hi Saravana,
> >
> > 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,
I doubt if any of these probe functions even take up 4KB of memory.

-Saravana
Geert Uytterhoeven Jan. 26, 2021, 8:50 a.m. UTC | #17
Hi Saravana,

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?

Gr{oetje,eeting}s,

                        Geert
Lorenzo Pieralisi Jan. 26, 2021, 10:02 a.m. UTC | #18
On Wed, Jan 20, 2021 at 11:52:46AM +0100, Michael Walle 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().
> 
> Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")

I will have to drop this Fixes: tag if you don't mind, it is not
in the mainline.

Lorenzo

> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/pci/controller/dwc/pci-layerscape.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
> index 44ad34cdc3bc..5b9c625df7b8 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> @@ -232,7 +232,7 @@ static const struct of_device_id ls_pcie_of_match[] = {
>  	{ },
>  };
>  
> -static int __init ls_pcie_probe(struct platform_device *pdev)
> +static int ls_pcie_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct dw_pcie *pci;
> @@ -271,10 +271,11 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
>  }
>  
>  static struct platform_driver ls_pcie_driver = {
> +	.probe = ls_pcie_probe,
>  	.driver = {
>  		.name = "layerscape-pcie",
>  		.of_match_table = ls_pcie_of_match,
>  		.suppress_bind_attrs = true,
>  	},
>  };
> -builtin_platform_driver_probe(ls_pcie_driver, ls_pcie_probe);
> +builtin_platform_driver(ls_pcie_driver);
> -- 
> 2.20.1
>
Michael Walle Jan. 26, 2021, 10:39 a.m. UTC | #19
Am 2021-01-26 11:02, schrieb Lorenzo Pieralisi:
> On Wed, Jan 20, 2021 at 11:52:46AM +0100, Michael Walle 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().
>> 
>> Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
> 
> I will have to drop this Fixes: tag if you don't mind, it is not
> in the mainline.

That commit is in Greg's for-next tree:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next&id=e590474768f1cc04852190b61dec692411b22e2a

I was under the impression there are other commits with this
particular fixes tag, too. Either it was removed from
for-next queues or I was confused.

But I'm fine with removing the tag, assuming this will end
up together with the "driver core: Set fw_devlink=on by default"
commit in 5.11.

-michael
Lorenzo Pieralisi Jan. 26, 2021, 10:55 a.m. UTC | #20
On Wed, 20 Jan 2021 11:52:46 +0100, Michael Walle 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().

Applied to pci/dwc, thanks!

[1/1] PCI: dwc: layerscape: Convert to builtin_platform_driver()
      https://git.kernel.org/lpieralisi/pci/c/538157be1e

Thanks,
Lorenzo
Geert Uytterhoeven Jan. 26, 2021, 10:56 a.m. UTC | #21
Hi Michael,

On Tue, Jan 26, 2021 at 11:46 AM Michael Walle <michael@walle.cc> wrote:
> Am 2021-01-26 11:02, schrieb Lorenzo Pieralisi:
> > On Wed, Jan 20, 2021 at 11:52:46AM +0100, Michael Walle 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().
> >>
> >> Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
> >
> > I will have to drop this Fixes: tag if you don't mind, it is not
> > in the mainline.
>
> That commit is in Greg's for-next tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next&id=e590474768f1cc04852190b61dec692411b22e2a
>
> I was under the impression there are other commits with this
> particular fixes tag, too. Either it was removed from
> for-next queues or I was confused.
>
> But I'm fine with removing the tag, assuming this will end
> up together with the "driver core: Set fw_devlink=on by default"
> commit in 5.11.

Definitely not v5.11.

And I sincerely doubt it will be applied for v5.12.
It's already way too late to implement all changes to existing drivers
needed, and get them accepted for v5.12.

Gr{oetje,eeting}s,

                        Geert
Saravana Kannan Jan. 27, 2021, 12:44 a.m. UTC | #22
On Tue, Jan 26, 2021 at 12:50 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> 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
of those actually fit the description above? We can probably just
check the DT?

-Saravana
Geert Uytterhoeven Jan. 27, 2021, 7:43 a.m. UTC | #23
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

> of those actually fit the description above? We can probably just
> check the DT?

What do you mean by checking the DT?

Gr{oetje,eeting}s,

                        Geert
Saravana Kannan Jan. 27, 2021, 4:41 p.m. UTC | #24
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
Geert Uytterhoeven Jan. 27, 2021, 4:56 p.m. UTC | #25
Hi Saravana,

On Wed, Jan 27, 2021 at 5:42 PM Saravana Kannan <saravanak@google.com> wrote:
> On Tue, Jan 26, 2021 at 11:43 PM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > 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).

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

Gr{oetje,eeting}s,

                        Geert
Saravana Kannan Jan. 27, 2021, 5:10 p.m. UTC | #26
On Wed, Jan 27, 2021 at 8:56 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Wed, Jan 27, 2021 at 5:42 PM Saravana Kannan <saravanak@google.com> wrote:
> > On Tue, Jan 26, 2021 at 11:43 PM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > 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).
>
> 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?

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?

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
step towards that would be to allow *platform_probe() to allow
deferred probes until late_initcall().


-Saravana
Geert Uytterhoeven Jan. 28, 2021, 9:25 a.m. UTC | #27
Hi Saravana,

On Wed, Jan 27, 2021 at 6:11 PM Saravana Kannan <saravanak@google.com> wrote:
> On Wed, Jan 27, 2021 at 8:56 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Jan 27, 2021 at 5:42 PM Saravana Kannan <saravanak@google.com> wrote:
> > > On Tue, Jan 26, 2021 at 11:43 PM Geert Uytterhoeven
> > > <geert@linux-m68k.org> wrote:
> > > > 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).
> >
> > 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
Tony Lindgren Jan. 28, 2021, 10 a.m. UTC | #28
Hi,

* Michael Walle <michael@walle.cc> [210125 19:52]:
> 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().

I just noticed this thread today and have pretty much come to the same
conclusions. No need to post a patch for pci-dra7xx.c, I already posted
a patch for pci-dra7xx.c yesterday as part of genpd related changes.

For me probing started breaking as the power-domains property got added.

FYI, it's the following patch:

[PATCH 01/15] PCI: pci-dra7xx: Prepare for deferred probe with module_platform_driver

Regards,

Tony
Tony Lindgren Jan. 28, 2021, 10:35 a.m. UTC | #29
* Geert Uytterhoeven <geert@linux-m68k.org> [210128 09:32]:
> 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?

I'm afraid that may be unfixable.. It depends on things like the bus
driver probe that might get also deferred.

As suggested, I agree it's best to get rid of builtin_platform_driver_probe
where possible at the cost of dropping the __init as needed.

To me it seems we can't even add a warning to __platform_driver_probe()
if there's drv->driver.of_match_table for example. That warning would
show up on all the devices with driver in question built in even if
the device has no such hardware.

Regards,

Tony
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
index 44ad34cdc3bc..5b9c625df7b8 100644
--- a/drivers/pci/controller/dwc/pci-layerscape.c
+++ b/drivers/pci/controller/dwc/pci-layerscape.c
@@ -232,7 +232,7 @@  static const struct of_device_id ls_pcie_of_match[] = {
 	{ },
 };
 
-static int __init ls_pcie_probe(struct platform_device *pdev)
+static int ls_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct dw_pcie *pci;
@@ -271,10 +271,11 @@  static int __init ls_pcie_probe(struct platform_device *pdev)
 }
 
 static struct platform_driver ls_pcie_driver = {
+	.probe = ls_pcie_probe,
 	.driver = {
 		.name = "layerscape-pcie",
 		.of_match_table = ls_pcie_of_match,
 		.suppress_bind_attrs = true,
 	},
 };
-builtin_platform_driver_probe(ls_pcie_driver, ls_pcie_probe);
+builtin_platform_driver(ls_pcie_driver);