| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <53DA7DBD.10704@arm.com>
Date: Thu, 31 Jul 2014 18:32:45 +0100
From: Sudeep Holla <sudeep.holla@....com>
To: Jassi Brar <jaswinder.singh@...aro.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: Sudeep Holla <sudeep.holla@....com>,
"ks.giri@...sung.com" <ks.giri@...sung.com>,
"arnd@...db.de" <arnd@...db.de>,
"ijc@...lion.org.uk" <ijc@...lion.org.uk>,
Mark Rutland <Mark.Rutland@....com>,
"robh@...nel.org" <robh@...nel.org>,
Pawel Moll <Pawel.Moll@....com>,
"courtney.cavin@...ymobile.com" <courtney.cavin@...ymobile.com>,
"mporter@...aro.org" <mporter@...aro.org>,
"slapdau@...oo.com.au" <slapdau@...oo.com.au>,
"lftan.linux@...il.com" <lftan.linux@...il.com>,
"loic.pallardy@...com" <loic.pallardy@...com>,
"s-anna@...com" <s-anna@...com>,
"ashwin.chaugule@...aro.org" <ashwin.chaugule@...aro.org>,
"bjorn@...o.se" <bjorn@...o.se>,
"patches@...aro.org" <patches@...aro.org>,
"t.takinishi@...fujitsu.com" <t.takinishi@...fujitsu.com>,
"broonie@...aro.org" <broonie@...aro.org>,
"khilman@...aro.org" <khilman@...aro.org>,
"mollie.wu@...aro.org" <mollie.wu@...aro.org>,
"andy.green@...aro.org" <andy.green@...aro.org>,
"lee.jones@...aro.org" <lee.jones@...aro.org>
Subject: Re: [PATCHv9 2/4] mailbox: Introduce framework for mailbox
On 22/07/14 19:56, Jassi Brar wrote:
> Introduce common framework for client/protocol drivers and
> controller drivers of Inter-Processor-Communication (IPC).
>
> Client driver developers should have a look at
> include/linux/mailbox_client.h to understand the part of
> the API exposed to client drivers.
> Similarly controller driver developers should have a look
> at include/linux/mailbox_controller.h
>
> Signed-off-by: Jassi Brar <jaswinder.singh@...aro.org>
> ---
> MAINTAINERS | 8 +
> drivers/mailbox/Makefile | 4 +
> drivers/mailbox/mailbox.c | 467 +++++++++++++++++++++++++++++++++++++
> include/linux/mailbox_client.h | 45 ++++
> include/linux/mailbox_controller.h | 131 +++++++++++
> 5 files changed, 655 insertions(+)
> create mode 100644 drivers/mailbox/mailbox.c
> create mode 100644 include/linux/mailbox_client.h
> create mode 100644 include/linux/mailbox_controller.h
>
[...]
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> new file mode 100644
> index 0000000..99c0d23
> --- /dev/null
> +++ b/drivers/mailbox/mailbox.c
> @@ -0,0 +1,467 @@
> +/*
> + * Mailbox: Common code for Mailbox controllers and users
> + *
> + * Copyright (C) 2013-2014 Linaro Ltd.
> + * Author: Jassi Brar <jassisinghbrar@...il.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/spinlock.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mailbox_controller.h>
> +
> +#define TXDONE_BY_IRQ (1 << 0) /* controller has remote RTR irq */
> +#define TXDONE_BY_POLL (1 << 1) /* controller can read status of last TX */
> +#define TXDONE_BY_ACK (1 << 2) /* S/W ACK recevied by Client ticks the TX */
> +
> +static LIST_HEAD(mbox_cons);
> +static DEFINE_MUTEX(con_mutex);
> +
> +static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
> +{
> + int idx;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&chan->lock, flags);
> +
> + /* See if there is any space left */
> + if (chan->msg_count == MBOX_TX_QUEUE_LEN) {
> + spin_unlock_irqrestore(&chan->lock, flags);
> + return -ENOMEM;
> + }
> +
> + idx = chan->msg_free;
> + chan->msg_data[idx] = mssg;
> + chan->msg_count++;
> +
> + if (idx == MBOX_TX_QUEUE_LEN - 1)
> + chan->msg_free = 0;
> + else
> + chan->msg_free++;
> +
> + spin_unlock_irqrestore(&chan->lock, flags);
> +
> + return idx;
> +}
> +
> +static void msg_submit(struct mbox_chan *chan)
> +{
> + unsigned count, idx;
> + unsigned long flags;
> + void *data;
> + int err;
> +
> + spin_lock_irqsave(&chan->lock, flags);
> +
> + if (!chan->msg_count || chan->active_req) {
> + spin_unlock_irqrestore(&chan->lock, flags);
> + return;
> + }
> +
> + count = chan->msg_count;
> + idx = chan->msg_free;
> + if (idx >= count)
> + idx -= count;
> + else
> + idx += MBOX_TX_QUEUE_LEN - count;
> +
> + data = chan->msg_data[idx];
> +
> + /* Try to submit a message to the MBOX controller */
> + err = chan->mbox->ops->send_data(chan, data);
Probably this is discussed already, but again I missed to find it
in archives, so asking here. If the protocol has some payload associated
with the message and controller expects it to be in place before calling
send_data, we essentially end up not using this queue at all by waiting
in the protocol layer(may be in it's own queue)
Instead can we have some kind of chan->cl->prepare_data callback so that
client can prepare payload ? This in turn avoids to implement it's
*own Tx queue* and *reusing the mailbox queue*.
Or am I missing something ?
> + if (!err) {
> + chan->active_req = data;
> + chan->msg_count--;
> + }
> +
> + spin_unlock_irqrestore(&chan->lock, flags);
> +}
> +
> +static void tx_tick(struct mbox_chan *chan, int r)
> +{
> + unsigned long flags;
> + void *mssg;
> +
> + spin_lock_irqsave(&chan->lock, flags);
> + mssg = chan->active_req;
> + chan->active_req = NULL;
> + spin_unlock_irqrestore(&chan->lock, flags);
> +
> + /* Submit next message */
> + msg_submit(chan);
> +
> + /* Notify the client */
> + if (mssg && chan->cl->tx_done)
> + chan->cl->tx_done(chan->cl, mssg, r);
> +
> + if (chan->cl->tx_block)
> + complete(&chan->tx_complete);
> +}
> +
> +static void poll_txdone(unsigned long data)
> +{
> + struct mbox_controller *mbox = (struct mbox_controller *)data;
> + bool txdone, resched = false;
> + int i;
> +
> + for (i = 0; i < mbox->num_chans; i++) {
> + struct mbox_chan *chan = &mbox->chans[i];
> +
> + if (chan->active_req && chan->cl) {
> + resched = true;
> + txdone = chan->mbox->ops->last_tx_done(chan);
> + if (txdone)
> + tx_tick(chan, 0);
What if all the active channels finished Tx(i.e. txdone = 1), we still have
resched = true and add a timer, not really harmful though. But IMO you can
move it to else part instead ?
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists