Re: [alsa-devel] [PATCH 5.6 09/71] ALSA: firewire: Use managed buffer allocation

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

Hi,

I'm not opposed to this patchset. The direction is preferable.

However changes in this patchset brings bugs in error path of
each implementation for pcm.hw_params.

On Mon, Dec 09, 2019 at 10:48:41AM +0100, Takashi Iwai wrote:
> diff --git a/sound/firewire/bebob/bebob_pcm.c b/sound/firewire/bebob/bebob_pcm.c
> index d4edd06d32cf..2ee91ff6891e 100644
> --- a/sound/firewire/bebob/bebob_pcm.c
> +++ b/sound/firewire/bebob/bebob_pcm.c
> @@ -214,10 +214,6 @@ static int pcm_hw_params(struct snd_pcm_substream *substream,
>  	struct snd_bebob *bebob = substream->private_data;
>  	int err;
>  
> -	err = snd_pcm_lib_malloc_pages(substream, params_buffer_bytes(hw_params));
> -	if (err < 0)
> -		return err;
> -
>  	if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN) {
>  		unsigned int rate = params_rate(hw_params);
>  		unsigned int frames_per_period = params_period_size(hw_params);
> @@ -231,7 +227,7 @@ static int pcm_hw_params(struct snd_pcm_substream *substream,
>  		mutex_unlock(&bebob->mutex);
>  	}
>  
> -	return err;
> +	return 0;
>  }
  
This should return the err variable to report failure of isochronous
resources reservation.

However I found that ALSA oxfw/fireface drivers returns 0 even if it
fails. This is bug since v5.3 kernel.

Would you please apply below two patches into your three, then modify
your patch for all firewire drivers to return error correctly?

======== 8< --------

From a2cddad1c74e8e825739db63c17c0efabed1c93d Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Date: Tue, 10 Dec 2019 00:03:04 +0900
Subject: [PATCH 1/2] ALSA: oxfw: fix return value in error path of isochronous
 resources reservation

Even if isochronous resources reservation fails, error code doesn't return
in pcm.hw_params callback.

Cc: <stable@vger.kernel.org> #5.3+
Fixes: 4f380d007052 ("ALSA: oxfw: configure packet format in pcm.hw_params callback")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/oxfw/oxfw-pcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/firewire/oxfw/oxfw-pcm.c b/sound/firewire/oxfw/oxfw-pcm.c
index 9124603edabe..67fd3e844dd6 100644
--- a/sound/firewire/oxfw/oxfw-pcm.c
+++ b/sound/firewire/oxfw/oxfw-pcm.c
@@ -285,7 +285,7 @@ static int pcm_playback_hw_params(struct snd_pcm_substream *substream,
 		mutex_unlock(&oxfw->mutex);
 	}
 
-	return 0;
+	return err;
 }
 
 static int pcm_capture_hw_free(struct snd_pcm_substream *substream)
-- 
2.20.1

======== 8< --------

From cde8560eddc0e1f276865fa7f9bbccd27714d286 Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Date: Tue, 10 Dec 2019 00:05:41 +0900
Subject: [PATCH 2/2] ALSA: fireface: fix return value in error path of isochronous
 resources reservation

Even if isochronous resources reservation fails, error code doesn't return
in pcm.hw_params callback.

Cc: <stable@vger.kernel.org> #5.3+
Fixes: 55162d2bb0e8 ("ALSA: fireface: reserve/release isochronous resources in pcm.hw_params/hw_free callbacks")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/fireface/ff-pcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/firewire/fireface/ff-pcm.c b/sound/firewire/fireface/ff-pcm.c
index 4e3bd9a2bec0..bd91c6ecb112 100644
--- a/sound/firewire/fireface/ff-pcm.c
+++ b/sound/firewire/fireface/ff-pcm.c
@@ -247,7 +247,7 @@ static int pcm_hw_params(struct snd_pcm_substream *substream,
 		mutex_unlock(&ff->mutex);
 	}
 
-	return 0;
+	return err;
 }
 
 static int pcm_hw_free(struct snd_pcm_substream *substream)
-- 
2.20.1

======== 8< --------


Thanks

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel