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
