Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] staging: bcm2835-audio: Fix draining behavior regression
To:     Takashi Iwai <tiwai@suse.de>, Eric Anholt <eric@anholt.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-rpi-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20190914152405.7416-1-tiwai@suse.de>
From:   Stefan Wahren <wahrenst@gmx.net>
Message-ID: <3b41702b-1cc2-233d-94a6-ece8f5a19957@gmx.net>
Date:   Sun, 15 Sep 2019 15:54:16 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20190914152405.7416-1-tiwai@suse.de>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Takashi,

Am 14.09.19 um 17:24 schrieb Takashi Iwai:
> The PCM draining behavior got broken since the recent refactoring, and
> this turned out to be the incorrect expectation of the firmware
> behavior regarding "draining".  While I expected the "drain" flag at
> the stop operation would do processing the queued samples, it seems
> rather dropping the samples.
>
> As a quick fix, just drop the SNDRV_PCM_INFO_DRAIN_TRIGGER flag, so
> that the driver uses the normal PCM draining procedure.  Also, put
> some caution comment to the function for future readers not to fall
> into the same pitfall.
>
> Fixes: d7ca3a71545b ("staging: bcm2835-audio: Operate non-atomic PCM ops=
")
> BugLink: https://github.com/raspberrypi/linux/issues/2983

thanks for taking care of this. Wouldn't it be better to add the link to
the new comment to provide more context of the unexpected behavior?

Nevertheless:

Acked-by: Stefan Wahren <wahrenst@gmx.net>

> Cc: stable@vger.kernel.org
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c   | 4 ++--
>  drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b=
/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> index bc1eaa3a0773..826016c3431a 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> @@ -12,7 +12,7 @@
>  static const struct snd_pcm_hardware snd_bcm2835_playback_hw =3D {
>  	.info =3D (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER =
|
>  		 SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
> -		 SNDRV_PCM_INFO_DRAIN_TRIGGER | SNDRV_PCM_INFO_SYNC_APPLPTR),
> +		 SNDRV_PCM_INFO_SYNC_APPLPTR),
>  	.formats =3D SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE,
>  	.rates =3D SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_8000_48000,
>  	.rate_min =3D 8000,
> @@ -29,7 +29,7 @@ static const struct snd_pcm_hardware snd_bcm2835_playb=
ack_hw =3D {
>  static const struct snd_pcm_hardware snd_bcm2835_playback_spdif_hw =3D =
{
>  	.info =3D (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER =
|
>  		 SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
> -		 SNDRV_PCM_INFO_DRAIN_TRIGGER | SNDRV_PCM_INFO_SYNC_APPLPTR),
> +		 SNDRV_PCM_INFO_SYNC_APPLPTR),
>  	.formats =3D SNDRV_PCM_FMTBIT_S16_LE,
>  	.rates =3D SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_44100 |
>  	SNDRV_PCM_RATE_48000,
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c=
 b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> index 23fba01107b9..c6f9cf1913d2 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> @@ -289,6 +289,7 @@ int bcm2835_audio_stop(struct bcm2835_alsa_stream *a=
lsa_stream)
>  					 VC_AUDIO_MSG_TYPE_STOP, false);
>  }
>
> +/* FIXME: this doesn't seem working as expected for "draining" */
>  int bcm2835_audio_drain(struct bcm2835_alsa_stream *alsa_stream)
>  {
>  	struct vc_audio_msg m =3D {