RE: [net-next 1/1] virtual-bus: Implementation of Virtual Bus

[Parav Pandit <parav@xxxxxxxxxxxx>]

Hi Jason,

> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Tuesday, November 12, 2019 6:08 PM
> 
> On Tue, Nov 12, 2019 at 10:28:26PM +0100, Greg KH wrote:
> 
> > > + */
> > > +struct virtbus_device {
> > > +	const char			*name;
> > > +	int				id;
> > > +	const struct virtbus_dev_id	*dev_id;
> > > +	struct device			dev;
> > > +	void				*data;
> > > +};
> > > +
> > > +struct virtbus_driver {
> > > +	int (*probe)(struct virtbus_device *);
> > > +	int (*remove)(struct virtbus_device *);
> > > +	void (*shutdown)(struct virtbus_device *);
> > > +	int (*suspend)(struct virtbus_device *, pm_message_t state);
> > > +	int (*resume)(struct virtbus_device *);
> > > +	struct device_driver driver;
> > > +	const struct virtbus_dev_id *id_table; };
> > > +
> > > +#define virtbus_get_dev_id(vdev)	((vdev)->id_entry)
> > > +#define virtbus_get_devdata(dev)	((dev)->devdata)
> >
> > What are these for?
> 
> As far as I can see, the scheme here, using the language from the most
> recent discussion is:
> 
>    // in core or netdev module
>    int mlx5_core_create()
>    {
>       struct mlx5_core_dev *core = kzalloc(..)
> 
>       [..]
> 
>       core->vdev = virtbus_dev_alloc("mlx5_core", core);
>    }
> 
> 
>    // in rdma module
>    static int mlx5_rdma_probe(struct virtbus_device *dev)
>    {
>         // Get the value passed to virtbus_dev_alloc()
> 	struct mlx5_core_dev *core = virtbus_get_devdata(dev)
> 
> 	// Use the huge API surrounding struct mlx5_core_dev
> 	qp = mlx5_core_create_qp(core, ...);
>    }
> 
>    static struct virtbus_driver mlx5_rdma_driver = {
>       .probe = mlx5_rdma_probe,
>       .match = {"mlx5_core"}
>    }
> 
> Drivers that match "mlx5_core" know that the opaque
> 'virtbus_get_devdata()' is a 'struct mlx5_core_dev *' and use that access the
> core driver.
> 
> A "ice_core" would know it is some 'struct ice_core_dev *' for Intel and uses
> that pointer, etc.
> 
> ie it is just a way to a pass a 'void *' from one module to another while using
> the driver core to manage module autoloading and binding.

A small improvement below, because get_drvdata() and set_drvdata() is supposed to be called by the bus driver, not its creator.
And below data structure achieve strong type checks, no void* casts, and exactly achieves the foo_device example.
Isn't it better?

mlx5_virtbus_device {
	struct virtbus_device dev;
	struct mlx5_core_dev *dev;
};

mlx5_core_create(const struct mlx5_core_dev *coredev)
{
	struct mlx5_virtbus_device *dev;

	dev = virtdev_alloc(sizeof(*dev));
	dev->core = coredev;	/* this should not be stored in drvdata */
	virtdev_register(dev);
}

mlx5_rdma_probe(struct virtbus_device *dev)
{
	struct mlx5_core_dev *coredev;
	struct mlx5_virtdev *virtdev;
	struct mlx5_ib_dev *ibdev;

	virtdev = container_of(virtdev, struct mlx5_virtdev, dev);
	coredev = virtdev->coredev;
	ibdev = ib_alloc_dev();
	if (!bdev)
		return -ENOMEM;
	virtdev_set_drvdata(dev, ibdev);
	/* setup.. */
	return 0;	
}

mlx5_rdma_remove(struct virtbus_device *dev)
{
	struct mlx5_ib_dev *ibdev;

	ibdev = virtdev_get_drvdata(dev);
	/* previous load failed */
	if(!ibdev)
		return;
	[..]
	/* mirror of probe */
}