Re: [bug report] drm/vkms: Allow to configure multiple CRTCs

["José Expósito" <jose.exposito89@gmail.com>]

Hi Dan,

Thanks for sharing these warnings.

On Thu, Aug 07, 2025 at 06:53:12PM +0300, Dan Carpenter wrote:
> Hello José Expósito,
> 
> Commit 600df32dac40 ("drm/vkms: Allow to configure multiple CRTCs")
> from Feb 18, 2025 (linux-next), leads to the following Smatch static
> checker warning:
> 
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:220 vkms_config_test_get_planes() error: 'plane_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:258 vkms_config_test_get_crtcs() error: 'crtc_cfg2' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:300 vkms_config_test_get_encoders() error: 'encoder_cfg2' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:345 vkms_config_test_get_connectors() error: 'connector_cfg2' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:672 vkms_config_test_plane_attach_crtc() error: 'overlay_cfg' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:674 vkms_config_test_plane_attach_crtc() error: 'primary_cfg' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:676 vkms_config_test_plane_attach_crtc() error: 'cursor_cfg' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:685 vkms_config_test_plane_attach_crtc() error: 'crtc_cfg' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:746 vkms_config_test_plane_get_possible_crtcs() error: 'crtc_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:746 vkms_config_test_plane_get_possible_crtcs() error: 'plane_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:748 vkms_config_test_plane_get_possible_crtcs() error: 'crtc_cfg2' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:810 vkms_config_test_encoder_get_possible_crtcs() error: 'crtc_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:810 vkms_config_test_encoder_get_possible_crtcs() error: 'encoder_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:812 vkms_config_test_encoder_get_possible_crtcs() error: 'crtc_cfg2' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:876 vkms_config_test_connector_get_possible_encoders() error: 'connector_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:876 vkms_config_test_connector_get_possible_encoders() error: 'encoder_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:878 vkms_config_test_connector_get_possible_encoders() error: 'encoder_cfg2' dereferencing possible ERR_PTR()
> 
> drivers/gpu/drm/vkms/tests/vkms_config_test.c
>     231 static void vkms_config_test_get_crtcs(struct kunit *test)
>     232 {
>     233         struct vkms_config *config;
>     234         struct vkms_config_crtc *crtc_cfg;
>     235         struct vkms_config_crtc *crtc_cfg1, *crtc_cfg2;
>     236 
>     237         config = vkms_config_create("test");
>     238         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, config);
>     239 
>     240         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 0);
>     241         vkms_config_for_each_crtc(config, crtc_cfg)
>     242                 KUNIT_FAIL(test, "Unexpected CRTC");
>     243 
>     244         crtc_cfg1 = vkms_config_create_crtc(config);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This file has no error checking.
> 
> I didn't send an email about it at first because this is just test code so
> who cares, but I was recently burned by ignoring errors so now I'm going
> through a bunch of old warnings to say that, "Hey, if the author ignores the
> error checking that's fine, but I'm in the clear."
> 
>     245         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 1);

While the "crtc_cfg1" pointer is not checked, we check that the	number 
of CRTCs matches the expected value and...

>     246         vkms_config_for_each_crtc(config, crtc_cfg) {
>     247                 if (crtc_cfg != crtc_cfg1)
>     248                         KUNIT_FAIL(test, "Unexpected CRTC");
>     249         }

...that the stored CRTC matches the one returned by vkms_config_create_crtc().

By program logic, vkms_config_create_crtc() returns an error if allocation
fails:

```
struct vkms_config_crtc *vkms_config_create_crtc(struct vkms_config *config)
{
	struct vkms_config_crtc *crtc_cfg;

	crtc_cfg = kzalloc(sizeof(*crtc_cfg), GFP_KERNEL);
	if (!crtc_cfg)
		return ERR_PTR(-ENOMEM);
```

Therefore, the current validations make sure that the pointer is valid.

In other places, for example vkms_config_test_connector_get_possible_encoders(),
the return value of vkms_config_create_connector/encoder() is indeed not
checked. It is not a big deal, since it is test code, but anyways, I'll send a
patch checking for return values.

Not because I think it can be problematic, but because test code can be used to
learn how the API works and I prefer to be explicit about its usage.

Best wishes,
Jose

>     250 
>     251         crtc_cfg2 = vkms_config_create_crtc(config);
>     252         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 2);
>     253         vkms_config_for_each_crtc(config, crtc_cfg) {
>     254                 if (crtc_cfg != crtc_cfg1 && crtc_cfg != crtc_cfg2)
>     255                         KUNIT_FAIL(test, "Unexpected CRTC");
>     256         }
>     257 
> --> 258         vkms_config_destroy_crtc(config, crtc_cfg2);
>     259         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 1);
>     260         vkms_config_for_each_crtc(config, crtc_cfg) {
>     261                 if (crtc_cfg != crtc_cfg1)
>     262                         KUNIT_FAIL(test, "Unexpected CRTC");
>     263         }
>     264 
>     265         vkms_config_destroy(config);
>     266 }
> 
> regards,
> dan carpenter