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.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 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 964F2C432C0 for ; Mon, 18 Nov 2019 07:48:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 582EC206F4 for ; Mon, 18 Nov 2019 07:48:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1574063320; bh=C3o+zsML0lPQRe4p26aKJrwfilU10RU+U7Kxiw+59G8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=ofD4pT/r8gLxqwgXgbRWW4OgWDuhIVBfGIuU8gUjHE06DgtgD6ushXGwUmoEruxch Tm996HhsULiZPnjKF2OtLYG1d3vB0o3j2vBghvFz905V4b4vJiAB1uFqoaZ59qD/+q TY05ilguX4q6b7byxUy+CsXvN7BLs0AjgmbUQ3dU= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726536AbfKRHsj (ORCPT ); Mon, 18 Nov 2019 02:48:39 -0500 Received: from mail.kernel.org ([198.145.29.99]:33694 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726283AbfKRHsj (ORCPT ); Mon, 18 Nov 2019 02:48:39 -0500 Received: from localhost (83-86-89-107.cable.dynamic.v4.ziggo.nl [83.86.89.107]) (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 0240020409; Mon, 18 Nov 2019 07:48:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1574063317; bh=C3o+zsML0lPQRe4p26aKJrwfilU10RU+U7Kxiw+59G8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=nQABsS/wWaA9gfCfcktGAARoEKRc0ShaS4ZANqjTXMvFGmDbnezgAG1uIuaEq85Is BumzQVe/+/iZTONgMXjdBy+9VoOHyanGSUtq9yN69jR4aRZqQrWKGuZpV4BttQEIq5 XG9lu0L6Cbf7F4MaqkeO+MiORffV/GPL4/7Koa1A= Date: Mon, 18 Nov 2019 08:48:34 +0100 From: Greg KH To: Jeff Kirsher Cc: davem@davemloft.net, Dave Ertman , netdev@vger.kernel.org, linux-rdma@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com, jgg@ziepe.ca, parav@mellanox.com, Kiran Patil Subject: Re: [net-next v2 1/1] virtual-bus: Implementation of Virtual Bus Message-ID: <20191118074834.GA130507@kroah.com> References: <20191115223355.1277139-1-jeffrey.t.kirsher@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191115223355.1277139-1-jeffrey.t.kirsher@intel.com> User-Agent: Mutt/1.12.2 (2019-09-21) Sender: linux-rdma-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org On Fri, Nov 15, 2019 at 02:33:55PM -0800, Jeff Kirsher wrote: > From: Dave Ertman > > This is the initial implementation of the Virtual Bus, > virtbus_device and virtbus_driver. The virtual bus is > a software based bus intended to support lightweight > devices and drivers and provide matching between them > and probing of the registered drivers. > > The primary purpose of the virual bus is to provide > matching services and to pass the data pointer > contained in the virtbus_device to the virtbus_driver > during its probe call. This will allow two separate > kernel objects to match up and start communication. > > The bus will support probe/remove shutdown and > suspend/resume callbacks. > > Kconfig and Makefile alterations are included > > Signed-off-by: Dave Ertman > Signed-off-by: Kiran Patil > Signed-off-by: Jeff Kirsher > --- > v2: Cleaned up the virtual bus interface based on feedback from Greg KH > and provided a test driver and test virtual bus device as an example > of how to implement the virtual bus. > > Documentation/driver-api/virtual_bus.rst | 76 ++++ > drivers/bus/Kconfig | 14 + > drivers/bus/Makefile | 1 + > drivers/bus/virtual_bus.c | 326 ++++++++++++++++++ > include/linux/virtual_bus.h | 55 +++ > .../virtual_bus/virtual_bus_dev/Makefile | 7 + > .../virtual_bus_dev/virtual_bus_dev.c | 67 ++++ > .../virtual_bus/virtual_bus_drv/Makefile | 7 + > .../virtual_bus_drv/virtual_bus_drv.c | 101 ++++++ > 9 files changed, 654 insertions(+) > create mode 100644 Documentation/driver-api/virtual_bus.rst > create mode 100644 drivers/bus/virtual_bus.c > create mode 100644 include/linux/virtual_bus.h > create mode 100644 tools/testing/selftests/virtual_bus/virtual_bus_dev/Makefile > create mode 100644 tools/testing/selftests/virtual_bus/virtual_bus_dev/virtual_bus_dev.c > create mode 100644 tools/testing/selftests/virtual_bus/virtual_bus_drv/Makefile > create mode 100644 tools/testing/selftests/virtual_bus/virtual_bus_drv/virtual_bus_drv.c > > diff --git a/Documentation/driver-api/virtual_bus.rst b/Documentation/driver-api/virtual_bus.rst > new file mode 100644 > index 000000000000..970e06267284 > --- /dev/null > +++ b/Documentation/driver-api/virtual_bus.rst > @@ -0,0 +1,76 @@ > +=============================== > +Virtual Bus Devices and Drivers > +=============================== > + > +See for the models for virtbus_device and virtbus_driver. > +This bus is meant to be a lightweight software based bus to attach generic > +devices and drivers to so that a chunk of data can be passed between them. > + > +One use case example is an rdma driver needing to connect with several > +different types of PCI LAN devices to be able to request resources from > +them (queue sets). Each LAN driver that supports rdma will register a > +virtbus_device on the virtual bus for each physical function. The rdma > +driver will register as a virtbus_driver on the virtual bus to be > +matched up with multiple virtbus_devices and receive a pointer to a > +struct containing the callbacks that the PCI LAN drivers support for > +registering with them. > + > +Sections in this document: > + Virtbus devices > + Virtbus drivers > + Device Enumeration > + Device naming and driver binding > + Virtual Bus API entry points > + > +Virtbus devices > +~~~~~~~~~~~~~~~ > +Virtbus_devices are lightweight objects that support the minimal device > +functionality. Devices will accept a name, and then an automatically > +generated index is concatenated onto it for the virtbus_device->name. > + > +The memory backing the "void *data" element of the virtbus_device is > +expected to be allocated and freed outside the context of the bus > +operations. This memory is also expected to remain viable for the > +duration of the time that the virtbus_device is registered to the > +virtual bus. (e.g. from before the virtbus_dev_register until after > +the paired virtbus_dev_unregister). > + > +The provided API for virtbus_dev_alloc is an efficient way of allocating > +the memory for the virtbus_device (except for the data element) and > +automatically freeing it when the device is removed from the bus. > + > +Virtbus drivers > +~~~~~~~~~~~~~~~ > +Virtbus drivers register with the virtual bus to be matched with virtbus > +devices. They expect to be registered with a probe and remove callback, > +and also support shutdown, suspend, and resume callbacks. They otherwise > +follow the standard driver behavior of having discovery and enumeration > +handled in the bus infrastructure. > + > +Virtbus drivers register themselves with the API entry point virtbus_drv_reg > +and unregister with virtbus_drv_unreg. > + > +Device Enumeration > +~~~~~~~~~~~~~~~~~~ > +Enumeration is handled automatically by the bus infrastructure via the > +ida_simple methods. > + > +Device naming and driver binding > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > +The virtbus_device.dev.name is the canonical name for the device. It is > +built from two other parts: > + > + - virtbus_device.name (also used for matching). > + - virtbus_device.id (generated automatically from ida_simple calls) > + > +This allows for multiple virtbus_devices with the same name, which will all > +be matched to the same virtbus_driver. Driver binding is performed by the > +driver core, invoking driver probe() after finding a match between device and driver. > + > +Virtual Bus API entry points > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > +struct virtbus_device *virtbus_dev_alloc(const char *name, void *data) > +int virtbus_dev_register(struct virtbus_device *vdev) > +void virtbus_dev_unregister(struct virtbus_device *vdev) > +int virtbus_drv_register(struct virtbus_driver *vdrv, struct module *owner) > +void virtbus_drv_unregister(struct virtbus_driver *vdrv) > diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig > index 6b331061d34b..30cef35b0c30 100644 > --- a/drivers/bus/Kconfig > +++ b/drivers/bus/Kconfig > @@ -193,4 +193,18 @@ config DA8XX_MSTPRI > > source "drivers/bus/fsl-mc/Kconfig" > > +config VIRTUAL_BUS > + tristate "lightweight Virtual Bus" > + depends on PM > + help > + Provides a lightweight bus for virtbus_devices to be added to it > + and virtbus_drivers to be registered on it. Will create a match > + between the driver and device, then call the driver's probe with > + the virtbus_device's struct (including a pointer for data). > + One example is the irdma driver needing to connect with various > + PCI LAN drivers to request resources (queues) to be able to perform > + its function. The data in the virtbus_device created by the > + PCI LAN driver is a set of ops (function pointers) for the irdma > + driver to use to register and communicate with the PCI LAN driver. > + > endmenu > diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile > index 16b43d3468c6..0b0ba53cbe5b 100644 > --- a/drivers/bus/Makefile > +++ b/drivers/bus/Makefile > @@ -33,3 +33,4 @@ obj-$(CONFIG_UNIPHIER_SYSTEM_BUS) += uniphier-system-bus.o > obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o > > obj-$(CONFIG_DA8XX_MSTPRI) += da8xx-mstpri.o > +obj-$(CONFIG_VIRTUAL_BUS) += virtual_bus.o > diff --git a/drivers/bus/virtual_bus.c b/drivers/bus/virtual_bus.c > new file mode 100644 > index 000000000000..c6eab1658391 > --- /dev/null > +++ b/drivers/bus/virtual_bus.c > @@ -0,0 +1,326 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * virtual_bus.c - lightweight software based bus for virtual devices > + * > + * Copyright (c) 2019-20 Intel Corporation > + * > + * Please see Documentation/driver-api/virtual_bus.rst for > + * more information > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("Lightweight Virtual Bus"); > +MODULE_AUTHOR("David Ertman "); > +MODULE_AUTHOR("Kiran Patil "); > + > +static DEFINE_IDA(virtbus_dev_ida); Do you ever clean up this when unloaded? I didn't see that happening but I might have missed it. > + > +static const > +struct virtbus_dev_id *virtbus_match_id(const struct virtbus_dev_id *id, > + struct virtbus_device *vdev) > +{ > + while (id->name[0]) { > + if (!strcmp(vdev->name, id->name)) { > + vdev->dev_id = id; Why are you changing/setting the id? > + return id; > + } > + id++; > + } > + return NULL; > +} > + > +#define to_virtbus_dev(x) (container_of((x), struct virtbus_device, dev)) > +#define to_virtbus_drv(x) (container_of((x), struct virtbus_driver, \ > + driver)) > + > +/** > + * virtbus_match - bind virtbus device to virtbus driver > + * @dev: device > + * @drv: driver > + * > + * Virtbus device IDs are always in "." format. > + * Instances are automatically selected through an ida_simple_get so > + * are positive integers. Names are taken from the device name field. > + * Driver IDs are simple . Need to extract the name from the > + * Virtual Device compare to name of the driver. > + */ > +static int virtbus_match(struct device *dev, struct device_driver *drv) > +{ > + struct virtbus_driver *vdrv = to_virtbus_drv(drv); > + struct virtbus_device *vdev = to_virtbus_dev(dev); > + > + if (vdrv->id_table) > + return virtbus_match_id(vdrv->id_table, vdev) != NULL; > + > + return !strcmp(vdev->name, drv->name); > +} > + > +/** > + * virtbus_probe - call probe of the virtbus_drv > + * @dev: device struct > + */ > +static int virtbus_probe(struct device *dev) > +{ > + if (dev->driver->probe) > + return dev->driver->probe(dev); > + > + return 0; > +} > + > +static int virtbus_remove(struct device *dev) > +{ > + if (dev->driver->remove) > + return dev->driver->remove(dev); > + > + return 0; > +} > + > +static void virtbus_shutdown(struct device *dev) > +{ > + if (dev->driver->shutdown) > + dev->driver->shutdown(dev); > +} > + > +static int virtbus_suspend(struct device *dev, pm_message_t state) > +{ > + if (dev->driver->suspend) > + return dev->driver->suspend(dev, state); > + > + return 0; > +} > + > +static int virtbus_resume(struct device *dev) > +{ > + if (dev->driver->resume) > + return dev->driver->resume(dev); > + > + return 0; > +} > + > +struct bus_type virtual_bus_type = { > + .name = "virtbus", > + .match = virtbus_match, > + .probe = virtbus_probe, > + .remove = virtbus_remove, > + .shutdown = virtbus_shutdown, > + .suspend = virtbus_suspend, > + .resume = virtbus_resume, > +}; > + > +/** > + * virtbus_dev_register - add a virtual bus device > + * @vdev: virtual bus device to add > + */ > +int virtbus_dev_register(struct virtbus_device *vdev) > +{ > + int ret; > + > + if (!vdev) > + return -EINVAL; > + > + device_initialize(&vdev->dev); > + > + vdev->dev.bus = &virtual_bus_type; > + /* All device IDs are automatically allocated */ > + ret = ida_simple_get(&virtbus_dev_ida, 0, 0, GFP_KERNEL); > + if (ret < 0) > + return ret; > + > + vdev->id = ret; > + dev_set_name(&vdev->dev, "%s.%d", vdev->name, vdev->id); > + > + dev_dbg(&vdev->dev, "Registering VirtBus device '%s'\n", > + dev_name(&vdev->dev)); > + > + ret = device_add(&vdev->dev); > + if (!ret) > + return ret; This logic has tripped me up multiple times, it's an anti-pattern. Please do: if (ret) goto device_add_error; return 0; device_add_error: ... > + > + /* Error adding virtual device */ > + device_del(&vdev->dev); > + ida_simple_remove(&virtbus_dev_ida, vdev->id); > + vdev->id = VIRTBUS_DEVID_NONE; > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(virtbus_dev_register); > + > +/** > + * virtbus_dev_unregister - remove a virtual bus device > + * vdev: virtual bus device we are removing > + */ > +void virtbus_dev_unregister(struct virtbus_device *vdev) > +{ > + if (!IS_ERR_OR_NULL(vdev)) { > + device_del(&vdev->dev); > + > + ida_simple_remove(&virtbus_dev_ida, vdev->id); > + vdev->id = VIRTBUS_DEVID_NONE; Why set the id? What will care/check this? > + } > +} > +EXPORT_SYMBOL_GPL(virtbus_dev_unregister); > + > +struct virtbus_object { > + struct virtbus_device vdev; > + char name[]; > +}; Why not use the name in the device structure? thanks, greg k-h