Re: [PATCH v2 10/17] drm/vram-helper: make drm_vram_mm_debugfs_init() return 0

[Thomas Zimmermann]

Hi

Am 19.03.20 um 13:27 schrieb Wambui Karuga:
> 
> 
> On Thu, 19 Mar 2020, Daniel Vetter wrote:
> 
>> On Thu, Mar 19, 2020 at 08:55:24AM +0100, Greg KH wrote:
>>> On Wed, Mar 18, 2020 at 08:10:43PM +0100, Daniel Vetter wrote:
>>>> On Wed, Mar 18, 2020 at 5:58 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
>>>> wrote:
>>>>>
>>>>> On Wed, Mar 18, 2020 at 05:31:47PM +0100, Daniel Vetter wrote:
>>>>>> On Wed, Mar 18, 2020 at 5:03 PM Wambui Karuga
>>>>>> <wambui.karugax@xxxxxxxxx> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Wed, 18 Mar 2020, Daniel Vetter wrote:
>>>>>>>
>>>>>>>> On Tue, Mar 10, 2020 at 04:31:14PM +0300, Wambui Karuga wrote:
>>>>>>>>> Since 987d65d01356 (drm: debugfs: make
>>>>>>>>> drm_debugfs_create_files() never fail),
>>>>>>>>> drm_debugfs_create_files() never
>>>>>>>>> fails and should return void. Therefore, remove its use as the
>>>>>>>>> return value of drm_vram_mm_debugfs_init(), and have the function
>>>>>>>>> return 0 directly.
>>>>>>>>>
>>>>>>>>> v2: have drm_vram_mm_debugfs_init() return 0 instead of void to
>>>>>>>>> avoid
>>>>>>>>> introducing build issues and build breakage.
>>>>>>>>>
>>>>>>>>> References:
>>>>>>>>> https://lists.freedesktop.org/archives/dri-devel/2020-February/257183.html
>>>>>>>>>
>>>>>>>>> Signed-off-by: Wambui Karuga <wambui.karugax@xxxxxxxxx>
>>>>>>>>> Acked-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
>>>>>>>>> ---
>>>>>>>>> Âdrivers/gpu/drm/drm_gem_vram_helper.c | 10 ++++------
>>>>>>>>> Â1 file changed, 4 insertions(+), 6 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>>> index 92a11bb42365..c8bcc8609650 100644
>>>>>>>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>>> @@ -1048,14 +1048,12 @@ static const struct drm_info_list
>>>>>>>>> drm_vram_mm_debugfs_list[] = {
>>>>>>>>> Â */
>>>>>>>>> Âint drm_vram_mm_debugfs_init(struct drm_minor *minor)
>>>>>>>>> Â{
>>>>>>>>> -ÂÂÂ int ret = 0;
>>>>>>>>> -
>>>>>>>>> Â#if defined(CONFIG_DEBUG_FS)
>>>>>>>>
>>>>>>>> Just noticed that this #if here is not needed, we already have a
>>>>>>>> dummy
>>>>>>>> function for that case. Care to write a quick patch to remove
>>>>>>>> it? On top
>>>>>>>> of this patch series here ofc, I'm in the processing of merging
>>>>>>>> the entire
>>>>>>>> pile.
>>>>>>>>
>>>>>>>> Thanks, Daniel
>>>>>>> Hi Daniel,
>>>>>>> Without this check here, and compiling without CONFIG_DEBUG_FS, this
>>>>>>> function is run and the drm_debugfs_create_files() does not have
>>>>>>> access to
>>>>>>> the parameters also protected by an #if above this function. So
>>>>>>> the change
>>>>>>> throws an error for me. Is that correct?
>>>>>>
>>>>>> Hm right. Other drivers don't #ifdef out their debugfs file functions
>>>>>> ... kinda a bit disappointing that we can't do this in the neatest
>>>>>> way
>>>>>> possible.
>>>>>>
>>>>>> Greg, has anyone ever suggested to convert the debugfs_create_file
>>>>>> function (and similar things) to macros that don't use any of the
>>>>>> arguments, and then also annotating all the static
>>>>>> functions/tables as
>>>>>> __maybe_unused and let the compiler garbage collect everything?
>>>>>> Instead of explicit #ifdef in all the drivers ...
>>>>>
>>>>> No, no one has suggested that, having the functions be static inline
>>>>> should make it all "just work" properly if debugfs is not enabled.Â
>>>>> The
>>>>> variables will not be used, so the compiler should just optimize them
>>>>> away properly.
>>>>>
>>>>> No checks for CONFIG_DEBUG_FS should be needed anywhere in .c code.
>>>>
>>>> So the trouble with this one is that the static inline functions for
>>>> the debugfs file are wrapped in a #if too, and hence if we drop the
>>>> #if around the function call stuff won't compile. Should we drop all
>>>> the #if in the .c file and assume the compiler will remove all the
>>>> dead code and dead functions?
>>>
>>> Yes you should :)
>>>
>>> there should not be any need for #if in a .c file for debugfs stuff.
>>
>> Wambui, can you pls try that out? I.e. removing all the #if for
>> CONFIG_DEBUG_FS from that file.
> 
> Removing them works with CONFIG_DEBUG_FS enabled or disabled.
> I can send a patch for that.

Please do. Removing explicit checks for CONFIG_ is usually a good thing.

Best regards
Thomas

> 
> wambui karuga
>> -Daniel
>> --Â
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 NÃrnberg, Germany
(HRB 36809, AG NÃrnberg)
GeschÃftsfÃhrer: Felix ImendÃrffer

Attachment: signature.asc
Description: OpenPGP digital signature