[PATCH 0/9] ALSA: compress offfload fixes

[PATCH 0/9] ALSA: compress offfload fixes

[Vinod Koul]

Hi Takashi,

Here is the patch series which fixes various issues being reported by users (out
of tree sadly)

The first three and and last one are marked to stable as would like these to be
fixed in older kernels as well. It would be good if you can send them as fixes
to linus for 3.11.

Rest can go in the merge window

Fixes:
 - using lock for all operation was a very bad idead. This is bad as some of the
   ioctls like drain, partial drain can be time consuming and thus prevent any
other operation while these are ongoing like Pause, Stop or timestamp query, so
fix this be removing bunch of ioctls not to use device lock.
 - Now we dont have lock for pointer updates so this maybe racy, so use lock
   for doing lowest level calculation. 
 - As disscused on our sample rate problem, lets move to use rate values and I
   will fix the lib too. Since the driver are not upstream the impact of this
change wont be huge.
 - Plus few fix like use snprintf, state chacks for pause, write etc..


Vinod Koul (9):
  ALSA: Compress - dont use lock for all ioctls
  ALSA: compress: use mutex in drain
  ASoC: compress: dont aquire lock for draining states
  ALSA: compress: use snprint instread of sprintf
  ALSA: compres: wakeup the poll thread on pause
  ALSA: compress: dont write when stream is paused
  ALSA: compress: allow write when stream is setup
  ALSA: compress: call pointer callback and updates under a lock
  ALSA: compress: use rate values for passing sampling rates

 include/sound/compress_driver.h       |    2 +
 include/uapi/sound/compress_offload.h |    2 +-
 sound/core/compress_offload.c         |  140 +++++++++++++++++++++++++-------
 sound/soc/soc-compress.c              |   10 +++
 4 files changed, 122 insertions(+), 32 deletions(-)

Thanks
~Vinod

[PATCH 1/9] ALSA: Compress - dont use lock for all ioctls

[Vinod Koul]

Some simple ioctls like timsetamp query, capabities query can be done anytime
and should not be under the stream lock. Move these to
snd_compress_simple_iotcls() which is invoked without lock held

While at it, improve readblity a bit by sprinkling some empty lines

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Cc: <stable@vger.kernel.org>
---
 sound/core/compress_offload.c |   79 ++++++++++++++++++++++++++++++-----------
 1 files changed, 58 insertions(+), 21 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 99db892..868aefd 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -729,70 +729,107 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
 	return retval;
 }
 
-static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
+static int snd_compress_simple_ioctls(struct file *file,
+				struct snd_compr_stream *stream,
+				unsigned int cmd, unsigned long arg)
 {
-	struct snd_compr_file *data = f->private_data;
-	struct snd_compr_stream *stream;
 	int retval = -ENOTTY;
 
-	if (snd_BUG_ON(!data))
-		return -EFAULT;
-	stream = &data->stream;
-	if (snd_BUG_ON(!stream))
-		return -EFAULT;
-	mutex_lock(&stream->device->lock);
 	switch (_IOC_NR(cmd)) {
 	case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION):
 		put_user(SNDRV_COMPRESS_VERSION,
 				(int __user *)arg) ? -EFAULT : 0;
 		break;
+
 	case _IOC_NR(SNDRV_COMPRESS_GET_CAPS):
 		retval = snd_compr_get_caps(stream, arg);
 		break;
+
 	case _IOC_NR(SNDRV_COMPRESS_GET_CODEC_CAPS):
 		retval = snd_compr_get_codec_caps(stream, arg);
 		break;
+
+	case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
+		retval = snd_compr_tstamp(stream, arg);
+		break;
+
+	case _IOC_NR(SNDRV_COMPRESS_AVAIL):
+		retval = snd_compr_ioctl_avail(stream, arg);
+		break;
+
+		/* drain and partial drain need special handling
+	 * we need to drop the locks here as the streams would get blocked on the
+	 * dsp to get drained. The locking would be handled in respective
+	 * function here
+	 */
+	case _IOC_NR(SNDRV_COMPRESS_DRAIN):
+		retval = snd_compr_drain(stream);
+		break;
+
+	case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
+		retval = snd_compr_partial_drain(stream);
+		break;
+	}
+
+	return retval;
+}
+
+static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
+{
+	struct snd_compr_file *data = f->private_data;
+	struct snd_compr_stream *stream;
+	int retval = -ENOTTY;
+
+	if (snd_BUG_ON(!data))
+		return -EFAULT;
+	stream = &data->stream;
+	if (snd_BUG_ON(!stream))
+		return -EFAULT;
+
+	mutex_lock(&stream->device->lock);
+	switch (_IOC_NR(cmd)) {
 	case _IOC_NR(SNDRV_COMPRESS_SET_PARAMS):
 		retval = snd_compr_set_params(stream, arg);
 		break;
+
 	case _IOC_NR(SNDRV_COMPRESS_GET_PARAMS):
 		retval = snd_compr_get_params(stream, arg);
 		break;
+
 	case _IOC_NR(SNDRV_COMPRESS_SET_METADATA):
 		retval = snd_compr_set_metadata(stream, arg);
 		break;
+
 	case _IOC_NR(SNDRV_COMPRESS_GET_METADATA):
 		retval = snd_compr_get_metadata(stream, arg);
 		break;
-	case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
-		retval = snd_compr_tstamp(stream, arg);
-		break;
-	case _IOC_NR(SNDRV_COMPRESS_AVAIL):
-		retval = snd_compr_ioctl_avail(stream, arg);
-		break;
+
 	case _IOC_NR(SNDRV_COMPRESS_PAUSE):
 		retval = snd_compr_pause(stream);
 		break;
+
 	case _IOC_NR(SNDRV_COMPRESS_RESUME):
 		retval = snd_compr_resume(stream);
 		break;
+
 	case _IOC_NR(SNDRV_COMPRESS_START):
 		retval = snd_compr_start(stream);
 		break;
+
 	case _IOC_NR(SNDRV_COMPRESS_STOP):
 		retval = snd_compr_stop(stream);
 		break;
-	case _IOC_NR(SNDRV_COMPRESS_DRAIN):
-		retval = snd_compr_drain(stream);
-		break;
-	case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
-		retval = snd_compr_partial_drain(stream);
-		break;
+
 	case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK):
 		retval = snd_compr_next_track(stream);
 		break;
 
+	default:
+		mutex_unlock(&stream->device->lock);
+		return snd_compress_simple_ioctls(f, stream, cmd, arg);
+
 	}
+
 	mutex_unlock(&stream->device->lock);
 	return retval;
 }
-- 
1.7.0.4

[PATCH 2/9] ALSA: compress: use mutex in drain

[Vinod Koul]

Since we dont have lock over the function, we need to aquire mutex when checking
and modfying states in drain and partial_drain handlers

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Cc: <stable@vger.kernel.org>
---
 sound/core/compress_offload.c |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 868aefd..e640f8c 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 	return retval;
 }
 
+/* this fn is called without lock being held and we change stream states here
+ * so while using the stream state auquire the lock but relase before invoking
+ * DSP as the call will possibly take a while
+ */
 static int snd_compr_drain(struct snd_compr_stream *stream)
 {
 	int retval;
 
+	mutex_lock(&stream->device->lock);
 	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
-			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
-		return -EPERM;
+			stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
+		retval = -EPERM;
+		goto ret;
+	}
+	mutex_unlock(&stream->device->lock);
 	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
+	mutex_lock(&stream->device->lock);
 	if (!retval) {
 		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
 		wake_up(&stream->runtime->sleep);
 	}
+ret:
+	mutex_unlock(&stream->device->lock);
 	return retval;
 }
 
@@ -716,9 +727,14 @@ static int snd_compr_next_track(struct snd_compr_stream *stream)
 static int snd_compr_partial_drain(struct snd_compr_stream *stream)
 {
 	int retval;
+
+	mutex_lock(&stream->device->lock);
 	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
-			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
+			stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
+		mutex_unlock(&stream->device->lock);
 		return -EPERM;
+	}
+	mutex_unlock(&stream->device->lock);
 	/* stream can be drained only when next track has been signalled */
 	if (stream->next_track == false)
 		return -EPERM;
-- 
1.7.0.4

[PATCH 3/9] ASoC: compress: dont aquire lock for draining states

[Vinod Koul]

Both draining and partial draning states will take a while getting executed. The
lock aquired will block the other operations like pause, stop etc which are
perfectly valid cmds during these states. So dont use mutex while invoking DSP
for these ops

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Cc: <stable@vger.kernel.org>
---
 sound/soc/soc-compress.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 06a8000..800ee89 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -171,6 +171,16 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd)
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	int ret = 0;
 
