| Message ID | 20250121210935.84357-1-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/print: Include drm_device.h | expand |
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@intel.com> > --- > 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? 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. -Sima > > struct debugfs_regset32; > struct drm_device; > -- > 2.48.1 >
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@intel.com> >> --- >> 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. -- Gustavo Sousa > 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. >-Sima > >> >> struct debugfs_regset32; >> struct drm_device; >> -- >> 2.48.1 >> > >-- >Simona Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch
On Wed, 22 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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@intel.com> >>> --- >>> 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. >> 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
Quoting Jani Nikula (2025-01-22 11:02:31-03:00) >On Wed, 22 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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@intel.com> >>>> --- >>>> 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. > >>> 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
On Wed, 22 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> wrote: > Quoting Jani Nikula (2025-01-22 11:02:31-03:00) >>On Wed, 22 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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@intel.com> >>>>> --- >>>>> 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@intel.com> > >> >>>> 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
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> struct debugfs_regset32; struct drm_device;
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@intel.com> --- include/drm/drm_print.h | 1 + 1 file changed, 1 insertion(+)