Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1722498 > unrolled thread

[PATCH] PCI: tegra: Use PTR_ERR_OR_ZERO

Back to article view | Back to linux.kernel

Contents

  [PATCH] PCI: tegra: Use PTR_ERR_OR_ZERO Himanshu Jha <himanshujha199640@gmail.com> - 2017-08-29 15:40 +0200
    Re: [PATCH] PCI: tegra: Use PTR_ERR_OR_ZERO Thierry Reding <thierry.reding@gmail.com> - 2017-08-29 16:00 +0200
      Re: [PATCH] PCI: tegra: Use PTR_ERR_OR_ZERO Himanshu Jha <himanshujha199640@gmail.com> - 2017-08-29 16:20 +0200
        Re: [PATCH] PCI: tegra: Use PTR_ERR_OR_ZERO Thierry Reding <thierry.reding@gmail.com> - 2017-08-29 17:20 +0200
      Re: [PATCH] PCI: tegra: Use PTR_ERR_OR_ZERO Bjorn Helgaas <helgaas@kernel.org> - 2017-08-30 16:00 +0200
        Re: [PATCH] PCI: tegra: Use PTR_ERR_OR_ZERO Thierry Reding <thierry.reding@gmail.com> - 2017-08-30 16:30 +0200
          Re: [PATCH] PCI: tegra: Use PTR_ERR_OR_ZERO Bjorn Helgaas <helgaas@kernel.org> - 2017-08-30 18:30 +0200

#1722498 — [PATCH] PCI: tegra: Use PTR_ERR_OR_ZERO

Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR

Signed-off-by: Himanshu Jha <himanshujha199640@gmail.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);
 }
 
 static int tegra_pcie_phys_get_legacy(struct tegra_pcie *pcie)
-- 
2.7.4

[toc] | [next] | [standalone]

#1722513

[Multipart message — attachments visible in raw view] — view raw

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@gmail.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:

Acked-by: Thierry Reding <treding@nvidia.com>

[toc] | [prev] | [next] | [standalone]

#1722524

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@gmail.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:

What do you suggest ? Shall I stop sending these patches ?

Thanks
-Himanshu Jha

> Acked-by: Thierry Reding <treding@nvidia.com>

[toc] | [prev] | [next] | [standalone]

#1722558

[Multipart message — attachments visible in raw view] — view raw

On Tue, Aug 29, 2017 at 07:44:01PM +0530, Himanshu Jha 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@gmail.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:
> 
> What do you suggest ? Shall I stop sending these patches ?

No, it's fine. I'm just saying that there are cases where this doesn't
make sense. In this case I think it's fine because the driver is fairly
mature and unlikely to change, so there is not a lot of potential for
churn later on. In other cases, use your best judgement.

Ultimately it is up to maintainers whether or not they apply this kind
of patch.

Thierry

[toc] | [prev] | [next] | [standalone]

#1723353

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@gmail.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.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1723385

[Multipart message — attachments visible in raw view] — view raw

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@gmail.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

[toc] | [prev] | [next] | [standalone]

#1723484

On Wed, Aug 30, 2017 at 04:25:47PM +0200, Thierry Reding wrote:
> 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@gmail.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.

OK, I'm going to drop this then.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web