Re: [PATCH v2 2/2] i3c: master: fix for SETDASA and DAA process

From: Boris Brezillon
Date: Thu Aug 20 2020 - 08:40:08 EST


On Thu, 20 Aug 2020 09:23:25 +0000
Parshuram Raju Thombare <pthombar@xxxxxxxxxxx> wrote:

> Hi Boris,
>
> Thanks for your comments.
>
> >> + * We anyway don't attach devices which are not addressable
> >
> >You can drop the anyway.
> Sure, I will make above mentioned change in the comment.
>
> >> + * (no static_addr and dyn_addr) and devices with static_addr
> >> + * but no init_dyn_addr will participate in DAA.
> >> + */
> >> + if (!boardinfo->static_addr || !boardinfo->init_dyn_addr)
> >> + return -EINVAL;
> >
> >If we consider this as an error, we should probably check that before
> >calling i3c_master_pre_assign_dyn_addr() in i3c_master_bus_init().
> Ok, I will move this check to i3c_master_bus_init(), before calling
> i3c_master_pre_assign_dyn_addr. It will probably add extra if condition,
> but will save one function call.
>
> >> * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C
> >> - * devices that have a static address
> >> + * devices that have a static address and attach corresponding statically
> >> + * defined I3C devices to the master.
> >
> > and attach them to the
> > master if
> > the dynamic address assignment succeeds
> Sure, I will append above mentioned change to the comment.
>
> >> + /*
> >> + * Free reserved init_dyn_addr so that attach can
> >> + * get it before trying setnewda.
> >> + */
> >> + if (i3cboardinfo->init_dyn_addr)
> >> + i3c_bus_set_addr_slot_status(&master->bus,
> >> + init_dyn_addr,
> >> + I3C_ADDR_SLOT_FREE);
> >
> >Hm, it's a bit more complicated. I don't think we can unconditionally
> >release the init_dyn_addr here. Say you have a device that's been
> >assigned its init_dyn_addr, and userspace decided to re-assign a new
> >one (the feature is not available yet, but I thought about letting
> >userspace write to the dyn_addr sysfs entry and wire that to a SETDA).
> >The init_dyn_addr can now be re-assigned to a different device. After
> >some time the device ends up resetting and thus lose its DA. A new DAA
> >is issued to re-discover it, but you want this device to be assigned its
> >last known address not the init address. And when
> >i3c_master_attach_boardinfo() is called on this new device, you release
> >a slot that's no longer yours.
> >
> >That was the rational behind the "address slots are attached to i3cdevs
> >not boardinfo". Maybe we should have another list where we keep i3c
> >devs that have not been discovered yet but have boardinfo attached to
> >them. This way we can reserve dynamic addresses without blocking a
> >slot in the master device table.
>
> I think the sequence of events you are discussing here is
> 1. User assign address to device A with init_dyn_addr in boardinfo.
> 2. That particular init_dyn_addr is assigned to device B, which may be hotplugged ?
> and don't have boardinfo or init_dyn_addr in boardinfo ?
> 3. Device A resets and trigger DAA due to hot plug ?
> A. Here now init_dyn_addr is already assigned to device B so device A shouldn't be freeing it.
> B. Now preferable dyn_addr is the one received from user in step 1.

No, that's not what I'm talking about. I meant:

1. Device A is assigned a default init address X in the DT.
2. Device B has no init address
3. The framework reserves address X for and assigns it to device A
when it appears on the bus (DAA, SETDASA or HJ+DDA)
4. Device B is assigned address Y
5. User decides to explicitly assign a different address to device A by
issuing "echo Z > /sys/bus/i3c/..../<i3c-dev>/dyn_addr" (not yet
supported, but I think we should allow that at some point), such
that device A gets a lower/higher priority
6. User manually assigns address X to device B (that should be allowed
since device A no longer uses X)
7. Device A is reset for some reason and loses its dynamic address,
thus requiring a new DAA (or HJ+DAA). During this new discovery,
device A is re-assigned its last known address (Z), but in the
meantime you've marked address X as free (when attaching boardinfo
to the newdev object).

>
> If we are to prefer init_dyn_addr always, that will rule out possibility of making init_dyn_addr
> available to any other device when original device is assigned with user or master
> provided address owing to SETDATA or SETNEWDA failures. And we can be sure of not freeing
> init_dyn_addr inadvertently while it is being used by any other device.
>
> Else if we want to prefer user provided address even across resets, since we don't need init_dyn_addr
> anymore, it can be used to store user provided address. This will serve both the purposes A and B stated above.
>
> And in my opinion this can be handled when we add code to allow user to change the device address.

If we go for a temporary solution, I'd opt for relaxing the test done
in i3c_master_get_i3c_addrs() to not reserve the init address (since it
should have been reserved at probe time) and keep those init addressed
reserved.