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 <[email protected]>
---
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
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 <[email protected]>
Reported-by: kernel test robot <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Luca Ceresoli <[email protected]>
---
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
On Tue, Dec 14, 2021 at 4:15 PM Luca Ceresoli <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
>> ---
>> 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
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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
+Saravana
On Tue, Jan 11, 2022 at 4:35 AM Luca Ceresoli <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > >>> ---
> > >>> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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
On Thu, May 12, 2022 at 7:07 AM Luca Ceresoli <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>>>> ---
> >>>>>> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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
On Tue, May 17, 2022 at 12:32 AM Luca Ceresoli <[email protected]> wrote:
>
> Hi Saravana,
>
> On 14/05/22 05:46, Saravana Kannan wrote:
> > On Thu, May 12, 2022 at 7:07 AM Luca Ceresoli <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>>>>>> ---
> >>>>>>>> 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 <[email protected]> wrote:
>>
>> Hi Saravana,
>>
>> On 14/05/22 05:46, Saravana Kannan wrote:
>>> On Thu, May 12, 2022 at 7:07 AM Luca Ceresoli <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>>>>>> ---
>>>>>>>>>> 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