[lkml]   [2018]   [Mar]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
On Fri, Mar 23, 2018 at 11:38 PM, Ioana Ciornei <> 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) {
> + 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
> + *
> */
> struct fsl_mc_command {
> __u64 header;
> __u64 params[MC_CMD_NUM_OF_PARAMS];
> };
> +#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


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.


 \ /
  Last update: 2018-03-24 08:52    [W:1.463 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site