Date:   Sat, 24 Mar 2018 15:51:30 +0800
From:   Arnd Bergmann <arnd@...db.de>
To:     Ioana Ciornei <ioana.ciornei@....com>
Cc:     gregkh <gregkh@...uxfoundation.org>,
        Laurentiu Tudor <laurentiu.tudor@....com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Stuart Yoder <stuyoder@...il.com>,
        Ruxandra Ioana Radulescu <ruxandra.radulescu@....com>,
        razvan.stefanescu@....com, Roy Pledge <Roy.Pledge@....com>
Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

On Fri, Mar 23, 2018 at 11:38 PM, Ioana Ciornei <ioana.ciornei@....com> wrote:
> Adding kernel support for restool, a userspace tool for resource
> management, means exporting an ioctl capable device file representing
> the root resource container.
> This new functionality in the fsl-mc bus driver intends to provide
> restool an interface to interact with the MC firmware.
> Commands that are composed in userspace are sent to the MC firmware
> through the RESTOOL_SEND_MC_COMMAND ioctl.
> By default the implicit MC I/O portal is used for this operation,
> but if the implicit one is busy, a dynamic portal is allocated and then
> freed upon execution.

Hi Ioana,

So this driver is a direct passthrough to your hardware for passing
fixed-length command/response pairs. Have you considered using
a higher-level interface instead?

Can you list some of the commands that are passed here as
clarification, and explain what the tradeoffs are that have led
to adopting a low-level interface instead of a high-level interface?

The main downside of the direct passthrough obviously is that you
tie your user space to a particular hardware implementation, while
a high-level abstraction could in principle work across a wider range
of hardware revisions or even across multiple vendors implementing
the same concept by different means.

> +static long fsl_mc_restool_dev_ioctl(struct file *file,
> +                                    unsigned int cmd,
> +                                    unsigned long arg)
> +{
> +       int error;
> +
> +       switch (cmd) {
> +       case RESTOOL_SEND_MC_COMMAND:
> +               error = fsl_mc_restool_send_command(arg, file->private_data);
> +               break;

> @@ -14,10 +14,18 @@
>   * struct fsl_mc_command - Management Complex (MC) command structure
>   * @header: MC command header
>   * @params: MC command parameters
> + *
> + * Used by RESTOOL_SEND_MC_COMMAND
>   */
>  struct fsl_mc_command {
>         __u64 header;
>         __u64 params[MC_CMD_NUM_OF_PARAMS];
>  };
>
> +#define RESTOOL_IOCTL_TYPE     'R'
> +#define RESTOOL_IOCTL_SEQ      0xE0

I tried to follow the code path into the hardware and am a bit confused
about the semantics of the 'header' field and the data. Both are
accessed passed to the hardware using

       writeq(le64_to_cpu(cmd->header))

which would indicate a fixed byte layout on the user structure and that
it should really be a '__le64' instead of '__u64', or possibly should be
represented as '__u8 header[8]' to clarify that the byte ordering is
supposed to match the order of the byte addresses of the register.

However, the in-kernel usage of that field suggests that we treat it
as a 64-bit cpu-endian number, for which the hardware needs to know
the endianess of the currently running kernel and user space.

Can you have a look at where the mistake is and what the byteorder
for the fsl_mc_command structure is supposed to be? Obviously,
this is one thing that would be simplified by using a high-level
interface, but it's possible to do it like this as long as it's completely
clear how the structure layout is meant to be used in the uapi header.

        Arnd

Powered by blists - more mailing lists