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