| Message ID | 20220324001628.13028-4-Sergey.Semin@baikalelectronics.ru |
|---|---|
| State | New |
| Headers | show |
| Series | ata: ahci: Add DWC/Baikal-T1 AHCI SATA support | expand |
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 > 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; You I did say that I do not like this. Especially considering that this function uses rc only in the case of an error return. Having it uselessly initialized to 0 does not make sense and goes against your goal of avoiding problems in case it is not set in an error path. returning "ERR_PTR(0)" is the same as returning NULL, which defeats the purpose of using ERR_PTR/PTR_ERR. Please drop this patch. > 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); >
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@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. > > 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
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; + } 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> --- drivers/ata/libahci_platform.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)