[PATCH] i2c: sprd: use a specific timeout to avoid system hang up issue

[PATCH] i2c: sprd: use a specific timeout to avoid system hang up issue

[Chunyan Zhang]

From: Chunyan Zhang <chunyan.zhang@unisoc.com>

If the i2c device SCL bus being pulled up due to some exception before
message transfer done, the system cannot receive the completing interrupt
signal any more, it would not exit waiting loop until MAX_SCHEDULE_TIMEOUT
jiffies eclipse, that would make the system seemed hang up. To avoid that
happen, this patch adds a specific timeout for message transfer.

Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
Original-by: Linhua Xu <linhua.xu@unisoc.com>
Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/i2c/busses/i2c-sprd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index 19cda6742423..dba3d526444e 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -72,6 +72,8 @@
 
 /* timeout (ms) for pm runtime autosuspend */
 #define SPRD_I2C_PM_TIMEOUT	1000
+/* timeout (ms) for transfer message */
+#define IC2_XFER_TIMEOUT	1000
 
 /* SPRD i2c data structure */
 struct sprd_i2c {
@@ -244,6 +246,7 @@ static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
 			       struct i2c_msg *msg, bool is_last_msg)
 {
 	struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
+	unsigned long timeout = msecs_to_jiffies(I2C_XFER_TIMEOUT);
 
 	i2c_dev->msg = msg;
 	i2c_dev->buf = msg->buf;
@@ -273,7 +276,9 @@ static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
 
 	sprd_i2c_opt_start(i2c_dev);
 
-	wait_for_completion(&i2c_dev->complete);
+	timeout = wait_for_completion_timeout(&i2c_dev->complete, timeout);
+	if (!timeout)
+		return -EIO;
 
 	return i2c_dev->err;
 }
-- 
2.25.1

Re: [PATCH] i2c: sprd: use a specific timeout to avoid system hang up issue

[Wolfram Sang]

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

Hi,

thanks for your patch!

> If the i2c device SCL bus being pulled up due to some exception before
> message transfer done, the system cannot receive the completing interrupt
> signal any more, it would not exit waiting loop until MAX_SCHEDULE_TIMEOUT
> jiffies eclipse, that would make the system seemed hang up. To avoid that
> happen, this patch adds a specific timeout for message transfer.

Yes.

> Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
> Original-by: Linhua Xu <linhua.xu@unisoc.com>

I can't find this tag documented. Maybe "Co-developed by"? Or just
"Signed-off-by"?

> +	unsigned long timeout = msecs_to_jiffies(I2C_XFER_TIMEOUT);
>  
>  	i2c_dev->msg = msg;
>  	i2c_dev->buf = msg->buf;
> @@ -273,7 +276,9 @@ static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
>  
>  	sprd_i2c_opt_start(i2c_dev);
>  
> -	wait_for_completion(&i2c_dev->complete);
> +	timeout = wait_for_completion_timeout(&i2c_dev->complete, timeout);
> +	if (!timeout)
> +		return -EIO;

Basically OK, but readability can be improved. Because it reads "if not
timeout, then return error". But it IS a timeout :) What about this:

	time_left = wait_for_completion_timeout(&i2c_dev->complete,
						msecs_to_jiffies(I2C_XFER_TIMEOUT));
	if (!time_left)
		...

and the rest adjusted accordingly. What do you think?

Kind regards,

   Wolfram


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

Re: [PATCH] i2c: sprd: use a specific timeout to avoid system hang up issue

[kernel test robot]

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

Hi Chunyan,

I love your patch! Yet something to improve:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v5.10-rc7 next-20201211]
[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]

url:    https://github.com/0day-ci/linux/commits/Chunyan-Zhang/i2c-sprd-use-a-specific-timeout-to-avoid-system-hang-up-issue/20201211-182817
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: mips-randconfig-r035-20201209 (attached as .config)
compiler: mipsel-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/725c61cfaa18f63c1fbc7f4a25a04a72c4fbda48
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Chunyan-Zhang/i2c-sprd-use-a-specific-timeout-to-avoid-system-hang-up-issue/20201211-182817
        git checkout 725c61cfaa18f63c1fbc7f4a25a04a72c4fbda48
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips 

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

All errors (new ones prefixed by >>):

   drivers/i2c/busses/i2c-sprd.c: In function 'sprd_i2c_handle_msg':
>> drivers/i2c/busses/i2c-sprd.c:249:43: error: 'I2C_XFER_TIMEOUT' undeclared (first use in this function); did you mean 'IC2_XFER_TIMEOUT'?
     249 |  unsigned long timeout = msecs_to_jiffies(I2C_XFER_TIMEOUT);
         |                                           ^~~~~~~~~~~~~~~~
         |                                           IC2_XFER_TIMEOUT
   drivers/i2c/busses/i2c-sprd.c:249:43: note: each undeclared identifier is reported only once for each function it appears in

