linux-kernel - Re: [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <615718f9-151e-20fb-fcb0-56063ae61ca6@lucaceresoli.net>
Date:   Thu, 12 May 2022 16:07:51 +0200
From:   Luca Ceresoli <luca@...aceresoli.net>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Rob Herring <robh@...nel.org>
Cc:     Saravana Kannan <saravanak@...gle.com>,
        PCI <linux-pci@...r.kernel.org>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        linux-omap <linux-omap@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Sekhar Nori <nsekhar@...com>
Subject: Re: [PATCH 1/2] PCI: dra7xx: Fix link removal on probe error

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@...aceresoli.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@...aceresoli.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@...aceresoli.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