[PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Lionel Debieve]

Use raw_spin_lock in enable/disable channel as it comes from
interrupt context.

BUG: sleeping function called from invalid context at
kernel/locking/rtmutex.c:995
in_atomic(): 1, irqs_disabled(): 128, pid: 307, name: pulseaudio
Preemption disabled at:
[<c01790fc>] __handle_domain_irq+0x4c/0xec
CPU: 0 PID: 307 Comm: pulseaudio
Hardware name: STi SoC with Flattened Device Tree
[<c011046c>] (unwind_backtrace)
[<c010c7f4>] (show_stack)
[<c03d1578>] (dump_stack)
[<c014e440>] (___might_sleep)
[<c08e7f24>] (rt_spin_lock)
[<c069bb04>] (sti_mbox_disable_channel)
[<c069befc>] (sti_mbox_irq_handler)
[<c0179900>] (__handle_irq_event_percpu)
[<c01799dc>] (handle_irq_event_percpu)
[<c0179a78>] (handle_irq_event)
[<c017d1c8>] (handle_fasteoi_irq)
[<c0178c08>] (generic_handle_irq)
[<c017912c>] (__handle_domain_irq)
[<c0101488>] (gic_handle_irq)

Signed-off-by: Lionel Debieve <lionel.debieve@st.com>
---
 drivers/mailbox/mailbox-sti.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mailbox/mailbox-sti.c b/drivers/mailbox/mailbox-sti.c
index 41bcd33..f9674ca 100644
--- a/drivers/mailbox/mailbox-sti.c
+++ b/drivers/mailbox/mailbox-sti.c
@@ -60,7 +60,7 @@ struct sti_mbox_device {
 	void __iomem		*base;
 	const char		*name;
 	u32			enabled[STI_MBOX_INST_MAX];
-	spinlock_t		lock;
+	raw_spinlock_t		lock;
 };
 
 /**
@@ -129,10 +129,10 @@ static void sti_mbox_enable_channel(struct mbox_chan *chan)
 	unsigned long flags;
 	void __iomem *base = MBOX_BASE(mdev, instance);
 
-	spin_lock_irqsave(&mdev->lock, flags);
+	raw_spin_lock_irqsave(&mdev->lock, flags);
 	mdev->enabled[instance] |= BIT(channel);
 	writel_relaxed(BIT(channel), base + STI_ENA_SET_OFFSET);
-	spin_unlock_irqrestore(&mdev->lock, flags);
+	raw_spin_unlock_irqrestore(&mdev->lock, flags);
 }
 
 static void sti_mbox_disable_channel(struct mbox_chan *chan)
@@ -144,10 +144,10 @@ static void sti_mbox_disable_channel(struct mbox_chan *chan)
 	unsigned long flags;
 	void __iomem *base = MBOX_BASE(mdev, instance);
 
-	spin_lock_irqsave(&mdev->lock, flags);
+	raw_spin_lock_irqsave(&mdev->lock, flags);
 	mdev->enabled[instance] &= ~BIT(channel);
 	writel_relaxed(BIT(channel), base + STI_ENA_CLR_OFFSET);
-	spin_unlock_irqrestore(&mdev->lock, flags);
+	raw_spin_unlock_irqrestore(&mdev->lock, flags);
 }
 
 static void sti_mbox_clear_irq(struct mbox_chan *chan)
@@ -450,7 +450,7 @@ static int sti_mbox_probe(struct platform_device *pdev)
 	mdev->dev		= &pdev->dev;
 	mdev->mbox		= mbox;
 
-	spin_lock_init(&mdev->lock);
+	raw_spin_lock_init(&mdev->lock);
 
 	/* STi Mailbox does not have a Tx-Done or Tx-Ready IRQ */
 	mbox->txdone_irq	= false;
-- 
2.7.4

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Steven Rostedt]

On Wed, 22 Mar 2017 16:18:43 +0100
Lionel Debieve <lionel.debieve@st.com> wrote:

> Use raw_spin_lock in enable/disable channel as it comes from
> interrupt context.
> 
> BUG: sleeping function called from invalid context at
> kernel/locking/rtmutex.c:995
> in_atomic(): 1, irqs_disabled(): 128, pid: 307, name: pulseaudio
> Preemption disabled at:
> [<c01790fc>] __handle_domain_irq+0x4c/0xec
> CPU: 0 PID: 307 Comm: pulseaudio
> Hardware name: STi SoC with Flattened Device Tree
> [<c011046c>] (unwind_backtrace)
> [<c010c7f4>] (show_stack)
> [<c03d1578>] (dump_stack)
> [<c014e440>] (___might_sleep)
> [<c08e7f24>] (rt_spin_lock)
> [<c069bb04>] (sti_mbox_disable_channel)
> [<c069befc>] (sti_mbox_irq_handler)
> [<c0179900>] (__handle_irq_event_percpu)
> [<c01799dc>] (handle_irq_event_percpu)
> [<c0179a78>] (handle_irq_event)
> [<c017d1c8>] (handle_fasteoi_irq)
> [<c0178c08>] (generic_handle_irq)
> [<c017912c>] (__handle_domain_irq)
> [<c0101488>] (gic_handle_irq)
> 
> Signed-off-by: Lionel Debieve <lionel.debieve@st.com>

