Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754125AbdCHT0d (ORCPT ); Wed, 8 Mar 2017 14:26:33 -0500 Received: from mail.kernel.org ([198.145.29.136]:53930 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753996AbdCHT0b (ORCPT ); Wed, 8 Mar 2017 14:26:31 -0500 Date: Wed, 8 Mar 2017 11:26:03 -0800 (PST) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-X260 To: Boris Ostrovsky cc: Stefano Stabellini , xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, Stefano Stabellini , jgross@suse.com, Eric Van Hensbergen , Ron Minnich , Latchesar Ionkov , v9fs-developer@lists.sourceforge.net Subject: Re: [PATCH 6/7] xen/9pfs: receive responses In-Reply-To: Message-ID: References: <1488830488-18506-1-git-send-email-sstabellini@kernel.org> <1488830488-18506-6-git-send-email-sstabellini@kernel.org> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1890 Lines: 57 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.