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:     Alexandre Belloni <alexandre.belloni@bootlin.com>
CC:     Nicolas Pitre <nico@fluxnic.net>,
        "slongerbeam@gmail.com" <slongerbeam@gmail.com>,
        "vitor.soares@synopsys.com" <vitor.soares@synopsys.com>,
        "praneeth@ti.com" <praneeth@ti.com>,
        Milind Parab <mparab@cadence.com>,
        "linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v10 2/7] i3c: master: use i3c_master_register only for
 main master
Thread-Topic: [PATCH v10 2/7] i3c: master: use i3c_master_register only for
 main master
Thread-Index: AQHWxuCK9YC2+rNE30iVVcs2nY9FcKnmXNiAgAZffLCBHy/NAIANK1GA
Date:   Thu, 17 Jun 2021 08:42:28 +0000
Message-ID: <CY4PR07MB27570948E3536C160662AEEAC10E9@CY4PR07MB2757.namprd07.prod.outlook.com>
References: <1606716983-3645-1-git-send-email-pthombar@cadence.com>
 <1606717066-3785-1-git-send-email-pthombar@cadence.com>
 <2s3n7419-1nr9-8885-882o-7qrsos6qq6q@syhkavp.arg>
 <DM5PR07MB319618AFCB415501CD33741FC1CD0@DM5PR07MB3196.namprd07.prod.outlook.com>
 <YL/7XP1fs/2bRiTM@piout.net>
