Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200923151511.3842150-1-luzmaximilian@gmail.com> <20200923151511.3842150-9-luzmaximilian@gmail.com>
In-Reply-To: <20200923151511.3842150-9-luzmaximilian@gmail.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Wed, 23 Sep 2020 18:48:07 +0200
Message-ID: <CAK8P3a0=98pzgWwBKddy7BQ9g90ga8JEx=MtADW+aqTe0AVV6w@mail.gmail.com>
Subject: Re: [RFC PATCH 8/9] surface_aggregator: Add DebugFS interface
To:     Maximilian Luz <luzmaximilian@gmail.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?Q?Bla=C5=BE_Hrastnik?= <blaz@mxxn.io>,
        Dorian Stoll <dorian.stoll@tmsp.io>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>> +   * - ``0xA5``
> +     - ``1``
> +     - ``WR``
> +     - ``REQUEST``
> +     - Perform synchronous SAM request.
> +
> +
> +``GETVERSION``
> +--------------
> +
> +Defined as ``_IOR(0xA5, 0, __u32)``.
> +
> +Gets the current interface version. This should be used to check for changes
> +in the interface and determine if certain functionality is available. While
> +the interface should under normal circumstances kept backward compatible, as
> +this is a debug interface, backwards compatibility is not guaranteed.
> +
> +The version number follows the semantic versioning scheme, roughly meaning
> +that an increment in the highest non-zero version number signals a breaking
> +change. It can be decomposed as follows:

Versioned interfaces are basically always a mess, try to avoid them. I'd much
rather see this done in one of two ways:

a) make it a proper documented interface, in this case probably a misc
character device, and then maintain the interface forever, without
breaking compatibility with existing users.

b) keep it as a debugfs file, but don't even pretend for it
to be a documented interface. Anything using it should know
what they are doing and have a matching user space.

> +/**
> + * struct ssam_debug_request - Controller request IOCTL argument.
> + * @target_category: Target category of the SAM request.
> + * @target_id:       Target ID of the SAM request.
> + * @command_id:      Command ID of the SAM request.
> + * @instance_id:     Instance ID of the SAM request.
> + * @flags:           SAM Request flags.
> + * @status:          Request status (output).
> + * @payload:         Request payload (input data).
> + * @payload.data:    Pointer to request payload data.
> + * @payload.length:  Length of request payload data (in bytes).
> + * @response:        Request response (output data).
> + * @response.data:   Pointer to response buffer.
> + * @response.length: On input: Capacity of response buffer (in bytes).
> + *                   On output: Length of request response (number of bytes
> + *                   in the buffer that are actually used).
> + */
> +struct ssam_dbg_request {
> +       __u8 target_category;
> +       __u8 target_id;
> +       __u8 command_id;
> +       __u8 instance_id;
> +       __u16 flags;
> +       __s16 status;
> +
> +       struct {
> +               const __u8 __user *data;
> +               __u16 length;
> +               __u8 __pad[6];
> +       } payload;
> +
> +       struct {
> +               __u8 __user *data;
> +               __u16 length;
> +               __u8 __pad[6];
> +       } response;
> +};

Binary interfaces are hard. In this case the indirect pointers mean that
32-bit user space has an incompatible layout, which you should not do.

Also, having an ioctl on a debugfs file is a bit odd. I wonder if you
could have this as a transactional file that performs only read/write
commands, i.e. you pass in a

struct ssam_dbg_request {
       __u8 target_category;
       __u8 target_id;
       __u8 command_id;
       __u8 instance_id;
       __u16 flags;
      __u8 payload[]; /* variable-length */
};

and you get out a

struct ssam_dbg_response {
      __s16 status;
     __u8 payload[];
};

and keep the rest unchanged. See fs/libfs.c for how this could be done
with simple_transaction files.

      Arnd