linux-kernel - Re: [Linaro-mm-sig] Re: [PATCH v2 4/4] drm/nvdla/uapi: Add UAPI of NVDLA driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a2w1t7Sk897u0ndD66Lwp5a4DuOqqQLN4yHSg=JmrpOHQ@mail.gmail.com>
Date:   Tue, 26 Apr 2022 12:50:50 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Christian König <christian.koenig@....com>
Cc:     Cai Huoqing <cai.huoqing@...ux.dev>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        linaro-mm-sig@...ts.linaro.org
Subject: Re: [Linaro-mm-sig] Re: [PATCH v2 4/4] drm/nvdla/uapi: Add UAPI of
 NVDLA driver

On Tue, Apr 26, 2022 at 8:31 AM Christian König
<christian.koenig@....com> wrote:
> Am 26.04.22 um 08:08 schrieb Cai Huoqing:
> > The NVIDIA Deep Learning Accelerator (NVDLA) is an open source IP
> > which is integrated into NVIDIA Jetson AGX Xavier,
> > so add UAPI of this driver.
> >
> > Signed-off-by: Cai Huoqing <cai.huoqing@...ux.dev>

I saw the reply but no the original mail, so I'll comment here.

> > +
> > +#if !defined(__KERNEL__)
> > +#define __user
> > +#endif

This is done in the 'make headers_install' step, no need to define it
separately.

> > +#define NVDLA_NO_TIMEOUT    (0xffffffff)
> > +     __u32 timeout;
>
> What format does that timeout value have?
>
> In general it is best practice to have absolute 64bit nanosecond
> timeouts (to be used with ktime inside the kernel) so that restarting
> interrupted IOCTLs works smooth.

When using absolute values, one also needs to decide whether this should be
realtime, monotonic or boottime and document the decision.


> > + * struct nvdla_submit_args structure for task submit
> > + *
> > + * @tasks            pointer to array of struct nvdla_ioctl_submit_task
> > + * @num_tasks                number of entries in tasks
> > + * @flags            flags for task submit, no flags defined yet
> > + * @version          version of task structure
> > + *
> > + */
> > +struct nvdla_submit_args {
> > +     __u64 tasks;
> > +     __u16 num_tasks;
> > +#define NVDLA_MAX_TASKS_PER_SUBMIT   24
> > +#define NVDLA_SUBMIT_FLAGS_ATOMIC    (1 << 0)
>
> Well that "no flags defined yet" from the comment above is probably
> outdated :)

> > +     __u16 flags;
> > +     __u32 version;
> > +};

Versioned interfaces are usually a bad idea. If you introduce an ioctl command,
it should generally keep working. If you ever need to change the interface, just
use a new command number for the new version.

> > +/**
> > + * struct nvdla_gem_create_args for allocating DMA buffer through GEM
> > + *
> > + * @handle           handle updated by kernel after allocation
> > + * @flags            implementation specific flags
> > + * @size             size of buffer to allocate
> > + */
> > +struct nvdla_gem_create_args {
> > +     __u32 handle;
> > +     __u32 flags;
> > +     __u64 size;
> > +};
> > +
> > +/**
> > + * struct nvdla_gem_map_offset_args for mapping DMA buffer
> > + *
> > + * @handle           handle of the buffer
> > + * @reserved         reserved for padding
> > + * @offset           offset updated by kernel after mapping
> > + */
> > +struct nvdla_gem_map_offset_args {
> > +     __u32 handle;
> > +     __u32 reserved;
> > +     __u64 offset;
> > +};
> > +
> > +#define DRM_NVDLA_SUBMIT             0x00
> > +#define DRM_NVDLA_GEM_CREATE 0x01
> > +#define DRM_NVDLA_GEM_MMAP           0x02

Is this an actual mmap() call, or something that needs to be done before the
mmap()? Is the 'handle' a file descriptor or some internal number?

      Arnd