[v2,2/4] ASoC: hdmi-codec: remove reference to the current substream
diff mbox series

Message ID 20190506095815.24578-3-jbrunet@baylibre.com
State New
Headers show
Series
  • ASoC: hdmi-codec: fixes and improvements
Related show

Commit Message

Jerome Brunet May 6, 2019, 9:58 a.m. UTC
If the hdmi-codec is on a codec-to-codec link, the substream pointer
it receives is completely made up by snd_soc_dai_link_event().
The pointer will be different between startup() and shutdown().

The hdmi-codec complains when this happens even if it is not really a
problem. The current_substream pointer is not used for anything useful
apart from getting the exclusive ownership of the device.

Remove current_substream pointer and replace the exclusive locking
mechanism with a simple variable and some atomic operations.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/codecs/hdmi-codec.c | 58 ++++++++++-------------------------
 1 file changed, 16 insertions(+), 42 deletions(-)

Comments

Mark Brown May 8, 2019, 7 a.m. UTC | #1
On Mon, May 06, 2019 at 11:58:13AM +0200, Jerome Brunet wrote:

> Remove current_substream pointer and replace the exclusive locking
> mechanism with a simple variable and some atomic operations.

The advantage of mutexes is that they are very simple and clear to
reason about.  It is therefore unclear that this conversion to use
atomic variables is an improvement, they're generally more complex 
to reason about and fragile.
Jerome Brunet May 8, 2019, 8:08 a.m. UTC | #2
On Wed, 2019-05-08 at 16:00 +0900, Mark Brown wrote:
> On Mon, May 06, 2019 at 11:58:13AM +0200, Jerome Brunet wrote:
> 
> > Remove current_substream pointer and replace the exclusive locking
> > mechanism with a simple variable and some atomic operations.
> 
> The advantage of mutexes is that they are very simple and clear to
> reason about.  It is therefore unclear that this conversion to use
> atomic variables is an improvement, they're generally more complex 
> to reason about and fragile.

The point of this patch is not to remove the mutex in favor of atomic
operations

The point was to remove the current_substream pointer in favor of a
simple 'busy' flag. After that, I ended up with a mutex protecting
a flag and it seemed a bit overkill to me. Atomic op seemed like a
good fit for this but I don't really care about that particular
point.

I can put back mutex to protect the flag if you prefer.

Another solution would be to use the mutex as the 'busy' flag.
Get the ownership of the hdmi using try_lock() in startup() and
release it in shutdown()

Would you have a preference Mark ?
Mark Brown May 8, 2019, 8:57 a.m. UTC | #3
On Wed, May 08, 2019 at 10:08:44AM +0200, Jerome Brunet wrote:
> On Wed, 2019-05-08 at 16:00 +0900, Mark Brown wrote:

> > The advantage of mutexes is that they are very simple and clear to
> > reason about.  It is therefore unclear that this conversion to use
> > atomic variables is an improvement, they're generally more complex 
> > to reason about and fragile.

> The point of this patch is not to remove the mutex in favor of atomic
> operations

Sure, but you mixed in a conversion to atomic operations as well.

> I can put back mutex to protect the flag if you prefer.

> Another solution would be to use the mutex as the 'busy' flag.
> Get the ownership of the hdmi using try_lock() in startup() and
> release it in shutdown()

> Would you have a preference Mark ? 

Probably using a mutex for the flag is clearer.  I'd rather keep things
as simple as absolutely possible to avoid people making mistakes in
future.
Jerome Brunet May 9, 2019, 8:11 a.m. UTC | #4
On Wed, 2019-05-08 at 17:57 +0900, Mark Brown wrote:
> > Another solution would be to use the mutex as the 'busy' flag.
> > Get the ownership of the hdmi using try_lock() in startup() and
> > release it in shutdown()
> 
> > Would you have a preference Mark ? 
> 
> Probably using a mutex for the flag is clearer.  I'd rather keep things
> as simple as absolutely possible to avoid people making mistakes in
> future.

Hi Mark,

I received a notification that you applied this patch.
Just to confirm, you expect a follow up patch to re-introduce the mutex, right ?

Thanks
Jerome
Mark Brown May 12, 2019, 8:27 a.m. UTC | #5
On Thu, May 09, 2019 at 10:11:48AM +0200, Jerome Brunet wrote:
> On Wed, 2019-05-08 at 17:57 +0900, Mark Brown wrote:

