diff mbox series

i2c: rk3x: fix potential spinlock recursion on poll

Message ID 20231206090641.18849-1-jensenhuang@friendlyarm.com
State Superseded
Headers show
Series i2c: rk3x: fix potential spinlock recursion on poll | expand

Commit Message

Jensen Huang Dec. 6, 2023, 9:06 a.m. UTC
Possible deadlock scenario (on reboot):
rk3x_i2c_xfer_common(polling)
    -> rk3x_i2c_wait_xfer_poll()
        -> rk3x_i2c_irq(0, i2c);
            --> spin_lock(&i2c->lock);
            ...
        <rk3x i2c interrupt>
        -> rk3x_i2c_irq(0, i2c);
            --> spin_lock(&i2c->lock); (deadlock here)

Store the IRQ number and disable/enable it around the polling transfer.
This patch has been tested on NanoPC-T4.

Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>
---
 drivers/i2c/busses/i2c-rk3x.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Heiko Stuebner Dec. 6, 2023, 9:29 a.m. UTC | #1
Am Mittwoch, 6. Dezember 2023, 10:06:40 CET schrieb Jensen Huang:
> Possible deadlock scenario (on reboot):
> rk3x_i2c_xfer_common(polling)
>     -> rk3x_i2c_wait_xfer_poll()
>         -> rk3x_i2c_irq(0, i2c);
>             --> spin_lock(&i2c->lock);
>             ...
>         <rk3x i2c interrupt>
>         -> rk3x_i2c_irq(0, i2c);
>             --> spin_lock(&i2c->lock); (deadlock here)
>
> Store the IRQ number and disable/enable it around the polling transfer.
> This patch has been tested on NanoPC-T4.
> 
> Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>


> ---
>  drivers/i2c/busses/i2c-rk3x.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index a044ca0c35a1..94514637c3bd 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -200,6 +200,7 @@ struct rk3x_i2c {
>  	struct clk *clk;
>  	struct clk *pclk;
>  	struct notifier_block clk_rate_nb;
> +	int irq;
>  
>  	/* Settings */
>  	struct i2c_timings t;
> @@ -1087,13 +1088,18 @@ static int rk3x_i2c_xfer_common(struct i2c_adapter *adap,
>  
>  		spin_unlock_irqrestore(&i2c->lock, flags);
>  
> -		rk3x_i2c_start(i2c);
> -
>  		if (!polling) {
> +			rk3x_i2c_start(i2c);
> +
>  			timeout = wait_event_timeout(i2c->wait, !i2c->busy,
>  						     msecs_to_jiffies(WAIT_TIMEOUT));
>  		} else {
> +			disable_irq(i2c->irq);
> +			rk3x_i2c_start(i2c);
> +
>  			timeout = rk3x_i2c_wait_xfer_poll(i2c);
> +
> +			enable_irq(i2c->irq);
>  		}
>  
>  		spin_lock_irqsave(&i2c->lock, flags);
> @@ -1310,6 +1316,8 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	i2c->irq = irq;
> +
>  	platform_set_drvdata(pdev, i2c);
>  
>  	if (i2c->soc_data->calc_timings == rk3x_i2c_v0_calc_timings) {
>
Andi Shyti Dec. 6, 2023, 12:27 p.m. UTC | #2
Hi Jensen,

On Wed, Dec 06, 2023 at 05:06:40PM +0800, Jensen Huang wrote:
> Possible deadlock scenario (on reboot):
> rk3x_i2c_xfer_common(polling)
>     -> rk3x_i2c_wait_xfer_poll()
>         -> rk3x_i2c_irq(0, i2c);
>             --> spin_lock(&i2c->lock);
>             ...
>         <rk3x i2c interrupt>
>         -> rk3x_i2c_irq(0, i2c);
>             --> spin_lock(&i2c->lock); (deadlock here)
> 
> Store the IRQ number and disable/enable it around the polling transfer.
> This patch has been tested on NanoPC-T4.
> 
> Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>

Looks good,

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi
kernel test robot Dec. 6, 2023, 8:36 p.m. UTC | #3
Hi Jensen,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rockchip/for-next]
[also build test WARNING on linus/master v6.7-rc4 next-20231206]
[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/Jensen-Huang/i2c-rk3x-fix-potential-spinlock-recursion-on-poll/20231206-170807
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
patch link:    https://lore.kernel.org/r/20231206090641.18849-1-jensenhuang%40friendlyarm.com
patch subject: [PATCH] i2c: rk3x: fix potential spinlock recursion on poll
config: arc-randconfig-r081-20231206 (https://download.01.org/0day-ci/archive/20231207/202312070421.Lccd0PSI-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231207/202312070421.Lccd0PSI-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/202312070421.Lccd0PSI-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/i2c/busses/i2c-rk3x.c:223: warning: Function parameter or member 'irq' not described in 'rk3x_i2c'


vim +223 drivers/i2c/busses/i2c-rk3x.c

c41aa3ce938b68 Max Schwarz  2014-06-11  171  
0a6ad2f95f36bf David Wu     2016-05-16  172  /**
0a6ad2f95f36bf David Wu     2016-05-16  173   * struct rk3x_i2c - private data of the controller
0a6ad2f95f36bf David Wu     2016-05-16  174   * @adap: corresponding I2C adapter
0a6ad2f95f36bf David Wu     2016-05-16  175   * @dev: device for this controller
0a6ad2f95f36bf David Wu     2016-05-16  176   * @soc_data: related soc data struct
0a6ad2f95f36bf David Wu     2016-05-16  177   * @regs: virtual memory area
7e086c3fc2df09 David Wu     2016-05-16  178   * @clk: function clk for rk3399 or function & Bus clks for others
7e086c3fc2df09 David Wu     2016-05-16  179   * @pclk: Bus clk for rk3399
0a6ad2f95f36bf David Wu     2016-05-16  180   * @clk_rate_nb: i2c clk rate change notify
0a6ad2f95f36bf David Wu     2016-05-16  181   * @t: I2C known timing information
0a6ad2f95f36bf David Wu     2016-05-16  182   * @lock: spinlock for the i2c bus
0a6ad2f95f36bf David Wu     2016-05-16  183   * @wait: the waitqueue to wait for i2c transfer
0a6ad2f95f36bf David Wu     2016-05-16  184   * @busy: the condition for the event to wait for
0a6ad2f95f36bf David Wu     2016-05-16  185   * @msg: current i2c message
0a6ad2f95f36bf David Wu     2016-05-16  186   * @addr: addr of i2c slave device
0a6ad2f95f36bf David Wu     2016-05-16  187   * @mode: mode of i2c transfer
0a6ad2f95f36bf David Wu     2016-05-16  188   * @is_last_msg: flag determines whether it is the last msg in this transfer
0a6ad2f95f36bf David Wu     2016-05-16  189   * @state: state of i2c transfer
0a6ad2f95f36bf David Wu     2016-05-16  190   * @processed: byte length which has been send or received
0a6ad2f95f36bf David Wu     2016-05-16  191   * @error: error code for i2c transfer
0a6ad2f95f36bf David Wu     2016-05-16  192   */
c41aa3ce938b68 Max Schwarz  2014-06-11  193  struct rk3x_i2c {
c41aa3ce938b68 Max Schwarz  2014-06-11  194  	struct i2c_adapter adap;
c41aa3ce938b68 Max Schwarz  2014-06-11  195  	struct device *dev;
d032a2eb2e3b9f Julia Lawall 2018-01-02  196  	const struct rk3x_i2c_soc_data *soc_data;
c41aa3ce938b68 Max Schwarz  2014-06-11  197  
c41aa3ce938b68 Max Schwarz  2014-06-11  198  	/* Hardware resources */
c41aa3ce938b68 Max Schwarz  2014-06-11  199  	void __iomem *regs;
c41aa3ce938b68 Max Schwarz  2014-06-11  200  	struct clk *clk;
7e086c3fc2df09 David Wu     2016-05-16  201  	struct clk *pclk;
249051f49907e7 Max Schwarz  2014-11-20  202  	struct notifier_block clk_rate_nb;
58bb392dd4c7fc Jensen Huang 2023-12-06  203  	int irq;
c41aa3ce938b68 Max Schwarz  2014-06-11  204  
c41aa3ce938b68 Max Schwarz  2014-06-11  205  	/* Settings */
1ab92956d4aac5 David Wu     2016-03-17  206  	struct i2c_timings t;
c41aa3ce938b68 Max Schwarz  2014-06-11  207  
c41aa3ce938b68 Max Schwarz  2014-06-11  208  	/* Synchronization & notification */
c41aa3ce938b68 Max Schwarz  2014-06-11  209  	spinlock_t lock;
c41aa3ce938b68 Max Schwarz  2014-06-11  210  	wait_queue_head_t wait;
c41aa3ce938b68 Max Schwarz  2014-06-11  211  	bool busy;
c41aa3ce938b68 Max Schwarz  2014-06-11  212  
c41aa3ce938b68 Max Schwarz  2014-06-11  213  	/* Current message */
c41aa3ce938b68 Max Schwarz  2014-06-11  214  	struct i2c_msg *msg;
c41aa3ce938b68 Max Schwarz  2014-06-11  215  	u8 addr;
c41aa3ce938b68 Max Schwarz  2014-06-11  216  	unsigned int mode;
c41aa3ce938b68 Max Schwarz  2014-06-11  217  	bool is_last_msg;
c41aa3ce938b68 Max Schwarz  2014-06-11  218  
c41aa3ce938b68 Max Schwarz  2014-06-11  219  	/* I2C state machine */
c41aa3ce938b68 Max Schwarz  2014-06-11  220  	enum rk3x_i2c_state state;
0a6ad2f95f36bf David Wu     2016-05-16  221  	unsigned int processed;
c41aa3ce938b68 Max Schwarz  2014-06-11  222  	int error;
c41aa3ce938b68 Max Schwarz  2014-06-11 @223  };
c41aa3ce938b68 Max Schwarz  2014-06-11  224
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index a044ca0c35a1..94514637c3bd 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -200,6 +200,7 @@  struct rk3x_i2c {
 	struct clk *clk;
 	struct clk *pclk;
 	struct notifier_block clk_rate_nb;
+	int irq;
 
 	/* Settings */
 	struct i2c_timings t;
@@ -1087,13 +1088,18 @@  static int rk3x_i2c_xfer_common(struct i2c_adapter *adap,
 
 		spin_unlock_irqrestore(&i2c->lock, flags);
 
-		rk3x_i2c_start(i2c);
-
 		if (!polling) {
+			rk3x_i2c_start(i2c);
+
 			timeout = wait_event_timeout(i2c->wait, !i2c->busy,
 						     msecs_to_jiffies(WAIT_TIMEOUT));
 		} else {
+			disable_irq(i2c->irq);
+			rk3x_i2c_start(i2c);
+
 			timeout = rk3x_i2c_wait_xfer_poll(i2c);
+
+			enable_irq(i2c->irq);
 		}
 
 		spin_lock_irqsave(&i2c->lock, flags);
@@ -1310,6 +1316,8 @@  static int rk3x_i2c_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	i2c->irq = irq;
+
 	platform_set_drvdata(pdev, i2c);
 
 	if (i2c->soc_data->calc_timings == rk3x_i2c_v0_calc_timings) {