Protection of critical section in PREEMPT_RT

Protection of critical section in PREEMPT_RT

[Jacky Lam]

Hello all,

It is my first time to use PREEMPT_RT patch. I have a kernel driver
needed to do a indirect register access to a hardware interface. In
past, I use spinlock to protect those code. After turning on
PREEMPT_RT, the driver doesn't work anymore.

I know spinlock is now preemptible, but how can I protect a "really"
critical section in PREEMPT_RT? If it is a bad design, could give me
some tutorial/readings on how to modify my driver?

Thanks very much.

Yours,
Jacky

Re: Protection of critical section in PREEMPT_RT

[Stanislav Meduna]

On 22.01.2013 04:29, Jacky Lam wrote:

> I have a kernel driver needed to do a indirect register access
> to a hardware interface. In past, I use spinlock to protect
> those code. After turning on PREEMPT_RT, the driver doesn't
> work anymore.

I'm not an expert but AFAIK the spinlock still does the protection,
it just can be preempted. If your driver does not work, there
is either a timing issue as well (i.e. you have time constraints
that are not met when the spinlock is preempted) or there is
a bug that is triggered by the preemption (such as unprotected
access in the code that preempts your spinlock).

> I know spinlock is now preemptible, but how can I protect a "really"
> critical section in PREEMPT_RT? If it is a bad design, could give me
> some tutorial/readings on how to modify my driver?

https://rt.wiki.kernel.org/index.php/RT_PREEMPT_HOWTO

says

Critical sections protected by i.e. spinlock_t and rwlock_t are now
preemptible. The creation of non-preemptible sections (in kernel) is
still possible with raw_spinlock_t (same APIs like spinlock_t)

However, try to identify the real culprit first.

--
                                          Stano

Re: Protection of critical section in PREEMPT_RT

[Jacky Lam]

Thanks Stano,

I am still debugging this driver. But I find very strange that
sometimes printk() in interrupt handler do not output anything. This
makes me very difficult to debug. Is it RT patch's expected behaviour?
Or I am doing something wrong??

Jackky

Re: Protection of critical section in PREEMPT_RT

[Frank Rowand]

On 01/31/13 01:27, Jacky Lam wrote:
> Thanks Stano,
> 
> I am still debugging this driver. But I find very strange that
> sometimes printk() in interrupt handler do not output anything. This
> makes me very difficult to debug. Is it RT patch's expected behaviour?
> Or I am doing something wrong??

If CONFIG_PREEMPT_RT_FULL then printk() from irq context will be buffered
but the console driver will not be called to flush the buffer.  The
next printk() outside irq context will flush the buffer.

-Frank

Re: Protection of critical section in PREEMPT_RT

[Jacky Lam]

Thanks.

I finally find that if I disable interrupt during ata_qc_issue() in
libata-scsi.c, my sata driver works without any problem.

But I find USB EHCI driver is more complicated. Kernel crashes every
where when RT is turned on. It seems whenever the stack  spin_lock
ehci->lock and interrupt comes at that moment, kernel will be crashes.
The reason is EHCI interrupt handler will also take the same lock. If
my point of view, USB stack seems no PREEMPT_RT safe. Is it correct?
Am I miss anything?


On Fri, Feb 1, 2013 at 3:52 AM, Frank Rowand <frank.rowand@am.sony.com> wrote:
> On 01/31/13 01:27, Jacky Lam wrote:
>> Thanks Stano,
>>
>> I am still debugging this driver. But I find very strange that
>> sometimes printk() in interrupt handler do not output anything. This
>> makes me very difficult to debug. Is it RT patch's expected behaviour?
>> Or I am doing something wrong??
>
> If CONFIG_PREEMPT_RT_FULL then printk() from irq context will be buffered
> but the console driver will not be called to flush the buffer.  The
> next printk() outside irq context will flush the buffer.
>
> -Frank


On Fri, Feb 1, 2013 at 3:52 AM, Frank Rowand <frank.rowand@am.sony.com> wrote:
> On 01/31/13 01:27, Jacky Lam wrote:
>> Thanks Stano,
>>
>> I am still debugging this driver. But I find very strange that
>> sometimes printk() in interrupt handler do not output anything. This
>> makes me very difficult to debug. Is it RT patch's expected behaviour?
>> Or I am doing something wrong??
>
> If CONFIG_PREEMPT_RT_FULL then printk() from irq context will be buffered
> but the console driver will not be called to flush the buffer.  The
> next printk() outside irq context will flush the buffer.
>
> -Frank