In-Reply-To: <YL/7XP1fs/2bRiTM@piout.net>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?2ZWC3zuMKEcunXC35YdoBcoofxQ6SJQ1nPOcq5rM2RqpILGhCP6LDPUe2hNd?=
 =?us-ascii?Q?ID3Wr2GxD6gaRKX7gpb68DVNgRO3becDmKnmPM+n5uQrAML98nNhTwNKPahl?=
 =?us-ascii?Q?8hS8DwFOEZN9Y4VsZOmT4cGdeso0Z4CIDdgg4oW4JRsNb3aaTPD+U+qUpjXB?=
 =?us-ascii?Q?7k+1nmIM7SoDOJ48phB//xhNDalBXiqrKfVeSd64MjZyvYPK1sGujcZqtIFb?=
 =?us-ascii?Q?eohexJ6ucIru79TJrSYUilmaBT+pzpfc3g73ec+GFrr16HFxc3bH7jLMRQd4?=
 =?us-ascii?Q?T9Y8UsfWSo6h84der7kkUT8FKnBjcpthqDV3WnV/vd52+HLZ9HpjOXV1VCUW?=
 =?us-ascii?Q?0Wal0y6E7sOYPf+QVXW2R9aAiXOb0jvmG7RNqz0vQ/J/lshYxyh1K8Q0XQm4?=
 =?us-ascii?Q?eIu+0VBSN1Y4XDQcqmaVdBDsaPGiGypAPyY5+SPzMLAb3G3J9936A/ajgmgT?=
 =?us-ascii?Q?TETyufwkSBK3L4o8XlN/1jfupdOkwU5TL9APa1f55Z+4SYuvDD845iVwJTju?=
 =?us-ascii?Q?8R9aZilPSimV5fKXVTx0K+pCjhiEYQsJFJay+tk98GxMQ7ko/SAEMvc+Q10C?=
 =?us-ascii?Q?kgQ+ssWGNJQm0FEtQjtCAJqHDolsogj6/GVSxsltR9OzLt699v5JGsZ6RmH6?=
 =?us-ascii?Q?3tas9bHmrIjDHCYsmDC3X8ljs4u0dxaWSNR/4bw9f1uq2+RikfyyjCKixeGg?=
 =?us-ascii?Q?4z3Fz6EiUZoIUMzofsiT6Aj6mhhIajqlytMMXJiolJzSGJaIZx/BM01DdqNw?=
 =?us-ascii?Q?8loql8W5JXGJ1OA7auIZtzyffno72EmaJLWuxJuAs74/yZztKW1ap28OFkwE?=
 =?us-ascii?Q?yUwkDGpGUeA4x7t4gkE1lH/HfR8bqnjkZG8QvuiZ9fc8sgYwpX53BX4a5pFa?=
 =?us-ascii?Q?N9xFt42EkZ6Pyl6ca1jEey1fqz4qoyQpmZs3enwi8iILrO11p6idCbiOOJcJ?=
 =?us-ascii?Q?6OXFHUPqdQrUZ4VZMs9yJINuzcAP2s1dvJ2OTUk+SuJZAL6TmbmhqKw2DfrU?=
 =?us-ascii?Q?krXYKQUIN90xiV2Uwt/JNs2j+0Tb/ald6zxV4v2eS8pFY/wIEAqsG93z3hAE?=
 =?us-ascii?Q?lFwjo5ZXkomuOvuqOk01d3cj8aAn9Rtp/weZZtwGEcz5vclT8Np0WshrWQnl?=
 =?us-ascii?Q?/lYdYph8tf9y7l9oAa9xFCpVd5eVIp96sPBnlWCLy+w2OeUr/qM5ZTPY0ja9?=
 =?us-ascii?Q?/kTyaKyy75ddSveq1GKInID5GFtq/MHi/3SFjiLrCD7wHBR9SFhK+d890G6a?=
 =?us-ascii?Q?wjJp8ZVfCADuk7UP9dpzP/cMisTRPtnm7RGpbmuUeh9Ql7dJBx/h2BQuwvvc?=
 =?us-ascii?Q?gqQ=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: cadence.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CY4PR07MB2757.namprd07.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b6bbee71-65de-48e6-83dc-08d9316bd71e
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Jun 2021 08:42:28.5950
 (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: BEug6SxLHR8etDmKTmAH/wJ56otvbLCODEcUEtRLZUC/K4+9NRIDFEn+inC+1nzGnN73uMxRQZdrvfoYCRBOOO1Tah51uk+bLAMcYc9bRK0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR0701MB3795
X-Proofpoint-GUID: WzX8c86hSylx_A8TmiAAyeEPwh_45lJr
X-Proofpoint-ORIG-GUID: WzX8c86hSylx_A8TmiAAyeEPwh_45lJr
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.790
 definitions=2021-06-17_05:2021-06-15,2021-06-17 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_check_notspam policy=outbound_check score=0 mlxlogscore=999
 priorityscore=1501 adultscore=0 impostorscore=0 suspectscore=0
 lowpriorityscore=0 phishscore=0 malwarescore=0 bulkscore=0 clxscore=1011
 mlxscore=0 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104190000 definitions=main-2106170060
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Alexandre,

Thank you for review comments.

>On 08/12/2020 05:47:51+0000, Parshuram Raju Thombare wrote:
>> >This looks a bit confusing. Here you're rolling back detailss in
>> >i3c_primary_master_register() that were factored out in
>> >i3c_master_init(). If i3c_master_init() is successful, then you
>> >shouldn't be undoing its things openly in i3c_primary_master_register()=
.
>> >Instead, there should be another function that does the reverse of
>> >i3c_master_init() here.
>>
>> Every function do its cleanup in case of failures.
>> And if any failure occur after successful i3c_master_init(), we have
>> function i3c_master_bus_cleanup() which does the major cleanup.
>>
>
>The point from Nicolas here was that the workqueue is allocated in
>i3c_master_init so you should have a function to destroy it instead of
>having to do that separately in i3c_primary_master_register. The same is
>true for the put_device. Or you have to ensure i3c_masterdev_release
>is called when i3c_primary_master_register fails.

Ok, IIUC, it is just that we need separate clean up function.=20
i3c_master_bus_cleanup is used here to call driver cleanup function and
detach + release devices from bus, and I am not sure i3c_masterdev_release
can be used for that.
Better alternative is i3c_master_unregister(). However, it doesn't handle
the case where device_add fails, as this function unconditionally calls
device_unregister which call both device_del and put_device.