[PATCH 1/2] i3c: master: fixes i3c bus driver probe failure if no i3c device attached

[PATCH 1/2] i3c: master: fixes i3c bus driver probe failure if no i3c device attached

[Frank Li]

In i3c_master_bus_init()
{	...
	ret = i3c_master_rstdaa_locked(master, I3C_BROADCAST_ADDR);
	if (ret && ret != I3C_ERROR_M2)
			  ^^^ // it is enum i3c_error_code
	...
}

In dw-i3c-master.c implementation:
dw_i3c_ccc_set()
{	...
	ret = xfer->ret;
	if (xfer->cmds[0].error == RESPONSE_ERROR_IBA_NACK)
		ccc->err = I3C_ERROR_M2;

	dw_i3c_master_free_xfer(xfer);

	return ret;
}

Return enum i3c_error_code when error happen in i3c_master_rstdaa_locked().

Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 08aeb69a78003..00a82f3ab9ac0 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -783,6 +783,9 @@ static int i3c_master_rstdaa_locked(struct i3c_master_controller *master,
 	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
 	i3c_ccc_cmd_dest_cleanup(&dest);
 
+	if (ret)
+		ret = cmd.err;
+
 	return ret;
 }
 
-- 
2.34.1

[PATCH 2/2] i3c: master: svc: fix probe failure when no i3c device exist

[Frank Li]

If there are not i3c device, all ccc command will get NACK. Set
i3c_ccc_cmd::err as I3C_ERROR_M2.

