| 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: <CABb+yY1hK4T6KAQcqNKF3D_pkLCcDVN=+T18ueV35jXbpLR+kg@mail.gmail.com>
Date: Fri, 4 Mar 2016 10:48:22 +0530
From: Jassi Brar <jassisinghbrar@...il.com>
To: Nishanth Menon <nm@...com>
Cc: Rob Herring <robh+dt@...nel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Devicetree List <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Santosh Shilimkar <ssantosh@...nel.org>,
Franklin S Cooper Jr <fcooper@...com>,
Suman Anna <s-anna@...com>
Subject: Re: [PATCH V2 2/2] mailbox: Introduce TI message manager driver
On Sat, Feb 27, 2016 at 3:54 AM, Nishanth Menon <nm@...com> wrote:
> Support for TI Message Manager Module. This hardware block manages a
> bunch of hardware queues meant for communication between processor
> entities.
>
> Clients sitting on top of this would manage the required protocol
> for communicating with the counterpart entities.
>
> For more details on TI Message Manager hardware block, see documentation
> that will is available here: http://www.ti.com/lit/ug/spruhy8/spruhy8.pdf
> Chapter 8.1(Message Manager)
>
> Signed-off-by: Nishanth Menon <nm@...com>
> ---
> V2: Changes since V1:
> - Major refactoring of code for binding changes
> - moved list of valid K2G queueus into driver
> - split up probe into sub functions for easier maintenance
>
> V1: https://patchwork.kernel.org/patch/8237171/
>
> drivers/mailbox/Kconfig | 11 +
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/ti-msgmgr.c | 657
Do you want to call it something more specific than 'msgmgr from TI'?
Maybe its about Keystone, like mailbox-keystone.c ?
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/ti-msgmgr.h>
>
This seems a bit bold. I think include/linux/soc/ti/ is the right place.
> + /* Do I actually have messages to read? */
> + msg_count = ti_msgmgr_queue_get_num_messages(qinst);
> + if (!msg_count) {
> + /* Shared IRQ? */
> + dev_dbg(dev, "Spurious event - 0 pending data!\n");
> + return IRQ_NONE;
> + }
> +
> + /*
> + * I have no idea about the protocol being used to communicate with the
> + * remote producer - 0 could be valid data, so I wont make a judgement
> + * of how many bytes I should be reading. Let the client figure this
> + * out.. I just read the full message and pass it on..
> + */
Exactly. And similarly when you send data, you should not have more
than one message in transit. Now please see my note in
ti_msgmgr_send_data()
> +static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
> +{
> + struct device *dev = chan->mbox->dev;
> + struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
> + const struct ti_msgmgr_desc *desc;
> + struct ti_queue_inst *qinst = chan->con_priv;
> + int msg_count, num_words, trail_bytes;
> + struct ti_msgmgr_message *message = data;
> + void __iomem *data_reg;
> + u32 *word_data;
> +
> + if (WARN_ON(!inst)) {
> + dev_err(dev, "no platform drv data??\n");
> + return -EINVAL;
> + }
> + desc = inst->desc;
> +
> + if (desc->max_message_size < message->len) {
> + dev_err(dev, "Queue %s message length %d > max %d\n",
> + qinst->name, message->len, desc->max_message_size);
> + return -EINVAL;
> + }
> +
> + /* Are we able to send this or not? */
> + msg_count = ti_msgmgr_queue_get_num_messages(qinst);
> + if (msg_count >= desc->max_messages) {
> + dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
> + msg_count);
> + return -EBUSY;
> + }
This seems fishy. mailbox api always submit 1 'complete' message to be
sent and checks for completion by last_tx_done() before calling
send_data() again. Controller drivers are not supposed to queue
messages - mailbox core does. So you should never be unable to send a
message.
> + /*
> + * NOTE about register access involved here:
> + * the hardware block is implemented with 32bit access operations and no
> + * support for data splitting. We don't want the hardware to misbehave
> + * with sub 32bit access - For example: if the last register write is
> + * split into byte wise access, it can result in the queue getting
> + * stuck or dummy messages being transmitted or indeterminate behavior.
> + * The same can occur if out of order operations take place.
> + * Hence, we do not use memcpy_toio or ___iowrite32_copy here, instead
> + * we use writel which ensures the sequencing we need.
> + */
.... deja-vu ?
> +
> +/* Keystone K2G SoC integration details */
> +static const struct ti_msgmgr_valid_queue_desc k2g_valid_queues[] = {
> + {.queue_id = 0, .proxy_id = 0, .is_tx = true,},
> + {.queue_id = 1, .proxy_id = 0, .is_tx = true,},
> + {.queue_id = 2, .proxy_id = 0, .is_tx = true,},
> + {.queue_id = 3, .proxy_id = 0, .is_tx = true,},
> + {.queue_id = 5, .proxy_id = 2, .is_tx = false,},
> + {.queue_id = 56, .proxy_id = 1, .is_tx = true,},
> + {.queue_id = 57, .proxy_id = 2, .is_tx = false,},
> + {.queue_id = 58, .proxy_id = 3, .is_tx = true,},
> + {.queue_id = 59, .proxy_id = 4, .is_tx = true,},
> + {.queue_id = 60, .proxy_id = 5, .is_tx = true,},
> + {.queue_id = 61, .proxy_id = 6, .is_tx = true,},
> +};
> +
> +static const struct ti_msgmgr_desc k2g_desc = {
> + .queue_count = 64,
> + .max_message_size = 64,
> + .max_messages = 128,
> + .q_slices = 1,
> + .q_proxies = 1,
> + .data_first_reg = 16,
> + .data_last_reg = 31,
> + .tx_polled = false,
> + .valid_queues = k2g_valid_queues,
> + .num_valid_queues = ARRAY_SIZE(k2g_valid_queues),
> +};
If these parameters are very configurable, maybe they should be in DT?
Thanks.
Powered by blists - more mailing lists