+	/* for partial drain and drain cmd dont aquire lock while invoking DSP.
+	 * These calls will be blocked till these operation can complete while
+	 * will be a while. And during that app can invoke STOP, PAUSE etc
+	 */
+	if (cmd == SND_COMPR_TRIGGER_PARTIAL_DRAIN || cmd == SND_COMPR_TRIGGER_DRAIN) {
+		if (platform->driver->compr_ops &&
+					platform->driver->compr_ops->trigger)
+			return platform->driver->compr_ops->trigger(cstream, cmd);
+	}
+
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
 	if (platform->driver->compr_ops && platform->driver->compr_ops->trigger) {
-- 
1.7.0.4

[PATCH 4/9] ALSA: compress: use snprint instread of sprintf

[Vinod Koul]

To prevent possiblity of exceeding buffer size

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/core/compress_offload.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index e640f8c..1e722c2 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -871,7 +871,7 @@ static int snd_compress_dev_register(struct snd_device *device)
 		return -EBADFD;
 	compr = device->device_data;
 
-	sprintf(str, "comprC%iD%i", compr->card->number, compr->device);
+	snprintf(str, sizeof(str), "comprC%iD%i", compr->card->number, compr->device);
 	pr_debug("reg %s for device %s, direction %d\n", str, compr->name,
 			compr->direction);
 	/* register compressed device */
-- 
1.7.0.4

[PATCH 5/9] ALSA: compres: wakeup the poll thread on pause

[Vinod Koul]

this allows the poll to be unblocked, if it is blocked and yeilds the control
of userthread blocked

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/core/compress_offload.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 1e722c2..417a456 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -629,9 +629,12 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
 
 	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
 		return -EPERM;
+
 	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
-	if (!retval)
+	if (!retval) {
 		stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+		wake_up(&stream->runtime->sleep);
+	}
 	return retval;
 }
 
-- 
1.7.0.4

[PATCH 6/9] ALSA: compress: dont write when stream is paused

[Vinod Koul]

and return bytes written as 0 and not error

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/core/compress_offload.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 417a456..fb570c8 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -256,7 +256,7 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
 	struct snd_compr_file *data = f->private_data;
 	struct snd_compr_stream *stream;
 	size_t avail;
-	int retval;
+	int retval = 0;
 
 	if (snd_BUG_ON(!data))
 		return -EFAULT;
@@ -264,6 +264,14 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
 	stream = &data->stream;
 	mutex_lock(&stream->device->lock);
 	/* write is allowed when stream is running or has been steup */
