diff mbox series

[1/6] i3c: master: svc: fix race condition in ibi work thread

Message ID 20231016153232.2851095-2-Frank.Li@nxp.com
State New
Headers
Series i3c: master: svc: collection of bugs fixes |

Commit Message

Frank Li Oct. 16, 2023, 3:32 p.m. UTC 
  The ibi work thread operates asynchronously with other transfers, such as
svc_i3c_master_priv_xfers(). Introduces mutex protection to ensure the
completion of the entire i3c/i2c transaction.

Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

kernel test robot Oct. 17, 2023, 1:33 p.m. UTC | #1 
Hi Frank,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.6-rc6 next-20231017]
[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/Frank-Li/i3c-master-svc-fix-race-condition-in-ibi-work-thread/20231017-151123
base:   linus/master
patch link:    https://lore.kernel.org/r/20231016153232.2851095-2-Frank.Li%40nxp.com
patch subject: [PATCH 1/6] i3c: master: svc: fix race condition in ibi work thread
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231017/202310172150.4GVdV44X-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231017/202310172150.4GVdV44X-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/202310172150.4GVdV44X-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/i3c/master/svc-i3c-master.c:207: warning: Function parameter or member 'lock' not described in 'svc_i3c_master'


vim +207 drivers/i3c/master/svc-i3c-master.c

1c5ee2a77b1bacd Clark Wang    2023-05-17  153  
dd3c52846d5954a Miquel Raynal 2021-01-21  154  /**
dd3c52846d5954a Miquel Raynal 2021-01-21  155   * struct svc_i3c_master - Silvaco I3C Master structure
dd3c52846d5954a Miquel Raynal 2021-01-21  156   * @base: I3C master controller
dd3c52846d5954a Miquel Raynal 2021-01-21  157   * @dev: Corresponding device
dd3c52846d5954a Miquel Raynal 2021-01-21  158   * @regs: Memory mapping
5496eac6ad7428f Miquel Raynal 2023-08-17  159   * @saved_regs: Volatile values for PM operations
dd3c52846d5954a Miquel Raynal 2021-01-21  160   * @free_slots: Bit array of available slots
dd3c52846d5954a Miquel Raynal 2021-01-21  161   * @addrs: Array containing the dynamic addresses of each attached device
dd3c52846d5954a Miquel Raynal 2021-01-21  162   * @descs: Array of descriptors, one per attached device
dd3c52846d5954a Miquel Raynal 2021-01-21  163   * @hj_work: Hot-join work
dd3c52846d5954a Miquel Raynal 2021-01-21  164   * @ibi_work: IBI work
dd3c52846d5954a Miquel Raynal 2021-01-21  165   * @irq: Main interrupt
dd3c52846d5954a Miquel Raynal 2021-01-21  166   * @pclk: System clock
dd3c52846d5954a Miquel Raynal 2021-01-21  167   * @fclk: Fast clock (bus)
dd3c52846d5954a Miquel Raynal 2021-01-21  168   * @sclk: Slow clock (other events)
dd3c52846d5954a Miquel Raynal 2021-01-21  169   * @xferqueue: Transfer queue structure
dd3c52846d5954a Miquel Raynal 2021-01-21  170   * @xferqueue.list: List member
dd3c52846d5954a Miquel Raynal 2021-01-21  171   * @xferqueue.cur: Current ongoing transfer
dd3c52846d5954a Miquel Raynal 2021-01-21  172   * @xferqueue.lock: Queue lock
dd3c52846d5954a Miquel Raynal 2021-01-21  173   * @ibi: IBI structure
dd3c52846d5954a Miquel Raynal 2021-01-21  174   * @ibi.num_slots: Number of slots available in @ibi.slots
dd3c52846d5954a Miquel Raynal 2021-01-21  175   * @ibi.slots: Available IBI slots
dd3c52846d5954a Miquel Raynal 2021-01-21  176   * @ibi.tbq_slot: To be queued IBI slot
dd3c52846d5954a Miquel Raynal 2021-01-21  177   * @ibi.lock: IBI lock
dd3c52846d5954a Miquel Raynal 2021-01-21  178   */
dd3c52846d5954a Miquel Raynal 2021-01-21  179  struct svc_i3c_master {
dd3c52846d5954a Miquel Raynal 2021-01-21  180  	struct i3c_master_controller base;
dd3c52846d5954a Miquel Raynal 2021-01-21  181  	struct device *dev;
dd3c52846d5954a Miquel Raynal 2021-01-21  182  	void __iomem *regs;
1c5ee2a77b1bacd Clark Wang    2023-05-17  183  	struct svc_i3c_regs_save saved_regs;
dd3c52846d5954a Miquel Raynal 2021-01-21  184  	u32 free_slots;
dd3c52846d5954a Miquel Raynal 2021-01-21  185  	u8 addrs[SVC_I3C_MAX_DEVS];
dd3c52846d5954a Miquel Raynal 2021-01-21  186  	struct i3c_dev_desc *descs[SVC_I3C_MAX_DEVS];
dd3c52846d5954a Miquel Raynal 2021-01-21  187  	struct work_struct hj_work;
dd3c52846d5954a Miquel Raynal 2021-01-21  188  	struct work_struct ibi_work;
dd3c52846d5954a Miquel Raynal 2021-01-21  189  	int irq;
dd3c52846d5954a Miquel Raynal 2021-01-21  190  	struct clk *pclk;
dd3c52846d5954a Miquel Raynal 2021-01-21  191  	struct clk *fclk;
dd3c52846d5954a Miquel Raynal 2021-01-21  192  	struct clk *sclk;
dd3c52846d5954a Miquel Raynal 2021-01-21  193  	struct {
dd3c52846d5954a Miquel Raynal 2021-01-21  194  		struct list_head list;
dd3c52846d5954a Miquel Raynal 2021-01-21  195  		struct svc_i3c_xfer *cur;
dd3c52846d5954a Miquel Raynal 2021-01-21  196  		/* Prevent races between transfers */
dd3c52846d5954a Miquel Raynal 2021-01-21  197  		spinlock_t lock;
dd3c52846d5954a Miquel Raynal 2021-01-21  198  	} xferqueue;
dd3c52846d5954a Miquel Raynal 2021-01-21  199  	struct {
dd3c52846d5954a Miquel Raynal 2021-01-21  200  		unsigned int num_slots;
dd3c52846d5954a Miquel Raynal 2021-01-21  201  		struct i3c_dev_desc **slots;
dd3c52846d5954a Miquel Raynal 2021-01-21  202  		struct i3c_ibi_slot *tbq_slot;
dd3c52846d5954a Miquel Raynal 2021-01-21  203  		/* Prevent races within IBI handlers */
dd3c52846d5954a Miquel Raynal 2021-01-21  204  		spinlock_t lock;
dd3c52846d5954a Miquel Raynal 2021-01-21  205  	} ibi;
f32ae0219a47f74 Frank Li      2023-10-16  206  	struct mutex lock;
dd3c52846d5954a Miquel Raynal 2021-01-21 @207  };
dd3c52846d5954a Miquel Raynal 2021-01-21  208
Miquel Raynal Oct. 17, 2023, 2:16 p.m. UTC | #2 
Hi Frank,

Frank.Li@nxp.com wrote on Mon, 16 Oct 2023 11:32:27 -0400:

> The ibi work thread operates asynchronously with other transfers, such as
> svc_i3c_master_priv_xfers(). Introduces mutex protection to ensure the

Introduce

> completion of the entire i3c/i2c transaction.

Did you experience faulty conditions or was it reported thanks to
static analysis?

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

> 
> Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---

Thanks,
Miquèl
Frank Li Oct. 17, 2023, 2:37 p.m. UTC | #3 
On Tue, Oct 17, 2023 at 04:16:58PM +0200, Miquel Raynal wrote:
> Hi Frank,
> 
> Frank.Li@nxp.com wrote on Mon, 16 Oct 2023 11:32:27 -0400:
> 
> > The ibi work thread operates asynchronously with other transfers, such as
> > svc_i3c_master_priv_xfers(). Introduces mutex protection to ensure the
> 
> Introduce
> 
> > completion of the entire i3c/i2c transaction.
> 
> Did you experience faulty conditions or was it reported thanks to
> static analysis?

Yes, I met. But it needs my slave part patches, which will be ready sent
out review soon.

Frank

> 
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> > 
> > Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> 
> Thanks,
> Miquèl
Miquel Raynal Oct. 17, 2023, 2:49 p.m. UTC | #4 
Hi Frank,

Frank.li@nxp.com wrote on Tue, 17 Oct 2023 10:37:57 -0400:

> On Tue, Oct 17, 2023 at 04:16:58PM +0200, Miquel Raynal wrote:
> > Hi Frank,
> > 
> > Frank.Li@nxp.com wrote on Mon, 16 Oct 2023 11:32:27 -0400:
> >   
> > > The ibi work thread operates asynchronously with other transfers, such as
> > > svc_i3c_master_priv_xfers(). Introduces mutex protection to ensure the  
> > 
> > Introduce
> >   
> > > completion of the entire i3c/i2c transaction.  
> > 
> > Did you experience faulty conditions or was it reported thanks to
> > static analysis?  
> 
> Yes, I met. But it needs my slave part patches, which will be ready sent
> out review soon.

I believe several of the "fixes" in this series are related to newer
uses (typically your i3c slave support) which were not in the scope of
the original submission. As these new features are not supposed to be
backported in stable kernels and because these are new corner cases,you
may drop the CC:/Fixes tags to avoid useless backports.

Some of them however are real fixes for situations we may happen with
the current level of support, please keep the tags in these, and move
them all to the beginning of your series.

Thanks,
Miquèl
Frank Li Oct. 17, 2023, 3:10 p.m. UTC | #5 
On Tue, Oct 17, 2023 at 04:49:46PM +0200, Miquel Raynal wrote:
> Hi Frank,
> 
> Frank.li@nxp.com wrote on Tue, 17 Oct 2023 10:37:57 -0400:
> 
> > On Tue, Oct 17, 2023 at 04:16:58PM +0200, Miquel Raynal wrote:
> > > Hi Frank,
> > > 
> > > Frank.Li@nxp.com wrote on Mon, 16 Oct 2023 11:32:27 -0400:
> > >   
> > > > The ibi work thread operates asynchronously with other transfers, such as
> > > > svc_i3c_master_priv_xfers(). Introduces mutex protection to ensure the  
> > > 
> > > Introduce
> > >   
> > > > completion of the entire i3c/i2c transaction.  
> > > 
> > > Did you experience faulty conditions or was it reported thanks to
> > > static analysis?  
> > 
> > Yes, I met. But it needs my slave part patches, which will be ready sent
> > out review soon.
> 
> I believe several of the "fixes" in this series are related to newer
> uses (typically your i3c slave support) which were not in the scope of
> the original submission. As these new features are not supposed to be
> backported in stable kernels and because these are new corner cases,you
> may drop the CC:/Fixes tags to avoid useless backports.

I don't think so. The issue exists. Just difficult to find it. If there are
i3c device that use IBI frequently. The problem should be trigger. My case
just make it easy to trigger the problem.

In previous existed code, IBI is supported.

I think it is typical case, which need CC/Fixes.

Frank

> 
> Some of them however are real fixes for situations we may happen with
> the current level of support, please keep the tags in these, and move
> them all to the beginning of your series.
> 
> Thanks,
> Miquèl
Miquel Raynal Oct. 17, 2023, 3:23 p.m. UTC | #6 
Hi Frank,

Frank.li@nxp.com wrote on Tue, 17 Oct 2023 11:10:54 -0400:

> On Tue, Oct 17, 2023 at 04:49:46PM +0200, Miquel Raynal wrote:
> > Hi Frank,
> > 
> > Frank.li@nxp.com wrote on Tue, 17 Oct 2023 10:37:57 -0400:
> >   
> > > On Tue, Oct 17, 2023 at 04:16:58PM +0200, Miquel Raynal wrote:  
> > > > Hi Frank,
> > > > 
> > > > Frank.Li@nxp.com wrote on Mon, 16 Oct 2023 11:32:27 -0400:
> > > >     
> > > > > The ibi work thread operates asynchronously with other transfers, such as
> > > > > svc_i3c_master_priv_xfers(). Introduces mutex protection to ensure the    
> > > > 
> > > > Introduce
> > > >     
> > > > > completion of the entire i3c/i2c transaction.    
> > > > 
> > > > Did you experience faulty conditions or was it reported thanks to
> > > > static analysis?    
> > > 
> > > Yes, I met. But it needs my slave part patches, which will be ready sent
> > > out review soon.  
> > 
> > I believe several of the "fixes" in this series are related to newer
> > uses (typically your i3c slave support) which were not in the scope of
> > the original submission. As these new features are not supposed to be
> > backported in stable kernels and because these are new corner cases,you
> > may drop the CC:/Fixes tags to avoid useless backports.  
> 
> I don't think so. The issue exists. Just difficult to find it. If there are
> i3c device that use IBI frequently. The problem should be trigger. My case
> just make it easy to trigger the problem.
> 
> In previous existed code, IBI is supported.
> 
> I think it is typical case, which need CC/Fixes.

I am not talking about this patch in particular.

> 
> Frank
> 
> > 
> > Some of them however are real fixes for situations we may happen with
> > the current level of support, please keep the tags in these, and move
> > them all to the beginning of your series.
> > 
> > Thanks,
> > Miquèl  


Thanks,
Miquèl
diff mbox series

Patch

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index c308e22f0ac5..ebdb3ea1af9d 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -202,6 +202,7 @@  struct svc_i3c_master {
 		/* Prevent races within IBI handlers */
 		spinlock_t lock;
 	} ibi;
+	struct mutex lock;
 };
 
 /**
@@ -383,6 +384,7 @@  static void svc_i3c_master_ibi_work(struct work_struct *work)
 	u32 status, val;
 	int ret;
 
+	mutex_lock(&master->lock);
 	/* Acknowledge the incoming interrupt with the AUTOIBI mechanism */
 	writel(SVC_I3C_MCTRL_REQUEST_AUTO_IBI |
 	       SVC_I3C_MCTRL_IBIRESP_AUTO,
@@ -459,6 +461,7 @@  static void svc_i3c_master_ibi_work(struct work_struct *work)
 
 reenable_ibis:
 	svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART);
