| Message ID | 20231016153232.2851095-2-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i3c: master: svc: collection of bugs fixes | expand |
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
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
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
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
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
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 --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)
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(+)