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

[Jani Nikula <jani.nikula@linux.intel.com>]

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