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=-10.5 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, 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 704C0C433E7 for ; Fri, 9 Oct 2020 19:40:15 +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 5516622282 for ; Fri, 9 Oct 2020 19:40:12 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="nrFo0oxA" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5516622282 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com 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 707BB1671; Fri, 9 Oct 2020 21:39:20 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 707BB1671 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1602272410; bh=T/kZdTTyCl5F6WH4TRCpMOeELXqybEEoqGLFzgadyMg=; h=Subject:To:References:From:Date:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=nrFo0oxAXnADPyR6BfipIWLC2ldGR6eFArCAlqne+k77VZC8+8oHi4z58iZ++04LW W0O9Ube9H5vWZGmONNhxhQ8rzFhcOeijds2l7saFocrVdKC/H3M3i+nPfRIT78P0j/ RhJ2mewQfQUbfk/Pu5gopop0/NosTt3dH2eMEwhs= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id D469CF8014D; Fri, 9 Oct 2020 21:39:19 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 8DF08F8015B; Fri, 9 Oct 2020 21:39:17 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id B6273F800BF for ; Fri, 9 Oct 2020 21:39:09 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz B6273F800BF IronPort-SDR: tKYzSvDqe5nbDXvyAAj12xcrHArcuIPTa/VZnCQ9dIjThW6TOq7Y9TkNmLpaDgIItX0ESfsJqK Zpaaz2OEAlzg== X-IronPort-AV: E=McAfee;i="6000,8403,9769"; a="165591059" X-IronPort-AV: E=Sophos;i="5.77,355,1596524400"; d="scan'208";a="165591059" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Oct 2020 12:39:05 -0700 IronPort-SDR: uMWCl/z89ewPADQDqDsvE8uTFL+LonNGnd79CWADG9OCdGywjmbO0qu5ER6L8tWeAIcomXKybm iE+SSrwwwBIg== X-IronPort-AV: E=Sophos;i="5.77,355,1596524400"; d="scan'208";a="462298616" Received: from dnittama-mobl.amr.corp.intel.com (HELO [10.212.225.198]) ([10.212.225.198]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Oct 2020 12:39:03 -0700 Subject: Re: [PATCH v2 1/6] Add ancillary bus support To: Dan Williams References: <20201005182446.977325-1-david.m.ertman@intel.com> <20201005182446.977325-2-david.m.ertman@intel.com> <20201006172317.GN1874917@unreal> <7dbbc51c-2cbd-a7c5-69de-76f190f1d130@linux.intel.com> From: Pierre-Louis Bossart Message-ID: Date: Fri, 9 Oct 2020 14:39:02 -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 Cc: "alsa-devel@alsa-project.org" , "kuba@kernel.org" , "parav@mellanox.com" , Leon Romanovsky , "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" , "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" >>>>>> + >>>>>> + ancildrv->driver.owner = owner; >>>>>> + ancildrv->driver.bus = &ancillary_bus_type; >>>>>> + ancildrv->driver.probe = ancillary_probe_driver; >>>>>> + ancildrv->driver.remove = ancillary_remove_driver; >>>>>> + ancildrv->driver.shutdown = ancillary_shutdown_driver; >>>>>> + >>>>> >>>>> I think that this part is wrong, probe/remove/shutdown functions should >>>>> come from ancillary_bus_type. >>>> >>>> From checking other usage cases, this is the model that is used for probe, remove, >>>> and shutdown in drivers. Here is the example from Greybus. >>>> >>>> int greybus_register_driver(struct greybus_driver *driver, struct module *owner, >>>> const char *mod_name) >>>> { >>>> int retval; >>>> >>>> if (greybus_disabled()) >>>> return -ENODEV; >>>> >>>> driver->driver.bus = &greybus_bus_type; >>>> driver->driver.name = driver->name; >>>> driver->driver.probe = greybus_probe; >>>> driver->driver.remove = greybus_remove; >>>> driver->driver.owner = owner; >>>> driver->driver.mod_name = mod_name; >>>> >>>> >>>>> You are overwriting private device_driver >>>>> callbacks that makes impossible to make container_of of ancillary_driver >>>>> to chain operations. >>>>> >>>> >>>> I am sorry, you lost me here. you cannot perform container_of on the callbacks >>>> because they are pointers, but if you are referring to going from device_driver >>>> to the auxiliary_driver, that is what happens in auxiliary_probe_driver in the >>>> very beginning. >>>> >>>> static int auxiliary_probe_driver(struct device *dev) >>>> 145 { >>>> 146 struct auxiliary_driver *auxdrv = to_auxiliary_drv(dev->driver); >>>> 147 struct auxiliary_device *auxdev = to_auxiliary_dev(dev); >>>> >>>> Did I miss your meaning? >>> >>> I think you're misunderstanding the cases when the >>> bus_type.{probe,remove} is used vs the driver.{probe,remove} >>> callbacks. The bus_type callbacks are to implement a pattern where the >>> 'probe' and 'remove' method are typed to the bus device type. For >>> example 'struct pci_dev *' instead of raw 'struct device *'. See this >>> conversion of dax bus as an example of going from raw 'struct device >>> *' typed probe/remove to dax-device typed probe/remove: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=75797273189d >> >> Thanks Dan for the reference, very useful. This doesn't look like a a >> big change to implement, just wondering about the benefits and >> drawbacks, if any? I am a bit confused here. >> >> First, was the initial pattern wrong as Leon asserted it? Such code >> exists in multiple examples in the kernel and there's nothing preventing >> the use of container_of that I can think of. Put differently, if this >> code was wrong then there are other existing buses that need to be updated. >> >> Second, what additional functionality does this move from driver to >> bus_type provide? The commit reference just states 'In preparation for >> introducing seed devices the dax-bus core needs to be able to intercept >> ->probe() and ->remove() operations", but that doesn't really help me >> figure out what 'intercept' means. Would you mind elaborating? >> >> And last, the existing probe function does calls dev_pm_domain_attach(): >> >> 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); >> >> So the need to access the raw device still exists. Is this still legit >> if the probe() is moved to the bus_type structure? > > Sure, of course. > >> >> I have no objection to this change if it preserves the same >> functionality and possibly extends it, just wanted to better understand >> the reasons for the change and in which cases the bus probe() makes more >> sense than a driver probe(). >> >> Thanks for enlightening the rest of us! > > tl;dr: The ops set by the device driver should never be overwritten by > the bus, the bus can only wrap them in its own ops. > > The reason to use the bus_type is because the bus type is the only > agent that knows both how to convert a raw 'struct device *' to the > bus's native type, and how to convert a raw 'struct device_driver *' > to the bus's native driver type. The driver core does: > > if (dev->bus->probe) { > ret = dev->bus->probe(dev); > } else if (drv->probe) { > ret = drv->probe(dev); > } > > ...so that the bus has the first priority for probing a device / > wrapping the native driver ops. The bus ->probe, in addition to > optionally performing some bus specific pre-work, lets the bus upcast > the device to bus-native type. > > The bus also knows the types of drivers that will be registered to it, > so the bus can upcast the dev->driver to the native type. > > So with bus_type based driver ops driver authors can do: > > struct auxiliary_device_driver auxdrv { > .probe = fn(struct auxiliary_device *, ) > }; > > auxiliary_driver_register(&auxdrv); <-- the core code can hide bus details > > Without bus_type the driver author would need to do: > > struct auxiliary_device_driver auxdrv { > .drv = { > .probe = fn(struct device *), <-- no opportunity for bus > specific probe args > .bus = &auxilary_bus_type, <-- unnecessary export to device drivers > }, > }; > > driver_register(&auxdrv.drv) Thanks Dan, I appreciate the explanation. I guess the misunderstanding on my side was that in practice the drivers only declare a probe at the auxiliary level: struct auxiliary_device_driver auxdrv { .drv = { .name = "my driver" <<< .probe not set here. } .probe = fn(struct auxiliary_device *, int id), } It looks indeed cleaner with your suggestion. DaveE and I were talking about this moments ago and made the change, will be testing later today. Again thanks for the write-up and have a nice week-end.