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