| Message ID | 20210120105246.23218-1-michael@walle.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: dwc: layerscape: convert to builtin_platform_driver() | expand |
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 >
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 >>
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
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 > >
[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
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
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
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
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
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
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
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
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
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
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
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
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 >
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
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
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
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
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
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
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
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
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
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
* Geert Uytterhoeven <geert@linux-m68k.org> [210128 09:32]: > It wasn't. The regression is that the driver no longer probes at first > try, because its dependencies are now probed later. The question is: > why are the dependencies now probed later? How to fix that? I'm afraid that may be unfixable.. It depends on things like the bus driver probe that might get also deferred. As suggested, I agree it's best to get rid of builtin_platform_driver_probe where possible at the cost of dropping the __init as needed. To me it seems we can't even add a warning to __platform_driver_probe() if there's drv->driver.of_match_table for example. That warning would show up on all the devices with driver in question built in even if the device has no such hardware. Regards, Tony
diff --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);
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(-)