Looks fine to me. Should this go to mainline?

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> ---
>  drivers/mailbox/mailbox-sti.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mailbox/mailbox-sti.c
> b/drivers/mailbox/mailbox-sti.c index 41bcd33..f9674ca 100644
> --- a/drivers/mailbox/mailbox-sti.c
> +++ b/drivers/mailbox/mailbox-sti.c
> @@ -60,7 +60,7 @@ struct sti_mbox_device {
>  	void __iomem		*base;
>  	const char		*name;
>  	u32			enabled[STI_MBOX_INST_MAX];
> -	spinlock_t		lock;
> +	raw_spinlock_t		lock;
>  };
>  
>  /**
> @@ -129,10 +129,10 @@ static void sti_mbox_enable_channel(struct
> mbox_chan *chan) unsigned long flags;
>  	void __iomem *base = MBOX_BASE(mdev, instance);
>  
> -	spin_lock_irqsave(&mdev->lock, flags);
> +	raw_spin_lock_irqsave(&mdev->lock, flags);
>  	mdev->enabled[instance] |= BIT(channel);
>  	writel_relaxed(BIT(channel), base + STI_ENA_SET_OFFSET);
> -	spin_unlock_irqrestore(&mdev->lock, flags);
> +	raw_spin_unlock_irqrestore(&mdev->lock, flags);
>  }
>  
>  static void sti_mbox_disable_channel(struct mbox_chan *chan)
> @@ -144,10 +144,10 @@ static void sti_mbox_disable_channel(struct
> mbox_chan *chan) unsigned long flags;
>  	void __iomem *base = MBOX_BASE(mdev, instance);
>  
> -	spin_lock_irqsave(&mdev->lock, flags);
> +	raw_spin_lock_irqsave(&mdev->lock, flags);
>  	mdev->enabled[instance] &= ~BIT(channel);
>  	writel_relaxed(BIT(channel), base + STI_ENA_CLR_OFFSET);
> -	spin_unlock_irqrestore(&mdev->lock, flags);
> +	raw_spin_unlock_irqrestore(&mdev->lock, flags);
>  }
>  
>  static void sti_mbox_clear_irq(struct mbox_chan *chan)
> @@ -450,7 +450,7 @@ static int sti_mbox_probe(struct platform_device
> *pdev) mdev->dev		= &pdev->dev;
>  	mdev->mbox		= mbox;
>  
> -	spin_lock_init(&mdev->lock);
> +	raw_spin_lock_init(&mdev->lock);
>  
>  	/* STi Mailbox does not have a Tx-Done or Tx-Ready IRQ */
>  	mbox->txdone_irq	= false;

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Lionel DEBIEVE]

On 03/22/2017 05:05 PM, Steven Rostedt wrote:
> On Wed, 22 Mar 2017 16:18:43 +0100
> Lionel Debieve <lionel.debieve@st.com> wrote:
>
>> Use raw_spin_lock in enable/disable channel as it comes from
>> interrupt context.
>>
>> BUG: sleeping function called from invalid context at
>> kernel/locking/rtmutex.c:995
>> in_atomic(): 1, irqs_disabled(): 128, pid: 307, name: pulseaudio
>> Preemption disabled at:
>> [<c01790fc>] __handle_domain_irq+0x4c/0xec
>> CPU: 0 PID: 307 Comm: pulseaudio
>> Hardware name: STi SoC with Flattened Device Tree
>> [<c011046c>] (unwind_backtrace)
>> [<c010c7f4>] (show_stack)
>> [<c03d1578>] (dump_stack)
>> [<c014e440>] (___might_sleep)
>> [<c08e7f24>] (rt_spin_lock)
>> [<c069bb04>] (sti_mbox_disable_channel)
>> [<c069befc>] (sti_mbox_irq_handler)
>> [<c0179900>] (__handle_irq_event_percpu)
>> [<c01799dc>] (handle_irq_event_percpu)
>> [<c0179a78>] (handle_irq_event)
>> [<c017d1c8>] (handle_fasteoi_irq)
>> [<c0178c08>] (generic_handle_irq)
>> [<c017912c>] (__handle_domain_irq)
>> [<c0101488>] (gic_handle_irq)
>>
>> Signed-off-by: Lionel Debieve <lionel.debieve@st.com>
> Looks fine to me. Should this go to mainline?
>
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>
> -- Steve

Will look deeper if it can be.

Lionel

>
>> ---
>>   drivers/mailbox/mailbox-sti.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mailbox/mailbox-sti.c
>> b/drivers/mailbox/mailbox-sti.c index 41bcd33..f9674ca 100644
>> --- a/drivers/mailbox/mailbox-sti.c
>> +++ b/drivers/mailbox/mailbox-sti.c
>> @@ -60,7 +60,7 @@ struct sti_mbox_device {
>>   	void __iomem		*base;
>>   	const char		*name;
>>   	u32			enabled[STI_MBOX_INST_MAX];
>> -	spinlock_t		lock;
>> +	raw_spinlock_t		lock;
>>   };
>>   
>>   /**
>> @@ -129,10 +129,10 @@ static void sti_mbox_enable_channel(struct
>> mbox_chan *chan) unsigned long flags;
>>   	void __iomem *base = MBOX_BASE(mdev, instance);
>>   
>> -	spin_lock_irqsave(&mdev->lock, flags);
>> +	raw_spin_lock_irqsave(&mdev->lock, flags);
>>   	mdev->enabled[instance] |= BIT(channel);
>>   	writel_relaxed(BIT(channel), base + STI_ENA_SET_OFFSET);
>> -	spin_unlock_irqrestore(&mdev->lock, flags);
>> +	raw_spin_unlock_irqrestore(&mdev->lock, flags);
>>   }
>>   
>>   static void sti_mbox_disable_channel(struct mbox_chan *chan)
>> @@ -144,10 +144,10 @@ static void sti_mbox_disable_channel(struct
>> mbox_chan *chan) unsigned long flags;
>>   	void __iomem *base = MBOX_BASE(mdev, instance);
>>   
>> -	spin_lock_irqsave(&mdev->lock, flags);
>> +	raw_spin_lock_irqsave(&mdev->lock, flags);
>>   	mdev->enabled[instance] &= ~BIT(channel);
>>   	writel_relaxed(BIT(channel), base + STI_ENA_CLR_OFFSET);
>> -	spin_unlock_irqrestore(&mdev->lock, flags);
>> +	raw_spin_unlock_irqrestore(&mdev->lock, flags);
>>   }
>>   
>>   static void sti_mbox_clear_irq(struct mbox_chan *chan)
>> @@ -450,7 +450,7 @@ static int sti_mbox_probe(struct platform_device
>> *pdev) mdev->dev		= &pdev->dev;
>>   	mdev->mbox		= mbox;
>>   
>> -	spin_lock_init(&mdev->lock);
>> +	raw_spin_lock_init(&mdev->lock);
>>   
>>   	/* STi Mailbox does not have a Tx-Done or Tx-Ready IRQ */
>>   	mbox->txdone_irq	= false;
>

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Julia Cartwright]

On Wed, Mar 22, 2017 at 04:18:43PM +0100, Lionel Debieve wrote:
> Use raw_spin_lock in enable/disable channel as it comes from
> interrupt context.
> 
> BUG: sleeping function called from invalid context at
> kernel/locking/rtmutex.c:995
> in_atomic(): 1, irqs_disabled(): 128, pid: 307, name: pulseaudio
> Preemption disabled at:
> [<c01790fc>] __handle_domain_irq+0x4c/0xec
> CPU: 0 PID: 307 Comm: pulseaudio
> Hardware name: STi SoC with Flattened Device Tree
> [<c011046c>] (unwind_backtrace)
> [<c010c7f4>] (show_stack)
> [<c03d1578>] (dump_stack)
> [<c014e440>] (___might_sleep)
> [<c08e7f24>] (rt_spin_lock)
> [<c069bb04>] (sti_mbox_disable_channel)
> [<c069befc>] (sti_mbox_irq_handler)
> [<c0179900>] (__handle_irq_event_percpu)
> [<c01799dc>] (handle_irq_event_percpu)
> [<c0179a78>] (handle_irq_event)
> [<c017d1c8>] (handle_fasteoi_irq)
> [<c0178c08>] (generic_handle_irq)
> [<c017912c>] (__handle_domain_irq)
> [<c0101488>] (gic_handle_irq)

Which kernel were you testing on, here?  From what I can tell, this
should have been fixed with Thomas's commit:

   2a1d3ab8986d ("genirq: Handle force threading of irqs with primary and thread handler")

Which landed in 4.4.  It forces the primary handler to be threaded as
well.

   Julia

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Steven Rostedt]

On Wed, 22 Mar 2017 12:37:59 -0500
Julia Cartwright <julia@ni.com> wrote:

> Which kernel were you testing on, here?  From what I can tell, this
> should have been fixed with Thomas's commit:
> 
>    2a1d3ab8986d ("genirq: Handle force threading of irqs with primary
> and thread handler")

Thanks Julia for looking into this. I just looked at the code, and saw
that it does very little with the lock held, and was fine with the
conversion. But if that interrupt handler should be in a thread, we
should see if that's the issue first.

> 
> Which landed in 4.4.  It forces the primary handler to be threaded as
> well.
> 

Lionel,

Can you check if this is an issue with 4.4 too?

-- Steve

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Grygorii Strashko]

On 03/22/2017 01:01 PM, Steven Rostedt wrote:
> On Wed, 22 Mar 2017 12:37:59 -0500
> Julia Cartwright <julia@ni.com> wrote:
>
>> Which kernel were you testing on, here?  From what I can tell, this
>> should have been fixed with Thomas's commit:
>>
>>    2a1d3ab8986d ("genirq: Handle force threading of irqs with primary
>> and thread handler")
>
> Thanks Julia for looking into this. I just looked at the code, and saw
> that it does very little with the lock held, and was fine with the
> conversion. But if that interrupt handler should be in a thread, we
> should see if that's the issue first.


It will not be threaded because there are IRQF_ONESHOT used.

	ret = devm_request_threaded_irq(&pdev->dev, irq,
					sti_mbox_irq_handler,
					sti_mbox_thread_handler,
					IRQF_ONESHOT, mdev->name, mdev);

>
>>
>> Which landed in 4.4.  It forces the primary handler to be threaded as
>> well.
>>
>
> Lionel,
>
> Can you check if this is an issue with 4.4 too?
>


-- 
regards,
-grygorii

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Julia Cartwright]

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

