lkml.org 
[lkml]   [2018]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
From
Date
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@epam.com>
>>>>
>>>> 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@epam.com>
>>>> ---
>>>>   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@epam.com>
>>>> + */
>>>> +
>>>> +#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

\
 
 \ /
  Last update: 2018-04-17 13:22    [W:0.105 / U:0.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site