| Message ID | 20211214221450.589884-1-luca@lucaceresoli.net (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [1/2] PCI: dra7xx: Fix link removal on probe error | expand |
On Tue, Dec 14, 2021 at 4:15 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > If a devm_phy_get() calls fails with phy_count==N (N > 0), then N links > have already been added by device_link_add() and won't be deleted by > device_link_del() because the code calls 'return' and not 'goto err_link'. > > Fix in a very simple way by doing all the devm_phy_get() calls before all > the device_link_add() calls. > > Fixes: 7a4db656a635 ("PCI: dra7xx: Create functional dependency between PCIe and PHY") > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > --- > drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c > index f7f1490e7beb..2ccc53869e13 100644 > --- a/drivers/pci/controller/dwc/pci-dra7xx.c > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > @@ -757,7 +757,9 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) > phy[i] = devm_phy_get(dev, name); > if (IS_ERR(phy[i])) > return PTR_ERR(phy[i]); > + } > > + for (i = 0; i < phy_count; i++) { > link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS); I think this should happen automatically now with fw_devlink being enabled by default. Can you try? > if (!link[i]) { > ret = -EINVAL; > -- > 2.25.1 >
Hi Rob, thanks for the quick feedback! On 14/12/21 23:42, Rob Herring wrote: > On Tue, Dec 14, 2021 at 4:15 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: >> >> If a devm_phy_get() calls fails with phy_count==N (N > 0), then N links >> have already been added by device_link_add() and won't be deleted by >> device_link_del() because the code calls 'return' and not 'goto err_link'. >> >> Fix in a very simple way by doing all the devm_phy_get() calls before all >> the device_link_add() calls. >> >> Fixes: 7a4db656a635 ("PCI: dra7xx: Create functional dependency between PCIe and PHY") >> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >> --- >> drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c >> index f7f1490e7beb..2ccc53869e13 100644 >> --- a/drivers/pci/controller/dwc/pci-dra7xx.c >> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c >> @@ -757,7 +757,9 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) >> phy[i] = devm_phy_get(dev, name); >> if (IS_ERR(phy[i])) >> return PTR_ERR(phy[i]); >> + } >> >> + for (i = 0; i < phy_count; i++) { >> link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS); > > I think this should happen automatically now with fw_devlink being > enabled by default. Can you try? Do you mean removal should be done automatically? I think they are not due to the DL_FLAG_STATELESS flag.
Hi Rob, On 16/12/21 10:08, Luca Ceresoli wrote: > Hi Rob, > > thanks for the quick feedback! > > On 14/12/21 23:42, Rob Herring wrote: >> On Tue, Dec 14, 2021 at 4:15 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: >>> >>> If a devm_phy_get() calls fails with phy_count==N (N > 0), then N links >>> have already been added by device_link_add() and won't be deleted by >>> device_link_del() because the code calls 'return' and not 'goto err_link'. >>> >>> Fix in a very simple way by doing all the devm_phy_get() calls before all >>> the device_link_add() calls. >>> >>> Fixes: 7a4db656a635 ("PCI: dra7xx: Create functional dependency between PCIe and PHY") >>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >>> --- >>> drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c >>> index f7f1490e7beb..2ccc53869e13 100644 >>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c >>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c >>> @@ -757,7 +757,9 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) >>> phy[i] = devm_phy_get(dev, name); >>> if (IS_ERR(phy[i])) >>> return PTR_ERR(phy[i]); >>> + } >>> >>> + for (i = 0; i < phy_count; i++) { >>> link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS); >> >> I think this should happen automatically now with fw_devlink being >> enabled by default. Can you try? > > Do you mean removal should be done automatically? I think they are not > due to the DL_FLAG_STATELESS flag. I would love to have feedback because, as said, I think my patch is correct, but if I'm wrong (which might well be) I have to drop patch 1 and rewrite patch 2 in a slightly more complex form. About your request to try: I only have hardware with phy_count==1, and anyway I cannot access it at the moment. :( Regards.
+Saravana On Tue, Jan 11, 2022 at 4:35 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > Hi Rob, > > On 16/12/21 10:08, Luca Ceresoli wrote: > > Hi Rob, > > > > thanks for the quick feedback! > > > > On 14/12/21 23:42, Rob Herring wrote: > >> On Tue, Dec 14, 2021 at 4:15 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: > >>> > >>> If a devm_phy_get() calls fails with phy_count==N (N > 0), then N links > >>> have already been added by device_link_add() and won't be deleted by > >>> device_link_del() because the code calls 'return' and not 'goto err_link'. > >>> > >>> Fix in a very simple way by doing all the devm_phy_get() calls before all > >>> the device_link_add() calls. > >>> > >>> Fixes: 7a4db656a635 ("PCI: dra7xx: Create functional dependency between PCIe and PHY") > >>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > >>> --- > >>> drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ > >>> 1 file changed, 2 insertions(+) > >>> > >>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c > >>> index f7f1490e7beb..2ccc53869e13 100644 > >>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c > >>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > >>> @@ -757,7 +757,9 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) > >>> phy[i] = devm_phy_get(dev, name); > >>> if (IS_ERR(phy[i])) > >>> return PTR_ERR(phy[i]); > >>> + } > >>> > >>> + for (i = 0; i < phy_count; i++) { > >>> link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS); > >> > >> I think this should happen automatically now with fw_devlink being > >> enabled by default. Can you try? > > > > Do you mean removal should be done automatically? I think they are not > > due to the DL_FLAG_STATELESS flag. > > I would love to have feedback because, as said, I think my patch is > correct, but if I'm wrong (which might well be) I have to drop patch 1 > and rewrite patch 2 in a slightly more complex form. I mean that why do you need explicit dependency tracking here when dependencies on a PHY should happen automatically now. IOW, what is special about this driver and dependency? Rob
On Sat, Jan 15, 2022 at 10:02:00AM -0600, Rob Herring wrote: > +Saravana > > On Tue, Jan 11, 2022 at 4:35 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > > > Hi Rob, > > > > On 16/12/21 10:08, Luca Ceresoli wrote: > > > Hi Rob, > > > > > > thanks for the quick feedback! > > > > > > On 14/12/21 23:42, Rob Herring wrote: > > >> On Tue, Dec 14, 2021 at 4:15 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > >>> > > >>> If a devm_phy_get() calls fails with phy_count==N (N > 0), then N links > > >>> have already been added by device_link_add() and won't be deleted by > > >>> device_link_del() because the code calls 'return' and not 'goto err_link'. > > >>> > > >>> Fix in a very simple way by doing all the devm_phy_get() calls before all > > >>> the device_link_add() calls. > > >>> > > >>> Fixes: 7a4db656a635 ("PCI: dra7xx: Create functional dependency between PCIe and PHY") > > >>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > > >>> --- > > >>> drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ > > >>> 1 file changed, 2 insertions(+) > > >>> > > >>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c > > >>> index f7f1490e7beb..2ccc53869e13 100644 > > >>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c > > >>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > > >>> @@ -757,7 +757,9 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) > > >>> phy[i] = devm_phy_get(dev, name); > > >>> if (IS_ERR(phy[i])) > > >>> return PTR_ERR(phy[i]); > > >>> + } > > >>> > > >>> + for (i = 0; i < phy_count; i++) { > > >>> link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS); > > >> > > >> I think this should happen automatically now with fw_devlink being > > >> enabled by default. Can you try? > > > > > > Do you mean removal should be done automatically? I think they are not > > > due to the DL_FLAG_STATELESS flag. > > > > I would love to have feedback because, as said, I think my patch is > > correct, but if I'm wrong (which might well be) I have to drop patch 1 > > and rewrite patch 2 in a slightly more complex form. > > I mean that why do you need explicit dependency tracking here when > dependencies on a PHY should happen automatically now. IOW, what is > special about this driver and dependency? Any update on this patch ? I think patch 2 can be merged, please let me know if this one can be dropped. Thanks, Lorenzo
Hi Lorenzo, On 11/05/22 18:41, Lorenzo Pieralisi wrote: > On Sat, Jan 15, 2022 at 10:02:00AM -0600, Rob Herring wrote: >> +Saravana >> >> On Tue, Jan 11, 2022 at 4:35 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: >>> >>> Hi Rob, >>> >>> On 16/12/21 10:08, Luca Ceresoli wrote: >>>> Hi Rob, >>>> >>>> thanks for the quick feedback! >>>> >>>> On 14/12/21 23:42, Rob Herring wrote: >>>>> On Tue, Dec 14, 2021 at 4:15 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: >>>>>> >>>>>> If a devm_phy_get() calls fails with phy_count==N (N > 0), then N links >>>>>> have already been added by device_link_add() and won't be deleted by >>>>>> device_link_del() because the code calls 'return' and not 'goto err_link'. >>>>>> >>>>>> Fix in a very simple way by doing all the devm_phy_get() calls before all >>>>>> the device_link_add() calls. >>>>>> >>>>>> Fixes: 7a4db656a635 ("PCI: dra7xx: Create functional dependency between PCIe and PHY") >>>>>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >>>>>> --- >>>>>> drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ >>>>>> 1 file changed, 2 insertions(+) >>>>>> >>>>>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c >>>>>> index f7f1490e7beb..2ccc53869e13 100644 >>>>>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c >>>>>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c >>>>>> @@ -757,7 +757,9 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) >>>>>> phy[i] = devm_phy_get(dev, name); >>>>>> if (IS_ERR(phy[i])) >>>>>> return PTR_ERR(phy[i]); >>>>>> + } >>>>>> >>>>>> + for (i = 0; i < phy_count; i++) { >>>>>> link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS); >>>>> >>>>> I think this should happen automatically now with fw_devlink being >>>>> enabled by default. Can you try? >>>> >>>> Do you mean removal should be done automatically? I think they are not >>>> due to the DL_FLAG_STATELESS flag. >>> >>> I would love to have feedback because, as said, I think my patch is >>> correct, but if I'm wrong (which might well be) I have to drop patch 1 >>> and rewrite patch 2 in a slightly more complex form. >> >> I mean that why do you need explicit dependency tracking here when >> dependencies on a PHY should happen automatically now. IOW, what is >> special about this driver and dependency? > > Any update on this patch ? I think patch 2 can be merged, please > let me know if this one can be dropped. Thanks for the feedback! You would say yes, you can merge patch 2, except it probably does not even apply as it is written in a way that is based on the changes in patch 1. I could rewrite patch 2 to not depend on patch 1 of course, but it wouldn't make code simpler, perhaps more complex. And moreover the hardware that I used to have access to has phy_count==1 so I could never test the failing case, and sadly now I have no access to that hardware.
On Thu, May 12, 2022 at 7:07 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > Hi Lorenzo, > > On 11/05/22 18:41, Lorenzo Pieralisi wrote: > > On Sat, Jan 15, 2022 at 10:02:00AM -0600, Rob Herring wrote: > >> +Saravana > >> > >> On Tue, Jan 11, 2022 at 4:35 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: > >>> > >>> Hi Rob, > >>> > >>> On 16/12/21 10:08, Luca Ceresoli wrote: > >>>> Hi Rob, > >>>> > >>>> thanks for the quick feedback! > >>>> > >>>> On 14/12/21 23:42, Rob Herring wrote: > >>>>> On Tue, Dec 14, 2021 at 4:15 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: > >>>>>> > >>>>>> If a devm_phy_get() calls fails with phy_count==N (N > 0), then N links > >>>>>> have already been added by device_link_add() and won't be deleted by > >>>>>> device_link_del() because the code calls 'return' and not 'goto err_link'. > >>>>>> > >>>>>> Fix in a very simple way by doing all the devm_phy_get() calls before all > >>>>>> the device_link_add() calls. > >>>>>> > >>>>>> Fixes: 7a4db656a635 ("PCI: dra7xx: Create functional dependency between PCIe and PHY") > >>>>>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > >>>>>> --- > >>>>>> drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ > >>>>>> 1 file changed, 2 insertions(+) > >>>>>> > >>>>>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c > >>>>>> index f7f1490e7beb..2ccc53869e13 100644 > >>>>>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c > >>>>>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > >>>>>> @@ -757,7 +757,9 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) > >>>>>> phy[i] = devm_phy_get(dev, name); > >>>>>> if (IS_ERR(phy[i])) > >>>>>> return PTR_ERR(phy[i]); > >>>>>> + } > >>>>>> > >>>>>> + for (i = 0; i < phy_count; i++) { > >>>>>> link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS); > >>>>> > >>>>> I think this should happen automatically now with fw_devlink being > >>>>> enabled by default. Can you try? > >>>> > >>>> Do you mean removal should be done automatically? I think they are not > >>>> due to the DL_FLAG_STATELESS flag. > >>> > >>> I would love to have feedback because, as said, I think my patch is > >>> correct, but if I'm wrong (which might well be) I have to drop patch 1 > >>> and rewrite patch 2 in a slightly more complex form. > >> > >> I mean that why do you need explicit dependency tracking here when > >> dependencies on a PHY should happen automatically now. IOW, what is > >> special about this driver and dependency? > > > > Any update on this patch ? I think patch 2 can be merged, please > > let me know if this one can be dropped. > > Thanks for the feedback! You would say yes, you can merge patch 2, > except it probably does not even apply as it is written in a way that is > based on the changes in patch 1. > > I could rewrite patch 2 to not depend on patch 1 of course, but it > wouldn't make code simpler, perhaps more complex. And moreover the > hardware that I used to have access to has phy_count==1 so I could never > test the failing case, and sadly now I have no access to that hardware. Hi Luca, The fw_devlink code to create device links from consumers to "phys" suppliers is pretty well exercised. Most/all Android devices running 5.10+ kernels (including Pixel 6) use fw_devlink=on to be able to boot properly. So I'd be pretty confident in deleting the device_link_add/del() code in drivers/pci/controller/dwc/pci-dra7xx.c. The device links should already be there before the probe is even called. Also, if you want to check if the device links (even the 1 phy one you have) are being created, you can look at /sys/class/devlink to see the list of all device links that are currently present. You can delete the code and then use this to check too. -Saravana > > -- > Luca
Hi Saravana, On 14/05/22 05:46, Saravana Kannan wrote: > On Thu, May 12, 2022 at 7:07 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: >> >> Hi Lorenzo, >> >> On 11/05/22 18:41, Lorenzo Pieralisi wrote: >>> On Sat, Jan 15, 2022 at 10:02:00AM -0600, Rob Herring wrote: >>>> +Saravana >>>> >>>> On Tue, Jan 11, 2022 at 4:35 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: >>>>> >>>>> Hi Rob, >>>>> >>>>> On 16/12/21 10:08, Luca Ceresoli wrote: >>>>>> Hi Rob, >>>>>> >>>>>> thanks for the quick feedback! >>>>>> >>>>>> On 14/12/21 23:42, Rob Herring wrote: >>>>>>> On Tue, Dec 14, 2021 at 4:15 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: >>>>>>>> >>>>>>>> If a devm_phy_get() calls fails with phy_count==N (N > 0), then N links >>>>>>>> have already been added by device_link_add() and won't be deleted by >>>>>>>> device_link_del() because the code calls 'return' and not 'goto err_link'. >>>>>>>> >>>>>>>> Fix in a very simple way by doing all the devm_phy_get() calls before all >>>>>>>> the device_link_add() calls. >>>>>>>> >>>>>>>> Fixes: 7a4db656a635 ("PCI: dra7xx: Create functional dependency between PCIe and PHY") >>>>>>>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >>>>>>>> --- >>>>>>>> drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ >>>>>>>> 1 file changed, 2 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c >>>>>>>> index f7f1490e7beb..2ccc53869e13 100644 >>>>>>>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c >>>>>>>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c >>>>>>>> @@ -757,7 +757,9 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) >>>>>>>> phy[i] = devm_phy_get(dev, name); >>>>>>>> if (IS_ERR(phy[i])) >>>>>>>> return PTR_ERR(phy[i]); >>>>>>>> + } >>>>>>>> >>>>>>>> + for (i = 0; i < phy_count; i++) { >>>>>>>> link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS); >>>>>>> >>>>>>> I think this should happen automatically now with fw_devlink being >>>>>>> enabled by default. Can you try? >>>>>> >>>>>> Do you mean removal should be done automatically? I think they are not >>>>>> due to the DL_FLAG_STATELESS flag. >>>>> >>>>> I would love to have feedback because, as said, I think my patch is >>>>> correct, but if I'm wrong (which might well be) I have to drop patch 1 >>>>> and rewrite patch 2 in a slightly more complex form. >>>> >>>> I mean that why do you need explicit dependency tracking here when >>>> dependencies on a PHY should happen automatically now. IOW, what is >>>> special about this driver and dependency? >>> >>> Any update on this patch ? I think patch 2 can be merged, please >>> let me know if this one can be dropped. >> >> Thanks for the feedback! You would say yes, you can merge patch 2, >> except it probably does not even apply as it is written in a way that is >> based on the changes in patch 1. >> >> I could rewrite patch 2 to not depend on patch 1 of course, but it >> wouldn't make code simpler, perhaps more complex. And moreover the >> hardware that I used to have access to has phy_count==1 so I could never >> test the failing case, and sadly now I have no access to that hardware. > > Hi Luca, > > The fw_devlink code to create device links from consumers to "phys" > suppliers is pretty well exercised. Most/all Android devices running > 5.10+ kernels (including Pixel 6) use fw_devlink=on to be able to boot > properly. > > So I'd be pretty confident in deleting the device_link_add/del() code > in drivers/pci/controller/dwc/pci-dra7xx.c. The device links should > already be there before the probe is even called. > > Also, if you want to check if the device links (even the 1 phy one you > have) are being created, you can look at /sys/class/devlink to see the > list of all device links that are currently present. You can delete > the code and then use this to check too. Thank you for your feedback. Unfortunately as I said I have no access to the hardware, and won't have anymore. I don't think it is a good idea to send a patch that I cannot test on real hardware, especially since it is for a generic hardware that thus might affect others. But I would be glad to review any such patch that might be sent, FWIW.
On Tue, May 17, 2022 at 12:32 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > Hi Saravana, > > On 14/05/22 05:46, Saravana Kannan wrote: > > On Thu, May 12, 2022 at 7:07 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: > >> > >> Hi Lorenzo, > >> > >> On 11/05/22 18:41, Lorenzo Pieralisi wrote: > >>> On Sat, Jan 15, 2022 at 10:02:00AM -0600, Rob Herring wrote: > >>>> +Saravana > >>>> > >>>> On Tue, Jan 11, 2022 at 4:35 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: > >>>>> > >>>>> Hi Rob, > >>>>> > >>>>> On 16/12/21 10:08, Luca Ceresoli wrote: > >>>>>> Hi Rob, > >>>>>> > >>>>>> thanks for the quick feedback! > >>>>>> > >>>>>> On 14/12/21 23:42, Rob Herring wrote: > >>>>>>> On Tue, Dec 14, 2021 at 4:15 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: > >>>>>>>> > >>>>>>>> If a devm_phy_get() calls fails with phy_count==N (N > 0), then N links > >>>>>>>> have already been added by device_link_add() and won't be deleted by > >>>>>>>> device_link_del() because the code calls 'return' and not 'goto err_link'. > >>>>>>>> > >>>>>>>> Fix in a very simple way by doing all the devm_phy_get() calls before all > >>>>>>>> the device_link_add() calls. > >>>>>>>> > >>>>>>>> Fixes: 7a4db656a635 ("PCI: dra7xx: Create functional dependency between PCIe and PHY") > >>>>>>>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > >>>>>>>> --- > >>>>>>>> drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ > >>>>>>>> 1 file changed, 2 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c > >>>>>>>> index f7f1490e7beb..2ccc53869e13 100644 > >>>>>>>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c > >>>>>>>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > >>>>>>>> @@ -757,7 +757,9 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) > >>>>>>>> phy[i] = devm_phy_get(dev, name); > >>>>>>>> if (IS_ERR(phy[i])) > >>>>>>>> return PTR_ERR(phy[i]); > >>>>>>>> + } > >>>>>>>> > >>>>>>>> + for (i = 0; i < phy_count; i++) { > >>>>>>>> link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS); > >>>>>>> > >>>>>>> I think this should happen automatically now with fw_devlink being > >>>>>>> enabled by default. Can you try? > >>>>>> > >>>>>> Do you mean removal should be done automatically? I think they are not > >>>>>> due to the DL_FLAG_STATELESS flag. > >>>>> > >>>>> I would love to have feedback because, as said, I think my patch is > >>>>> correct, but if I'm wrong (which might well be) I have to drop patch 1 > >>>>> and rewrite patch 2 in a slightly more complex form. > >>>> > >>>> I mean that why do you need explicit dependency tracking here when > >>>> dependencies on a PHY should happen automatically now. IOW, what is > >>>> special about this driver and dependency? > >>> > >>> Any update on this patch ? I think patch 2 can be merged, please > >>> let me know if this one can be dropped. > >> > >> Thanks for the feedback! You would say yes, you can merge patch 2, > >> except it probably does not even apply as it is written in a way that is > >> based on the changes in patch 1. > >> > >> I could rewrite patch 2 to not depend on patch 1 of course, but it > >> wouldn't make code simpler, perhaps more complex. And moreover the > >> hardware that I used to have access to has phy_count==1 so I could never > >> test the failing case, and sadly now I have no access to that hardware. > > > > Hi Luca, > > > > The fw_devlink code to create device links from consumers to "phys" > > suppliers is pretty well exercised. Most/all Android devices running > > 5.10+ kernels (including Pixel 6) use fw_devlink=on to be able to boot > > properly. > > > > So I'd be pretty confident in deleting the device_link_add/del() code > > in drivers/pci/controller/dwc/pci-dra7xx.c. The device links should > > already be there before the probe is even called. > > > > Also, if you want to check if the device links (even the 1 phy one you > > have) are being created, you can look at /sys/class/devlink to see the > > list of all device links that are currently present. You can delete > > the code and then use this to check too. > > Thank you for your feedback. Unfortunately as I said I have no access to > the hardware, and won't have anymore. I don't think it is a good idea to > send a patch that I cannot test on real hardware, especially since it is > for a generic hardware that thus might affect others. But I would be > glad to review any such patch that might be sent, FWIW. Just to make sure I'm on the same page. I thought you at least had a device where phy_count = 1. But looks like you are saying you don't? If all you want to check is "phys" have device links created for them for whatever random DT device that has a "phys" property, then I can test and confirm that for you on whatever platform I have. But if you want a test specifically for the device that corresponds to the driver you were fixing, then I can't. Let me know. -Saravana > > -- > Luca
Hi Saravana, On 19/05/22 22:25, Saravana Kannan wrote: > On Tue, May 17, 2022 at 12:32 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: >> >> Hi Saravana, >> >> On 14/05/22 05:46, Saravana Kannan wrote: >>> On Thu, May 12, 2022 at 7:07 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: >>>> >>>> Hi Lorenzo, >>>> >>>> On 11/05/22 18:41, Lorenzo Pieralisi wrote: >>>>> On Sat, Jan 15, 2022 at 10:02:00AM -0600, Rob Herring wrote: >>>>>> +Saravana >>>>>> >>>>>> On Tue, Jan 11, 2022 at 4:35 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: >>>>>>> >>>>>>> Hi Rob, >>>>>>> >>>>>>> On 16/12/21 10:08, Luca Ceresoli wrote: >>>>>>>> Hi Rob, >>>>>>>> >>>>>>>> thanks for the quick feedback! >>>>>>>> >>>>>>>> On 14/12/21 23:42, Rob Herring wrote: >>>>>>>>> On Tue, Dec 14, 2021 at 4:15 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: >>>>>>>>>> >>>>>>>>>> If a devm_phy_get() calls fails with phy_count==N (N > 0), then N links >>>>>>>>>> have already been added by device_link_add() and won't be deleted by >>>>>>>>>> device_link_del() because the code calls 'return' and not 'goto err_link'. >>>>>>>>>> >>>>>>>>>> Fix in a very simple way by doing all the devm_phy_get() calls before all >>>>>>>>>> the device_link_add() calls. >>>>>>>>>> >>>>>>>>>> Fixes: 7a4db656a635 ("PCI: dra7xx: Create functional dependency between PCIe and PHY") >>>>>>>>>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >>>>>>>>>> --- >>>>>>>>>> drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ >>>>>>>>>> 1 file changed, 2 insertions(+) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c >>>>>>>>>> index f7f1490e7beb..2ccc53869e13 100644 >>>>>>>>>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c >>>>>>>>>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c >>>>>>>>>> @@ -757,7 +757,9 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) >>>>>>>>>> phy[i] = devm_phy_get(dev, name); >>>>>>>>>> if (IS_ERR(phy[i])) >>>>>>>>>> return PTR_ERR(phy[i]); >>>>>>>>>> + } >>>>>>>>>> >>>>>>>>>> + for (i = 0; i < phy_count; i++) { >>>>>>>>>> link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS); >>>>>>>>> >>>>>>>>> I think this should happen automatically now with fw_devlink being >>>>>>>>> enabled by default. Can you try? >>>>>>>> >>>>>>>> Do you mean removal should be done automatically? I think they are not >>>>>>>> due to the DL_FLAG_STATELESS flag. >>>>>>> >>>>>>> I would love to have feedback because, as said, I think my patch is >>>>>>> correct, but if I'm wrong (which might well be) I have to drop patch 1 >>>>>>> and rewrite patch 2 in a slightly more complex form. >>>>>> >>>>>> I mean that why do you need explicit dependency tracking here when >>>>>> dependencies on a PHY should happen automatically now. IOW, what is >>>>>> special about this driver and dependency? >>>>> >>>>> Any update on this patch ? I think patch 2 can be merged, please >>>>> let me know if this one can be dropped. >>>> >>>> Thanks for the feedback! You would say yes, you can merge patch 2, >>>> except it probably does not even apply as it is written in a way that is >>>> based on the changes in patch 1. >>>> >>>> I could rewrite patch 2 to not depend on patch 1 of course, but it >>>> wouldn't make code simpler, perhaps more complex. And moreover the >>>> hardware that I used to have access to has phy_count==1 so I could never >>>> test the failing case, and sadly now I have no access to that hardware. >>> >>> Hi Luca, >>> >>> The fw_devlink code to create device links from consumers to "phys" >>> suppliers is pretty well exercised. Most/all Android devices running >>> 5.10+ kernels (including Pixel 6) use fw_devlink=on to be able to boot >>> properly. >>> >>> So I'd be pretty confident in deleting the device_link_add/del() code >>> in drivers/pci/controller/dwc/pci-dra7xx.c. The device links should >>> already be there before the probe is even called. >>> >>> Also, if you want to check if the device links (even the 1 phy one you >>> have) are being created, you can look at /sys/class/devlink to see the >>> list of all device links that are currently present. You can delete >>> the code and then use this to check too. >> >> Thank you for your feedback. Unfortunately as I said I have no access to >> the hardware, and won't have anymore. I don't think it is a good idea to >> send a patch that I cannot test on real hardware, especially since it is >> for a generic hardware that thus might affect others. But I would be >> glad to review any such patch that might be sent, FWIW. > > Just to make sure I'm on the same page. I thought you at least had a > device where phy_count = 1. But looks like you are saying you don't? I used to have access to a hardware with phy_count = 1 on a former job, but I don't have it anymore and won't have it since I left that job position. > If all you want to check is "phys" have device links created for them > for whatever random DT device that has a "phys" property, then I can > test and confirm that for you on whatever platform I have. But if you > want a test specifically for the device that corresponds to the driver > you were fixing, then I can't. Let me know. Honestly, I'm afraid I don't have much time to invest in trying to recollect all the details and motivations for this patchset. Likely I spotted this by code inspection while debugging other issues (I had a non-working PCIe device, but it was not the host fault). If you think there is little value in these patches, I'm OK in dropping them.
diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c index f7f1490e7beb..2ccc53869e13 100644 --- a/drivers/pci/controller/dwc/pci-dra7xx.c +++ b/drivers/pci/controller/dwc/pci-dra7xx.c @@ -757,7 +757,9 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) phy[i] = devm_phy_get(dev, name); if (IS_ERR(phy[i])) return PTR_ERR(phy[i]); + } + for (i = 0; i < phy_count; i++) { link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS); if (!link[i]) { ret = -EINVAL;
If a devm_phy_get() calls fails with phy_count==N (N > 0), then N links have already been added by device_link_add() and won't be deleted by device_link_del() because the code calls 'return' and not 'goto err_link'. Fix in a very simple way by doing all the devm_phy_get() calls before all the device_link_add() calls. Fixes: 7a4db656a635 ("PCI: dra7xx: Create functional dependency between PCIe and PHY") Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> --- drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ 1 file changed, 2 insertions(+)