| Message ID | 20220511231810.4928-6-Sergey.Semin@baikalelectronics.ru |
|---|---|
| State | New |
| Headers | show |
| Series | ata: ahci: Add DWC/Baikal-T1 AHCI SATA support | expand |
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. 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. Cheers, Hannes
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. > 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. Yes. Let's drop this patch. Not improving anything. > > Cheers, > > Hannes
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
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);
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(-)