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 <[email protected]> > > Co-developed-by: Louis Chauvet <[email protected]> > > Signed-off-by: Louis Chauvet <[email protected]> > > Signed-off-by: José Expósito <[email protected]> > > [...] > > 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
