On Wed, Jan 22, 2025 at 04:49:21PM +0200, Jani Nikula wrote: > On Wed, 22 Jan 2025, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote: > > Quoting Jani Nikula (2025-01-22 11:02:31-03:00) > >>On Wed, 22 Jan 2025, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote: > >>> Quoting Simona Vetter (2025-01-22 08:11:53-03:00) > >>>>On Tue, Jan 21, 2025 at 06:09:25PM -0300, Gustavo Sousa wrote: > >>>>> The header drm_print.h uses members of struct drm_device pointers, as > >>>>> such, it should include drm_device.h to let the compiler know the full > >>>>> type definition. > >>>>> > >>>>> Without such include, users of drm_print.h that don't explicitly need > >>>>> drm_device.h would bump into build errors and be forced to include the > >>>>> latter. > >>>>> > >>>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx> > >>>>> --- > >>>>> include/drm/drm_print.h | 1 + > >>>>> 1 file changed, 1 insertion(+) > >>>>> > >>>>> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h > >>>>> index f77fe1531cf8..9732f514566d 100644 > >>>>> --- a/include/drm/drm_print.h > >>>>> +++ b/include/drm/drm_print.h > >>>>> @@ -32,6 +32,7 @@ > >>>>> #include <linux/dynamic_debug.h> > >>>>> > >>>>> #include <drm/drm.h> > >>>>> +#include <drm/drm_device.h> > >>>> > >>>>We much prefer just a struct device forward decl to avoid monster headers. > >>>>Is that not doable here? > >>> > >>> I don't think so. This header explicitly uses members of struct > >>> drm_device, so the compiler needs to know the full type definition. As > >>> an example see the definition of drm_WARN(), it uses (drm)->dev. > >> > >>I grudgingly agree. I don't think there are actual cases where this > >>happens, but I can imagine you could create one. > > > > It happened to me, and that motivated me to send this patch. > > > > I had a local patch where I just needed the drm_print.h header, but I > > ended up having to include drm_device.h in my .c file. > > Right. > > Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx> So at least in the past we sometimes intentionally used macros instead of static inline functions, so that we could avoid pulling in headers from headers all over the place. That's why I only looked at the one static inline functions, which treats the pointer as opaque so should work. I don't mind either way, just bringing this in so that we're consistent here. -Sima > > > > >> > >>>> Worst case I'd convert the drm_info_printer() static inline to a > >>>> macro, not sure about the exact rules here if you never deref a > >>>> pointer. > >> > >>The forward declaration is enough for passing pointers around without > >>dereferencing. It's the dereferencing in the macros that could fail the > >>build if the .c using them doesn't include drm_device.h. > >> > >>To balance things out, I think we could probably drop drm/drm.h if we > >>inlined one use of DRM_NAME to just "drm". > >> > >> > >>BR, > >>Jani. > >> > >> > >>>>-Sima > >>>> > >>>>> > >>>>> struct debugfs_regset32; > >>>>> struct drm_device; > >>>>> -- > >>>>> 2.48.1 > >>>>> > >>>> > >>>>-- > >>>>Simona Vetter > >>>>Software Engineer, Intel Corporation > >>>>http://blog.ffwll.ch > >> > >>-- > >>Jani Nikula, Intel > > -- > Jani Nikula, Intel -- Simona Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
