[PATCH 0/6] i3c: master: svc: collection of bugs fixes

[PATCH 0/6] i3c: master: svc: collection of bugs fixes

[Frank Li]

Each patch is indepedents. See commit message for detail.

Frank Li (6):
  i3c: master: svc: fix race condition in ibi work thread
  i3c: master: svc: fix wrong data return when IBI happen during start
    frame
  i3c: master: svc: fix ibi may not return mandatory data byte
  i3c: master: svc: fix check wrong status register in irq handler
  i3c: master: svc: fix SDA keep low when polling IBIWON timeout happen
  i3c: master: svc: fix random hot join failure since timeout error

 drivers/i3c/master/svc-i3c-master.c | 50 ++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Frank Li]

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(+)

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)
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[kernel test robot]

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  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Miquel Raynal]

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Frank Li]

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Miquel Raynal]

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Frank Li]

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Miquel Raynal]

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH 2/6] i3c: master: svc: fix wrong data return when IBI happen during start frame

[Frank Li]

     ┌─────┐     ┏──┐  ┏──┐  ┏──┐  ┏──┐  ┏──┐  ┏──┐  ┏──┐  ┏──┐  ┌─────
SCL: ┘     └─────┛  └──┛  └──┛  └──┛  └──┛  └──┛  └──┛  └──┛  └──┘
     ───┐                       ┌─────┐     ┌─────┐     ┌───────────┐
SDA:    └───────────────────────┘     └─────┘     └─────┘           └─────
     xxx╱    ╲╱                                        ╲╱    ╲╱    ╲╱    ╲
   : xxx╲IBI ╱╲               Addr(0x0a)               ╱╲ RW ╱╲NACK╱╲ S  ╱

In-Band Interrupt (IBI) occurred and IBI work thread may not to be
scheduled. When svc_i3c_master_priv_xfers() initiates the I3C transfer
frame and attempts to send address 0x7e, the target interprets it as an
IBI handler and returns the target address 0x0a.

However, svc_i3c_master_priv_xfers() does not handle this case and proceeds
with other transfers, resulting in incorrect data being returned.

IBIWON check has been added in svc_i3c_master_xfer(). In case this
situation occurs, a failure is now returned to the driver.

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 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index ebdb3ea1af9d..0f57a5f75e39 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -1009,6 +1009,9 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 	u32 reg;
 	int ret;
 
+	/* clean SVC_I3C_MINT_IBIWON w1c bits */
+	writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
+
 	writel(SVC_I3C_MCTRL_REQUEST_START_ADDR |
 	       xfer_type |
 	       SVC_I3C_MCTRL_IBIRESP_NACK |
@@ -1027,6 +1030,23 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		goto emit_stop;
 	}
 
+	/*
+	 * According to I3C spec ver 1.1.1, 5.1.2.2.3 Consequence of Controller Starting a Frame
+	 * with I3C Target Address.
+	 *
+	 * The I3C Controller normally should start a Frame, the Address may be arbitrated, and so
+	 * the Controller shall monitor to see whether an In-Band Interrupt request, a Controller
+	 * Role Request (i.e., Secondary Controller requests to become the Active Controller), or
+	 * a Hot-Join Request has been made.
+	 *
+	 * If missed IBIWON check, the wrong data will be return. When IBIWON happen, return falure
+	 * and yeild the above events handler.
+	 */
+	if (SVC_I3C_MSTATUS_IBIWON(reg)) {
+		ret = -ENXIO;
+		goto emit_stop;
+	}
+
 	if (rnw)
 		ret = svc_i3c_master_read(master, in, xfer_len);
 	else
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 2/6] i3c: master: svc: fix wrong data return when IBI happen during start frame

[Miquel Raynal]

Hi Frank,

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

>      ┌─────┐     ┏──┐  ┏──┐  ┏──┐  ┏──┐  ┏──┐  ┏──┐  ┏──┐  ┏──┐  ┌─────
> SCL: ┘     └─────┛  └──┛  └──┛  └──┛  └──┛  └──┛  └──┛  └──┛  └──┘
>      ───┐                       ┌─────┐     ┌─────┐     ┌───────────┐
> SDA:    └───────────────────────┘     └─────┘     └─────┘           └─────
>      xxx╱    ╲╱                                        ╲╱    ╲╱    ╲╱    ╲
>    : xxx╲IBI ╱╲               Addr(0x0a)               ╱╲ RW ╱╲NACK╱╲ S  ╱
> 
> In-Band Interrupt (IBI) occurred and IBI work thread may not to be

