Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754188AbdCHTXl (ORCPT ); Wed, 8 Mar 2017 14:23:41 -0500 Received: from mail.kernel.org ([198.145.29.136]:53476 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752094AbdCHTXj (ORCPT ); Wed, 8 Mar 2017 14:23:39 -0500 Date: Wed, 8 Mar 2017 11:22:55 -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 4/7] xen/9pfs: connect to the backend In-Reply-To: <7a016728-5a22-6ab0-a34a-4f7354927cf1@oracle.com> Message-ID: References: <1488830488-18506-1-git-send-email-sstabellini@kernel.org> <1488830488-18506-4-git-send-email-sstabellini@kernel.org> <7a016728-5a22-6ab0-a34a-4f7354927cf1@oracle.com> 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: 2983 Lines: 88 On Wed, 8 Mar 2017, Boris Ostrovsky wrote: > >>> + ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO, XEN_9PFS_RING_ORDER); > >>> + if (ring->bytes == NULL) > >>> + goto error; > >>> + for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++) > >>> + ring->intf->ref[i] = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->bytes) + i), 0); > >>> + ring->ref = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->intf)), 0); > >>> + ring->ring.in = ring->bytes; > >>> + ring->ring.out = ring->bytes + XEN_9PFS_RING_SIZE; > >>> + > >>> + ret = xenbus_alloc_evtchn(dev, &ring->evtchn); > >>> + if (ret) > >>> + goto error; > >>> + ring->irq = bind_evtchn_to_irqhandler(ring->evtchn, xen_9pfs_front_event_handler, > >>> + 0, "xen_9pfs-frontend", ring); > >>> + if (ring->irq < 0) { > >>> + xenbus_free_evtchn(dev, ring->evtchn); > >>> + ret = ring->irq; > >>> + goto error; > >>> + } > >>> return 0; > >>> + > >>> +error: > >> You may need to gnttab_end_foreign_access(). > > Actually this error path is unnecessary because it will be handled by > > xen_9pfs_front_probe, that calls xen_9pfs_front_free on errors. I'll > > remove it. > > > (It's a matter of personal preference but I think a routine should clean > up its own mess whenever it can.) > > > > > > > + > > + again: > > + ret = xenbus_transaction_start(&xbt); > > + if (ret) { > > + xenbus_dev_fatal(dev, ret, "starting transaction"); > > + goto error; > > + } > > + ret = xenbus_printf(xbt, dev->nodename, "version", "%u", 1); > > + if (ret) > > + goto error_xenbus; > > + ret = xenbus_printf(xbt, dev->nodename, "num-rings", "%u", priv->num_rings); > > + if (ret) > > + goto error_xenbus; > > + for (i = 0; i < priv->num_rings; i++) { > > + char str[16]; > > + > > + priv->rings[i].priv = priv; > > + ret = xen_9pfs_front_alloc_dataring(dev, &priv->rings[i]); > >> Not for -EAGAIN, I think. > > I don't think xen_9pfs_front_alloc_dataring can return EAGAIN. EAGAIN > > can only come from xenbus_transaction_end, the case we handle below. > > I didn't mean that xen_9pfs_front_alloc_dataring() can return -EAGAIN. I > was referring to... > > > > > > >>> + if (ret < 0) > >>> + goto error_xenbus; > >>> + > >>> + sprintf(str, "ring-ref%u", i); > >>> + ret = xenbus_printf(xbt, dev->nodename, str, "%d", priv->rings[i].ref); > >>> + if (ret) > >>> + goto error_xenbus; > >>> + > >>> + sprintf(str, "event-channel-%u", i); > >>> + ret = xenbus_printf(xbt, dev->nodename, str, "%u", priv->rings[i].evtchn); > >>> + if (ret) > >>> + goto error_xenbus; > >>> + } > >>> + priv->tag = xenbus_read(xbt, dev->nodename, "tag", NULL); > >>> + if (ret) > >>> + goto error_xenbus; > >>> + ret = xenbus_transaction_end(xbt, 0); > >>> + if (ret) { > >>> + if (ret == -EAGAIN) > >>> + goto again; > > ... this. > > Sorry for not being clear. You are right! I'll move the call to xen_9pfs_front_alloc_dataring out of the xenbus loop.