Re: [Xen-devel] [PATCH v5 4/7] xen/9pfs: connect to the backend

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 21 Mar 2017, Juergen Gross wrote:
> On 20/03/17 18:59, Stefano Stabellini wrote:
> > Implement functions to handle the xenbus handshake. Upon connection,
> > allocate the rings according to the protocol specification.
> > 
> > Initialize a work_struct and a wait_queue. The work_struct will be used
> > to schedule work upon receiving an event channel notification from the
> > backend. The wait_queue will be used to wait when the ring is full and
> > we need to send a new request.
> > 
> > Signed-off-by: Stefano Stabellini <stefano@xxxxxxxxxxx>
> > CC: groug@xxxxxxxx
> > CC: boris.ostrovsky@xxxxxxxxxx
> > CC: jgross@xxxxxxxx
> > CC: Eric Van Hensbergen <ericvh@xxxxxxxxx>
> > CC: Ron Minnich <rminnich@xxxxxxxxxx>
> > CC: Latchesar Ionkov <lucho@xxxxxxxxxx>
> > CC: v9fs-developer@xxxxxxxxxxxxxxxxxxxxx
> > ---
> >  net/9p/trans_xen.c | 273 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 273 insertions(+)
> > 
> > diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> > index 3d07260..5279ae0 100644
> > --- a/net/9p/trans_xen.c
> > +++ b/net/9p/trans_xen.c
> ...
> > +static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
> > +                                    struct xen_9pfs_dataring *ring)
> > +{
> > +   int i = 0;
> > +   int ret = -ENOMEM;
> > +   void *bytes = NULL;
> > +
> > +   init_waitqueue_head(&ring->wq);
> > +   spin_lock_init(&ring->lock);
> > +   INIT_WORK(&ring->work, p9_xen_response);
> > +
> > +   ring->intf = (struct xen_9pfs_data_intf *)get_zeroed_page(GFP_KERNEL);
> > +   if (!ring->intf)
> > +           return ret;
> > +   ret = gnttab_grant_foreign_access(dev->otherend_id,
> > +                                     virt_to_gfn(ring->intf), 0);
> > +   if (ret < 0)
> > +           goto out;
> > +   ring->ref = ret;
> > +   bytes = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> > +                   XEN_9PFS_RING_ORDER - (PAGE_SHIFT - XEN_PAGE_SHIFT));
> > +   if (!bytes)
> > +           goto out;
> 
> ret is wrong here.

I'll fix


> > +   for (; i < (1 << XEN_9PFS_RING_ORDER); i++) {
> > +           ret = gnttab_grant_foreign_access(
> > +                           dev->otherend_id, virt_to_gfn(bytes) + i, 0);
> > +           if (ret < 0)
> > +                   goto out;
> > +           ring->intf->ref[i] = ret;
> > +   }
> > +   ring->intf->ring_order = XEN_9PFS_RING_ORDER;
> > +   ring->data.in = bytes;
> > +   ring->data.out = bytes + XEN_9PFS_RING_SIZE;
> > +
> > +   ret = xenbus_alloc_evtchn(dev, &ring->evtchn);
> > +   if (ret)
> > +           goto out;
> > +   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 out;
> > +   }
> >     return 0;
> 
> 
> What about:
> 
> if (ring->irq >= 0)
>       return 0;
> 
> xenbus_free_evtchn(dev, ring->evtchn);
> ret = ring->irq;

OK


> > +
> > +out:
> > +   if (!bytes) {
> 
> Really? Shouldn't this be "if (bytes)" ?

Wops. I'll fix.


> > +           for (i--; i >= 0; i--)
> > +                   gnttab_end_foreign_access(ring->intf->ref[i], 0, 0);
> > +           free_pages((unsigned long)bytes,
> > +                      XEN_9PFS_RING_ORDER -
> > +                      (PAGE_SHIFT - XEN_PAGE_SHIFT));
> > +   }
> > +   gnttab_end_foreign_access(ring->ref, 0, 0);
> > +   free_page((unsigned long)ring->intf);
> > +   return ret;
> >  }
> 
> 
> Juergen
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel