Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling

[Juergen Gross <jgross@xxxxxxxx>]

On 16/04/18 08:24, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> Handle Xen event channels:
>   - create for all configured streams and publish
>     corresponding ring references and event channels in Xen store,
>     so backend can connect
>   - implement event channels interrupt handlers
>   - create and destroy event channels with respect to Xen bus state
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> ---
>  sound/xen/Makefile                |   3 +-
>  sound/xen/xen_snd_front.c         |  10 +-
>  sound/xen/xen_snd_front.h         |   7 +
>  sound/xen/xen_snd_front_evtchnl.c | 474 
> ++++++++++++++++++++++++++++++++++++++
>  sound/xen/xen_snd_front_evtchnl.h |  92 ++++++++
>  5 files changed, 584 insertions(+), 2 deletions(-)
>  create mode 100644 sound/xen/xen_snd_front_evtchnl.c
>  create mode 100644 sound/xen/xen_snd_front_evtchnl.h
> 
> diff --git a/sound/xen/Makefile b/sound/xen/Makefile
> index 06705bef61fa..03c669984000 100644
> --- a/sound/xen/Makefile
> +++ b/sound/xen/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0 OR MIT
>  
>  snd_xen_front-objs := xen_snd_front.o \
> -                   xen_snd_front_cfg.o
> +                   xen_snd_front_cfg.o \
> +                   xen_snd_front_evtchnl.o
>  
>  obj-$(CONFIG_SND_XEN_FRONTEND) += snd_xen_front.o
> diff --git a/sound/xen/xen_snd_front.c b/sound/xen/xen_snd_front.c
> index 65d2494a9d14..eb46bf4070f9 100644
> --- a/sound/xen/xen_snd_front.c
> +++ b/sound/xen/xen_snd_front.c
> @@ -18,9 +18,11 @@
>  #include <xen/interface/io/sndif.h>
>  
>  #include "xen_snd_front.h"
> +#include "xen_snd_front_evtchnl.h"

Does it really make sense to have multiple driver-private headers?

I think those can be merged.

>  
>  static void xen_snd_drv_fini(struct xen_snd_front_info *front_info)
>  {
> +     xen_snd_front_evtchnl_free_all(front_info);
>  }
>  
>  static int sndback_initwait(struct xen_snd_front_info *front_info)
> @@ -32,7 +34,12 @@ static int sndback_initwait(struct xen_snd_front_info 
> *front_info)
>       if (ret < 0)
>               return ret;
>  
> -     return 0;
> +     /* create event channels for all streams and publish */
> +     ret = xen_snd_front_evtchnl_create_all(front_info, num_streams);
> +     if (ret < 0)
> +             return ret;
> +
> +     return xen_snd_front_evtchnl_publish_all(front_info);
>  }
>  
>  static int sndback_connect(struct xen_snd_front_info *front_info)
> @@ -122,6 +129,7 @@ static int xen_drv_probe(struct xenbus_device *xb_dev,
>               return -ENOMEM;
>  
>       front_info->xb_dev = xb_dev;
> +     spin_lock_init(&front_info->io_lock);
>       dev_set_drvdata(&xb_dev->dev, front_info);
>  
>       return xenbus_switch_state(xb_dev, XenbusStateInitialising);
> diff --git a/sound/xen/xen_snd_front.h b/sound/xen/xen_snd_front.h
> index b52226cb30bc..9c2ffbb4e4b8 100644
> --- a/sound/xen/xen_snd_front.h
> +++ b/sound/xen/xen_snd_front.h
> @@ -13,9 +13,16 @@
>  
>  #include "xen_snd_front_cfg.h"
>  
> +struct xen_snd_front_evtchnl_pair;
> +
>  struct xen_snd_front_info {
>       struct xenbus_device *xb_dev;
>  
> +     /* serializer for backend IO: request/response */
> +     spinlock_t io_lock;
> +     int num_evt_pairs;
> +     struct xen_snd_front_evtchnl_pair *evt_pairs;
> +
>       struct xen_front_cfg_card cfg;
>  };
>  
> diff --git a/sound/xen/xen_snd_front_evtchnl.c 
> b/sound/xen/xen_snd_front_evtchnl.c
> new file mode 100644
> index 000000000000..9ece39f938f8
> --- /dev/null
> +++ b/sound/xen/xen_snd_front_evtchnl.c
> @@ -0,0 +1,474 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +/*
> + * Xen para-virtual sound device
> + *
> + * Copyright (C) 2016-2018 EPAM Systems Inc.
> + *
> + * Author: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> + */
> +
> +#include <xen/events.h>
> +#include <xen/grant_table.h>
> +#include <xen/xen.h>
> +#include <xen/xenbus.h>
> +
> +#include "xen_snd_front.h"
> +#include "xen_snd_front_cfg.h"
> +#include "xen_snd_front_evtchnl.h"
> +
> +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id)
> +{
> +     struct xen_snd_front_evtchnl *channel = dev_id;
> +     struct xen_snd_front_info *front_info = channel->front_info;
> +     struct xensnd_resp *resp;
> +     RING_IDX i, rp;
> +     unsigned long flags;
> +
> +     if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED))
> +             return IRQ_HANDLED;
> +
> +     spin_lock_irqsave(&front_info->io_lock, flags);
> +
> +again:
> +     rp = channel->u.req.ring.sring->rsp_prod;
> +     /* ensure we see queued responses up to rp */
> +     rmb();
> +
> +     for (i = channel->u.req.ring.rsp_cons; i != rp; i++) {
> +             resp = RING_GET_RESPONSE(&channel->u.req.ring, i);
> +             if (resp->id != channel->evt_id)
> +                     continue;
> +             switch (resp->operation) {
> +             case XENSND_OP_OPEN:
> +                     /* fall through */
> +             case XENSND_OP_CLOSE:
> +                     /* fall through */
> +             case XENSND_OP_READ:
> +                     /* fall through */
> +             case XENSND_OP_WRITE:
> +                     /* fall through */
> +             case XENSND_OP_TRIGGER:
> +                     channel->u.req.resp_status = resp->status;
> +                     complete(&channel->u.req.completion);
> +                     break;
> +             case XENSND_OP_HW_PARAM_QUERY:
> +                     channel->u.req.resp_status = resp->status;
> +                     channel->u.req.resp.hw_param =
> +                                     resp->resp.hw_param;
> +                     complete(&channel->u.req.completion);
> +                     break;
> +
> +             default:
> +                     dev_err(&front_info->xb_dev->dev,
> +                             "Operation %d is not supported\n",
> +                             resp->operation);
> +                     break;
> +             }
> +     }
> +
> +     channel->u.req.ring.rsp_cons = i;
> +     if (i != channel->u.req.ring.req_prod_pvt) {
> +             int more_to_do;
> +
> +             RING_FINAL_CHECK_FOR_RESPONSES(&channel->u.req.ring,
> +                                            more_to_do);
> +             if (more_to_do)
> +                     goto again;
> +     } else {
> +             channel->u.req.ring.sring->rsp_event = i + 1;
> +     }
> +
> +     spin_unlock_irqrestore(&front_info->io_lock, flags);
> +     return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t evtchnl_interrupt_evt(int irq, void *dev_id)
> +{
> +     struct xen_snd_front_evtchnl *channel = dev_id;
> +     struct xen_snd_front_info *front_info = channel->front_info;
> +     struct xensnd_event_page *page = channel->u.evt.page;
> +     u32 cons, prod;
> +     unsigned long flags;
> +
> +     if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED))
> +             return IRQ_HANDLED;
> +
> +     spin_lock_irqsave(&front_info->io_lock, flags);
> +
> +     prod = page->in_prod;
> +     /* ensure we see ring contents up to prod */
> +     virt_rmb();
> +     if (prod == page->in_cons)
> +             goto out;
> +
> +     for (cons = page->in_cons; cons != prod; cons++) {
> +             struct xensnd_evt *event;
> +
> +             event = &XENSND_IN_RING_REF(page, cons);
> +             if (unlikely(event->id != channel->evt_id++))
> +                     continue;
> +
> +             switch (event->type) {
> +             case XENSND_EVT_CUR_POS:
> +                     /* do nothing at the moment */
> +                     break;
> +             }
> +     }
> +
> +     page->in_cons = cons;
> +     /* ensure ring contents */
> +     virt_wmb();
> +
> +out:
> +     spin_unlock_irqrestore(&front_info->io_lock, flags);
> +     return IRQ_HANDLED;
> +}
> +
> +void xen_snd_front_evtchnl_flush(struct xen_snd_front_evtchnl *channel)
> +{
> +     int notify;
> +
> +     channel->u.req.ring.req_prod_pvt++;
> +     RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&channel->u.req.ring, notify);
> +     if (notify)
> +             notify_remote_via_irq(channel->irq);
> +}
> +
> +static void evtchnl_free(struct xen_snd_front_info *front_info,
> +                      struct xen_snd_front_evtchnl *channel)
> +{
> +     unsigned long page = 0;
> +
> +     if (channel->type == EVTCHNL_TYPE_REQ)
> +             page = (unsigned long)channel->u.req.ring.sring;
> +     else if (channel->type == EVTCHNL_TYPE_EVT)
> +             page = (unsigned long)channel->u.evt.page;
> +
> +     if (!page)
> +             return;
> +
> +     channel->state = EVTCHNL_STATE_DISCONNECTED;
> +     if (channel->type == EVTCHNL_TYPE_REQ) {
> +             /* release all who still waits for response if any */
> +             channel->u.req.resp_status = -EIO;
> +             complete_all(&channel->u.req.completion);
> +     }
> +
> +     if (channel->irq)
> +             unbind_from_irqhandler(channel->irq, channel);
> +
> +     if (channel->port)
> +             xenbus_free_evtchn(front_info->xb_dev, channel->port);
> +
> +     /* end access and free the page */
> +     if (channel->gref != GRANT_INVALID_REF)
> +             gnttab_end_foreign_access(channel->gref, 0, page);

Free page?

> +
> +     memset(channel, 0, sizeof(*channel));
> +}
> +
> +void xen_snd_front_evtchnl_free_all(struct xen_snd_front_info *front_info)
> +{
> +     int i;
> +
> +     if (!front_info->evt_pairs)
> +             return;
> +
> +     for (i = 0; i < front_info->num_evt_pairs; i++) {
> +             evtchnl_free(front_info, &front_info->evt_pairs[i].req);
> +             evtchnl_free(front_info, &front_info->evt_pairs[i].evt);
> +     }
> +
> +     kfree(front_info->evt_pairs);
> +     front_info->evt_pairs = NULL;
> +}
> +
> +static int evtchnl_alloc(struct xen_snd_front_info *front_info, int index,
> +                      struct xen_snd_front_evtchnl *channel,
> +                      enum xen_snd_front_evtchnl_type type)
> +{
> +     struct xenbus_device *xb_dev = front_info->xb_dev;
> +     unsigned long page;
> +     grant_ref_t gref;
> +     irq_handler_t handler;
> +     char *handler_name = NULL;
> +     int ret;
> +
> +     memset(channel, 0, sizeof(*channel));
> +     channel->type = type;
> +     channel->index = index;
> +     channel->front_info = front_info;
> +     channel->state = EVTCHNL_STATE_DISCONNECTED;
> +     channel->gref = GRANT_INVALID_REF;
> +     page = get_zeroed_page(GFP_NOIO | __GFP_HIGH);

Why GFP_NOIO | __GFP_HIGH? Could it be you copied that from blkfront
driver? I believe swapping via sound card is rather uncommon.

> +     if (!page) {
> +             ret = -ENOMEM;
> +             goto fail;
> +     }
> +
> +     handler_name = kasprintf(GFP_KERNEL, "%s-%s", XENSND_DRIVER_NAME,
> +                              type == EVTCHNL_TYPE_REQ ?
> +                              XENSND_FIELD_RING_REF :
> +                              XENSND_FIELD_EVT_RING_REF);
> +     if (!handler_name) {
> +             ret = -ENOMEM;

Missing free_page(page)? Maybe you rather add it in the common
fail path instead of the numerous instances below?


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel