Re: [RFC PATCH 8/9] surface_aggregator: Add DebugFS interface

[Maximilian Luz <luzmaximilian@gmail.com>]

On 9/23/20 8:51 PM, Arnd Bergmann wrote:
> On Wed, Sep 23, 2020 at 8:29 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>> On 9/23/20 6:48 PM, Arnd Bergmann wrote:
>>>> + * 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.
>>
>> Thanks! Is there a way to make this compatible with a 32-bit user space?
> 
> The version I showed avoids the pointers and is compatible with
> 32-bit user space.

I'm not completely convinced yet that the read/write approach is the way
I want to do it, especially with Greg suggesting a misc device, but I'll
keep your solution in mind.
  
>>   From a quick search, compat_ptr and compat_uptr_t would be the right way
>> to transfer pointer?
> 
> If you end up needing an indirect pointer, the most portable way is to
> use a __u64 and read it using u64_to_user_ptr() in the kernel.

Thanks!

>> 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
> 
> The debugfs way is usually to just have additional files when you
> do more than one thing, or if you need a new variant of that interface,
> they are cheap.

Alright, thanks!

Regards,
Max