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

[Serge Semin]

On Fri, Mar 25, 2022 at 10:56:29AM +0900, Damien Le Moal wrote:
> On 2022/03/25 6:37, Serge Semin wrote:
> > 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.
> 
> I agree with that change. Setting "rc = -ENOMEM" under the "if" checking for
> error is fine.
> 
> > 
> > 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@xxxxxxxxxxxxxxxxxxxx>
> >>> ---
> >>>  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.
> 

> No it is not. We have code reviews and testing to catch mistakes. If we start
> thinking along these lines, any useless local variable initialization can be
> justified. So let's not go there please.

Ok. That's up to the maintainer to decide after all. I'll just drop the
initialization as you say then.

-Sergey

> 
> 
> -- 
> Damien Le Moal
> Western Digital Research