On Wed, Mar 22, 2017 at 01:30:12PM -0500, Grygorii Strashko wrote:
> 
> On 03/22/2017 01:01 PM, Steven Rostedt wrote:
> > On Wed, 22 Mar 2017 12:37:59 -0500
> > Julia Cartwright <julia@ni.com> wrote:
> > 
> > > Which kernel were you testing on, here?  From what I can tell, this
> > > should have been fixed with Thomas's commit:
> > > 
> > >    2a1d3ab8986d ("genirq: Handle force threading of irqs with primary
> > > and thread handler")
> > 
> > Thanks Julia for looking into this. I just looked at the code, and saw
> > that it does very little with the lock held, and was fine with the
> > conversion. But if that interrupt handler should be in a thread, we
> > should see if that's the issue first.
> 
> 
> It will not be threaded because there are IRQF_ONESHOT used.
> 
> 	ret = devm_request_threaded_irq(&pdev->dev, irq,
> 					sti_mbox_irq_handler,
> 					sti_mbox_thread_handler,
> 					IRQF_ONESHOT, mdev->name, mdev);

Indeed.  I had skipped over this important detail when I was skimming
through the code.

Thanks for clarifying!

Is IRQF_ONESHOT really necessary for this device?  The primary handler
invokes sti_mbox_disable_channel() on the interrupting channel, which I
would hope would acquiesce the pending interrupt at the device-level?

Also, as written there are num_inst reads of STI_IRQ_VAL_OFFSET in the
primary handler, which seems inefficient...(unless of course reading
incurs side effects, here).

   Julia

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

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Lionel DEBIEVE]

On 03/22/2017 07:47 PM, Julia Cartwright wrote:
> On Wed, Mar 22, 2017 at 01:30:12PM -0500, Grygorii Strashko wrote:
>> On 03/22/2017 01:01 PM, Steven Rostedt wrote:
>>> On Wed, 22 Mar 2017 12:37:59 -0500
>>> Julia Cartwright <julia@ni.com> wrote:
>>>
>>>> Which kernel were you testing on, here?  From what I can tell, this
>>>> should have been fixed with Thomas's commit:
>>>>
>>>>     2a1d3ab8986d ("genirq: Handle force threading of irqs with primary
>>>> and thread handler")
>>> Thanks Julia for looking into this. I just looked at the code, and saw
>>> that it does very little with the lock held, and was fine with the
>>> conversion. But if that interrupt handler should be in a thread, we
>>> should see if that's the issue first.
>>
>> It will not be threaded because there are IRQF_ONESHOT used.
>>
>> 	ret = devm_request_threaded_irq(&pdev->dev, irq,
>> 					sti_mbox_irq_handler,
>> 					sti_mbox_thread_handler,
>> 					IRQF_ONESHOT, mdev->name, mdev);
> Indeed.  I had skipped over this important detail when I was skimming
> through the code.
>
> Thanks for clarifying!
>
> Is IRQF_ONESHOT really necessary for this device?  The primary handler
> invokes sti_mbox_disable_channel() on the interrupting channel, which I
> would hope would acquiesce the pending interrupt at the device-level?
>
> Also, as written there are num_inst reads of STI_IRQ_VAL_OFFSET in the
> primary handler, which seems inefficient...(unless of course reading
> incurs side effects, here).
>
>     Julia

First to reply Julia, test was made using 4.9.y kernel branch.
For the IRQF_ONESHOT, I rely on Lee (adding in mail thread) that was at the device driver origin.

Steven, you're also right as the patch can be also pushed in mainline too.

Lionel

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Lee Jones]

On Thu, 23 Mar 2017, Lionel DEBIEVE wrote:

> On 03/22/2017 07:47 PM, Julia Cartwright wrote:
> > On Wed, Mar 22, 2017 at 01:30:12PM -0500, Grygorii Strashko wrote:
> >> On 03/22/2017 01:01 PM, Steven Rostedt wrote:
> >>> On Wed, 22 Mar 2017 12:37:59 -0500
> >>> Julia Cartwright <julia@ni.com> wrote:
> >>>
> >>>> Which kernel were you testing on, here?  From what I can tell, this
> >>>> should have been fixed with Thomas's commit:
> >>>>
> >>>>     2a1d3ab8986d ("genirq: Handle force threading of irqs with primary
> >>>> and thread handler")
> >>> Thanks Julia for looking into this. I just looked at the code, and saw
> >>> that it does very little with the lock held, and was fine with the
> >>> conversion. But if that interrupt handler should be in a thread, we
> >>> should see if that's the issue first.
> >>
> >> It will not be threaded because there are IRQF_ONESHOT used.
> >>
> >> 	ret = devm_request_threaded_irq(&pdev->dev, irq,
> >> 					sti_mbox_irq_handler,
> >> 					sti_mbox_thread_handler,
> >> 					IRQF_ONESHOT, mdev->name, mdev);
> > Indeed.  I had skipped over this important detail when I was skimming
> > through the code.
> >
> > Thanks for clarifying!
> >
> > Is IRQF_ONESHOT really necessary for this device?  The primary handler
> > invokes sti_mbox_disable_channel() on the interrupting channel, which I
> > would hope would acquiesce the pending interrupt at the device-level?

Not sure.  This part of the code is remanent from when I re-wrote it.

What is the alternative?

NB: What does 'acquiesce' mean in this context?  Is that a typo?

> > Also, as written there are num_inst reads of STI_IRQ_VAL_OFFSET in the
> > primary handler, which seems inefficient...(unless of course reading
> > incurs side effects, here).

Inefficient in what respect?

> First to reply Julia, test was made using 4.9.y kernel branch.
> For the IRQF_ONESHOT, I rely on Lee (adding in mail thread) that was at the device driver origin.
> 
> Steven, you're also right as the patch can be also pushed in mainline too.
> 
> Lionel
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Julia Cartwright]

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

