From: Takashi Iwai <tiwai@suse.de> To: Arnaud POULIQUEN <arnaud.pouliquen@st.com> Cc: Daniel Palmer <daniel@0x0f.com>, "broonie@kernel.org" <broonie@kernel.org>, "tiwai@suse.com" <tiwai@suse.com>, "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] ASoC: sti: sti_uniperif: Remove driver Date: Tue, 15 Mar 2022 15:34:40 +0100 [thread overview] Message-ID: <s5hee338etb.wl-tiwai@suse.de> (raw) In-Reply-To: <PAXPR10MB47185B76D5F38482FB1125A5F1109@PAXPR10MB4718.EURPRD10.PROD.OUTLOOK.COM> 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. thanks, Takashi
WARNING: multiple messages have this Message-ID (diff)
From: Takashi Iwai <tiwai@suse.de> To: Arnaud POULIQUEN <arnaud.pouliquen@st.com> Cc: "broonie@kernel.org" <broonie@kernel.org>, Daniel Palmer <daniel@0x0f.com>, "tiwai@suse.com" <tiwai@suse.com>, "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] ASoC: sti: sti_uniperif: Remove driver Date: Tue, 15 Mar 2022 15:34:40 +0100 [thread overview] Message-ID: <s5hee338etb.wl-tiwai@suse.de> (raw) In-Reply-To: <PAXPR10MB47185B76D5F38482FB1125A5F1109@PAXPR10MB4718.EURPRD10.PROD.OUTLOOK.COM> 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. thanks, Takashi
next prev parent reply other threads:[~2022-03-15 14:34 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-15 9:13 [PATCH] ASoC: sti: sti_uniperif: Remove driver Daniel Palmer 2022-03-15 9:13 ` Daniel Palmer 2022-03-15 10:27 ` Takashi Iwai 2022-03-15 10:27 ` Takashi Iwai 2022-03-15 13:15 ` Arnaud POULIQUEN 2022-03-15 13:15 ` Arnaud POULIQUEN 2022-03-15 13:26 ` Daniel Palmer 2022-03-15 13:26 ` Daniel Palmer 2022-03-15 14:34 ` Takashi Iwai [this message] 2022-03-15 14:34 ` Takashi Iwai 2022-03-15 15:27 ` Arnaud POULIQUEN 2022-03-15 15:27 ` Arnaud POULIQUEN 2022-03-15 15:34 ` Takashi Iwai 2022-03-15 15:34 ` Takashi Iwai 2022-03-15 13:03 ` kernel test robot 2022-03-15 13:03 ` kernel test robot 2022-03-15 13:24 ` kernel test robot 2022-03-15 13:24 ` kernel test robot 2022-03-15 13:56 ` kernel test robot 2022-03-15 13:56 ` kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=s5hee338etb.wl-tiwai@suse.de \ --to=tiwai@suse.de \ --cc=alsa-devel@alsa-project.org \ --cc=arnaud.pouliquen@st.com \ --cc=broonie@kernel.org \ --cc=daniel@0x0f.com \ --cc=linux-kernel@vger.kernel.org \ --cc=tiwai@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.