If an In-Band...	occurs and the IBI work thread is not
immediately scheduled, when svc... initiates an I3C transfer and
attempts...

> scheduled. When svc_i3c_master_priv_xfers() initiates the I3C transfer
> frame and attempts to send address 0x7e, the target interprets it as an
> IBI handler and returns the target address 0x0a.
> 
> However, svc_i3c_master_priv_xfers() does not handle this case and proceeds
> with other transfers, resulting in incorrect data being returned.
> 
> IBIWON check has been added in svc_i3c_master_xfer(). In case this

Add IBIWON check in svc_...

> situation occurs, a failure is now returned to the driver.

			return a failure...

> 
> 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 | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index ebdb3ea1af9d..0f57a5f75e39 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -1009,6 +1009,9 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  	u32 reg;
>  	int ret;
>  
> +	/* clean SVC_I3C_MINT_IBIWON w1c bits */
> +	writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
> +
>  	writel(SVC_I3C_MCTRL_REQUEST_START_ADDR |
>  	       xfer_type |
>  	       SVC_I3C_MCTRL_IBIRESP_NACK |
> @@ -1027,6 +1030,23 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  		goto emit_stop;
>  	}
>  
> +	/*
> +	 * According to I3C spec ver 1.1.1, 5.1.2.2.3 Consequence of Controller Starting a Frame
> +	 * with I3C Target Address.
> +	 *
> +	 * The I3C Controller normally should start a Frame, the Address may be arbitrated, and so
> +	 * the Controller shall monitor to see whether an In-Band Interrupt request, a Controller
> +	 * Role Request (i.e., Secondary Controller requests to become the Active Controller), or
> +	 * a Hot-Join Request has been made.
> +	 *
> +	 * If missed IBIWON check, the wrong data will be return. When IBIWON happen, return falure
> +	 * and yeild the above events handler.

Typos: yeild and falure

> +	 */
> +	if (SVC_I3C_MSTATUS_IBIWON(reg)) {
> +		ret = -ENXIO;
> +		goto emit_stop;
> +	}
> +
>  	if (rnw)
>  		ret = svc_i3c_master_read(master, in, xfer_len);
>  	else

With all the typos fixed:

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

Thanks,
Miquèl

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH 3/6] i3c: master: svc: fix ibi may not return mandatory data byte

[Frank Li]

MSTATUS[RXPEND] is only updated after the data transfer cycle started. This
creates an issue when the I3C clock is slow, and the CPU is running fast
enough that MSTATUS[RXPEND] may not be updated when the code reach checking
point. As a result, mandatory data are being missed.

Add a wait for MSTATUS[COMPLETE] to ensure that all mandatory data already
in FIFO.

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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 0f57a5f75e39..c252446b2bc5 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -331,6 +331,7 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
 	struct i3c_ibi_slot *slot;
 	unsigned int count;
 	u32 mdatactrl;
+	int ret, val;
 	u8 *buf;
 
 	slot = i3c_generic_ibi_get_free_slot(data->ibi_pool);
@@ -340,6 +341,13 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
 	slot->len = 0;
 	buf = slot->data;
 
+	ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val,
+						SVC_I3C_MSTATUS_COMPLETE(val), 0, 1000);
+	if (ret) {
+		dev_err(master->dev, "Timeout when polling for COMPLETE\n");
+		return ret;
+	}
+
 	while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS))  &&
 	       slot->len < SVC_I3C_FIFO_SIZE) {
 		mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL);
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 3/6] i3c: master: svc: fix ibi may not return mandatory data byte

[Miquel Raynal]

Hi Frank,

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

> MSTATUS[RXPEND] is only updated after the data transfer cycle started. This
> creates an issue when the I3C clock is slow, and the CPU is running fast
> enough that MSTATUS[RXPEND] may not be updated when the code reach checking
> point. As a result, mandatory data are being missed.

					can be missed.

