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.8 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 18BA0C4727F for ; Wed, 7 Oct 2020 19:26:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CC71B2085B for ; Wed, 7 Oct 2020 19:26:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1602098777; bh=9Efq9BJM0KiSLLuORpyyMZ3t9JH6nYm17qQk8rQ+PLI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=rvjPKYR4qFbFNZ0nx/4x2J1l3pBCOFly6vhJBP6vtCdVUnDZUGaLUs3/P7rZ9h1os o4rQ+ORlVl9jCXm8kjmPLv9Vw0iggGVZtkkol44NtBvBzmWrosNtk4o+MGHtxWLxcQ SoVfXouxf8LsEa0uvKj4nH8Pm1GP7T9nswE41f3c= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728319AbgJGT0Q (ORCPT ); Wed, 7 Oct 2020 15:26:16 -0400 Received: from mail.kernel.org ([198.145.29.99]:58306 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726671AbgJGT0P (ORCPT ); Wed, 7 Oct 2020 15:26:15 -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 C41192173E; Wed, 7 Oct 2020 19:26:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1602098774; bh=9Efq9BJM0KiSLLuORpyyMZ3t9JH6nYm17qQk8rQ+PLI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=qgUBgA3R7+nTAqG++g8VKyGQRyilQJ6UoRyHiVUvMT4DZpk9MbV2HSNwVFUJak3/I xepI6uJRdeQo5JbXMjMsFZ9HSv/GWk4iAuGOz0HfoKwKw67rYF+T164DG/upF8atkw GdbDbu56q2++rrZ4BGpT0Z++xOfgecjJcuvXKd/E= Date: Wed, 7 Oct 2020 22:26:10 +0300 From: Leon Romanovsky To: "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" , "jgg@nvidia.com" , "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: <20201007192610.GD3964015@unreal> References: <20201005182446.977325-1-david.m.ertman@intel.com> <20201005182446.977325-2-david.m.ertman@intel.com> <20201006071821.GI1874917@unreal> <20201006170241.GM1874917@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: netdev@vger.kernel.org On Wed, Oct 07, 2020 at 06:06:30PM +0000, Ertman, David M wrote: > > -----Original Message----- > > From: Leon Romanovsky > > Sent: Tuesday, October 6, 2020 10:03 AM > > To: Pierre-Louis Bossart > > Cc: 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; > > jgg@nvidia.com; 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 > > > > On Tue, Oct 06, 2020 at 10:18:07AM -0500, Pierre-Louis Bossart wrote: > > > Thanks for the review Leon. > > > > > > > > Add support for the Ancillary Bus, ancillary_device and ancillary_driver. > > > > > It enables drivers to create an ancillary_device and bind an > > > > > ancillary_driver to it. > > > > > > > > I was under impression that this name is going to be changed. > > > > > > It's part of the opens stated in the cover letter. > > > > ok, so what are the variants? > > system bus (sysbus), sbsystem bus (subbus), crossbus ? > > > > > > > > [...] > > > > > > > > + const struct my_driver my_drv = { > > > > > + .ancillary_drv = { > > > > > + .driver = { > > > > > + .name = "myancillarydrv", > > > > > > > > Why do we need to give control over driver name to the driver authors? > > > > It can be problematic if author puts name that already exists. > > > > > > Good point. When I used the ancillary_devices for my own SoundWire test, > > the > > > driver name didn't seem specifically meaningful but needed to be set to > > > something, what mattered was the id_table. Just thinking aloud, maybe we > > can > > > add prefixing with KMOD_BUILD, as we've done already to avoid collisions > > > between device names? > > > > IMHO, it shouldn't be controlled by the drivers at all and need to have > > kernel module name hardwired. Users will use it later for various > > bind/unbind/autoprobe tricks and it will give predictability for them. > > > > > > > > [...] > > > > > > > > +int __ancillary_device_add(struct ancillary_device *ancildev, const > > char *modname) > > > > > +{ > > > > > + struct device *dev = &ancildev->dev; > > > > > + int ret; > > > > > + > > > > > + if (!modname) { > > > > > + pr_err("ancillary device modname is NULL\n"); > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + ret = dev_set_name(dev, "%s.%s.%d", modname, ancildev->name, > > ancildev->id); > > > > > + if (ret) { > > > > > + pr_err("ancillary device dev_set_name failed: %d\n", ret); > > > > > + return ret; > > > > > + } > > > > > + > > > > > + ret = device_add(dev); > > > > > + if (ret) > > > > > + dev_err(dev, "adding ancillary device failed!: %d\n", ret); > > > > > + > > > > > + return ret; > > > > > +} > > > > > > > > Sorry, but this is very strange API that requires users to put > > > > internal call to "dev" that is buried inside "struct ancillary_device". > > > > > > > > For example in your next patch, you write this "put_device(&cdev- > > >ancildev.dev);" > > > > > > > > I'm pretty sure that the amount of bugs in error unwind will be > > > > astonishing, so if you are doing wrappers over core code, better do not > > > > pass complexity to the users. > > > > > > In initial reviews, there was pushback on adding wrappers that don't do > > > anything except for a pointer indirection. > > > > > > Others had concerns that the API wasn't balanced and blurring layers. > > > > Are you talking about internal review or public? > > If it is public, can I get a link to it? > > > > > > > > Both points have merits IMHO. Do we want wrappers for everything and > > > completely hide the low-level device? > > > > 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() return err; } And prefer this flow: ancillary_device_initialize() if (ancillary_device_add()) { ancillary_device_unregister() return err; } In this way, the ancillary users won't need to do non-intuitive put_device(); Thanks