From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oleksandr Andrushchenko Subject: Re: [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling Date: Tue, 17 Apr 2018 14:21:06 +0300 Message-ID: References: <20180416062453.24743-1-andr2000@gmail.com> <20180416062453.24743-4-andr2000@gmail.com> <2e06f714-ebc2-11bb-078b-8a453ea8464c@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Juergen Gross , xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org, boris.ostrovsky@oracle.com, konrad.wilk@oracle.com, perex@perex.cz, tiwai@suse.com Cc: Oleksandr Andrushchenko List-Id: alsa-devel@alsa-project.org 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 >>>> >>>> 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 >>>> >>>> --- >>>>   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 >>>>     #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 >>>> + */ >>>> + >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +#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