> Add a wait for MSTATUS[COMPLETE] to ensure that all mandatory data already
> in FIFO.

is already in the FIFO

> 
> 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 | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 0f57a5f75e39..c252446b2bc5 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -331,6 +331,7 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
>  	struct i3c_ibi_slot *slot;
>  	unsigned int count;
>  	u32 mdatactrl;
> +	int ret, val;
>  	u8 *buf;
>  
>  	slot = i3c_generic_ibi_get_free_slot(data->ibi_pool);
> @@ -340,6 +341,13 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
>  	slot->len = 0;
>  	buf = slot->data;
>  
> +	ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val,
> +						SVC_I3C_MSTATUS_COMPLETE(val), 0, 1000);

Are you sure !MSTATUS_COMPLETE(val) is never a valid condition?
Especially with non-mandatory bytes?

Also, are you sure of the indentation here?

> +	if (ret) {
> +		dev_err(master->dev, "Timeout when polling for COMPLETE\n");
> +		return ret;
> +	}
> +
>  	while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS))  &&
>  	       slot->len < SVC_I3C_FIFO_SIZE) {
>  		mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL);


Thanks,
Miquèl

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 3/6] i3c: master: svc: fix ibi may not return mandatory data byte

[Frank Li]

On Tue, Oct 17, 2023 at 04:27:07PM +0200, Miquel Raynal wrote:
> Hi Frank,
> 
> Frank.Li@nxp.com wrote on Mon, 16 Oct 2023 11:32:29 -0400:
> 
> > MSTATUS[RXPEND] is only updated after the data transfer cycle started. This
> > creates an issue when the I3C clock is slow, and the CPU is running fast
> > enough that MSTATUS[RXPEND] may not be updated when the code reach checking
> > point. As a result, mandatory data are being missed.
> 
> 					can be missed.
> 
> > Add a wait for MSTATUS[COMPLETE] to ensure that all mandatory data already
> > in FIFO.
> 
> is already in the FIFO
> 
> > 
> > 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 | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > index 0f57a5f75e39..c252446b2bc5 100644
> > --- a/drivers/i3c/master/svc-i3c-master.c
> > +++ b/drivers/i3c/master/svc-i3c-master.c
> > @@ -331,6 +331,7 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
> >  	struct i3c_ibi_slot *slot;
> >  	unsigned int count;
> >  	u32 mdatactrl;
> > +	int ret, val;
> >  	u8 *buf;
> >  
> >  	slot = i3c_generic_ibi_get_free_slot(data->ibi_pool);
> > @@ -340,6 +341,13 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
> >  	slot->len = 0;
> >  	buf = slot->data;
> >  
> > +	ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val,
> > +						SVC_I3C_MSTATUS_COMPLETE(val), 0, 1000);
> 
> Are you sure !MSTATUS_COMPLETE(val) is never a valid condition?
> Especially with non-mandatory bytes?
> 
> Also, are you sure of the indentation here?

If no-mandatory data, it is equal RDTERAM is 0. It takes some times to
change my slave code to create such test case.

Frank Li

> 
> > +	if (ret) {
> > +		dev_err(master->dev, "Timeout when polling for COMPLETE\n");
> > +		return ret;
> > +	}
> > +
> >  	while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS))  &&
> >  	       slot->len < SVC_I3C_FIFO_SIZE) {
> >  		mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL);
> 
> 
> Thanks,
> Miquèl

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH 4/6] i3c: master: svc: fix check wrong status register in irq handler

[Frank Li]

svc_i3c_master_irq_handler() wrong check register SVC_I3C_MINTMASKED. It
should be SVC_I3C_MSTATUS.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index c252446b2bc5..5ab68d6e439d 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -475,7 +475,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
 {
 	struct svc_i3c_master *master = (struct svc_i3c_master *)dev_id;
-	u32 active = readl(master->regs + SVC_I3C_MINTMASKED);
+	u32 active = readl(master->regs + SVC_I3C_MSTATUS);
 
 	if (!SVC_I3C_MSTATUS_SLVSTART(active))
 		return IRQ_NONE;
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 4/6] i3c: master: svc: fix check wrong status register in irq handler

[Miquel Raynal]

Hi Frank,

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

