| Message ID | 20180416062453.24743-4-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > 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? > + > + 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? I believe swapping via sound card is rather uncommon. > + 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? Juergen
Hi Oleksandr,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on sound/for-next]
[also build test ERROR on v4.17-rc1 next-20180416]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Oleksandr-Andrushchenko/ALSA-xen-front-Add-Xen-para-virtualized-frontend-driver/20180416-143123
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
In file included from sound/xen/xen_snd_front.c:21:0:
>> sound/xen/xen_snd_front_evtchnl.h:62:34: error: field 'hw_param' has incomplete type
struct xensnd_query_hw_param hw_param;
^~~~~~~~
--
sound/xen/xen_snd_front_evtchnl.c:51:22: sparse: undefined identifier 'XENSND_OP_TRIGGER'
sound/xen/xen_snd_front_evtchnl.c:55:22: sparse: undefined identifier 'XENSND_OP_HW_PARAM_QUERY'
sound/xen/xen_snd_front_evtchnl.c:58:45: sparse: no member 'resp' in struct xensnd_resp
sound/xen/xen_snd_front_evtchnl.c:51:22: sparse: incompatible types for 'case' statement
sound/xen/xen_snd_front_evtchnl.c:55:22: sparse: incompatible types for 'case' statement
sound/xen/xen_snd_front_evtchnl.c:99:20: sparse: using member 'in_prod' in incomplete struct xensnd_event_page
sound/xen/xen_snd_front_evtchnl.c:102:25: sparse: using member 'in_cons' in incomplete struct xensnd_event_page
sound/xen/xen_snd_front_evtchnl.c:105:25: sparse: using member 'in_cons' in incomplete struct xensnd_event_page
sound/xen/xen_snd_front_evtchnl.c:108:26: sparse: undefined identifier 'XENSND_IN_RING_REF'
sound/xen/xen_snd_front_evtchnl.c:109:21: sparse: using member 'id' in incomplete struct xensnd_evt
sound/xen/xen_snd_front_evtchnl.c:112:30: sparse: using member 'type' in incomplete struct xensnd_evt
sound/xen/xen_snd_front_evtchnl.c:113:22: sparse: undefined identifier 'XENSND_EVT_CUR_POS'
sound/xen/xen_snd_front_evtchnl.c:113:22: sparse: incompatible types for 'case' statement
sound/xen/xen_snd_front_evtchnl.c:119:13: sparse: using member 'in_cons' in incomplete struct xensnd_event_page
sound/xen/xen_snd_front_evtchnl.c:213:34: sparse: undefined identifier 'XENSND_FIELD_EVT_RING_REF'
sound/xen/xen_snd_front_evtchnl.c:412:47: sparse: undefined identifier 'XENSND_FIELD_EVT_RING_REF'
sound/xen/xen_snd_front_evtchnl.c:432:47: sparse: undefined identifier 'XENSND_FIELD_EVT_RING_REF'
sound/xen/xen_snd_front_evtchnl.c:51:22: sparse: Expected constant expression in case statement
sound/xen/xen_snd_front_evtchnl.c:55:22: sparse: Expected constant expression in case statement
sound/xen/xen_snd_front_evtchnl.c:113:22: sparse: Expected constant expression in case statement
In file included from sound/xen/xen_snd_front_evtchnl.c:18:0:
>> sound/xen/xen_snd_front_evtchnl.h:62:34: error: field 'hw_param' has incomplete type
struct xensnd_query_hw_param hw_param;
^~~~~~~~
sound/xen/xen_snd_front_evtchnl.c: In function 'evtchnl_interrupt_req':
>> sound/xen/xen_snd_front_evtchnl.c:51:8: error: 'XENSND_OP_TRIGGER' undeclared (first use in this function); did you mean 'XENSND_OP_WRITE'?
case XENSND_OP_TRIGGER:
^~~~~~~~~~~~~~~~~
XENSND_OP_WRITE
sound/xen/xen_snd_front_evtchnl.c:51:8: note: each undeclared identifier is reported only once for each function it appears in
>> sound/xen/xen_snd_front_evtchnl.c:55:8: error: 'XENSND_OP_HW_PARAM_QUERY' undeclared (first use in this function); did you mean 'XENSND_OP_TRIGGER'?
case XENSND_OP_HW_PARAM_QUERY:
^~~~~~~~~~~~~~~~~~~~~~~~
XENSND_OP_TRIGGER
>> sound/xen/xen_snd_front_evtchnl.c:58:10: error: 'struct xensnd_resp' has no member named 'resp'
resp->resp.hw_param;
^~
sound/xen/xen_snd_front_evtchnl.c: In function 'evtchnl_interrupt_evt':
>> sound/xen/xen_snd_front_evtchnl.c:99:13: error: dereferencing pointer to incomplete type 'struct xensnd_event_page'
prod = page->in_prod;
^~
>> sound/xen/xen_snd_front_evtchnl.c:108:12: error: implicit declaration of function 'XENSND_IN_RING_REF'; did you mean 'XENSND_FIELD_RING_REF'? [-Werror=implicit-function-declaration]
event = &XENSND_IN_RING_REF(page, cons);
^~~~~~~~~~~~~~~~~~
XENSND_FIELD_RING_REF
>> sound/xen/xen_snd_front_evtchnl.c:108:11: error: lvalue required as unary '&' operand
event = &XENSND_IN_RING_REF(page, cons);
^
In file included from include/linux/kernel.h:10:0,
from include/linux/interrupt.h:6,
from include/xen/events.h:5,
from sound/xen/xen_snd_front_evtchnl.c:11:
>> sound/xen/xen_snd_front_evtchnl.c:109:21: error: dereferencing pointer to incomplete type 'struct xensnd_evt'
if (unlikely(event->id != channel->evt_id++))
^
include/linux/compiler.h:77:42: note: in definition of macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
^
>> sound/xen/xen_snd_front_evtchnl.c:113:8: error: 'XENSND_EVT_CUR_POS' undeclared (first use in this function); did you mean 'XENSND_OP_CLOSE'?
case XENSND_EVT_CUR_POS:
^~~~~~~~~~~~~~~~~~
XENSND_OP_CLOSE
sound/xen/xen_snd_front_evtchnl.c: In function 'evtchnl_alloc':
>> sound/xen/xen_snd_front_evtchnl.c:213:6: error: 'XENSND_FIELD_EVT_RING_REF' undeclared (first use in this function); did you mean 'XENSND_FIELD_RING_REF'?
XENSND_FIELD_EVT_RING_REF);
^~~~~~~~~~~~~~~~~~~~~~~~~
XENSND_FIELD_RING_REF
sound/xen/xen_snd_front_evtchnl.c: In function 'xen_snd_front_evtchnl_publish_all':
sound/xen/xen_snd_front_evtchnl.c:412:12: error: 'XENSND_FIELD_EVT_RING_REF' undeclared (first use in this function); did you mean 'XENSND_FIELD_RING_REF'?
XENSND_FIELD_EVT_RING_REF,
^~~~~~~~~~~~~~~~~~~~~~~~~
XENSND_FIELD_RING_REF
>> sound/xen/xen_snd_front_evtchnl.c:413:12: error: 'XENSND_FIELD_EVT_EVT_CHNL' undeclared (first use in this function); did you mean 'XENSND_FIELD_EVT_CHNL'?
XENSND_FIELD_EVT_EVT_CHNL);
^~~~~~~~~~~~~~~~~~~~~~~~~
XENSND_FIELD_EVT_CHNL
cc1: some warnings being treated as errors
sparse warnings: (new ones prefixed by >>)
sound/xen/xen_snd_front_evtchnl.c:51:22: sparse: undefined identifier 'XENSND_OP_TRIGGER'
sound/xen/xen_snd_front_evtchnl.c:55:22: sparse: undefined identifier 'XENSND_OP_HW_PARAM_QUERY'
sound/xen/xen_snd_front_evtchnl.c:58:45: sparse: no member 'resp' in struct xensnd_resp
>> sound/xen/xen_snd_front_evtchnl.c:51:22: sparse: incompatible types for 'case' statement
sound/xen/xen_snd_front_evtchnl.c:55:22: sparse: incompatible types for 'case' statement
sound/xen/xen_snd_front_evtchnl.c:99:20: sparse: using member 'in_prod' in incomplete struct xensnd_event_page
sound/xen/xen_snd_front_evtchnl.c:102:25: sparse: using member 'in_cons' in incomplete struct xensnd_event_page
sound/xen/xen_snd_front_evtchnl.c:105:25: sparse: using member 'in_cons' in incomplete struct xensnd_event_page
sound/xen/xen_snd_front_evtchnl.c:108:26: sparse: undefined identifier 'XENSND_IN_RING_REF'
sound/xen/xen_snd_front_evtchnl.c:109:21: sparse: using member 'id' in incomplete struct xensnd_evt
sound/xen/xen_snd_front_evtchnl.c:112:30: sparse: using member 'type' in incomplete struct xensnd_evt
sound/xen/xen_snd_front_evtchnl.c:113:22: sparse: undefined identifier 'XENSND_EVT_CUR_POS'
sound/xen/xen_snd_front_evtchnl.c:113:22: sparse: incompatible types for 'case' statement
sound/xen/xen_snd_front_evtchnl.c:119:13: sparse: using member 'in_cons' in incomplete struct xensnd_event_page
sound/xen/xen_snd_front_evtchnl.c:213:34: sparse: undefined identifier 'XENSND_FIELD_EVT_RING_REF'
sound/xen/xen_snd_front_evtchnl.c:412:47: sparse: undefined identifier 'XENSND_FIELD_EVT_RING_REF'
sound/xen/xen_snd_front_evtchnl.c:432:47: sparse: undefined identifier 'XENSND_FIELD_EVT_RING_REF'
sound/xen/xen_snd_front_evtchnl.c:51:22: sparse: Expected constant expression in case statement
sound/xen/xen_snd_front_evtchnl.c:55:22: sparse: Expected constant expression in case statement
sound/xen/xen_snd_front_evtchnl.c:113:22: sparse: Expected constant expression in case statement
In file included from sound/xen/xen_snd_front_evtchnl.c:18:0:
sound/xen/xen_snd_front_evtchnl.h:62:34: error: field 'hw_param' has incomplete type
struct xensnd_query_hw_param hw_param;
^~~~~~~~
sound/xen/xen_snd_front_evtchnl.c: In function 'evtchnl_interrupt_req':
sound/xen/xen_snd_front_evtchnl.c:51:8: error: 'XENSND_OP_TRIGGER' undeclared (first use in this function); did you mean 'XENSND_OP_WRITE'?
case XENSND_OP_TRIGGER:
^~~~~~~~~~~~~~~~~
XENSND_OP_WRITE
sound/xen/xen_snd_front_evtchnl.c:51:8: note: each undeclared identifier is reported only once for each function it appears in
sound/xen/xen_snd_front_evtchnl.c:55:8: error: 'XENSND_OP_HW_PARAM_QUERY' undeclared (first use in this function); did you mean 'XENSND_OP_TRIGGER'?
case XENSND_OP_HW_PARAM_QUERY:
^~~~~~~~~~~~~~~~~~~~~~~~
XENSND_OP_TRIGGER
sound/xen/xen_snd_front_evtchnl.c:58:10: error: 'struct xensnd_resp' has no member named 'resp'
resp->resp.hw_param;
^~
sound/xen/xen_snd_front_evtchnl.c: In function 'evtchnl_interrupt_evt':
sound/xen/xen_snd_front_evtchnl.c:99:13: error: dereferencing pointer to incomplete type 'struct xensnd_event_page'
prod = page->in_prod;
^~
sound/xen/xen_snd_front_evtchnl.c:108:12: error: implicit declaration of function 'XENSND_IN_RING_REF'; did you mean 'XENSND_FIELD_RING_REF'? [-Werror=implicit-function-declaration]
event = &XENSND_IN_RING_REF(page, cons);
^~~~~~~~~~~~~~~~~~
XENSND_FIELD_RING_REF
sound/xen/xen_snd_front_evtchnl.c:108:11: error: lvalue required as unary '&' operand
event = &XENSND_IN_RING_REF(page, cons);
^
In file included from include/linux/kernel.h:10:0,
from include/linux/interrupt.h:6,
from include/xen/events.h:5,
from sound/xen/xen_snd_front_evtchnl.c:11:
sound/xen/xen_snd_front_evtchnl.c:109:21: error: dereferencing pointer to incomplete type 'struct xensnd_evt'
if (unlikely(event->id != channel->evt_id++))
^
include/linux/compiler.h:77:42: note: in definition of macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
^
sound/xen/xen_snd_front_evtchnl.c:113:8: error: 'XENSND_EVT_CUR_POS' undeclared (first use in this function); did you mean 'XENSND_OP_CLOSE'?
case XENSND_EVT_CUR_POS:
^~~~~~~~~~~~~~~~~~
XENSND_OP_CLOSE
sound/xen/xen_snd_front_evtchnl.c: In function 'evtchnl_alloc':
sound/xen/xen_snd_front_evtchnl.c:213:6: error: 'XENSND_FIELD_EVT_RING_REF' undeclared (first use in this function); did you mean 'XENSND_FIELD_RING_REF'?
XENSND_FIELD_EVT_RING_REF);
^~~~~~~~~~~~~~~~~~~~~~~~~
XENSND_FIELD_RING_REF
sound/xen/xen_snd_front_evtchnl.c: In function 'xen_snd_front_evtchnl_publish_all':
sound/xen/xen_snd_front_evtchnl.c:412:12: error: 'XENSND_FIELD_EVT_RING_REF' undeclared (first use in this function); did you mean 'XENSND_FIELD_RING_REF'?
XENSND_FIELD_EVT_RING_REF,
^~~~~~~~~~~~~~~~~~~~~~~~~
XENSND_FIELD_RING_REF
sound/xen/xen_snd_front_evtchnl.c:413:12: error: 'XENSND_FIELD_EVT_EVT_CHNL' undeclared (first use in this function); did you mean 'XENSND_FIELD_EVT_CHNL'?
XENSND_FIELD_EVT_EVT_CHNL);
^~~~~~~~~~~~~~~~~~~~~~~~~
XENSND_FIELD_EVT_CHNL
cc1: some warnings being treated as errors
vim +/hw_param +62 sound/xen/xen_snd_front_evtchnl.h
39
40 struct xen_snd_front_evtchnl {
41 struct xen_snd_front_info *front_info;
42 int gref;
43 int port;
44 int irq;
45 int index;
46 /* state of the event channel */
47 enum xen_snd_front_evtchnl_state state;
48 enum xen_snd_front_evtchnl_type type;
49 /* either response id or incoming event id */
50 u16 evt_id;
51 /* next request id or next expected event id */
52 u16 evt_next_id;
53 union {
54 struct {
55 struct xen_sndif_front_ring ring;
56 struct completion completion;
57 /* latest response status */
58 int resp_status;
59 /* serializer for backend IO: request/response */
60 struct mutex req_io_lock;
61 union {
> 62 struct xensnd_query_hw_param hw_param;
63 } resp;
64 } req;
65 struct {
66 struct xensnd_event_page *page;
67 /* this is needed to handle XENSND_EVT_CUR_POS event */
68 struct snd_pcm_substream *substream;
69 } evt;
70 } u;
71 };
72
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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
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. Juergen
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
On Mon, 16 Apr 2018 08:24:51 +0200, Oleksandr Andrushchenko wrote: > +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++) { I'm not familiar with Xen stuff in general, but through a quick glance, this kind of code worries me a bit. If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a very long loop, no? Better to have a sanity check of the ring buffer size. > +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++) { Ditto. thanks, Takashi
On 04/24/2018 05:20 PM, Takashi Iwai wrote: > On Mon, 16 Apr 2018 08:24:51 +0200, > Oleksandr Andrushchenko wrote: >> +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++) { > I'm not familiar with Xen stuff in general, but through a quick > glance, this kind of code worries me a bit. > > If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a > very long loop, no? Better to have a sanity check of the ring buffer > size. In this loop I have: resp = RING_GET_RESPONSE(&channel->u.req.ring, i); and the RING_GET_RESPONSE macro is designed in the way that it wraps around when *i* in the question gets bigger than the ring size: #define RING_GET_REQUEST(_r, _idx) \ (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) So, even if the counter has a bogus number it will not last long >> +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++) { > Ditto. Same as above > > thanks, > > Takashi Thank you, Oleksandr
On Tue, 24 Apr 2018 16:29:15 +0200, Oleksandr Andrushchenko wrote: > > On 04/24/2018 05:20 PM, Takashi Iwai wrote: > > On Mon, 16 Apr 2018 08:24:51 +0200, > > Oleksandr Andrushchenko wrote: > >> +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++) { > > I'm not familiar with Xen stuff in general, but through a quick > > glance, this kind of code worries me a bit. > > > > If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a > > very long loop, no? Better to have a sanity check of the ring buffer > > size. > In this loop I have: > resp = RING_GET_RESPONSE(&channel->u.req.ring, i); > and the RING_GET_RESPONSE macro is designed in the way that > it wraps around when *i* in the question gets bigger than > the ring size: > > #define RING_GET_REQUEST(_r, _idx) \ > (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) > > So, even if the counter has a bogus number it will not last long Hm, this prevents from accessing outside the ring buffer, but does it change the loop behavior? Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below would still consume the whole 32bit counts, no? for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { resp = RING_GET_RESPONSE(&channel->u.req.ring, i); ... } Takashi
On 04/24/2018 05:35 PM, Takashi Iwai wrote: > On Tue, 24 Apr 2018 16:29:15 +0200, > Oleksandr Andrushchenko wrote: >> On 04/24/2018 05:20 PM, Takashi Iwai wrote: >>> On Mon, 16 Apr 2018 08:24:51 +0200, >>> Oleksandr Andrushchenko wrote: >>>> +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++) { >>> I'm not familiar with Xen stuff in general, but through a quick >>> glance, this kind of code worries me a bit. >>> >>> If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a >>> very long loop, no? Better to have a sanity check of the ring buffer >>> size. >> In this loop I have: >> resp = RING_GET_RESPONSE(&channel->u.req.ring, i); >> and the RING_GET_RESPONSE macro is designed in the way that >> it wraps around when *i* in the question gets bigger than >> the ring size: >> >> #define RING_GET_REQUEST(_r, _idx) \ >> (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) >> >> So, even if the counter has a bogus number it will not last long > Hm, this prevents from accessing outside the ring buffer, but does it > change the loop behavior? no, it doesn't > Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below > would still consume the whole 32bit counts, no? > > for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { > resp = RING_GET_RESPONSE(&channel->u.req.ring, i); > ... > } You are right here and the comment is totally valid. I'll put an additional check like here [1] and here [2] Will this address your comment? > > Takashi Thank you, Oleksandr [1] https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1127 [2] https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1135
On Tue, 24 Apr 2018 16:58:43 +0200, Oleksandr Andrushchenko wrote: > > On 04/24/2018 05:35 PM, Takashi Iwai wrote: > > On Tue, 24 Apr 2018 16:29:15 +0200, > > Oleksandr Andrushchenko wrote: > >> On 04/24/2018 05:20 PM, Takashi Iwai wrote: > >>> On Mon, 16 Apr 2018 08:24:51 +0200, > >>> Oleksandr Andrushchenko wrote: > >>>> +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++) { > >>> I'm not familiar with Xen stuff in general, but through a quick > >>> glance, this kind of code worries me a bit. > >>> > >>> If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a > >>> very long loop, no? Better to have a sanity check of the ring buffer > >>> size. > >> In this loop I have: > >> resp = RING_GET_RESPONSE(&channel->u.req.ring, i); > >> and the RING_GET_RESPONSE macro is designed in the way that > >> it wraps around when *i* in the question gets bigger than > >> the ring size: > >> > >> #define RING_GET_REQUEST(_r, _idx) \ > >> (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) > >> > >> So, even if the counter has a bogus number it will not last long > > Hm, this prevents from accessing outside the ring buffer, but does it > > change the loop behavior? > no, it doesn't > > Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below > > would still consume the whole 32bit counts, no? > > > > for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { > > resp = RING_GET_RESPONSE(&channel->u.req.ring, i); > > ... > > } > You are right here and the comment is totally valid. > I'll put an additional check like here [1] and here [2] > Will this address your comment? Yep, this kind of sanity checks should work. thanks, Takashi > > > > Takashi > Thank you, > Oleksandr > > [1] > https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1127 > [2] > https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1135 >
On 04/24/2018 06:02 PM, Takashi Iwai wrote: > On Tue, 24 Apr 2018 16:58:43 +0200, > Oleksandr Andrushchenko wrote: >> On 04/24/2018 05:35 PM, Takashi Iwai wrote: >>> On Tue, 24 Apr 2018 16:29:15 +0200, >>> Oleksandr Andrushchenko wrote: >>>> On 04/24/2018 05:20 PM, Takashi Iwai wrote: >>>>> On Mon, 16 Apr 2018 08:24:51 +0200, >>>>> Oleksandr Andrushchenko wrote: >>>>>> +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++) { >>>>> I'm not familiar with Xen stuff in general, but through a quick >>>>> glance, this kind of code worries me a bit. >>>>> >>>>> If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a >>>>> very long loop, no? Better to have a sanity check of the ring buffer >>>>> size. >>>> In this loop I have: >>>> resp = RING_GET_RESPONSE(&channel->u.req.ring, i); >>>> and the RING_GET_RESPONSE macro is designed in the way that >>>> it wraps around when *i* in the question gets bigger than >>>> the ring size: >>>> >>>> #define RING_GET_REQUEST(_r, _idx) \ >>>> (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) >>>> >>>> So, even if the counter has a bogus number it will not last long >>> Hm, this prevents from accessing outside the ring buffer, but does it >>> change the loop behavior? >> no, it doesn't >>> Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below >>> would still consume the whole 32bit counts, no? >>> >>> for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { >>> resp = RING_GET_RESPONSE(&channel->u.req.ring, i); >>> ... >>> } >> You are right here and the comment is totally valid. >> I'll put an additional check like here [1] and here [2] >> Will this address your comment? > Yep, this kind of sanity checks should work. > Great, will implement the checks this way then > thanks, > > Takashi Thank you, Oleksandr >>> Takashi >> Thank you, >> Oleksandr >> >> [1] >> https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1127 >> [2] >> https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1135 >>
On 04/24/2018 07:23 PM, Oleksandr Andrushchenko wrote: > On 04/24/2018 06:02 PM, Takashi Iwai wrote: >> On Tue, 24 Apr 2018 16:58:43 +0200, >> Oleksandr Andrushchenko wrote: >>> On 04/24/2018 05:35 PM, Takashi Iwai wrote: >>>> On Tue, 24 Apr 2018 16:29:15 +0200, >>>> Oleksandr Andrushchenko wrote: >>>>> On 04/24/2018 05:20 PM, Takashi Iwai wrote: >>>>>> On Mon, 16 Apr 2018 08:24:51 +0200, >>>>>> Oleksandr Andrushchenko wrote: >>>>>>> +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++) { >>>>>> I'm not familiar with Xen stuff in general, but through a quick >>>>>> glance, this kind of code worries me a bit. >>>>>> >>>>>> If channel->u.req.ring.rsp_cons has a bogus number, this may lead >>>>>> to a >>>>>> very long loop, no? Better to have a sanity check of the ring >>>>>> buffer >>>>>> size. >>>>> In this loop I have: >>>>> resp = RING_GET_RESPONSE(&channel->u.req.ring, i); >>>>> and the RING_GET_RESPONSE macro is designed in the way that >>>>> it wraps around when *i* in the question gets bigger than >>>>> the ring size: >>>>> >>>>> #define RING_GET_REQUEST(_r, _idx) \ >>>>> (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) >>>>> >>>>> So, even if the counter has a bogus number it will not last long >>>> Hm, this prevents from accessing outside the ring buffer, but does it >>>> change the loop behavior? >>> no, it doesn't >>>> Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below >>>> would still consume the whole 32bit counts, no? >>>> >>>> for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { >>>> resp = RING_GET_RESPONSE(&channel->u.req.ring, i); >>>> ... >>>> } >>> You are right here and the comment is totally valid. >>> I'll put an additional check like here [1] and here [2] >>> Will this address your comment? >> Yep, this kind of sanity checks should work. >> > Great, will implement the checks this way then Well, after thinking a bit more on that and chatting on #xendevel IRC with Juergen (he is on CC list), it seems that the way the code is now it is all fine without the checks: the assumption here is that the backend is trusted to always write sane values to the ring counters, thus no overflow checks on frontend side are required. Even if I implement the checks then I have no means to recover, but just print an error message and bail out not handling any responses. This is probably why the checks [1] and [2] are only implemented for the backend side and there are no such macros for the frontend side. Takashi, please let me know if the above sounds reasonable and addresses your comments. >> thanks, >> >> Takashi > Thank you, > Oleksandr >>>> Takashi >>> Thank you, >>> Oleksandr >>> >>> [1] >>> https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1127 >>> >>> [2] >>> https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1135 >>> >>> >
On Wed, 25 Apr 2018 10:26:34 +0200, Oleksandr Andrushchenko wrote: > > On 04/24/2018 07:23 PM, Oleksandr Andrushchenko wrote: > > On 04/24/2018 06:02 PM, Takashi Iwai wrote: > >> On Tue, 24 Apr 2018 16:58:43 +0200, > >> Oleksandr Andrushchenko wrote: > >>> On 04/24/2018 05:35 PM, Takashi Iwai wrote: > >>>> On Tue, 24 Apr 2018 16:29:15 +0200, > >>>> Oleksandr Andrushchenko wrote: > >>>>> On 04/24/2018 05:20 PM, Takashi Iwai wrote: > >>>>>> On Mon, 16 Apr 2018 08:24:51 +0200, > >>>>>> Oleksandr Andrushchenko wrote: > >>>>>>> +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++) { > >>>>>> I'm not familiar with Xen stuff in general, but through a quick > >>>>>> glance, this kind of code worries me a bit. > >>>>>> > >>>>>> If channel->u.req.ring.rsp_cons has a bogus number, this may > >>>>>> lead to a > >>>>>> very long loop, no? Better to have a sanity check of the ring > >>>>>> buffer > >>>>>> size. > >>>>> In this loop I have: > >>>>> resp = RING_GET_RESPONSE(&channel->u.req.ring, i); > >>>>> and the RING_GET_RESPONSE macro is designed in the way that > >>>>> it wraps around when *i* in the question gets bigger than > >>>>> the ring size: > >>>>> > >>>>> #define RING_GET_REQUEST(_r, _idx) \ > >>>>> (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) > >>>>> > >>>>> So, even if the counter has a bogus number it will not last long > >>>> Hm, this prevents from accessing outside the ring buffer, but does it > >>>> change the loop behavior? > >>> no, it doesn't > >>>> Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below > >>>> would still consume the whole 32bit counts, no? > >>>> > >>>> for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { > >>>> resp = RING_GET_RESPONSE(&channel->u.req.ring, i); > >>>> ... > >>>> } > >>> You are right here and the comment is totally valid. > >>> I'll put an additional check like here [1] and here [2] > >>> Will this address your comment? > >> Yep, this kind of sanity checks should work. > >> > > Great, will implement the checks this way then > Well, after thinking a bit more on that and chatting on #xendevel IRC > with Juergen (he is on CC list), it seems that the way the code is now > it is all fine without the checks: the assumption here is that > the backend is trusted to always write sane values to the ring counters, > thus no overflow checks on frontend side are required. > Even if I implement the checks then I have no means to recover, but > just print > an error message and bail out not handling any responses. > This is probably why the checks [1] and [2] are only implemented for the > backend side and there are no such macros for the frontend side. > > Takashi, please let me know if the above sounds reasonable and > addresses your comments. If it's guaranteed to work, that's OK. But maybe it's worth to comment for readers. thanks, Takashi
On 04/25/2018 12:02 PM, Takashi Iwai wrote: > On Wed, 25 Apr 2018 10:26:34 +0200, > Oleksandr Andrushchenko wrote: >> On 04/24/2018 07:23 PM, Oleksandr Andrushchenko wrote: >>> On 04/24/2018 06:02 PM, Takashi Iwai wrote: >>>> On Tue, 24 Apr 2018 16:58:43 +0200, >>>> Oleksandr Andrushchenko wrote: >>>>> On 04/24/2018 05:35 PM, Takashi Iwai wrote: >>>>>> On Tue, 24 Apr 2018 16:29:15 +0200, >>>>>> Oleksandr Andrushchenko wrote: >>>>>>> On 04/24/2018 05:20 PM, Takashi Iwai wrote: >>>>>>>> On Mon, 16 Apr 2018 08:24:51 +0200, >>>>>>>> Oleksandr Andrushchenko wrote: >>>>>>>>> +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++) { >>>>>>>> I'm not familiar with Xen stuff in general, but through a quick >>>>>>>> glance, this kind of code worries me a bit. >>>>>>>> >>>>>>>> If channel->u.req.ring.rsp_cons has a bogus number, this may >>>>>>>> lead to a >>>>>>>> very long loop, no? Better to have a sanity check of the ring >>>>>>>> buffer >>>>>>>> size. >>>>>>> In this loop I have: >>>>>>> resp = RING_GET_RESPONSE(&channel->u.req.ring, i); >>>>>>> and the RING_GET_RESPONSE macro is designed in the way that >>>>>>> it wraps around when *i* in the question gets bigger than >>>>>>> the ring size: >>>>>>> >>>>>>> #define RING_GET_REQUEST(_r, _idx) \ >>>>>>> (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) >>>>>>> >>>>>>> So, even if the counter has a bogus number it will not last long >>>>>> Hm, this prevents from accessing outside the ring buffer, but does it >>>>>> change the loop behavior? >>>>> no, it doesn't >>>>>> Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below >>>>>> would still consume the whole 32bit counts, no? >>>>>> >>>>>> for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { >>>>>> resp = RING_GET_RESPONSE(&channel->u.req.ring, i); >>>>>> ... >>>>>> } >>>>> You are right here and the comment is totally valid. >>>>> I'll put an additional check like here [1] and here [2] >>>>> Will this address your comment? >>>> Yep, this kind of sanity checks should work. >>>> >>> Great, will implement the checks this way then >> Well, after thinking a bit more on that and chatting on #xendevel IRC >> with Juergen (he is on CC list), it seems that the way the code is now >> it is all fine without the checks: the assumption here is that >> the backend is trusted to always write sane values to the ring counters, >> thus no overflow checks on frontend side are required. >> Even if I implement the checks then I have no means to recover, but >> just print >> an error message and bail out not handling any responses. >> This is probably why the checks [1] and [2] are only implemented for the >> backend side and there are no such macros for the frontend side. >> >> Takashi, please let me know if the above sounds reasonable and >> addresses your comments. > If it's guaranteed to work, that's OK. > But maybe it's worth to comment for readers. ok, will put a comment on that > > thanks, > > Takashi Thank you, Oleksandr
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" 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); + + 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); + 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; + goto fail; + } + + if (type == EVTCHNL_TYPE_REQ) { + struct xen_sndif_sring *sring = (struct xen_sndif_sring *)page; + + init_completion(&channel->u.req.completion); + mutex_init(&channel->u.req.req_io_lock); + SHARED_RING_INIT(sring); + FRONT_RING_INIT(&channel->u.req.ring, sring, XEN_PAGE_SIZE); + + ret = xenbus_grant_ring(xb_dev, sring, 1, &gref); + if (ret < 0) { + channel->u.req.ring.sring = NULL; + free_page(page); + goto fail; + } + + handler = evtchnl_interrupt_req; + } else { + ret = gnttab_grant_foreign_access(xb_dev->otherend_id, + virt_to_gfn((void *)page), 0); + if (ret < 0) { + free_page(page); + goto fail; + } + + channel->u.evt.page = (struct xensnd_event_page *)page; + gref = ret; + handler = evtchnl_interrupt_evt; + } + + channel->gref = gref; + + ret = xenbus_alloc_evtchn(xb_dev, &channel->port); + if (ret < 0) + goto fail; + + ret = bind_evtchn_to_irq(channel->port); + if (ret < 0) { + dev_err(&xb_dev->dev, + "Failed to bind IRQ for domid %d port %d: %d\n", + front_info->xb_dev->otherend_id, channel->port, ret); + goto fail; + } + + channel->irq = ret; + + ret = request_threaded_irq(channel->irq, NULL, handler, + IRQF_ONESHOT, handler_name, channel); + if (ret < 0) { + dev_err(&xb_dev->dev, "Failed to request IRQ %d: %d\n", + channel->irq, ret); + goto fail; + } + + kfree(handler_name); + return 0; + +fail: + kfree(handler_name); + dev_err(&xb_dev->dev, "Failed to allocate ring: %d\n", ret); + return ret; +} + +int xen_snd_front_evtchnl_create_all(struct xen_snd_front_info *front_info, + int num_streams) +{ + struct xen_front_cfg_card *cfg = &front_info->cfg; + struct device *dev = &front_info->xb_dev->dev; + int d, ret = 0; + + front_info->evt_pairs = + kcalloc(num_streams, + sizeof(struct xen_snd_front_evtchnl_pair), + GFP_KERNEL); + if (!front_info->evt_pairs) + return -ENOMEM; + + /* iterate over devices and their streams and create event channels */ + for (d = 0; d < cfg->num_pcm_instances; d++) { + struct xen_front_cfg_pcm_instance *pcm_instance; + int s, index; + + pcm_instance = &cfg->pcm_instances[d]; + + for (s = 0; s < pcm_instance->num_streams_pb; s++) { + index = pcm_instance->streams_pb[s].index; + + ret = evtchnl_alloc(front_info, index, + &front_info->evt_pairs[index].req, + EVTCHNL_TYPE_REQ); + if (ret < 0) { + dev_err(dev, "Error allocating control channel\n"); + goto fail; + } + + ret = evtchnl_alloc(front_info, index, + &front_info->evt_pairs[index].evt, + EVTCHNL_TYPE_EVT); + if (ret < 0) { + dev_err(dev, "Error allocating in-event channel\n"); + goto fail; + } + } + + for (s = 0; s < pcm_instance->num_streams_cap; s++) { + index = pcm_instance->streams_cap[s].index; + + ret = evtchnl_alloc(front_info, index, + &front_info->evt_pairs[index].req, + EVTCHNL_TYPE_REQ); + if (ret < 0) { + dev_err(dev, "Error allocating control channel\n"); + goto fail; + } + + ret = evtchnl_alloc(front_info, index, + &front_info->evt_pairs[index].evt, + EVTCHNL_TYPE_EVT); + if (ret < 0) { + dev_err(dev, "Error allocating in-event channel\n"); + goto fail; + } + } + } + if (ret < 0) + goto fail; + + front_info->num_evt_pairs = num_streams; + return 0; + +fail: + xen_snd_front_evtchnl_free_all(front_info); + return ret; +} + +static int evtchnl_publish(struct xenbus_transaction xbt, + struct xen_snd_front_evtchnl *channel, + const char *path, const char *node_ring, + const char *node_chnl) +{ + struct xenbus_device *xb_dev = channel->front_info->xb_dev; + int ret; + + /* write control channel ring reference */ + ret = xenbus_printf(xbt, path, node_ring, "%u", channel->gref); + if (ret < 0) { + dev_err(&xb_dev->dev, "Error writing ring-ref: %d\n", ret); + return ret; + } + + /* write event channel ring reference */ + ret = xenbus_printf(xbt, path, node_chnl, "%u", channel->port); + if (ret < 0) { + dev_err(&xb_dev->dev, "Error writing event channel: %d\n", ret); + return ret; + } + + return 0; +} + +int xen_snd_front_evtchnl_publish_all(struct xen_snd_front_info *front_info) +{ + struct xen_front_cfg_card *cfg = &front_info->cfg; + struct xenbus_transaction xbt; + int ret, d; + +again: + ret = xenbus_transaction_start(&xbt); + if (ret < 0) { + xenbus_dev_fatal(front_info->xb_dev, ret, + "starting transaction"); + return ret; + } + + for (d = 0; d < cfg->num_pcm_instances; d++) { + struct xen_front_cfg_pcm_instance *pcm_instance; + int s, index; + + pcm_instance = &cfg->pcm_instances[d]; + + for (s = 0; s < pcm_instance->num_streams_pb; s++) { + index = pcm_instance->streams_pb[s].index; + + ret = evtchnl_publish(xbt, + &front_info->evt_pairs[index].req, + pcm_instance->streams_pb[s].xenstore_path, + XENSND_FIELD_RING_REF, + XENSND_FIELD_EVT_CHNL); + if (ret < 0) + goto fail; + + ret = evtchnl_publish(xbt, + &front_info->evt_pairs[index].evt, + pcm_instance->streams_pb[s].xenstore_path, + XENSND_FIELD_EVT_RING_REF, + XENSND_FIELD_EVT_EVT_CHNL); + if (ret < 0) + goto fail; + } + + for (s = 0; s < pcm_instance->num_streams_cap; s++) { + index = pcm_instance->streams_cap[s].index; + + ret = evtchnl_publish(xbt, + &front_info->evt_pairs[index].req, + pcm_instance->streams_cap[s].xenstore_path, + XENSND_FIELD_RING_REF, + XENSND_FIELD_EVT_CHNL); + if (ret < 0) + goto fail; + + ret = evtchnl_publish(xbt, + &front_info->evt_pairs[index].evt, + pcm_instance->streams_cap[s].xenstore_path, + XENSND_FIELD_EVT_RING_REF, + XENSND_FIELD_EVT_EVT_CHNL); + if (ret < 0) + goto fail; + } + } + ret = xenbus_transaction_end(xbt, 0); + if (ret < 0) { + if (ret == -EAGAIN) + goto again; + + xenbus_dev_fatal(front_info->xb_dev, ret, + "completing transaction"); + goto fail_to_end; + } + return 0; +fail: + xenbus_transaction_end(xbt, 1); +fail_to_end: + xenbus_dev_fatal(front_info->xb_dev, ret, "writing XenStore"); + return ret; +} + +void xen_snd_front_evtchnl_pair_set_connected(struct xen_snd_front_evtchnl_pair *evt_pair, + bool is_connected) +{ + enum xen_snd_front_evtchnl_state state; + + if (is_connected) + state = EVTCHNL_STATE_CONNECTED; + else + state = EVTCHNL_STATE_DISCONNECTED; + + evt_pair->req.state = state; + evt_pair->evt.state = state; +} + +void xen_snd_front_evtchnl_pair_clear(struct xen_snd_front_evtchnl_pair *evt_pair) +{ + evt_pair->req.evt_next_id = 0; + evt_pair->evt.evt_next_id = 0; +} + diff --git a/sound/xen/xen_snd_front_evtchnl.h b/sound/xen/xen_snd_front_evtchnl.h new file mode 100644 index 000000000000..c4921f469347 --- /dev/null +++ b/sound/xen/xen_snd_front_evtchnl.h @@ -0,0 +1,92 @@ +/* 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> + */ + +#ifndef __XEN_SND_FRONT_EVTCHNL_H +#define __XEN_SND_FRONT_EVTCHNL_H + +#include <xen/interface/io/sndif.h> + +struct xen_snd_front_info; + +#ifndef GRANT_INVALID_REF +/* + * FIXME: usage of grant reference 0 as invalid grant reference: + * grant reference 0 is valid, but never exposed to a PV driver, + * because of the fact it is already in use/reserved by the PV console. + */ +#define GRANT_INVALID_REF 0 +#endif + +/* timeout in ms to wait for backend to respond */ +#define VSND_WAIT_BACK_MS 3000 + +enum xen_snd_front_evtchnl_state { + EVTCHNL_STATE_DISCONNECTED, + EVTCHNL_STATE_CONNECTED, +}; + +enum xen_snd_front_evtchnl_type { + EVTCHNL_TYPE_REQ, + EVTCHNL_TYPE_EVT, +}; + +struct xen_snd_front_evtchnl { + struct xen_snd_front_info *front_info; + int gref; + int port; + int irq; + int index; + /* state of the event channel */ + enum xen_snd_front_evtchnl_state state; + enum xen_snd_front_evtchnl_type type; + /* either response id or incoming event id */ + u16 evt_id; + /* next request id or next expected event id */ + u16 evt_next_id; + union { + struct { + struct xen_sndif_front_ring ring; + struct completion completion; + /* latest response status */ + int resp_status; + /* serializer for backend IO: request/response */ + struct mutex req_io_lock; + union { + struct xensnd_query_hw_param hw_param; + } resp; + } req; + struct { + struct xensnd_event_page *page; + /* this is needed to handle XENSND_EVT_CUR_POS event */ + struct snd_pcm_substream *substream; + } evt; + } u; +}; + +struct xen_snd_front_evtchnl_pair { + struct xen_snd_front_evtchnl req; + struct xen_snd_front_evtchnl evt; +}; + +int xen_snd_front_evtchnl_create_all(struct xen_snd_front_info *front_info, + int num_streams); + +void xen_snd_front_evtchnl_free_all(struct xen_snd_front_info *front_info); + +int xen_snd_front_evtchnl_publish_all(struct xen_snd_front_info *front_info); + +void xen_snd_front_evtchnl_flush(struct xen_snd_front_evtchnl *evtchnl); + +void xen_snd_front_evtchnl_pair_set_connected(struct xen_snd_front_evtchnl_pair *evt_pair, + bool is_connected); + +void xen_snd_front_evtchnl_pair_clear(struct xen_snd_front_evtchnl_pair *evt_pair); + +#endif /* __XEN_SND_FRONT_EVTCHNL_H */