Messages in this thread | | | Subject | Re: [PATCH V4 5/5] vdpasim: vDPA device simulator | From | Jason Wang <> | Date | Fri, 21 Feb 2020 15:57:29 +0800 |
| |
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 >
| |