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

[Juergen Gross <jgross@xxxxxxxx>]

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.

> +     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;

> +
> +out:
> +     if (!bytes) {

Really? Shouldn't this be "if (bytes)" ?

> +             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