+	mutex_unlock(&master->lock);
 }
 
 static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
@@ -1203,9 +1206,11 @@  static int svc_i3c_master_send_bdcast_ccc_cmd(struct svc_i3c_master *master,
 	cmd->read_len = 0;
 	cmd->continued = false;
 
+	mutex_lock(&master->lock);
 	svc_i3c_master_enqueue_xfer(master, xfer);
 	if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
 		svc_i3c_master_dequeue_xfer(master, xfer);
+	mutex_unlock(&master->lock);
 
 	ret = xfer->ret;
 	kfree(buf);
@@ -1249,9 +1254,11 @@  static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
 	cmd->read_len = read_len;
 	cmd->continued = false;
 
+	mutex_lock(&master->lock);
 	svc_i3c_master_enqueue_xfer(master, xfer);
 	if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
 		svc_i3c_master_dequeue_xfer(master, xfer);
+	mutex_unlock(&master->lock);
 
 	if (cmd->read_len != xfer_len)
 		ccc->dests[0].payload.len = cmd->read_len;
@@ -1308,9 +1315,11 @@  static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
 		cmd->continued = (i + 1) < nxfers;
 	}
 
+	mutex_lock(&master->lock);
 	svc_i3c_master_enqueue_xfer(master, xfer);
 	if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
 		svc_i3c_master_dequeue_xfer(master, xfer);
+	mutex_unlock(&master->lock);
 
 	ret = xfer->ret;
 	svc_i3c_master_free_xfer(xfer);
@@ -1346,9 +1355,11 @@  static int svc_i3c_master_i2c_xfers(struct i2c_dev_desc *dev,
 		cmd->continued = (i + 1 < nxfers);
 	}
 
+	mutex_lock(&master->lock);
 	svc_i3c_master_enqueue_xfer(master, xfer);
 	if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
 		svc_i3c_master_dequeue_xfer(master, xfer);
+	mutex_unlock(&master->lock);
 
 	ret = xfer->ret;
 	svc_i3c_master_free_xfer(xfer);
@@ -1539,6 +1550,8 @@  static int svc_i3c_master_probe(struct platform_device *pdev)
 
 	INIT_WORK(&master->hj_work, svc_i3c_master_hj_work);
 	INIT_WORK(&master->ibi_work, svc_i3c_master_ibi_work);
+	mutex_init(&master->lock);
+
 	ret = devm_request_irq(dev, master->irq, svc_i3c_master_irq_handler,
 			       IRQF_NO_SUSPEND, "svc-i3c-irq", master);
 	if (ret)