Re: [PATCH v3] i3c: master: fix for SETDASA and DAA process

[Boris Brezillon]

On Thu, 20 Aug 2020 21:03:11 +0200
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:

> On Thu, 20 Aug 2020 18:16:14 +0000
> Parshuram Raju Thombare <pthombar@xxxxxxxxxxx> wrote:
> 
> > >Hm, not sure that qualifies as a fix. The current implementation was
> > >correct, it was just reserving a slot in the device table for devices
> > >that didn't have an init address or on which SETDASA failed.    
> > If I3C controllers like ours use hardware slots to store slave devices info, 
> > due to limited available slots this can cause issue. 
> >  If some slots are lost due to
> > 1. only init_dyn_addr and no static_addr in DT 
> > OR
> > 2. SETDASA failed  
> 
> Well, having a slot with a static address is valid, though I agree
> it's not really useful.
> 
> > at the end of DAA some devices may be left without dyn_addr allocated from master
> > and hence can't work properly.  
> 
> My point is, there's no address or device slot leak, it's just that
> reserving a slot for I3C devices that only have a static address is
> kind of useless. But let's be honest, given the number of I3C devices
> available out there, I don't think it will hurt us before quite some
> time :P. That's not to say we shouldn't address that, I just don't
> think it deserves a Fixes tag.
> 
> > I think during our discussion we recognized this change as a bug.  
> 
> IIRC, I was talking about the first patch in the series.
> 
> > That is the reason I added fixes tag, but if you think otherwise I can remove this tag.
> >    
> > >> -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> > >> +static int i3c_master_pre_assign_dyn_addr(struct i3c_master_controller    
> > >That function now does more than just assigning a dynamic address: it
> > >also creates the i3c_dev_desc. It should be renamed accordingly
> > >(i3c_master_early_i3c_dev_add() maybe).    
> > Ok
> >   
> > >You should reserve the address before calling
> > >i3c_master_pre_assign_dyn_addr():
> > >
> > >		/*
> > >		 * We don't attach devices which are not addressable
> > >		 * (no static_addr and dyn_addr) and devices with
> > >		 * static_addr but no init_dyn_addr will participate in DAA.
> > >		 */
> > >		if (!i3cboardinfo->init_dyn_addr ||
> > >		    !i3cboardinfo->static_addr)
> > >			continue;    
> > Don't we want to cover the case when only init_dyn_addr is present ?  
> 
> Uh, yes, my bad.
> 
> > I am not sure if we can't have init_dyn_addr without static_addr.  
> 
> You can, when you want to assign a specific dynamic address to a device
> that doesn't have a static address (see the 'try to assign init_addr
> dance' in i3c_dev_add()).
> 
> > May be what we need is 
> > 		if (!i3cboardinfo->init_dyn_addr)
> > 			continue;
> > 
> > 		ret = i3c_bus_get_addr_slot_status(&master->bus,
> > 						   i3cboardinfo->init_dyn_addr);
> > 		if (ret != I3C_ADDR_SLOT_FREE) {
> > 			ret = -EBUSY;
> > 			goto err_rstdaa;
> > 		}
> > 
> > 		i3c_bus_set_addr_slot_status(&master->bus,
> > 					     i3cboardinfo->init_dyn_addr,
> > 					     I3C_ADDR_SLOT_I3C_DEV);
> > 
> > 		if (i3cboardinfo->static_addr)
> > 			i3c_master_pre_assign_dyn_addr(master, i3cboardinfo);  
> 
> Yep, that's correct.
> 
> > IMHO this is functionally same to what I sent. Just that init_dyn_addr is reserved before,
> > and we leverage the change in reattach to bypass failure due to second attempt
> > to get init_dyn_addr in reattach called from i3c_master_pre_assign_dyn_addr().  
> 
> Unless I'm missing something, your solution didn't reserve the
> init address when there's no static address, and we definitely want
> that to happen, otherwise another device might steal it during DAA.

My bad, it did. This being said, I find it much easier to follow when
the reservation happens in one place, so I'm still in favor of the new
version.