[PATCH] drm/print: Include drm_device.h

[PATCH] drm/print: Include drm_device.h

[Gustavo Sousa]

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>
 
 struct debugfs_regset32;
 struct drm_device;
-- 
2.48.1

Re: [PATCH] drm/print: Include drm_device.h

[Simona Vetter]

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
> 

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

Re: [PATCH] drm/print: Include drm_device.h

[Gustavo Sousa]

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

Re: [PATCH] drm/print: Include drm_device.h

[Jani Nikula]

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

-- 
Jani Nikula, Intel

Re: [PATCH] drm/print: Include drm_device.h

[Gustavo Sousa]

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

Re: [PATCH] drm/print: Include drm_device.h

[Jani Nikula]

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

-- 
Jani Nikula, Intel

Re: [PATCH] drm/print: Include drm_device.h

[Simona Vetter]

On Wed, Jan 22, 2025 at 04:49:21PM +0200, Jani Nikula wrote:
> 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>

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

Re: [PATCH] drm/print: Include drm_device.h

[Lucas De Marchi]

On Tue, 21 Jan 2025 18:09:25 -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.
> 
> [...]

Applied to drm-misc-fixes, thanks!

[1/1] drm/print: Include drm_device.h
      commit: e0f63bc68f59d281e2d06e596f6c1bd9382a15cd

Best regards,
-- 
Lucas De Marchi <lucas.demarchi@intel.com>