Re: [PATCH v2 08/15] drm/vkms: Add a validation function for VKMS configuration

[Louis Chauvet <louis.chauvet@bootlin.com>]

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