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