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.0 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,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 27839C43467 for ; Thu, 8 Oct 2020 10:18:48 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 6F7572145D for ; Thu, 8 Oct 2020 10:18:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="KV4kFv7l"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="R0BAMywu" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6F7572145D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-devel-bounces@alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 97A37167A; Thu, 8 Oct 2020 12:17:54 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 97A37167A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1602152324; bh=jnzPcjlckmhKzqEG8+iGrWISe7UZj+mwIWSZMDd+HC8=; h=Date:From:To:Subject:References:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=KV4kFv7ly2tLCUWOWhvSGsEDZCY8KUuj25h9TeKMwo7Ner1zN0DkhYPSAi6Jx1Z+0 2aJPsGvPHMnDwywtdL7u2aFWhczY/py8xEDlHkR6nCW6upqroBBRjqcZjsvxQhLa9U NHvWmyBfeQX7MhfA8cpHpusIaDVRKWf/kl7/3nEY= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 0C836F80163; Thu, 8 Oct 2020 12:17:54 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id AA5A7F80164; Thu, 8 Oct 2020 12:17:52 +0200 (CEST) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id B1CE3F80090 for ; Thu, 8 Oct 2020 12:17:45 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz B1CE3F80090 Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="R0BAMywu" 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 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: Cc: "alsa-devel@alsa-project.org" , "kuba@kernel.org" , "parav@mellanox.com" , "tiwai@suse.de" , "netdev@vger.kernel.org" , Pierre-Louis Bossart , "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" , "Ertman, David M" , "Williams, Dan J" , "Saleem, Shiraz" , "davem@davemloft.net" , "Patil, Kiran" X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" 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