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=-8.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable 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 0FFDBC4727D for ; Tue, 6 Oct 2020 15:27:58 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A84AC206F7 for ; Tue, 6 Oct 2020 15:27:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726105AbgJFP15 (ORCPT ); Tue, 6 Oct 2020 11:27:57 -0400 Received: from mga12.intel.com ([192.55.52.136]:33659 "EHLO mga12.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725902AbgJFP14 (ORCPT ); Tue, 6 Oct 2020 11:27:56 -0400 IronPort-SDR: QeDM/ScQrcW2A+HZNRbbrBs6431VhL8Qeh83rsOSjF0a8XrNi4Ch8LTEXdn5yOjNhjejNGAJ5r FKugDPss33HA== X-IronPort-AV: E=McAfee;i="6000,8403,9765"; a="143926600" X-IronPort-AV: E=Sophos;i="5.77,343,1596524400"; d="scan'208";a="143926600" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Oct 2020 08:18:11 -0700 IronPort-SDR: jyseSU0pg3l97W6nWc2ghmP+1PlhkgiTtjpSWzlQToieuZXfHvP5XeXyhpnzuq+dfIx1uY6gyr n6Y0n91f/yMQ== X-IronPort-AV: E=Sophos;i="5.77,343,1596524400"; d="scan'208";a="460840058" Received: from mforsman-mobl.amr.corp.intel.com (HELO [10.212.97.68]) ([10.212.97.68]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Oct 2020 08:18:08 -0700 Subject: Re: [PATCH v2 1/6] Add ancillary bus support To: Leon Romanovsky , Dave Ertman 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, jgg@nvidia.com, gregkh@linuxfoundation.org, kuba@kernel.org, dan.j.williams@intel.com, shiraz.saleem@intel.com, davem@davemloft.net, kiran.patil@intel.com References: <20201005182446.977325-1-david.m.ertman@intel.com> <20201005182446.977325-2-david.m.ertman@intel.com> <20201006071821.GI1874917@unreal> From: Pierre-Louis Bossart Message-ID: Date: Tue, 6 Oct 2020 10:18:07 -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: <20201006071821.GI1874917@unreal> 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 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. [...] >> + 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? [...] >> +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. Both points have merits IMHO. Do we want wrappers for everything and completely hide the low-level device? > >> +EXPORT_SYMBOL_GPL(__ancillary_device_add); >> + >> +static int ancillary_probe_driver(struct device *dev) >> +{ >> + struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver); >> + struct ancillary_device *ancildev = to_ancillary_dev(dev); >> + int ret; >> + >> + ret = dev_pm_domain_attach(dev, true); >> + if (ret) { >> + dev_warn(dev, "Failed to attach to PM Domain : %d\n", ret); >> + return ret; >> + } >> + >> + ret = ancildrv->probe(ancildev, ancillary_match_id(ancildrv->id_table, ancildev)); > > I don't think that you need to call ->probe() if ancillary_match_id() > returned NULL and probably that check should be done before > dev_pm_domain_attach(). we'll look into this. > >> + if (ret) >> + dev_pm_domain_detach(dev, true); >> + >> + return ret; >> +} >> + >> +static int ancillary_remove_driver(struct device *dev) >> +{ >> + struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver); >> + struct ancillary_device *ancildev = to_ancillary_dev(dev); >> + int ret; >> + >> + ret = ancildrv->remove(ancildev); >> + dev_pm_domain_detach(dev, true); >> + >> + return ret; > > You returned an error to user and detached from PM, what will user do > with this information? Should user ignore it? retry? That comment was also provided in earlier reviews. In practice the error is typically ignored so there was a suggestion to move the return type to void, that could be done if this was desired by the majority. [...] >> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h >> index 5b08a473cdba..7d596dc30833 100644 >> --- a/include/linux/mod_devicetable.h >> +++ b/include/linux/mod_devicetable.h >> @@ -838,4 +838,12 @@ struct mhi_device_id { >> kernel_ulong_t driver_data; >> }; >> >> +#define ANCILLARY_NAME_SIZE 32 >> +#define ANCILLARY_MODULE_PREFIX "ancillary:" >> + >> +struct ancillary_device_id { >> + char name[ANCILLARY_NAME_SIZE]; > > I hope that this be enough. Are you suggesting a different value to allow for a longer string?