On Thu, Mar 23, 2017 at 10:26:49AM +0000, Lee Jones wrote:
> On Thu, 23 Mar 2017, Lionel DEBIEVE wrote:
> 
> > On 03/22/2017 07:47 PM, Julia Cartwright wrote:
> > > On Wed, Mar 22, 2017 at 01:30:12PM -0500, Grygorii Strashko wrote:
> > >> On 03/22/2017 01:01 PM, Steven Rostedt wrote:
> > >>> On Wed, 22 Mar 2017 12:37:59 -0500
> > >>> Julia Cartwright <julia@ni.com> wrote:
> > >>>
> > >>>> Which kernel were you testing on, here?  From what I can tell, this
> > >>>> should have been fixed with Thomas's commit:
> > >>>>
> > >>>>     2a1d3ab8986d ("genirq: Handle force threading of irqs with primary
> > >>>> and thread handler")
> > >>> Thanks Julia for looking into this. I just looked at the code, and saw
> > >>> that it does very little with the lock held, and was fine with the
> > >>> conversion. But if that interrupt handler should be in a thread, we
> > >>> should see if that's the issue first.
> > >>
> > >> It will not be threaded because there are IRQF_ONESHOT used.
> > >>
> > >> 	ret = devm_request_threaded_irq(&pdev->dev, irq,
> > >> 					sti_mbox_irq_handler,
> > >> 					sti_mbox_thread_handler,
> > >> 					IRQF_ONESHOT, mdev->name, mdev);
> > > Indeed.  I had skipped over this important detail when I was skimming
> > > through the code.
> > >
> > > Thanks for clarifying!
> > >
> > > Is IRQF_ONESHOT really necessary for this device?  The primary handler
> > > invokes sti_mbox_disable_channel() on the interrupting channel, which I
> > > would hope would acquiesce the pending interrupt at the device-level?
> 
> Not sure.  This part of the code is remanent from when I re-wrote it.
> 
> What is the alternative?

If, on the completed execution of the registered primary handler, you
can ensure that the device is no longer asserting an interrupt to the
connected irq chip, then the IRQF_ONESHOT isn't necessary, because it's
safe for the irq core to unmask the interrupt after the primary handler
runs.

It appears that it might be able to make this guarantee, if that's what
sti_mbox_disable_channel() is doing.

> NB: What does 'acquiesce' mean in this context?  Is that a typo?

I mean 'acquiesce' to mean what I mention before: prevent the device
from asserting the interrupt.  Perhaps it's a uncommon use of the word.

> > > Also, as written there are num_inst reads of STI_IRQ_VAL_OFFSET in the
> > > primary handler, which seems inefficient...(unless of course reading
> > > incurs side effects, here).
>
> Inefficient in what respect?

I've since looked again and realized that the register base is
recalculated based on 'instance', so disregard this comment.

   Julia

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

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Thomas Gleixner]

On Wed, 22 Mar 2017, Julia Cartwright wrote:
> On Wed, Mar 22, 2017 at 01:30:12PM -0500, Grygorii Strashko wrote:
> > It will not be threaded because there are IRQF_ONESHOT used.
> > 
> > 	ret = devm_request_threaded_irq(&pdev->dev, irq,
> > 					sti_mbox_irq_handler,
> > 					sti_mbox_thread_handler,
> > 					IRQF_ONESHOT, mdev->name, mdev);
> 
> Indeed.  I had skipped over this important detail when I was skimming
> through the code.
> 
> Thanks for clarifying!
> 
> Is IRQF_ONESHOT really necessary for this device?  The primary handler
> invokes sti_mbox_disable_channel() on the interrupting channel, which I
> would hope would acquiesce the pending interrupt at the device-level?

Well, this is a typical case of well done split into primary and threaded
handler. There is no advantage on RT to force this into double threaded
handling. Making the lock raw is definitely the proper solution here.

Thanks,

	tglx

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Lee Jones]

On Thu, 23 Mar 2017, Julia Cartwright wrote:

