On Thu, 20 Aug 2020 21:03:11 +0200
Boris Brezillon <boris.brezil...@collabora.com> wrote:
> On Thu, 20 Aug 2020 18:16:14 +0000
> Parshuram Raju Thombare <pthom...@cadence.com> 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.
