diff mbox series

i2c: qup: Add error handling in qup_i2c_xfer_v2()

Message ID 20250519141918.2522-1-vulab@iscas.ac.cn
State New
Headers show
Series i2c: qup: Add error handling in qup_i2c_xfer_v2() | expand

Commit Message

Wentao Liang May 19, 2025, 2:19 p.m. UTC 
The qup_i2c_xfer_v2() calls the qup_i2c_change_state() but does
not check its return value. A proper implementation can be
found in qup_i2c_xfer().

Add error handling for qup_i2c_change_state(). If the function
fails, return the error code.

Fixes: 7545c7dba169 ("i2c: qup: reorganization of driver code to remove polling for qup v2")
Cc: stable@vger.kernel.org # v4.17
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/i2c/busses/i2c-qup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Andi Shyti May 20, 2025, 8:41 a.m. UTC | #1 
Hi Wentao,

On Mon, May 19, 2025 at 10:19:18PM +0800, Wentao Liang wrote:
> The qup_i2c_xfer_v2() calls the qup_i2c_change_state() but does
> not check its return value. A proper implementation can be
> found in qup_i2c_xfer().
> 
> Add error handling for qup_i2c_change_state(). If the function
> fails, return the error code.
> 
> Fixes: 7545c7dba169 ("i2c: qup: reorganization of driver code to remove polling for qup v2")
> Cc: stable@vger.kernel.org # v4.17

no need to Cc stable here, it's not such a big issue.

> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  drivers/i2c/busses/i2c-qup.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
> index da20b4487c9a..2477f570fe86 100644
> --- a/drivers/i2c/busses/i2c-qup.c
> +++ b/drivers/i2c/busses/i2c-qup.c
> @@ -1538,7 +1538,7 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
>  			   int num)
>  {
>  	struct qup_i2c_dev *qup = i2c_get_adapdata(adap);
> -	int ret, idx = 0;
> +	int ret, err, idx = 0;
>  
>  	qup->bus_err = 0;
>  	qup->qup_err = 0;
> @@ -1588,7 +1588,9 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
>  		ret = qup_i2c_bus_active(qup, ONE_BYTE);
>  
>  	if (!ret)
> -		qup_i2c_change_state(qup, QUP_RESET_STATE);
> +		err = qup_i2c_change_state(qup, QUP_RESET_STATE);

This check was removed on purpose, not by accident in the commit
you pointed out in the Fixes tag. On the other hand I agree that
this needs to be checked, perhaps restoring the original code:

	if (!ret)
		ret = qup_i2c_change_state(qup, QUP_RESET_STATE);

	if (ret == 0)
		ret = num;

What is exactly that you are trying to fix here? What is the
error you have faced?

Thanks,
Andi

> +	if (err)
> +		return err;
>  
>  	if (ret == 0)
>  		ret = num;

PS: your code can be refactored in a way that we don't need this
extra variable. E.g., something like this:

	if (!ret)
		ret = qup_i2c_bus_active(qup, ONE_BYTE);

	if (!ret) {
		ret = qup_i2c_change_state(qup, QUP_RESET_STATE);
		if (ret)
			return ret;
		ret = num;
	}

Looks the same, no? But I think the original version should work
better.
Mukesh Kumar Savaliya May 20, 2025, 11:16 a.m. UTC | #2 
On 5/19/2025 7:49 PM, Wentao Liang wrote:
> The qup_i2c_xfer_v2() calls the qup_i2c_change_state() but does
> not check its return value. A proper implementation can be
> found in qup_i2c_xfer().
> 
> Add error handling for qup_i2c_change_state(). If the function
> fails, return the error code.
> 
> Fixes: 7545c7dba169 ("i2c: qup: reorganization of driver code to remove polling for qup v2")
> Cc: stable@vger.kernel.org # v4.17
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>   drivers/i2c/busses/i2c-qup.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
> index da20b4487c9a..2477f570fe86 100644
> --- a/drivers/i2c/busses/i2c-qup.c
> +++ b/drivers/i2c/busses/i2c-qup.c
> @@ -1538,7 +1538,7 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
>   			   int num)
>   {
>   	struct qup_i2c_dev *qup = i2c_get_adapdata(adap);
> -	int ret, idx = 0;
> +	int ret, err, idx = 0;
>   
>   	qup->bus_err = 0;
>   	qup->qup_err = 0;
> @@ -1588,7 +1588,9 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
>   		ret = qup_i2c_bus_active(qup, ONE_BYTE);
>   
>   	if (!ret)
> -		qup_i2c_change_state(qup, QUP_RESET_STATE);
> +		err = qup_i2c_change_state(qup, QUP_RESET_STATE);
> +	if (err)
> +		return err;
Is there an error seen around this ? Expecting this to work as is.
After an error, what next ? Just return back to framework ?
>   
>   	if (ret == 0)
>   		ret = num;
Andi Shyti May 20, 2025, 11:51 a.m. UTC | #3 
Hi Mukesh,

...

> > @@ -1588,7 +1588,9 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
> >   		ret = qup_i2c_bus_active(qup, ONE_BYTE);
> >   	if (!ret)
> > -		qup_i2c_change_state(qup, QUP_RESET_STATE);
> > +		err = qup_i2c_change_state(qup, QUP_RESET_STATE);
> > +	if (err)
> > +		return err;
> Is there an error seen around this ? Expecting this to work as is.
> After an error, what next ? Just return back to framework ?

thanks for chiming in. qup_i2c_change_state() can fail, why
shouldn't we consider the possibility to fail?

Thanks,
Andi
kernel test robot May 20, 2025, 8:30 p.m. UTC | #4 
Hi Wentao,

kernel test robot noticed the following build warnings:

[auto build test WARNING on andi-shyti/i2c/i2c-host]
[also build test WARNING on linus/master v6.15-rc7 next-20250516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Wentao-Liang/i2c-qup-Add-error-handling-in-qup_i2c_xfer_v2/20250519-222137
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20250519141918.2522-1-vulab%40iscas.ac.cn
patch subject: [PATCH] i2c: qup: Add error handling in qup_i2c_xfer_v2()
config: hexagon-randconfig-001-20250521 (https://download.01.org/0day-ci/archive/20250521/202505210438.G4nfkpQ2-lkp@intel.com/config)
compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250521/202505210438.G4nfkpQ2-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>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505210438.G4nfkpQ2-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/i2c/busses/i2c-qup.c:1619:6: warning: variable 'err' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    1619 |         if (!ret)
         |             ^~~~
   drivers/i2c/busses/i2c-qup.c:1621:6: note: uninitialized use occurs here
    1621 |         if (err)
         |             ^~~
   drivers/i2c/busses/i2c-qup.c:1619:2: note: remove the 'if' if its condition is always true
    1619 |         if (!ret)
         |         ^~~~~~~~~
    1620 |                 err = qup_i2c_change_state(qup, QUP_RESET_STATE);
   drivers/i2c/busses/i2c-qup.c:1570:14: note: initialize the variable 'err' to silence this warning
    1570 |         int ret, err, idx = 0;
         |                     ^
         |                      = 0
   1 warning generated.


vim +1619 drivers/i2c/busses/i2c-qup.c

7545c7dba169c4 Abhishek Sahu         2018-03-12  1564  
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1565  static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1566  			   struct i2c_msg msgs[],
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1567  			   int num)
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1568  {
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1569  	struct qup_i2c_dev *qup = i2c_get_adapdata(adap);
61f647e9d36d67 Wentao Liang          2025-05-19  1570  	int ret, err, idx = 0;
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1571  
fbf9921f8b35d9 Sricharan Ramabadhran 2016-06-10  1572  	qup->bus_err = 0;
fbf9921f8b35d9 Sricharan Ramabadhran 2016-06-10  1573  	qup->qup_err = 0;
fbf9921f8b35d9 Sricharan Ramabadhran 2016-06-10  1574  
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1575  	ret = pm_runtime_get_sync(qup->dev);
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1576  	if (ret < 0)
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1577  		goto out;
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1578  
7545c7dba169c4 Abhishek Sahu         2018-03-12  1579  	ret = qup_i2c_determine_mode_v2(qup, msgs, num);
7545c7dba169c4 Abhishek Sahu         2018-03-12  1580  	if (ret)
7545c7dba169c4 Abhishek Sahu         2018-03-12  1581  		goto out;
7545c7dba169c4 Abhishek Sahu         2018-03-12  1582  
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1583  	writel(1, qup->base + QUP_SW_RESET);
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1584  	ret = qup_i2c_poll_state(qup, QUP_RESET_STATE);
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1585  	if (ret)
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1586  		goto out;
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1587  
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1588  	/* Configure QUP as I2C mini core */
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1589  	writel(I2C_MINI_CORE | I2C_N_VAL_V2, qup->base + QUP_CONFIG);
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1590  	writel(QUP_V2_TAGS_EN, qup->base + QUP_I2C_MASTER_GEN);
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1591  
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1592  	if (qup_i2c_poll_state_i2c_master(qup)) {
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1593  		ret = -EIO;
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1594  		goto out;
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1595  	}
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1596  
eb422b539c1f39 Abhishek Sahu         2018-03-12  1597  	if (qup->use_dma) {
7545c7dba169c4 Abhishek Sahu         2018-03-12  1598  		reinit_completion(&qup->xfer);
7545c7dba169c4 Abhishek Sahu         2018-03-12  1599  		ret = qup_i2c_bam_xfer(adap, &msgs[0], num);
eb422b539c1f39 Abhishek Sahu         2018-03-12  1600  		qup->use_dma = false;
9cedf3b2f09946 Sricharan Ramabadhran 2016-02-22  1601  	} else {
7545c7dba169c4 Abhishek Sahu         2018-03-12  1602  		qup_i2c_conf_mode_v2(qup);
7545c7dba169c4 Abhishek Sahu         2018-03-12  1603  
7545c7dba169c4 Abhishek Sahu         2018-03-12  1604  		for (idx = 0; idx < num; idx++) {
7545c7dba169c4 Abhishek Sahu         2018-03-12  1605  			qup->msg = &msgs[idx];
7545c7dba169c4 Abhishek Sahu         2018-03-12  1606  			qup->is_last = idx == (num - 1);
7545c7dba169c4 Abhishek Sahu         2018-03-12  1607  
7545c7dba169c4 Abhishek Sahu         2018-03-12  1608  			ret = qup_i2c_xfer_v2_msg(qup, idx,
7545c7dba169c4 Abhishek Sahu         2018-03-12  1609  					!!(msgs[idx].flags & I2C_M_RD));
7545c7dba169c4 Abhishek Sahu         2018-03-12  1610  			if (ret)
7545c7dba169c4 Abhishek Sahu         2018-03-12  1611  				break;
7545c7dba169c4 Abhishek Sahu         2018-03-12  1612  		}
7545c7dba169c4 Abhishek Sahu         2018-03-12  1613  		qup->msg = NULL;
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1614  	}
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1615  
f74187932d30e4 Sricharan Ramabadhran 2016-01-19  1616  	if (!ret)
7545c7dba169c4 Abhishek Sahu         2018-03-12  1617  		ret = qup_i2c_bus_active(qup, ONE_BYTE);
7545c7dba169c4 Abhishek Sahu         2018-03-12  1618  
7545c7dba169c4 Abhishek Sahu         2018-03-12 @1619  	if (!ret)
61f647e9d36d67 Wentao Liang          2025-05-19  1620  		err = qup_i2c_change_state(qup, QUP_RESET_STATE);
61f647e9d36d67 Wentao Liang          2025-05-19  1621  	if (err)
61f647e9d36d67 Wentao Liang          2025-05-19  1622  		return err;
f74187932d30e4 Sricharan Ramabadhran 2016-01-19  1623  
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1624  	if (ret == 0)
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1625  		ret = num;
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1626  out:
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1627  	pm_runtime_mark_last_busy(qup->dev);
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1628  	pm_runtime_put_autosuspend(qup->dev);
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1629  
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1630  	return ret;
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1631  }
191424bb6166f6 Sricharan Ramabadhran 2016-01-19  1632
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index da20b4487c9a..2477f570fe86 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -1538,7 +1538,7 @@  static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
 			   int num)
 {
 	struct qup_i2c_dev *qup = i2c_get_adapdata(adap);
-	int ret, idx = 0;
+	int ret, err, idx = 0;
 
 	qup->bus_err = 0;
 	qup->qup_err = 0;
@@ -1588,7 +1588,9 @@  static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
 		ret = qup_i2c_bus_active(qup, ONE_BYTE);
 
 	if (!ret)
-		qup_i2c_change_state(qup, QUP_RESET_STATE);
+		err = qup_i2c_change_state(qup, QUP_RESET_STATE);
+	if (err)
+		return err;
 
 	if (ret == 0)
 		ret = num;