> On Thu, Mar 23, 2017 at 10:26:49AM +0000, Lee Jones wrote:
> > On Thu, 23 Mar 2017, Lionel DEBIEVE wrote:
> > 
> > > On 03/22/2017 07:47 PM, Julia Cartwright wrote:
> > > > On Wed, Mar 22, 2017 at 01:30:12PM -0500, Grygorii Strashko wrote:
> > > >> On 03/22/2017 01:01 PM, Steven Rostedt wrote:
> > > >>> On Wed, 22 Mar 2017 12:37:59 -0500
> > > >>> Julia Cartwright <julia@ni.com> wrote:
> > > >>>
> > > >>>> Which kernel were you testing on, here?  From what I can tell, this
> > > >>>> should have been fixed with Thomas's commit:
> > > >>>>
> > > >>>>     2a1d3ab8986d ("genirq: Handle force threading of irqs with primary
> > > >>>> and thread handler")
> > > >>> Thanks Julia for looking into this. I just looked at the code, and saw
> > > >>> that it does very little with the lock held, and was fine with the
> > > >>> conversion. But if that interrupt handler should be in a thread, we
> > > >>> should see if that's the issue first.
> > > >>
> > > >> It will not be threaded because there are IRQF_ONESHOT used.
> > > >>
> > > >> 	ret = devm_request_threaded_irq(&pdev->dev, irq,
> > > >> 					sti_mbox_irq_handler,
> > > >> 					sti_mbox_thread_handler,
> > > >> 					IRQF_ONESHOT, mdev->name, mdev);
> > > > Indeed.  I had skipped over this important detail when I was skimming
> > > > through the code.
> > > >
> > > > Thanks for clarifying!
> > > >
> > > > Is IRQF_ONESHOT really necessary for this device?  The primary handler
> > > > invokes sti_mbox_disable_channel() on the interrupting channel, which I
> > > > would hope would acquiesce the pending interrupt at the device-level?
> > 
> > Not sure.  This part of the code is remanent from when I re-wrote it.
> > 
> > What is the alternative?
> 
> If, on the completed execution of the registered primary handler, you
> can ensure that the device is no longer asserting an interrupt to the
> connected irq chip, then the IRQF_ONESHOT isn't necessary, because it's
> safe for the irq core to unmask the interrupt after the primary handler
> runs.
> 
> It appears that it might be able to make this guarantee, if that's what
> sti_mbox_disable_channel() is doing.

Yes, I'm inclined to agree.

> > NB: What does 'acquiesce' mean in this context?  Is that a typo?
> 
> I mean 'acquiesce' to mean what I mention before: prevent the device
> from asserting the interrupt.  Perhaps it's a uncommon use of the word.

http://www.thesaurus.com/browse/acquiesce

Perhaps 'suppress' or 'quell' would better suit the situation.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Sebastian Andrzej Siewior]

On 2017-03-22 09:05:58 [-0700], Steven Rostedt wrote:
> On Wed, 22 Mar 2017 16:18:43 +0100
> Lionel Debieve <lionel.debieve@st.com> wrote:
> 
> > Use raw_spin_lock in enable/disable channel as it comes from
> > interrupt context.
> > 
> > BUG: sleeping function called from invalid context at
> > kernel/locking/rtmutex.c:995
> > in_atomic(): 1, irqs_disabled(): 128, pid: 307, name: pulseaudio
> > Preemption disabled at:
> > [<c01790fc>] __handle_domain_irq+0x4c/0xec
> > CPU: 0 PID: 307 Comm: pulseaudio
> > Hardware name: STi SoC with Flattened Device Tree
> > [<c011046c>] (unwind_backtrace)
> > [<c010c7f4>] (show_stack)
> > [<c03d1578>] (dump_stack)
> > [<c014e440>] (___might_sleep)
> > [<c08e7f24>] (rt_spin_lock)
> > [<c069bb04>] (sti_mbox_disable_channel)
> > [<c069befc>] (sti_mbox_irq_handler)
> > [<c0179900>] (__handle_irq_event_percpu)
> > [<c01799dc>] (handle_irq_event_percpu)
> > [<c0179a78>] (handle_irq_event)
> > [<c017d1c8>] (handle_fasteoi_irq)
> > [<c0178c08>] (generic_handle_irq)
> > [<c017912c>] (__handle_domain_irq)
> > [<c0101488>] (gic_handle_irq)
> > 
> > Signed-off-by: Lionel Debieve <lionel.debieve@st.com>
> 
> Looks fine to me. Should this go to mainline?
> 
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Could this be applied upstream, please? From looking at the thread there
was no reason not to do so.

> -- Steve
> 
> > ---
> >  drivers/mailbox/mailbox-sti.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/mailbox/mailbox-sti.c
> > b/drivers/mailbox/mailbox-sti.c index 41bcd33..f9674ca 100644
> > --- a/drivers/mailbox/mailbox-sti.c
> > +++ b/drivers/mailbox/mailbox-sti.c
> > @@ -60,7 +60,7 @@ struct sti_mbox_device {
> >  	void __iomem		*base;
> >  	const char		*name;
> >  	u32			enabled[STI_MBOX_INST_MAX];
> > -	spinlock_t		lock;
> > +	raw_spinlock_t		lock;
> >  };
> >  
> >  /**
> > @@ -129,10 +129,10 @@ static void sti_mbox_enable_channel(struct
> > mbox_chan *chan) unsigned long flags;
> >  	void __iomem *base = MBOX_BASE(mdev, instance);
> >  
> > -	spin_lock_irqsave(&mdev->lock, flags);
> > +	raw_spin_lock_irqsave(&mdev->lock, flags);
> >  	mdev->enabled[instance] |= BIT(channel);
> >  	writel_relaxed(BIT(channel), base + STI_ENA_SET_OFFSET);
> > -	spin_unlock_irqrestore(&mdev->lock, flags);
> > +	raw_spin_unlock_irqrestore(&mdev->lock, flags);
> >  }
> >  
> >  static void sti_mbox_disable_channel(struct mbox_chan *chan)
> > @@ -144,10 +144,10 @@ static void sti_mbox_disable_channel(struct
> > mbox_chan *chan) unsigned long flags;
> >  	void __iomem *base = MBOX_BASE(mdev, instance);
> >  
> > -	spin_lock_irqsave(&mdev->lock, flags);
> > +	raw_spin_lock_irqsave(&mdev->lock, flags);
> >  	mdev->enabled[instance] &= ~BIT(channel);
> >  	writel_relaxed(BIT(channel), base + STI_ENA_CLR_OFFSET);
> > -	spin_unlock_irqrestore(&mdev->lock, flags);
> > +	raw_spin_unlock_irqrestore(&mdev->lock, flags);
> >  }
> >  
> >  static void sti_mbox_clear_irq(struct mbox_chan *chan)
> > @@ -450,7 +450,7 @@ static int sti_mbox_probe(struct platform_device
> > *pdev) mdev->dev		= &pdev->dev;
> >  	mdev->mbox		= mbox;
> >  
> > -	spin_lock_init(&mdev->lock);
> > +	raw_spin_lock_init(&mdev->lock);
> >  
> >  	/* STi Mailbox does not have a Tx-Done or Tx-Ready IRQ */
> >  	mbox->txdone_irq	= false;

