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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAL_Jsq+hrQG5YwPBTOg-nt2pJ-ZgsEcSqL+BYaeVaj19ckJEhQ@mail.gmail.com>
Date:   Thu, 17 Feb 2022 16:15:26 -0600
From:   Rob Herring <robh@...nel.org>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Ben Dooks <ben.dooks@...ethink.co.uk>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        bhelgaas@...gle.comv, PCI <linux-pci@...r.kernel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Greentime Hu <greentime.hu@...ive.com>,
        david.abdurachmanov@...il.com,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Subject: Re: [PATCH 1/2] PCI: fu740: fix finding GPIOs

On Mon, Feb 14, 2022 at 10:05 AM Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> [+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".

This is purely a kernel issue. The gpio API appends '-gpios' (and a
fallback to '-gpio') to the name passed. So passing 'reset-gpios'
looks for 'reset-gpios-gpios' property. This happens enough, there
should probably be a check for this mistake.

>
> 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
> >