Re: [PATCH v2 02/16] drm/vkms: Add and remove VKMS instances via configfs

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

Hi Louis,

On Fri, Feb 28, 2025 at 04:19:13PM +0100, Louis Chauvet wrote:
> Le 25/02/2025 à 18:59, José Expósito a écrit :
> > Allow to create, enable, disable and destroy VKMS instances using
> > configfs.
> > 
> > For the moment, it is not possible to add pipeline items, so trying to
> > enable the device will fail printing an informative error to the log.
> > 
> > Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > Co-developed-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> > [...]
> > diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> > new file mode 100644
> > index 000000000000..92512d52ddae
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> > @@ -0,0 +1,169 @@
> > [...]
> > +static ssize_t device_enabled_store(struct config_item *item, const char *page,
> > +				    size_t count)
> > +{
> > +	struct vkms_configfs_device *dev;
> > +	bool enabled;
> > +	int ret = 0;
> > +
> > +	dev = device_item_to_vkms_configfs_device(item);
> > +
> > +	if (kstrtobool(page, &enabled))
> > +		return -EINVAL;
> > +
> > +	guard(mutex)(&dev->lock);
> > +
> > +	if (!dev->enabled && enabled) {
> > +		if (!vkms_config_is_valid(dev->config))
> > +			return -EINVAL;
> > +
> > +		ret = vkms_create(dev->config);
> > +		if (ret)
> > +			return ret;
> > +	} else if (dev->enabled && !enabled) {
> > +		vkms_destroy(dev->config);
> > +	}
> > +
> > +	dev->enabled = enabled;
> 
> Sorry, I was maybe not clear enough, and you may hate me: I don't like
> `guard(mutex)` :‑(. I proposed scoped_guard because it makes very clear when
> the mutex is taken/released.
> 
> For me guard(mutex) is almost the same as mutex_lock/unlock. Yes, your mutex
> is always released, but:
> - without reading the code carefully, you don't know you have a mutex (even
> worse than a mutex_lock because you don't have a bunch of mutex_unlock to
> remind you)
> - you keep it until the end of the function, which may lock your mutex for
> too long
> 
> The scoped_guard solves the two issues:
> - you are in a dedicated block + indentation, so it is very easy to see that
> you currently have a mutex
> - you know exactly when the mutex is released: leaving the block
> 
> I am very sorry to make you work twice on this

It is fine, don't worry :) I'll send v3 using scoped_guard() and addressing
other review comments.

Thanks for your reviews,
Jose