>>
>>> + }
>>> +
>>> + /* When host has read buffer, this completes via host_ack */
>>
>> "A host repsonse results in "host_ack" getting called" ... ?
>>
>>> + wait_event(req->host_acked, req->done);
>>> + err = req->ret;
>>> +ret:
>>> + kfree(req);
>>> + return err;
>>> +};
>>> +
>>> +/* The asynchronous flush callback function */
>>> +int async_pmem_flush(struct nd_region *nd_region, struct bio *bio)
>>> +{
>>> + int rc = 0;
>>> +
>>> + /* Create child bio for asynchronous flush and chain with
>>> + * parent bio. Otherwise directly call nd_region flush.
>>> + */
>>> + if (bio && bio->bi_iter.bi_sector != -1) {
>>> + struct bio *child = bio_alloc(GFP_ATOMIC, 0);
>>> +
>>> + if (!child)
>>> + return -ENOMEM;
>>> + bio_copy_dev(child, bio);
>>> + child->bi_opf = REQ_PREFLUSH;
>>> + child->bi_iter.bi_sector = -1;
>>> + bio_chain(child, bio);
>>> + submit_bio(child);
>>
>> return 0;
>>
>> Then, drop the "else" case and "int rc" and do directly
>>
>> if (virtio_pmem_flush(nd_region))
>> return -EIO;
>
> and another 'return 0' here :)
>
> I don't like return from multiple places instead I prefer
> single exit point from function.
Makes this function more complicated than necessary. I agree when there
are locks involved.
>
>>
>>> +
>>> + return 0;
>>> +};
>>> +
>>> +static int virtio_pmem_probe(struct virtio_device *vdev)
>>> +{
>>> + int err = 0;
>>> + struct resource res;
>>> + struct virtio_pmem *vpmem;
>>> + struct nd_region_desc ndr_desc = {};
>>> + int nid = dev_to_node(&vdev->dev);
>>> + struct nd_region *nd_region;
>>
>> Nit: use reverse Christmas tree layout :)
>
> Done.
>
>>
>>> +
>>> + if (!vdev->config->get) {
>>> + dev_err(&vdev->dev, "%s failure: config access disabled\n",
>>> + __func__);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem), GFP_KERNEL);
>>> + if (!vpmem) {
>>> + err = -ENOMEM;
>>> + goto out_err;
>>> + }
>>> +
>>> + vpmem->vdev = vdev;
>>> + vdev->priv = vpmem;
>>> + err = init_vq(vpmem);
>>> + if (err)
>>> + goto out_err;
>>> +
>>> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
>>> + start, &vpmem->start);
>>> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
>>> + size, &vpmem->size);
>>> +
>>> + res.start = vpmem->start;
>>> + res.end = vpmem->start + vpmem->size-1;
>>> + vpmem->nd_desc.provider_name = "virtio-pmem";
>>> + vpmem->nd_desc.module = THIS_MODULE;
>>> +
>>> + vpmem->nvdimm_bus = nvdimm_bus_register(&vdev->dev,
>>> + &vpmem->nd_desc);
>>> + if (!vpmem->nvdimm_bus)
>>> + goto out_vq;
>>> +
>>> + dev_set_drvdata(&vdev->dev, vpmem->nvdimm_bus);
>>> +
>>> + ndr_desc.res = &res;
>>> + ndr_desc.numa_node = nid;
>>> + ndr_desc.flush = async_pmem_flush;
>>> + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
>>> + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
>>> + nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
>>> +
>>
>> I'd drop this empty line.
>
> hmm.
>
The common pattern after allocating something, immediately check for it
in the next line (like you do throughout this patch ;) )
...
>> You are not freeing "vdev->priv".
>
> vdev->priv is vpmem which is allocated using devm API.
I'm confused. Looking at drivers/virtio/virtio_balloon.c:
static void virtballoon_remove(struct virtio_device *vdev)
{
struct virtio_balloon *vb = vdev->priv;
...
kfree(vb);
}
I think you should do the same here, vdev->priv is allocated in
virtio_pmem_probe.
But maybe I am missing something important here :)
>>
>>> + nvdimm_bus_unregister(nvdimm_bus);
>>> + vdev->config->del_vqs(vdev);
>>> + vdev->config->reset(vdev);
>>> +}
>>> +
--
Thanks,
David / dhildenb
