| Message ID | 20240227085306.9764-6-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e6684d08cc1935774b07efc262e70b41ac1c0f93 |
| Headers | show |
| Series | Clean up locking with guard() in ALSA core | expand |
Hi Takashi, On Tue, Feb 27, 2024 at 09:52:47AM +0100, Takashi Iwai wrote: > We can simplify the code gracefully with new guard() macro and co for > automatic cleanup of locks. > > There are still a few remaining explicit mutex_lock/unlock calls, and > those are for the places where we do temporary unlock/relock, which > doesn't fit well with the guard(), so far. > > Only the code refactoring, and no functional changes. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > sound/core/hwdep.c | 85 +++++++++++++++++++++------------------------- > 1 file changed, 38 insertions(+), 47 deletions(-) > > diff --git a/sound/core/hwdep.c b/sound/core/hwdep.c > index de7476034f2c..9d7e5039c893 100644 > --- a/sound/core/hwdep.c > +++ b/sound/core/hwdep.c > @@ -149,12 +149,12 @@ static int snd_hwdep_release(struct inode *inode, struct file * file) > struct snd_hwdep *hw = file->private_data; > struct module *mod = hw->card->module; > > - mutex_lock(&hw->open_mutex); > - if (hw->ops.release) > - err = hw->ops.release(hw, file); > - if (hw->used > 0) > - hw->used--; > - mutex_unlock(&hw->open_mutex); > + scoped_guard(mutex, &hw->open_mutex) { > + if (hw->ops.release) > + err = hw->ops.release(hw, file); > + if (hw->used > 0) > + hw->used--; > + } > wake_up(&hw->open_wait); > > snd_card_file_remove(hw->card, file); > @@ -272,43 +272,43 @@ static int snd_hwdep_control_ioctl(struct snd_card *card, > > if (get_user(device, (int __user *)arg)) > return -EFAULT; > - mutex_lock(®ister_mutex); > > - if (device < 0) > - device = 0; > - else if (device < SNDRV_MINOR_HWDEPS) > - device++; > - else > - device = SNDRV_MINOR_HWDEPS; > + scoped_guard(mutex, ®ister_mutex) { > + if (device < 0) > + device = 0; > + else if (device < SNDRV_MINOR_HWDEPS) > + device++; > + else > + device = SNDRV_MINOR_HWDEPS; > > - while (device < SNDRV_MINOR_HWDEPS) { > - if (snd_hwdep_search(card, device)) > - break; > - device++; > + while (device < SNDRV_MINOR_HWDEPS) { > + if (snd_hwdep_search(card, device)) > + break; > + device++; > + } > + if (device >= SNDRV_MINOR_HWDEPS) > + device = -1; > + if (put_user(device, (int __user *)arg)) > + return -EFAULT; > + return 0; > } > - if (device >= SNDRV_MINOR_HWDEPS) > - device = -1; > - mutex_unlock(®ister_mutex); > - if (put_user(device, (int __user *)arg)) > - return -EFAULT; > - return 0; > + break; > } Due to a bug in clang that was resolved in clang-17 [1], clang-13 through clang-16 fail to build this file for ARCH=powerpc after this change in -next as commit e6684d08cc19 ("ALSA: hwdep: Use guard() for locking"): sound/core/hwdep.c:291:9: error: cannot jump from this asm goto statement to one of its possible targets if (put_user(device, (int __user *)arg)) ^ arch/powerpc/include/asm/uaccess.h:66:5: note: expanded from macro 'put_user' __put_user(x, _pu_addr) : -EFAULT; \ ^ arch/powerpc/include/asm/uaccess.h:48:3: note: expanded from macro '__put_user' __put_user_size_goto(__pu_val, __pu_addr, __pu_size, __pu_failed); \ ^ arch/powerpc/include/asm/uaccess.h:116:10: note: expanded from macro '__put_user_size_goto' case 1: __put_user_asm_goto(x, __pus_addr, label, "stb"); break; \ ^ arch/powerpc/include/asm/uaccess.h:86:2: note: expanded from macro '__put_user_asm_goto' asm goto( \ ^ sound/core/hwdep.c:273:8: note: possible target of asm goto statement if (get_user(device, (int __user *)arg)) ^ arch/powerpc/include/asm/uaccess.h:295:5: note: expanded from macro 'get_user' __get_user(x, _gu_addr) : \ ^ arch/powerpc/include/asm/uaccess.h:283:2: note: expanded from macro '__get_user' __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \ ^ arch/powerpc/include/asm/uaccess.h:201:16: note: expanded from macro '__get_user_size_allowed' break; \ ^ sound/core/hwdep.c:276:4: note: jump exits scope of variable with __attribute__((cleanup)) scoped_guard(mutex, ®ister_mutex) { ^ include/linux/cleanup.h:169:20: note: expanded from macro 'scoped_guard' for (CLASS(_name, scope)(args), \ ^ sound/core/hwdep.c:273:8: error: cannot jump from this asm goto statement to one of its possible targets if (get_user(device, (int __user *)arg)) ^ arch/powerpc/include/asm/uaccess.h:295:5: note: expanded from macro 'get_user' __get_user(x, _gu_addr) : \ ^ arch/powerpc/include/asm/uaccess.h:283:2: note: expanded from macro '__get_user' __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \ ^ arch/powerpc/include/asm/uaccess.h:199:3: note: expanded from macro '__get_user_size_allowed' __get_user_size_goto(x, ptr, size, __gus_failed); \ ^ arch/powerpc/include/asm/uaccess.h:187:10: note: expanded from macro '__get_user_size_goto' case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \ ^ arch/powerpc/include/asm/uaccess.h:158:2: note: expanded from macro '__get_user_asm_goto' asm_goto_output( \ ^ include/linux/compiler_types.h:380:31: note: expanded from macro 'asm_goto_output' #define asm_goto_output(x...) asm volatile goto(x) ^ sound/core/hwdep.c:291:9: note: possible target of asm goto statement if (put_user(device, (int __user *)arg)) ^ arch/powerpc/include/asm/uaccess.h:66:5: note: expanded from macro 'put_user' __put_user(x, _pu_addr) : -EFAULT; \ ^ arch/powerpc/include/asm/uaccess.h:52:9: note: expanded from macro '__put_user' \ ^ sound/core/hwdep.c:276:4: note: jump bypasses initialization of variable with __attribute__((cleanup)) scoped_guard(mutex, ®ister_mutex) { ^ include/linux/cleanup.h:169:20: note: expanded from macro 'scoped_guard' for (CLASS(_name, scope)(args), \ ^ 2 errors generated. However, looking at the original change, could this be avoided/worked around by just moving the put_user() call out of the scoped_guard()? The put_user() call was not originally under register_mutex, so it seems like this would work? As I understand it, mutex_unlock() will be called once the scope of the guard is left. I am happy to send a formal patch. [1]: https://github.com/ClangBuiltLinux/linux/issues/1886 diff --git a/sound/core/hwdep.c b/sound/core/hwdep.c index 9d7e5039c893..09200df2932c 100644 --- a/sound/core/hwdep.c +++ b/sound/core/hwdep.c @@ -288,11 +288,10 @@ static int snd_hwdep_control_ioctl(struct snd_card *card, } if (device >= SNDRV_MINOR_HWDEPS) device = -1; - if (put_user(device, (int __user *)arg)) - return -EFAULT; - return 0; } - break; + if (put_user(device, (int __user *)arg)) + return -EFAULT; + return 0; } case SNDRV_CTL_IOCTL_HWDEP_INFO: {
On Thu, 29 Feb 2024 22:00:34 +0100, Nathan Chancellor wrote: > > Hi Takashi, > > On Tue, Feb 27, 2024 at 09:52:47AM +0100, Takashi Iwai wrote: > > We can simplify the code gracefully with new guard() macro and co for > > automatic cleanup of locks. > > > > There are still a few remaining explicit mutex_lock/unlock calls, and > > those are for the places where we do temporary unlock/relock, which > > doesn't fit well with the guard(), so far. > > > > Only the code refactoring, and no functional changes. > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > sound/core/hwdep.c | 85 +++++++++++++++++++++------------------------- > > 1 file changed, 38 insertions(+), 47 deletions(-) > > > > diff --git a/sound/core/hwdep.c b/sound/core/hwdep.c > > index de7476034f2c..9d7e5039c893 100644 > > --- a/sound/core/hwdep.c > > +++ b/sound/core/hwdep.c > > @@ -149,12 +149,12 @@ static int snd_hwdep_release(struct inode *inode, struct file * file) > > struct snd_hwdep *hw = file->private_data; > > struct module *mod = hw->card->module; > > > > - mutex_lock(&hw->open_mutex); > > - if (hw->ops.release) > > - err = hw->ops.release(hw, file); > > - if (hw->used > 0) > > - hw->used--; > > - mutex_unlock(&hw->open_mutex); > > + scoped_guard(mutex, &hw->open_mutex) { > > + if (hw->ops.release) > > + err = hw->ops.release(hw, file); > > + if (hw->used > 0) > > + hw->used--; > > + } > > wake_up(&hw->open_wait); > > > > snd_card_file_remove(hw->card, file); > > @@ -272,43 +272,43 @@ static int snd_hwdep_control_ioctl(struct snd_card *card, > > > > if (get_user(device, (int __user *)arg)) > > return -EFAULT; > > - mutex_lock(®ister_mutex); > > > > - if (device < 0) > > - device = 0; > > - else if (device < SNDRV_MINOR_HWDEPS) > > - device++; > > - else > > - device = SNDRV_MINOR_HWDEPS; > > + scoped_guard(mutex, ®ister_mutex) { > > + if (device < 0) > > + device = 0; > > + else if (device < SNDRV_MINOR_HWDEPS) > > + device++; > > + else > > + device = SNDRV_MINOR_HWDEPS; > > > > - while (device < SNDRV_MINOR_HWDEPS) { > > - if (snd_hwdep_search(card, device)) > > - break; > > - device++; > > + while (device < SNDRV_MINOR_HWDEPS) { > > + if (snd_hwdep_search(card, device)) > > + break; > > + device++; > > + } > > + if (device >= SNDRV_MINOR_HWDEPS) > > + device = -1; > > + if (put_user(device, (int __user *)arg)) > > + return -EFAULT; > > + return 0; > > } > > - if (device >= SNDRV_MINOR_HWDEPS) > > - device = -1; > > - mutex_unlock(®ister_mutex); > > - if (put_user(device, (int __user *)arg)) > > - return -EFAULT; > > - return 0; > > + break; > > } > > Due to a bug in clang that was resolved in clang-17 [1], clang-13 > through clang-16 fail to build this file for ARCH=powerpc after this > change in -next as commit e6684d08cc19 ("ALSA: hwdep: Use guard() for > locking"): > > sound/core/hwdep.c:291:9: error: cannot jump from this asm goto statement to one of its possible targets > if (put_user(device, (int __user *)arg)) > ^ > arch/powerpc/include/asm/uaccess.h:66:5: note: expanded from macro 'put_user' > __put_user(x, _pu_addr) : -EFAULT; \ > ^ > arch/powerpc/include/asm/uaccess.h:48:3: note: expanded from macro '__put_user' > __put_user_size_goto(__pu_val, __pu_addr, __pu_size, __pu_failed); \ > ^ > arch/powerpc/include/asm/uaccess.h:116:10: note: expanded from macro '__put_user_size_goto' > case 1: __put_user_asm_goto(x, __pus_addr, label, "stb"); break; \ > ^ > arch/powerpc/include/asm/uaccess.h:86:2: note: expanded from macro '__put_user_asm_goto' > asm goto( \ > ^ > sound/core/hwdep.c:273:8: note: possible target of asm goto statement > if (get_user(device, (int __user *)arg)) > ^ > arch/powerpc/include/asm/uaccess.h:295:5: note: expanded from macro 'get_user' > __get_user(x, _gu_addr) : \ > ^ > arch/powerpc/include/asm/uaccess.h:283:2: note: expanded from macro '__get_user' > __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \ > ^ > arch/powerpc/include/asm/uaccess.h:201:16: note: expanded from macro '__get_user_size_allowed' > break; \ > ^ > sound/core/hwdep.c:276:4: note: jump exits scope of variable with __attribute__((cleanup)) > scoped_guard(mutex, ®ister_mutex) { > ^ > include/linux/cleanup.h:169:20: note: expanded from macro 'scoped_guard' > for (CLASS(_name, scope)(args), \ > ^ > sound/core/hwdep.c:273:8: error: cannot jump from this asm goto statement to one of its possible targets > if (get_user(device, (int __user *)arg)) > ^ > arch/powerpc/include/asm/uaccess.h:295:5: note: expanded from macro 'get_user' > __get_user(x, _gu_addr) : \ > ^ > arch/powerpc/include/asm/uaccess.h:283:2: note: expanded from macro '__get_user' > __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \ > ^ > arch/powerpc/include/asm/uaccess.h:199:3: note: expanded from macro '__get_user_size_allowed' > __get_user_size_goto(x, ptr, size, __gus_failed); \ > ^ > arch/powerpc/include/asm/uaccess.h:187:10: note: expanded from macro '__get_user_size_goto' > case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \ > ^ > arch/powerpc/include/asm/uaccess.h:158:2: note: expanded from macro '__get_user_asm_goto' > asm_goto_output( \ > ^ > include/linux/compiler_types.h:380:31: note: expanded from macro 'asm_goto_output' > #define asm_goto_output(x...) asm volatile goto(x) > ^ > sound/core/hwdep.c:291:9: note: possible target of asm goto statement > if (put_user(device, (int __user *)arg)) > ^ > arch/powerpc/include/asm/uaccess.h:66:5: note: expanded from macro 'put_user' > __put_user(x, _pu_addr) : -EFAULT; \ > ^ > arch/powerpc/include/asm/uaccess.h:52:9: note: expanded from macro '__put_user' > \ > ^ > sound/core/hwdep.c:276:4: note: jump bypasses initialization of variable with __attribute__((cleanup)) > scoped_guard(mutex, ®ister_mutex) { > ^ > include/linux/cleanup.h:169:20: note: expanded from macro 'scoped_guard' > for (CLASS(_name, scope)(args), \ > ^ > 2 errors generated. > > However, looking at the original change, could this be avoided/worked > around by just moving the put_user() call out of the scoped_guard()? > The put_user() call was not originally under register_mutex, so it seems > like this would work? As I understand it, mutex_unlock() will be called > once the scope of the guard is left. I am happy to send a formal patch. Yes, that was my intention but somehow it slipped :-< Could you submit the fix patch? Thanks! Takashi
On Fri, Mar 01, 2024 at 08:27:43AM +0100, Takashi Iwai wrote: > On Thu, 29 Feb 2024 22:00:34 +0100, > Nathan Chancellor wrote: > > > > Hi Takashi, > > > > On Tue, Feb 27, 2024 at 09:52:47AM +0100, Takashi Iwai wrote: > > > We can simplify the code gracefully with new guard() macro and co for > > > automatic cleanup of locks. > > > > > > There are still a few remaining explicit mutex_lock/unlock calls, and > > > those are for the places where we do temporary unlock/relock, which > > > doesn't fit well with the guard(), so far. > > > > > > Only the code refactoring, and no functional changes. > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > --- > > > sound/core/hwdep.c | 85 +++++++++++++++++++++------------------------- > > > 1 file changed, 38 insertions(+), 47 deletions(-) > > > > > > diff --git a/sound/core/hwdep.c b/sound/core/hwdep.c > > > index de7476034f2c..9d7e5039c893 100644 > > > --- a/sound/core/hwdep.c > > > +++ b/sound/core/hwdep.c > > > @@ -149,12 +149,12 @@ static int snd_hwdep_release(struct inode *inode, struct file * file) > > > struct snd_hwdep *hw = file->private_data; > > > struct module *mod = hw->card->module; > > > > > > - mutex_lock(&hw->open_mutex); > > > - if (hw->ops.release) > > > - err = hw->ops.release(hw, file); > > > - if (hw->used > 0) > > > - hw->used--; > > > - mutex_unlock(&hw->open_mutex); > > > + scoped_guard(mutex, &hw->open_mutex) { > > > + if (hw->ops.release) > > > + err = hw->ops.release(hw, file); > > > + if (hw->used > 0) > > > + hw->used--; > > > + } > > > wake_up(&hw->open_wait); > > > > > > snd_card_file_remove(hw->card, file); > > > @@ -272,43 +272,43 @@ static int snd_hwdep_control_ioctl(struct snd_card *card, > > > > > > if (get_user(device, (int __user *)arg)) > > > return -EFAULT; > > > - mutex_lock(®ister_mutex); > > > > > > - if (device < 0) > > > - device = 0; > > > - else if (device < SNDRV_MINOR_HWDEPS) > > > - device++; > > > - else > > > - device = SNDRV_MINOR_HWDEPS; > > > + scoped_guard(mutex, ®ister_mutex) { > > > + if (device < 0) > > > + device = 0; > > > + else if (device < SNDRV_MINOR_HWDEPS) > > > + device++; > > > + else > > > + device = SNDRV_MINOR_HWDEPS; > > > > > > - while (device < SNDRV_MINOR_HWDEPS) { > > > - if (snd_hwdep_search(card, device)) > > > - break; > > > - device++; > > > + while (device < SNDRV_MINOR_HWDEPS) { > > > + if (snd_hwdep_search(card, device)) > > > + break; > > > + device++; > > > + } > > > + if (device >= SNDRV_MINOR_HWDEPS) > > > + device = -1; > > > + if (put_user(device, (int __user *)arg)) > > > + return -EFAULT; > > > + return 0; > > > } > > > - if (device >= SNDRV_MINOR_HWDEPS) > > > - device = -1; > > > - mutex_unlock(®ister_mutex); > > > - if (put_user(device, (int __user *)arg)) > > > - return -EFAULT; > > > - return 0; > > > + break; > > > } > > > > Due to a bug in clang that was resolved in clang-17 [1], clang-13 > > through clang-16 fail to build this file for ARCH=powerpc after this > > change in -next as commit e6684d08cc19 ("ALSA: hwdep: Use guard() for > > locking"): > > > > sound/core/hwdep.c:291:9: error: cannot jump from this asm goto statement to one of its possible targets > > if (put_user(device, (int __user *)arg)) > > ^ > > arch/powerpc/include/asm/uaccess.h:66:5: note: expanded from macro 'put_user' > > __put_user(x, _pu_addr) : -EFAULT; \ > > ^ > > arch/powerpc/include/asm/uaccess.h:48:3: note: expanded from macro '__put_user' > > __put_user_size_goto(__pu_val, __pu_addr, __pu_size, __pu_failed); \ > > ^ > > arch/powerpc/include/asm/uaccess.h:116:10: note: expanded from macro '__put_user_size_goto' > > case 1: __put_user_asm_goto(x, __pus_addr, label, "stb"); break; \ > > ^ > > arch/powerpc/include/asm/uaccess.h:86:2: note: expanded from macro '__put_user_asm_goto' > > asm goto( \ > > ^ > > sound/core/hwdep.c:273:8: note: possible target of asm goto statement > > if (get_user(device, (int __user *)arg)) > > ^ > > arch/powerpc/include/asm/uaccess.h:295:5: note: expanded from macro 'get_user' > > __get_user(x, _gu_addr) : \ > > ^ > > arch/powerpc/include/asm/uaccess.h:283:2: note: expanded from macro '__get_user' > > __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \ > > ^ > > arch/powerpc/include/asm/uaccess.h:201:16: note: expanded from macro '__get_user_size_allowed' > > break; \ > > ^ > > sound/core/hwdep.c:276:4: note: jump exits scope of variable with __attribute__((cleanup)) > > scoped_guard(mutex, ®ister_mutex) { > > ^ > > include/linux/cleanup.h:169:20: note: expanded from macro 'scoped_guard' > > for (CLASS(_name, scope)(args), \ > > ^ > > sound/core/hwdep.c:273:8: error: cannot jump from this asm goto statement to one of its possible targets > > if (get_user(device, (int __user *)arg)) > > ^ > > arch/powerpc/include/asm/uaccess.h:295:5: note: expanded from macro 'get_user' > > __get_user(x, _gu_addr) : \ > > ^ > > arch/powerpc/include/asm/uaccess.h:283:2: note: expanded from macro '__get_user' > > __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \ > > ^ > > arch/powerpc/include/asm/uaccess.h:199:3: note: expanded from macro '__get_user_size_allowed' > > __get_user_size_goto(x, ptr, size, __gus_failed); \ > > ^ > > arch/powerpc/include/asm/uaccess.h:187:10: note: expanded from macro '__get_user_size_goto' > > case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \ > > ^ > > arch/powerpc/include/asm/uaccess.h:158:2: note: expanded from macro '__get_user_asm_goto' > > asm_goto_output( \ > > ^ > > include/linux/compiler_types.h:380:31: note: expanded from macro 'asm_goto_output' > > #define asm_goto_output(x...) asm volatile goto(x) > > ^ > > sound/core/hwdep.c:291:9: note: possible target of asm goto statement > > if (put_user(device, (int __user *)arg)) > > ^ > > arch/powerpc/include/asm/uaccess.h:66:5: note: expanded from macro 'put_user' > > __put_user(x, _pu_addr) : -EFAULT; \ > > ^ > > arch/powerpc/include/asm/uaccess.h:52:9: note: expanded from macro '__put_user' > > \ > > ^ > > sound/core/hwdep.c:276:4: note: jump bypasses initialization of variable with __attribute__((cleanup)) > > scoped_guard(mutex, ®ister_mutex) { > > ^ > > include/linux/cleanup.h:169:20: note: expanded from macro 'scoped_guard' > > for (CLASS(_name, scope)(args), \ > > ^ > > 2 errors generated. > > > > However, looking at the original change, could this be avoided/worked > > around by just moving the put_user() call out of the scoped_guard()? > > The put_user() call was not originally under register_mutex, so it seems > > like this would work? As I understand it, mutex_unlock() will be called > > once the scope of the guard is left. I am happy to send a formal patch. > > Yes, that was my intention but somehow it slipped :-< > Could you submit the fix patch? Sure thing: https://lore.kernel.org/20240301-fix-snd-hwdep-guard-v1-1-6aab033f3f83@kernel.org/ Thanks for the input! Cheers, Nathan
diff --git a/sound/core/hwdep.c b/sound/core/hwdep.c index de7476034f2c..9d7e5039c893 100644 --- a/sound/core/hwdep.c +++ b/sound/core/hwdep.c @@ -149,12 +149,12 @@ static int snd_hwdep_release(struct inode *inode, struct file * file) struct snd_hwdep *hw = file->private_data; struct module *mod = hw->card->module; - mutex_lock(&hw->open_mutex); - if (hw->ops.release) - err = hw->ops.release(hw, file); - if (hw->used > 0) - hw->used--; - mutex_unlock(&hw->open_mutex); + scoped_guard(mutex, &hw->open_mutex) { + if (hw->ops.release) + err = hw->ops.release(hw, file); + if (hw->used > 0) + hw->used--; + } wake_up(&hw->open_wait); snd_card_file_remove(hw->card, file); @@ -272,43 +272,43 @@ static int snd_hwdep_control_ioctl(struct snd_card *card, if (get_user(device, (int __user *)arg)) return -EFAULT; - mutex_lock(®ister_mutex); - if (device < 0) - device = 0; - else if (device < SNDRV_MINOR_HWDEPS) - device++; - else - device = SNDRV_MINOR_HWDEPS; + scoped_guard(mutex, ®ister_mutex) { + if (device < 0) + device = 0; + else if (device < SNDRV_MINOR_HWDEPS) + device++; + else + device = SNDRV_MINOR_HWDEPS; - while (device < SNDRV_MINOR_HWDEPS) { - if (snd_hwdep_search(card, device)) - break; - device++; + while (device < SNDRV_MINOR_HWDEPS) { + if (snd_hwdep_search(card, device)) + break; + device++; + } + if (device >= SNDRV_MINOR_HWDEPS) + device = -1; + if (put_user(device, (int __user *)arg)) + return -EFAULT; + return 0; } - if (device >= SNDRV_MINOR_HWDEPS) - device = -1; - mutex_unlock(®ister_mutex); - if (put_user(device, (int __user *)arg)) - return -EFAULT; - return 0; + break; } case SNDRV_CTL_IOCTL_HWDEP_INFO: { struct snd_hwdep_info __user *info = (struct snd_hwdep_info __user *)arg; - int device, err; + int device; struct snd_hwdep *hwdep; if (get_user(device, &info->device)) return -EFAULT; - mutex_lock(®ister_mutex); - hwdep = snd_hwdep_search(card, device); - if (hwdep) - err = snd_hwdep_info(hwdep, info); - else - err = -ENXIO; - mutex_unlock(®ister_mutex); - return err; + scoped_guard(mutex, ®ister_mutex) { + hwdep = snd_hwdep_search(card, device); + if (!hwdep) + return -ENXIO; + return snd_hwdep_info(hwdep, info); + } + break; } } return -ENOIOCTLCMD; @@ -422,11 +422,9 @@ static int snd_hwdep_dev_register(struct snd_device *device) struct snd_card *card = hwdep->card; int err; - mutex_lock(®ister_mutex); - if (snd_hwdep_search(card, hwdep->device)) { - mutex_unlock(®ister_mutex); + guard(mutex)(®ister_mutex); + if (snd_hwdep_search(card, hwdep->device)) return -EBUSY; - } list_add_tail(&hwdep->list, &snd_hwdep_devices); err = snd_register_device(SNDRV_DEVICE_TYPE_HWDEP, hwdep->card, hwdep->device, @@ -434,7 +432,6 @@ static int snd_hwdep_dev_register(struct snd_device *device) if (err < 0) { dev_err(hwdep->dev, "unable to register\n"); list_del(&hwdep->list); - mutex_unlock(®ister_mutex); return err; } @@ -454,7 +451,6 @@ static int snd_hwdep_dev_register(struct snd_device *device) hwdep->ossreg = 1; } #endif - mutex_unlock(®ister_mutex); return 0; } @@ -464,12 +460,10 @@ static int snd_hwdep_dev_disconnect(struct snd_device *device) if (snd_BUG_ON(!hwdep)) return -ENXIO; - mutex_lock(®ister_mutex); - if (snd_hwdep_search(hwdep->card, hwdep->device) != hwdep) { - mutex_unlock(®ister_mutex); + guard(mutex)(®ister_mutex); + if (snd_hwdep_search(hwdep->card, hwdep->device) != hwdep) return -EINVAL; - } - mutex_lock(&hwdep->open_mutex); + guard(mutex)(&hwdep->open_mutex); wake_up(&hwdep->open_wait); #ifdef CONFIG_SND_OSSEMUL if (hwdep->ossreg) @@ -477,8 +471,6 @@ static int snd_hwdep_dev_disconnect(struct snd_device *device) #endif snd_unregister_device(hwdep->dev); list_del_init(&hwdep->list); - mutex_unlock(&hwdep->open_mutex); - mutex_unlock(®ister_mutex); return 0; } @@ -492,11 +484,10 @@ static void snd_hwdep_proc_read(struct snd_info_entry *entry, { struct snd_hwdep *hwdep; - mutex_lock(®ister_mutex); + guard(mutex)(®ister_mutex); list_for_each_entry(hwdep, &snd_hwdep_devices, list) snd_iprintf(buffer, "%02i-%02i: %s\n", hwdep->card->number, hwdep->device, hwdep->name); - mutex_unlock(®ister_mutex); } static struct snd_info_entry *snd_hwdep_proc_entry;
We can simplify the code gracefully with new guard() macro and co for automatic cleanup of locks. There are still a few remaining explicit mutex_lock/unlock calls, and those are for the places where we do temporary unlock/relock, which doesn't fit well with the guard(), so far. Only the code refactoring, and no functional changes. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/core/hwdep.c | 85 +++++++++++++++++++++------------------------- 1 file changed, 38 insertions(+), 47 deletions(-)