Re: [PATCH V2 3/5] vDPA: introduce vDPA bus

On 2020/2/11 下午9:47, Jason Gunthorpe wrote:

On Mon, Feb 10, 2020 at 11:56:06AM +0800, Jason Wang wrote:

quoted

+/**
+ * vdpa_register_device - register a vDPA device
+ * Callers must have a succeed call of vdpa_init_device() before.
+ * @vdev: the vdpa device to be registered to vDPA bus
+ *
+ * Returns an error when fail to add to vDPA bus
+ */
+int vdpa_register_device(struct vdpa_device *vdev)
+{
+	int err = device_add(&vdev->dev);
+
+	if (err) {
+		put_device(&vdev->dev);
+		ida_simple_remove(&vdpa_index_ida, vdev->index);
+	}

This is a very dangerous construction, I've seen it lead to driver
bugs. Better to require the driver to always do the put_device on
error unwind

Ok.

The ida_simple_remove should probably be part of the class release
function to make everything work right

It looks to me bus instead of class is the correct abstraction here 
since the devices share a set of programming interface but not the 
semantics.

Or do you actually mean type here?

quoted

+/**
+ * vdpa_unregister_driver - unregister a vDPA device driver
+ * @drv: the vdpa device driver to be unregistered
+ */
+void vdpa_unregister_driver(struct vdpa_driver *drv)
+{
+	driver_unregister(&drv->driver);
+}
+EXPORT_SYMBOL_GPL(vdpa_unregister_driver);
+
+static int vdpa_init(void)
+{
+	if (bus_register(&vdpa_bus) != 0)
+		panic("virtio bus registration failed");
+	return 0;
+}

Linus will tell you not to kill the kernel - return the error code and
propagate it up to the module init function.

Yes, will fix.

quoted

+/**
+ * vDPA device - representation of a vDPA device
+ * @dev: underlying device
+ * @dma_dev: the actual device that is performing DMA
+ * @config: the configuration ops for this device.
+ * @index: device index
+ */
+struct vdpa_device {
+	struct device dev;
+	struct device *dma_dev;
+	const struct vdpa_config_ops *config;
+	int index;

unsigned values shuld be unsigned int

Jason

Will fix.

Thanks