On Tue, 15 Mar 2022 16:27:40 +0100, Arnaud POULIQUEN wrote:
ST Restricted
-----Original Message----- From: Takashi Iwai tiwai@suse.de Sent: mardi 15 mars 2022 15:35 To: Arnaud POULIQUEN arnaud.pouliquen@st.com Cc: Daniel Palmer daniel@0x0f.com; broonie@kernel.org; tiwai@suse.com; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH] ASoC: sti: sti_uniperif: Remove driver
On Tue, 15 Mar 2022 14:15:20 +0100, Arnaud POULIQUEN wrote:
Hello,
ST Restricted
-----Original Message----- From: Takashi Iwai tiwai@suse.de Sent: mardi 15 mars 2022 11:28 To: Daniel Palmer daniel@0x0f.com Cc: broonie@kernel.org; tiwai@suse.com; Arnaud POULIQUEN arnaud.pouliquen@st.com; alsa-devel@alsa-project.org; linux- kernel@vger.kernel.org Subject: Re: [PATCH] ASoC: sti: sti_uniperif: Remove driver
On Tue, 15 Mar 2022 10:13:19 +0100, Daniel Palmer wrote:
This driver seems to be in the "only good for attracting bot generated patches" phase of it's life.
It doesn't seem like anyone actually tested the patches that have been applied in the last few years as uni_reader_irq_handler() had a dead lock added to it (it locks the stream, then calls snd_pcm_stop_xrun() which will also lock the stream).
Mea culpa, that was an obvious deadlock I overlooked in the patch series.
Seems best just to remove it.
Signed-off-by: Daniel Palmer daniel@0x0f.com
I've never used this driver, don't have the hardware etc. I just noticed that this looks broken when debugging my own driver that uses snd_pcm_stop_xrun() and was looking at other users to see if I was using it wrong and noticed this was the only place that locked the stream before calling snd_pcm_stop_xrun().
There are probably some other bits of the driver that should be removed but I didn't look that hard.
TL;DR; This driver seems broken, seems like nobody uses it. Maybe it should be deleted?
Yeah, that looks dead.
The platform is still used for instance: https://lore.kernel.org/all/1d95209f-9cb4-47a3-2696-7a93df7cdc05@foss. st.com/
So please do not remove the driver
Ah, it's always good to see a vital sign!
The issue has not been detected because it is related to an error that occurs only when we reach the limit of the platform, with application that stop the stream at same time. So almost no chance to occur.
OTOH, if anyone really wants to keep the stuff, please revert the commit dc865fb9e7c2251c9585ff6a7bf185d499db13e4.
Yes reverting the commit is one solution. The other is to clean-up the snd_pcm_stream_lock/ snd_pcm_stream_unlock in the Handler.
That would work, but maybe it's safer to keep that lock, as the state change isn't protected by irq_lock but only implicitly by stream lock in start/stop callbacks.
You are right, trying to use the snd_pcm_stop_xrun needs deeper update that could introduce regression. It seems wiser to revert your commit dc865fb9e7c2 as you propose.
OK, I'm going to submit the revert.
thanks,
Takashi