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.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 53F00C4727E for ; Thu, 1 Oct 2020 19:32:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id EC93220848 for ; Thu, 1 Oct 2020 19:32:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1601580738; bh=mXlByQDgsBBs1Axpxf6TJecUhVnUqdpDS/jr3iSqSME=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=BAdZQvAvuPpUH0UIwr3NQ8ignrTipndFtYACP+xjecietmGFto2B/2QSGqQmKJ4Om U5q1qjytI9VytRP7KErjotyF4krgq4pVgJcbpYlk4Nt5iTohIcQZZ+E9cL1jtX3yZt DGTaBnNCetJ5jWymKd+C/wOwkmHWPNMsCUO5+ZQ8= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730064AbgJATcR (ORCPT ); Thu, 1 Oct 2020 15:32:17 -0400 Received: from mail.kernel.org ([198.145.29.99]:56336 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729990AbgJATcR (ORCPT ); Thu, 1 Oct 2020 15:32:17 -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 1FE6020759; Thu, 1 Oct 2020 19:32:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1601580736; bh=mXlByQDgsBBs1Axpxf6TJecUhVnUqdpDS/jr3iSqSME=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=znTgB5/NBxXdrTa0EqptHeKLcDkNHDxhUTAoHflU6uBnTU6Owl8E43S43lcVO0aAA xP5uKE+3rN2nreUd4pNjB7goO0xksK3BpGArXjUPb8zs7qm7XNccujWOyFtp5WfEdy VEnESKDnbb1WtNFkAq6wC5y0cDw3vIdjZBL5iLGY= Date: Thu, 1 Oct 2020 22:32:11 +0300 From: Leon Romanovsky To: Parav Pandit Cc: "Ertman, David M" , "linux-rdma@vger.kernel.org" Subject: Re: [PATCH 1/6] Add ancillary bus support Message-ID: <20201001193211.GX3094@unreal> References: <20201001050534.890666-1-david.m.ertman@intel.com> <20201001050534.890666-2-david.m.ertman@intel.com> <20201001083229.GV3094@unreal> <20201001174025.GW3094@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: linux-rdma@vger.kernel.org On Thu, Oct 01, 2020 at 06:29:22PM +0000, Parav Pandit wrote: > > > > From: Leon Romanovsky > > Sent: Thursday, October 1, 2020 11:10 PM > > > > On Thu, Oct 01, 2020 at 05:20:35PM +0000, Ertman, David M wrote: > > > > -----Original Message----- > > > > From: Leon Romanovsky > > > > Sent: Thursday, October 1, 2020 1:32 AM > > > > To: Ertman, David M > > > > Cc: linux-rdma@vger.kernel.org > > > > Subject: Re: [PATCH 1/6] Add ancillary bus support > > > > > > > > On Wed, Sep 30, 2020 at 10:05:29PM -0700, Dave Ertman wrote: > > > > > 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. > > > > > > > > > > The bus supports probe/remove shutdown and suspend/resume > > callbacks. > > > > > Each ancillary_device has a unique string based id; driver binds > > > > > to an ancillary_device based on this id through the bus. > > > > > > > > > > Co-developed-by: Kiran Patil > > > > > Signed-off-by: Kiran Patil > > > > > Co-developed-by: Ranjani Sridharan > > > > > > > > > > Signed-off-by: Ranjani Sridharan > > > > > > > > > > Co-developed-by: Fred Oh > > > > > Signed-off-by: Fred Oh > > > > > Reviewed-by: Pierre-Louis Bossart > > > > > > > > > > Reviewed-by: Shiraz Saleem > > > > > Reviewed-by: Parav Pandit > > > > > Reviewed-by: Dan Williams > > > > > Signed-off-by: Dave Ertman > > > > > --- > > > > > Documentation/driver-api/ancillary_bus.rst | 230 > > > > +++++++++++++++++++++ > > > > > Documentation/driver-api/index.rst | 1 + > > > > > drivers/bus/Kconfig | 3 + > > > > > drivers/bus/Makefile | 3 + > > > > > drivers/bus/ancillary.c | 191 +++++++++++++++++ > > > > > include/linux/ancillary_bus.h | 58 ++++++ > > > > > include/linux/mod_devicetable.h | 8 + > > > > > scripts/mod/devicetable-offsets.c | 3 + > > > > > scripts/mod/file2alias.c | 8 + > > > > > 9 files changed, 505 insertions(+) create mode 100644 > > > > > Documentation/driver-api/ancillary_bus.rst > > > > > create mode 100644 drivers/bus/ancillary.c create mode 100644 > > > > > include/linux/ancillary_bus.h > > > > > > > > > > diff --git a/Documentation/driver-api/ancillary_bus.rst > > > > b/Documentation/driver-api/ancillary_bus.rst > > > > > new file mode 100644 > > > > > index 000000000000..0a11979aa927 > > > > > --- /dev/null > > > > > +++ b/Documentation/driver-api/ancillary_bus.rst > > > > > @@ -0,0 +1,230 @@ > > > > > +.. SPDX-License-Identifier: GPL-2.0-only > > > > > + > > > > > +============= > > > > > +Ancillary Bus > > > > > +============= > > > > > + > > > > > +In some subsystems, the functionality of the core device > > > > (PCI/ACPI/other) is > > > > > +too complex for a single device to be managed as a monolithic > > > > > +block or a > > > > part of > > > > > +the functionality needs to be exposed to a different subsystem. > > > > > +Splitting > > > > the > > > > > +functionality into smaller orthogonal devices would make it > > > > > +easier to > > > > manage > > > > > +data, power management and domain-specific interaction with the > > > > hardware. A key > > > > > +requirement for such a split is that there is no dependency on a > > > > > +physical > > > > bus, > > > > > +device, register accesses or regmap support. These individual > > > > > +devices split > > > > from > > > > > +the core cannot live on the platform bus as they are not physical > > > > > +devices > > > > that > > > > > +are controlled by DT/ACPI. The same argument applies for not > > > > > +using MFD > > > > in this > > > > > +scenario as MFD relies on individual function devices being > > > > > +physical > > > > devices > > > > > +that are DT enumerated. > > > > > + > > > > > +An example for this kind of requirement is the audio subsystem > > > > > +where a > > > > single > > > > > +IP is handling multiple entities such as HDMI, Soundwire, local > > > > > +devices > > > > such as > > > > > +mics/speakers etc. The split for the core's functionality can be > > > > > +arbitrary or be defined by the DSP firmware topology and include > > > > > +hooks for > > > > test/debug. This > > > > > +allows for the audio core device to be minimal and focused on > > > > > +hardware- > > > > specific > > > > > +control and communication. > > > > > + > > > > > +The ancillary bus is intended to be minimal, generic and avoid > > > > > +domain- > > > > specific > > > > > +assumptions. Each ancillary_device represents a part of its > > > > > +parent functionality. The generic behavior can be extended and > > > > > +specialized as > > > > needed > > > > > +by encapsulating an ancillary_device within other domain-specific > > > > structures and > > > > > +the use of .ops callbacks. Devices on the ancillary bus do not > > > > > +share any structures and the use of a communication channel with > > > > > +the parent is domain-specific. > > > > > + > > > > > +When Should the Ancillary Bus Be Used > > > > > +===================================== > > > > > + > > > > > +The ancillary bus is to be used when a driver and one or more > > > > > +kernel > > > > modules, > > > > > +who share a common header file with the driver, need a mechanism > > > > > +to > > > > connect and > > > > > +provide access to a shared object allocated by the > > > > > +ancillary_device's registering driver. The registering driver > > > > > +for the ancillary_device(s) and > > > > the > > > > > +kernel module(s) registering ancillary_drivers can be from the > > > > > +same > > > > subsystem, > > > > > +or from multiple subsystems. > > > > > + > > > > > +The emphasis here is on a common generic interface that keeps > > > > subsystem > > > > > +customization out of the bus infrastructure. > > > > > + > > > > > +One example could be a multi-port PCI network device that is > > > > > +rdma- > > > > capable and > > > > > +needs to export this functionality and attach to an rdma driver > > > > > +in another subsystem. The PCI driver will allocate and register > > > > > +an ancillary_device for each physical function on the NIC. The > > > > > +rdma driver will register an ancillary_driver that will be > > > > > +matched with and probed for each of these ancillary_devices. > > > > > +This will give the rdma driver access to the shared > > > > data/ops > > > > > +in the PCI drivers shared object to establish a connection with > > > > > +the PCI > > > > driver. > > > > > + > > > > > +Another use case is for the a PCI device to be split out into > > > > > +multiple sub functions. For each sub function an > > > > > +ancillary_device will be created. A PCI sub function driver will > > > > > +bind to such devices that will create its own one or more class > > > > > +devices. A PCI sub function ancillary device will likely be > > > > > +contained in a struct with additional attributes such as user > > > > > +defined sub function number and optional attributes such as > > > > > +resources and a link to > > > > the > > > > > +parent device. These attributes could be used by systemd/udev; > > > > > +and > > > > hence should > > > > > +be initialized before a driver binds to an ancillary_device. > > > > > + > > > > > +Ancillary Device > > > > > +================ > > > > > + > > > > > +An ancillary_device is created and registered to represent a part > > > > > +of its > > > > parent > > > > > +device's functionality. It is given a name that, combined with > > > > > +the > > > > registering > > > > > +drivers KBUILD_MODNAME, creates a match_name that is used for > > > > > +driver > > > > binding, > > > > > +and an id that combined with the match_name provide a unique name > > > > > +to > > > > register > > > > > +with the bus subsystem. > > > > > + > > > > > +Registering an ancillary_device is a two-step process. First you > > > > > +must call ancillary_device_initialize(), which will check several > > > > > +aspects of the ancillary_device struct and perform a > > > > > +device_initialize(). After this step completes, any error state > > > > > +must have a call to put_device() in its > > > > resolution > > > > > +path. The second step in registering an ancillary_device is to > > > > > +perform a > > > > call > > > > > +to ancillary_device_add(), which will set the name of the device > > > > > +and add > > > > the > > > > > +device to the bus. > > > > > + > > > > > +To unregister an ancillary_device, just a call to > > > > ancillary_device_unregister() > > > > > +is used. This will perform both a device_del() and a put_device(). > > > > > > > > Why did you chose ancillary_device_initialize() and not > > > > ancillary_device_register() to be paired with > > ancillary_device_unregister()? > > > > > > > > Thanks > > > > > > We originally had a single call to ancillary_device_register() that > > > paired with unregister, but there was an ask to separate the register > > > into an initialize and add to make the error condition unwind more > > compartimentalized. > > > > It is correct thing to separate, but I would expect: > > ancillary_device_register() > > ancillary_device_add() > > > device_initialize(), device_add() and device_unregister() is the pattern widely followed in the core. It doesn't mean that I need to agree with that, right? > > > vs. > > ancillary_device_unregister() > > > > It is not a big deal, just curious. > > > > The much more big deal is that I'm required to create 1-to-1 mapping > > between device and driver, and I can't connect all my different modules to > > one xxx_core.pf.y device in N-to-1 mapping. "N" represents different > > protocols (IB, ETH, SCSI) and "1" is one PCI core. > > For one mlx5 (pf/vf/sf) device, there are three class erivers (ib, vdpa, en). > > So there should be one ida allocate per mlx5 device type. > > Static const mlx5_adev_names[MLX5_INTERFACE_PROTOCOL_MAX] = { > "rdma", > "eth", > "vdpa" > }; > > Something like for current mlx5_register_device(), I know it and already implemented it, this is why I'm saying that it is not what I would expect from the implementation. It is wrong create mlx5_core.rdma.1 device that is equal to mlx5_core.eth.1 just to connect our mlx5_ib.ko to it, while documentation explains about creating single PCI code device and other drivers connect to it. Thanks > mlx5_register_device() > { > id = ida_alloc(0, UINT_MAX, GFP_KERNEL); > > for (i = 0; I < MLX5_INTERFACE_PROTOCOL_MAX; i++) { > adev = kzalloc(sizeof(*adev), GFP_KERNEL); > adev.name = mlx5_adev_names[i]; > adev.id = ret; > adev.dev.parent = mlx5_core_dev->device; > adev->coredev = mlx5_core_dev; > ret = ancillary_device_initialize(&adev); > ret = ancillary_device_register(adev); > } > > This will create 3 ancillary devices for each PCI PF/VF/SF. > mlx5_core.rdma.1 > mlx5_core.eth.1 > mlx5_core.vdpa.1 > > and mlx5_ib driver will do > > ancillary_driver_register() > { > For ID of mlx5_core.rdma. > } > > mlx5_vdpa driver does, > > ancillary_driver_register() > { > For ID of mlx5_core.vdpa > } > > This is uniform for pf/vf/sf for one or more all protocols.