Re: Protection of critical section in PREEMPT_RT

[Thomas Gleixner]

Jacky,

On Mon, 4 Feb 2013, Jacky Lam wrote:

> Thanks.

Please stop top posting!
 
> I finally find that if I disable interrupt during ata_qc_issue() in
> libata-scsi.c, my sata driver works without any problem.

This is wrong. Which driver are you taling about ?
 
> But I find USB EHCI driver is more complicated. Kernel crashes every
> where when RT is turned on. It seems whenever the stack  spin_lock
> ehci->lock and interrupt comes at that moment, kernel will be crashes.
> The reason is EHCI interrupt handler will also take the same lock. If
> my point of view, USB stack seems no PREEMPT_RT safe. Is it correct?
> Am I miss anything?

Probably. First of all you don't tell us which driver you are
using. And w/o seing the code and the output of the kernel crash, we
can't help you at all.

Thanks,

	tglx

Re: Protection of critical section in PREEMPT_RT

[Jacky Lam]

Thomas,

On Mon, Feb 4, 2013 at 6:40 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Jacky,
>
> On Mon, 4 Feb 2013, Jacky Lam wrote:
>
>> Thanks.
>
> Please stop top posting!
>

Thanks for reminding.

>> I finally find that if I disable interrupt during ata_qc_issue() in
>> libata-scsi.c, my sata driver works without any problem.
>
> This is wrong. Which driver are you taling about ?
>
>> But I find USB EHCI driver is more complicated. Kernel crashes every
>> where when RT is turned on. It seems whenever the stack  spin_lock
>> ehci->lock and interrupt comes at that moment, kernel will be crashes.
>> The reason is EHCI interrupt handler will also take the same lock. If
>> my point of view, USB stack seems no PREEMPT_RT safe. Is it correct?
>> Am I miss anything?
>
> Probably. First of all you don't tell us which driver you are
> using. And w/o seing the code and the output of the kernel crash, we
> can't help you at all.
>

I am sorry for that. But it is a driver for our in-house hardware. It
seems meaningless even I post the source here. Please tell me if it
did help. From your statement above, it seems PREEMPT_RT patch do not
work with USB stack. Is it true? I am happy to post the fix here if I
can fix that.

While I am porting my drivers to PREEMPT_RT patched kernel, I find a
common problem. In stock kernel, whenever I need to touch some data
structures in driver code and also in interrupt handler, we can
protect them by spin_lock_irqsave(). But in PREEMT_RT kernel, while
the driver code touching the data structures, interrupt can still come
in. If I don't protect the data structure by spinlcok, the data
structures will corrupt. If I put it, rt_spin_lock function will cause
a kernel dump. Of course, I can use raw_spin_lock_irqsave() instead.
But for the case like USB stack and SATA stack, if I disable the
interrupt just like what I did for ata_qc_issue(). It seems not very
good idea.

Is there any rule to resolve such conflict?

> Thanks,
>
>         tglx

Re: Protection of critical section in PREEMPT_RT

[Thomas Gleixner]

Jacky,

On Tue, 5 Feb 2013, Jacky Lam wrote:
> On Mon, Feb 4, 2013 at 6:40 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > Probably. First of all you don't tell us which driver you are
> > using. And w/o seing the code and the output of the kernel crash, we
> > can't help you at all.
> >
> 
> I am sorry for that. But it is a driver for our in-house hardware. It
> seems meaningless even I post the source here. Please tell me if it
> did help. 

Sure does posting the code help. W/o seing the code and the crash we
have no idea what your driver is doing and why you think you need
special code constructs.

> From your statement above, it seems PREEMPT_RT patch do not work
> with USB stack. Is it true?

How do you read that into what I wrote ?
 
> I am happy to post the fix here if I can fix that.

There is nothing to fix. The USB stack works perfectly fine on RT at
least with those drivers which are written correctly.

> While I am porting my drivers to PREEMPT_RT patched kernel, I find a
> common problem. In stock kernel, whenever I need to touch some data
> structures in driver code and also in interrupt handler, we can
> protect them by spin_lock_irqsave(). But in PREEMT_RT kernel, while
> the driver code touching the data structures, interrupt can still come
> in.

