diff mbox series

[1/4] echoaudio: Race conditions around "opencount"

Message ID 20200701122723.17814-1-mark@xwax.org (mailing list archive)
State New, archived
Headers show
Series [1/4] echoaudio: Race conditions around "opencount" | expand

Commit Message

Mark Hills July 1, 2020, 12:27 p.m. UTC 
Use of atomics does not make these statements robust:

       atomic_inc(&chip->opencount);
       if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
               chip->can_set_rate=0;

and

       if (atomic_read(&chip->opencount)) {
               if (chip->opencount) {
                       changed = -EAGAIN;
               } else {
                       changed = set_digital_mode(chip, dmode);

It would be necessary to atomically increment or decrement the value
and use the returned result. And yet we still need to prevent other
threads making use of "can_set_rate" while we set it.

However in all but one case the atomic is misleading as they are already
running with "mode_mutex" held.

Decisions are made on mode setting are often intrinsically connected
to "opencount" because some operations are not permitted unless
there is sole ownership.

So instead simplify this, and use "mode_mutex" as a lock for all reference
counting and mode setting.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 sound/pci/echoaudio/echoaudio.c | 76 ++++++++++++++++++---------------
 sound/pci/echoaudio/echoaudio.h |  6 +--
 2 files changed, 45 insertions(+), 37 deletions(-)

Comments

kernel test robot July 1, 2020, 4:37 p.m. UTC | #1 
Hi Mark,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on sound/for-next]
[also build test WARNING on v5.8-rc3 next-20200701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mark-Hills/echoaudio-Race-conditions-around-opencount/20200701-203413
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from sound/pci/echoaudio/mona.c:123:
   sound/pci/echoaudio/mona_dsp.c: In function 'set_input_clock':
   sound/pci/echoaudio/mona_dsp.c:305:18: error: passing argument 1 of 'atomic_read' from incompatible pointer type [-Werror=incompatible-pointer-types]
     305 |  if (atomic_read(&chip->opencount))
         |                  ^~~~~~~~~~~~~~~~
         |                  |
         |                  unsigned int *
   In file included from include/linux/atomic.h:7,
                    from include/linux/cpumask.h:13,
                    from include/linux/interrupt.h:8,
                    from sound/pci/echoaudio/mona.c:37:
   arch/mips/include/asm/atomic.h:28:55: note: expected 'const atomic_t *' {aka 'const struct <anonymous> *'} but argument is of type 'unsigned int *'
      28 | static __always_inline type pfx##_read(const pfx##_t *v)  \
         |                                        ~~~~~~~~~~~~~~~^
>> arch/mips/include/asm/atomic.h:49:1: note: in expansion of macro 'ATOMIC_OPS'
      49 | ATOMIC_OPS(atomic, int)
         | ^~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/ATOMIC_OPS +49 arch/mips/include/asm/atomic.h

^1da177e4c3f41 include/asm-mips/atomic.h      Linus Torvalds 2005-04-16  47  
1da7bce8591d58 arch/mips/include/asm/atomic.h Paul Burton    2019-10-01  48  #define ATOMIC_INIT(i)		{ (i) }
1da7bce8591d58 arch/mips/include/asm/atomic.h Paul Burton    2019-10-01 @49  ATOMIC_OPS(atomic, int)
^1da177e4c3f41 include/asm-mips/atomic.h      Linus Torvalds 2005-04-16  50  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot July 1, 2020, 5:32 p.m. UTC | #2 
Hi Mark,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on sound/for-next]
[also build test ERROR on v5.8-rc3 next-20200701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mark-Hills/echoaudio-Race-conditions-around-opencount/20200701-203413
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from include/linux/delay.h:22,
                    from sound/pci/echoaudio/mona.c:35:
   sound/pci/echoaudio/mona_dsp.c: In function 'set_input_clock':
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:2: note: in expansion of macro 'compiletime_assert'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:21: note: in expansion of macro '__native_word'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |                     ^~~~~~~~~~~~~
   include/linux/compiler.h:291:2: note: in expansion of macro 'compiletime_assert_rwonce_type'
     291 |  compiletime_assert_rwonce_type(x);    \
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
>> sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:2: note: in expansion of macro 'compiletime_assert'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:21: note: in expansion of macro '__native_word'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |                     ^~~~~~~~~~~~~
   include/linux/compiler.h:291:2: note: in expansion of macro 'compiletime_assert_rwonce_type'
     291 |  compiletime_assert_rwonce_type(x);    \
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
>> sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:2: note: in expansion of macro 'compiletime_assert'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:21: note: in expansion of macro '__native_word'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |                     ^~~~~~~~~~~~~
   include/linux/compiler.h:291:2: note: in expansion of macro 'compiletime_assert_rwonce_type'
     291 |  compiletime_assert_rwonce_type(x);    \
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
>> sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:2: note: in expansion of macro 'compiletime_assert'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:21: note: in expansion of macro '__native_word'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |                     ^~~~~~~~~~~~~
   include/linux/compiler.h:291:2: note: in expansion of macro 'compiletime_assert_rwonce_type'
     291 |  compiletime_assert_rwonce_type(x);    \
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
>> sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:2: note: in expansion of macro 'compiletime_assert'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:291:2: note: in expansion of macro 'compiletime_assert_rwonce_type'
     291 |  compiletime_assert_rwonce_type(x);    \
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
>> sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
   In file included from <command-line>:
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler_types.h:290:13: note: in definition of macro '__unqual_scalar_typeof'
     290 |   _Generic((x),      \
         |             ^
   include/linux/compiler.h:292:2: note: in expansion of macro '__READ_ONCE_SCALAR'
     292 |  __READ_ONCE_SCALAR(x);      \
         |  ^~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
>> sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler_types.h:290:13: note: in definition of macro '__unqual_scalar_typeof'
     290 |   _Generic((x),      \
         |             ^
   include/linux/compiler.h:284:34: note: in expansion of macro '__READ_ONCE'
     284 |  __unqual_scalar_typeof(x) __x = __READ_ONCE(x);   \
         |                                  ^~~~~~~~~~~
   include/linux/compiler.h:292:2: note: in expansion of macro '__READ_ONCE_SCALAR'
     292 |  __READ_ONCE_SCALAR(x);      \
         |  ^~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
   sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
   In file included from include/linux/kernel.h:11,
                    from include/linux/delay.h:22,
                    from sound/pci/echoaudio/mona.c:35:
   arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:280:72: note: in definition of macro '__READ_ONCE'
     280 | #define __READ_ONCE(x) (*(const volatile __unqual_scalar_typeof(x) *)&(x))
         |                                                                        ^
   include/linux/compiler.h:292:2: note: in expansion of macro '__READ_ONCE_SCALAR'
     292 |  __READ_ONCE_SCALAR(x);      \
         |  ^~~~~~~~~~~~~~~~~~
   arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
   sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
   arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:286:10: note: in definition of macro '__READ_ONCE_SCALAR'
     286 |  (typeof(x))__x;       \
         |          ^
   arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
   sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~

vim +/atomic_read +305 sound/pci/echoaudio/mona_dsp.c

dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  295  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  296  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  297  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  298  static int set_input_clock(struct echoaudio *chip, u16 clock)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  299  {
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  300  	u32 control_reg, clocks_from_dsp;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  301  	int err;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  302  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  303  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  304  	/* Prevent two simultaneous calls to switch_asic() */
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28 @305  	if (atomic_read(&chip->opencount))
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  306  		return -EAGAIN;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  307  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  308  	/* Mask off the clock select bits */
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  309  	control_reg = le32_to_cpu(chip->comm_page->control_register) &
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  310  		GML_CLOCK_CLEAR_MASK;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  311  	clocks_from_dsp = le32_to_cpu(chip->comm_page->status_clocks);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  312  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  313  	switch (clock) {
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  314  	case ECHO_CLOCK_INTERNAL:
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  315  		chip->input_clock = ECHO_CLOCK_INTERNAL;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  316  		return set_sample_rate(chip, chip->sample_rate);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  317  	case ECHO_CLOCK_SPDIF:
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  318  		if (chip->digital_mode == DIGITAL_MODE_ADAT)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  319  			return -EAGAIN;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  320  		spin_unlock_irq(&chip->lock);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  321  		err = switch_asic(chip, clocks_from_dsp &
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  322  				  GML_CLOCK_DETECT_BIT_SPDIF96);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  323  		spin_lock_irq(&chip->lock);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  324  		if (err < 0)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  325  			return err;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  326  		control_reg |= GML_SPDIF_CLOCK;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  327  		if (clocks_from_dsp & GML_CLOCK_DETECT_BIT_SPDIF96)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  328  			control_reg |= GML_DOUBLE_SPEED_MODE;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  329  		else
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  330  			control_reg &= ~GML_DOUBLE_SPEED_MODE;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  331  		break;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  332  	case ECHO_CLOCK_WORD:
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  333  		spin_unlock_irq(&chip->lock);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  334  		err = switch_asic(chip, clocks_from_dsp &
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  335  				  GML_CLOCK_DETECT_BIT_WORD96);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  336  		spin_lock_irq(&chip->lock);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  337  		if (err < 0)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  338  			return err;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  339  		control_reg |= GML_WORD_CLOCK;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  340  		if (clocks_from_dsp & GML_CLOCK_DETECT_BIT_WORD96)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  341  			control_reg |= GML_DOUBLE_SPEED_MODE;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  342  		else
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  343  			control_reg &= ~GML_DOUBLE_SPEED_MODE;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  344  		break;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  345  	case ECHO_CLOCK_ADAT:
b5b4a41b392960 Sudip Mukherjee  2014-11-03  346  		dev_dbg(chip->card->dev, "Set Mona clock to ADAT\n");
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  347  		if (chip->digital_mode != DIGITAL_MODE_ADAT)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  348  			return -EAGAIN;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  349  		control_reg |= GML_ADAT_CLOCK;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  350  		control_reg &= ~GML_DOUBLE_SPEED_MODE;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  351  		break;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  352  	default:
b5b4a41b392960 Sudip Mukherjee  2014-11-03  353  		dev_err(chip->card->dev,
b5b4a41b392960 Sudip Mukherjee  2014-11-03  354  			"Input clock 0x%x not supported for Mona\n", clock);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  355  		return -EINVAL;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  356  	}
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  357  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  358  	chip->input_clock = clock;
3f6175ece94735 Mark Brown       2015-08-10  359  	return write_control_reg(chip, control_reg, true);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  360  }
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  361  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Mark Hills July 2, 2020, 9:53 a.m. UTC | #3 
Takashi, my apologies, it looks like this patch broke the build of the 
Mona driver.

Thankfully the change is simple, as it just looks like a bit of confusion 
of responsibilies in the code for the Mona interface; the correct fix is 
to remove the code.

That is a lesson for working with only the echo3g driver enabled. Now I 
have done a full build of all echoaudio drivers, with no warnings or 
errors.

Here's a patch, or it can be squashed into the original patch if 
necessary.
Takashi Iwai July 7, 2020, 8:28 a.m. UTC | #4 
On Thu, 02 Jul 2020 11:53:51 +0200,
Mark Hills wrote:
> 
> Takashi, my apologies, it looks like this patch broke the build of the 
> Mona driver.
> 
> Thankfully the change is simple, as it just looks like a bit of confusion 
> of responsibilies in the code for the Mona interface; the correct fix is 
> to remove the code.
> 
> That is a lesson for working with only the echo3g driver enabled. Now I 
> have done a full build of all echoaudio drivers, with no warnings or 
> errors.
> 
> Here's a patch, or it can be squashed into the original patch if 
> necessary.

Could you rather fold the fix into the patch and resubmit the whole
patch set?  I'm just back from travel, and it'd be better anyway if I
receive a fresh patch set to apply.


Thanks!

Takashi
Mark Hills July 8, 2020, 10:16 a.m. UTC | #5 
On Tue, 7 Jul 2020, Takashi Iwai wrote:

> On Thu, 02 Jul 2020 11:53:51 +0200,
> Mark Hills wrote:
> > 
> > Takashi, my apologies, it looks like this patch broke the build of the 
> > Mona driver.
> > 
> > Thankfully the change is simple, as it just looks like a bit of confusion 
> > of responsibilies in the code for the Mona interface; the correct fix is 
> > to remove the code.
> > 
> > That is a lesson for working with only the echo3g driver enabled. Now I 
> > have done a full build of all echoaudio drivers, with no warnings or 
> > errors.
> > 
> > Here's a patch, or it can be squashed into the original patch if 
> > necessary.
> 
> Could you rather fold the fix into the patch and resubmit the whole
> patch set?  I'm just back from travel, and it'd be better anyway if I
> receive a fresh patch set to apply.

No problem, I'll follow up.

In the end, I decided it best to keep the patch separate, but re-order so 
as to not break the build.
diff mbox series

Patch

diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 0941a7a17623..82a49dfd2384 100644
--- a/sound/pci/echoaudio/echoaudio.c
+++ b/sound/pci/echoaudio/echoaudio.c
@@ -245,13 +245,20 @@  static int hw_rule_sample_rate(struct snd_pcm_hw_params *params,
 						      SNDRV_PCM_HW_PARAM_RATE);
 	struct echoaudio *chip = rule->private;
 	struct snd_interval fixed;
+	int err;
+
+	mutex_lock(&chip->mode_mutex);
 
-	if (!chip->can_set_rate) {
+	if (chip->can_set_rate) {
+		err = 0;
+	} else {
 		snd_interval_any(&fixed);
 		fixed.min = fixed.max = chip->sample_rate;
-		return snd_interval_refine(rate, &fixed);
+		err = snd_interval_refine(rate, &fixed);
 	}
-	return 0;
+
+	mutex_unlock(&chip->mode_mutex);
+	return err;
 }
 
 
@@ -322,7 +329,7 @@  static int pcm_open(struct snd_pcm_substream *substream,
 				       SNDRV_PCM_HW_PARAM_RATE, -1)) < 0)
 		return err;
 
-	/* Finally allocate a page for the scatter-gather list */
+	/* Allocate a page for the scatter-gather list */
 	if ((err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,
 				       &chip->pci->dev,
 				       PAGE_SIZE, &pipe->sgpage)) < 0) {
@@ -330,6 +337,17 @@  static int pcm_open(struct snd_pcm_substream *substream,
 		return err;
 	}
 
+	/*
+	 * Sole ownership required to set the rate
+	 */
+
+	dev_dbg(chip->card->dev, "pcm_open opencount=%d can_set_rate=%d, rate_set=%d",
+		chip->opencount, chip->can_set_rate, chip->rate_set);
+
+	chip->opencount++;
+	if (chip->opencount > 1 && chip->rate_set)
+		chip->can_set_rate = 0;
+
 	return 0;
 }
 
@@ -353,12 +371,7 @@  static int pcm_analog_in_open(struct snd_pcm_substream *substream)
 				       hw_rule_capture_format_by_channels, NULL,
 				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
 		return err;
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
-	dev_dbg(chip->card->dev, "pcm_analog_in_open  cs=%d  oc=%d  r=%d\n",
-		chip->can_set_rate, atomic_read(&chip->opencount),
-		chip->sample_rate);
+
 	return 0;
 }
 