Sebastian

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Lee Jones]

On Wed, 29 Mar 2017, Sebastian Andrzej Siewior wrote:

> On 2017-03-22 09:05:58 [-0700], Steven Rostedt wrote:
> > On Wed, 22 Mar 2017 16:18:43 +0100
> > Lionel Debieve <lionel.debieve@st.com> wrote:
> > 
> > > Use raw_spin_lock in enable/disable channel as it comes from
> > > interrupt context.
> > > 
> > > BUG: sleeping function called from invalid context at
> > > kernel/locking/rtmutex.c:995
> > > in_atomic(): 1, irqs_disabled(): 128, pid: 307, name: pulseaudio
> > > Preemption disabled at:
> > > [<c01790fc>] __handle_domain_irq+0x4c/0xec
> > > CPU: 0 PID: 307 Comm: pulseaudio
> > > Hardware name: STi SoC with Flattened Device Tree
> > > [<c011046c>] (unwind_backtrace)
> > > [<c010c7f4>] (show_stack)
> > > [<c03d1578>] (dump_stack)
> > > [<c014e440>] (___might_sleep)
> > > [<c08e7f24>] (rt_spin_lock)
> > > [<c069bb04>] (sti_mbox_disable_channel)
> > > [<c069befc>] (sti_mbox_irq_handler)
> > > [<c0179900>] (__handle_irq_event_percpu)
> > > [<c01799dc>] (handle_irq_event_percpu)
> > > [<c0179a78>] (handle_irq_event)
> > > [<c017d1c8>] (handle_fasteoi_irq)
> > > [<c0178c08>] (generic_handle_irq)
> > > [<c017912c>] (__handle_domain_irq)
> > > [<c0101488>] (gic_handle_irq)
> > > 
> > > Signed-off-by: Lionel Debieve <lionel.debieve@st.com>
> > 
> > Looks fine to me. Should this go to mainline?
> > 
> > Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> Could this be applied upstream, please? From looking at the thread there
> was no reason not to do so.

Acked-by: Lee Jones <lee.jones@linaro.org>

> > > ---
> > >  drivers/mailbox/mailbox-sti.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/mailbox/mailbox-sti.c
> > > b/drivers/mailbox/mailbox-sti.c index 41bcd33..f9674ca 100644
> > > --- a/drivers/mailbox/mailbox-sti.c
> > > +++ b/drivers/mailbox/mailbox-sti.c
> > > @@ -60,7 +60,7 @@ struct sti_mbox_device {
> > >  	void __iomem		*base;
> > >  	const char		*name;
> > >  	u32			enabled[STI_MBOX_INST_MAX];
> > > -	spinlock_t		lock;
> > > +	raw_spinlock_t		lock;
> > >  };
> > >  
> > >  /**
> > > @@ -129,10 +129,10 @@ static void sti_mbox_enable_channel(struct
> > > mbox_chan *chan) unsigned long flags;
> > >  	void __iomem *base = MBOX_BASE(mdev, instance);
> > >  
> > > -	spin_lock_irqsave(&mdev->lock, flags);
> > > +	raw_spin_lock_irqsave(&mdev->lock, flags);
> > >  	mdev->enabled[instance] |= BIT(channel);
> > >  	writel_relaxed(BIT(channel), base + STI_ENA_SET_OFFSET);
> > > -	spin_unlock_irqrestore(&mdev->lock, flags);
> > > +	raw_spin_unlock_irqrestore(&mdev->lock, flags);
> > >  }
> > >  
> > >  static void sti_mbox_disable_channel(struct mbox_chan *chan)
> > > @@ -144,10 +144,10 @@ static void sti_mbox_disable_channel(struct
> > > mbox_chan *chan) unsigned long flags;
> > >  	void __iomem *base = MBOX_BASE(mdev, instance);
> > >  
> > > -	spin_lock_irqsave(&mdev->lock, flags);
> > > +	raw_spin_lock_irqsave(&mdev->lock, flags);
> > >  	mdev->enabled[instance] &= ~BIT(channel);
> > >  	writel_relaxed(BIT(channel), base + STI_ENA_CLR_OFFSET);
> > > -	spin_unlock_irqrestore(&mdev->lock, flags);
> > > +	raw_spin_unlock_irqrestore(&mdev->lock, flags);
> > >  }
> > >  
> > >  static void sti_mbox_clear_irq(struct mbox_chan *chan)
> > > @@ -450,7 +450,7 @@ static int sti_mbox_probe(struct platform_device
> > > *pdev) mdev->dev		= &pdev->dev;
> > >  	mdev->mbox		= mbox;
> > >  
> > > -	spin_lock_init(&mdev->lock);
> > > +	raw_spin_lock_init(&mdev->lock);
> > >  
> > >  	/* STi Mailbox does not have a Tx-Done or Tx-Ready IRQ */
> > >  	mbox->txdone_irq	= false;
> 
> Sebastian

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RT 1/1] remoteproc: Prevent schedule while atomic

