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  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]
Date:   Wed, 30 Aug 2017 16:25:47 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Himanshu Jha <himanshujha199640@...il.com>, bhelgaas@...gle.com,
        jonathanh@...dia.com, linux-tegra@...r.kernel.org,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI: tegra: Use PTR_ERR_OR_ZERO

On Wed, Aug 30, 2017 at 08:59:31AM -0500, Bjorn Helgaas wrote:
> On Tue, Aug 29, 2017 at 03:55:17PM +0200, Thierry Reding wrote:
> > On Tue, Aug 29, 2017 at 07:09:00PM +0530, Himanshu Jha wrote:
> > > Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR
> > > 
> > > Signed-off-by: Himanshu Jha <himanshujha199640@...il.com>
> > > ---
> > >  drivers/pci/host/pci-tegra.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > > index 9c40da5..90cda5b 100644
> > > --- a/drivers/pci/host/pci-tegra.c
> > > +++ b/drivers/pci/host/pci-tegra.c
> > > @@ -1156,10 +1156,7 @@ static int tegra_pcie_resets_get(struct tegra_pcie *pcie)
> > >  		return PTR_ERR(pcie->afi_rst);
> > >  
> > >  	pcie->pcie_xrst = devm_reset_control_get_exclusive(dev, "pcie_x");
> > > -	if (IS_ERR(pcie->pcie_xrst))
> > > -		return PTR_ERR(pcie->pcie_xrst);
> > > -
> > > -	return 0;
> > > +	return PTR_ERR_OR_ZERO(pcie->pcie_xrst);
> > >  }
> > 
> > I'm not a big fan of this construct because it's a pain to undo this if
> > ever we need to add code to this function. But since we do have scripts
> > that will flag this, I guess this would pop up every now and again. The
> > driver is unlikely to change in this part, too, so:
> 
> Thanks for pointing this out.  Do you know what the benefit of
> PTR_ERR_OR_ZERO() is?  To me, it makes the following code harder
> to read because the error tests are no longer parallel:
> 
>   ...
>   res->ahb_reset = devm_reset_control_get(dev, "ahb");
>   if (IS_ERR(res->ahb_reset))
>     return PTR_ERR(res->ahb_reset);
> 
>   res->por_reset = devm_reset_control_get(dev, "por");
>   if (IS_ERR(res->por_reset))
>     return PTR_ERR(res->por_reset);
> 
>   res->phy_reset = devm_reset_control_get(dev, "phy");
>   return PTR_ERR_OR_ZERO(res->phy_reset);
> 
> So I'd be inclined to avoid it unless there's some significant benefit.

Yeah, I don't like the optics much either. Aside from the fact that it
reduces the line count, I'm not aware of any benefits that this inline
function has. It doesn't have any side-effects or anything, just wraps
the common pattern into a single line.

Looking at the history of the semantic patch that is the basis for the
conversions (scripts/coccinelle/api/ptr_ret.cocci), or the static inline
function itself, no rationale is given for why people prefer this. I've
certainly seen such patches applied in some cases, but I've also seen
other maintainers (including myself) reject them because of personal
preference.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists