Re: [PATCH v3 05/23] ata: libahci_platform: Explicitly set rc on devres_alloc failure

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

On Thu, May 12, 2022 at 12:32:42PM +0200, Damien Le Moal wrote:
> On 2022/05/12 8:27, Hannes Reinecke wrote:
> > On 5/12/22 01:17, 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
> >> 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>
> >>
> >> ---
> >>
> >> Changelog v2:
> >> - Drop rc variable initialization (@Damien)
> >> ---
> >>   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 32495ae96567..f7f9bfcfc164 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, 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;
> >> +	}
> >>   
> >>   	devres_add(dev, hpriv);
> >>   

> > I disagree.
> > As 'rc' is now only initialized within a conditional we're risking 'rc' 
> > will be left uninitialized.

That's what I told to @Damien in v1.

> > And in the end, it's a matter of style; this patch doesn't change the 
> > flow of events and the benefits are hard to see.

As a first time reader of the module I've saw them. It was hard
to comprehend right from the code context whether rc was properly
initialized especially seeing there are so many local variables
defined. Unified rc-initialization approach makes code easier to read
for sure. In this case the rc variable is re-initialized in each
error-case. So having it defaulted to a value which is relevant to the
code block in the twenty lines below isn't the most optimized design.

> 
> Yes. Let's drop this patch. Not improving anything.

It's up to you to decide after all. Even though I disagree with your
opinions the patch will be dropped in v4.

-Sergey

> 
> > 
> > Cheers,
> > 
> > Hannes
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research