Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 29 Nov 2022 08:19:56 +0300
From:   Dan Carpenter <error27@gmail.com>
To:     Conor Dooley <conor.dooley@microchip.com>
Cc:     Anup Patel <anup@brainfault.org>, oe-kbuild@lists.linux.dev,
        Randy Dunlap <rdunlap@infradead.org>, lkp@intel.com,
        oe-kbuild-all@lists.linux.dev, linux-kernel@vger.kernel.org,
        Palmer Dabbelt <palmer@rivosinc.com>
Subject: Re: drivers/cpuidle/cpuidle-riscv-sbi.c:506 sbi_genpd_probe() warn:
 missing error code 'ret'
Message-ID: <Y4WWfMhC8Mba35+F@kadam>
References: <202211220541.Ck9NkitJ-lkp@intel.com>
 <CAAhSdy2A26U5gF_WSfzcdT7v-Fg9aaKXLyT4M=4ba_=WboupzA@mail.gmail.com>
 <Y39zJaTMWQk376zN@wendy>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y39zJaTMWQk376zN@wendy>
Precedence: bulk

On Thu, Nov 24, 2022 at 01:35:33PM +0000, Conor Dooley wrote:
> Hey Anup,
> 
> On Tue, Nov 22, 2022 at 01:43:38PM +0530, Anup Patel wrote:
> > On Tue, Nov 22, 2022 at 12:41 PM Dan Carpenter <error27@gmail.com> wrote:
> > >
> > > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > head:   eb7081409f94a9a8608593d0fb63a1aa3d6f95d8
> > > commit: f81f7861ee2aaa6f652f18e8f622547bdd379724 cpuidle: riscv: support non-SMP config
> > > date:   7 months ago
> > > config: riscv-randconfig-m031-20221121
> > > compiler: riscv64-linux-gcc (GCC) 12.1.0
> > >
> > > If you fix the issue, kindly add following tag where applicable
> > > | Reported-by: kernel test robot <lkp@intel.com>
> > > | Reported-by: Dan Carpenter <error27@gmail.com>
> > >
> > > smatch warnings:
> > > drivers/cpuidle/cpuidle-riscv-sbi.c:506 sbi_genpd_probe() warn: missing error code 'ret'
> > >
> > > vim +/ret +506 drivers/cpuidle/cpuidle-riscv-sbi.c
> > >
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  481  static int sbi_genpd_probe(struct device_node *np)
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  482  {
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  483       struct device_node *node;
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  484       int ret = 0, pd_count = 0;
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  485
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  486       if (!np)
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  487               return -ENODEV;
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  488
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  489       /*
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  490        * Parse child nodes for the "#power-domain-cells" property and
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  491        * initialize a genpd/genpd-of-provider pair when it's found.
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  492        */
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  493       for_each_child_of_node(np, node) {
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  494               if (!of_find_property(node, "#power-domain-cells", NULL))
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  495                       continue;
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  496
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  497               ret = sbi_pd_init(node);
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  498               if (ret)
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  499                       goto put_node;
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  500
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  501               pd_count++;
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  502       }
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  503
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  504       /* Bail out if not using the hierarchical CPU topology. */
> > > 6abf32f1d9c500 Anup Patel 2022-02-10  505       if (!pd_count)
> > > 6abf32f1d9c500 Anup Patel 2022-02-10 @506               goto no_pd;
> > >
> > > Error code?
> > 
> > Yes, we intentionally "return 0" when there are no
> > generic power-domains defined for the CPUs, the
> > sbi_cpuidle_probe() continue further and try traditional
> > DT cpuidle states.
> 
> Happened upon this when looking for our other cpuidle conversation on
> lore earlier, would it not make more sense from a readability PoV to
> just return zero here?

I am always in favor of direct returns over a do nothing return because
of the ambiguity about error codes.  Also I just published a new Smatch
check where "return ret;" and "return 0;" are equivalent.

	ret = frob();
	if (ret)
		return ret;

	if (something else)
		return ret;

I have a different unpublished check for:

	ret = frob();
	if (!ret)
		return ret;

The bug I'm looking for here is that once or twice a year the !
character is unintentional.

regards,
dan carpenter