lkml.org 
[lkml]   [2020]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH V4 5/5] vdpasim: vDPA device simulator
    Date
    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 ?

    > + 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.

    > +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 ?

    Jason

    \
     
     \ /
      Last update: 2020-02-20 16:13    [W:3.625 / U:1.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site