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=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 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 48990C4727E for ; Wed, 7 Oct 2020 21:50:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0144420872 for ; Wed, 7 Oct 2020 21:50:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728736AbgJGVuA (ORCPT ); Wed, 7 Oct 2020 17:50:00 -0400 Received: from mga17.intel.com ([192.55.52.151]:45147 "EHLO mga17.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726105AbgJGVt7 (ORCPT ); Wed, 7 Oct 2020 17:49:59 -0400 IronPort-SDR: OdFSJ2DxI95zAkqRp6cXAGDRx1p2Z5Hyv3SL0peB/UusZpeLvuUqDVAKY8EX2VhQfbuNpVs6FZ ZMr/EifFM1ng== X-IronPort-AV: E=McAfee;i="6000,8403,9767"; a="145043843" X-IronPort-AV: E=Sophos;i="5.77,348,1596524400"; d="scan'208";a="145043843" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Oct 2020 14:49:59 -0700 IronPort-SDR: JF8stz/YrpGHkXZbc81aUFakPjvFFyGYpS5ISnNCXHa8o5WXXwAc/sefqSrxK31t2jIhgx8HFd Topz2qV89xOQ== X-IronPort-AV: E=Sophos;i="5.77,348,1596524400"; d="scan'208";a="528182559" Received: from unknown (HELO [10.135.3.161]) ([10.135.3.161]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Oct 2020 14:49:57 -0700 Subject: Re: [PATCH v2 1/6] Add ancillary bus support 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" References: <20201005182446.977325-1-david.m.ertman@intel.com> <20201005182446.977325-2-david.m.ertman@intel.com> <20201006071821.GI1874917@unreal> <20201006170241.GM1874917@unreal> <20201007192610.GD3964015@unreal> From: Pierre-Louis Bossart Message-ID: Date: Wed, 7 Oct 2020 16:49:57 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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.