linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] power: supply: max17042_battery: fix some usage of uninitialized variables
@ 2019-10-03 17:29 Yizhuo
  2019-10-14  3:23 ` Sebastian Reichel
  0 siblings, 1 reply; 4+ messages in thread
From: Yizhuo @ 2019-10-03 17:29 UTC (permalink / raw)
  Cc: csong, zhiyunq, Yizhuo, Sebastian Reichel, linux-pm, linux-kernel

Several functions in this file are trying to use regmap_read() to
initialize the specific variable, however, if regmap_read() fails,
the variable could be uninitialized but used directly, which is
potentially unsafe. The return value of regmap_read() should be
checked and handled. The same case also happens in function
max17042_thread_handler() but it needs more effort to patch.

Signed-off-by: Yizhuo <yzhai003@ucr.edu>
---
 drivers/power/supply/max17042_battery.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index e6a2dacaa730..b897776a2749 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -675,8 +675,12 @@ static void max17042_reset_vfsoc0_reg(struct max17042_chip *chip)
 {
 	unsigned int vfSoc;
 	struct regmap *map = chip->regmap;
+	int ret;
+
+	ret = regmap_read(map, MAX17042_VFSOC, &vfSoc);
+	if (ret)
+		return;
 
-	regmap_read(map, MAX17042_VFSOC, &vfSoc);
 	regmap_write(map, MAX17042_VFSOC0Enable, VFSOC0_UNLOCK);
 	max17042_write_verify_reg(map, MAX17042_VFSOC0, vfSoc);
 	regmap_write(map, MAX17042_VFSOC0Enable, VFSOC0_LOCK);
@@ -686,12 +690,18 @@ static void max17042_load_new_capacity_params(struct max17042_chip *chip)
 {
 	u32 full_cap0, rep_cap, dq_acc, vfSoc;
 	u32 rem_cap;
+	int ret;
 
 	struct max17042_config_data *config = chip->pdata->config_data;
 	struct regmap *map = chip->regmap;
 
-	regmap_read(map, MAX17042_FullCAP0, &full_cap0);
-	regmap_read(map, MAX17042_VFSOC, &vfSoc);
+	ret = regmap_read(map, MAX17042_FullCAP0, &full_cap0);
+	if (ret)
+		return;
+
+	ret = regmap_read(map, MAX17042_VFSOC, &vfSoc);
+	if (ret)
+		return;
 
 	/* fg_vfSoc needs to shifted by 8 bits to get the
 	 * perc in 1% accuracy, to get the right rem_cap multiply
@@ -1108,7 +1118,12 @@ static int max17042_probe(struct i2c_client *client,
 	if (!client->irq)
 		regmap_write(chip->regmap, MAX17042_SALRT_Th, 0xff00);
 
-	regmap_read(chip->regmap, MAX17042_STATUS, &val);
+	ret = regmap_read(chip->regmap, MAX17042_STATUS, &val);
+	if (ret) {
+		dev_err(&client->dev, "Failed to get MAX17042_STATUS.\n");
+		return ret;
+	}
+
 	if (val & STATUS_POR_BIT) {
 		INIT_WORK(&chip->work, max17042_init_worker);
 		ret = devm_add_action(&client->dev, max17042_stop_work, chip);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] power: supply: max17042_battery: fix some usage of uninitialized variables
  2019-10-03 17:29 [PATCH] power: supply: max17042_battery: fix some usage of uninitialized variables Yizhuo
@ 2019-10-14  3:23 ` Sebastian Reichel
  0 siblings, 0 replies; 4+ messages in thread
From: Sebastian Reichel @ 2019-10-14  3:23 UTC (permalink / raw)
  To: Yizhuo; +Cc: csong, zhiyunq, linux-pm, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2968 bytes --]

Hi,

On Thu, Oct 03, 2019 at 10:29:48AM -0700, Yizhuo wrote:
> Several functions in this file are trying to use regmap_read() to
> initialize the specific variable, however, if regmap_read() fails,
> the variable could be uninitialized but used directly, which is
> potentially unsafe. The return value of regmap_read() should be
> checked and handled. The same case also happens in function
> max17042_thread_handler() but it needs more effort to patch.
> 
> Signed-off-by: Yizhuo <yzhai003@ucr.edu>
> ---

This does not improve the situation much. You should change the
functions from 'void' to 'int' return code and propagate the error
code, otherwise the following code assumes everything to be correct.
Also the Signed-off-by name and the patch author's name seems to be
incomplete.

Thanks,

-- Sebastian

