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@xxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx/
Thanks for the input!
Cheers,
Nathan
[Index of Archives]
[Pulseaudio]
[Linux Audio Users]
[ALSA Devel]
[Fedora Desktop]
[Fedora SELinux]
[Big List of Linux Books]
[Yosemite News]
[KDE Users]
