Re: [PATCH 03/21] ata: libahci_platform: Explicitly set rc on devres_alloc failure

[Serge Semin <fancer.lancer@gmail.com>]

Hello Damien

On Thu, Mar 24, 2022 at 09:58:34AM +0900, Damien Le Moal wrote:
> On 3/24/22 09:16, Serge Semin wrote:
> > It's better for readability and maintainability to explicitly assign an
> > error number to the variable used then as a return value from the method
> > on the cleanup-on-error path. So adding new code in the method we won't
> 

> No it is not. On-stack variable initialization is not free. So if
> initializing the variable is not needed, do not do it.

This patch isn't about on-stack initialization, but about bringing an
order to the error-handling procedure of the
ahci_platform_get_resources() method. Explicitly setting the rc variable
with an error value closer to the place caused the error much easier
to perceive than keeping in mind that the variable has been set with
some default value. That turns to be even more justified seeing the
rest of the method does it that way.

See my next comment regarding the initialization.

> 
> > have to think whether the overridden rc-variable is set afterward in case
> > of an error. Saving one line of code doesn't worth it especially seeing
> > the rest of the ahci_platform_get_resources() function errors handling
> > blocks do explicitly write errno to rc.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >  drivers/ata/libahci_platform.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> > index 18296443ccba..1bd2f1686239 100644
> > --- a/drivers/ata/libahci_platform.c
> > +++ b/drivers/ata/libahci_platform.c
> > @@ -389,7 +389,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >  	struct ahci_host_priv *hpriv;
> >  	struct clk *clk;
> >  	struct device_node *child;
> > -	int i, enabled_ports = 0, rc = -ENOMEM, child_nodes;
> > +	int i, enabled_ports = 0, rc = 0, child_nodes;
> >  	u32 mask_port_map = 0;
> >  
> >  	if (!devres_open_group(dev, NULL, GFP_KERNEL))
> > @@ -397,8 +397,10 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >  
> >  	hpriv = devres_alloc(ahci_platform_put_resources, sizeof(*hpriv),
> >  			     GFP_KERNEL);
> > -	if (!hpriv)
> > +	if (!hpriv) {
> > +		rc = -ENOMEM;
> >  		goto err_out;
> > +	}
> 

> If you set rc to -ENOMEM here, then the 0 initialization of rc is not needed.

Normally you are right. But the case of the rc/ret/etc variables is
special. I'd stick with having it defaulted to 0 here. Here is why.

When it comes to using the rc/ret/etc variables the maintainability
gets to be more important than some small optimization (especially
here seeing the ahci_platform_get_resources() is called once per
device life-time) because in case of the method alteration these
variables very often get to be involved in one way or another. If due
to a mistake the rc/ret/etc variable isn't updated in case of an
erroneous situation but the method is terminated with the goto-pattern
and rc/ret/etc isn't initialized with any default value then we will
end up with having a garbage pointer returned. We'd be lucky if it was
a null pointer, but in general it can be a reference to some random
memory region. In the later case the kernel may experience random
crashes with hard-to-find cause of the problem. In the former case the
problem would have been tracked right away on the testing stage by
getting the system invalid-pointer de-reference crash. That's why
defaulting the variable to zero here is still useful.

-Sergey

> 
> >  
> >  	devres_add(dev, hpriv);
> >  
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research