lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240805153309.k_SfHw62@linutronix.de>
Date: Mon, 5 Aug 2024 17:33:09 +0200
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Jerome Brunet <jbrunet@...libre.com>
Cc: Mark Brown <broonie@...nel.org>, linux-rt-users@...r.kernel.org,
	Arseniy Krasnov <avkrasnov@...utedevices.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Kevin Hilman <khilman@...libre.com>,
	Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
	alsa-devel@...a-project.org, linux-sound@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org,
	kernel@...rdevices.ru, oxffffaa@...il.com,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v1] ASoC: meson: axg-fifo: set option to use raw spinlock

On 2024-07-29 17:57:05 [+0200], Jerome Brunet wrote:
> On Mon 29 Jul 2024 at 16:28, Mark Brown <broonie@...nel.org> wrote:
> 
> > On Mon, Jul 29, 2024 at 05:06:50PM +0200, Jerome Brunet wrote:
> >> On Mon 29 Jul 2024 at 15:44, Mark Brown <broonie@...nel.org> wrote:
> >
> >> > I don't recall this coming up much TBH.  It may be that people just set
> >> > raw spinlocks when they need it, or that there's not many people using
> >> > relevant devices with RT kernels.
> >
> >> I have not been playing much with RT TBH, but AFAIK, with RT irq
> >> handlers are threaded unless IRQF_NO_THREAD is set. In this case,
> >> something preemptible in the handler should not be a problem.
> >
> >> The axg-fifo drivers do not have IRQF_NO_THREAD so something is a bit
> >> unclear here.
> >
> > Yeah, it's definitely likely to happen all the time for people using RT
> > with relevant devices.  I'm not sure I have a good sense of if it's
> > likely to be a nasty surprise to switch raw spinlocks on by default when
> > it's currently controllable, I'd expect it'll generally be fine but it's
> > possibly a bit rude to any users that don't use interrupts...
> 
> Indeed it is bit radical.
> 
> My concern with this patch is that, IIUC, the handler should be
> threaded under RT and there should be no problem with the spinlock API.
> 
> Adding the RT folks to try to get a better understanding, they should
> have been CCed anyway.

I'm not sure if making the lock a raw_spinlock_t solves all the
problems. The regmap is regmap_mmio so direct memory-access and looks
simple enough to do so. In regmap_mmio_write() I see clk_enable() and
and this uses a spinlock_t so we should be back at the same problem.
There might be an additional problem if reg-caching is enabled.

Let me propose two alternatives:
#1: Why two handlers if we have IRQF_ONESHOT and the primary does almost
    nothing. Assuming the thread is always woken up and the "unexpected
    irq" case does not happen. If so, why not:

diff --git a/sound/soc/meson/axg-fifo.c b/sound/soc/meson/axg-fifo.c
index 7e6090af720b9..60af05a3cad6b 100644
--- a/sound/soc/meson/axg-fifo.c
+++ b/sound/soc/meson/axg-fifo.c
@@ -220,9 +220,21 @@ static irqreturn_t axg_fifo_pcm_irq_block(int irq, void *dev_id)
 static irqreturn_t axg_fifo_pcm_irq_block_thread(int irq, void *dev_id)
 {
 	struct snd_pcm_substream *ss = dev_id;
+	struct axg_fifo *fifo = axg_fifo_data(ss);
+	unsigned int status;
+
+	regmap_read(fifo->map, FIFO_STATUS1, &status);
+	status = FIELD_GET(STATUS1_INT_STS, status);
+
+	/* Use the thread to call period elapsed on nonatomic links */
+	if (!(status & FIFO_INT_COUNT_REPEAT)) {
+		dev_dbg(axg_fifo_dev(ss), "unexpected irq - STS 0x%02x\n",
+			status);
+		return IRQ_NONE;
+	}
+	axg_fifo_ack_irq(fifo, status);
 
 	snd_pcm_period_elapsed(ss);
-
 	return IRQ_HANDLED;
 }
 
@@ -251,9 +263,9 @@ int axg_fifo_pcm_open(struct snd_soc_component *component,
 	if (ret)
 		return ret;
 
-	ret = request_threaded_irq(fifo->irq, axg_fifo_pcm_irq_block,
-				   axg_fifo_pcm_irq_block_thread,
-				   IRQF_ONESHOT, dev_name(dev), ss);
+	ret = request_threaded_irq(fifo->irq, NULL,
+				   axg_fifo_pcm_irq_block_thread, IRQF_ONESHOT,
+				   dev_name(dev), ss);
 	if (ret)
 		return ret;
 

#2: If two handers are required due to $REASON then primary should ACK/
    disable the interrupt line while the secondary/ threaded handler is
    running. In this is case then IRQF_ONESHOT is not needed because its
    "tasks" is performed by the primary handler:

diff --git a/sound/soc/meson/axg-fifo.c b/sound/soc/meson/axg-fifo.c
index 7e6090af720b9..5b4c518eb4ccd 100644
--- a/sound/soc/meson/axg-fifo.c
+++ b/sound/soc/meson/axg-fifo.c
@@ -205,11 +205,16 @@ static irqreturn_t axg_fifo_pcm_irq_block(int irq, void *dev_id)
 
 	regmap_read(fifo->map, FIFO_STATUS1, &status);
 	status = FIELD_GET(STATUS1_INT_STS, status);
-	axg_fifo_ack_irq(fifo, status);
 
 	/* Use the thread to call period elapsed on nonatomic links */
-	if (status & FIFO_INT_COUNT_REPEAT)
+	if (status & FIFO_INT_COUNT_REPEAT) {
+		/*
+		 * ACKs/ Disables the interrupt until re-enabled by
+		 * axg_fifo_pcm_irq_block_thread()
+		 */
+		axg_fifo_ack_irq(fifo, status);
 		return IRQ_WAKE_THREAD;
+	}
 
 	dev_dbg(axg_fifo_dev(ss), "unexpected irq - STS 0x%02x\n",
 		status);
@@ -253,7 +258,7 @@ int axg_fifo_pcm_open(struct snd_soc_component *component,
 
 	ret = request_threaded_irq(fifo->irq, axg_fifo_pcm_irq_block,
 				   axg_fifo_pcm_irq_block_thread,
-				   IRQF_ONESHOT, dev_name(dev), ss);
+				   0, dev_name(dev), ss);
 	if (ret)
 		return ret;

On the PREEMPT_RT both handler will be threaded.
 
My favorite is #1. Also ACKing the interrupt only if it occurred for the
device/ driver in charge. Otherwise don't careā€¦

Sebastian

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