Message-ID: <AM0PR05MB4866CF61828A458319899664D1700@AM0PR05MB4866.eurprd05.prod.outlook.com>
Date:   Fri, 15 Nov 2019 23:25:47 +0000
From:   Parav Pandit <parav@...lanox.com>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     Dave Ertman <david.m.ertman@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "jgg@...pe.ca" <jgg@...pe.ca>, Kiran Patil <kiran.patil@...el.com>
Subject: RE: [net-next v2 1/1] virtual-bus: Implementation of Virtual Bus

Hi Jeff,

> From: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> Sent: Friday, November 15, 2019 4:34 PM
> 
> From: Dave Ertman <david.m.ertman@...el.com>
> 
> 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.
> 
It is fundamental to know that rdma device created by virtbus_driver will be anchored to which bus for an non abusive use.
virtbus or parent pci bus?
I asked this question in v1 version of this patch.

Also since it says - 'to support lightweight devices', documenting that information is critical to avoid ambiguity.

Since for a while I am working on the subbus/subdev_bus/xbus/mdev [1] whatever we want to call it, it overlaps with your comment about 'to support lightweight devices'.
Hence let's make things crystal clear weather the purpose is 'only matching service' or also 'lightweight devices'.
If this is only matching service, lets please remove lightweight devices part..

You additionally need modpost support for id table integration to modifo, modprobe and other tools.
A small patch similar to this one [2] is needed.
Please include in the series.

[..]

> +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;
Matching function shouldn't be modifying 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 "<name>.<instance>" format.
We might have to change this scheme depending on the first question I asked in the email about device anchoring.

> +
> +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,
> +};
Drop the tab alignment.

> +
> +/**
> + * 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;
No need for this check.
Driver shouldn't be called null device registration.

> +
> +	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;
> +
This is bug, once device_initialize() is done, it must do put_device() and follow the release sequence.

> +	vdev->id = ret;
> +	dev_set_name(&vdev->dev, "%s.%d", vdev->name, vdev->id);
> +
> +	dev_dbg(&vdev->dev, "Registering VirtBus device '%s'\n",
I think 'virtbus' naming is better instead of 'VirtBus' all over. We don't do "Pci' in prints etc.

> +		dev_name(&vdev->dev));
> +
> +	ret = device_add(&vdev->dev);
> +	if (!ret)
> +		return ret;
> +
> +	/* 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);
I believe this should be done in the release() because above device_del() may not ensure that all references to the devices are dropped.

> +		vdev->id = VIRTBUS_DEVID_NONE;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(virtbus_dev_unregister);
> +
> +struct virtbus_object {
> +	struct virtbus_device vdev;
> +	char name[];
> +};
> +
This shouldn't be needed once. More below.

> +/**
> + * virtbus_dev_release - Destroy a virtbus device
> + * @vdev: virtual device to release
> + *
> + * Note that the vdev->data which is separately allocated needs to be
> + * separately freed on it own.
> + */
> +static void virtbus_dev_release(struct device *dev) {
> +	struct virtbus_object *vo = container_of(dev, struct virtbus_object,
> +						 vdev.dev);
> +
> +	kfree(vo);
> +}
> +
> +/**
> + * virtbus_dev_alloc - allocate a virtbus device
> + * @name: name to associate with the vdev
> + * @data: pointer to data to be associated with this device  */ struct
> +virtbus_device *virtbus_dev_alloc(const char *name, void *data) {
> +	struct virtbus_object *vo;
> +
Data should not be used.
Caller needs to give a size of the object to allocate.
I discussed the example in detail with Jason in v1 of this patch. Please refer in that email.
It should be something like this.

/* size = sizeof(struct i40_virtbus_dev), and it is the first member */
virtbus_dev_alloc(size)
{
	[..]
}

struct i40_virtbus_dev {
	struct virbus_dev virtdev;
	/*... more fields that you want to share with other driver and to use in probe() */
};

irdma_probe(..)
{
	struct i40_virtbus_dev dev = container_of(dev, struct i40_virtbus_dev, dev);
}

[..]

> diff --git a/include/linux/virtual_bus.h b/include/linux/virtual_bus.h new file
> mode 100644 index 000000000000..b6f2406180f8
> --- /dev/null
> +++ b/include/linux/virtual_bus.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * virtual_bus.h - lightweight software bus
> + *
> + * Copyright (c) 2019-20 Intel Corporation
> + *
> + * Please see Documentation/driver-api/virtual_bus.rst for more
> +information  */
> +
> +#ifndef _VIRTUAL_BUS_H_
> +#define _VIRTUAL_BUS_H_
> +
> +#include <linux/device.h>
> +
> +#define VIRTBUS_DEVID_NONE	(-1)
> +#define VIRTBUS_NAME_SIZE	20
> +
> +struct virtbus_dev_id {
> +	char name[VIRTBUS_NAME_SIZE];
> +	u64 driver_data;
> +};
> +
> +struct virtbus_device {
> +	const char			*name;
> +	int				id;
> +	const struct virtbus_dev_id	*dev_id;
> +	struct device			dev;
Drop the tab based alignment and just please follow format of virtbus_driver you did below.
> +	void				*data;
Please drop data. we need only wrapper API virtbus_get/set_drvdata().
> +};

[1] https://lore.kernel.org/linux-rdma/20191107160448.20962-1-parav@mellanox.com/
[2] https://lore.kernel.org/patchwork/patch/1046991/

Powered by blists - more mailing lists