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

From: Oleksandr Andrushchenko
Date: Tue Apr 17 2018 - 07:21:16 EST

On 04/17/2018 02:14 PM, Juergen Gross wrote:
On 17/04/18 10:58, Oleksandr Andrushchenko wrote:
On 04/16/2018 04:12 PM, Juergen Gross wrote:
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.
I would really like to keep it separate as it clearly
shows which stuff belongs to which modules.
At least for me it is easier to maintain it this way.
  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?
According to [1] if page is provided to gnttab_end_foreign_access
it will be freed. So, no need to free it manually
Either a free_page() is missing here in an else clause or the if is
not necessary.
Good catch ;) I need else + free_page, because I won't be able
to end access for invalid grant ref.
Thank you

Juergen