lkml.org 
[lkml]   [2020]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V4 5/5] vdpasim: vDPA device simulator
From
Date

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
>

\
 
 \ /
  Last update: 2020-02-21 09:42    [W:0.103 / U:32.272 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site