+	 /*
+	  * if the stream is in paused state, return the
+	  * number of bytes consumed as 0
+	  */
+	if (stream->runtime->state == SNDRV_PCM_STATE_PAUSED) {
+		mutex_unlock(&stream->device->lock);
+		return retval;
+	}
 	if (stream->runtime->state != SNDRV_PCM_STATE_SETUP &&
 			stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
 		mutex_unlock(&stream->device->lock);
-- 
1.7.0.4

[PATCH 7/9] ALSA: compress: allow write when stream is setup

[Vinod Koul]

We should allow write and thus poll as well when stream has been prepared. This
allows usermode to write buffers ahead of time

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/core/compress_offload.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index fb570c8..898a1d9 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -263,7 +263,6 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
 
 	stream = &data->stream;
 	mutex_lock(&stream->device->lock);
-	/* write is allowed when stream is running or has been steup */
 	 /*
 	  * if the stream is in paused state, return the
 	  * number of bytes consumed as 0
@@ -272,8 +271,11 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
 		mutex_unlock(&stream->device->lock);
 		return retval;
 	}
+
+	/* write is allowed when stream is running or prepared or in setup */
 	if (stream->runtime->state != SNDRV_PCM_STATE_SETUP &&
-			stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
+			stream->runtime->state != SNDRV_PCM_STATE_RUNNING &&
+			stream->runtime->state != SNDRV_PCM_STATE_PREPARED) {
 		mutex_unlock(&stream->device->lock);
 		return -EBADFD;
 	}
@@ -380,8 +382,7 @@ static unsigned int snd_compr_poll(struct file *f, poll_table *wait)
 		return -EFAULT;
 
 	mutex_lock(&stream->device->lock);
-	if (stream->runtime->state == SNDRV_PCM_STATE_PAUSED ||
-			stream->runtime->state == SNDRV_PCM_STATE_OPEN) {
+	if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) {
 		retval = -EBADFD;
 		goto out;
 	}
-- 
1.7.0.4

[PATCH 8/9] ALSA: compress: call pointer callback and updates under a lock

[Vinod Koul]

to prevent racy claculations as multiple threads can invoke and update values

While at it give some emptylines in code to improve readablity

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 include/sound/compress_driver.h |    2 ++
 sound/core/compress_offload.c   |   13 +++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 9031a26..30258f3 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -48,6 +48,7 @@ struct snd_compr_ops;
  *	the ring buffer
  * @total_bytes_transferred: cumulative bytes transferred by offload DSP
  * @sleep: poll sleep
+ * @pointer_lock: lock used for buffer pointer update
  */
 struct snd_compr_runtime {
 	snd_pcm_state_t state;
@@ -60,6 +61,7 @@ struct snd_compr_runtime {
 	u64 total_bytes_transferred;
 	wait_queue_head_t sleep;
 	void *private_data;
+	struct mutex pointer_lock;
 };
 
 /**
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 898a1d9..01b179f 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -122,6 +122,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
 		return -ENOMEM;
 	}
 	runtime->state = SNDRV_PCM_STATE_OPEN;
+	mutex_init(&runtime->pointer_lock);
 	init_waitqueue_head(&runtime->sleep);
 	data->stream.runtime = runtime;
 	f->private_data = (void *)data;
@@ -151,13 +152,21 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream,
 {
 	if (!stream->ops->pointer)
 		return -ENOTSUPP;
+
+	mutex_lock(&stream->runtime->pointer_lock);
+
 	stream->ops->pointer(stream, tstamp);
+
 	pr_debug("dsp consumed till %d total %d bytes\n",
 		tstamp->byte_offset, tstamp->copied_total);
+
 	if (stream->direction == SND_COMPRESS_PLAYBACK)
 		stream->runtime->total_bytes_transferred = tstamp->copied_total;
 	else
 		stream->runtime->total_bytes_available = tstamp->copied_total;
+
+	mutex_unlock(&stream->runtime->pointer_lock);
+
 	return 0;
 }
 
@@ -165,7 +174,9 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
 		struct snd_compr_avail *avail)
 {
 	memset(avail, 0, sizeof(*avail));
+
 	snd_compr_update_tstamp(stream, &avail->tstamp);
+
 	/* Still need to return avail even if tstamp can't be filled in */
 
 	if (stream->runtime->total_bytes_available == 0 &&
@@ -174,9 +185,11 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
 		pr_debug("detected init and someone forgot to do a write\n");
 		return stream->runtime->buffer_size;
 	}
+
 	pr_debug("app wrote %lld, DSP consumed %lld\n",
 			stream->runtime->total_bytes_available,
 			stream->runtime->total_bytes_transferred);
+
 	if (stream->runtime->total_bytes_available ==
 				stream->runtime->total_bytes_transferred) {
 		if (stream->direction == SND_COMPRESS_PLAYBACK) {
-- 
1.7.0.4

[PATCH 9/9] ALSA: compress: use rate values for passing sampling rates

[Vinod Koul]

Clarify that its better to use the rate values like 8000, 12000, 11025, 44100,
48000 etc to send sampling rate values to driver. The changes kernel ABI but no
drivers exist upstream so okay to change now
Tinycompress to be fixed as well

For this change bump the API version as well

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Cc: <stable@vger.kernel.org>
---
 include/uapi/sound/compress_offload.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index d630163..cad87ff 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -62,7 +62,7 @@ struct snd_compr_params {
  *	progress. It shall not be used for timing estimates.
  * @pcm_io_frames: Frames rendered or received by DSP into a mixer or an audio
  * output/input. This field should be used for A/V sync or time estimates.
- * @sampling_rate: sampling rate of audio
+ * @sampling_rate: sampling rate value of audio,  eg 12000, 48000
  */
 struct snd_compr_tstamp {
 	__u32 byte_offset;
-- 
1.7.0.4

Re: [PATCH 1/9] ALSA: Compress - dont use lock for all ioctls

[Vinod Koul]

On Tue, Aug 27, 2013 at 12:22:31PM +0200, Takashi Iwai wrote:
> At Tue, 27 Aug 2013 12:10:31 +0530,
> Vinod Koul wrote:
> > 
> > Some simple ioctls like timsetamp query, capabities query can be done anytime
> > and should not be under the stream lock. Move these to
> > snd_compress_simple_iotcls() which is invoked without lock held
> > 
> > While at it, improve readblity a bit by sprinkling some empty lines
> > 
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > Cc: <stable@vger.kernel.org>
> 
> Why it's needed for stable?  Fixing any real bugs?
yup, users are complaining that while streams are draining they can't read
timstamps. Also one case where a user hit pause didnt go thru as lock preveted
the pause to be executed..

Since 3.10 is next LTS kernel, I forsee lots of folks using taht for a while so
makes sense to fix there as well
 
> > +	default:
> > +		mutex_unlock(&stream->device->lock);
> > +		return snd_compress_simple_ioctls(f, stream, cmd, arg);
> 
> In this code, it still locks/unlocks shortly unnecessarily.
> It should be rather like:
> 	switch (_IOC_NR(cmd)) {
>  	case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION):
> 		...
>  	case _IOC_NR(SNDRV_COMPRESS_GET_CAPS):
> 		....	
> 	default:
> 		retval = snd_compress_locked_ioctls(f, stream, cmd, arg);
> 	}
Hmmm, okay no point in blocking. I will reverse the flow

~Vinod

-- 

Re: [PATCH 2/9] ALSA: compress: use mutex in drain

[Vinod Koul]

On Tue, Aug 27, 2013 at 12:25:23PM +0200, Takashi Iwai wrote:
> At Tue, 27 Aug 2013 12:10:32 +0530,
> Vinod Koul wrote:
> > 
> > Since we dont have lock over the function, we need to aquire mutex when checking
> > and modfying states in drain and partial_drain handlers
> > 
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > Cc: <stable@vger.kernel.org>
> > ---
> >  sound/core/compress_offload.c |   22 +++++++++++++++++++---
> >  1 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > index 868aefd..e640f8c 100644
> > --- a/sound/core/compress_offload.c
> > +++ b/sound/core/compress_offload.c
> > @@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
> >  	return retval;
> >  }
> >  
> > +/* this fn is called without lock being held and we change stream states here
> > + * so while using the stream state auquire the lock but relase before invoking
> > + * DSP as the call will possibly take a while
> > + */
> >  static int snd_compr_drain(struct snd_compr_stream *stream)
> >  {
> >  	int retval;
> >  
> > +	mutex_lock(&stream->device->lock);
> >  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> > -			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> > -		return -EPERM;
> > +			stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
> > +		retval = -EPERM;
> > +		goto ret;
> > +	}
> > +	mutex_unlock(&stream->device->lock);
> >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> 
> Why release the lock here?  The trigger callback is called within this
> mutex lock in other places.
This is the main part :)

Since the drain states will take a while (order of few seconds) to execute so we
will be blocked. Thats why we cant have lock here. The point of lock is to sync
the stream states here. We are not modfying anything. During drain and partial
drain we need to allow other trigger ops like pause, stop tog o thru so drop the
lock here for these two ops only!

~Vinod

Re: [PATCH 8/9] ALSA: compress: call pointer callback and updates under a lock

[Vinod Koul]

On Tue, Aug 27, 2013 at 12:31:33PM +0200, Takashi Iwai wrote:
> At Tue, 27 Aug 2013 12:10:38 +0530,
> Vinod Koul wrote:
> > 
> > to prevent racy claculations as multiple threads can invoke and update values
> 
> Why a new lock?  Can't it be the same lock?
Pointer query and updates IMO should be independent of the trigger ops. I dont
see a reason why we should blcok pointers while we are pausing...

~Vinod

Re: [PATCH 0/9] ALSA: compress offfload fixes

[Vinod Koul]

On Tue, Aug 27, 2013 at 12:46:36PM +0200, Takashi Iwai wrote:
> At Tue, 27 Aug 2013 12:10:30 +0530,
> Vinod Koul wrote:
> > 
> > Hi Takashi,
> > 
> > Here is the patch series which fixes various issues being reported by users (out
> > of tree sadly)
> > 
> > The first three and and last one are marked to stable as would like these to be
> > fixed in older kernels as well. It would be good if you can send them as fixes
> > to linus for 3.11.
> 
> Sorry, no go.  If it's only about out-of-tree drivers, it's even
> questionable whether it worth for stable kernel, because we have no
> real test case for our own.
Since lot of embedded folks will be on 3.10 then would have been nicer if it
went to stable. I know users will backport, but if a kernel provided the desired
functionalty then would have been great. 

> > Fixes:
> >  - using lock for all operation was a very bad idead. This is bad as some of the
> >    ioctls like drain, partial drain can be time consuming and thus prevent any
> > other operation while these are ongoing like Pause, Stop or timestamp query, so
> > fix this be removing bunch of ioctls not to use device lock.
> 
> Although not all of them need locks, it's easier to manage in most
> cases to perform in the lock.  For drain or such operation, you can
> simply unlock and re-lock in the call itself, as your patch in the
> series does.a
> 
> >  - Now we dont have lock for pointer updates so this maybe racy, so use lock
> >    for doing lowest level calculation. 
> 
> Similarly, introducing yet another lock would just choke you neck.
Well i thought that pointer updates are orthogonal to other triggers so there is
no issue if we seprate the two. How do you think in long run will this impact..?

> >  - As disscused on our sample rate problem, lets move to use rate values and I
> >    will fix the lib too. Since the driver are not upstream the impact of this
> > change wont be huge.
> 
> I see no code touching sampling_rate field.
Yes its passed directly to the drivers, where tehy use values to prgram
decoders. Only meaning of the field is changing now.

> >  - Plus few fix like use snprintf, state chacks for pause, write etc..
> 
> I don't like to merge irrelevant space changes into a patch if it's a
> fix patch, especially if it's targeted to stable.  The fix is the fix.
> Please separate.
Sure makes sense...

So do you want these to be sent to stable or not. I would prefer to be sent

Thanks
~Vinod

Re: [PATCH 2/9] ALSA: compress: use mutex in drain

[Vinod Koul]

On Tue, Aug 27, 2013 at 12:53:54PM +0200, Takashi Iwai wrote:
> At Tue, 27 Aug 2013 15:17:41 +0530,
> Vinod Koul wrote:
> > On Tue, Aug 27, 2013 at 12:25:23PM +0200, Takashi Iwai wrote:
> > > > --- a/sound/core/compress_offload.c
> > > > +++ b/sound/core/compress_offload.c
> > > > @@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
> > > >  	return retval;
> > > >  }
> > > >  
> > > > +/* this fn is called without lock being held and we change stream states here
> > > > + * so while using the stream state auquire the lock but relase before invoking
> > > > + * DSP as the call will possibly take a while
> > > > + */
> > > >  static int snd_compr_drain(struct snd_compr_stream *stream)
> > > >  {
> > > >  	int retval;
> > > >  
> > > > +	mutex_lock(&stream->device->lock);
> > > >  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> > > > -			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> > > > -		return -EPERM;
> > > > +			stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
> > > > +		retval = -EPERM;
> > > > +		goto ret;
> > > > +	}
> > > > +	mutex_unlock(&stream->device->lock);
> > > >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> > > 
> > > Why release the lock here?  The trigger callback is called within this
> > > mutex lock in other places.
> > This is the main part :)
> > 
> > Since the drain states will take a while (order of few seconds) to execute so we
> > will be blocked. Thats why we cant have lock here.
> 
> What's about other places calling the trigger ops within lock?
> You can't mix things.
Well i was going to treat drain only as exception to this. The issue here is
during the long drain other triggers are perfectly valid cases
> 
> > The point of lock is to sync
> > the stream states here.
> 
> Without the lock, it's racy.  What if another thread calls the same
> function at the same time?
that part can be checked by seeing if we are already draining.
> 
> > We are not modfying anything. During drain and partial
> > drain we need to allow other trigger ops like pause, stop tog o thru so drop the
> > lock here for these two ops only!
> 
> Well, the biggest problem is that there is no proper design for which
> ops take a lock and which not.  The trigger callback is basically to
> trigger the action.  There should be no long-time blocking there.
> (Otherwise you'll definitely loose a gunfight :)
The reason for blocked implementation is to treat return of the call as
notifcation that draining is completed.

For example user has written all the buffers, lets says worth 3 secs and now has
triggered drain. User needs to wait till drain is complete before closing the
device etc. So he waits on drain to compelete..

Do you have a better way to manage this?

~Vinod

Re: [PATCH 1/9] ALSA: Compress - dont use lock for all ioctls

[Takashi Iwai]

At Tue, 27 Aug 2013 12:10:31 +0530,
Vinod Koul wrote:
> 
> Some simple ioctls like timsetamp query, capabities query can be done anytime
> and should not be under the stream lock. Move these to
> snd_compress_simple_iotcls() which is invoked without lock held
> 
> While at it, improve readblity a bit by sprinkling some empty lines
> 
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> Cc: <stable@vger.kernel.org>

Why it's needed for stable?  Fixing any real bugs?


> ---
>  sound/core/compress_offload.c |   79 ++++++++++++++++++++++++++++++-----------
>  1 files changed, 58 insertions(+), 21 deletions(-)
> 
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 99db892..868aefd 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -729,70 +729,107 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>  	return retval;
>  }
>  
> -static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> +static int snd_compress_simple_ioctls(struct file *file,
> +				struct snd_compr_stream *stream,
> +				unsigned int cmd, unsigned long arg)
>  {
> -	struct snd_compr_file *data = f->private_data;
> -	struct snd_compr_stream *stream;
>  	int retval = -ENOTTY;
>  
> -	if (snd_BUG_ON(!data))
> -		return -EFAULT;
> -	stream = &data->stream;
> -	if (snd_BUG_ON(!stream))
> -		return -EFAULT;
> -	mutex_lock(&stream->device->lock);
>  	switch (_IOC_NR(cmd)) {
>  	case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION):
>  		put_user(SNDRV_COMPRESS_VERSION,
>  				(int __user *)arg) ? -EFAULT : 0;
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_GET_CAPS):
>  		retval = snd_compr_get_caps(stream, arg);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_GET_CODEC_CAPS):
>  		retval = snd_compr_get_codec_caps(stream, arg);
>  		break;
> +
> +	case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
> +		retval = snd_compr_tstamp(stream, arg);
> +		break;
> +
> +	case _IOC_NR(SNDRV_COMPRESS_AVAIL):
> +		retval = snd_compr_ioctl_avail(stream, arg);
> +		break;
> +
> +		/* drain and partial drain need special handling
> +	 * we need to drop the locks here as the streams would get blocked on the
> +	 * dsp to get drained. The locking would be handled in respective
> +	 * function here
> +	 */
> +	case _IOC_NR(SNDRV_COMPRESS_DRAIN):
> +		retval = snd_compr_drain(stream);
> +		break;
> +
> +	case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
> +		retval = snd_compr_partial_drain(stream);
> +		break;
> +	}
> +
> +	return retval;
> +}
> +
> +static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> +{
> +	struct snd_compr_file *data = f->private_data;
> +	struct snd_compr_stream *stream;
> +	int retval = -ENOTTY;
> +
> +	if (snd_BUG_ON(!data))
> +		return -EFAULT;
> +	stream = &data->stream;
> +	if (snd_BUG_ON(!stream))
> +		return -EFAULT;
> +
> +	mutex_lock(&stream->device->lock);
> +	switch (_IOC_NR(cmd)) {
>  	case _IOC_NR(SNDRV_COMPRESS_SET_PARAMS):
>  		retval = snd_compr_set_params(stream, arg);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_GET_PARAMS):
>  		retval = snd_compr_get_params(stream, arg);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_SET_METADATA):
>  		retval = snd_compr_set_metadata(stream, arg);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_GET_METADATA):
>  		retval = snd_compr_get_metadata(stream, arg);
>  		break;
> -	case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
> -		retval = snd_compr_tstamp(stream, arg);
> -		break;
> -	case _IOC_NR(SNDRV_COMPRESS_AVAIL):
> -		retval = snd_compr_ioctl_avail(stream, arg);
> -		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_PAUSE):
>  		retval = snd_compr_pause(stream);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_RESUME):
>  		retval = snd_compr_resume(stream);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_START):
>  		retval = snd_compr_start(stream);
>  		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_STOP):
>  		retval = snd_compr_stop(stream);
>  		break;
> -	case _IOC_NR(SNDRV_COMPRESS_DRAIN):
> -		retval = snd_compr_drain(stream);
> -		break;
> -	case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
> -		retval = snd_compr_partial_drain(stream);
> -		break;
> +
>  	case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK):
>  		retval = snd_compr_next_track(stream);
>  		break;
>  
> +	default:
> +		mutex_unlock(&stream->device->lock);
> +		return snd_compress_simple_ioctls(f, stream, cmd, arg);

