Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Parshuram Raju Thombare <pthombar@cadence.com>
To:     Boris Brezillon <boris.brezillon@collabora.com>
CC:     "bbrezillon@kernel.org" <bbrezillon@kernel.org>,
        "vitor.soares@synopsys.com" <vitor.soares@synopsys.com>,
        Milind Parab <mparab@cadence.com>,
        "praneeth@ti.com" <praneeth@ti.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Przemyslaw Gaj <pgaj@cadence.com>,
        "linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org>
Subject: RE: [PATCH v2 2/2] i3c: master: fix for SETDASA and DAA process
Thread-Topic: [PATCH v2 2/2] i3c: master: fix for SETDASA and DAA process
Thread-Index: AQHWL1LbKgAavLYOLUSFNKBnioxWdKk/olsAgAFk1TA=
Date:   Thu, 20 Aug 2020 09:23:25 +0000
Message-ID: <DM5PR07MB31965D54B61E191E3C1325FEC15A0@DM5PR07MB3196.namprd07.prod.outlook.com>
References: <1590053467-32079-1-git-send-email-pthombar@cadence.com>
        <1590053581-803-1-git-send-email-pthombar@cadence.com>
 <20200819101238.760d4e90@collabora.com>
In-Reply-To: <20200819101238.760d4e90@collabora.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 6aa9b8de-5cd4-44cf-2ba8-08d844eab145
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Aug 2020 09:23:25.7342
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: d36035c5-6ce6-4662-a3dc-e762e61ae4c9
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Caj2NNpb0y24AIv+jupvqT8RYn78IoMYa9K4J3TML8ZBI78L2Noauvb4AO/r4jbM1c+fmCPPBXSWud/Rp/rBoizmJj97icWVXs3HMxt/Yzo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR07MB3877
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235,18.0.687
 definitions=2020-08-20_02:2020-08-19,2020-08-20 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_check_notspam policy=outbound_check score=0 phishscore=0
 mlxscore=0 priorityscore=1501 clxscore=1011 lowpriorityscore=0
 adultscore=0 impostorscore=0 malwarescore=0 spamscore=0 suspectscore=0
 bulkscore=0 mlxlogscore=999 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.12.0-2006250000 definitions=main-2008200080
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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 stati=
cally
>> + *    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 hotp=
lugged ?
    and don't have boardinfo or init_dyn_addr in boardinfo ?=20
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 sh=
ouldn't be freeing it.
   B. Now preferable dyn_addr is the one received from user in step 1.

If we are to prefer init_dyn_addr always, that will rule out possibility of=
 making init_dyn_addr=20
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 w=
e don't need init_dyn_addr
anymore, it can be used to store user provided address. This will serve bot=
h the purposes A and B stated above.
=20
And in my opinion this can be handled when we add code to allow user to cha=
nge the device address.

Regards,
Parshuram Thombare