Messages in this thread |  | | From | Arnd Bergmann <> | Date | Sat, 24 Mar 2018 15:51:30 +0800 | 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@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
|  |