In this code, it still locks/unlocks shortly unnecessarily.
It should be rather like:
	switch (_IOC_NR(cmd)) {
 	case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION):
		...
 	case _IOC_NR(SNDRV_COMPRESS_GET_CAPS):
		....	
	default:
		retval = snd_compress_locked_ioctls(f, stream, cmd, arg);
	}


Takashi

Re: [PATCH 2/9] ALSA: compress: use mutex in drain

[Takashi Iwai]

At Tue, 27 Aug 2013 12:10:32 +0530,
Vinod Koul wrote:
> 
> Since we dont have lock over the function, we need to aquire mutex when checking
> and modfying states in drain and partial_drain handlers
> 
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> Cc: <stable@vger.kernel.org>
> ---
>  sound/core/compress_offload.c |   22 +++++++++++++++++++---
>  1 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 868aefd..e640f8c 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
>  	return retval;
>  }
>  
> +/* this fn is called without lock being held and we change stream states here
> + * so while using the stream state auquire the lock but relase before invoking
> + * DSP as the call will possibly take a while
> + */
>  static int snd_compr_drain(struct snd_compr_stream *stream)
>  {
>  	int retval;
>  
> +	mutex_lock(&stream->device->lock);
>  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> -			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> -		return -EPERM;
> +			stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
> +		retval = -EPERM;
> +		goto ret;
> +	}
> +	mutex_unlock(&stream->device->lock);
>  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);

Why release the lock here?  The trigger callback is called within this
mutex lock in other places.


Takashi

> +	mutex_lock(&stream->device->lock);
>  	if (!retval) {
>  		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
>  		wake_up(&stream->runtime->sleep);
>  	}
> +ret:
> +	mutex_unlock(&stream->device->lock);
>  	return retval;
>  }
>  
> @@ -716,9 +727,14 @@ static int snd_compr_next_track(struct snd_compr_stream *stream)
>  static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>  {
>  	int retval;
> +
> +	mutex_lock(&stream->device->lock);
>  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> -			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> +			stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
> +		mutex_unlock(&stream->device->lock);
>  		return -EPERM;
> +	}
> +	mutex_unlock(&stream->device->lock);
>  	/* stream can be drained only when next track has been signalled */
>  	if (stream->next_track == false)
>  		return -EPERM;
> -- 
> 1.7.0.4
> 

Re: [PATCH 9/9] ALSA: compress: use rate values for passing sampling rates

[Takashi Iwai]

At Tue, 27 Aug 2013 12:10:39 +0530,
Vinod Koul wrote:
> 
> Clarify that its better to use the rate values like 8000, 12000, 11025, 44100,
> 48000 etc to send sampling rate values to driver. The changes kernel ABI but no
> drivers exist upstream so okay to change now
> Tinycompress to be fixed as well
> 
> For this change bump the API version as well

I see no corresponding changes at all...

> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> Cc: <stable@vger.kernel.org>

Again, why it must be to stable?


Takashi

