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

[Jerome Brunet <jbrunet@xxxxxxxxxxxx>]

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

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

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