| 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
| ||
|
Message-ID: <8ed1aa9d-5ff5-465b-b91f-b32cc25cac34@broadcom.com>
Date: Tue, 13 Aug 2024 13:06:12 -0400
From: James Quinlan <james.quinlan@...adcom.com>
To: Stanimir Varbanov <svarbanov@...e.de>, linux-pci@...r.kernel.org,
Nicolas Saenz Julienne <nsaenz@...nel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Cyril Brulebois <kibi@...ian.org>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Krzysztof Kozlowski <krzk@...nel.org>,
bcm-kernel-feedback-list@...adcom.com, jim2101024@...il.com
Cc: Florian Fainelli <florian.fainelli@...adcom.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Rob Herring <robh@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>,
"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
<linux-rpi-kernel@...ts.infradead.org>,
"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
<linux-arm-kernel@...ts.infradead.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 03/12] PCI: brcmstb: Use common error handling code in
brcm_pcie_probe()
On 8/13/24 12:45, Stanimir Varbanov wrote:
> Hi Jim,
>
> On 8/1/24 01:28, Jim Quinlan wrote:
>> o Move the clk_prepare_enable() below the resource allocations.
>> o Move the clk_prepare_enable() out of __brcm_pcie_remove() but
>> add it to the end of brcm_pcie_remove().
>> o Add a jump target (clk_disable_unprepare) so that a bit of exception
>> handling can be better reused at the end of this function implementation.
>> o Use dev_err_probe() where it makes sense.PASSWORD
> Those dev_err_probe produce these errors on RPi5:
Hi Stan,
Sorry, I clearly missed that. My perception of the dev_err_probe()
semantics were incorrect -- I thought it didn't print anything at all
if the "ret" param was zero, which would allow it to be used as an "if"
conditional, as I am doing. Obviously not the case.
Will fix.
BTW I do not yet possess a CM5 so my testing is done on a CM4, 7712, and
other STB chips.
Regards and thanks,
Jim Quinlan
Broadcom STB/CM
>
> rpi5:~ # dmesg -l err
> [ 1.004960] brcm-pcie 1000110000.pcie: error 0000000000000000: could
> not assert reset 'swinit'
> [ 1.013812] brcm-pcie 1000110000.pcie: error 0000000000000000: could
> not de-assert reset 'swinit' after asserting
> [ 1.024222] brcm-pcie 1000110000.pcie: error 0000000000000000: failed
> to deassert 'rescal'
> [ 1.533839] brcm-pcie 1000110000.pcie: link down
> [ 1.627564] brcm-pcie 1000120000.pcie: error 0000000000000000: could
> not assert reset 'swinit'
> [ 1.636415] brcm-pcie 1000120000.pcie: error 0000000000000000: could
> not de-assert reset 'swinit' after asserting
> [ 1.646829] brcm-pcie 1000120000.pcie: error 0000000000000000: failed
> to deassert 'rescal'
>
> ... as you can see there is no error at all.
>
> ~Stan
>
>> Signed-off-by: Jim Quinlan <james.quinlan@...adcom.com>
>> ---
>> drivers/pci/controller/pcie-brcmstb.c | 34 ++++++++++++---------------
>> 1 file changed, 15 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>> index c08683febdd4..7595e7009192 100644
>> --- a/drivers/pci/controller/pcie-brcmstb.c
>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>> @@ -1473,7 +1473,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
>> dev_err(pcie->dev, "Could not stop phy\n");
>> if (reset_control_rearm(pcie->rescal))
>> dev_err(pcie->dev, "Could not rearm rescal reset\n");
>> - clk_disable_unprepare(pcie->clk);
>> }
>>
>> static void brcm_pcie_remove(struct platform_device *pdev)
>> @@ -1484,6 +1483,7 @@ static void brcm_pcie_remove(struct platform_device *pdev)
>> pci_stop_root_bus(bridge->bus);
>> pci_remove_root_bus(bridge->bus);
>> __brcm_pcie_remove(pcie);
>> + clk_disable_unprepare(pcie->clk);
>> }
>>
>> static const int pcie_offsets[] = {
>> @@ -1613,31 +1613,26 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>
>> pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc");
>>
>> - ret = clk_prepare_enable(pcie->clk);
>> - if (ret) {
>> - dev_err(&pdev->dev, "could not enable clock\n");
>> - return ret;
>> - }
>> pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
>> - if (IS_ERR(pcie->rescal)) {
>> - clk_disable_unprepare(pcie->clk);
>> + if (IS_ERR(pcie->rescal))
>> return PTR_ERR(pcie->rescal);
>> - }
>> +
>> pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
>> - if (IS_ERR(pcie->perst_reset)) {
>> - clk_disable_unprepare(pcie->clk);
>> + if (IS_ERR(pcie->perst_reset))
>> return PTR_ERR(pcie->perst_reset);
>> - }
>>
>> - ret = reset_control_reset(pcie->rescal);
>> + ret = clk_prepare_enable(pcie->clk);
>> if (ret)
>> - dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
>> + return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
>> +
>> + ret = reset_control_reset(pcie->rescal);
>> + if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
>> + goto clk_disable_unprepare;
>>
>> ret = brcm_phy_start(pcie);
>> if (ret) {
>> reset_control_rearm(pcie->rescal);
>> - clk_disable_unprepare(pcie->clk);
>> - return ret;
>> + goto clk_disable_unprepare;
>> }
>>
>> ret = brcm_pcie_setup(pcie);
>> @@ -1654,10 +1649,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>> msi_np = of_parse_phandle(pcie->np, "msi-parent", 0);
>> if (pci_msi_enabled() && msi_np == pcie->np) {
>> ret = brcm_pcie_enable_msi(pcie);
>> - if (ret) {
>> - dev_err(pcie->dev, "probe of internal MSI failed");
>> + if (dev_err_probe(pcie->dev, ret, "probe of internal MSI failed"))
>> goto fail;
>> - }
>> }
>>
>> bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
>> @@ -1678,6 +1671,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>
>> fail:
>> __brcm_pcie_remove(pcie);
>> +clk_disable_unprepare:
>> + clk_disable_unprepare(pcie->clk);
>> +
>> return ret;
>> }
>>
Powered by blists - more mailing lists