[lkml]   [2020]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [RFC PATCH 8/9] surface_aggregator: Add DebugFS interface
On 9/23/20 6:48 PM, Arnd Bergmann wrote:
> 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.

I'll drop the version. I'd still very much like to keep the
documentation as well as keeping this a debugfs file. I hope that I've
made it clear enough in the documentation that it's not intended for use
by anything other than debugging, reverse-engineering, prototyping and
the likes. Especially as having that in debugfs should IMHO give the
impression: "If you rely on it and it breaks, it's not my fault", which
is very much what I want to stick by for now.

Thus I'm not really in favor of making it a "public" device, at least
not yet. This may make sense in case we ever have a concrete need for
user space applications communicating with the EC directly, although I'd
like to structure and commit to that interface once there is such.

>> +/**
>> + * 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).
>> + * Pointer to request payload data.
>> + * @payload.length: Length of request payload data (in bytes).
>> + * @response: Request response (output 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.

Thanks! Is there a way to make this compatible with a 32-bit user space?
From a quick search, compat_ptr and compat_uptr_t would be the right way
to transfer pointer?

I've already laid out my main two rationales for using an IOCTL in the
reply to Greg, but here's an overview: First, IOCTLs allow me to execute
requests in parallel with only a single open file descriptor, and
without having to care about allocating buffers for the responses and
waiting until the buffer is read (yes, arguably I still have to manage
buffers, but only in the IOCTL function which I consider a bit more
manageable). I was previously unaware of the simple_transaction helpers
though, so thanks for that pointer! Second, I can easily expand that
interface to handle events sent by the EC, by having the user space
application read from that file. Although that could be moved to a
second file. I just felt having that option of keeping in one would
eventually result in a cleaner interface.


 \ /
  Last update: 2020-09-23 20:30    [W:0.119 / U:5.428 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site