From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 114D4C43467 for ; Thu, 8 Oct 2020 10:17:47 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9B61621734 for ; Thu, 8 Oct 2020 10:17:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1602152266; bh=jnzPcjlckmhKzqEG8+iGrWISe7UZj+mwIWSZMDd+HC8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=bNjr7P4lnmRiEQP2SaDcsBI3nHf+niEr1YDN1Wlj7jIiL3L3AolrJhLtM1/nOPA2i XI5HR1ztgNRB4a3AZpHSBj+J+qYmAL4rWWbt9ylPKQ071j494uXZKTQtYhi9AhSzPb nasXi+AVXCnLNOyp24Wlbjx0Syxnq1Quhbjp2NZ4= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729311AbgJHKRp (ORCPT ); Thu, 8 Oct 2020 06:17:45 -0400 Received: from mail.kernel.org ([198.145.29.99]:50910 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729210AbgJHKRo (ORCPT ); Thu, 8 Oct 2020 06:17:44 -0400 Received: from localhost (unknown [213.57.247.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id C2DDC2076B; Thu, 8 Oct 2020 10:17:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1602152262; bh=jnzPcjlckmhKzqEG8+iGrWISe7UZj+mwIWSZMDd+HC8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=R0BAMywuKP/3WYWluorvqbOkEZdZTMSjU7Oe3EOYUSFguVokX6WYT7OUj5DN0soxB AhAbWQYAt1zDoKoVVwyye+FaTThbyAnK7QsQAmqDHv1aZ4k6gBC7aEMMl10vIgpM7L fTPltMCWepXGQc2Vn42MB8F+CStVKC9Jhay8ydws= Date: Thu, 8 Oct 2020 13:17:37 +0300 From: Leon Romanovsky To: Parav Pandit Cc: Pierre-Louis Bossart , "Ertman, David M" , "alsa-devel@alsa-project.org" , "parav@mellanox.com" , "tiwai@suse.de" , "netdev@vger.kernel.org" , "ranjani.sridharan@linux.intel.com" , "fred.oh@linux.intel.com" , "linux-rdma@vger.kernel.org" , "dledford@redhat.com" , "broonie@kernel.org" , Jason Gunthorpe , "gregkh@linuxfoundation.org" , "kuba@kernel.org" , "Williams, Dan J" , "Saleem, Shiraz" , "davem@davemloft.net" , "Patil, Kiran" Subject: Re: [PATCH v2 1/6] Add ancillary bus support Message-ID: <20201008101737.GL13580@unreal> References: <20201008052623.GB13580@unreal> <20201008074525.GJ13580@unreal> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org On Thu, Oct 08, 2020 at 09:45:29AM +0000, Parav Pandit wrote: > > > > From: Leon Romanovsky > > Sent: Thursday, October 8, 2020 1:15 PM > > > > On Thu, Oct 08, 2020 at 07:14:17AM +0000, Parav Pandit wrote: > > > > > > > > > > From: Leon Romanovsky > > > > Sent: Thursday, October 8, 2020 10:56 AM > > > > > > > > On Thu, Oct 08, 2020 at 04:56:01AM +0000, Parav Pandit wrote: > > > > > > > > > > > > > > > > From: Pierre-Louis Bossart > > > > > > > > > > > > Sent: Thursday, October 8, 2020 3:20 AM > > > > > > > > > > > > > > > > > > On 10/7/20 4:22 PM, Ertman, David M wrote: > > > > > > >> -----Original Message----- > > > > > > >> From: Pierre-Louis Bossart > > > > > > >> > > > > > > >> Sent: Wednesday, October 7, 2020 1:59 PM > > > > > > >> To: Ertman, David M ; Parav Pandit > > > > > > >> ; Leon Romanovsky > > > > > > >> Cc: alsa-devel@alsa-project.org; parav@mellanox.com; > > > > > > >> tiwai@suse.de; netdev@vger.kernel.org; > > > > > > >> ranjani.sridharan@linux.intel.com; > > > > > > >> fred.oh@linux.intel.com; linux-rdma@vger.kernel.org; > > > > > > >> dledford@redhat.com; broonie@kernel.org; Jason Gunthorpe > > > > > > >> ; gregkh@linuxfoundation.org; > > > > > > >> kuba@kernel.org; Williams, Dan J ; > > > > > > >> Saleem, Shiraz ; > > > > > > >> davem@davemloft.net; Patil, Kiran > > > > > > >> Subject: Re: [PATCH v2 1/6] Add ancillary bus support > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >>>> Below is most simple, intuitive and matching with core APIs > > > > > > >>>> for name and design pattern wise. > > > > > > >>>> init() > > > > > > >>>> { > > > > > > >>>> err = ancillary_device_initialize(); > > > > > > >>>> if (err) > > > > > > >>>> return ret; > > > > > > >>>> > > > > > > >>>> err = ancillary_device_add(); > > > > > > >>>> if (ret) > > > > > > >>>> goto err_unwind; > > > > > > >>>> > > > > > > >>>> err = some_foo(); > > > > > > >>>> if (err) > > > > > > >>>> goto err_foo; > > > > > > >>>> return 0; > > > > > > >>>> > > > > > > >>>> err_foo: > > > > > > >>>> ancillary_device_del(adev); > > > > > > >>>> err_unwind: > > > > > > >>>> ancillary_device_put(adev->dev); > > > > > > >>>> return err; > > > > > > >>>> } > > > > > > >>>> > > > > > > >>>> cleanup() > > > > > > >>>> { > > > > > > >>>> ancillary_device_de(adev); > > > > > > >>>> ancillary_device_put(adev); > > > > > > >>>> /* It is common to have a one wrapper for this as > > > > > > >>>> ancillary_device_unregister(). > > > > > > >>>> * This will match with core device_unregister() that has > > > > > > >>>> precise documentation. > > > > > > >>>> * but given fact that init() code need proper error > > > > > > >>>> unwinding, like above, > > > > > > >>>> * it make sense to have two APIs, and no need to export > > > > > > >>>> another symbol for unregister(). > > > > > > >>>> * This pattern is very easy to audit and code. > > > > > > >>>> */ > > > > > > >>>> } > > > > > > >>> > > > > > > >>> I like this flow +1 > > > > > > >>> > > > > > > >>> But ... since the init() function is performing both > > > > > > >>> device_init and device_add - it should probably be called > > > > > > >>> ancillary_device_register, and we are back to a single > > > > > > >>> exported API for both register and unregister. > > > > > > >> > > > > > > >> Kind reminder that we introduced the two functions to allow > > > > > > >> the caller to know if it needed to free memory when > > > > > > >> initialize() fails, and it didn't need to free memory when > > > > > > >> add() failed since > > > > > > >> put_device() takes care of it. If you have a single init() > > > > > > >> function it's impossible to know which behavior to select on error. > > > > > > >> > > > > > > >> I also have a case with SoundWire where it's nice to first > > > > > > >> initialize, then set some data and then add. > > > > > > >> > > > > > > > > > > > > > > The flow as outlined by Parav above does an initialize as the > > > > > > > first step, so every error path out of the function has to do > > > > > > > a put_device(), so you would never need to manually free the > > > > > > > memory in > > > > > > the setup function. > > > > > > > It would be freed in the release call. > > > > > > > > > > > > err = ancillary_device_initialize(); if (err) > > > > > > return ret; > > > > > > > > > > > > where is the put_device() here? if the release function does any > > > > > > sort of kfree, then you'd need to do it manually in this case. > > > > > Since device_initialize() failed, put_device() cannot be done here. > > > > > So yes, pseudo code should have shown, if (err) { > > > > > kfree(adev); > > > > > return err; > > > > > } > > > > > > > > > > If we just want to follow register(), unregister() pattern, > > > > > > > > > > Than, > > > > > > > > > > ancillar_device_register() should be, > > > > > > > > > > /** > > > > > * ancillar_device_register() - register an ancillary device > > > > > * NOTE: __never directly free @adev after calling this function, > > > > > even if it returned > > > > > * an error. Always use ancillary_device_put() to give up the > > > > > reference > > > > initialized by this function. > > > > > * This note matches with the core and caller knows exactly what > > > > > to be > > > > done. > > > > > */ > > > > > ancillary_device_register() > > > > > { > > > > > device_initialize(&adev->dev); > > > > > if (!dev->parent || !adev->name) > > > > > return -EINVAL; > > > > > if (!dev->release && !(dev->type && dev->type->release)) { > > > > > /* core is already capable and throws the warning when > > > > release callback is not set. > > > > > * It is done at drivers/base/core.c:1798. > > > > > * For NULL release it says, "does not have a release() > > > > function, it is broken and must be fixed" > > > > > */ > > > > > return -EINVAL; > > > > > } > > > > > err = dev_set_name(adev...); > > > > > if (err) { > > > > > /* kobject_release() -> kobject_cleanup() are capable to > > > > detect if name is set/ not set > > > > > * and free the const if it was set. > > > > > */ > > > > > return err; > > > > > } > > > > > err = device_add(&adev->dev); > > > > > If (err) > > > > > return err; > > > > > } > > > > > > > > > > Caller code: > > > > > init() > > > > > { > > > > > adev = kzalloc(sizeof(*foo_adev)..); > > > > > if (!adev) > > > > > return -ENOMEM; > > > > > err = ancillary_device_register(&adev); > > > > > if (err) > > > > > goto err; > > > > > > > > > > err: > > > > > ancillary_device_put(&adev); > > > > > return err; > > > > > } > > > > > > > > > > cleanup() > > > > > { > > > > > ancillary_device_unregister(&adev); > > > > > } > > > > > > > > > > Above pattern is fine too matching the core. > > > > > > > > > > If I understand Leon correctly, he prefers simple register(), > > > > > unregister() > > > > pattern. > > > > > If, so it should be explicit register(), unregister() API. > > > > > > > > This is my summary > > > > https://lore.kernel.org/linux-rdma/20201008052137.GA13580@unreal > > > > The API should be symmetric. > > > > > > > > > > I disagree to your below point. > > > > 1. You are not providing driver/core API but simplification and > > > > obfuscation of basic primitives and structures. This is new layer. > > > > There is no room for a claim that we must to follow internal API. > > > If ancillary bus has > > > ancillary_device_add(), it cannot do device_initialize() and device_add() in > > both. > > > > > > I provided two examples and what really matters is a given patchset > > > uses (need to use) which pattern, > > > initialize() + add(), or register() + unregister(). > > > > > > As we all know that API is not added for future. It is the future patch > > extends it. > > > So lets wait for Pierre to reply if soundwire can follow register(), > > unregister() sequence. > > > This way same APIs can service both use-cases. > > > > > > Regarding, > > > > 3. You can't "ask" from users to call internal calls (put_device) > > > > over internal fields in ancillary_device. > > > In that case if should be ancillary_device_put() ancillary_device_release(). > > > > > > Or we should follow the patten of ib_alloc_device [1], > > > ancillary_device_alloc() > > > -> kzalloc(adev + dev) with compile time assert check like rdma and vdpa > > subsystem. > > > ->device_initialize() > > > ancillary_device_add() > > > > > > ancillar_device_de() <- balances with add > > > ancillary_device_dealloc() <-- balances with device_alloc(), which does the > > put_device() + free the memory allocated in alloc(). > > > > > > This approach of [1] also eliminates exposing adev.dev.release = > > in drivers. > > > And container_of() benefit also continues.. > > > > > > [1] > > > https://elixir.bootlin.com/linux/v5.9-rc8/source/include/rdma/ib_verbs > > > .h#L2791 > > > > > > > My code looks like this, probably yours looks the same. > > > > 247 priv->adev[i] = kzalloc(sizeof(*priv->adev[i]), GFP_KERNEL); > > 248 if (!priv->adev[i]) > > 249 goto init_err; > > 250 > > 251 adev = &priv->adev[i]->adev; > > 252 adev->id = idx; > > 253 adev->name = mlx5_adev_devices[i].suffix; > > 254 adev->dev.parent = dev->device; > > 255 adev->dev.release = adev_release; > > 256 priv->adev[i]->mdev = dev; > > 257 > > 258 ret = ancillary_device_initialize(adev); > > 259 if (ret) > > 260 goto init_err; > > 261 > > 262 ret = ancillary_device_add(adev); > > 263 if (ret) { > > 264 put_device(&adev->dev); > > 265 goto add_err; > > 266 } > > Yes, subfunction code is also very similar. > You expressed concerned that you didn't like put_device() at [1]. > But in above code is touching adev->dev.{parent, release} is ok? Yes, "adev->dev.{parent, release}" is not ok, but at least it doesn't complicate error unwinding. This is why I didn't say anything about it. > > 254 adev->dev.parent = dev->device; > > 255 adev->dev.release = adev_release; > > If not, > > We can make it elegant by doing, I like your idea, IMHO it is more clear and less error prone. Thanks > > the patten of ib_alloc_device [1], > ancillary_device_alloc() > -> kzalloc(adev + dev) with compile time assert check like rdma and vdpa subsystem. > ->device_initialize() > ancillary_device_add() > > ancillar_device_de() <- balances with add > ancillary_device_dealloc() <-- balances with device_alloc(), which does the put_device() + free the memory allocated in alloc(). > > This approach of [2] also eliminates exposing adev.dev.release = in drivers. > And container_of() benefit also continues.. > > [1] https://lore.kernel.org/linux-rdma/20201007192610.GD3964015@unreal/ > [2] https://elixir.bootlin.com/linux/v5.9-rc8/source/include/rdma/ib_verbs.h#L2791