@@ -388,12 +401,7 @@  static int pcm_analog_out_open(struct snd_pcm_substream *substream)
 				       NULL,
 				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
 		return err;
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
-	dev_dbg(chip->card->dev, "pcm_analog_out_open  cs=%d  oc=%d  r=%d\n",
-		chip->can_set_rate, atomic_read(&chip->opencount),
-		chip->sample_rate);
+
 	return 0;
 }
 
@@ -429,10 +437,6 @@  static int pcm_digital_in_open(struct snd_pcm_substream *substream)
 				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
 		goto din_exit;
 
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
-
 din_exit:
 	mutex_unlock(&chip->mode_mutex);
 	return err;
@@ -471,9 +475,7 @@  static int pcm_digital_out_open(struct snd_pcm_substream *substream)
 				       NULL, SNDRV_PCM_HW_PARAM_CHANNELS,
 				       -1)) < 0)
 		goto dout_exit;
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
+
 dout_exit:
 	mutex_unlock(&chip->mode_mutex);
 	return err;
@@ -488,23 +490,29 @@  static int pcm_digital_out_open(struct snd_pcm_substream *substream)
 static int pcm_close(struct snd_pcm_substream *substream)
 {
 	struct echoaudio *chip = snd_pcm_substream_chip(substream);
-	int oc;
 
 	/* Nothing to do here. Audio is already off and pipe will be
 	 * freed by its callback
 	 */
 
-	atomic_dec(&chip->opencount);
-	oc = atomic_read(&chip->opencount);
-	dev_dbg(chip->card->dev, "pcm_close  oc=%d  cs=%d  rs=%d\n", oc,
-		chip->can_set_rate, chip->rate_set);
-	if (oc < 2)
+	mutex_lock(&chip->mode_mutex);
+
+	dev_dbg(chip->card->dev, "pcm_open opencount=%d can_set_rate=%d, rate_set=%d",
+		chip->opencount, chip->can_set_rate, chip->rate_set);
+
+	chip->opencount--;
+
+	switch (chip->opencount) {
+	case 1:
 		chip->can_set_rate = 1;
-	if (oc == 0)
+		break;
+
+	case 0:
 		chip->rate_set = 0;
-	dev_dbg(chip->card->dev, "pcm_close2 oc=%d  cs=%d  rs=%d\n", oc,
-		chip->can_set_rate, chip->rate_set);
+		break;
+	}
 
+	mutex_unlock(&chip->mode_mutex);
 	return 0;
 }
 
