Le 18/08/2022 à 04:06, Alexey Klimov a écrit : > [Vous ne recevez pas souvent de courriers de aklimov@xxxxxxxxxx. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] > > watchdog_open() does not have wd_data->lock locks at all unlike > the watchdog_release() for instance. Also watchdog_open() calls > watchdog_start() that should be called with wd_data->lock acquired. > The mutex lock should be acquired in the beginning of the function > after getting struct watchdog_core_data wd_data pointer to deal with > different status fields and be able to call watchdog_start(); and > released on exit and on different error paths. Why do you need the mutex for the open ? open is protected via atomic operation test_and_set_bit(), so what ? If the mutex is hold while calling open, it means that the device is already open so test_and_set_bit will fail. > > Signed-off-by: Alexey Klimov <aklimov@xxxxxxxxxx> > --- > drivers/watchdog/watchdog_dev.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c > index 804236a094f6..d07a864036d9 100644 > --- a/drivers/watchdog/watchdog_dev.c > +++ b/drivers/watchdog/watchdog_dev.c > @@ -836,7 +836,7 @@ static int watchdog_open(struct inode *inode, struct file *file) > struct watchdog_core_data *wd_data; > struct watchdog_device *wdd; > bool hw_running; > - int err; > + int err = -EBUSY; > > /* Get the corresponding watchdog device */ > if (imajor(inode) == MISC_MAJOR) > @@ -845,9 +845,10 @@ static int watchdog_open(struct inode *inode, struct file *file) > wd_data = container_of(inode->i_cdev, struct watchdog_core_data, > cdev); > > + mutex_lock(&wd_data->lock); > /* the watchdog is single open! */ > if (test_and_set_bit(_WDOG_DEV_OPEN, &wd_data->status)) > - return -EBUSY; > + goto out_unlock; > > wdd = wd_data->wdd; > > @@ -856,10 +857,8 @@ static int watchdog_open(struct inode *inode, struct file *file) > * to be unloaded. > */ > hw_running = watchdog_hw_running(wdd); > - if (!hw_running && !try_module_get(wdd->ops->owner)) { > - err = -EBUSY; > + if (!hw_running && !try_module_get(wdd->ops->owner)) > goto out_clear; > - } > > err = watchdog_start(wdd); > if (err < 0) > @@ -878,6 +877,7 @@ static int watchdog_open(struct inode *inode, struct file *file) > * applied. > */ > wd_data->open_deadline = KTIME_MAX; > + mutex_unlock(&wd_data->lock); > > /* dev/watchdog is a virtual (and thus non-seekable) filesystem */ > return stream_open(inode, file); > @@ -886,6 +886,8 @@ static int watchdog_open(struct inode *inode, struct file *file) > module_put(wd_data->wdd->ops->owner); > out_clear: > clear_bit(_WDOG_DEV_OPEN, &wd_data->status); > +out_unlock: > + mutex_unlock(&wd_data->lock); > return err; > } > > -- > 2.37.2 >