And how does that matter? The interrupt comes in and wakes the
interrupt thread. The interrupt thread either preempts your code or it
does not. If if preempts then it will contend on the spin lock and go
to sleep until the code which holds the lock releases it. So what's
the problem with using spin_lock_irqsave() here and let RT substitute
it with a "sleeping" PI lock?

> If I don't protect the data structure by spinlcok, the data
> structures will corrupt. If I put it, rt_spin_lock function will cause
> a kernel dump.

And how about posting that kernel dump, so we have at least an idea
what's going on?

> Of course, I can use raw_spin_lock_irqsave() instead.

You can, but this will cause other failures.

> But for the case like USB stack and SATA stack, if I disable the
> interrupt just like what I did for ata_qc_issue(). It seems not very
> good idea.

Definitely not. I told you already that disabling interrupts around
ata_qc_issue() is completely wrong. Why do you insist on your claims
that ATA and USB is broken on RT? It's not broken. Your drivers are
doing something really wrong.
  
> Is there any rule to resolve such conflict?

Sure. Run your driver on a non-RT patched kernel, turn on the
following debug options:

 - CONFIG_DEBUG_RT_MUTEXES
 - CONFIG_DEBUG_SPINLOCK
 - CONFIG_DEBUG_MUTEXES
 - CONFIG_DEBUG_LOCK_ALLOC
 - CONFIG_PROVE_LOCKING
 - CONFIG_DEBUG_SPINLOCK_SLEEP
 - CONFIG_DEBUG_PREEMPT

If these debug mechanisms cannot find an issue with your driver, then
it should work out of the box with RT. If they find an issue, then you
better fix it before even thinking about using RT.

If all issues are resolved, move over to RT and enable the same debug
options. Run your code and if it still explodes, post code and kernel
dump so we can have a look instead of having this completely pointless
discussion over and over.

Thanks,

	tglx

Re: Protection of critical section in PREEMPT_RT

[Jacky Lam]

On Tue, Feb 5, 2013 at 6:05 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Jacky,
>
> On Tue, 5 Feb 2013, Jacky Lam wrote:
>> On Mon, Feb 4, 2013 at 6:40 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > Probably. First of all you don't tell us which driver you are
>> > using. And w/o seing the code and the output of the kernel crash, we
>> > can't help you at all.
>> >
>>
>> I am sorry for that. But it is a driver for our in-house hardware. It
>> seems meaningless even I post the source here. Please tell me if it
>> did help.
>
> Sure does posting the code help. W/o seing the code and the crash we
> have no idea what your driver is doing and why you think you need
> special code constructs.
>
>> From your statement above, it seems PREEMPT_RT patch do not work
>> with USB stack. Is it true?
>
> How do you read that into what I wrote ?
>
>> I am happy to post the fix here if I can fix that.
>
> There is nothing to fix. The USB stack works perfectly fine on RT at
> least with those drivers which are written correctly.
>
>> While I am porting my drivers to PREEMPT_RT patched kernel, I find a
>> common problem. In stock kernel, whenever I need to touch some data
>> structures in driver code and also in interrupt handler, we can
>> protect them by spin_lock_irqsave(). But in PREEMT_RT kernel, while
>> the driver code touching the data structures, interrupt can still come
>> in.
>
> And how does that matter? The interrupt comes in and wakes the
> interrupt thread. The interrupt thread either preempts your code or it
> does not. If if preempts then it will contend on the spin lock and go
> to sleep until the code which holds the lock releases it. So what's
> the problem with using spin_lock_irqsave() here and let RT substitute
> it with a "sleeping" PI lock?
>
>> If I don't protect the data structure by spinlcok, the data
>> structures will corrupt. If I put it, rt_spin_lock function will cause
>> a kernel dump.
>
> And how about posting that kernel dump, so we have at least an idea
> what's going on?
>
>> Of course, I can use raw_spin_lock_irqsave() instead.
>
> You can, but this will cause other failures.
>
>> But for the case like USB stack and SATA stack, if I disable the
>> interrupt just like what I did for ata_qc_issue(). It seems not very
>> good idea.
>
> Definitely not. I told you already that disabling interrupts around
> ata_qc_issue() is completely wrong. Why do you insist on your claims
> that ATA and USB is broken on RT? It's not broken. Your drivers are
> doing something really wrong.
>
>> Is there any rule to resolve such conflict?
>
> Sure. Run your driver on a non-RT patched kernel, turn on the
> following debug options:
>
>  - CONFIG_DEBUG_RT_MUTEXES
>  - CONFIG_DEBUG_SPINLOCK
>  - CONFIG_DEBUG_MUTEXES
>  - CONFIG_DEBUG_LOCK_ALLOC
>  - CONFIG_PROVE_LOCKING
>  - CONFIG_DEBUG_SPINLOCK_SLEEP
>  - CONFIG_DEBUG_PREEMPT
>
> If these debug mechanisms cannot find an issue with your driver, then
> it should work out of the box with RT. If they find an issue, then you
> better fix it before even thinking about using RT.
>
> If all issues are resolved, move over to RT and enable the same debug
> options. Run your code and if it still explodes, post code and kernel
> dump so we can have a look instead of having this completely pointless
> discussion over and over.
>

