Re: [PATCH 6/7] xen/9pfs: receive responses

[Stefano Stabellini <sstabellini@kernel.org>]

On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
> >>> +
> >>> +		if (xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < sizeof(h)) {
> >>> +			notify_remote_via_irq(ring->irq);
> >>> +			return;
> >>> +		}
> >>> +
> >>> +		masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
> >>> +		masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
> >>> +
> >>> +		xen_9pfs_read_packet(ring->ring.in,
> >>> +				masked_prod, &masked_cons,
> >>> +				XEN_9PFS_RING_SIZE, &h, sizeof(h));
> >>> +
> >>> +		req = p9_tag_lookup(priv->client, h.tag);
> >>> +		if (!req || req->status != REQ_STATUS_SENT) {
> >>> +			dev_warn(&priv->dev->dev, "Wrong req tag=%x\n", h.tag);
> >>> +			cons += h.size;
> >>> +			mb();
> >>> +			ring->intf->in_cons = cons;
> >>> +			continue;
> >>
> >> I don't know what xen_9pfs_read_packet() does so perhaps it's done there
> >> but shouldn't the pointers be updated regardless of the 'if' condition?
> > This is the error path - the index is increased immediately. In the
> > non-error case, we do that right after the next read_packet call, few
> > lines below.
> >
> >
> >>> +		}
> >>> +
> >>> +		memcpy(req->rc, &h, sizeof(h));
> >>> +		req->rc->offset = 0;
> >>> +
> >>> +		masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
> >>> +		xen_9pfs_read_packet(ring->ring.in,
> >>> +				masked_prod, &masked_cons,
> >>> +				XEN_9PFS_RING_SIZE, req->rc->sdata, h.size);
> >>> +
> >>> +		mb();
> >>> +		cons += h.size;
> >>> +		ring->intf->in_cons = cons;
> >                    Here ^
> >
> 
> 
> So the second read is reading again from the same pointer in the ring,
> but this time it gets the whole packet, including the header. The first
> read was just poking at the header. Right?

That's right. First we read the header, to know how much data to read.


> If that's correct, can you add a comment somewhere? (unless this is
> obvious to everyone else but me.)

Sure.