| Message ID | 20201211102248.1018374-1-zhang.lyra@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: sprd: use a specific timeout to avoid system hang up issue | expand |
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
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
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; }