Re: [PATCH v3 05/24] ALSA: hwdep: Use guard() for locking

[Takashi Iwai <tiwai@xxxxxxx>]

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(&register_mutex);
> >  
> > -			if (device < 0)
> > -				device = 0;
> > -			else if (device < SNDRV_MINOR_HWDEPS)
> > -				device++;
> > -			else
> > -				device = SNDRV_MINOR_HWDEPS;
> > +			scoped_guard(mutex, &register_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(&register_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, &register_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, &register_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