linux-kernel - Re: [PATCH] drm: drm_gem.h: Add explicit includes for DEFINE_DRM_GEM

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] [day] [month] [year] [list]

Date:   Thu, 5 May 2022 10:28:14 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Jeffrey Hugo <quic_jhugo@...cinc.com>
Cc:     Jani Nikula <jani.nikula@...ux.intel.com>,
        maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
        tzimmermann@...e.de, airlied@...ux.ie,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm: drm_gem.h: Add explicit includes for
 DEFINE_DRM_GEM_FOPS

On Wed, May 04, 2022 at 07:58:38AM -0600, Jeffrey Hugo wrote:
> On 5/4/2022 3:38 AM, Daniel Vetter wrote:
> > On Mon, May 02, 2022 at 06:41:39PM +0300, Jani Nikula wrote:
> > > On Fri, 29 Apr 2022, Jeffrey Hugo <quic_jhugo@...cinc.com> wrote:
> > > > DEFINE_DRM_GEM_FOPS() references drm functions from other headers.  For
> > > > example drm_open() is defined in drm_file.h and drm_ioctl() is defined
> > > > in drm_ioctl.h.  Since drm_gem.h doesn't include these headers, it
> > > > relies on an implicit include from the .c file to have included these
> > > > required headers before DEFINE_DRM_GEM_FOPS() gets used.  Relying on
> > > > these implicit includes can cause build failures for new code that
> > > > doesn't know about these requirements, and can lead to future problems
> > > > if the headers ever get restructured as there will be a need to update
> > > > every downstream file that includes drm_gem.h.
> > > > 
> > > > Lets fix this explicitly including the required headers in drm_gem.h so
> > > > that code that includes drm_gem.h does not need to worry about these
> > > > implicit dependencies.
> > > 
> > > In the general case, I tend to agree, but in this specific instance I
> > > think I'd err on the side of fewer includes. I think the more likely
> > > outcome here is accumulating implicit dependencies on symbols from
> > > drm_file.h and drm_ioctl.h by including drm_gem.h only!
> > > 
> > > I do think headers need to be self-contained, and we actually enforce
> > > this in i915 (see HDRTEST in drivers/gpu/drm/i915/Makefile), but not to
> > > the point of macro expansions.
> > 
> > Yeah we abuse macros in a bunch of places to untangle header dependencies,
> > so then going back and pulling in all the headers back in feels a bit
> > silly and defeats the point.
> 
> Fair enough.  I'll consider this NAK'd
> 
> I've been pondering alternate solutions, but haven't come up with any. I
> guess, for now, the status quo will remain.

I think a standalone header for gem fops or so might be an option. We also
have some macro iirc for standard stuff in drm_driver (or at least had in
the past, maybe that's cleaned up now better).

That would allow drivers to include this monster header that pulls in a
lot of things in the one place they define their file ops, and nowhere
else. I think that would cover everything we'd want to achieve? But would
be a bit of churn to roll out everywhere.
-Daniel


> 
> > 
> > iow, I concur.
> > -Daniel
> > 
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > > 
> > > > 
> > > > Signed-off-by: Jeffrey Hugo <quic_jhugo@...cinc.com>
> > > > ---
> > > >   include/drm/drm_gem.h | 2 ++
> > > >   1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > > > index 9d7c61a..1cbe3d8 100644
> > > > --- a/include/drm/drm_gem.h
> > > > +++ b/include/drm/drm_gem.h
> > > > @@ -37,6 +37,8 @@
> > > >   #include <linux/kref.h>
> > > >   #include <linux/dma-resv.h>
> > > > +#include <drm/drm_file.h>
> > > > +#include <drm/drm_ioctl.h>
> > > >   #include <drm/drm_vma_manager.h>
> > > >   struct iosys_map;
> > > 
> > > -- 
> > > Jani Nikula, Intel Open Source Graphics Center
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch