linux-kernel - Re: [PATCH v5 10/12] PCI: brcmstb: Check return value of all reset_control

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: <CA+-6iNxpFXAyEMri=30kDu8irZeUoJ7iVO6P_PfV4br1=GEigA@mail.gmail.com>
Date: Mon, 12 Aug 2024 14:20:11 -0400
From: Jim Quinlan <james.quinlan@...adcom.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: 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>, Stanimir Varbanov <svarbanov@...e.de>, 
	Krzysztof Kozlowski <krzk@...nel.org>, bcm-kernel-feedback-list@...adcom.com, 
	jim2101024@...il.com, 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 10/12] PCI: brcmstb: Check return value of all
 reset_control_xxx calls

On Wed, Aug 7, 2024 at 10:11 AM Manivannan Sadhasivam
<manivannan.sadhasivam@...aro.org> wrote:
>
> On Wed, Jul 31, 2024 at 06:28:24PM -0400, Jim Quinlan wrote:
> > Always check the return value for invocations of reset_control_xxx() and
> > propagate the error to the next level.  Although the current functions
> > in reset-brcmstb.c cannot fail, this may someday change.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@...adcom.com>
>
> One comment below. With that addressed,
>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
>
> > Reviewed-by: Stanimir Varbanov <svarbanov@...e.de>
> > Reviewed-by: Florian Fainelli <florian.fainelli@...adcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 102 ++++++++++++++++++--------
> >  1 file changed, 73 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 0ecca3d9576f..c4ceb1823a79 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
>
> [...]
>
> >  static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
> > @@ -1478,9 +1514,12 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
> >  {
> >       struct brcm_pcie *pcie = dev_get_drvdata(dev);
> >       struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> > -     int ret;
> > +     int ret, rret;
> > +
> > +     ret = brcm_pcie_turn_off(pcie);
> > +     if (ret)
> > +             return ret;
> >
> > -     brcm_pcie_turn_off(pcie);
> >       /*
> >        * If brcm_phy_stop() returns an error, just dev_err(). If we
> >        * return the error it will cause the suspend to fail and this is a
> > @@ -1509,7 +1548,10 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
> >                                                    pcie->sr->supplies);
> >                       if (ret) {
> >                               dev_err(dev, "Could not turn off regulators\n");
> > -                             reset_control_reset(pcie->rescal);
> > +                             rret = reset_control_reset(pcie->rescal);
> > +                             if (rret)
> > +                                     dev_err(dev, "failed to reset 'rascal' controller ret=%d\n",
> > +                                             rret);
>
> I don't think it is really necessary to capture the return value in err path.
> Unable to turn off the regulator itself is fatal, so we could just assert reset
> and hope for the best.

Hi Mani,

I'm not sure what you would like changed.  Failure to turn off the
regulators isn't  necessarily fatal, it's when they cannot be turned
on that is fatal.

There can be two failures here: failure to turn off regulators and
failure to reset the "rascal" reset.  Since I can only return one
error value, I print the other value via dev_err().

Regards,
Jim Quinlan
Broadcom STB/CM


>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4210 bytes)