From: Louis Chauvet <louis.chauvet@bootlin.com>
As the configuration will be used by userspace, add a validator to avoid
creating a broken DRM device.
For the moment, the function always returns true, but rules will be
added in future patches.
Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
Co-developed-by: José Expósito <jose.exposito89@gmail.com>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
drivers/gpu/drm/vkms/tests/vkms_config_test.c | 2 ++
drivers/gpu/drm/vkms/vkms_config.c | 5 +++++
drivers/gpu/drm/vkms/vkms_config.h | 10 ++++++++++
drivers/gpu/drm/vkms/vkms_drv.h | 2 +-
drivers/gpu/drm/vkms/vkms_output.c | 3 +++
5 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vkms/tests/vkms_config_test.c b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
index 92798590051b..6e07139d261c 100644
--- a/drivers/gpu/drm/vkms/tests/vkms_config_test.c
+++ b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
@@ -54,6 +54,8 @@ static void vkms_config_test_default_config(struct kunit *test)
KUNIT_EXPECT_EQ(test, config->writeback, params->enable_writeback);
KUNIT_EXPECT_EQ(test, config->overlay, params->enable_overlay);
+ KUNIT_EXPECT_TRUE(test, vkms_config_is_valid(config));
+
vkms_config_destroy(config);
}
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index 11b0e539920b..67f71d29596e 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -47,6 +47,11 @@ void vkms_config_destroy(struct vkms_config *config)
kfree(config);
}
+bool vkms_config_is_valid(struct vkms_config *config)
+{
+ return true;
+}
+
static int vkms_config_show(struct seq_file *m, void *data)
{
struct drm_debugfs_entry *entry = m->private;
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index fcaa909fb2e0..0376dceaf6be 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -67,6 +67,16 @@ vkms_config_get_device_name(struct vkms_config *config)
return config->dev_name;
}
+/**
+ * vkms_config_is_valid() - Validate a configuration
+ * @config: Configuration to validate
+ *
+ * Returns:
+ * Whether the configuration is valid or not.
+ * For example, a configuration without primary planes is not valid.
+ */
+bool vkms_config_is_valid(struct vkms_config *config);
+
/**
* vkms_config_register_debugfs() - Register a debugfs file to show the device's
* configuration
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index a74a7fc3a056..95afc39ce985 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -204,7 +204,7 @@ struct vkms_config;
struct vkms_device {
struct drm_device drm;
struct platform_device *platform;
- const struct vkms_config *config;
+ struct vkms_config *config;
};
/*
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 068a7f87ecec..414cc933af41 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -16,6 +16,9 @@ int vkms_output_init(struct vkms_device *vkmsdev)
int writeback;
unsigned int n;
+ if (!vkms_config_is_valid(vkmsdev->config))
+ return -EINVAL;
+
/*
* Initialize used plane. One primary plane is required to perform the composition.
*
--
2.48.1
On 11/02/25 - 12:09, José Expósito wrote:
> From: Louis Chauvet <louis.chauvet@bootlin.com>
>
> As the configuration will be used by userspace, add a validator to avoid
> creating a broken DRM device.
>
> For the moment, the function always returns true, but rules will be
> added in future patches.
>
> Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> Co-developed-by: José Expósito <jose.exposito89@gmail.com>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
The compilation is broken when building as module:
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index b9267aef4804..82335006c94a 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -55,6 +55,7 @@ bool vkms_config_is_valid(struct vkms_config *config)
{
return true;
}
+EXPORT_SYMBOL_IF_KUNIT(vkms_config_is_valid);
static int vkms_config_show(struct seq_file *m, void *data)
{
> ---
> drivers/gpu/drm/vkms/tests/vkms_config_test.c | 2 ++
> drivers/gpu/drm/vkms/vkms_config.c | 5 +++++
> drivers/gpu/drm/vkms/vkms_config.h | 10 ++++++++++
> drivers/gpu/drm/vkms/vkms_drv.h | 2 +-
> drivers/gpu/drm/vkms/vkms_output.c | 3 +++
> 5 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vkms/tests/vkms_config_test.c b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> index 92798590051b..6e07139d261c 100644
> --- a/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> +++ b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> @@ -54,6 +54,8 @@ static void vkms_config_test_default_config(struct kunit *test)
> KUNIT_EXPECT_EQ(test, config->writeback, params->enable_writeback);
> KUNIT_EXPECT_EQ(test, config->overlay, params->enable_overlay);
>
> + KUNIT_EXPECT_TRUE(test, vkms_config_is_valid(config));
> +
> vkms_config_destroy(config);
> }
>
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index 11b0e539920b..67f71d29596e 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -47,6 +47,11 @@ void vkms_config_destroy(struct vkms_config *config)
> kfree(config);
> }
>
> +bool vkms_config_is_valid(struct vkms_config *config)
> +{
> + return true;
> +}
> +
> static int vkms_config_show(struct seq_file *m, void *data)
> {
> struct drm_debugfs_entry *entry = m->private;
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> index fcaa909fb2e0..0376dceaf6be 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -67,6 +67,16 @@ vkms_config_get_device_name(struct vkms_config *config)
> return config->dev_name;
> }
>
> +/**
> + * vkms_config_is_valid() - Validate a configuration
> + * @config: Configuration to validate
> + *
> + * Returns:
> + * Whether the configuration is valid or not.
> + * For example, a configuration without primary planes is not valid.
> + */
> +bool vkms_config_is_valid(struct vkms_config *config);
> +
I think here we can take a const pointer.
> /**
> * vkms_config_register_debugfs() - Register a debugfs file to show the device's
> * configuration
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index a74a7fc3a056..95afc39ce985 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -204,7 +204,7 @@ struct vkms_config;
> struct vkms_device {
> struct drm_device drm;
> struct platform_device *platform;
> - const struct vkms_config *config;
> + struct vkms_config *config;
So we can keep a const pointer here (for me the device should never modify
vkms_config)
> };
>
> /*
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 068a7f87ecec..414cc933af41 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -16,6 +16,9 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> int writeback;
> unsigned int n;
>
> + if (!vkms_config_is_valid(vkmsdev->config))
> + return -EINVAL;
> +
> /*
> * Initialize used plane. One primary plane is required to perform the composition.
> *
> --
> 2.48.1
>
On Thu, Feb 13, 2025 at 02:59:52PM +0100, Louis Chauvet wrote:
> On 11/02/25 - 12:09, José Expósito wrote:
> > From: Louis Chauvet <louis.chauvet@bootlin.com>
> >
> > As the configuration will be used by userspace, add a validator to avoid
> > creating a broken DRM device.
> >
> > For the moment, the function always returns true, but rules will be
> > added in future patches.
> >
> > Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > Co-developed-by: José Expósito <jose.exposito89@gmail.com>
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
>
> The compilation is broken when building as module:
>
>
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index b9267aef4804..82335006c94a 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -55,6 +55,7 @@ bool vkms_config_is_valid(struct vkms_config *config)
> {
> return true;
> }
> +EXPORT_SYMBOL_IF_KUNIT(vkms_config_is_valid);
Fixed the issue in all patches, thanks!
> > [...]
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> > index fcaa909fb2e0..0376dceaf6be 100644
> > --- a/drivers/gpu/drm/vkms/vkms_config.h
> > +++ b/drivers/gpu/drm/vkms/vkms_config.h
> > @@ -67,6 +67,16 @@ vkms_config_get_device_name(struct vkms_config *config)
> > return config->dev_name;
> > }
> >
> > +/**
> > + * vkms_config_is_valid() - Validate a configuration
> > + * @config: Configuration to validate
> > + *
> > + * Returns:
> > + * Whether the configuration is valid or not.
> > + * For example, a configuration without primary planes is not valid.
> > + */
> > +bool vkms_config_is_valid(struct vkms_config *config);
> > +
>
> I think here we can take a const pointer.
>
> > /**
> > * vkms_config_register_debugfs() - Register a debugfs file to show the device's
> > * configuration
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index a74a7fc3a056..95afc39ce985 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -204,7 +204,7 @@ struct vkms_config;
> > struct vkms_device {
> > struct drm_device drm;
> > struct platform_device *platform;
> > - const struct vkms_config *config;
> > + struct vkms_config *config;
>
> So we can keep a const pointer here (for me the device should never modify
> vkms_config)
I tryed keeping the const pointer, but, since list_count_nodes() is used in
several valid_* functions and it takes a non-const pointer, it causes
warnings.
We can fix them with a cast:
n_planes = list_count_nodes((struct list_head *)&config->planes);
But I feel that keeping the "const" creates more issues than it solves.
Anyway, if you prefer this pointer to be const, I will change it in v3.
Jose
> > };
> >
> > /*
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 068a7f87ecec..414cc933af41 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -16,6 +16,9 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> > int writeback;
> > unsigned int n;
> >
> > + if (!vkms_config_is_valid(vkmsdev->config))
> > + return -EINVAL;
> > +
> > /*
> > * Initialize used plane. One primary plane is required to perform the composition.
> > *
> > --
> > 2.48.1
> >
Le 14/02/2025 à 16:53, José Expósito a écrit :
> On Thu, Feb 13, 2025 at 02:59:52PM +0100, Louis Chauvet wrote:
>> On 11/02/25 - 12:09, José Expósito wrote:
>>> From: Louis Chauvet <louis.chauvet@bootlin.com>
>>>
>>> As the configuration will be used by userspace, add a validator to avoid
>>> creating a broken DRM device.
>>>
>>> For the moment, the function always returns true, but rules will be
>>> added in future patches.
>>>
>>> Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>> Co-developed-by: José Expósito <jose.exposito89@gmail.com>
>>> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
>>
>> The compilation is broken when building as module:
>>
>>
>> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
>> index b9267aef4804..82335006c94a 100644
>> --- a/drivers/gpu/drm/vkms/vkms_config.c
>> +++ b/drivers/gpu/drm/vkms/vkms_config.c
>> @@ -55,6 +55,7 @@ bool vkms_config_is_valid(struct vkms_config *config)
>> {
>> return true;
>> }
>> +EXPORT_SYMBOL_IF_KUNIT(vkms_config_is_valid);
>
> Fixed the issue in all patches, thanks!
>
>>> [...]
>>>
>>> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
>>> index fcaa909fb2e0..0376dceaf6be 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_config.h
>>> +++ b/drivers/gpu/drm/vkms/vkms_config.h
>>> @@ -67,6 +67,16 @@ vkms_config_get_device_name(struct vkms_config *config)
>>> return config->dev_name;
>>> }
>>>
>>> +/**
>>> + * vkms_config_is_valid() - Validate a configuration
>>> + * @config: Configuration to validate
>>> + *
>>> + * Returns:
>>> + * Whether the configuration is valid or not.
>>> + * For example, a configuration without primary planes is not valid.
>>> + */
>>> +bool vkms_config_is_valid(struct vkms_config *config);
>>> +
>>
>> I think here we can take a const pointer.
>>
>>> /**
>>> * vkms_config_register_debugfs() - Register a debugfs file to show the device's
>>> * configuration
>>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
>>> index a74a7fc3a056..95afc39ce985 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_drv.h
>>> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
>>> @@ -204,7 +204,7 @@ struct vkms_config;
>>> struct vkms_device {
>>> struct drm_device drm;
>>> struct platform_device *platform;
>>> - const struct vkms_config *config;
>>> + struct vkms_config *config;
>>
>> So we can keep a const pointer here (for me the device should never modify
>> vkms_config)
>
> I tryed keeping the const pointer, but, since list_count_nodes() is used in
> several valid_* functions and it takes a non-const pointer, it causes
> warnings.
I did not see this issue, and obviously no casting is better!
But I don't understand why list_count_node needs a non-const pointer. I
just compile-tested an allyesconfig with a const parameter, it seems to
works without issue (no warning, at least on x86_64, I just started some
build on other architectures to check most of the drivers).
I will submit a patch next week to make the list_count_nodes parameter
const.
> We can fix them with a cast:
>
> n_planes = list_count_nodes((struct list_head *)&config->planes);
>
> But I feel that keeping the "const" creates more issues than it solves.
>
> Anyway, if you prefer this pointer to be const, I will change it in v3.
Please use a const pointer + cast, and we will remove it once the
list_count_nodes patch is merged.
Thanks,
Louis Chauvet
> Jose
>
>>> };
>>>
>>> /*
>>> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
>>> index 068a7f87ecec..414cc933af41 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_output.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_output.c
>>> @@ -16,6 +16,9 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>>> int writeback;
>>> unsigned int n;
>>>
>>> + if (!vkms_config_is_valid(vkmsdev->config))
>>> + return -EINVAL;
>>> +
>>> /*
>>> * Initialize used plane. One primary plane is required to perform the composition.
>>> *
>>> --
>>> 2.48.1
>>>
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
© 2016 - 2025 Red Hat, Inc.