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/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
>> +
>> + 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?
It can be net-front, I guess, which has the same for rx/tx rings
> I believe swapping via sound card is rather uncommon.
Indeed, will use GFP_KERNEL here
>> + 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?
>
yes, will move it under the common fail: label
> Juergen
[1]
https://elixir.bootlin.com/linux/v4.17-rc1/source/include/xen/grant_table.h#L96

\
 
 \ /
  Last update: 2018-04-17 10:58    [W:0.111 / U:4.252 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site