Re: [PATCH v1] ASoC: meson: axg-fifo: set option to use raw spinlock

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2024-07-29 17:57:05 [+0200], Jerome Brunet wrote:
> On Mon 29 Jul 2024 at 16:28, Mark Brown <broonie@kernel.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@kernel.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