| Message ID | 20230202110312.808607-1-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/client: Convert to VISIBLE_IF_KUNIT | expand |
Hi Maxime, On 2/2/23 08:03, Maxime Ripard wrote: > Commit 8fc0380f6ba7 ("drm/client: Add some tests for > drm_connector_pick_cmdline_mode()") was meant to introduce unit tests > for the static drm_connector_pick_cmdline_mode() function. > > In such a case, the kunit documentation recommended to import the tests > source file directly from the source file with the static function to > test. > > While it was working, it's generally frowned upon. Fortunately, commit > 9c988fae6f6a ("kunit: add macro to allow conditionally exposing static > symbols to tests") introduced macros to easily deal with that case. We > can thus remove our include and use those macros instead. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/drm_client_modeset.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > index 1b12a3c201a3..f48882941852 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -8,6 +8,9 @@ > */ > > #include "drm/drm_modeset_lock.h" > + > +#include <kunit/visibility.h> > + > #include <linux/module.h> > #include <linux/mutex.h> > #include <linux/slab.h> > @@ -159,7 +162,8 @@ drm_connector_has_preferred_mode(struct drm_connector *connector, int width, int > return NULL; > } > > -static struct drm_display_mode *drm_connector_pick_cmdline_mode(struct drm_connector *connector) > +VISIBLE_IF_KUNIT struct drm_display_mode * > +drm_connector_pick_cmdline_mode(struct drm_connector *connector) > { > struct drm_cmdline_mode *cmdline_mode; > struct drm_display_mode *mode; > @@ -215,6 +219,7 @@ static struct drm_display_mode *drm_connector_pick_cmdline_mode(struct drm_conne > > return NULL; > } > +EXPORT_SYMBOL_IF_KUNIT(drm_connector_pick_cmdline_mode); > > static bool drm_connector_enabled(struct drm_connector *connector, bool strict) > { > @@ -1233,7 +1238,3 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode) > return ret; > } > EXPORT_SYMBOL(drm_client_modeset_dpms); > - > -#ifdef CONFIG_DRM_KUNIT_TEST > -#include "tests/drm_client_modeset_test.c" > -#endif As you removed this guard, you need to add drm_client_modeset_test.o to tests/Makefile, otherwise, drm_client_modeset_test won't compile. Best Regards, - Maíra Canal
Hi Am 02.02.23 um 12:03 schrieb Maxime Ripard: > Commit 8fc0380f6ba7 ("drm/client: Add some tests for > drm_connector_pick_cmdline_mode()") was meant to introduce unit tests > for the static drm_connector_pick_cmdline_mode() function. > > In such a case, the kunit documentation recommended to import the tests > source file directly from the source file with the static function to > test. > > While it was working, it's generally frowned upon. Fortunately, commit > 9c988fae6f6a ("kunit: add macro to allow conditionally exposing static > symbols to tests") introduced macros to easily deal with that case. We > can thus remove our include and use those macros instead. I like that this include statements is going away. But changing symbol visibility for tests is likewise awkward. Maybe i'm askin gtoo miuch for this simple patch, but can't we have a helper macro that generates an exported wrapper for Kunit tests? Something like this: EXPORT_KUNIT_WRAPPER(struct drm_display_mode *\ drm_connector_pick_cmdline_mode, struct drm_connector *connector); which then generates something like this: struct drm_display_mode * drm_connector_pick_cmdline_mode_kunit( struct drm_connector *connector) { return drm_connector_pick_cmdline_mode(connector); } I know that the macro for generating this code is more complex than illustrated here. But this solution separates Kunit and functions cleanly. The static functions that are exported for Kunit testing still need to be declared in a header file. That could also be done via such a macro. Best regards Thomas > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/drm_client_modeset.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > index 1b12a3c201a3..f48882941852 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -8,6 +8,9 @@ > */ > > #include "drm/drm_modeset_lock.h" > + > +#include <kunit/visibility.h> > + > #include <linux/module.h> > #include <linux/mutex.h> > #include <linux/slab.h> > @@ -159,7 +162,8 @@ drm_connector_has_preferred_mode(struct drm_connector *connector, int width, int > return NULL; > } > > -static struct drm_display_mode *drm_connector_pick_cmdline_mode(struct drm_connector *connector) > +VISIBLE_IF_KUNIT struct drm_display_mode * > +drm_connector_pick_cmdline_mode(struct drm_connector *connector) > { > struct drm_cmdline_mode *cmdline_mode; > struct drm_display_mode *mode; > @@ -215,6 +219,7 @@ static struct drm_display_mode *drm_connector_pick_cmdline_mode(struct drm_conne > > return NULL; > } > +EXPORT_SYMBOL_IF_KUNIT(drm_connector_pick_cmdline_mode); > > static bool drm_connector_enabled(struct drm_connector *connector, bool strict) > { > @@ -1233,7 +1238,3 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode) > return ret; > } > EXPORT_SYMBOL(drm_client_modeset_dpms); > - > -#ifdef CONFIG_DRM_KUNIT_TEST > -#include "tests/drm_client_modeset_test.c" > -#endif
Hi, On Thu, Feb 02, 2023 at 01:22:01PM +0100, Thomas Zimmermann wrote: > Am 02.02.23 um 12:03 schrieb Maxime Ripard: > > Commit 8fc0380f6ba7 ("drm/client: Add some tests for > > drm_connector_pick_cmdline_mode()") was meant to introduce unit tests > > for the static drm_connector_pick_cmdline_mode() function. > > > > In such a case, the kunit documentation recommended to import the tests > > source file directly from the source file with the static function to > > test. > > > > While it was working, it's generally frowned upon. Fortunately, commit > > 9c988fae6f6a ("kunit: add macro to allow conditionally exposing static > > symbols to tests") introduced macros to easily deal with that case. We > > can thus remove our include and use those macros instead. > > I like that this include statements is going away. Yeah, when I saw that it was now available, I remembered you really didn't like it :) > But changing symbol visibility for tests is likewise awkward. > > Maybe i'm askin gtoo miuch for this simple patch, but can't we have a helper > macro that generates an exported wrapper for Kunit tests? Something like > this: > > EXPORT_KUNIT_WRAPPER(struct drm_display_mode *\ > drm_connector_pick_cmdline_mode, > struct drm_connector *connector); > > which then generates something like this: > > struct drm_display_mode * drm_connector_pick_cmdline_mode_kunit( > struct drm_connector *connector) > { > return drm_connector_pick_cmdline_mode(connector); > } > > I know that the macro for generating this code is more complex than > illustrated here. But this solution separates Kunit and functions cleanly. > The static functions that are exported for Kunit testing still need to be > declared in a header file. That could also be done via such a macro. I mean, I guess we could do that, but what's the point? I don't really get what that wrapper brings to the table. Also, this deviates from the existing practice we had for selftests and EXPORT_SYMBOL_FOR_TESTS_ONLY Maxime
On Thu, Feb 02, 2023 at 08:31:27AM -0300, Maíra Canal wrote: > Hi Maxime, > > On 2/2/23 08:03, Maxime Ripard wrote: > > Commit 8fc0380f6ba7 ("drm/client: Add some tests for > > drm_connector_pick_cmdline_mode()") was meant to introduce unit tests > > for the static drm_connector_pick_cmdline_mode() function. > > > > In such a case, the kunit documentation recommended to import the tests > > source file directly from the source file with the static function to > > test. > > > > While it was working, it's generally frowned upon. Fortunately, commit > > 9c988fae6f6a ("kunit: add macro to allow conditionally exposing static > > symbols to tests") introduced macros to easily deal with that case. We > > can thus remove our include and use those macros instead. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > drivers/gpu/drm/drm_client_modeset.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > > index 1b12a3c201a3..f48882941852 100644 > > --- a/drivers/gpu/drm/drm_client_modeset.c > > +++ b/drivers/gpu/drm/drm_client_modeset.c > > @@ -8,6 +8,9 @@ > > */ > > #include "drm/drm_modeset_lock.h" > > + > > +#include <kunit/visibility.h> > > + > > #include <linux/module.h> > > #include <linux/mutex.h> > > #include <linux/slab.h> > > @@ -159,7 +162,8 @@ drm_connector_has_preferred_mode(struct drm_connector *connector, int width, int > > return NULL; > > } > > -static struct drm_display_mode *drm_connector_pick_cmdline_mode(struct drm_connector *connector) > > +VISIBLE_IF_KUNIT struct drm_display_mode * > > +drm_connector_pick_cmdline_mode(struct drm_connector *connector) > > { > > struct drm_cmdline_mode *cmdline_mode; > > struct drm_display_mode *mode; > > @@ -215,6 +219,7 @@ static struct drm_display_mode *drm_connector_pick_cmdline_mode(struct drm_conne > > return NULL; > > } > > +EXPORT_SYMBOL_IF_KUNIT(drm_connector_pick_cmdline_mode); > > static bool drm_connector_enabled(struct drm_connector *connector, bool strict) > > { > > @@ -1233,7 +1238,3 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode) > > return ret; > > } > > EXPORT_SYMBOL(drm_client_modeset_dpms); > > - > > -#ifdef CONFIG_DRM_KUNIT_TEST > > -#include "tests/drm_client_modeset_test.c" > > -#endif > > As you removed this guard, you need to add drm_client_modeset_test.o > to tests/Makefile, otherwise, drm_client_modeset_test won't compile. Urgh, thanks for noticing this. I'll fix it up in a v2 Maxime
Hi Am 02.02.23 um 13:35 schrieb Maxime Ripard: > Hi, > > On Thu, Feb 02, 2023 at 01:22:01PM +0100, Thomas Zimmermann wrote: >> Am 02.02.23 um 12:03 schrieb Maxime Ripard: >>> Commit 8fc0380f6ba7 ("drm/client: Add some tests for >>> drm_connector_pick_cmdline_mode()") was meant to introduce unit tests >>> for the static drm_connector_pick_cmdline_mode() function. >>> >>> In such a case, the kunit documentation recommended to import the tests >>> source file directly from the source file with the static function to >>> test. >>> >>> While it was working, it's generally frowned upon. Fortunately, commit >>> 9c988fae6f6a ("kunit: add macro to allow conditionally exposing static >>> symbols to tests") introduced macros to easily deal with that case. We >>> can thus remove our include and use those macros instead. >> >> I like that this include statements is going away. > > Yeah, when I saw that it was now available, I remembered you really > didn't like it :) > >> But changing symbol visibility for tests is likewise awkward. >> >> Maybe i'm askin gtoo miuch for this simple patch, but can't we have a helper >> macro that generates an exported wrapper for Kunit tests? Something like >> this: >> >> EXPORT_KUNIT_WRAPPER(struct drm_display_mode *\ >> drm_connector_pick_cmdline_mode, >> struct drm_connector *connector); >> >> which then generates something like this: >> >> struct drm_display_mode * drm_connector_pick_cmdline_mode_kunit( >> struct drm_connector *connector) >> { >> return drm_connector_pick_cmdline_mode(connector); >> } >> >> I know that the macro for generating this code is more complex than >> illustrated here. But this solution separates Kunit and functions cleanly. >> The static functions that are exported for Kunit testing still need to be >> declared in a header file. That could also be done via such a macro. > > I mean, I guess we could do that, but what's the point? I don't really > get what that wrapper brings to the table. The big benefit of the kunit wrapper is that we don't change the visibility or implementation of the tested code. The currently existing macros invite linker errors because symbol visibility now depends on whether Kunit it enabled. It's also not clear to me how Kunit knows the symbol. Is there a function declaration in the Kunit test's source code? If so, it might diverge from the implementation; with consequences. Best regards Thomas > > Also, this deviates from the existing practice we had for selftests and > EXPORT_SYMBOL_FOR_TESTS_ONLY > > Maxime
Hi Maxime,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.2-rc6 next-20230202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Maxime-Ripard/drm-client-Convert-to-VISIBLE_IF_KUNIT/20230202-190453
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20230202110312.808607-1-maxime%40cerno.tech
patch subject: [PATCH] drm/client: Convert to VISIBLE_IF_KUNIT
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230202/202302022027.LWmJQ4lL-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/dd1e4c34178f4049f33e639350a6ef66ae9b5fd3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Maxime-Ripard/drm-client-Convert-to-VISIBLE_IF_KUNIT/20230202-190453
git checkout dd1e4c34178f4049f33e639350a6ef66ae9b5fd3
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/gpu/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/drm_client_modeset.c:166:1: warning: no previous prototype for 'drm_connector_pick_cmdline_mode' [-Wmissing-prototypes]
166 | drm_connector_pick_cmdline_mode(struct drm_connector *connector)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/drm_connector_pick_cmdline_mode +166 drivers/gpu/drm/drm_client_modeset.c
164
165 VISIBLE_IF_KUNIT struct drm_display_mode *
> 166 drm_connector_pick_cmdline_mode(struct drm_connector *connector)
167 {
168 struct drm_cmdline_mode *cmdline_mode;
169 struct drm_display_mode *mode;
170 bool prefer_non_interlace;
171
172 /*
173 * Find a user-defined mode. If the user gave us a valid
174 * mode on the kernel command line, it will show up in this
175 * list.
176 */
177
178 list_for_each_entry(mode, &connector->modes, head) {
179 if (mode->type & DRM_MODE_TYPE_USERDEF)
180 return mode;
181 }
182
183 cmdline_mode = &connector->cmdline_mode;
184 if (cmdline_mode->specified == false)
185 return NULL;
186
187 /*
188 * Attempt to find a matching mode in the list of modes we
189 * have gotten so far.
190 */
191
192 prefer_non_interlace = !cmdline_mode->interlace;
193 again:
194 list_for_each_entry(mode, &connector->modes, head) {
195 /* check width/height */
196 if (mode->hdisplay != cmdline_mode->xres ||
197 mode->vdisplay != cmdline_mode->yres)
198 continue;
199
200 if (cmdline_mode->refresh_specified) {
201 if (drm_mode_vrefresh(mode) != cmdline_mode->refresh)
202 continue;
203 }
204
205 if (cmdline_mode->interlace) {
206 if (!(mode->flags & DRM_MODE_FLAG_INTERLACE))
207 continue;
208 } else if (prefer_non_interlace) {
209 if (mode->flags & DRM_MODE_FLAG_INTERLACE)
210 continue;
211 }
212 return mode;
213 }
214
215 if (prefer_non_interlace) {
216 prefer_non_interlace = false;
217 goto again;
218 }
219
220 return NULL;
221 }
222 EXPORT_SYMBOL_IF_KUNIT(drm_connector_pick_cmdline_mode);
223
On Thu, Feb 02, 2023 at 02:05:14PM +0100, Thomas Zimmermann wrote: > Hi > > Am 02.02.23 um 13:35 schrieb Maxime Ripard: > > Hi, > > > > On Thu, Feb 02, 2023 at 01:22:01PM +0100, Thomas Zimmermann wrote: > > > Am 02.02.23 um 12:03 schrieb Maxime Ripard: > > > > Commit 8fc0380f6ba7 ("drm/client: Add some tests for > > > > drm_connector_pick_cmdline_mode()") was meant to introduce unit tests > > > > for the static drm_connector_pick_cmdline_mode() function. > > > > > > > > In such a case, the kunit documentation recommended to import the tests > > > > source file directly from the source file with the static function to > > > > test. > > > > > > > > While it was working, it's generally frowned upon. Fortunately, commit > > > > 9c988fae6f6a ("kunit: add macro to allow conditionally exposing static > > > > symbols to tests") introduced macros to easily deal with that case. We > > > > can thus remove our include and use those macros instead. > > > > > > I like that this include statements is going away. > > > > Yeah, when I saw that it was now available, I remembered you really > > didn't like it :) > > > > > But changing symbol visibility for tests is likewise awkward. > > > > > > Maybe i'm askin gtoo miuch for this simple patch, but can't we have a helper > > > macro that generates an exported wrapper for Kunit tests? Something like > > > this: > > > > > > EXPORT_KUNIT_WRAPPER(struct drm_display_mode *\ > > > drm_connector_pick_cmdline_mode, > > > struct drm_connector *connector); > > > > > > which then generates something like this: > > > > > > struct drm_display_mode * drm_connector_pick_cmdline_mode_kunit( > > > struct drm_connector *connector) > > > { > > > return drm_connector_pick_cmdline_mode(connector); > > > } > > > > > > I know that the macro for generating this code is more complex than > > > illustrated here. But this solution separates Kunit and functions cleanly. > > > The static functions that are exported for Kunit testing still need to be > > > declared in a header file. That could also be done via such a macro. > > > > I mean, I guess we could do that, but what's the point? I don't really > > get what that wrapper brings to the table. > > The big benefit of the kunit wrapper is that we don't change the visibility > or implementation of the tested code. The currently existing macros invite > linker errors because symbol visibility now depends on whether Kunit it > enabled. Sure, it can happen, but saying that it encourages them is a stretch. And fortunately, we have build bots to detect that. The huge downside of the wrapper approach is that you're no longer testing the function you want to test but something else, and you have to trust that it's exactly the same thing. And it's far from obvious that we're supposed to do that in the first place, especially when everyone else is doing something else, and we're doing it ourselves in a similar situation. It's still fairly new in kunit, but if it was indeed creating any kind of friction, I think EXPORT_SYMBOL_FOR_TESTS_ONLY would have been reworked some time in the last 4 years. > It's also not clear to me how Kunit knows the symbol. Is there a > function declaration in the Kunit test's source code? If so, it might > diverge from the implementation; with consequences. apparmor has been using an internal header included in both the test file and the implementation. That way, the compiler will make sure there's no inconsistencies. Maxime
diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c index 1b12a3c201a3..f48882941852 100644 --- a/drivers/gpu/drm/drm_client_modeset.c +++ b/drivers/gpu/drm/drm_client_modeset.c @@ -8,6 +8,9 @@ */ #include "drm/drm_modeset_lock.h" + +#include <kunit/visibility.h> + #include <linux/module.h> #include <linux/mutex.h> #include <linux/slab.h> @@ -159,7 +162,8 @@ drm_connector_has_preferred_mode(struct drm_connector *connector, int width, int return NULL; } -static struct drm_display_mode *drm_connector_pick_cmdline_mode(struct drm_connector *connector) +VISIBLE_IF_KUNIT struct drm_display_mode * +drm_connector_pick_cmdline_mode(struct drm_connector *connector) { struct drm_cmdline_mode *cmdline_mode; struct drm_display_mode *mode; @@ -215,6 +219,7 @@ static struct drm_display_mode *drm_connector_pick_cmdline_mode(struct drm_conne return NULL; } +EXPORT_SYMBOL_IF_KUNIT(drm_connector_pick_cmdline_mode); static bool drm_connector_enabled(struct drm_connector *connector, bool strict) { @@ -1233,7 +1238,3 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode) return ret; } EXPORT_SYMBOL(drm_client_modeset_dpms); - -#ifdef CONFIG_DRM_KUNIT_TEST -#include "tests/drm_client_modeset_test.c" -#endif
Commit 8fc0380f6ba7 ("drm/client: Add some tests for drm_connector_pick_cmdline_mode()") was meant to introduce unit tests for the static drm_connector_pick_cmdline_mode() function. In such a case, the kunit documentation recommended to import the tests source file directly from the source file with the static function to test. While it was working, it's generally frowned upon. Fortunately, commit 9c988fae6f6a ("kunit: add macro to allow conditionally exposing static symbols to tests") introduced macros to easily deal with that case. We can thus remove our include and use those macros instead. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/drm_client_modeset.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)