[Lionel DEBIEVE]

Hi,

Looking at the thread discussion, except architecture discussion around the IRQF_ONESHOT, I think it could go to upstream too.

I'll re-upload patch for upstream.

Thanks for reviewing.

BR,

Lionel


On 03/30/2017 09:54 AM, Lee Jones wrote:
> On Wed, 29 Mar 2017, Sebastian Andrzej Siewior wrote:
>
>> On 2017-03-22 09:05:58 [-0700], Steven Rostedt wrote:
>>> On Wed, 22 Mar 2017 16:18:43 +0100
>>> Lionel Debieve <lionel.debieve@st.com> wrote:
>>>
>>>> Use raw_spin_lock in enable/disable channel as it comes from
>>>> interrupt context.
>>>>
>>>> BUG: sleeping function called from invalid context at
>>>> kernel/locking/rtmutex.c:995
>>>> in_atomic(): 1, irqs_disabled(): 128, pid: 307, name: pulseaudio
>>>> Preemption disabled at:
>>>> [<c01790fc>] __handle_domain_irq+0x4c/0xec
>>>> CPU: 0 PID: 307 Comm: pulseaudio
>>>> Hardware name: STi SoC with Flattened Device Tree
>>>> [<c011046c>] (unwind_backtrace)
>>>> [<c010c7f4>] (show_stack)
>>>> [<c03d1578>] (dump_stack)
>>>> [<c014e440>] (___might_sleep)
>>>> [<c08e7f24>] (rt_spin_lock)
>>>> [<c069bb04>] (sti_mbox_disable_channel)
>>>> [<c069befc>] (sti_mbox_irq_handler)
>>>> [<c0179900>] (__handle_irq_event_percpu)
>>>> [<c01799dc>] (handle_irq_event_percpu)
>>>> [<c0179a78>] (handle_irq_event)
>>>> [<c017d1c8>] (handle_fasteoi_irq)
>>>> [<c0178c08>] (generic_handle_irq)
>>>> [<c017912c>] (__handle_domain_irq)
>>>> [<c0101488>] (gic_handle_irq)
>>>>
>>>> Signed-off-by: Lionel Debieve <lionel.debieve@st.com>
>>> Looks fine to me. Should this go to mainline?
>>>
>>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>> Could this be applied upstream, please? From looking at the thread there
>> was no reason not to do so.
> Acked-by: Lee Jones <lee.jones@linaro.org>
>
>>>> ---
>>>>   drivers/mailbox/mailbox-sti.c | 12 ++++++------
>>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/mailbox/mailbox-sti.c
>>>> b/drivers/mailbox/mailbox-sti.c index 41bcd33..f9674ca 100644
>>>> --- a/drivers/mailbox/mailbox-sti.c
>>>> +++ b/drivers/mailbox/mailbox-sti.c
>>>> @@ -60,7 +60,7 @@ struct sti_mbox_device {
>>>>   	void __iomem		*base;
>>>>   	const char		*name;
>>>>   	u32			enabled[STI_MBOX_INST_MAX];
>>>> -	spinlock_t		lock;
>>>> +	raw_spinlock_t		lock;
>>>>   };
>>>>   
>>>>   /**
>>>> @@ -129,10 +129,10 @@ static void sti_mbox_enable_channel(struct
>>>> mbox_chan *chan) unsigned long flags;
>>>>   	void __iomem *base = MBOX_BASE(mdev, instance);
>>>>   
>>>> -	spin_lock_irqsave(&mdev->lock, flags);
>>>> +	raw_spin_lock_irqsave(&mdev->lock, flags);
>>>>   	mdev->enabled[instance] |= BIT(channel);
>>>>   	writel_relaxed(BIT(channel), base + STI_ENA_SET_OFFSET);
>>>> -	spin_unlock_irqrestore(&mdev->lock, flags);
>>>> +	raw_spin_unlock_irqrestore(&mdev->lock, flags);
>>>>   }
>>>>   
>>>>   static void sti_mbox_disable_channel(struct mbox_chan *chan)
>>>> @@ -144,10 +144,10 @@ static void sti_mbox_disable_channel(struct
>>>> mbox_chan *chan) unsigned long flags;
>>>>   	void __iomem *base = MBOX_BASE(mdev, instance);
>>>>   
>>>> -	spin_lock_irqsave(&mdev->lock, flags);
>>>> +	raw_spin_lock_irqsave(&mdev->lock, flags);
>>>>   	mdev->enabled[instance] &= ~BIT(channel);
>>>>   	writel_relaxed(BIT(channel), base + STI_ENA_CLR_OFFSET);
>>>> -	spin_unlock_irqrestore(&mdev->lock, flags);
>>>> +	raw_spin_unlock_irqrestore(&mdev->lock, flags);
>>>>   }
>>>>   
>>>>   static void sti_mbox_clear_irq(struct mbox_chan *chan)
>>>> @@ -450,7 +450,7 @@ static int sti_mbox_probe(struct platform_device
>>>> *pdev) mdev->dev		= &pdev->dev;
>>>>   	mdev->mbox		= mbox;
>>>>   
>>>> -	spin_lock_init(&mdev->lock);
>>>> +	raw_spin_lock_init(&mdev->lock);
>>>>   
>>>>   	/* STi Mailbox does not have a Tx-Done or Tx-Ready IRQ */
>>>>   	mbox->txdone_irq	= false;
>> Sebastian