Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

From: Lee Jones
Date: Thu Aug 13 2015 - 05:19:27 EST

On Thu, 13 Aug 2015, Jassi Brar wrote:

> On Wed, Aug 12, 2015 at 3:53 PM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> > On Mon, 10 Aug 2015, Jassi Brar wrote:
> >
> >> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> >> > This particular Client implementation uses shared memory in order
> >> > to pass messages between Mailbox users; however, it can be easily
> >> > hacked to support any type of Controller.
> >> >
> >> > Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
> >> > ---
> >> > drivers/mailbox/Kconfig | 7 ++
> >> > drivers/mailbox/Makefile | 2 +
> >> > drivers/mailbox/mailbox-test.c | 210 +++++++++++++++++++++++++++++++++++++++++
> >> > 3 files changed, 219 insertions(+)
> >> > create mode 100644 drivers/mailbox/mailbox-test.c
> >> >
> >> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> >> > index 2cc4c39..7720bde 100644
> >> > --- a/drivers/mailbox/Kconfig
> >> > +++ b/drivers/mailbox/Kconfig
> >> > @@ -77,4 +77,11 @@ config STI_MBOX
> >> > Mailbox implementation for STMicroelectonics family chips with
> >> > hardware for interprocessor communication.
> >> >
> >> > +config MAILBOX_TEST
> >> > + tristate "Mailbox Test Client"
> >> > + depends on OF
> >> > + help
> >> > + Test client to help with testing new Controller driver
> >> > + implementations.
> >> > +
> >> > endif
> >> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> >> > index 7cb4766..92435ef 100644
> >> > --- a/drivers/mailbox/Makefile
> >> > +++ b/drivers/mailbox/Makefile
> >> > @@ -2,6 +2,8 @@
> >> >
> >> > obj-$(CONFIG_MAILBOX) += mailbox.o
> >> >
> >> > +obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o
> >> > +
> >> > obj-$(CONFIG_ARM_MHU) += arm_mhu.o
> >> >
> >> > obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o
> >> > diff --git a/drivers/mailbox/mailbox-test.c b/drivers/mailbox/mailbox-test.c
> >> > new file mode 100644
> >> > index 0000000..10bfe3a
> >> > --- /dev/null
> >> > +++ b/drivers/mailbox/mailbox-test.c
> >> > @@ -0,0 +1,210 @@
> >> > +/*
> >> > + * Copyright (C) 2015 ST Microelectronics
> >> > + *
> >> > + * Author: Lee Jones <lee.jones@xxxxxxxxxx>
> >> > + *
> >> > + * This program is free software; you can redistribute it and/or modify
> >> > + * it under the terms of the GNU General Public License as published by
> >> > + * the Free Software Foundation; either version 2 of the License, or
> >> > + * (at your option) any later version.
> >> > + */
> >> > +
> >> > +#include <linux/debugfs.h>
> >> > +#include <linux/err.h>
> >> > +#include <linux/kernel.h>
> >> > +#include <linux/mailbox_client.h>
> >> > +#include <linux/module.h>
> >> > +#include <linux/of.h>
> >> > +#include <linux/platform_device.h>
> >> > +#include <linux/slab.h>
> >> > +#include <linux/uaccess.h>
> >> > +
> >> > +#define MBOX_MAX_MSG_LEN 128
> >> > +
> >> > +static struct dentry *root_debugfs_dir;
> >> > +
> >> > +struct mbox_test_device {
> >> > + struct device *dev;
> >> > + struct mbox_chan *tx_channel;
> >> > + struct mbox_chan *rx_channel;
> >> > + void __iomem *mmio;
> >> > +
> >> > +};
> >> > +
> >> > +static ssize_t mbox_test_write(struct file *filp,
> >> > + const char __user *userbuf,
> >> > + size_t count, loff_t *ppos)
> >> > +{
> >> > + struct mbox_test_device *tdev = filp->private_data;
> >> > + char *message;
> >> > + int ret;
> >> > +
> >> > + if (count > MBOX_MAX_MSG_LEN) {
> >> > + dev_err(tdev->dev,
> >> > + "Message length %d greater than max allowed %d\n",
> >> > + count, MBOX_MAX_MSG_LEN);
> >> > + return -EINVAL;
> >> > + }
> >> > +
> >> > + message = kzalloc(count, GFP_KERNEL);
> >> > + if (!message)
> >> > + return -ENOMEM;
> >> > +
> >> > + ret = copy_from_user(message, userbuf, count);
> >> > + if (ret)
> >> > + return -EFAULT;
> >> > +
> >> > + print_hex_dump(KERN_ERR, "Client: Sending: ", DUMP_PREFIX_ADDRESS,
> >> > + 16, 1, message, 16, true);
> >> > +
> >> > + ret = mbox_send_message(tdev->tx_channel, message);
> >> > + if (ret < 0)
> >> > + dev_err(tdev->dev, "Failed to send message via mailbox\n");
> >> > +
> >> > + kfree(message);
> >> > +
> >> > + return ret < 0 ? ret : count;
> >> > +}
> >> > +
> >> > +static const struct file_operations mbox_test_ops = {
> >> > + .write = mbox_test_write,
> >> > + .open = simple_open,
> >> > + .llseek = generic_file_llseek,
> >> > +};
> >> > +
> >> > +static int mbox_test_add_debugfs(struct platform_device *pdev,
> >> > + struct mbox_test_device *tdev)
> >> > +{
> >> > + if (!debugfs_initialized())
> >> > + return 0;
> >> > +
> >> > + root_debugfs_dir = debugfs_create_dir("mailbox", NULL);
> >> > + if (!root_debugfs_dir) {
> >> > + dev_err(&pdev->dev, "Failed to create Mailbox debugfs\n");
> >> > + return -EINVAL;
> >> > + }
> >> > +
> >> > + debugfs_create_file("send-message", 0200, root_debugfs_dir,
> >> > + tdev, &mbox_test_ops);
> >> > +
> >> > + return 0;
> >> > +}
> >> > +
> >> > +static void mbox_test_receive_message(struct mbox_client *client, void *message)
> >> > +{
> >> > + struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
> >> > +
> >> > + if (!tdev->mmio) {
> >> > + dev_info(tdev->dev, "Client: Recived something [read mmio]\n");
> >> > + return;
> >> > + }
> >> > +
> >> > + print_hex_dump(KERN_ERR, "Client: from co-proc: ", DUMP_PREFIX_ADDRESS,
> >> > + 16, 1, tdev->mmio, MBOX_MAX_MSG_LEN, true);
> >> > +}
> >> > +
> >> > +static void mbox_test_prepare_message(struct mbox_client *client, void *message)
> >> > +{
> >> > + struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
> >> > +
> >> > + if (tdev->mmio)
> >> > + memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
> >> >
> >> This is unlikely to work. Those platforms that need to send a 2 part
> >> message, they do :
> >> (a) Signal/Command/Target code via some controller register (via
> >> mbox_send_message).
> >> (b) Setup the payload in Shared-Memory (via tx_prepare).
> >>
> >> This test driver assumes both are the same. I think you have to
> >> declare something like
> >
> > This driver assumes that the framework will call client->tx_prepare()
> > first, which satisfies (b). It then assumes controller->send_data()
> > will be invoked, which will send the platform specific
> > signal/command/target code, which then satisfies (a).
> >
> Yeah, but what would be that code? Who provides that?
>
> > In what way does it assume they are the same?
> >
> notice the 'message' pointer in
> mbox_send_message(tdev->tx_channel, message);
> and
> memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
>
> >> struct mbox_test_message { /* same for TX and RX */
> >> unsigned sig_len;
> >> void *signal; /* rx/tx via mailbox api */
> >> unsigned pl_len;
> >> void *payload; /* rx/tx via shared-memory */
> >> };
> >
> > How do you think this should be set and use these?
> >
> The userspace would want to specify the command code (32bits or not)
> that would be passed via the fifo/register of the controller. So we
> need signal[]
>
> The data to be passed via share-memory could be provided by userspace
> via the payload[] array.

Okay, so would the solution be two userspace files 'signal' and
'message', so in the case of a client specified signal we can write it
into there first.

echo 255 > signal
echo test > message

... or in the case where no signal is required, or the controller
driver taking care of it, we just don't write anything to signal?

Just for clarification, in the case where a signal is required, do
clients usually pass that through mbox_send_message() too?

mbox_send_message(tdev->tx_channel, signal);
mbox_send_message(tdev->tx_channel, message);

... or vice versa.

> >> > +static void mbox_test_message_sent(struct mbox_client *client,
> >> > + void *message, int r)
> >> > +{
> >> > + if (r)
> >> > + dev_warn(client->dev,
> >> > + "Client: Message could not be sent: %d\n", r);
> >> > + else
> >> > + dev_info(client->dev,
> >> > + "Client: Message sent\n");
> >> > +}
> >> > +
> >> > +static struct mbox_chan *
> >> > +mbox_test_request_channel(struct platform_device *pdev, const char *name)
> >> > +{
> >> > + struct mbox_client *client;
> >> > +
> >> > + client = devm_kzalloc(&pdev->dev, sizeof(*client), GFP_KERNEL);
> >> > + if (!client)
> >> > + return ERR_PTR(-ENOMEM);
> >> > +
> >> > + client->dev = &pdev->dev;
> >> > + client->rx_callback = mbox_test_receive_message;
> >> > + client->tx_prepare = mbox_test_prepare_message;
> >> > + client->tx_done = mbox_test_message_sent;
> >> > + client->tx_block = true;
> >> > + client->knows_txdone = false;
> >> > + client->tx_tout = 500;
> >> > +
> >> > + return mbox_request_channel_byname(client, name);
> >> > +}
> >> > +
> >> > +static int mbox_test_probe(struct platform_device *pdev)
> >> > +{
> >> > + struct mbox_test_device *tdev;
> >> > + struct resource *res;
> >> > + int ret;
> >> > +
> >> > + tdev = devm_kzalloc(&pdev->dev, sizeof(*tdev), GFP_KERNEL);
> >> > + if (!tdev)
> >> > + return -ENOMEM;
> >> > +
> >> > + /* It's okay for MMIO to be NULL */
> >> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> > + tdev->mmio = devm_ioremap_resource(&pdev->dev, res);
> >> > + if (IS_ERR(tdev->mmio))
> >> > + tdev->mmio = NULL;
> >> > +
> >> > + tdev->tx_channel = mbox_test_request_channel(pdev, "tx");
> >> > + if (IS_ERR(tdev->tx_channel))
> >> > + return PTR_ERR(tdev->tx_channel);
> >> > +
> >> > + tdev->rx_channel = mbox_test_request_channel(pdev, "rx");
> >> > + if (IS_ERR(tdev->rx_channel))
> >> > + return PTR_ERR(tdev->rx_channel);
> >> > +
> >> Should it really fail on TX or RX only clients?
> >
> > Good question. Probably not, but I guess we'd need a flag for that.
> >
> Just assume 1 channel. You can't make it receive if the controller
> can't do rx, so the userspace will simply block on read requests. And
> you can't make the controller send, if it can't. So any write attempt
> will simply return with an error.
> And we are going to need very platform specific knowledge to execute
> the tests. It's not like some loop enumerating all possible
> combinations of parameters that the api must pass... so we are good.

I'm guessing most tests will be some kind of send-reply test, so we'd
need two channels for that. I can make it so the driver does not fail
if 'tx' or 'rx' is missing, but will fail if both are.

Bare in mind that this test driver isn't designed to cover all of the
corner cases, but more designed as a helpful boiler plate where it
will tick some use-case boxes, but is clear enough to be hacked around
by other vendors who have different requirements.

With regards to you comments on userspace; I suggest that whoever is
testing the controller will know it and will not attempt to check for
a reply from a controller (or co-proc) which is write-only.

> >> It takes write from userspace but shouldn't it also provide data
> >> received to the userspace?
> >
> > Currently we only print the returning message. If you want me to put
> > it in a userspace file too, that's not an issue.
> >
> Ideally we should support 'read' just like write.

Sure, I can do that.

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/