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

[Juergen Gross]

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.


Juergen