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

[José Expósito]

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@xxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxx>
> > Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
> > Co-developed-by: José Expósito <jose.exposito89@xxxxxxxxx>
> > Signed-off-by: José Expósito <jose.exposito89@xxxxxxxxx>
> 
> 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
> >