Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Arnd Bergmann]

On Fri, Mar 23, 2018 at 11:38 PM, Ioana Ciornei <ioana.ciornei@xxxxxxx> 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