vim +249 drivers/i2c/busses/i2c-sprd.c

   244	
   245	static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
   246				       struct i2c_msg *msg, bool is_last_msg)
   247	{
   248		struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
 > 249		unsigned long timeout = msecs_to_jiffies(I2C_XFER_TIMEOUT);
   250	
   251		i2c_dev->msg = msg;
   252		i2c_dev->buf = msg->buf;
   253		i2c_dev->count = msg->len;
   254	
   255		reinit_completion(&i2c_dev->complete);
   256		sprd_i2c_reset_fifo(i2c_dev);
   257		sprd_i2c_set_devaddr(i2c_dev, msg);
   258		sprd_i2c_set_count(i2c_dev, msg->len);
   259	
   260		if (msg->flags & I2C_M_RD) {
   261			sprd_i2c_opt_mode(i2c_dev, 1);
   262			sprd_i2c_send_stop(i2c_dev, 1);
   263		} else {
   264			sprd_i2c_opt_mode(i2c_dev, 0);
   265			sprd_i2c_send_stop(i2c_dev, !!is_last_msg);
   266		}
   267	
   268		/*
   269		 * We should enable rx fifo full interrupt to get data when receiving
   270		 * full data.
   271		 */
   272		if (msg->flags & I2C_M_RD)
   273			sprd_i2c_set_fifo_full_int(i2c_dev, 1);
   274		else
   275			sprd_i2c_data_transfer(i2c_dev);
   276	
   277		sprd_i2c_opt_start(i2c_dev);
   278	
   279		timeout = wait_for_completion_timeout(&i2c_dev->complete, timeout);
   280		if (!timeout)
   281			return -EIO;
   282	
   283		return i2c_dev->err;
   284	}
   285	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

Re: [PATCH] i2c: sprd: use a specific timeout to avoid system hang up issue

[kernel test robot]

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

Hi Chunyan,

I love your patch! Yet something to improve:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v5.10-rc7 next-20201211]
[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]

url:    https://github.com/0day-ci/linux/commits/Chunyan-Zhang/i2c-sprd-use-a-specific-timeout-to-avoid-system-hang-up-issue/20201211-182817
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: arm-randconfig-r012-20201210 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 5ff35356f1af2bb92785b38c657463924d9ec386)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/725c61cfaa18f63c1fbc7f4a25a04a72c4fbda48
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Chunyan-Zhang/i2c-sprd-use-a-specific-timeout-to-avoid-system-hang-up-issue/20201211-182817
        git checkout 725c61cfaa18f63c1fbc7f4a25a04a72c4fbda48
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

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

All errors (new ones prefixed by >>):

>> drivers/i2c/busses/i2c-sprd.c:249:43: error: use of undeclared identifier 'I2C_XFER_TIMEOUT'
           unsigned long timeout = msecs_to_jiffies(I2C_XFER_TIMEOUT);
                                                    ^
   1 error generated.

vim +/I2C_XFER_TIMEOUT +249 drivers/i2c/busses/i2c-sprd.c

   244	
   245	static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
   246				       struct i2c_msg *msg, bool is_last_msg)
   247	{
   248		struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
 > 249		unsigned long timeout = msecs_to_jiffies(I2C_XFER_TIMEOUT);
   250	
   251		i2c_dev->msg = msg;
   252		i2c_dev->buf = msg->buf;
   253		i2c_dev->count = msg->len;
   254	
   255		reinit_completion(&i2c_dev->complete);
   256		sprd_i2c_reset_fifo(i2c_dev);
   257		sprd_i2c_set_devaddr(i2c_dev, msg);
   258		sprd_i2c_set_count(i2c_dev, msg->len);
   259	
   260		if (msg->flags & I2C_M_RD) {
   261			sprd_i2c_opt_mode(i2c_dev, 1);
   262			sprd_i2c_send_stop(i2c_dev, 1);
   263		} else {
   264			sprd_i2c_opt_mode(i2c_dev, 0);
   265			sprd_i2c_send_stop(i2c_dev, !!is_last_msg);
   266		}
   267	
   268		/*
   269		 * We should enable rx fifo full interrupt to get data when receiving
   270		 * full data.
   271		 */
   272		if (msg->flags & I2C_M_RD)
   273			sprd_i2c_set_fifo_full_int(i2c_dev, 1);
   274		else
   275			sprd_i2c_data_transfer(i2c_dev);
   276	
   277		sprd_i2c_opt_start(i2c_dev);
   278	
   279		timeout = wait_for_completion_timeout(&i2c_dev->complete, timeout);
   280		if (!timeout)
   281			return -EIO;
   282	
   283		return i2c_dev->err;
   284	}
   285	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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