> ---
>  include/uapi/sound/compress_offload.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
> index d630163..cad87ff 100644
> --- a/include/uapi/sound/compress_offload.h
> +++ b/include/uapi/sound/compress_offload.h
> @@ -62,7 +62,7 @@ struct snd_compr_params {
>   *	progress. It shall not be used for timing estimates.
>   * @pcm_io_frames: Frames rendered or received by DSP into a mixer or an audio
>   * output/input. This field should be used for A/V sync or time estimates.
> - * @sampling_rate: sampling rate of audio
> + * @sampling_rate: sampling rate value of audio,  eg 12000, 48000
>   */
>  struct snd_compr_tstamp {
>  	__u32 byte_offset;
> -- 
> 1.7.0.4
> 

Re: [PATCH 8/9] ALSA: compress: call pointer callback and updates under a lock

[Takashi Iwai]

At Tue, 27 Aug 2013 12:10:38 +0530,
Vinod Koul wrote:
> 
> to prevent racy claculations as multiple threads can invoke and update values

Why a new lock?  Can't it be the same lock?


Takashi

> While at it give some emptylines in code to improve readablity
> 
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  include/sound/compress_driver.h |    2 ++
>  sound/core/compress_offload.c   |   13 +++++++++++++
>  2 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index 9031a26..30258f3 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -48,6 +48,7 @@ struct snd_compr_ops;
>   *	the ring buffer
>   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
>   * @sleep: poll sleep
> + * @pointer_lock: lock used for buffer pointer update
>   */
>  struct snd_compr_runtime {
>  	snd_pcm_state_t state;
> @@ -60,6 +61,7 @@ struct snd_compr_runtime {
>  	u64 total_bytes_transferred;
>  	wait_queue_head_t sleep;
>  	void *private_data;
> +	struct mutex pointer_lock;
>  };
>  
>  /**
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 898a1d9..01b179f 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -122,6 +122,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
>  		return -ENOMEM;
>  	}
>  	runtime->state = SNDRV_PCM_STATE_OPEN;
> +	mutex_init(&runtime->pointer_lock);
>  	init_waitqueue_head(&runtime->sleep);
>  	data->stream.runtime = runtime;
>  	f->private_data = (void *)data;
> @@ -151,13 +152,21 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream,
>  {
>  	if (!stream->ops->pointer)
>  		return -ENOTSUPP;
> +
> +	mutex_lock(&stream->runtime->pointer_lock);
> +
>  	stream->ops->pointer(stream, tstamp);
> +
>  	pr_debug("dsp consumed till %d total %d bytes\n",
>  		tstamp->byte_offset, tstamp->copied_total);
> +
>  	if (stream->direction == SND_COMPRESS_PLAYBACK)
>  		stream->runtime->total_bytes_transferred = tstamp->copied_total;
>  	else
>  		stream->runtime->total_bytes_available = tstamp->copied_total;
> +
> +	mutex_unlock(&stream->runtime->pointer_lock);
> +
>  	return 0;
>  }
>  
> @@ -165,7 +174,9 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
>  		struct snd_compr_avail *avail)
>  {
>  	memset(avail, 0, sizeof(*avail));
> +
>  	snd_compr_update_tstamp(stream, &avail->tstamp);
> +
>  	/* Still need to return avail even if tstamp can't be filled in */
>  
>  	if (stream->runtime->total_bytes_available == 0 &&
> @@ -174,9 +185,11 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
>  		pr_debug("detected init and someone forgot to do a write\n");
>  		return stream->runtime->buffer_size;
>  	}
> +
>  	pr_debug("app wrote %lld, DSP consumed %lld\n",
>  			stream->runtime->total_bytes_available,
>  			stream->runtime->total_bytes_transferred);
> +
>  	if (stream->runtime->total_bytes_available ==
>  				stream->runtime->total_bytes_transferred) {
>  		if (stream->direction == SND_COMPRESS_PLAYBACK) {
> -- 
> 1.7.0.4
> 

Re: [PATCH 0/9] ALSA: compress offfload fixes

[Takashi Iwai]

At Tue, 27 Aug 2013 12:10:30 +0530,
Vinod Koul wrote:
> 
> Hi Takashi,
> 
> Here is the patch series which fixes various issues being reported by users (out
> of tree sadly)
> 
> The first three and and last one are marked to stable as would like these to be
> fixed in older kernels as well. It would be good if you can send them as fixes
> to linus for 3.11.

Sorry, no go.  If it's only about out-of-tree drivers, it's even
questionable whether it worth for stable kernel, because we have no
real test case for our own.

> Rest can go in the merge window
> 
> Fixes:
>  - using lock for all operation was a very bad idead. This is bad as some of the
>    ioctls like drain, partial drain can be time consuming and thus prevent any
> other operation while these are ongoing like Pause, Stop or timestamp query, so
> fix this be removing bunch of ioctls not to use device lock.

Although not all of them need locks, it's easier to manage in most
cases to perform in the lock.  For drain or such operation, you can
simply unlock and re-lock in the call itself, as your patch in the
series does.

>  - Now we dont have lock for pointer updates so this maybe racy, so use lock
>    for doing lowest level calculation. 

Similarly, introducing yet another lock would just choke you neck.

>  - As disscused on our sample rate problem, lets move to use rate values and I
>    will fix the lib too. Since the driver are not upstream the impact of this
> change wont be huge.

I see no code touching sampling_rate field.

>  - Plus few fix like use snprintf, state chacks for pause, write etc..

I don't like to merge irrelevant space changes into a patch if it's a
fix patch, especially if it's targeted to stable.  The fix is the fix.
Please separate.


thanks,

Takashi

> 
> Vinod Koul (9):
>   ALSA: Compress - dont use lock for all ioctls
>   ALSA: compress: use mutex in drain
>   ASoC: compress: dont aquire lock for draining states
>   ALSA: compress: use snprint instread of sprintf
>   ALSA: compres: wakeup the poll thread on pause
>   ALSA: compress: dont write when stream is paused
>   ALSA: compress: allow write when stream is setup
>   ALSA: compress: call pointer callback and updates under a lock
>   ALSA: compress: use rate values for passing sampling rates
> 
>  include/sound/compress_driver.h       |    2 +
>  include/uapi/sound/compress_offload.h |    2 +-
>  sound/core/compress_offload.c         |  140 +++++++++++++++++++++++++-------
>  sound/soc/soc-compress.c              |   10 +++
>  4 files changed, 122 insertions(+), 32 deletions(-)
> 
> Thanks
> ~Vinod
> 

Re: [PATCH 2/9] ALSA: compress: use mutex in drain

[Takashi Iwai]

At Tue, 27 Aug 2013 15:17:41 +0530,
Vinod Koul wrote:
> 
> On Tue, Aug 27, 2013 at 12:25:23PM +0200, Takashi Iwai wrote:
> > At Tue, 27 Aug 2013 12:10:32 +0530,
> > Vinod Koul wrote:
> > > 
> > > Since we dont have lock over the function, we need to aquire mutex when checking
> > > and modfying states in drain and partial_drain handlers
> > > 
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > Cc: <stable@vger.kernel.org>
> > > ---
> > >  sound/core/compress_offload.c |   22 +++++++++++++++++++---
> > >  1 files changed, 19 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > > index 868aefd..e640f8c 100644
> > > --- a/sound/core/compress_offload.c
> > > +++ b/sound/core/compress_offload.c
> > > @@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
> > >  	return retval;
> > >  }
> > >  
> > > +/* this fn is called without lock being held and we change stream states here
> > > + * so while using the stream state auquire the lock but relase before invoking
> > > + * DSP as the call will possibly take a while
> > > + */
> > >  static int snd_compr_drain(struct snd_compr_stream *stream)
> > >  {
> > >  	int retval;
> > >  
> > > +	mutex_lock(&stream->device->lock);
> > >  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> > > -			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> > > -		return -EPERM;
> > > +			stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
> > > +		retval = -EPERM;
> > > +		goto ret;
> > > +	}
> > > +	mutex_unlock(&stream->device->lock);
> > >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> > 
> > Why release the lock here?  The trigger callback is called within this
> > mutex lock in other places.
> This is the main part :)
> 
> Since the drain states will take a while (order of few seconds) to execute so we
> will be blocked. Thats why we cant have lock here.

What's about other places calling the trigger ops within lock?
You can't mix things.

> The point of lock is to sync
> the stream states here.

Without the lock, it's racy.  What if another thread calls the same
function at the same time?

> We are not modfying anything. During drain and partial
> drain we need to allow other trigger ops like pause, stop tog o thru so drop the
> lock here for these two ops only!

Well, the biggest problem is that there is no proper design for which
ops take a lock and which not.  The trigger callback is basically to
trigger the action.  There should be no long-time blocking there.
(Otherwise you'll definitely loose a gunfight :)


Takashi

Re: [PATCH 1/9] ALSA: Compress - dont use lock for all ioctls

[Takashi Iwai]

At Tue, 27 Aug 2013 15:14:15 +0530,
Vinod Koul wrote:
> 
> On Tue, Aug 27, 2013 at 12:22:31PM +0200, Takashi Iwai wrote:
> > At Tue, 27 Aug 2013 12:10:31 +0530,
> > Vinod Koul wrote:
> > > 
> > > Some simple ioctls like timsetamp query, capabities query can be done anytime
> > > and should not be under the stream lock. Move these to
> > > snd_compress_simple_iotcls() which is invoked without lock held
> > > 
> > > While at it, improve readblity a bit by sprinkling some empty lines
> > > 
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > Cc: <stable@vger.kernel.org>
> > 
> > Why it's needed for stable?  Fixing any real bugs?
> yup, users are complaining that while streams are draining they can't read
> timstamps. Also one case where a user hit pause didnt go thru as lock preveted
> the pause to be executed..

Then write the problems more clearly.  I saw no such information in
the changelog.

> Since 3.10 is next LTS kernel, I forsee lots of folks using taht for a while so
> makes sense to fix there as well

Depends on the fix and the problem.  The fact that this can't be
tested only with 3.10 kernel (no real driver exists), I doubt it's
worth.  Stable kernels aren't for out-of-tree drivers.

And, if you really target to the stable tree, don't put any
unnecessary changes like white space fixes.  Read
stable_kernel_rules.txt once more.


thanks,

Takashi

> > > +	default:
> > > +		mutex_unlock(&stream->device->lock);
> > > +		return snd_compress_simple_ioctls(f, stream, cmd, arg);
> > 
> > In this code, it still locks/unlocks shortly unnecessarily.
> > It should be rather like:
> > 	switch (_IOC_NR(cmd)) {
> >  	case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION):
> > 		...
> >  	case _IOC_NR(SNDRV_COMPRESS_GET_CAPS):
> > 		....	
> > 	default:
> > 		retval = snd_compress_locked_ioctls(f, stream, cmd, arg);
> > 	}
> Hmmm, okay no point in blocking. I will reverse the flow
> 
> ~Vinod
> 
> -- 
> 

Re: [PATCH 8/9] ALSA: compress: call pointer callback and updates under a lock

[Takashi Iwai]

At Tue, 27 Aug 2013 15:18:59 +0530,
Vinod Koul wrote:
> 
> On Tue, Aug 27, 2013 at 12:31:33PM +0200, Takashi Iwai wrote:
> > At Tue, 27 Aug 2013 12:10:38 +0530,
> > Vinod Koul wrote:
> > > 
> > > to prevent racy claculations as multiple threads can invoke and update values
> > 
> > Why a new lock?  Can't it be the same lock?
> Pointer query and updates IMO should be independent of the trigger ops. I dont
> see a reason why we should blcok pointers while we are pausing...

What happens if the trigger or reset is called during your taking the
timestamp?


Takashi

Re: [PATCH 2/9] ALSA: compress: use mutex in drain

[Takashi Iwai]

At Tue, 27 Aug 2013 15:46:14 +0530,
Vinod Koul wrote:
> 
> On Tue, Aug 27, 2013 at 12:53:54PM +0200, Takashi Iwai wrote:
> > At Tue, 27 Aug 2013 15:17:41 +0530,
> > Vinod Koul wrote:
> > > On Tue, Aug 27, 2013 at 12:25:23PM +0200, Takashi Iwai wrote:
> > > > > --- a/sound/core/compress_offload.c
> > > > > +++ b/sound/core/compress_offload.c
> > > > > @@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
> > > > >  	return retval;
> > > > >  }
> > > > >  
> > > > > +/* this fn is called without lock being held and we change stream states here
> > > > > + * so while using the stream state auquire the lock but relase before invoking
> > > > > + * DSP as the call will possibly take a while
> > > > > + */
> > > > >  static int snd_compr_drain(struct snd_compr_stream *stream)
> > > > >  {
> > > > >  	int retval;
> > > > >  
> > > > > +	mutex_lock(&stream->device->lock);
> > > > >  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> > > > > -			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> > > > > -		return -EPERM;
> > > > > +			stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
> > > > > +		retval = -EPERM;
> > > > > +		goto ret;
> > > > > +	}
> > > > > +	mutex_unlock(&stream->device->lock);
> > > > >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> > > > 
> > > > Why release the lock here?  The trigger callback is called within this
> > > > mutex lock in other places.
> > > This is the main part :)
> > > 
> > > Since the drain states will take a while (order of few seconds) to execute so we
> > > will be blocked. Thats why we cant have lock here.
> > 
> > What's about other places calling the trigger ops within lock?
> > You can't mix things.
> Well i was going to treat drain only as exception to this. The issue here is
> during the long drain other triggers are perfectly valid cases

An ops callback must be defined either to be locked or unlocked.
Calling in the unlocked context only for some case doesn't sound
right.

> > > The point of lock is to sync
> > > the stream states here.
> > 
> > Without the lock, it's racy.  What if another thread calls the same
> > function at the same time?
> that part can be checked by seeing if we are already draining.

But how?  The place you're calling trigger is unlocked.
Suppose another thread calling trigger_stop just between
mutex_unlock() and stream->ops->trigger(DRAIN) call in the above.
The state check doesn't work there.

> > > We are not modfying anything. During drain and partial
> > > drain we need to allow other trigger ops like pause, stop tog o thru so drop the
> > > lock here for these two ops only!
> > 
> > Well, the biggest problem is that there is no proper design for which
> > ops take a lock and which not.  The trigger callback is basically to
> > trigger the action.  There should be no long-time blocking there.
> > (Otherwise you'll definitely loose a gunfight :)
> The reason for blocked implementation is to treat return of the call as
> notifcation that draining is completed.
> 
> For example user has written all the buffers, lets says worth 3 secs and now has
> triggered drain. User needs to wait till drain is complete before closing the
> device etc. So he waits on drain to compelete..
> 
> Do you have a better way to manage this?

Split the drain action in two parts, trigger and synchronization:

	lock();
	...
	trigger(pause);
	while (!pause_finished) {
		unlock();
		schedule_or_sleep_or_whatever();
		lock();
	}
	...
	unlock();


Takashi

Re: [PATCH 0/9] ALSA: compress offfload fixes

[Takashi Iwai]

At Tue, 27 Aug 2013 15:39:55 +0530,
Vinod Koul wrote:
> 
> On Tue, Aug 27, 2013 at 12:46:36PM +0200, Takashi Iwai wrote:
> > At Tue, 27 Aug 2013 12:10:30 +0530,
> > Vinod Koul wrote:
> > > 
> > > Hi Takashi,
> > > 
> > > Here is the patch series which fixes various issues being reported by users (out
> > > of tree sadly)
> > > 
> > > The first three and and last one are marked to stable as would like these to be
> > > fixed in older kernels as well. It would be good if you can send them as fixes
> > > to linus for 3.11.
> > 
> > Sorry, no go.  If it's only about out-of-tree drivers, it's even
> > questionable whether it worth for stable kernel, because we have no
> > real test case for our own.
> Since lot of embedded folks will be on 3.10 then would have been nicer if it
> went to stable.

"It'd be nice" doesn't satisfy the condition for stable kernels,
unfortunately.

> I know users will backport, but if a kernel provided the desired
> functionalty then would have been great. 
> 
> > > Fixes:
> > >  - using lock for all operation was a very bad idead. This is bad as some of the
> > >    ioctls like drain, partial drain can be time consuming and thus prevent any
> > > other operation while these are ongoing like Pause, Stop or timestamp query, so
> > > fix this be removing bunch of ioctls not to use device lock.
> > 
> > Although not all of them need locks, it's easier to manage in most
> > cases to perform in the lock.  For drain or such operation, you can
> > simply unlock and re-lock in the call itself, as your patch in the
> > series does.a
> > 
> > >  - Now we dont have lock for pointer updates so this maybe racy, so use lock
> > >    for doing lowest level calculation. 
> > 
> > Similarly, introducing yet another lock would just choke you neck.
> Well i thought that pointer updates are orthogonal to other triggers so there is
> no issue if we seprate the two. How do you think in long run will this impact..?

If both locks can be never called in nested way, then it'd work.
But if the locks can be nested, better to avoid as much as possible.
Put in that way: what if the time for critical section via
stream->device->lock is short enough (it should be)?

> > >  - As disscused on our sample rate problem, lets move to use rate values and I
> > >    will fix the lib too. Since the driver are not upstream the impact of this
> > > change wont be huge.
> > 
> > I see no code touching sampling_rate field.
> Yes its passed directly to the drivers, where tehy use values to program
> decoders. Only meaning of the field is changing now.

So you're proposing a patch just changing the comment in the header
file as a stable fix patch?  Please reread stable_kernel_rules.txt
once again.

> > >  - Plus few fix like use snprintf, state chacks for pause, write etc..
> > 
> > I don't like to merge irrelevant space changes into a patch if it's a
> > fix patch, especially if it's targeted to stable.  The fix is the fix.
> > Please separate.
> Sure makes sense...
> 
> So do you want these to be sent to stable or not. I would prefer to be sent

This pretty depends on how it can be "fixed"...


Takashi

Re: [PATCH 2/9] ALSA: compress: use mutex in drain

[Vinod Koul]

On Tue, Aug 27, 2013 at 02:23:19PM +0200, Takashi Iwai wrote:
> > > > > > +/* this fn is called without lock being held and we change stream states here
> > > > > > + * so while using the stream state auquire the lock but relase before invoking
> > > > > > + * DSP as the call will possibly take a while
> > > > > > + */
> > > > > >  static int snd_compr_drain(struct snd_compr_stream *stream)
> > > > > >  {
> > > > > >  	int retval;
> > > > > >  
> > > > > > +	mutex_lock(&stream->device->lock);
> > > > > >  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> > > > > > -			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> > > > > > -		return -EPERM;
> > > > > > +			stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
> > > > > > +		retval = -EPERM;
> > > > > > +		goto ret;
> > > > > > +	}
> > > > > > +	mutex_unlock(&stream->device->lock);
> > > > > >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> > > > > 
> > > > > Why release the lock here?  The trigger callback is called within this
> > > > > mutex lock in other places.
> > > > This is the main part :)
> > > > 
> > > > Since the drain states will take a while (order of few seconds) to execute so we
> > > > will be blocked. Thats why we cant have lock here.
> > > 
> > > What's about other places calling the trigger ops within lock?
> > > You can't mix things.
> > Well i was going to treat drain only as exception to this. The issue here is
> > during the long drain other triggers are perfectly valid cases
> 
> An ops callback must be defined either to be locked or unlocked.
> Calling in the unlocked context only for some case doesn't sound
> right.
well all the callbacks except drain is called with lock held from framework...

> > > > The point of lock is to sync
> > > > the stream states here.
> > > 
> > > Without the lock, it's racy.  What if another thread calls the same
> > > function at the same time?
> > that part can be checked by seeing if we are already draining.
> 
> But how?  The place you're calling trigger is unlocked.
> Suppose another thread calling trigger_stop just between
> mutex_unlock() and stream->ops->trigger(DRAIN) call in the above.
> The state check doesn't work there.
the framework does this with lock held and then calls the driver
something like this will ensure this while making sync right...
	mutex_lock(&lock);
	if (state == DRAINING) {
		mutex_unlock(&lock);
		return -EPERM;
	} else
		state = DRAINING;
	mutex_unlock(&lock);

	ops->drain(substream);

	mutex_lock(&lock);
	state = DRAINED;
	mutex_unlock(&lock);
	
> 
> > > > We are not modfying anything. During drain and partial
> > > > drain we need to allow other trigger ops like pause, stop tog o thru so drop the
> > > > lock here for these two ops only!
> > > 
> > > Well, the biggest problem is that there is no proper design for which
> > > ops take a lock and which not.  The trigger callback is basically to
> > > trigger the action.  There should be no long-time blocking there.
> > > (Otherwise you'll definitely loose a gunfight :)
> > The reason for blocked implementation is to treat return of the call as
> > notifcation that draining is completed.
> > 
> > For example user has written all the buffers, lets says worth 3 secs and now has
> > triggered drain. User needs to wait till drain is complete before closing the
> > device etc. So he waits on drain to compelete..
> > 
> > Do you have a better way to manage this?
> 
> Split the drain action in two parts, trigger and synchronization:
> 
> 	lock();
> 	...
> 	trigger(pause);
> 	while (!pause_finished) {
> 		unlock();
> 		schedule_or_sleep_or_whatever();
> 		lock();
> 	}
> 	...
> 	unlock();
okay, i guess above is on same lines...

~Vinod
-- 

Re: [PATCH 0/9] ALSA: compress offfload fixes

[Vinod Koul]

On Tue, Aug 27, 2013 at 02:32:28PM +0200, Takashi Iwai wrote:
> > > >  - As disscused on our sample rate problem, lets move to use rate values and I
> > > >    will fix the lib too. Since the driver are not upstream the impact of this
> > > > change wont be huge.
> > > 
> > > I see no code touching sampling_rate field.
> > Yes its passed directly to the drivers, where tehy use values to program
> > decoders. Only meaning of the field is changing now.
> 
> So you're proposing a patch just changing the comment in the header
> file as a stable fix patch?  Please reread stable_kernel_rules.txt
> once again.
Yes along with header version so that tinycompress can cope with it.

The meaning of the value is changing here... 

~Vinod
-- 

Re: [PATCH 0/9] ALSA: compress offfload fixes

[Vinod Koul]

[-- Attachment #1.1: Type: text/plain, Size: 937 bytes --]

On Tue, Aug 27, 2013 at 02:28:08PM +0100, Mark Brown wrote:
> On Tue, Aug 27, 2013 at 02:32:28PM +0200, Takashi Iwai wrote:
> > Vinod Koul wrote:
> 
> > > Since lot of embedded folks will be on 3.10 then would have been nicer if it
> > > went to stable.
> 
> > "It'd be nice" doesn't satisfy the condition for stable kernels,
> > unfortunately.
> 
> On the other hand if there's an issue which essentially every user is
> most likely going to end up backporting a fix for it seems reasonable to
> consider getting that resolved.  Personally I'd be a lot more
> conservative than Vinod with this one and only consider the fix for
> blocking during drain.  The current series seems rather invasive though.
Not all are marked for stable. Only thecouple to fix locking problem and
defination of sampling_rate. Rest will go thru usual cycle, though I suspect
with folks in embedded will take complete series.

~Vinod
-- 

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 9/9] ALSA: compress: use rate values for passing sampling rates

[Vinod Koul]

[-- Attachment #1.1: Type: text/plain, Size: 1120 bytes --]

On Tue, Aug 27, 2013 at 02:29:59PM +0100, Mark Brown wrote:
> On Tue, Aug 27, 2013 at 12:10:39PM +0530, Vinod Koul wrote:
> > Clarify that its better to use the rate values like 8000, 12000, 11025, 44100,
> > 48000 etc to send sampling rate values to driver. The changes kernel ABI but no
> > drivers exist upstream so okay to change now
> > Tinycompress to be fixed as well
> 
> > - * @sampling_rate: sampling rate of audio
> > + * @sampling_rate: sampling rate value of audio,  eg 12000, 48000
> 
> It's not clear why this is a change in the ABI - the values written and
> the types don't change.  Personally I'd not change the ABI - it's a bit
> late now that the code has been in the kernel for several releases.
the problem is the meaning of sampling_rate. It was SND_PCM_RATE_XXX. It was a
lack of foresight on my side, we should have done values which we are changing
now to handle rates not defined here.

But since none of drivers are upstream, impact is IMO managble. I was planning
to communicate this along with tinycompress fixes to all folks who i know are
using this :)

~Vinod
-- 

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/9] ALSA: compress offfload fixes

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 616 bytes --]

