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