> > Probably using a mutex for the flag is clearer.  I'd rather keep things
> > as simple as absolutely possible to avoid people making mistakes in
> > future.

> I received a notification that you applied this patch.
> Just to confirm, you expect a follow up patch to re-introduce the mutex, right ?

Right.

Patch
diff mbox series

diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index eb31d7eddcbf..4d32f93f6be6 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -280,11 +280,10 @@  struct hdmi_codec_priv {
 	struct hdmi_codec_pdata hcd;
 	struct snd_soc_dai_driver *daidrv;
 	struct hdmi_codec_daifmt daifmt[2];
-	struct mutex current_stream_lock;
-	struct snd_pcm_substream *current_stream;
 	uint8_t eld[MAX_ELD_BYTES];
 	struct snd_pcm_chmap *chmap_info;
 	unsigned int chmap_idx;
+	unsigned long busy;
 };
 
 static const struct snd_soc_dapm_widget hdmi_widgets[] = {
@@ -392,42 +391,22 @@  static int hdmi_codec_chmap_ctl_get(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
-static int hdmi_codec_new_stream(struct snd_pcm_substream *substream,
-				 struct snd_soc_dai *dai)
-{
-	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
-	int ret = 0;
-
-	mutex_lock(&hcp->current_stream_lock);
-	if (!hcp->current_stream) {
-		hcp->current_stream = substream;
-	} else if (hcp->current_stream != substream) {
-		dev_err(dai->dev, "Only one simultaneous stream supported!\n");
-		ret = -EINVAL;
-	}
-	mutex_unlock(&hcp->current_stream_lock);
-
-	return ret;
-}
-
 static int hdmi_codec_startup(struct snd_pcm_substream *substream,
 			      struct snd_soc_dai *dai)
 {
 	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
 	int ret = 0;
 
-	ret = hdmi_codec_new_stream(substream, dai);
-	if (ret)
-		return ret;
+	ret = test_and_set_bit(0, &hcp->busy);
+	if (ret) {
+		dev_err(dai->dev, "Only one simultaneous stream supported!\n");
+		return -EINVAL;
+	}
 
 	if (hcp->hcd.ops->audio_startup) {
 		ret = hcp->hcd.ops->audio_startup(dai->dev->parent, hcp->hcd.data);
-		if (ret) {
-			mutex_lock(&hcp->current_stream_lock);
-			hcp->current_stream = NULL;
-			mutex_unlock(&hcp->current_stream_lock);
-			return ret;
-		}
+		if (ret)
+			goto err;
 	}
 
 	if (hcp->hcd.ops->get_eld) {
@@ -437,17 +416,18 @@  static int hdmi_codec_startup(struct snd_pcm_substream *substream,
 		if (!ret) {
 			ret = snd_pcm_hw_constraint_eld(substream->runtime,
 							hcp->eld);
-			if (ret) {
-				mutex_lock(&hcp->current_stream_lock);
-				hcp->current_stream = NULL;
-				mutex_unlock(&hcp->current_stream_lock);
-				return ret;
-			}
+			if (ret)
+				goto err;
 		}
 		/* Select chmap supported */
 		hdmi_codec_eld_chmap(hcp);
 	}
 	return 0;
+
+err:
+	/* Release the exclusive lock on error */
+	clear_bit(0, &hcp->busy);
+	return ret;
 }
 
 static void hdmi_codec_shutdown(struct snd_pcm_substream *substream,
@@ -455,14 +435,10 @@  static void hdmi_codec_shutdown(struct snd_pcm_substream *substream,
 {
 	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
 
-	WARN_ON(hcp->current_stream != substream);
-
 	hcp->chmap_idx = HDMI_CODEC_CHMAP_IDX_UNKNOWN;
 	hcp->hcd.ops->audio_shutdown(dai->dev->parent, hcp->hcd.data);
 
-	mutex_lock(&hcp->current_stream_lock);
-	hcp->current_stream = NULL;
-	mutex_unlock(&hcp->current_stream_lock);
+	clear_bit(0, &hcp->busy);
 }
 
 static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
@@ -761,8 +737,6 @@  static int hdmi_codec_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	hcp->hcd = *hcd;
-	mutex_init(&hcp->current_stream_lock);
-
 	hcp->daidrv = devm_kcalloc(dev, dai_count, sizeof(*hcp->daidrv),
 				   GFP_KERNEL);
 	if (!hcp->daidrv)