> svc_i3c_master_irq_handler() wrong check register SVC_I3C_MINTMASKED. It

				wrongly checks
> should be SVC_I3C_MSTATUS.

Ah right, good catch.

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>
> ---
>  drivers/i3c/master/svc-i3c-master.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index c252446b2bc5..5ab68d6e439d 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -475,7 +475,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>  static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
>  {
>  	struct svc_i3c_master *master = (struct svc_i3c_master *)dev_id;
> -	u32 active = readl(master->regs + SVC_I3C_MINTMASKED);
> +	u32 active = readl(master->regs + SVC_I3C_MSTATUS);
>  
>  	if (!SVC_I3C_MSTATUS_SLVSTART(active))
>  		return IRQ_NONE;


Thanks,
Miquèl

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH 5/6] i3c: master: svc: fix SDA keep low when polling IBIWON timeout happen

[Frank Li]

Need call svc_i3c_master_emit_stop() release bus.

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 5ab68d6e439d..5bca369d6912 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -403,6 +403,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 					 SVC_I3C_MSTATUS_IBIWON(val), 0, 1000);
 	if (ret) {
 		dev_err(master->dev, "Timeout when polling for IBIWON\n");
+		svc_i3c_master_emit_stop(master);
 		goto reenable_ibis;
 	}
 
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 5/6] i3c: master: svc: fix SDA keep low when polling IBIWON timeout happen

[Miquel Raynal]

Hi Frank,

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

> Need call svc_i3c_master_emit_stop() release bus.

Can you please elaborate this commit message please?

> 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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 5ab68d6e439d..5bca369d6912 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -403,6 +403,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>  					 SVC_I3C_MSTATUS_IBIWON(val), 0, 1000);
>  	if (ret) {
>  		dev_err(master->dev, "Timeout when polling for IBIWON\n");
> +		svc_i3c_master_emit_stop(master);
>  		goto reenable_ibis;
>  	}
>  


Thanks,
Miquèl

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH 6/6] i3c: master: svc: fix random hot join failure since timeout error

[Frank Li]

master side report:
  silvaco-i3c-master 44330000.i3c-master: Error condition: MSTATUS 0x020090c7, MERRWARN 0x00100000

BIT 20: TIMEOUT error
  The module has stalled too long in a frame. This happens when:
  - The TX FIFO or RX FIFO is not handled and the bus is stuck in the
middle of a message,
  - No STOP was issued and between messages,
  - IBI manual is used and no decision was made.
  The maximum stall period is 10 KHz or 100 μs.

This is a just warning. System irq thread schedule latency is possible
bigger than 100us. Just omit this waring.

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 5bca369d6912..18bc277edc8a 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -93,6 +93,7 @@
 #define SVC_I3C_MINTMASKED   0x098
 #define SVC_I3C_MERRWARN     0x09C
 #define   SVC_I3C_MERRWARN_NACK BIT(2)
+#define   SVC_I3C_MERRWARN_TIMEOUT BIT(20)
 #define SVC_I3C_MDMACTRL     0x0A0
 #define SVC_I3C_MDATACTRL    0x0AC
 #define   SVC_I3C_MDATACTRL_FLUSHTB BIT(0)
@@ -225,6 +226,11 @@ static bool svc_i3c_master_error(struct svc_i3c_master *master)
 	if (SVC_I3C_MSTATUS_ERRWARN(mstatus)) {
 		merrwarn = readl(master->regs + SVC_I3C_MERRWARN);
 		writel(merrwarn, master->regs + SVC_I3C_MERRWARN);
+
+		/* ignore timeout error */
+		if (merrwarn & SVC_I3C_MERRWARN_TIMEOUT)
+			return false;
+
 		dev_err(master->dev,
 			"Error condition: MSTATUS 0x%08x, MERRWARN 0x%08x\n",
 			mstatus, merrwarn);
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 6/6] i3c: master: svc: fix random hot join failure since timeout error

[Miquel Raynal]

Hi Frank,

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

