Re: [RFC v3 2/2] virtio-pmem: Add virtio pmem driver

[Pankaj Gupta <pagupta-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

Hi Stefan,

> > + /* The request submission function */
> > +static int virtio_pmem_flush(struct device *dev)
> > +{
> > +	int err;
> > +	unsigned long flags;
> > +	struct scatterlist *sgs[2], sg, ret;
> > +	struct virtio_device *vdev = dev_to_virtio(dev->parent->parent);
> > +	struct virtio_pmem *vpmem = vdev->priv;
> > +	struct virtio_pmem_request *req = kmalloc(sizeof(*req), GFP_KERNEL);
> > +
> > +	req->done = false;
> > +	init_waitqueue_head(&req->acked);
> > +	spin_lock_irqsave(&vpmem->pmem_lock, flags);
> > +
> > +	sg_init_one(&sg, req, sizeof(req));
> 
> What are you trying to do here?
> 
> sizeof(req) == sizeof(struct virtio_pmem_request *) == sizeof(void *)
> 
> Did you mean sizeof(*req)?

yes, I meant: sizeof(struct virtio_pmem_request)

Thanks for catching this.

> 
> But why map struct virtio_pmem_request to the device?  struct
> virtio_pmem_request is the driver-internal request state and is not part
> of the hardware interface.

o.k. I will separate out request from 'virtio_pmem_request' struct
and use that. 

> 
> > +	sgs[0] = &sg;
> > +	sg_init_one(&ret, &req->ret, sizeof(req->ret));
> > +	sgs[1] = &ret;
> > +	err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req, GFP_ATOMIC);
> > +	if (err) {
> > +		dev_err(&vdev->dev, "failed to send command to virtio pmem device\n");
> 
> This can happen if the virtqueue is full.  Printing a message and
> failing the flush isn't appropriate.  This thread needs to wait until
> virtqueue space becomes available.

o.k. I will implement this.

> 
> > +		spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> > +		return -ENOSPC;
> 
> req is leaked.

will free req.

> 
> > +	virtio_device_ready(vdev);
> 
> This call isn't needed.  Driver use it when they wish to submit buffers
> on virtqueues before ->probe() returns.

o.k. I will remove it.

> 
> > diff --git a/include/linux/virtio_pmem.h b/include/linux/virtio_pmem.h
> > new file mode 100644
> > index 0000000..0f83d9c
> > --- /dev/null
> > +++ b/include/linux/virtio_pmem.h
> 
> include/ is for declarations (e.g. kernel APIs) needed by other
> compilation units.  The contents of this header are internal to the
> virtio_pmem driver implementation and can therefore be in virtio_pmem.c.
> include/linux/virtio_pmem.h isn't necessary since nothing besides
> virtio_pmem.c will need to include it.

Agree. Will move declarations from virtio_pmem.h to virtio_pmem.c

Thanks,
Pankaj