Return success when no i3c device found at svc_i3c_master_do_daa_locked().

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

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 770b40e28015e..a5620103acb73 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -789,6 +789,9 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
 				 */
 				break;
 			} else if (SVC_I3C_MSTATUS_NACKED(reg)) {
+				/* No I3C devices attached */
+				if (dev_nb == 0)
+					break;
 				/*
 				 * A slave device nacked the address, this is
 				 * allowed only once, DAA will be stopped and
@@ -1263,11 +1266,17 @@ static int svc_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
 {
 	struct svc_i3c_master *master = to_svc_i3c_master(m);
 	bool broadcast = cmd->id < 0x80;
+	int ret;
 
 	if (broadcast)
-		return svc_i3c_master_send_bdcast_ccc_cmd(master, cmd);
+		ret = svc_i3c_master_send_bdcast_ccc_cmd(master, cmd);
 	else
-		return svc_i3c_master_send_direct_ccc_cmd(master, cmd);
+		ret = svc_i3c_master_send_direct_ccc_cmd(master, cmd);
+
+	if (ret)
+		cmd->err = I3C_ERROR_M2;
+
+	return ret;
 }
 
 static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
-- 
2.34.1

Re: [PATCH 2/2] i3c: master: svc: fix probe failure when no i3c device exist

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Mon, 28 Aug 2023 15:25:02 -0400:

> If there are not i3c device, all ccc command will get NACK. Set

		no					NACKed?

> i3c_ccc_cmd::err as I3C_ERROR_M2.

This sentence should come last and be slightly more explicit.

> Return success when no i3c device found at svc_i3c_master_do_daa_locked().

Please explain why this is important/useful.

> Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")

Shall we consider a backport into stable kernels?

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/i3c/master/svc-i3c-master.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 770b40e28015e..a5620103acb73 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -789,6 +789,9 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
>  				 */
>  				break;
>  			} else if (SVC_I3C_MSTATUS_NACKED(reg)) {
> +				/* No I3C devices attached */
> +				if (dev_nb == 0)
> +					break;

\n ?

>  				/*
>  				 * A slave device nacked the address, this is
>  				 * allowed only once, DAA will be stopped and
> @@ -1263,11 +1266,17 @@ static int svc_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
>  {
>  	struct svc_i3c_master *master = to_svc_i3c_master(m);
>  	bool broadcast = cmd->id < 0x80;
> +	int ret;
>  
>  	if (broadcast)
> -		return svc_i3c_master_send_bdcast_ccc_cmd(master, cmd);
> +		ret = svc_i3c_master_send_bdcast_ccc_cmd(master, cmd);
>  	else
> -		return svc_i3c_master_send_direct_ccc_cmd(master, cmd);
> +		ret = svc_i3c_master_send_direct_ccc_cmd(master, cmd);
> +
> +	if (ret)
> +		cmd->err = I3C_ERROR_M2;
> +
> +	return ret;
>  }
>  
>  static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,


Thanks,
Miquèl

Re: [PATCH 2/2] i3c: master: svc: fix probe failure when no i3c device exist

[Frank Li]

On Tue, Aug 29, 2023 at 11:16:11AM +0200, Miquel Raynal wrote:
> Hi Frank,
> 
> Frank.Li@nxp.com wrote on Mon, 28 Aug 2023 15:25:02 -0400:
> 
> > If there are not i3c device, all ccc command will get NACK. Set
> 
> 		no					NACKed?
> 
> > i3c_ccc_cmd::err as I3C_ERROR_M2.
> 
> This sentence should come last and be slightly more explicit.
> 
> > Return success when no i3c device found at svc_i3c_master_do_daa_locked().
> 
> Please explain why this is important/useful.

If return failure, driver will probe failure when there are no any i3c
devices. I3C master supposed support hot join. The probe failure don't
make sense if no i3c devices found when master driver probe.

How about rewrite commit log as

"I3C master supports device hot join, it doesn't make sense master driver
probe failure when there are no I3C devices.

When there are no I3C devices attached, all CCC commands are NACKed. So
SVC I3C master fails to probe.

Set i3c_cc_cmd:err as I3C_ERROR_M2. So I3C master framework
i3c_master_rstdaa_locked() can return expected ERROR code and continue
driver probe process.

Return success at svc_i3c_master_do_daa_locked() if no i3c devices found.
So SVC master driver can probe successfully even if no I3C devices are
attached"

> 
> > Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> 
> Shall we consider a backport into stable kernels?

Yes, I can add stable tag at next version.
> 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/i3c/master/svc-i3c-master.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > index 770b40e28015e..a5620103acb73 100644
> > --- a/drivers/i3c/master/svc-i3c-master.c
> > +++ b/drivers/i3c/master/svc-i3c-master.c
> > @@ -789,6 +789,9 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> >  				 */
> >  				break;
> >  			} else if (SVC_I3C_MSTATUS_NACKED(reg)) {
> > +				/* No I3C devices attached */
> > +				if (dev_nb == 0)
> > +					break;
> 
> \n ?
> 
> >  				/*
> >  				 * A slave device nacked the address, this is
> >  				 * allowed only once, DAA will be stopped and
> > @@ -1263,11 +1266,17 @@ static int svc_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
> >  {
> >  	struct svc_i3c_master *master = to_svc_i3c_master(m);
> >  	bool broadcast = cmd->id < 0x80;
> > +	int ret;
> >  
> >  	if (broadcast)
> > -		return svc_i3c_master_send_bdcast_ccc_cmd(master, cmd);
> > +		ret = svc_i3c_master_send_bdcast_ccc_cmd(master, cmd);
> >  	else
> > -		return svc_i3c_master_send_direct_ccc_cmd(master, cmd);
> > +		ret = svc_i3c_master_send_direct_ccc_cmd(master, cmd);
> > +
> > +	if (ret)
> > +		cmd->err = I3C_ERROR_M2;
> > +
> > +	return ret;
> >  }
> >  
> >  static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> 
> 
> Thanks,
> Miquèl

Re: [PATCH 2/2] i3c: master: svc: fix probe failure when no i3c device exist

[Miquel Raynal]

Hi Frank,

Frank.li@nxp.com wrote on Tue, 29 Aug 2023 11:26:42 -0400:

> On Tue, Aug 29, 2023 at 11:16:11AM +0200, Miquel Raynal wrote:
> > Hi Frank,
> > 
> > Frank.Li@nxp.com wrote on Mon, 28 Aug 2023 15:25:02 -0400:
> >   
> > > If there are not i3c device, all ccc command will get NACK. Set  
> > 
> > 		no					NACKed?
> >   
> > > i3c_ccc_cmd::err as I3C_ERROR_M2.  
> > 
> > This sentence should come last and be slightly more explicit.
> >   
> > > Return success when no i3c device found at svc_i3c_master_do_daa_locked().  
> > 
> > Please explain why this is important/useful.  
> 
> If return failure, driver will probe failure when there are no any i3c
> devices. I3C master supposed support hot join. The probe failure don't
> make sense if no i3c devices found when master driver probe.
> 
> How about rewrite commit log as

Much better, let me propose something slightly clearer.

> "I3C master supports device hot join, it doesn't make sense master driver
> probe failure when there are no I3C devices.
>
> When there are no I3C devices attached, all CCC commands are NACKed. So
> SVC I3C master fails to probe.

What about:

I3C masters are expected to support hot-join. This means at
initialization time we might not yet discover any device and this
should not be treated as a fatal error.

During the DAA procedure which happens at probe time, if no device has
joined, all CCC will be NACKed (from a bus perspective). This leads to
an early return with an error code which fails the probe of the master.

Let's avoid this by just telling the core through an I3C_ERROR_M2
return command code that no device was discovered, which is a valid
situation. This way the master will no longer bail out and fail to
probe for a wrong reason.

> Set i3c_cc_cmd:err as I3C_ERROR_M2. So I3C master framework
> i3c_master_rstdaa_locked() can return expected ERROR code and continue
> driver probe process.
> 
> Return success at svc_i3c_master_do_daa_locked() if no i3c devices found.
> So SVC master driver can probe successfully even if no I3C devices are
> attached"
> 
> >   
> > > Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")  
> > 
> > Shall we consider a backport into stable kernels?  
> 
> Yes, I can add stable tag at next version.

Yes, please.

> >   
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  drivers/i3c/master/svc-i3c-master.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > > index 770b40e28015e..a5620103acb73 100644
> > > --- a/drivers/i3c/master/svc-i3c-master.c
> > > +++ b/drivers/i3c/master/svc-i3c-master.c
> > > @@ -789,6 +789,9 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> > >  				 */
> > >  				break;
> > >  			} else if (SVC_I3C_MSTATUS_NACKED(reg)) {
> > > +				/* No I3C devices attached */
> > > +				if (dev_nb == 0)
> > > +					break;  
> > 
> > \n ?
> >   
> > >  				/*
> > >  				 * A slave device nacked the address, this is
> > >  				 * allowed only once, DAA will be stopped and
> > > @@ -1263,11 +1266,17 @@ static int svc_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
> > >  {
> > >  	struct svc_i3c_master *master = to_svc_i3c_master(m);
> > >  	bool broadcast = cmd->id < 0x80;
> > > +	int ret;
> > >  
> > >  	if (broadcast)
> > > -		return svc_i3c_master_send_bdcast_ccc_cmd(master, cmd);
> > > +		ret = svc_i3c_master_send_bdcast_ccc_cmd(master, cmd);
> > >  	else
> > > -		return svc_i3c_master_send_direct_ccc_cmd(master, cmd);
> > > +		ret = svc_i3c_master_send_direct_ccc_cmd(master, cmd);
> > > +
> > > +	if (ret)
> > > +		cmd->err = I3C_ERROR_M2;
> > > +
> > > +	return ret;
> > >  }
> > >  
> > >  static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,  
> > 
> > 
> > Thanks,
> > Miquèl  


Thanks,
Miquèl