linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
@ 2021-01-20 10:52 Michael Walle
  2021-01-20 14:23 ` Rob Herring
                   ` (2 more replies)
  0 siblings, 3 replies; 30+ messages in thread
From: Michael Walle @ 2021-01-20 10:52 UTC (permalink / raw)
  To: linuxppc-dev, linux-pci, linux-arm-kernel, linux-kernel
  Cc: Minghuan Lian, Mingkai Hu, Roy Zang, Lorenzo Pieralisi,
	Rob Herring, Bjorn Helgaas, Greg Kroah-Hartman, Saravana Kannan,
	Michael Walle

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(-)

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


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 10:52 [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver() Michael Walle
@ 2021-01-20 14:23 ` Rob Herring
  2021-01-20 14:34   ` Michael Walle
                     ` (2 more replies)
  2021-01-26 10:02 ` Lorenzo Pieralisi
  2021-01-26 10:55 ` Lorenzo Pieralisi
  2 siblings, 3 replies; 30+ messages in thread
From: Rob Herring @ 2021-01-20 14:23 UTC (permalink / raw)
  To: Michael Walle
  Cc: linuxppc-dev, PCI, linux-arm-kernel, linux-kernel, Minghuan Lian,
	Mingkai Hu, Roy Zang, Lorenzo Pieralisi, Bjorn Helgaas,
	Greg Kroah-Hartman, Saravana Kannan

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
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 14:23 ` Rob Herring
@ 2021-01-20 14:34   ` Michael Walle
  2021-01-20 15:50   ` Greg Kroah-Hartman
  2021-01-20 19:02   ` Saravana Kannan
  2 siblings, 0 replies; 30+ messages in thread
From: Michael Walle @ 2021-01-20 14:34 UTC (permalink / raw)
  To: Rob Herring
  Cc: linuxppc-dev, PCI, linux-arm-kernel, linux-kernel, Minghuan Lian,
	Mingkai Hu, Roy Zang, Lorenzo Pieralisi, Bjorn Helgaas,
	Greg Kroah-Hartman, Saravana Kannan

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 14:23 ` Rob Herring
  2021-01-20 14:34   ` Michael Walle
@ 2021-01-20 15:50   ` Greg Kroah-Hartman
  2021-01-20 19:02   ` Saravana Kannan
  2 siblings, 0 replies; 30+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-20 15:50 UTC (permalink / raw)
  To: Rob Herring
  Cc: Michael Walle, linuxppc-dev, PCI, linux-arm-kernel, linux-kernel,
	Minghuan Lian, Mingkai Hu, Roy Zang, Lorenzo Pieralisi,
	Bjorn Helgaas, Saravana Kannan

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 14:23 ` Rob Herring
  2021-01-20 14:34   ` Michael Walle
  2021-01-20 15:50   ` Greg Kroah-Hartman
@ 2021-01-20 19:02   ` Saravana Kannan
  2021-01-20 19:25     ` Michael Walle
  2021-01-20 19:28     ` Michael Walle
  2 siblings, 2 replies; 30+ messages in thread
From: Saravana Kannan @ 2021-01-20 19:02 UTC (permalink / raw)
  To: Rob Herring
  Cc: Michael Walle, linuxppc-dev, PCI, linux-arm-kernel, linux-kernel,
	Minghuan Lian, Mingkai Hu, Roy Zang, Lorenzo Pieralisi,
	Bjorn Helgaas, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 19:02   ` Saravana Kannan
@ 2021-01-20 19:25     ` Michael Walle
  2021-01-20 19:28     ` Michael Walle
  1 sibling, 0 replies; 30+ messages in thread
From: Michael Walle @ 2021-01-20 19:25 UTC (permalink / raw)


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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 19:02   ` Saravana Kannan
  2021-01-20 19:25     ` Michael Walle
@ 2021-01-20 19:28     ` Michael Walle
  2021-01-20 19:47       ` Saravana Kannan
  2021-01-25 16:50       ` Lorenzo Pieralisi
  1 sibling, 2 replies; 30+ messages in thread
From: Michael Walle @ 2021-01-20 19:28 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Rob Herring, linuxppc-dev, PCI, linux-arm-kernel, linux-kernel,
	Minghuan Lian, Mingkai Hu, Roy Zang, Lorenzo Pieralisi,
	Bjorn Helgaas, Greg Kroah-Hartman

[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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 19:28     ` Michael Walle
@ 2021-01-20 19:47       ` Saravana Kannan
  2021-01-20 19:47         ` Saravana Kannan
  2021-01-20 23:53         ` Michael Walle
  2021-01-25 16:50       ` Lorenzo Pieralisi
  1 sibling, 2 replies; 30+ messages in thread
From: Saravana Kannan @ 2021-01-20 19:47 UTC (permalink / raw)
  To: Michael Walle
  Cc: Rob Herring, linuxppc-dev, PCI, linux-arm-kernel, LKML,
	Minghuan Lian, Mingkai Hu, Roy Zang, Lorenzo Pieralisi,
	Bjorn Helgaas, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 19:47       ` Saravana Kannan
@ 2021-01-20 19:47         ` Saravana Kannan
  2021-01-20 23:53         ` Michael Walle
  1 sibling, 0 replies; 30+ messages in thread
From: Saravana Kannan @ 2021-01-20 19:47 UTC (permalink / raw)
  To: Michael Walle
  Cc: Rob Herring, linuxppc-dev, PCI, linux-arm-kernel, LKML,
	Minghuan Lian, Mingkai Hu, Roy Zang, Lorenzo Pieralisi,
	Bjorn Helgaas, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 19:47       ` Saravana Kannan
  2021-01-20 19:47         ` Saravana Kannan
@ 2021-01-20 23:53         ` Michael Walle
  2021-01-20 23:58           ` Saravana Kannan
  1 sibling, 1 reply; 30+ messages in thread
From: Michael Walle @ 2021-01-20 23:53 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Rob Herring, linuxppc-dev, PCI, linux-arm-kernel, LKML,
	Minghuan Lian, Mingkai Hu, Roy Zang, Lorenzo Pieralisi,
	Bjorn Helgaas, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 23:53         ` Michael Walle
@ 2021-01-20 23:58           ` Saravana Kannan
  2021-01-21 11:01             ` Geert Uytterhoeven
  0 siblings, 1 reply; 30+ messages in thread
From: Saravana Kannan @ 2021-01-20 23:58 UTC (permalink / raw)
  To: Michael Walle
  Cc: Rob Herring, linuxppc-dev, PCI, linux-arm-kernel, LKML,
	Minghuan Lian, Mingkai Hu, Roy Zang, Lorenzo Pieralisi,
	Bjorn Helgaas, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 23:58           ` Saravana Kannan
@ 2021-01-21 11:01             ` Geert Uytterhoeven
  2021-01-25 19:49               ` Michael Walle
  0 siblings, 1 reply; 30+ messages in thread
From: Geert Uytterhoeven @ 2021-01-21 11:01 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Michael Walle, Lorenzo Pieralisi, Roy Zang, PCI, LKML,
	Minghuan Lian, Mingkai Hu, Greg Kroah-Hartman, Bjorn Helgaas,
	linuxppc-dev, linux-arm-kernel

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

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 19:28     ` Michael Walle
  2021-01-20 19:47       ` Saravana Kannan
@ 2021-01-25 16:50       ` Lorenzo Pieralisi
  2021-01-25 18:58         ` Saravana Kannan
  1 sibling, 1 reply; 30+ messages in thread
From: Lorenzo Pieralisi @ 2021-01-25 16:50 UTC (permalink / raw)
  To: Michael Walle
  Cc: Saravana Kannan, Rob Herring, linuxppc-dev, PCI,
	linux-arm-kernel, linux-kernel, Minghuan Lian, Mingkai Hu,
	Roy Zang, Bjorn Helgaas, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-25 16:50       ` Lorenzo Pieralisi
@ 2021-01-25 18:58         ` Saravana Kannan
  2021-01-25 19:44           ` Michael Walle
  0 siblings, 1 reply; 30+ messages in thread
From: Saravana Kannan @ 2021-01-25 18:58 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Michael Walle, Rob Herring, linuxppc-dev, PCI, linux-arm-kernel,
	LKML, Minghuan Lian, Mingkai Hu, Roy Zang, Bjorn Helgaas,
	Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-25 18:58         ` Saravana Kannan
@ 2021-01-25 19:44           ` Michael Walle
  0 siblings, 0 replies; 30+ messages in thread
From: Michael Walle @ 2021-01-25 19:44 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Lorenzo Pieralisi, Rob Herring, linuxppc-dev, PCI,
	linux-arm-kernel, LKML, Minghuan Lian, Mingkai Hu, Roy Zang,
	Bjorn Helgaas, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-21 11:01             ` Geert Uytterhoeven
@ 2021-01-25 19:49               ` Michael Walle
  2021-01-25 22:41                 ` Saravana Kannan
  2021-01-28 10:00                 ` Tony Lindgren
  0 siblings, 2 replies; 30+ messages in thread
From: Michael Walle @ 2021-01-25 19:49 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Saravana Kannan, Lorenzo Pieralisi, Roy Zang, PCI, LKML,
	Minghuan Lian, Mingkai Hu, Greg Kroah-Hartman, Bjorn Helgaas,
	linuxppc-dev, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-25 19:49               ` Michael Walle
@ 2021-01-25 22:41                 ` Saravana Kannan
  2021-01-26  8:50                   ` Geert Uytterhoeven
  2021-01-28 10:00                 ` Tony Lindgren
  1 sibling, 1 reply; 30+ messages in thread
From: Saravana Kannan @ 2021-01-25 22:41 UTC (permalink / raw)
  To: Michael Walle
  Cc: Geert Uytterhoeven, Lorenzo Pieralisi, Roy Zang, PCI, LKML,
	Minghuan Lian, Mingkai Hu, Greg Kroah-Hartman, Bjorn Helgaas,
	linuxppc-dev, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-25 22:41                 ` Saravana Kannan
@ 2021-01-26  8:50                   ` Geert Uytterhoeven
  2021-01-27  0:44                     ` Saravana Kannan
  0 siblings, 1 reply; 30+ messages in thread
From: Geert Uytterhoeven @ 2021-01-26  8:50 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Michael Walle, Lorenzo Pieralisi, Roy Zang, PCI, LKML,
	Minghuan Lian, Mingkai Hu, Greg Kroah-Hartman, Bjorn Helgaas,
	linuxppc-dev, linux-arm-kernel

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

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 10:52 [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver() Michael Walle
  2021-01-20 14:23 ` Rob Herring
@ 2021-01-26 10:02 ` Lorenzo Pieralisi
  2021-01-26 10:39   ` Michael Walle
  2021-01-26 10:55 ` Lorenzo Pieralisi
  2 siblings, 1 reply; 30+ messages in thread
From: Lorenzo Pieralisi @ 2021-01-26 10:02 UTC (permalink / raw)
  To: Michael Walle
  Cc: linuxppc-dev, linux-pci, linux-arm-kernel, linux-kernel,
	Minghuan Lian, Mingkai Hu, Roy Zang, Rob Herring, Bjorn Helgaas,
	Greg Kroah-Hartman, Saravana Kannan

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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-26 10:02 ` Lorenzo Pieralisi
@ 2021-01-26 10:39   ` Michael Walle
  2021-01-26 10:56     ` Geert Uytterhoeven
  0 siblings, 1 reply; 30+ messages in thread
From: Michael Walle @ 2021-01-26 10:39 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linuxppc-dev, linux-pci, linux-arm-kernel, linux-kernel,
	Minghuan Lian, Mingkai Hu, Roy Zang, Rob Herring, Bjorn Helgaas,
	Greg Kroah-Hartman, Saravana Kannan

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-20 10:52 [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver() Michael Walle
  2021-01-20 14:23 ` Rob Herring
  2021-01-26 10:02 ` Lorenzo Pieralisi
@ 2021-01-26 10:55 ` Lorenzo Pieralisi
  2 siblings, 0 replies; 30+ messages in thread
From: Lorenzo Pieralisi @ 2021-01-26 10:55 UTC (permalink / raw)
  To: linux-kernel, Michael Walle, linux-pci, linux-arm-kernel, linuxppc-dev
  Cc: Lorenzo Pieralisi, Mingkai Hu, Greg Kroah-Hartman,
	Saravana Kannan, Bjorn Helgaas, Minghuan Lian, Rob Herring,
	Roy Zang

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-26 10:39   ` Michael Walle
@ 2021-01-26 10:56     ` Geert Uytterhoeven
  0 siblings, 0 replies; 30+ messages in thread
From: Geert Uytterhoeven @ 2021-01-26 10:56 UTC (permalink / raw)
  To: Michael Walle
  Cc: Lorenzo Pieralisi, Roy Zang, Saravana Kannan, linux-pci,
	Linux Kernel Mailing List, Minghuan Lian, Linux ARM,
	Greg Kroah-Hartman, Bjorn Helgaas, linuxppc-dev, Mingkai Hu

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

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-26  8:50                   ` Geert Uytterhoeven
@ 2021-01-27  0:44                     ` Saravana Kannan
  2021-01-27  7:43                       ` Geert Uytterhoeven
  0 siblings, 1 reply; 30+ messages in thread
From: Saravana Kannan @ 2021-01-27  0:44 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Michael Walle, Lorenzo Pieralisi, Roy Zang, PCI, LKML,
	Minghuan Lian, Mingkai Hu, Greg Kroah-Hartman, Bjorn Helgaas,
	linuxppc-dev, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-27  0:44                     ` Saravana Kannan
@ 2021-01-27  7:43                       ` Geert Uytterhoeven
  2021-01-27 16:41                         ` Saravana Kannan
  0 siblings, 1 reply; 30+ messages in thread
From: Geert Uytterhoeven @ 2021-01-27  7:43 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Michael Walle, Lorenzo Pieralisi, Roy Zang, PCI, LKML,
	Minghuan Lian, Mingkai Hu, Greg Kroah-Hartman, Bjorn Helgaas,
	linuxppc-dev, linux-arm-kernel

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

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-27  7:43                       ` Geert Uytterhoeven
@ 2021-01-27 16:41                         ` Saravana Kannan
  2021-01-27 16:56                           ` Geert Uytterhoeven
  0 siblings, 1 reply; 30+ messages in thread
From: Saravana Kannan @ 2021-01-27 16:41 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Michael Walle, Lorenzo Pieralisi, Roy Zang, PCI, LKML,
	Minghuan Lian, Mingkai Hu, Greg Kroah-Hartman, Bjorn Helgaas,
	linuxppc-dev, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-27 16:41                         ` Saravana Kannan
@ 2021-01-27 16:56                           ` Geert Uytterhoeven
  2021-01-27 17:10                             ` Saravana Kannan
  0 siblings, 1 reply; 30+ messages in thread
From: Geert Uytterhoeven @ 2021-01-27 16:56 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Michael Walle, Lorenzo Pieralisi, Roy Zang, PCI, LKML,
	Minghuan Lian, Mingkai Hu, Greg Kroah-Hartman, Bjorn Helgaas,
	linuxppc-dev, linux-arm-kernel

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

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-27 16:56                           ` Geert Uytterhoeven
@ 2021-01-27 17:10                             ` Saravana Kannan
  2021-01-28  9:25                               ` Geert Uytterhoeven
  0 siblings, 1 reply; 30+ messages in thread
From: Saravana Kannan @ 2021-01-27 17:10 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Michael Walle, Lorenzo Pieralisi, Roy Zang, PCI, LKML,
	Minghuan Lian, Mingkai Hu, Greg Kroah-Hartman, Bjorn Helgaas,
	linuxppc-dev, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-27 17:10                             ` Saravana Kannan
@ 2021-01-28  9:25                               ` Geert Uytterhoeven
  2021-01-28 10:35                                 ` Tony Lindgren
  0 siblings, 1 reply; 30+ messages in thread
From: Geert Uytterhoeven @ 2021-01-28  9:25 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Michael Walle, Lorenzo Pieralisi, Roy Zang, PCI, LKML,
	Minghuan Lian, Mingkai Hu, Greg Kroah-Hartman, Bjorn Helgaas,
	linuxppc-dev, linux-arm-kernel

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

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-25 19:49               ` Michael Walle
  2021-01-25 22:41                 ` Saravana Kannan
@ 2021-01-28 10:00                 ` Tony Lindgren
  1 sibling, 0 replies; 30+ messages in thread
From: Tony Lindgren @ 2021-01-28 10:00 UTC (permalink / raw)
  To: Michael Walle
  Cc: Geert Uytterhoeven, Roy Zang, Lorenzo Pieralisi, Saravana Kannan,
	PCI, LKML, Minghuan Lian, linux-arm-kernel, Greg Kroah-Hartman,
	Bjorn Helgaas, linuxppc-dev, Mingkai Hu, Kishon Vijay Abraham I

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



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver()
  2021-01-28  9:25                               ` Geert Uytterhoeven
@ 2021-01-28 10:35                                 ` Tony Lindgren
  0 siblings, 0 replies; 30+ messages in thread
From: Tony Lindgren @ 2021-01-28 10:35 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Saravana Kannan, Roy Zang, Lorenzo Pieralisi, PCI, LKML,
	Minghuan Lian, Michael Walle, linux-arm-kernel,
	Greg Kroah-Hartman, Bjorn Helgaas, linuxppc-dev, Mingkai Hu

* 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

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2021-01-28 10:36 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-20 10:52 [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver() Michael Walle
2021-01-20 14:23 ` Rob Herring
2021-01-20 14:34   ` Michael Walle
2021-01-20 15:50   ` Greg Kroah-Hartman
2021-01-20 19:02   ` Saravana Kannan
2021-01-20 19:25     ` Michael Walle
2021-01-20 19:28     ` Michael Walle
2021-01-20 19:47       ` Saravana Kannan
2021-01-20 19:47         ` Saravana Kannan
2021-01-20 23:53         ` Michael Walle
2021-01-20 23:58           ` Saravana Kannan
2021-01-21 11:01             ` Geert Uytterhoeven
2021-01-25 19:49               ` Michael Walle
2021-01-25 22:41                 ` Saravana Kannan
2021-01-26  8:50                   ` Geert Uytterhoeven
2021-01-27  0:44                     ` Saravana Kannan
2021-01-27  7:43                       ` Geert Uytterhoeven
2021-01-27 16:41                         ` Saravana Kannan
2021-01-27 16:56                           ` Geert Uytterhoeven
2021-01-27 17:10                             ` Saravana Kannan
2021-01-28  9:25                               ` Geert Uytterhoeven
2021-01-28 10:35                                 ` Tony Lindgren
2021-01-28 10:00                 ` Tony Lindgren
2021-01-25 16:50       ` Lorenzo Pieralisi
2021-01-25 18:58         ` Saravana Kannan
2021-01-25 19:44           ` Michael Walle
2021-01-26 10:02 ` Lorenzo Pieralisi
2021-01-26 10:39   ` Michael Walle
2021-01-26 10:56     ` Geert Uytterhoeven
2021-01-26 10:55 ` Lorenzo Pieralisi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).