[PATCH 1/2] ALSA: emu10k1: fix SNDRV_EMU10K1_IOCTL_SINGLE_STEP

[PATCH 1/2] ALSA: emu10k1: fix SNDRV_EMU10K1_IOCTL_SINGLE_STEP

[Oswald Buddenhagen]

- Use correct address limit for Audigy
- Use the right constant to actually make a step on Audigy
- Don't store *_DBG_STEP and the address in emu->fx8010.dbg, as
  otherwise unrelated operations would make steps, too

This is untested. as10k1 was never ported to Audigy anyway.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emufx.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
index 6a51aed59238..70ec2cb9efe8 100644
--- a/sound/pci/emu10k1/emufx.c
+++ b/sound/pci/emu10k1/emufx.c
@@ -2646,17 +2646,19 @@ static int snd_emu10k1_fx8010_ioctl(struct snd_hwdep * hw, struct file *file, un
 			return -EPERM;
 		if (get_user(addr, (unsigned int __user *)argp))
 			return -EFAULT;
-		if (addr > 0x1ff)
-			return -EINVAL;
-		if (emu->audigy)
-			snd_emu10k1_ptr_write(emu, A_DBG, 0, emu->fx8010.dbg |= A_DBG_SINGLE_STEP | addr);
-		else
-			snd_emu10k1_ptr_write(emu, DBG, 0, emu->fx8010.dbg |= EMU10K1_DBG_SINGLE_STEP | addr);
-		udelay(10);
-		if (emu->audigy)
-			snd_emu10k1_ptr_write(emu, A_DBG, 0, emu->fx8010.dbg |= A_DBG_SINGLE_STEP | A_DBG_STEP_ADDR | addr);
-		else
-			snd_emu10k1_ptr_write(emu, DBG, 0, emu->fx8010.dbg |= EMU10K1_DBG_SINGLE_STEP | EMU10K1_DBG_STEP | addr);
+		if (emu->audigy) {
+			if (addr > A_DBG_STEP_ADDR)
+				return -EINVAL;
+			snd_emu10k1_ptr_write(emu, A_DBG, 0, emu->fx8010.dbg |= A_DBG_SINGLE_STEP);
+			udelay(10);
+			snd_emu10k1_ptr_write(emu, A_DBG, 0, emu->fx8010.dbg | A_DBG_STEP | addr);
+		} else {
+			if (addr > EMU10K1_DBG_SINGLE_STEP_ADDR)
+				return -EINVAL;
+			snd_emu10k1_ptr_write(emu, DBG, 0, emu->fx8010.dbg |= EMU10K1_DBG_SINGLE_STEP);
+			udelay(10);
+			snd_emu10k1_ptr_write(emu, DBG, 0, emu->fx8010.dbg | EMU10K1_DBG_STEP | addr);
+		}
 		return 0;
 	case SNDRV_EMU10K1_IOCTL_DBG_READ:
 		if (emu->audigy)
-- 
2.40.0.152.g15d061e6df

[PATCH 2/2] ALSA: emu10k1: don't stop DSP in _snd_emu10k1_{,audigy_}init_efx()

[Oswald Buddenhagen]

These functions don't actually touch the DSP until they poke the code
into it, at which point it's temporarily stopped anyway. And fx8010.dbg
is already zero anyway.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emufx.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
index 70ec2cb9efe8..b43acf0611a2 100644
--- a/sound/pci/emu10k1/emufx.c
+++ b/sound/pci/emu10k1/emufx.c
@@ -1259,9 +1259,6 @@ static int _snd_emu10k1_audigy_init_efx(struct snd_emu10k1 *emu)
 	gpr_map[gpr++] = 0x0000ffff;
 	bit_shifter16 = gpr;
 
-	/* stop FX processor */
-	snd_emu10k1_ptr_write(emu, A_DBG, 0, (emu->fx8010.dbg = 0) | A_DBG_SINGLE_STEP);
-
 #if 1
 	/* PCM front Playback Volume (independent from stereo mix)
 	 * playback = 0 + ( gpr * FXBUS_PCM_LEFT_FRONT >> 31)
@@ -1899,9 +1896,6 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu)
 	tmp = 0x88;	/* we need 4 temporary GPR */
 	/* from 0x8c to 0xff is the area for tone control */
 
-	/* stop FX processor */
-	snd_emu10k1_ptr_write(emu, DBG, 0, (emu->fx8010.dbg = 0) | EMU10K1_DBG_SINGLE_STEP);
-
 	/*
 	 *  Process FX Buses
 	 */
-- 
2.40.0.152.g15d061e6df

Re: [PATCH 1/2] ALSA: emu10k1: fix SNDRV_EMU10K1_IOCTL_SINGLE_STEP

[kernel test robot]

Hi Oswald,

kernel test robot noticed the following build errors:

[auto build test ERROR on tiwai-sound/for-next]
[also build test ERROR on tiwai-sound/for-linus linus/master v6.3-rc7 next-20230421]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Oswald-Buddenhagen/ALSA-emu10k1-don-t-stop-DSP-in-_snd_emu10k1_-audigy_-init_efx/20230423-002213
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link:    https://lore.kernel.org/r/20230422161021.1144004-1-oswald.buddenhagen%40gmx.de
patch subject: [PATCH 1/2] ALSA: emu10k1: fix SNDRV_EMU10K1_IOCTL_SINGLE_STEP
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20230423/202304230435.xkjl2DgF-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/dfbd3fc49e3b9ec81160caee29b1d41476d5a6aa
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Oswald-Buddenhagen/ALSA-emu10k1-don-t-stop-DSP-in-_snd_emu10k1_-audigy_-init_efx/20230423-002213
        git checkout dfbd3fc49e3b9ec81160caee29b1d41476d5a6aa
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 olddefconfig
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304230435.xkjl2DgF-lkp@intel.com/

All errors (new ones prefixed by >>):

   sound/pci/emu10k1/emufx.c: In function 'snd_emu10k1_fx8010_ioctl':
>> sound/pci/emu10k1/emufx.c:2654:80: error: 'A_DBG_STEP' undeclared (first use in this function); did you mean 'A_DBG_ZC'?
    2654 |                         snd_emu10k1_ptr_write(emu, A_DBG, 0, emu->fx8010.dbg | A_DBG_STEP | addr);
         |                                                                                ^~~~~~~~~~
         |                                                                                A_DBG_ZC
   sound/pci/emu10k1/emufx.c:2654:80: note: each undeclared identifier is reported only once for each function it appears in


vim +2654 sound/pci/emu10k1/emufx.c

  2541	
  2542	static int snd_emu10k1_fx8010_ioctl(struct snd_hwdep * hw, struct file *file, unsigned int cmd, unsigned long arg)
  2543	{
  2544		struct snd_emu10k1 *emu = hw->private_data;
  2545		struct snd_emu10k1_fx8010_info *info;
  2546		struct snd_emu10k1_fx8010_code *icode;
  2547		struct snd_emu10k1_fx8010_pcm_rec *ipcm;
  2548		unsigned int addr;
  2549		void __user *argp = (void __user *)arg;
  2550		int res;
  2551		
  2552		switch (cmd) {
  2553		case SNDRV_EMU10K1_IOCTL_PVERSION:
  2554			emu->support_tlv = 1;
  2555			return put_user(SNDRV_EMU10K1_VERSION, (int __user *)argp);
  2556		case SNDRV_EMU10K1_IOCTL_INFO:
  2557			info = kzalloc(sizeof(*info), GFP_KERNEL);
  2558			if (!info)
  2559				return -ENOMEM;
  2560			snd_emu10k1_fx8010_info(emu, info);
  2561			if (copy_to_user(argp, info, sizeof(*info))) {
  2562				kfree(info);
  2563				return -EFAULT;
  2564			}
  2565			kfree(info);
  2566			return 0;
  2567		case SNDRV_EMU10K1_IOCTL_CODE_POKE:
  2568			if (!capable(CAP_SYS_ADMIN))
  2569				return -EPERM;
  2570	
  2571			icode = memdup_user(argp, sizeof(*icode));
  2572			if (IS_ERR(icode))
  2573				return PTR_ERR(icode);
  2574			res = snd_emu10k1_icode_poke(emu, icode, false);
  2575			kfree(icode);
  2576			return res;
  2577		case SNDRV_EMU10K1_IOCTL_CODE_PEEK:
  2578			icode = memdup_user(argp, sizeof(*icode));
  2579			if (IS_ERR(icode))
  2580				return PTR_ERR(icode);
  2581			res = snd_emu10k1_icode_peek(emu, icode);
  2582			if (res == 0 && copy_to_user(argp, icode, sizeof(*icode))) {
  2583				kfree(icode);
  2584				return -EFAULT;
  2585			}
  2586			kfree(icode);
  2587			return res;
  2588		case SNDRV_EMU10K1_IOCTL_PCM_POKE:
  2589			ipcm = memdup_user(argp, sizeof(*ipcm));
  2590			if (IS_ERR(ipcm))
  2591				return PTR_ERR(ipcm);
  2592			res = snd_emu10k1_ipcm_poke(emu, ipcm);
  2593			kfree(ipcm);
  2594			return res;
  2595		case SNDRV_EMU10K1_IOCTL_PCM_PEEK:
  2596			ipcm = memdup_user(argp, sizeof(*ipcm));
  2597			if (IS_ERR(ipcm))
  2598				return PTR_ERR(ipcm);
  2599			res = snd_emu10k1_ipcm_peek(emu, ipcm);
  2600			if (res == 0 && copy_to_user(argp, ipcm, sizeof(*ipcm))) {
  2601				kfree(ipcm);
  2602				return -EFAULT;
  2603			}
  2604			kfree(ipcm);
  2605			return res;
  2606		case SNDRV_EMU10K1_IOCTL_TRAM_SETUP:
  2607			if (!capable(CAP_SYS_ADMIN))
  2608				return -EPERM;
  2609			if (get_user(addr, (unsigned int __user *)argp))
  2610				return -EFAULT;
  2611			mutex_lock(&emu->fx8010.lock);
  2612			res = snd_emu10k1_fx8010_tram_setup(emu, addr);
  2613			mutex_unlock(&emu->fx8010.lock);
  2614			return res;
  2615		case SNDRV_EMU10K1_IOCTL_STOP:
  2616			if (!capable(CAP_SYS_ADMIN))
  2617				return -EPERM;
  2618			if (emu->audigy)
  2619				snd_emu10k1_ptr_write(emu, A_DBG, 0, emu->fx8010.dbg |= A_DBG_SINGLE_STEP);
  2620			else
  2621				snd_emu10k1_ptr_write(emu, DBG, 0, emu->fx8010.dbg |= EMU10K1_DBG_SINGLE_STEP);
  2622			return 0;
  2623		case SNDRV_EMU10K1_IOCTL_CONTINUE:
  2624			if (!capable(CAP_SYS_ADMIN))
  2625				return -EPERM;
  2626			if (emu->audigy)
  2627				snd_emu10k1_ptr_write(emu, A_DBG, 0, emu->fx8010.dbg = 0);
  2628			else
  2629				snd_emu10k1_ptr_write(emu, DBG, 0, emu->fx8010.dbg = 0);
  2630			return 0;
  2631		case SNDRV_EMU10K1_IOCTL_ZERO_TRAM_COUNTER:
  2632			if (!capable(CAP_SYS_ADMIN))
  2633				return -EPERM;
  2634			if (emu->audigy)
  2635				snd_emu10k1_ptr_write(emu, A_DBG, 0, emu->fx8010.dbg | A_DBG_ZC);
  2636			else
  2637				snd_emu10k1_ptr_write(emu, DBG, 0, emu->fx8010.dbg | EMU10K1_DBG_ZC);
  2638			udelay(10);
  2639			if (emu->audigy)
  2640				snd_emu10k1_ptr_write(emu, A_DBG, 0, emu->fx8010.dbg);
  2641			else
  2642				snd_emu10k1_ptr_write(emu, DBG, 0, emu->fx8010.dbg);
  2643			return 0;
  2644		case SNDRV_EMU10K1_IOCTL_SINGLE_STEP:
  2645			if (!capable(CAP_SYS_ADMIN))
  2646				return -EPERM;
  2647			if (get_user(addr, (unsigned int __user *)argp))
  2648				return -EFAULT;
  2649			if (emu->audigy) {
  2650				if (addr > A_DBG_STEP_ADDR)
  2651					return -EINVAL;
  2652				snd_emu10k1_ptr_write(emu, A_DBG, 0, emu->fx8010.dbg |= A_DBG_SINGLE_STEP);
  2653				udelay(10);
> 2654				snd_emu10k1_ptr_write(emu, A_DBG, 0, emu->fx8010.dbg | A_DBG_STEP | addr);
  2655			} else {
  2656				if (addr > EMU10K1_DBG_SINGLE_STEP_ADDR)
  2657					return -EINVAL;
  2658				snd_emu10k1_ptr_write(emu, DBG, 0, emu->fx8010.dbg |= EMU10K1_DBG_SINGLE_STEP);
  2659				udelay(10);
  2660				snd_emu10k1_ptr_write(emu, DBG, 0, emu->fx8010.dbg | EMU10K1_DBG_STEP | addr);
  2661			}
  2662			return 0;
  2663		case SNDRV_EMU10K1_IOCTL_DBG_READ:
  2664			if (emu->audigy)
  2665				addr = snd_emu10k1_ptr_read(emu, A_DBG, 0);
  2666			else
  2667				addr = snd_emu10k1_ptr_read(emu, DBG, 0);
  2668			if (put_user(addr, (unsigned int __user *)argp))
  2669				return -EFAULT;
  2670			return 0;
  2671		}
  2672		return -ENOTTY;
  2673	}
  2674	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 1/2] ALSA: emu10k1: fix SNDRV_EMU10K1_IOCTL_SINGLE_STEP

[Takashi Iwai]

On Sat, 22 Apr 2023 18:10:20 +0200,
Oswald Buddenhagen wrote:
> 
> - Use correct address limit for Audigy
> - Use the right constant to actually make a step on Audigy
> - Don't store *_DBG_STEP and the address in emu->fx8010.dbg, as
>   otherwise unrelated operations would make steps, too
> 
> This is untested. as10k1 was never ported to Audigy anyway.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

Applied both patches now.
Although 0-day bot caught some build error, I couldn't reproduce it,
so took as is.  Let's see whether it's broken at some config combo.


thanks,

Takashi

Re: [PATCH 1/2] ALSA: emu10k1: fix SNDRV_EMU10K1_IOCTL_SINGLE_STEP

[Oswald Buddenhagen]

On Sun, Apr 23, 2023 at 09:34:55AM +0200, Takashi Iwai wrote:
>Although 0-day bot caught some build error, I couldn't reproduce it,
>so took as is.  Let's see whether it's broken at some config combo.
>
undeclared logical dependency on the "fixup DSP defines" patch, i think.

but even without that mistake the bot would croak, because i made my 
tooling describe deps in a human-readable way rather than sha1 + 
patch-ids. i guess i'll have to include both to make it bot-friendly ...

regards