From: Chunyan Zhang <[email protected]>
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 <[email protected]>
Signed-off-by: Chunyan Zhang <[email protected]>
---
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
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 <[email protected]>
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
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 <[email protected]>
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/[email protected]
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 <[email protected]>
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/[email protected]