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@xxxxxxx 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@xxxxxxx>
> > ---
> >  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