* [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error @ 2021-12-14 22:14 Luca Ceresoli 2021-12-14 22:14 ` [PATCH 2/2] PCI: dra7xx: Fix clk disabling in some error paths Luca Ceresoli 2021-12-14 22:42 ` [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error Rob Herring 0 siblings, 2 replies; 12+ messages in thread From: Luca Ceresoli @ 2021-12-14 22:14 UTC (permalink / raw) To: linux-pci Cc: Luca Ceresoli, Dan Carpenter, linux-omap, linux-arm-kernel, linux-kernel, Kishon Vijay Abraham I, Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński, Bjorn Helgaas, Sekhar Nori 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); if (!link[i]) { ret = -EINVAL; -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] PCI: dra7xx: Fix clk disabling in some error paths 2021-12-14 22:14 [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error Luca Ceresoli @ 2021-12-14 22:14 ` Luca Ceresoli 2021-12-14 22:42 ` [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error Rob Herring 1 sibling, 0 replies; 12+ messages in thread From: Luca Ceresoli @ 2021-12-14 22:14 UTC (permalink / raw) To: linux-pci Cc: Luca Ceresoli, Dan Carpenter, linux-omap, linux-arm-kernel, linux-kernel, Kishon Vijay Abraham I, Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński, Bjorn Helgaas, Sekhar Nori, kernel test robot dra7xx->clk is not disabled+unprepared in some one the error paths, specifically devm_phy_get() fails. Fix by moving the clk_prepare_enable() stanza after all the devm_*() resource grabbing but before all the goto-based error management. This way it is possible to keep the 'return err' without the need to replace it with a new goto statement. Fixes: 5af9405397bf ("PCI: dra7xx: Get an optional clock") Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Reported-by: kernel test robot <lkp@intel.com> Link: https://lore.kernel.org/lkml/202111301803.NOwoj4Jd-lkp@intel.com/ Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> --- drivers/pci/controller/dwc/pci-dra7xx.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c index 2ccc53869e13..d17cc088d07e 100644 --- a/drivers/pci/controller/dwc/pci-dra7xx.c +++ b/drivers/pci/controller/dwc/pci-dra7xx.c @@ -748,10 +748,6 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) return dev_err_probe(dev, PTR_ERR(dra7xx->clk), "clock request failed"); - ret = clk_prepare_enable(dra7xx->clk); - if (ret) - return ret; - for (i = 0; i < phy_count; i++) { snprintf(name, sizeof(name), "pcie-phy%d", i); phy[i] = devm_phy_get(dev, name); @@ -759,6 +755,10 @@ static int dra7xx_pcie_probe(struct platform_device *pdev) return PTR_ERR(phy[i]); } + ret = clk_prepare_enable(dra7xx->clk); + if (ret) + return ret; + for (i = 0; i < phy_count; i++) { link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS); if (!link[i]) { -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error 2021-12-14 22:14 [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error Luca Ceresoli 2021-12-14 22:14 ` [PATCH 2/2] PCI: dra7xx: Fix clk disabling in some error paths Luca Ceresoli @ 2021-12-14 22:42 ` Rob Herring 2021-12-16 9:08 ` Luca Ceresoli 1 sibling, 1 reply; 12+ messages in thread From: Rob Herring @ 2021-12-14 22:42 UTC (permalink / raw) To: Luca Ceresoli Cc: PCI, Dan Carpenter, linux-omap, linux-arm-kernel, linux-kernel, Kishon Vijay Abraham I, Lorenzo Pieralisi, Krzysztof Wilczyński, Bjorn Helgaas, Sekhar Nori 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error 2021-12-14 22:42 ` [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error Rob Herring @ 2021-12-16 9:08 ` Luca Ceresoli 2022-01-11 10:35 ` Luca Ceresoli 0 siblings, 1 reply; 12+ messages in thread From: Luca Ceresoli @ 2021-12-16 9:08 UTC (permalink / raw) To: Rob Herring Cc: PCI, Dan Carpenter, linux-omap, linux-arm-kernel, linux-kernel, Kishon Vijay Abraham I, Lorenzo Pieralisi, Krzysztof Wilczyński, Bjorn Helgaas, Sekhar Nori 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. -- Luca ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error 2021-12-16 9:08 ` Luca Ceresoli @ 2022-01-11 10:35 ` Luca Ceresoli 2022-01-15 16:02 ` Rob Herring 0 siblings, 1 reply; 12+ messages in thread From: Luca Ceresoli @ 2022-01-11 10:35 UTC (permalink / raw) To: Rob Herring Cc: PCI, Dan Carpenter, linux-omap, linux-arm-kernel, linux-kernel, Kishon Vijay Abraham I, Lorenzo Pieralisi, Krzysztof Wilczyński, Bjorn Helgaas, Sekhar Nori 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. -- Luca ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error 2022-01-11 10:35 ` Luca Ceresoli @ 2022-01-15 16:02 ` Rob Herring 2022-05-11 16:41 ` Lorenzo Pieralisi 0 siblings, 1 reply; 12+ messages in thread From: Rob Herring @ 2022-01-15 16:02 UTC (permalink / raw) To: Luca Ceresoli, Saravana Kannan Cc: PCI, Dan Carpenter, linux-omap, linux-arm-kernel, linux-kernel, Kishon Vijay Abraham I, Lorenzo Pieralisi, Krzysztof Wilczyński, Bjorn Helgaas, Sekhar Nori +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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error 2022-01-15 16:02 ` Rob Herring @ 2022-05-11 16:41 ` Lorenzo Pieralisi 2022-05-12 14:07 ` Luca Ceresoli 0 siblings, 1 reply; 12+ messages in thread From: Lorenzo Pieralisi @ 2022-05-11 16:41 UTC (permalink / raw) To: Rob Herring Cc: Luca Ceresoli, Saravana Kannan, PCI, Dan Carpenter, linux-omap, linux-arm-kernel, linux-kernel, Kishon Vijay Abraham I, Krzysztof Wilczyński, Bjorn Helgaas, Sekhar Nori 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error 2022-05-11 16:41 ` Lorenzo Pieralisi @ 2022-05-12 14:07 ` Luca Ceresoli 2022-05-14 3:46 ` Saravana Kannan 0 siblings, 1 reply; 12+ messages in thread From: Luca Ceresoli @ 2022-05-12 14:07 UTC (permalink / raw) To: Lorenzo Pieralisi, Rob Herring Cc: Saravana Kannan, PCI, Dan Carpenter, linux-omap, linux-arm-kernel, linux-kernel, Kishon Vijay Abraham I, Krzysztof Wilczyński, Bjorn Helgaas, Sekhar Nori 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. -- Luca ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error 2022-05-12 14:07 ` Luca Ceresoli @ 2022-05-14 3:46 ` Saravana Kannan 2022-05-17 7:32 ` Luca Ceresoli 0 siblings, 1 reply; 12+ messages in thread From: Saravana Kannan @ 2022-05-14 3:46 UTC (permalink / raw) To: Luca Ceresoli Cc: Lorenzo Pieralisi, Rob Herring, PCI, Dan Carpenter, linux-omap, linux-arm-kernel, linux-kernel, Kishon Vijay Abraham I, Krzysztof Wilczyński, Bjorn Helgaas, Sekhar Nori 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error 2022-05-14 3:46 ` Saravana Kannan @ 2022-05-17 7:32 ` Luca Ceresoli 2022-05-19 20:25 ` Saravana Kannan 0 siblings, 1 reply; 12+ messages in thread From: Luca Ceresoli @ 2022-05-17 7:32 UTC (permalink / raw) To: Saravana Kannan Cc: Lorenzo Pieralisi, Rob Herring, PCI, Dan Carpenter, linux-omap, linux-arm-kernel, linux-kernel, Kishon Vijay Abraham I, Krzysztof Wilczyński, Bjorn Helgaas, Sekhar Nori 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. -- Luca ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error 2022-05-17 7:32 ` Luca Ceresoli @ 2022-05-19 20:25 ` Saravana Kannan 2022-05-23 13:28 ` Luca Ceresoli 0 siblings, 1 reply; 12+ messages in thread From: Saravana Kannan @ 2022-05-19 20:25 UTC (permalink / raw) To: Luca Ceresoli Cc: Lorenzo Pieralisi, Rob Herring, PCI, Dan Carpenter, linux-omap, linux-arm-kernel, linux-kernel, Kishon Vijay Abraham I, Krzysztof Wilczyński, Bjorn Helgaas, Sekhar Nori, Android Kernel Team 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error 2022-05-19 20:25 ` Saravana Kannan @ 2022-05-23 13:28 ` Luca Ceresoli 0 siblings, 0 replies; 12+ messages in thread From: Luca Ceresoli @ 2022-05-23 13:28 UTC (permalink / raw) To: Saravana Kannan Cc: Lorenzo Pieralisi, Rob Herring, PCI, Dan Carpenter, linux-omap, linux-arm-kernel, linux-kernel, Kishon Vijay Abraham I, Krzysztof Wilczyński, Bjorn Helgaas, Sekhar Nori, Android Kernel Team 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. -- Luca ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-05-23 13:28 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-14 22:14 [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error Luca Ceresoli 2021-12-14 22:14 ` [PATCH 2/2] PCI: dra7xx: Fix clk disabling in some error paths Luca Ceresoli 2021-12-14 22:42 ` [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error Rob Herring 2021-12-16 9:08 ` Luca Ceresoli 2022-01-11 10:35 ` Luca Ceresoli 2022-01-15 16:02 ` Rob Herring 2022-05-11 16:41 ` Lorenzo Pieralisi 2022-05-12 14:07 ` Luca Ceresoli 2022-05-14 3:46 ` Saravana Kannan 2022-05-17 7:32 ` Luca Ceresoli 2022-05-19 20:25 ` Saravana Kannan 2022-05-23 13:28 ` Luca Ceresoli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).