linux-kernel - Re: [PATCH 1/2] PCI: fu740: fix finding GPIOs

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20220214160556.GA9253@bhelgaas>
Date:   Mon, 14 Feb 2022 10:05:56 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Ben Dooks <ben.dooks@...ethink.co.uk>
Cc:     linux-kernel@...r.kernel.org, bhelgaas@...gle.comv,
        linux-pci@...r.kernel.org, paul.walmsley@...ive.com,
        greentime.hu@...ive.com, david.abdurachmanov@...il.com,
        Rob Herring <robh@...nel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Subject: Re: [PATCH 1/2] PCI: fu740: fix finding GPIOs

[+cc Rob for possible DT/kernel match issue,
Lorenzo (native host bridge driver maintainer)]

s/fix finding/Fix finding/ (in subject)
Or even better, say something specific about the DT properties in
question, e.g., look for "reset" instead of "reset-gpios".

On Mon, Feb 14, 2022 at 08:21:43AM +0000, Ben Dooks wrote:
> The calls to devm_gpiod_get_optional() have the -gpios at the end of
> the name. This means the pcie driver is not finding the necessary
> reset or power GPOOs to allow the PCIe devices on the SiFive Unmatched
> boards.

s/pcie/PCIe/
s/GPOOs/GPIOs/
"to allow the PCIe devices ...?"  Something is missing from this
sentence.  "To allow the devices <to do what>"?  Or maybe the driver
needs these GPIOs to power up the PCIe devices?

I guess the implication is that the code looks for "reset-gpios" and
"pwren-gpios", but the DT contains "reset" and "pwren"?

But both Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
and arch/riscv/boot/dts/sifive/fu740-c000.dtsi actually do contain
"reset-gpios" and "pwren-gpios".

If we *do* want to change the code, please change the error messages
to match.

> This has not been a noted bug as the PCIe probe from u-boot has been
> required to get the PCIe working due to other issues with the system
> setup. It could have been broken since the driver inclusion, and not
> been noticed as it is not necessary for the driver to funciton.

s/u-boot/U-Boot/
s/funciton/function/

Please add a line about what the connection between U-Boot and this
issue is, e.g., maybe U-Boot powers up the devices, so we wouldn't
notice the kernel's inability to do so?

> Signed-off-by: Ben Dooks <ben.dooks@...ethink.co.uk>
> ---
>  drivers/pci/controller/dwc/pcie-fu740.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
> index 00cde9a248b5..842b7202b96e 100644
> --- a/drivers/pci/controller/dwc/pcie-fu740.c
> +++ b/drivers/pci/controller/dwc/pcie-fu740.c
> @@ -259,11 +259,11 @@ static int fu740_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(afp->mgmt_base);
>  
>  	/* Fetch GPIOs */
> -	afp->reset = devm_gpiod_get_optional(dev, "reset-gpios", GPIOD_OUT_LOW);
> +	afp->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>  	if (IS_ERR(afp->reset))
>  		return dev_err_probe(dev, PTR_ERR(afp->reset), "unable to get reset-gpios\n");
>  
> -	afp->pwren = devm_gpiod_get_optional(dev, "pwren-gpios", GPIOD_OUT_LOW);
> +	afp->pwren = devm_gpiod_get_optional(dev, "pwren", GPIOD_OUT_LOW);
>  	if (IS_ERR(afp->pwren))
>  		return dev_err_probe(dev, PTR_ERR(afp->pwren), "unable to get pwren-gpios\n");
>  
> -- 
> 2.34.1
>