>  drivers/power/supply/max17042_battery.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index e6a2dacaa730..b897776a2749 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -675,8 +675,12 @@ static void max17042_reset_vfsoc0_reg(struct max17042_chip *chip)
>  {
>  	unsigned int vfSoc;
>  	struct regmap *map = chip->regmap;
> +	int ret;
> +
> +	ret = regmap_read(map, MAX17042_VFSOC, &vfSoc);
> +	if (ret)
> +		return;
>  
> -	regmap_read(map, MAX17042_VFSOC, &vfSoc);
>  	regmap_write(map, MAX17042_VFSOC0Enable, VFSOC0_UNLOCK);
>  	max17042_write_verify_reg(map, MAX17042_VFSOC0, vfSoc);
>  	regmap_write(map, MAX17042_VFSOC0Enable, VFSOC0_LOCK);
> @@ -686,12 +690,18 @@ static void max17042_load_new_capacity_params(struct max17042_chip *chip)
>  {
>  	u32 full_cap0, rep_cap, dq_acc, vfSoc;
>  	u32 rem_cap;
> +	int ret;
>  
>  	struct max17042_config_data *config = chip->pdata->config_data;
>  	struct regmap *map = chip->regmap;
>  
> -	regmap_read(map, MAX17042_FullCAP0, &full_cap0);
> -	regmap_read(map, MAX17042_VFSOC, &vfSoc);
> +	ret = regmap_read(map, MAX17042_FullCAP0, &full_cap0);
> +	if (ret)
> +		return;
> +
> +	ret = regmap_read(map, MAX17042_VFSOC, &vfSoc);
> +	if (ret)
> +		return;
>  
>  	/* fg_vfSoc needs to shifted by 8 bits to get the
>  	 * perc in 1% accuracy, to get the right rem_cap multiply
> @@ -1108,7 +1118,12 @@ static int max17042_probe(struct i2c_client *client,
>  	if (!client->irq)
>  		regmap_write(chip->regmap, MAX17042_SALRT_Th, 0xff00);
>  
> -	regmap_read(chip->regmap, MAX17042_STATUS, &val);
> +	ret = regmap_read(chip->regmap, MAX17042_STATUS, &val);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to get MAX17042_STATUS.\n");
> +		return ret;
> +	}
> +
>  	if (val & STATUS_POR_BIT) {
>  		INIT_WORK(&chip->work, max17042_init_worker);
>  		ret = devm_add_action(&client->dev, max17042_stop_work, chip);
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] power: supply: max17042_battery: fix some usage of uninitialized variables
  2019-10-02 15:55 Yizhuo
@ 2019-10-02 20:51 ` kbuild test robot
  0 siblings, 0 replies; 4+ messages in thread
From: kbuild test robot @ 2019-10-02 20:51 UTC (permalink / raw)
  To: Yizhuo
  Cc: kbuild-all, csong, zhiyunq, Yizhuo, Sebastian Reichel, linux-pm,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3667 bytes --]

Hi Yizhuo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on power-supply/for-next]
[cannot apply to v5.4-rc1 next-20191002]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Yizhuo/power-supply-max17042_battery-fix-some-usage-of-uninitialized-variables/20191003-040948
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/power/supply/max17042_battery.c: In function 'max17042_load_new_capacity_params':
>> drivers/power/supply/max17042_battery.c:697:10: warning: 'return' with a value, in function returning void
      return ret;
             ^~~
   drivers/power/supply/max17042_battery.c:686:13: note: declared here
    static void max17042_load_new_capacity_params(struct max17042_chip *chip)
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/power/supply/max17042_battery.c:701:10: warning: 'return' with a value, in function returning void
      return ret;
             ^~~
   drivers/power/supply/max17042_battery.c:686:13: note: declared here
    static void max17042_load_new_capacity_params(struct max17042_chip *chip)
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/return +697 drivers/power/supply/max17042_battery.c

   685	
   686	static void max17042_load_new_capacity_params(struct max17042_chip *chip)
   687	{
   688		u32 full_cap0, rep_cap, dq_acc, vfSoc;
   689		u32 rem_cap;
   690		int ret;
   691	
   692		struct max17042_config_data *config = chip->pdata->config_data;
   693		struct regmap *map = chip->regmap;
   694	
   695		ret = regmap_read(map, MAX17042_FullCAP0, &full_cap0);
   696		if (ret)
 > 697			return ret;
   698	
   699		ret = regmap_read(map, MAX17042_VFSOC, &vfSoc);
   700		if (ret)
   701			return ret;
   702	
   703		/* fg_vfSoc needs to shifted by 8 bits to get the
   704		 * perc in 1% accuracy, to get the right rem_cap multiply
   705		 * full_cap0, fg_vfSoc and devide by 100
   706		 */
   707		rem_cap = ((vfSoc >> 8) * full_cap0) / 100;
   708		max17042_write_verify_reg(map, MAX17042_RemCap, rem_cap);
   709	
   710		rep_cap = rem_cap;
   711		max17042_write_verify_reg(map, MAX17042_RepCap, rep_cap);
   712	
   713		/* Write dQ_acc to 200% of Capacity and dP_acc to 200% */
   714		dq_acc = config->fullcap / dQ_ACC_DIV;
   715		max17042_write_verify_reg(map, MAX17042_dQacc, dq_acc);
   716		max17042_write_verify_reg(map, MAX17042_dPacc, dP_ACC_200);
   717	
   718		max17042_write_verify_reg(map, MAX17042_FullCAP,
   719				config->fullcap);
   720		regmap_write(map, MAX17042_DesignCap,
   721				config->design_cap);
   722		max17042_write_verify_reg(map, MAX17042_FullCAPNom,
   723				config->fullcapnom);
   724		/* Update SOC register with new SOC */
   725		regmap_write(map, MAX17042_RepSOC, vfSoc);
   726	}
   727	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51832 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [PATCH] power: supply: max17042_battery: fix some usage of uninitialized variables
@ 2019-10-02 15:55 Yizhuo
  2019-10-02 20:51 ` kbuild test robot
  0 siblings, 1 reply; 4+ messages in thread
From: Yizhuo @ 2019-10-02 15:55 UTC (permalink / raw)
  Cc: csong, zhiyunq, Yizhuo, Sebastian Reichel, linux-pm, linux-kernel

Several functions in this file are trying to use regmap_read() to
initialize the specific variable, however, if regmap_read() fails,
the variable could be uninitialized but used directly, which is
potentially unsafe. The return value of regmap_read() should be
checked and handled. The same case also happens in function
max17042_thread_handler() but it needs more effort to patch.

Signed-off-by: Yizhuo <yzhai003@ucr.edu>
---
 drivers/power/supply/max17042_battery.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index e6a2dacaa730..8e5727010ed2 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -675,8 +675,12 @@ static void max17042_reset_vfsoc0_reg(struct max17042_chip *chip)
 {
 	unsigned int vfSoc;
 	struct regmap *map = chip->regmap;
+	int ret;
+
+	ret = regmap_read(map, MAX17042_VFSOC, &vfSoc);
+	if (ret)
+		return;
 
-	regmap_read(map, MAX17042_VFSOC, &vfSoc);
 	regmap_write(map, MAX17042_VFSOC0Enable, VFSOC0_UNLOCK);
 	max17042_write_verify_reg(map, MAX17042_VFSOC0, vfSoc);
 	regmap_write(map, MAX17042_VFSOC0Enable, VFSOC0_LOCK);
@@ -686,12 +690,18 @@ static void max17042_load_new_capacity_params(struct max17042_chip *chip)
 {
 	u32 full_cap0, rep_cap, dq_acc, vfSoc;
 	u32 rem_cap;
+	int ret;
 
 	struct max17042_config_data *config = chip->pdata->config_data;
 	struct regmap *map = chip->regmap;
 
-	regmap_read(map, MAX17042_FullCAP0, &full_cap0);
-	regmap_read(map, MAX17042_VFSOC, &vfSoc);
+	ret = regmap_read(map, MAX17042_FullCAP0, &full_cap0);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(map, MAX17042_VFSOC, &vfSoc);
+	if (ret)
+		return ret;
 
 	/* fg_vfSoc needs to shifted by 8 bits to get the
 	 * perc in 1% accuracy, to get the right rem_cap multiply
@@ -1108,7 +1118,12 @@ static int max17042_probe(struct i2c_client *client,
 	if (!client->irq)
 		regmap_write(chip->regmap, MAX17042_SALRT_Th, 0xff00);
 
-	regmap_read(chip->regmap, MAX17042_STATUS, &val);
+	ret = regmap_read(chip->regmap, MAX17042_STATUS, &val);
+	if (ret) {
+		dev_err(&client->dev, "Failed to get MAX17042_STATUS.\n");
+		return ret;
+	}
+
 	if (val & STATUS_POR_BIT) {
 		INIT_WORK(&chip->work, max17042_init_worker);
 		ret = devm_add_action(&client->dev, max17042_stop_work, chip);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-14  3:23 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-03 17:29 [PATCH] power: supply: max17042_battery: fix some usage of uninitialized variables Yizhuo
2019-10-14  3:23 ` Sebastian Reichel
  -- strict thread matches above, loose matches on Subject: below --
2019-10-02 15:55 Yizhuo
2019-10-02 20:51 ` kbuild test robot

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).