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;
Message-ID: <2c0860e2-40d6-ec91-60cf-2684edd52676@linaro.org>
Date:   Wed, 2 Aug 2023 14:19:26 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: drivers/power/supply/qcom_pmi8998_charger.c:565
 smb2_status_change_work() error: uninitialized symbol 'usb_online'.
Content-Language: en-US
To:     kernel test robot <dan.carpenter@linaro.org>,
        oe-kbuild@lists.linux.dev
Cc:     lkp@intel.com, Dan Carpenter <error27@gmail.com>,
        oe-kbuild-all@lists.linux.dev, linux-kernel@vger.kernel.org,
        Sebastian Reichel <sebastian.reichel@collabora.com>,
        Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
References: <ZMNWCD66TAhahJ2Y@kadam>
From:   Caleb Connolly <caleb.connolly@linaro.org>
In-Reply-To: <ZMNWCD66TAhahJ2Y@kadam>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 28/07/2023 06:45, kernel test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   57012c57536f8814dec92e74197ee96c3498d24e
> commit: 8648aeb5d7b70e13264ff5f444f22081d37d4670 power: supply: add Qualcomm PMI8998 SMB2 Charger driver
> config: arm-randconfig-m041-20230727 (https://download.01.org/0day-ci/archive/20230728/202307280638.556PrzIS-lkp@intel.com/config)
> compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
> reproduce: (https://download.01.org/0day-ci/archive/20230728/202307280638.556PrzIS-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
> | Closes: https://lore.kernel.org/r/202307280638.556PrzIS-lkp@intel.com/
> 
> smatch warnings:
> drivers/power/supply/qcom_pmi8998_charger.c:565 smb2_status_change_work() error: uninitialized symbol 'usb_online'.

Hi, thanks for the report.
> 
> vim +/usb_online +565 drivers/power/supply/qcom_pmi8998_charger.c
> 
> 8648aeb5d7b70e Caleb Connolly 2023-05-26  556  static void smb2_status_change_work(struct work_struct *work)
> 8648aeb5d7b70e Caleb Connolly 2023-05-26  557  {
> 8648aeb5d7b70e Caleb Connolly 2023-05-26  558  	unsigned int charger_type, current_ua;
> 8648aeb5d7b70e Caleb Connolly 2023-05-26  559  	int usb_online, count, rc;
> 8648aeb5d7b70e Caleb Connolly 2023-05-26  560  	struct smb2_chip *chip;
> 8648aeb5d7b70e Caleb Connolly 2023-05-26  561  
> 8648aeb5d7b70e Caleb Connolly 2023-05-26  562  	chip = container_of(work, struct smb2_chip, status_change_work.work);
> 8648aeb5d7b70e Caleb Connolly 2023-05-26  563  
> 8648aeb5d7b70e Caleb Connolly 2023-05-26  564  	smb2_get_prop_usb_online(chip, &usb_online);
> 
> This can only happen if regmap_read() fails, and in real life they
> can't actually fail can they?  We can't really recover if regmap
> breaks so in that situation this uninitialized variable would be the
> least of our concerns.  Right?

In this case, the driver is for a peripheral on the SPMI bus, a read
failing is extremely unlikely but under some conditions like bandwidth
constraints it could happen. Though admittedly there are likely bigger
issues to deal with in that situation heh.

It's a trivial fix so I'll send a patch over.
> 
> So what I could do is just delete the regmap_read error paths from
> the DB.  I just add these two lines to smatch_data/db/kernel.delete.return_states
> 
> regmap_read (-22)
> regmap_read (-4095)-(-1)
> 
> 8648aeb5d7b70e Caleb Connolly 2023-05-26 @565  	if (!usb_online)
>                                                      ^^^^^^^^^^
> 
> 8648aeb5d7b70e Caleb Connolly 2023-05-26  566  		return;
> 8648aeb5d7b70e Caleb Connolly 2023-05-26  567  
> 8648aeb5d7b70e Caleb Connolly 2023-05-26  568  	for (count = 0; count < 3; count++) {
> 8648aeb5d7b70e Caleb Connolly 2023-05-26  569  		dev_dbg(chip->dev, "get charger type retry %d\n", count);
> 

-- 
// Caleb (they/them)