On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximil...@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
