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 F369FC43467 for ; Thu, 8 Oct 2020 07:01:43 +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 8358C2087D for ; Thu, 8 Oct 2020 07:01:41 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="REOLBoUC"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="G5JC8jtq" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8358C2087D 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 6F19116AA; Thu, 8 Oct 2020 09:00:49 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 6F19116AA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1602140499; bh=3tOO4RN1EatOt5kk0t9+qLCoWAl86clXtcWvklZwDZQ=; h=Date:From:To:Subject:References:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=REOLBoUCTFZvd6qF2qp/D5BY56HHr63fGq8pwbkXu8uqDeMbhAoJB2wYzN08X2FO0 CDUOzhIqTJkVSZVa4/12cSTXjhoLj2THQBhyvGGTsNPAmzCQ01pmSJ7uJzmpEJVEr/ v4VqscwWeBFyMXxhrw7RGdoiaKIieJTU5we9j1K0= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 0AC07F8015B; Thu, 8 Oct 2020 09:00:49 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 966AEF80164; Thu, 8 Oct 2020 09:00:47 +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 5C221F8015A for ; Thu, 8 Oct 2020 09:00:40 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 5C221F8015A Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="G5JC8jtq" 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 D8C1C2168B; Thu, 8 Oct 2020 07:00:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1602140437; bh=3tOO4RN1EatOt5kk0t9+qLCoWAl86clXtcWvklZwDZQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=G5JC8jtqkp68NJG95+W4kQtjS/psmxzZTsevnnGPg2L+CakCzTGygxEijW7Sne2q8 q89F7hzIsWHti924+e0jIYNuScrw/f2Xm64QwAbnHJ9DOnDrbQPOWURrevpjPUQau/ Ahd53Snm6mYuoNnlPsotYwkI6htOk0Ia8PoMtGhE= Date: Thu, 8 Oct 2020 10:00:32 +0300 From: Leon Romanovsky To: Dan Williams Subject: Re: [PATCH v2 1/6] Add ancillary bus support Message-ID: <20201008070032.GG13580@unreal> References: <20201005182446.977325-2-david.m.ertman@intel.com> <20201006071821.GI1874917@unreal> <20201006170241.GM1874917@unreal> <20201007192610.GD3964015@unreal> <20201008052137.GA13580@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" , Parav Pandit , Jason Gunthorpe , "gregkh@linuxfoundation.org" , "Ertman, David M" , "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 Wed, Oct 07, 2020 at 11:32:11PM -0700, Dan Williams wrote: > On Wed, Oct 7, 2020 at 10:21 PM Leon Romanovsky wrote: > > > > On Wed, Oct 07, 2020 at 08:46:45PM +0000, Ertman, David M wrote: > > > > -----Original Message----- > > > > From: Parav Pandit > > > > Sent: Wednesday, October 7, 2020 1:17 PM > > > > To: Leon Romanovsky ; Ertman, David M > > > > > > > > Cc: Pierre-Louis Bossart ; 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 > > > > > > > > > > > > > From: Leon Romanovsky > > > > > Sent: Thursday, October 8, 2020 12:56 AM > > > > > > > > > > > > This API is partially obscures low level driver-core code and needs > > > > > > > to provide clear and proper abstractions without need to remember > > > > > > > about put_device. There is already _add() interface why don't you do > > > > > > > put_device() in it? > > > > > > > > > > > > > > > > > > > The pushback Pierre is referring to was during our mid-tier internal > > > > > > review. It was primarily a concern of Parav as I recall, so he can speak to > > > > his > > > > > reasoning. > > > > > > > > > > > > What we originally had was a single API call > > > > > > (ancillary_device_register) that started with a call to > > > > > > device_initialize(), and every error path out of the function performed a > > > > > put_device(). > > > > > > > > > > > > Is this the model you have in mind? > > > > > > > > > > I don't like this flow: > > > > > ancillary_device_initialize() > > > > > if (ancillary_ancillary_device_add()) { > > > > > put_device(....) > > > > > ancillary_device_unregister() > > > > Calling device_unregister() is incorrect, because add() wasn't successful. > > > > Only put_device() or a wrapper ancillary_device_put() is necessary. > > > > > > > > > return err; > > > > > } > > > > > > > > > > And prefer this flow: > > > > > ancillary_device_initialize() > > > > > if (ancillary_device_add()) { > > > > > ancillary_device_unregister() > > > > This is incorrect and a clear deviation from the current core APIs that adds the > > > > confusion. > > > > > > > > > return err; > > > > > } > > > > > > > > > > In this way, the ancillary users won't need to do non-intuitive put_device(); > > > > > > > > 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. > > > > > > At that point, do we need wrappers on the primitives init, add, del, > > > and put? > > > > Let me summarize. > > 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. > > Yes, this a driver core api, Greg even questioned why it was in > drivers/bus instead of drivers/base which I think makes sense. We can argue till death, but at the end, this is a bus. > > > 2. API should be symmetric. If you call to _register()/_add(), you will need > > to call to _unregister()/_del(). Please don't add obscure _put(). > > It's not obscure it's a long standing semantic for how to properly > handle device_add() failures. Especially in this case where there is > no way to have something like a common auxiliary_device_alloc() that > will work for everyone the only other option is require all device > destruction to go through the provided release method (put_device()) > after a device_add() failure. And this is my main concern, this is not device_add() failure but ancillary_device_add() which hides driver_* logic. We won't expect to see inside ancillary drivers direct calls to device_*(), why will it be different here with put_device? > > > 3. You can't "ask" from users to call internal calls (put_device) over internal > > fields in ancillary_device. > > Sure it can. platform_device_add() requires a put_device() on failure, > but also note how platform_device_add() *requires* > platform_device_alloc() be used to create the device. That > inflexibility is something this auxiliary bus is trying to avoid. I'm writing below, the rationale behind this bus is RDMA, netdev and other cross-subsystem devices. > > > 4. This API should be clear to drivers authors, "device_add()" call (and > > semantic) is not used by the drivers (git grep " device_add(" drivers/). > > This shows 141 instances for me, so I'm not sure what you're getting at? Did you look at them? I did, most if not all of the calls are in bus/core/generic logic, drivers are not calling to it or at least not supposed to. > > Look, this api is meant to be a replacement for places where platform > devices were being abused. The device_initialize() + customize device > + device_add() organization has the flexibility needed to let users > customize naming and other parts of device creation in a way that a > device_register() flow, or platform_device_{register,add} in > particular, did not. It is hard me to say if the goal it to replace platform devices or not, but this ancillary_device bus adventure started after request to stop reinvent PCI logic for every new RDMA (RoCE) drivers. This is there full power of this virtbus solution comes into full power by deleting tons of complex code. > > If the concern is that you'd like to have an auxiliary_device_put() > for symmetry that would need to come with the same warning as > commented on platform_device_put(), i.e. that's it's really only > vanity symmetry to be used in error paths. The semantics of > device_add() and device_put() on failure are long established, don't > invent new behavior for auxiliary_device_add() and > auxiliary_device_put() / put_device(). All stated above is my opinion, it can be different from yours. Thanks