On Tue, Aug 27, 2013 at 02:32:28PM +0200, Takashi Iwai wrote:
> Vinod Koul wrote:

> > Since lot of embedded folks will be on 3.10 then would have been nicer if it
> > went to stable.

> "It'd be nice" doesn't satisfy the condition for stable kernels,
> unfortunately.

On the other hand if there's an issue which essentially every user is
most likely going to end up backporting a fix for it seems reasonable to
consider getting that resolved.  Personally I'd be a lot more
conservative than Vinod with this one and only consider the fix for
blocking during drain.  The current series seems rather invasive though.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 9/9] ALSA: compress: use rate values for passing sampling rates

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 633 bytes --]

On Tue, Aug 27, 2013 at 12:10:39PM +0530, Vinod Koul wrote:
> Clarify that its better to use the rate values like 8000, 12000, 11025, 44100,
> 48000 etc to send sampling rate values to driver. The changes kernel ABI but no
> drivers exist upstream so okay to change now
> Tinycompress to be fixed as well

> - * @sampling_rate: sampling rate of audio
> + * @sampling_rate: sampling rate value of audio,  eg 12000, 48000

It's not clear why this is a change in the ABI - the values written and
the types don't change.  Personally I'd not change the ABI - it's a bit
late now that the code has been in the kernel for several releases.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/9] ALSA: compress offfload fixes

