Re: [PATCH V4 5/5] vdpasim: vDPA device simulator

[Jason Wang]

On 2020/2/20 äå11:12, Jason Gunthorpe wrote:
> On Thu, Feb 20, 2020 at 02:11:41PM +0800, Jason Wang wrote:
> > +static void vdpasim_device_release(struct device *dev)
> > +{
> > +	struct vdpasim *vdpasim = dev_to_sim(dev);
> > +
> > +	cancel_work_sync(&vdpasim->work);
> > +	kfree(vdpasim->buffer);
> > +	vhost_iotlb_free(vdpasim->iommu);
> > +	kfree(vdpasim);
> > +}
> > +
> > +static struct vdpasim *vdpasim_create(void)
> > +{
> > +	struct virtio_net_config *config;
> > +	struct vhost_iotlb *iommu;
> > +	struct vdpasim *vdpasim;
> > +	struct device *dev;
> > +	void *buffer;
> > +	int ret = -ENOMEM;
> > +
> > +	iommu = vhost_iotlb_alloc(2048, 0);
> > +	if (!iommu)
> > +		goto err;
> > +
> > +	buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > +	if (!buffer)
> > +		goto err_buffer;
> > +
> > +	vdpasim = kzalloc(sizeof(*vdpasim), GFP_KERNEL);
> > +	if (!vdpasim)
> > +		goto err_alloc;
> > +
> > +	vdpasim->buffer = buffer;
> > +	vdpasim->iommu = iommu;
> > +
> > +	config = &vdpasim->config;
> > +	config->mtu = 1500;
> > +	config->status = VIRTIO_NET_S_LINK_UP;
> > +	eth_random_addr(config->mac);
> > +
> > +	INIT_WORK(&vdpasim->work, vdpasim_work);
> > +	spin_lock_init(&vdpasim->lock);
> > +
> > +	vringh_set_iotlb(&vdpasim->vqs[0].vring, vdpasim->iommu);
> > +	vringh_set_iotlb(&vdpasim->vqs[1].vring, vdpasim->iommu);
> > +
> > +	dev = &vdpasim->dev;
> > +	dev->release = vdpasim_device_release;
> > +	dev->coherent_dma_mask = DMA_BIT_MASK(64);
> > +	set_dma_ops(dev, &vdpasim_dma_ops);
> > +	dev_set_name(dev, "%s", VDPASIM_NAME);
> > +
> > +	ret = device_register(&vdpasim->dev);
> > +	if (ret)
> > +		goto err_init;
> It is a bit weird to be creating this dummy parent, couldn't this be
> done by just passing a NULL parent to vdpa_alloc_device, doing
> set_dma_ops() on the vdpasim->vdpa->dev and setting dma_device to
> vdpasim->vdpa->dev ?


I think it works.


> > +	vdpasim->vdpa = vdpa_alloc_device(dev, dev, &vdpasim_net_config_ops);
> > +	if (ret)
> > +		goto err_vdpa;
> > +	ret = vdpa_register_device(vdpasim->vdpa);
> > +	if (ret)
> > +		goto err_register;
> > +
> > +	return vdpasim;
> > +
> > +err_register:
> > +	put_device(&vdpasim->vdpa->dev);
> > +err_vdpa:
> > +	device_del(&vdpasim->dev);
> > +	goto err;
> > +err_init:
> > +	put_device(&vdpasim->dev);
> > +	goto err;
> If you do the vdmasim alloc first, and immediately do
> device_initialize() then all the failure paths can do put_device
> instead of having this ugly goto unwind split. Just check for
> vdpasim->iommu == NULL during release.


Yes, that looks simpler.


> > +static int __init vdpasim_dev_init(void)
> > +{
> > +	vdpasim_dev = vdpasim_create();
> > +
> > +	if (!IS_ERR(vdpasim_dev))
> > +		return 0;
> > +
> > +	return PTR_ERR(vdpasim_dev);
> > +}
> > +
> > +static int vdpasim_device_remove_cb(struct device *dev, void *data)
> > +{
> > +	struct vdpa_device *vdpa = dev_to_vdpa(dev);
> > +
> > +	vdpa_unregister_device(vdpa);
> > +
> > +	return 0;
> > +}
> > +
> > +static void __exit vdpasim_dev_exit(void)
> > +{
> > +	device_for_each_child(&vdpasim_dev->dev, NULL,
> > +			      vdpasim_device_remove_cb);
> Why the loop? There is only one device, and it is in the global
> varaible vdmasim_dev ?


Not necessary but doesn't harm, will remove this.

Thanks


> Jason