From: Serge Semin <fancer.lancer@gmail.com>
To: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>,
Hans de Goede <hdegoede@redhat.com>, Jens Axboe <axboe@kernel.dk>,
Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>,
Rob Herring <robh+dt@kernel.org>,
linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org
Subject: Re: [PATCH 03/21] ata: libahci_platform: Explicitly set rc on devres_alloc failure
Date: Wed, 6 Apr 2022 23:03:33 +0300 [thread overview]
Message-ID: <20220406200333.3olw47az7eqjqnyq@mobilestation> (raw)
In-Reply-To: <f163f982-852f-027f-8d24-e624e45fbbf1@opensource.wdc.com>
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
next prev parent reply other threads:[~2022-04-06 21:16 UTC|newest]
Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-24 0:16 [PATCH 00/21] ata: ahci: Add DWC/Baikal-T1 AHCI SATA support Serge Semin
2022-03-24 0:16 ` [PATCH 01/21] dt-bindings: ata: sata: Extend number of SATA ports Serge Semin
2022-03-29 8:15 ` Damien Le Moal
2022-04-11 19:25 ` Serge Semin
2022-03-24 0:16 ` [PATCH 02/21] dt-bindings: ata: Convert AHCI-bindings to DT schema Serge Semin
2022-03-28 19:32 ` Rob Herring
2022-04-11 19:34 ` Serge Semin
2022-03-24 0:16 ` [PATCH 03/21] ata: libahci_platform: Explicitly set rc on devres_alloc failure Serge Semin
2022-03-24 0:58 ` Damien Le Moal
2022-03-24 21:37 ` Serge Semin
2022-03-25 1:56 ` Damien Le Moal
2022-04-06 20:03 ` Serge Semin [this message]
2022-03-29 8:20 ` Damien Le Moal
2022-03-24 0:16 ` [PATCH 04/21] ata: libahci_platform: Convert to using handy devm-ioremap methods Serge Semin
2022-03-24 1:11 ` Damien Le Moal
2022-04-06 20:42 ` Serge Semin
2022-03-24 0:16 ` [PATCH 05/21] ata: libahci_platform: Convert to using devm bulk clocks API Serge Semin
2022-03-24 1:29 ` Damien Le Moal
2022-04-09 4:59 ` Serge Semin
2022-03-28 22:36 ` kernel test robot
2022-03-28 23:42 ` kernel test robot
2022-03-29 0:03 ` kernel test robot
2022-03-24 0:16 ` [PATCH 06/21] ata: libahci_platform: Add function returning a clock-handle by id Serge Semin
2022-03-24 1:36 ` Damien Le Moal
2022-04-11 6:02 ` Serge Semin
2022-03-24 0:16 ` [PATCH 07/21] ata: libahci_platform: Sanity check the DT child nodes number Serge Semin
2022-03-24 1:40 ` Damien Le Moal
2022-03-24 8:12 ` Sergey Shtylyov
2022-03-24 8:13 ` Damien Le Moal
2022-04-11 13:10 ` Serge Semin
2022-03-24 0:16 ` [PATCH 08/21] ata: libahci_platform: Parse ports-implemented property in resources getter Serge Semin
2022-03-24 0:16 ` [PATCH 09/21] ata: libahci_platform: Introduce reset assertion/deassertion methods Serge Semin
2022-03-24 1:52 ` Damien Le Moal
2022-04-11 10:57 ` Serge Semin
2022-03-24 0:16 ` [PATCH 10/21] dt-bindings: ata: ahci: Add platform capability properties Serge Semin
2022-03-24 0:16 ` [PATCH 11/21] ata: libahci: Extend port-cmd flags set with port capabilities Serge Semin
2022-03-24 0:16 ` [PATCH 12/21] ata: libahci: Discard redundant force_port_map parameter Serge Semin
2022-03-24 2:05 ` Damien Le Moal
2022-04-11 12:11 ` Serge Semin
2022-04-11 12:25 ` Damien Le Moal
2022-04-11 20:52 ` Serge Semin
2022-04-11 22:48 ` Damien Le Moal
2022-04-12 12:29 ` Serge Semin
2022-03-24 0:16 ` [PATCH 13/21] ata: libahci: Don't read AHCI version twice in the save-config method Serge Semin
2022-03-24 0:16 ` [PATCH 14/21] ata: ahci: Convert __ahci_port_base to accepting hpriv as arguments Serge Semin
2022-03-24 0:16 ` [PATCH 15/21] ata: ahci: Introduce firmware-specific caps initialization Serge Semin
2022-03-24 0:16 ` [PATCH 16/21] dt-bindings: ata: ahci: Add DWC AHCI SATA controller DT schema Serge Semin
2022-04-01 0:06 ` Rob Herring
2022-04-11 20:00 ` Serge Semin
2022-03-24 0:16 ` [PATCH 17/21] ata: ahci: Add DWC AHCI SATA controller support Serge Semin
2022-03-24 2:21 ` Damien Le Moal
2022-04-11 12:41 ` Serge Semin
2022-04-11 13:03 ` Damien Le Moal
2022-04-11 20:41 ` Serge Semin
2022-03-24 0:16 ` [PATCH 18/21] dt-bindings: ata: ahci: Add Baikal-T1 AHCI SATA controller DT schema Serge Semin
2022-03-24 0:16 ` [PATCH 19/21] ata: ahci-dwc: Add platform-specific quirks support Serge Semin
2022-03-24 0:16 ` [PATCH 20/21] ata: ahci-dwc: Add Baikal-T1 AHCI SATA interface support Serge Semin
2022-03-24 0:16 ` [PATCH 21/21] MAINTAINERS: Add maintainers for DWC AHCI SATA driver Serge Semin
2022-03-24 2:17 ` Damien Le Moal
2022-04-11 12:16 ` Serge Semin
2022-03-28 20:06 ` [PATCH 00/21] ata: ahci: Add DWC/Baikal-T1 AHCI SATA support Damien Le Moal
2022-03-29 8:30 ` Damien Le Moal
2022-04-06 19:54 ` Serge Semin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220406200333.3olw47az7eqjqnyq@mobilestation \
--to=fancer.lancer@gmail.com \
--cc=Alexey.Malahov@baikalelectronics.ru \
--cc=Pavel.Parkhomenko@baikalelectronics.ru \
--cc=Sergey.Semin@baikalelectronics.ru \
--cc=axboe@kernel.dk \
--cc=damien.lemoal@opensource.wdc.com \
--cc=devicetree@vger.kernel.org \
--cc=hdegoede@redhat.com \
--cc=linux-ide@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).