> master side report:
>   silvaco-i3c-master 44330000.i3c-master: Error condition: MSTATUS 0x020090c7, MERRWARN 0x00100000
> 
> BIT 20: TIMEOUT error
>   The module has stalled too long in a frame. This happens when:
>   - The TX FIFO or RX FIFO is not handled and the bus is stuck in the
> middle of a message,
>   - No STOP was issued and between messages,
>   - IBI manual is used and no decision was made.
>   The maximum stall period is 10 KHz or 100 μs.
> 
> This is a just warning. System irq thread schedule latency is possible

							can be bigger
> bigger than 100us. Just omit this waring.

I'm not sure this is the correct approach. It's a real issue but there
is not much we can do about it. Perhaps dev_err is too high, but I
would not entirely drop this message. Maybe a comment and turning the
message into a dbg printk would be more appropriate?

> 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 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 5bca369d6912..18bc277edc8a 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -93,6 +93,7 @@
>  #define SVC_I3C_MINTMASKED   0x098
>  #define SVC_I3C_MERRWARN     0x09C
>  #define   SVC_I3C_MERRWARN_NACK BIT(2)
> +#define   SVC_I3C_MERRWARN_TIMEOUT BIT(20)
>  #define SVC_I3C_MDMACTRL     0x0A0
>  #define SVC_I3C_MDATACTRL    0x0AC
>  #define   SVC_I3C_MDATACTRL_FLUSHTB BIT(0)
> @@ -225,6 +226,11 @@ static bool svc_i3c_master_error(struct svc_i3c_master *master)
>  	if (SVC_I3C_MSTATUS_ERRWARN(mstatus)) {
>  		merrwarn = readl(master->regs + SVC_I3C_MERRWARN);
>  		writel(merrwarn, master->regs + SVC_I3C_MERRWARN);
> +
> +		/* ignore timeout error */
> +		if (merrwarn & SVC_I3C_MERRWARN_TIMEOUT)
> +			return false;
> +
>  		dev_err(master->dev,
>  			"Error condition: MSTATUS 0x%08x, MERRWARN 0x%08x\n",
>  			mstatus, merrwarn);


Thanks,
Miquèl

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 6/6] i3c: master: svc: fix random hot join failure since timeout error

[Frank Li]

On Tue, Oct 17, 2023 at 04:33:35PM +0200, Miquel Raynal wrote:
> Hi Frank,
> 
> Frank.Li@nxp.com wrote on Mon, 16 Oct 2023 11:32:32 -0400:
> 
> > master side report:
> >   silvaco-i3c-master 44330000.i3c-master: Error condition: MSTATUS 0x020090c7, MERRWARN 0x00100000
> > 
> > BIT 20: TIMEOUT error
> >   The module has stalled too long in a frame. This happens when:
> >   - The TX FIFO or RX FIFO is not handled and the bus is stuck in the
> > middle of a message,
> >   - No STOP was issued and between messages,
> >   - IBI manual is used and no decision was made.
> >   The maximum stall period is 10 KHz or 100 μs.
> > 
> > This is a just warning. System irq thread schedule latency is possible
> 
> 							can be bigger
> > bigger than 100us. Just omit this waring.
> 
> I'm not sure this is the correct approach. It's a real issue but there
> is not much we can do about it. Perhaps dev_err is too high, but I
> would not entirely drop this message. Maybe a comment and turning the
> message into a dbg printk would be more appropriate?

The key is not message. It return true, means IBI/HJ thread will not run.

Frank

> 
> > 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 | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > index 5bca369d6912..18bc277edc8a 100644
> > --- a/drivers/i3c/master/svc-i3c-master.c
> > +++ b/drivers/i3c/master/svc-i3c-master.c
> > @@ -93,6 +93,7 @@
> >  #define SVC_I3C_MINTMASKED   0x098
> >  #define SVC_I3C_MERRWARN     0x09C
> >  #define   SVC_I3C_MERRWARN_NACK BIT(2)
> > +#define   SVC_I3C_MERRWARN_TIMEOUT BIT(20)
> >  #define SVC_I3C_MDMACTRL     0x0A0
> >  #define SVC_I3C_MDATACTRL    0x0AC
> >  #define   SVC_I3C_MDATACTRL_FLUSHTB BIT(0)
> > @@ -225,6 +226,11 @@ static bool svc_i3c_master_error(struct svc_i3c_master *master)
> >  	if (SVC_I3C_MSTATUS_ERRWARN(mstatus)) {
> >  		merrwarn = readl(master->regs + SVC_I3C_MERRWARN);
> >  		writel(merrwarn, master->regs + SVC_I3C_MERRWARN);
> > +
> > +		/* ignore timeout error */
> > +		if (merrwarn & SVC_I3C_MERRWARN_TIMEOUT)
> > +			return false;
> > +
> >  		dev_err(master->dev,
> >  			"Error condition: MSTATUS 0x%08x, MERRWARN 0x%08x\n",
> >  			mstatus, merrwarn);
> 
> 
> Thanks,
> Miquèl

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 6/6] i3c: master: svc: fix random hot join failure since timeout error

