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>,
        Przemyslaw Gaj <pgaj@cadence.com>,
        "linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Milind Parab <mparab@cadence.com>,
        "praneeth@ti.com" <praneeth@ti.com>
Subject: RE: [PATCH v3] i3c: master: fix for SETDASA and DAA process
Thread-Topic: [PATCH v3] i3c: master: fix for SETDASA and DAA process
Thread-Index: AQHWdvc2nK9xkXfr/kODGwYfmmKGXqlBE/UAgAAwIfA=
Date:   Thu, 20 Aug 2020 18:16:14 +0000
Message-ID: <DM5PR07MB3196085F8628478E7E2F5FC8C15A0@DM5PR07MB3196.namprd07.prod.outlook.com>
References: <1597930706-15744-1-git-send-email-pthombar@cadence.com>
 <20200820164820.4fec97b3@collabora.com>
In-Reply-To: <20200820164820.4fec97b3@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: edd1027b-ec3a-4b0e-a9de-08d845351fe7
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Aug 2020 18:16:14.3004
 (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: d1s4t4748iJ6B/Cj7NlX/uey9spPXWH25pS5XedNWZIk0lEqpAYgxt3aaMkFxLwlOPd4S6zsvcO45EnG1XMI7QmNMa9GI4zM/ClpFW6NyYw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR07MB6889
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235,18.0.687
 definitions=2020-08-20_03:2020-08-19,2020-08-20 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_check_notspam policy=outbound_check score=0 mlxscore=0
 suspectscore=0 impostorscore=0 bulkscore=0 phishscore=0 adultscore=0
 spamscore=0 lowpriorityscore=0 malwarescore=0 clxscore=1015
 mlxlogscore=999 priorityscore=1501 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.12.0-2006250000 definitions=main-2008200147
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>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=
,=20
due to limited available slots this can cause issue.=20
 If some slots are lost due to
1. only init_dyn_addr and no static_addr in DT=20
OR
2. SETDASA failed
at the end of DAA some devices may be left without dyn_addr allocated from =
master
and hence can't work properly.
I think during our discussion we recognized this change as a bug.
That is the reason I added fixes tag, but if you think otherwise I can remo=
ve this tag.
=20
>> -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 ?
I am not sure if we can't have init_dyn_addr without static_addr.
May be what we need is=20
		if (!i3cboardinfo->init_dyn_addr)
			continue;

		ret =3D i3c_bus_get_addr_slot_status(&master->bus,
						   i3cboardinfo->init_dyn_addr);
		if (ret !=3D I3C_ADDR_SLOT_FREE) {
			ret =3D -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);
IMHO this is functionally same to what I sent. Just that init_dyn_addr is r=
eserved before,
and we leverage the change in reattach to bypass failure due to second atte=
mpt
to get init_dyn_addr in reattach called from i3c_master_pre_assign_dyn_addr=
().