I try to make an example to show the problem. Our hardware  use
indirect method to access the SATA/USB controller internal registers,
i.e CPU has a SATA/USB IO Addr register and a DATA register. In order
to read from controller's internal register, I need to put the offset
into IO Addr register and then read from the IO Data register. As this
process cannot be interrupted, I need to put a spin_lock_irqsave to
protect the code.

When driver code is reading the controller's registers and interrupt
comes, interrupt handler (not the interrupt thread) need to clear the
interrupt source first and return IRQ_WAKE_THREAD to wait for
interrupt thread to actually handle the interrupt task. But in order
to clear the interrupt source, I need to access the controller's
internal registers. This is the problem I have now.

Is there anything I misunderstand? Or the hardware design does not
allow me to use RT?

I play with Linux kernel for long time, but it is my first time to
play with real-time stuff. Maybe I have some basic concept wrong.
Please don't hesitate to point it out. Thanks very much.

> Thanks,
>
>         tglx
>
>

Re: Protection of critical section in PREEMPT_RT

[Stanislav Meduna]

On 06.02.2013 02:59, Jacky Lam wrote:

> When driver code is reading the controller's registers and interrupt
> comes, interrupt handler (not the interrupt thread) need to clear the
> interrupt source first and return IRQ_WAKE_THREAD to wait for
> interrupt thread to actually handle the interrupt task. But in order
> to clear the interrupt source, I need to access the controller's
> internal registers. This is the problem I have now.

Maybe I am unterstanding this wrong, but shouldn't you unmask the
interrupt source when you are _done_ with processing the interrupt?

I have no real experience with interrupt driven drivers in RT-Linux,
but in other RTOSs I worked with this was the way to go -
the interrupt handler clears the source and schedules
the service thread but does not unmask the interrupt in question.
This is first done when the interrupt is processed.

That way you cannot be interrupted by the source you are
already processing. However, even that should IMHO work if the
locking is correct. Things can get complicated if you are
sharing interrupts between devices or if one device is accessed
with more drivers.

Do as Thomas is suggesting - turn all the CONFIG_DEBUG* on and
try with a non-RT kernel.

Regards
-- 
                                            Stano

Re: Protection of critical section in PREEMPT_RT

[Thomas Gleixner]

Jacky,

On Wed, 6 Feb 2013, Jacky Lam wrote:
> On Tue, Feb 5, 2013 at 6:05 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > If all issues are resolved, move over to RT and enable the same debug
> > options. Run your code and if it still explodes, post code and kernel
> > dump so we can have a look instead of having this completely pointless
> > discussion over and over.
> >
> 
> I try to make an example to show the problem. Our hardware  use
> indirect method to access the SATA/USB controller internal registers,
> i.e CPU has a SATA/USB IO Addr register and a DATA register. In order
> to read from controller's internal register, I need to put the offset
> into IO Addr register and then read from the IO Data register. As this
> process cannot be interrupted, I need to put a spin_lock_irqsave to
> protect the code.

Right. That's the usual way to solve this.
 
> When driver code is reading the controller's registers and interrupt
> comes, interrupt handler (not the interrupt thread) need to clear the
> interrupt source first and return IRQ_WAKE_THREAD to wait for
> interrupt thread to actually handle the interrupt task. But in order
> to clear the interrupt source, I need to access the controller's
> internal registers. This is the problem I have now.

Do I understand you correct, that you use a threaded interrupt handler
already in the driver?

If yes, then RT is of course not force threading your primary
handler. In that case the protection of the indirect HW access must
use a raw_spinlock not a spinlock.

Thanks,

	tglx