| Message ID | 20200310133121.27913-11-wambui.karugax@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: subsytem-wide debugfs functions refactor | expand |
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@gmail.com> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > 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 > - ret = drm_debugfs_create_files(drm_vram_mm_debugfs_list, > - ARRAY_SIZE(drm_vram_mm_debugfs_list), > - minor->debugfs_root, minor); > + drm_debugfs_create_files(drm_vram_mm_debugfs_list, > + ARRAY_SIZE(drm_vram_mm_debugfs_list), > + minor->debugfs_root, minor); > #endif > - return ret; > + return 0; > } > EXPORT_SYMBOL(drm_vram_mm_debugfs_init); > > -- > 2.25.1 >
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@gmail.com> >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> 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? Thanks, wambui karuga >> - ret = drm_debugfs_create_files(drm_vram_mm_debugfs_list, >> - ARRAY_SIZE(drm_vram_mm_debugfs_list), >> - minor->debugfs_root, minor); >> + drm_debugfs_create_files(drm_vram_mm_debugfs_list, >> + ARRAY_SIZE(drm_vram_mm_debugfs_list), >> + minor->debugfs_root, minor); >> #endif >> - return ret; >> + return 0; >> } >> EXPORT_SYMBOL(drm_vram_mm_debugfs_init); >> >> -- >> 2.25.1 >> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch >
On Wed, Mar 18, 2020 at 5:03 PM Wambui Karuga <wambui.karugax@gmail.com> 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@gmail.com> > >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > >> --- > >> 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 ... -Daniel > > Thanks, > wambui karuga > > >> - ret = drm_debugfs_create_files(drm_vram_mm_debugfs_list, > >> - ARRAY_SIZE(drm_vram_mm_debugfs_list), > >> - minor->debugfs_root, minor); > >> + drm_debugfs_create_files(drm_vram_mm_debugfs_list, > >> + ARRAY_SIZE(drm_vram_mm_debugfs_list), > >> + minor->debugfs_root, minor); > >> #endif > >> - return ret; > >> + return 0; > >> } > >> EXPORT_SYMBOL(drm_vram_mm_debugfs_init); > >> > >> -- > >> 2.25.1 > >> > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > >
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@gmail.com> 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@gmail.com> > > >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > > >> --- > > >> 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. thanks, greg k-h
On Wed, Mar 18, 2020 at 5:58 PM Greg KH <gregkh@linuxfoundation.org> 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@gmail.com> 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@gmail.com> > > > >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > > > >> --- > > > >> 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? -Daniel (who has no idea how this all works really)
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@linuxfoundation.org> 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@gmail.com> 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@gmail.com> > > > > >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > > > > >> --- > > > > >> 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. thanks, greg k-h
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@linuxfoundation.org> 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@gmail.com> 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@gmail.com> > > > > > >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > > > > > >> --- > > > > > >> 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. -Daniel
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@linuxfoundation.org> 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@gmail.com> 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@gmail.com> >>>>>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> >>>>>>>> --- >>>>>>>> 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. wambui karuga > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch >
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@linuxfoundation.org> >>>> 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@gmail.com> 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@gmail.com> >>>>>>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> >>>>>>>>> --- >>>>>>>>> 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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) - ret = drm_debugfs_create_files(drm_vram_mm_debugfs_list, - ARRAY_SIZE(drm_vram_mm_debugfs_list), - minor->debugfs_root, minor); + drm_debugfs_create_files(drm_vram_mm_debugfs_list, + ARRAY_SIZE(drm_vram_mm_debugfs_list), + minor->debugfs_root, minor); #endif - return ret; + return 0; } EXPORT_SYMBOL(drm_vram_mm_debugfs_init);