[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAK8P3a0_i=wHnQmfbjCdMgFRSD0eGjEM2uMFft5FJ4M1GSUbHg@mail.gmail.com>
Date: Wed, 23 Sep 2020 20:51:12 +0200
From: Arnd Bergmann <arnd@...db.de>
To: Maximilian Luz <luzmaximilian@...il.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Blaž Hrastnik <blaz@...n.io>,
Dorian Stoll <dorian.stoll@...p.io>
Subject: Re: [RFC PATCH 8/9] surface_aggregator: Add DebugFS interface
On Wed, Sep 23, 2020 at 8:29 PM Maximilian Luz <luzmaximilian@...il.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
Powered by blists - more mailing lists