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

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

[Jerome Brunet]

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.

-- 
Jerome

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

[Sebastian Andrzej Siewior]

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

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

[Jerome Brunet]

On Mon 05 Aug 2024 at 17:33, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> 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.

Hi Sebastian,

Thanks a lot for taking the time to dig into the driver specifics.
The IRQ handler is actually not awfully critical in this case. The HW
will continue to run regardless of the IRQ being acked or not

The purpose of the handler is mainly to let Alsa know that 1 (or more)
period is ready. If alsa is too slow to react, and the buffer allows
just a few periods, the HW may under/overflow the buffer.

IRQF_ONESHOT is fine because snd_pcm_period_elapsed() 'notifies'
all past periods, not just one. The irq handler does not need to
run again until this function has been called. It also helps if the
period is ridiculously small, to try to reduce the number of IRQs.

>
> 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:

That was mainly there for initial debugging. It does not happen (yet).

>
> 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…

I'd prefer #1 too. IRQ is not shared, so the ownership should be fine.

However I still don't fully understand what we are fixing here TBH. It
seems it could apply to other parts of the kernel so I'd like to
understand what is wrong (and avoid repeating the same mistake)

* With PREEMPT_RT, both handler will threaded, so they should be able to
  call preemptible function, right ?
* If so, and spinlock_lock() actually becomes preemptible with
  PREEMPT_RT as stated in this change description, why is it problem here ?

Do you have an idea about what is going on ?

Again, thanks a lot for your help.

>
> Sebastian

-- 
Jerome

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

[Sebastian Andrzej Siewior]

On 2024-08-05 18:07:28 [+0200], Jerome Brunet wrote:
> Hi Sebastian,
Hi Jerome,

> Thanks a lot for taking the time to dig into the driver specifics.
> The IRQ handler is actually not awfully critical in this case. The HW
> will continue to run regardless of the IRQ being acked or not
> 
> The purpose of the handler is mainly to let Alsa know that 1 (or more)
> period is ready. If alsa is too slow to react, and the buffer allows
> just a few periods, the HW may under/overflow the buffer.
> 
> IRQF_ONESHOT is fine because snd_pcm_period_elapsed() 'notifies'
> all past periods, not just one. The irq handler does not need to
> run again until this function has been called. It also helps if the
> period is ridiculously small, to try to reduce the number of IRQs.

IRQF_ONESHOT is used to disable to keep the IRQ line disabled (after the
primary handler) while the threaded handler is running. This implies
that the primary handler must not be threaded under PREEMPT_RT.
…
> I'd prefer #1 too. IRQ is not shared, so the ownership should be fine.
> 
> However I still don't fully understand what we are fixing here TBH. It
> seems it could apply to other parts of the kernel so I'd like to
> understand what is wrong (and avoid repeating the same mistake)
> 
> * With PREEMPT_RT, both handler will threaded, so they should be able to
>   call preemptible function, right ?

Correct. But flags like IRQF_ONESHOT won't thread the primary handler.

> * If so, and spinlock_lock() actually becomes preemptible with
>   PREEMPT_RT as stated in this change description, why is it problem here ?

Because in this case the primary handler is not threaded and runs in
not preemptible hard-IRQ context.

> Do you have an idea about what is going on ?

Yes.

Sebastian

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

[Jerome Brunet]

On Tue 06 Aug 2024 at 08:50, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> On 2024-08-05 18:07:28 [+0200], Jerome Brunet wrote:
>> Hi Sebastian,
> Hi Jerome,
>
>> Thanks a lot for taking the time to dig into the driver specifics.
>> The IRQ handler is actually not awfully critical in this case. The HW
>> will continue to run regardless of the IRQ being acked or not
>> 
>> The purpose of the handler is mainly to let Alsa know that 1 (or more)
>> period is ready. If alsa is too slow to react, and the buffer allows
>> just a few periods, the HW may under/overflow the buffer.
>> 
>> IRQF_ONESHOT is fine because snd_pcm_period_elapsed() 'notifies'
>> all past periods, not just one. The irq handler does not need to
>> run again until this function has been called. It also helps if the
>> period is ridiculously small, to try to reduce the number of IRQs.
>
> IRQF_ONESHOT is used to disable to keep the IRQ line disabled (after the
> primary handler) while the threaded handler is running. This implies
> that the primary handler must not be threaded under PREEMPT_RT.
> …

This is the point I was missing. It is clear now. Thanks a lot.

I have tweaked #1 and added a few tags but the gist remains the same.
I was going to add you under 'Suggested-by' but maybe putting you as the
actual Author would be more appropriate. 

What do you prefer ?

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

[Sebastian Andrzej Siewior]

On 2024-08-06 11:21:50 [+0200], Jerome Brunet wrote:
> I have tweaked #1 and added a few tags but the gist remains the same.
> I was going to add you under 'Suggested-by' but maybe putting you as the
> actual Author would be more appropriate. 
> 
> What do you prefer ?

Suggested-by is fine.

Sebastian

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

On Mon, Aug 05, 2024 at 05:33:09PM +0200, Sebastian Andrzej Siewior wrote:

> 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.

The clk_enable() is optional, users simply shouldn't use the internal
clock management with devices that it'll cause problems for.

> There might be an additional problem if reg-caching is enabled.

The flat cache is there mostly for the benefit of things accessed from
interrupt context, it guarantees to never do any allocations and doesn't
lock.  You can also use other caches if you ensure that any registers
accessed in interrupt context are already cached so won't trigger any
new allocations.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Sebastian Andrzej Siewior]

On 2024-08-05 17:56:22 [+0100], Mark Brown wrote:
> On Mon, Aug 05, 2024 at 05:33:09PM +0200, Sebastian Andrzej Siewior wrote:
> 
> > 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.
> 
> The clk_enable() is optional, users simply shouldn't use the internal
> clock management with devices that it'll cause problems for.
> 
> > There might be an additional problem if reg-caching is enabled.
> 
> The flat cache is there mostly for the benefit of things accessed from
> interrupt context, it guarantees to never do any allocations and doesn't
> lock.  You can also use other caches if you ensure that any registers
> accessed in interrupt context are already cached so won't trigger any
> new allocations.

My point is simply that those two things could complicate things further
if the desired fix is to (always) use raw_spinlock_t.

Sebastian