[Miquel Raynal]

Hi Frank,

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

> On Tue, Oct 17, 2023 at 04:33:35PM +0200, Miquel Raynal wrote:
> > Hi Frank,
> > 
> > Frank.Li@nxp.com wrote on Mon, 16 Oct 2023 11:32:32 -0400:
> >   
> > > master side report:
> > >   silvaco-i3c-master 44330000.i3c-master: Error condition: MSTATUS 0x020090c7, MERRWARN 0x00100000
> > > 
> > > BIT 20: TIMEOUT error
> > >   The module has stalled too long in a frame. This happens when:
> > >   - The TX FIFO or RX FIFO is not handled and the bus is stuck in the
> > > middle of a message,
> > >   - No STOP was issued and between messages,
> > >   - IBI manual is used and no decision was made.
> > >   The maximum stall period is 10 KHz or 100 μs.
> > > 
> > > This is a just warning. System irq thread schedule latency is possible  
> > 
> > 							can be bigger  
> > > bigger than 100us. Just omit this waring.  
> > 
> > I'm not sure this is the correct approach. It's a real issue but there
> > is not much we can do about it. Perhaps dev_err is too high, but I
> > would not entirely drop this message. Maybe a comment and turning the
> > message into a dbg printk would be more appropriate?  
> 
> The key is not message. It return true, means IBI/HJ thread will not run.

But why should the workers run if it's too late?

Thanks,
Miquèl

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 6/6] i3c: master: svc: fix random hot join failure since timeout error

[Frank Li]

On Tue, Oct 17, 2023 at 05:06:03PM +0200, Miquel Raynal wrote:
> Hi Frank,
> 
> Frank.li@nxp.com wrote on Tue, 17 Oct 2023 10:45:14 -0400:
> 
> > On Tue, Oct 17, 2023 at 04:33:35PM +0200, Miquel Raynal wrote:
> > > Hi Frank,
> > > 
> > > Frank.Li@nxp.com wrote on Mon, 16 Oct 2023 11:32:32 -0400:
> > >   
> > > > master side report:
> > > >   silvaco-i3c-master 44330000.i3c-master: Error condition: MSTATUS 0x020090c7, MERRWARN 0x00100000
> > > > 
> > > > BIT 20: TIMEOUT error
> > > >   The module has stalled too long in a frame. This happens when:
> > > >   - The TX FIFO or RX FIFO is not handled and the bus is stuck in the
> > > > middle of a message,
> > > >   - No STOP was issued and between messages,
> > > >   - IBI manual is used and no decision was made.
> > > >   The maximum stall period is 10 KHz or 100 μs.
> > > > 
> > > > This is a just warning. System irq thread schedule latency is possible  
> > > 
> > > 							can be bigger  
> > > > bigger than 100us. Just omit this waring.  
> > > 
> > > I'm not sure this is the correct approach. It's a real issue but there
> > > is not much we can do about it. Perhaps dev_err is too high, but I
> > > would not entirely drop this message. Maybe a comment and turning the
> > > message into a dbg printk would be more appropriate?  
> > 
> > The key is not message. It return true, means IBI/HJ thread will not run.
> 
> But why should the workers run if it's too late?

IBI ACK already sent, target think master already accepted IBI. then master
driver check TIMEOUT, 

If without run IBI thread, target's driver will wait target sent IBI. And
target wait for driver handle IBI. 

So the whole system may lock or wait for long time out.

Hardware check TIMEOUT and software send ACK is totally async.

> 
> Thanks,
> Miquèl

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c