@@ -1409,7 +1417,7 @@  static int snd_echo_digital_mode_put(struct snd_kcontrol *kcontrol,
 		/* Do not allow the user to change the digital mode when a pcm
 		device is open because it also changes the number of channels
 		and the allowed sample rates */
-		if (atomic_read(&chip->opencount)) {
+		if (chip->opencount) {
 			changed = -EAGAIN;
 		} else {
 			changed = set_digital_mode(chip, dmode);
@@ -1874,7 +1882,7 @@  static int snd_echo_create(struct snd_card *card,
 		chip->card = card;
 		chip->pci = pci;
 		chip->irq = -1;
-		atomic_set(&chip->opencount, 0);
+		chip->opencount = 0;
 		mutex_init(&chip->mode_mutex);
 		chip->can_set_rate = 1;
 	} else {
diff --git a/sound/pci/echoaudio/echoaudio.h b/sound/pci/echoaudio/echoaudio.h
index be4d0489394a..6fd283e4e676 100644
--- a/sound/pci/echoaudio/echoaudio.h
+++ b/sound/pci/echoaudio/echoaudio.h
@@ -336,7 +336,7 @@  struct echoaudio {
 	struct mutex mode_mutex;
 	u16 num_digital_modes, digital_mode_list[6];
 	u16 num_clock_sources, clock_source_list[10];
-	atomic_t opencount;
+	unsigned int opencount;  /* protected by mode_mutex */
 	struct snd_kcontrol *clock_src_ctl;
 	struct snd_pcm *analog_pcm, *digital_pcm;
 	struct snd_card *card;
@@ -353,8 +353,8 @@  struct echoaudio {
 	struct timer_list timer;
 	char tinuse;				/* Timer in use */
 	char midi_full;				/* MIDI output buffer is full */
-	char can_set_rate;
-	char rate_set;
+	char can_set_rate;                      /* protected by mode_mutex */
+	char rate_set;                          /* protected by mode_mutex */
 
 	/* This stuff is used mainly by the lowlevel code */
 	struct comm_page *comm_page;	/* Virtual address of the memory