Messages in this thread | | | From | Arnd Bergmann <> | Date | Wed, 23 Sep 2020 20:51:12 +0200 | Subject | Re: [RFC PATCH 8/9] surface_aggregator: Add DebugFS interface |
| |
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.
> 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.
> 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.
Arnd
| |