Messages in this thread | | | Subject | Re: [RFC PATCH 8/9] surface_aggregator: Add DebugFS interface | From | Maximilian Luz <> | Date | Thu, 24 Sep 2020 00:23:05 +0200 |
| |
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
| |