Message-ID: <20150813102335.GC8782@x1>
Date:	Thu, 13 Aug 2015 11:23:35 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Jassi Brar <jassisinghbrar@...il.com>
Cc:	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	kernel@...inux.com, Devicetree List <devicetree@...r.kernel.org>
Subject: Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing
 Mailbox Controllers

On Thu, 13 Aug 2015, Jassi Brar wrote:

> On Thu, Aug 13, 2015 at 2:49 PM, Lee Jones <lee.jones@...aro.org> wrote:
> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> 
> >> >> > +
> >> >> > +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?
> >
> file_operations.write() should parse signal and message, coming from
> userspace, into a local structure before routing them via
> mbox_send_message and tx_prepare respectively.

Okay.  So before I code this up we should agree on syntax.

How would you like to represent the break between signal and message?
Obviously ' ' would be a bad idea, as some clients may want to send
messages with white space contained.  How about '\t' or '\n'?

> > 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);
> >
> No. Command/Signal code is passed via mbox_send_message(signal). The
> payload is passed via Shared-Memory during tx_prepare(message)

Okay, I see.  Thanks for the clarification.

> >> >> > +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.
> >
> I have seen more channels to be rx+tx, than rx/tx only. And for latter
> case the user should run 2 test threads, one each for RX and TX.

Okay, makes sense to me.

> > 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.
> >
> That's actually my point :)
> When the userspace already has all that platform specific knowledge,
> why check for 'valid' usage in controller driver? Simply reject any
> request that the controller isn't capable of (which is practically
> never supposed to happen).

This conversation doesn't really belong in this thread; but seeing as
you bought it up ...

I absolutely and fundamentally disagree that the controller shouldn't
do error checking during mbox_request_channel(). It is
mbox_request_channel() that should fail on a known bad configuration,
rather than pass back a known rotten apple and wait for that it to be
eaten (appreciate I've gone a little overboard with the analogies)
before flagging it up.  And for what purpose?  To make the controller
driver a little simpler?  I do hope that you do see sense on this one
and allow me to keep the error checking in the driver!

If you have a reply to this, please do so one the driver thread, so
it's easier to follow.

Secondly, a reply more akin to this thread.  The guys using _this_
driver are more likely to be Controller authors.  These are the chaps
I was speaking about that are going to know the platform specifics of
the controller.  The error checking in mbox_request_channel() is
designed for 'client' authors, who are more likely to be requesting
invalid configurations.

-- 
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@...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