[Vinod Koul]

On Tue, Aug 27, 2013 at 04:05:36PM +0200, Takashi Iwai wrote:
> At Tue, 27 Aug 2013 18:44:38 +0530,
> Vinod Koul wrote:
> > 
> > On Tue, Aug 27, 2013 at 02:32:28PM +0200, Takashi Iwai wrote:
> > > > > >  - As disscused on our sample rate problem, lets move to use rate values and I
> > > > > >    will fix the lib too. Since the driver are not upstream the impact of this
> > > > > > change wont be huge.
> > > > > 
> > > > > I see no code touching sampling_rate field.
> > > > Yes its passed directly to the drivers, where tehy use values to program
> > > > decoders. Only meaning of the field is changing now.
> > > 
> > > So you're proposing a patch just changing the comment in the header
> > > file as a stable fix patch?  Please reread stable_kernel_rules.txt
> > > once again.
> > Yes along with header version so that tinycompress can cope with it.
> 
> I've seen nothing but changing the comment in the patch.
> What's missing?
> 
> > The meaning of the value is changing here... 
> 
> Again, is this a bug fix?  Stable patches are only for bug fixes.
yup... we can't do 12K, 24K decoding without this. We actually found when folks
tried aac with 12K

~Vinod
-- 

Re: [PATCH 0/9] ALSA: compress offfload fixes

[Vinod Koul]

On Tue, Aug 27, 2013 at 04:22:39PM +0200, Takashi Iwai wrote:
> I understand it, but my question is what fixes your patch except for
> the comment in the header file?  In other words, by changing the
> comment text, what will be fixed in the end?
the meaning and API version which tinycompress will use to send right values..

~Vinod

Re: [PATCH 2/9] ALSA: compress: use mutex in drain

[Takashi Iwai]

At Tue, 27 Aug 2013 18:39:22 +0530,
Vinod Koul wrote:
> 
> On Tue, Aug 27, 2013 at 02:23:19PM +0200, Takashi Iwai wrote:
> > > > > > > +/* this fn is called without lock being held and we change stream states here
> > > > > > > + * so while using the stream state auquire the lock but relase before invoking
> > > > > > > + * DSP as the call will possibly take a while
> > > > > > > + */
> > > > > > >  static int snd_compr_drain(struct snd_compr_stream *stream)
> > > > > > >  {
> > > > > > >  	int retval;
> > > > > > >  
> > > > > > > +	mutex_lock(&stream->device->lock);
> > > > > > >  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> > > > > > > -			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> > > > > > > -		return -EPERM;
> > > > > > > +			stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
> > > > > > > +		retval = -EPERM;
> > > > > > > +		goto ret;
> > > > > > > +	}
> > > > > > > +	mutex_unlock(&stream->device->lock);
> > > > > > >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> > > > > > 
> > > > > > Why release the lock here?  The trigger callback is called within this
> > > > > > mutex lock in other places.
> > > > > This is the main part :)
> > > > > 
> > > > > Since the drain states will take a while (order of few seconds) to execute so we
> > > > > will be blocked. Thats why we cant have lock here.
> > > > 
> > > > What's about other places calling the trigger ops within lock?
> > > > You can't mix things.
> > > Well i was going to treat drain only as exception to this. The issue here is
> > > during the long drain other triggers are perfectly valid cases
> > 
> > An ops callback must be defined either to be locked or unlocked.
> > Calling in the unlocked context only for some case doesn't sound
> > right.
> well all the callbacks except drain is called with lock held from framework...

You're calling the same ops with and without the lock.
I'd understand better if you created a new ops for drain, but in the
current patch, it's still the same ops...

> > > > > The point of lock is to sync
> > > > > the stream states here.
> > > > 
> > > > Without the lock, it's racy.  What if another thread calls the same
> > > > function at the same time?
> > > that part can be checked by seeing if we are already draining.
> > 
> > But how?  The place you're calling trigger is unlocked.
> > Suppose another thread calling trigger_stop just between
> > mutex_unlock() and stream->ops->trigger(DRAIN) call in the above.
> > The state check doesn't work there.
> the framework does this with lock held and then calls the driver
> something like this will ensure this while making sync right...

It's still racy.  In your code below:

> 	mutex_lock(&lock);
> 	if (state == DRAINING) {
> 		mutex_unlock(&lock);
> 		return -EPERM;
> 	} else
> 		state = DRAINING;
> 	mutex_unlock(&lock);

Suppose that another thread calls stop at this point before
ops->drain() is called.  It'll change the state to STOP, call
ops->trigger(STOP).  And ops->drain() (or ops->trigger(DRAIN)) may be
called at the same time because there is no protection without knowing
that it's being stopped.

> 	ops->drain(substream);
> 
> 	mutex_lock(&lock);
> 	state = DRAINED;
> 	mutex_unlock(&lock);

Takashi


> > > > > We are not modfying anything. During drain and partial
> > > > > drain we need to allow other trigger ops like pause, stop tog o thru so drop the
> > > > > lock here for these two ops only!
> > > > 
> > > > Well, the biggest problem is that there is no proper design for which
> > > > ops take a lock and which not.  The trigger callback is basically to
> > > > trigger the action.  There should be no long-time blocking there.
> > > > (Otherwise you'll definitely loose a gunfight :)
> > > The reason for blocked implementation is to treat return of the call as
> > > notifcation that draining is completed.
> > > 
> > > For example user has written all the buffers, lets says worth 3 secs and now has
> > > triggered drain. User needs to wait till drain is complete before closing the
> > > device etc. So he waits on drain to compelete..
> > > 
> > > Do you have a better way to manage this?
> > 
> > Split the drain action in two parts, trigger and synchronization:
> > 
> > 	lock();
> > 	...
> > 	trigger(pause);
> > 	while (!pause_finished) {
> > 		unlock();
> > 		schedule_or_sleep_or_whatever();
> > 		lock();
> > 	}
> > 	...
> > 	unlock();
> okay, i guess above is on same lines...
> 
> ~Vinod
> -- 
> 

Re: [PATCH 0/9] ALSA: compress offfload fixes

[Takashi Iwai]

At Tue, 27 Aug 2013 18:44:38 +0530,
Vinod Koul wrote:
> 
> On Tue, Aug 27, 2013 at 02:32:28PM +0200, Takashi Iwai wrote:
> > > > >  - As disscused on our sample rate problem, lets move to use rate values and I
> > > > >    will fix the lib too. Since the driver are not upstream the impact of this
> > > > > change wont be huge.
> > > > 
> > > > I see no code touching sampling_rate field.
> > > Yes its passed directly to the drivers, where tehy use values to program
> > > decoders. Only meaning of the field is changing now.
> > 
> > So you're proposing a patch just changing the comment in the header
> > file as a stable fix patch?  Please reread stable_kernel_rules.txt
> > once again.
> Yes along with header version so that tinycompress can cope with it.

I've seen nothing but changing the comment in the patch.
What's missing?

> The meaning of the value is changing here... 

Again, is this a bug fix?  Stable patches are only for bug fixes.


Takashi

Re: [PATCH 2/9] ALSA: compress: use mutex in drain

[Vinod Koul]

On Tue, Aug 27, 2013 at 04:03:03PM +0200, Takashi Iwai wrote:
> > > An ops callback must be defined either to be locked or unlocked.
> > > Calling in the unlocked context only for some case doesn't sound
> > > right.
> > well all the callbacks except drain is called with lock held from framework...
> 
> You're calling the same ops with and without the lock.
> I'd understand better if you created a new ops for drain, but in the
> current patch, it's still the same ops...
yup but cant different callbacks in the ops have different conditions. Anyway
all except drain ones are supposed to NOT block.
> 
> > > > > > The point of lock is to sync
> > > > > > the stream states here.
> > > > > 
> > > > > Without the lock, it's racy.  What if another thread calls the same
> > > > > function at the same time?
> > > > that part can be checked by seeing if we are already draining.
> > > 
> > > But how?  The place you're calling trigger is unlocked.
> > > Suppose another thread calling trigger_stop just between
> > > mutex_unlock() and stream->ops->trigger(DRAIN) call in the above.
> > > The state check doesn't work there.
> > the framework does this with lock held and then calls the driver
> > something like this will ensure this while making sync right...
> 
> It's still racy.  In your code below:
> 
> > 	mutex_lock(&lock);
> > 	if (state == DRAINING) {
> > 		mutex_unlock(&lock);
> > 		return -EPERM;
> > 	} else
> > 		state = DRAINING;
> > 	mutex_unlock(&lock);
> 
> Suppose that another thread calls stop at this point before
> ops->drain() is called.  It'll change the state to STOP, call
> ops->trigger(STOP).  And ops->drain() (or ops->trigger(DRAIN)) may be
> called at the same time because there is no protection without knowing
> that it's being stopped.
yup...

> 
> > 	ops->drain(substream);
> > 
> > 	mutex_lock(&lock);
> > 	state = DRAINED;
> > 	mutex_unlock(&lock);
> 
> Takashi
> 
> 
> > > > > > We are not modfying anything. During drain and partial
> > > > > > drain we need to allow other trigger ops like pause, stop tog o thru so drop the
> > > > > > lock here for these two ops only!
> > > > > 
> > > > > Well, the biggest problem is that there is no proper design for which
> > > > > ops take a lock and which not.  The trigger callback is basically to
> > > > > trigger the action.  There should be no long-time blocking there.
> > > > > (Otherwise you'll definitely loose a gunfight :)
> > > > The reason for blocked implementation is to treat return of the call as
> > > > notifcation that draining is completed.
> > > > 
> > > > For example user has written all the buffers, lets says worth 3 secs and now has
> > > > triggered drain. User needs to wait till drain is complete before closing the
> > > > device etc. So he waits on drain to compelete..
> > > > 
> > > > Do you have a better way to manage this?
> > > 
> > > Split the drain action in two parts, trigger and synchronization:
> > > 
> > > 	lock();
> > > 	...
> > > 	trigger(pause);
> > > 	while (!pause_finished) {
> > > 		unlock();
> > > 		schedule_or_sleep_or_whatever();
> > > 		lock();
> > > 	}
> > > 	...
> > > 	unlock();
with this am thinking of handling it differently now on the above lines and
split the drain action and drain blocking parts. Trigger and action.

How about making the drain and partial drain triggers calls NOT blocking.
This will take care of sync issues etc.

But we also need to have notification for when drain completes.

IMO this can be handled in two ways
a) like in above we sleep and wake up on notification from driver.
Driver can call the drain_complete() which wakes up and we return to user
b) we call the driver for drain and then call blocking new callback which
returns when action is complete

~Vinod
-- 

Re: [PATCH 0/9] ALSA: compress offfload fixes

[Vinod Koul]

On Tue, Aug 27, 2013 at 04:41:13PM +0200, Takashi Iwai wrote:
> At Tue, 27 Aug 2013 19:11:46 +0530,
> Vinod Koul wrote:
> > 
> > On Tue, Aug 27, 2013 at 04:22:39PM +0200, Takashi Iwai wrote:
> > > I understand it, but my question is what fixes your patch except for
> > > the comment in the header file?  In other words, by changing the
> > > comment text, what will be fixed in the end?
> > the meaning and API version which tinycompress will use to send right values..
> 
> It's just a comment text, which itself is nothing bug a explanation,
> and doesn't define the actual behavior.  And there is no code in the
> upstream tree regarding this field, so no place to "fix" there.
> 
> Moreover, tinycompress library should be self-contained so that it can
> be compiled without the kernel source tree.
yes it is and can be compiled without kernel sources...

~Vinod

-- 

Re: [PATCH 0/9] ALSA: compress offfload fixes

[Takashi Iwai]

At Tue, 27 Aug 2013 19:00:30 +0530,
Vinod Koul wrote:
> 
> On Tue, Aug 27, 2013 at 04:05:36PM +0200, Takashi Iwai wrote:
> > At Tue, 27 Aug 2013 18:44:38 +0530,
> > Vinod Koul wrote:
> > > 
> > > On Tue, Aug 27, 2013 at 02:32:28PM +0200, Takashi Iwai wrote:
> > > > > > >  - As disscused on our sample rate problem, lets move to use rate values and I
> > > > > > >    will fix the lib too. Since the driver are not upstream the impact of this
> > > > > > > change wont be huge.
> > > > > > 
> > > > > > I see no code touching sampling_rate field.
> > > > > Yes its passed directly to the drivers, where tehy use values to program
> > > > > decoders. Only meaning of the field is changing now.
> > > > 
> > > > So you're proposing a patch just changing the comment in the header
> > > > file as a stable fix patch?  Please reread stable_kernel_rules.txt
> > > > once again.
> > > Yes along with header version so that tinycompress can cope with it.
> > 
> > I've seen nothing but changing the comment in the patch.
> > What's missing?
> > 
> > > The meaning of the value is changing here... 
> > 
> > Again, is this a bug fix?  Stable patches are only for bug fixes.
> yup... we can't do 12K, 24K decoding without this. We actually found when folks
> tried aac with 12K

I understand it, but my question is what fixes your patch except for
the comment in the header file?  In other words, by changing the
comment text, what will be fixed in the end?


Takashi

Re: [PATCH 9/9] ALSA: compress: use rate values for passing sampling rates

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 748 bytes --]

On Tue, Aug 27, 2013 at 06:56:41PM +0530, Vinod Koul wrote:

> > > - * @sampling_rate: sampling rate of audio
> > > + * @sampling_rate: sampling rate value of audio,  eg 12000, 48000

> > It's not clear why this is a change in the ABI - the values written and
> > the types don't change.  Personally I'd not change the ABI - it's a bit
> > late now that the code has been in the kernel for several releases.

> the problem is the meaning of sampling_rate. It was SND_PCM_RATE_XXX. It was a
> lack of foresight on my side, we should have done values which we are changing
> now to handle rates not defined here.

Given that the documentation didn't mention the constants and there's no
example users I'm not sure this is actually a change at all...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/9] ALSA: compress offfload fixes

[Takashi Iwai]

At Tue, 27 Aug 2013 19:11:46 +0530,
Vinod Koul wrote:
> 
> On Tue, Aug 27, 2013 at 04:22:39PM +0200, Takashi Iwai wrote:
> > I understand it, but my question is what fixes your patch except for
> > the comment in the header file?  In other words, by changing the
> > comment text, what will be fixed in the end?
> the meaning and API version which tinycompress will use to send right values..

It's just a comment text, which itself is nothing bug a explanation,
and doesn't define the actual behavior.  And there is no code in the
upstream tree regarding this field, so no place to "fix" there.

Moreover, tinycompress library should be self-contained so that it can
be compiled without the kernel source tree.


Takashi