From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752601AbbHMKXl (ORCPT ); Thu, 13 Aug 2015 06:23:41 -0400 Received: from mail-wi0-f182.google.com ([209.85.212.182]:34584 "EHLO mail-wi0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751641AbbHMKXj (ORCPT ); Thu, 13 Aug 2015 06:23:39 -0400 Date: Thu, 13 Aug 2015 11:23:35 +0100 From: Lee Jones To: Jassi Brar Cc: "linux-arm-kernel@lists.infradead.org" , Linux Kernel Mailing List , kernel@stlinux.com, Devicetree List Subject: Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers Message-ID: <20150813102335.GC8782@x1> References: <1437990272-23111-1-git-send-email-lee.jones@linaro.org> <1437990272-23111-6-git-send-email-lee.jones@linaro.org> <20150812102328.GD18282@x1> <20150813091914.GB8782@x1> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 13 Aug 2015, Jassi Brar wrote: > On Thu, Aug 13, 2015 at 2:49 PM, Lee Jones 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