Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello All,

> All works now for me with preempt-rt. The problem is using hrtimer.
> I think that hrtimer are executed with interrupts disabled so, if
> this happen when I must receive a char, i have an overrun.

No, they share the same interrupt line...
So, while the timer interrupt handler is running, the serial handler
has to wait until the timer interrupt handler has finished.
Notice that the HRT interrupt handler is quite heavy and takes a long
time to complete.

And, as I already mentioned, related to the 1 byte FIFO and a
interrupt latency of about 85us (without HRT), it is logical that you
can get an overrun at the higher serial speeds... (>=115200bps)

> The only solution was the dma support to serial device.

Or, use flow control?


Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello All,

 All works now for me with preempt-rt. The problem is using hrtimer.
 I think that hrtimer are executed with interrupts disabled so, if
 this happen when I must receive a char, i have an overrun.

No, they share the same interrupt line...
So, while the timer interrupt handler is running, the serial handler
has to wait until the timer interrupt handler has finished.
Notice that the HRT interrupt handler is quite heavy and takes a long
time to complete.

And, as I already mentioned, related to the 1 byte FIFO and a
interrupt latency of about 85us (without HRT), it is logical that you
can get an overrun at the higher serial speeds... (=115200bps)

 The only solution was the dma support to serial device.

Or, use flow control?


Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Michael,

> the serial driver works fine. The problem seems to be related to the
> tclib, when I use it as a clocksource.

tclib as a clocksource should be no problem on Preempt-RT, do not use
it as clockevent device, it will not work properly on preempt-rt on
at91* yet, especially with the NO_HZ config.

> The numbers of overruns depends on the type of
> files too. It is possible?

I am still very curious to your (RT-) thread-prio layout. The problems
you mention can also be caused by a weird configuration of these
threads on preempt-rt.

Remy


2008/2/6, michael <[EMAIL PROTECTED]>:
> Hi,
>
> the serial driver works fine. The problem seems to be related to the
> tclib, when I use
> it as a clocksource. The numbers of overruns depends on the type of
> files too.
> It is possible?
>
> Regards
> Michael
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [EMAIL PROTECTED]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Michael,

 the serial driver works fine. The problem seems to be related to the
 tclib, when I use it as a clocksource.

tclib as a clocksource should be no problem on Preempt-RT, do not use
it as clockevent device, it will not work properly on preempt-rt on
at91* yet, especially with the NO_HZ config.

 The numbers of overruns depends on the type of
 files too. It is possible?

I am still very curious to your (RT-) thread-prio layout. The problems
you mention can also be caused by a weird configuration of these
threads on preempt-rt.

Remy


2008/2/6, michael [EMAIL PROTECTED]:
 Hi,

 the serial driver works fine. The problem seems to be related to the
 tclib, when I use
 it as a clocksource. The numbers of overruns depends on the type of
 files too.
 It is possible?

 Regards
 Michael


 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to [EMAIL PROTECTED]
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

> That's what I was thinking too. If this is indeed the cause, the
> dev_err() added by the debug patch I posted should trigger and we may
> consider boosting the priority of the tasklet (using
> tasklet_hi_schedule.)

Notice that we are talking about Preempt-RT here. Everything is
running in thread context, even tasklets, softirqs etc. They are _all_
preemptible, and if Michael has some RT-thread in the system that has
a higher priority than this tasklet or softirq, than the buffer will
eventually overflow.
I wonder also if Michael has set the RT-priorities correctly, on RT
_every_ softirq/irq thread starts by default on priority 50,
SCHED_FIFO. If they are still at 50, any other softirq/tasklet, or irq
can make this behavior worse.
Notice that the default 50 thingy rarely gives a decent behaving
system. (But any other default will also give problems anyway, so it
_has_ to be customized/tuned by the end user)

Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

 That's what I was thinking too. If this is indeed the cause, the
 dev_err() added by the debug patch I posted should trigger and we may
 consider boosting the priority of the tasklet (using
 tasklet_hi_schedule.)

Notice that we are talking about Preempt-RT here. Everything is
running in thread context, even tasklets, softirqs etc. They are _all_
preemptible, and if Michael has some RT-thread in the system that has
a higher priority than this tasklet or softirq, than the buffer will
eventually overflow.
I wonder also if Michael has set the RT-priorities correctly, on RT
_every_ softirq/irq thread starts by default on priority 50,
SCHED_FIFO. If they are still at 50, any other softirq/tasklet, or irq
can make this behavior worse.
Notice that the default 50 thingy rarely gives a decent behaving
system. (But any other default will also give problems anyway, so it
_has_ to be customized/tuned by the end user)

Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

> It seems to be very sensitive to network traffic though...could it have
> something to do with softirq scheduling? Could you try the patch below
> and see if you can trigger the error message?

Funny that you mention this.
The largest latencies I currently have on RT (and rm9200) occur when
using a telnet session or NFS filesystems, thus while using network.
The impact on hardware Interrupt latencies are limited (<85us), so the
interrupt handler should still be able to keep up the receive buffer,
but context switches between threads can stall for a longer time under
some conditions.
A long shot, but can it be that the ringbuffer overflows, and that
therefor characters are lost?


Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

 It seems to be very sensitive to network traffic though...could it have
 something to do with softirq scheduling? Could you try the patch below
 and see if you can trigger the error message?

Funny that you mention this.
The largest latencies I currently have on RT (and rm9200) occur when
using a telnet session or NFS filesystems, thus while using network.
The impact on hardware Interrupt latencies are limited (85us), so the
interrupt handler should still be able to keep up the receive buffer,
but context switches between threads can stall for a longer time under
some conditions.
A long shot, but can it be that the ringbuffer overflows, and that
therefor characters are lost?


Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

> The code above is from the tasklet, so I don't think that's the problem.
> The interrupt handler doesn't take any locks.
> Or are spinlocks not allowed in softirq context either?

They are allowed there...
So, it has to be something different.

> > I believe I have to look at the latest set of patches, and try to find
> > any regressions. Do you have a location somewhere where I can download
> > the latest versions? Or do I need to dig through LKML to find the
> > latest... ;-)
> They are in -mm. You were Cc'ed I think...

Yes, I saw them, but I did not know if there were any updates in the
mean time, because I had seen some discussions, which confused me a
bit about the current status of the complete set.

> - with full preemption  it runs but the serial line can't be used for
> receiving at high bit rate (using lrz)

A few questions arise here to me:
* What serial port is used here? (DBGU, or something else)
* No DMA was used, was flow-control enabled? (cannot with DBGU)
* If some other UART, why not using DMA?

Notice that the DBGU has no flow control, and just a 1 byte FIFO (thus
no fifo at all).
At high speeds (e.g. >=115200) it is _likely_ that you will miss
characters, nothing can prevent that. DBGU should only be used at
lower speeds, or just as text console. 115200 is running fine here as
text-console.
I would not expect that the behaviour is worse than without the
patchset, because without it it does not work at all on Preempt-RT,
but also: there was done much more in interrupt context previously, so
the chance of buffer overruns was much more likely in the old
situation.
The real interrupt handler (doing the reading from the fifo) must be
as short as possible, to be able to keep up with the data flow.

A simple calculation: 115200bps results in approx. 11520 bytes per second.
This means that the interrupt handler must be capable of handling each
byte on DBGU within 87us. With a worst case interrupt latency of about
85us, and average between 2us and 54us (on Preempt-RT and AT91RM9200),
you can simply understand that this will not match, how good/fast the
interrupt handling will ever be.

So, I suggest to either use flow-control, or DMA for bulkdata... (thus not DBGU)

Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard (and Michael),

First I want to mention that I did not found the time yet to test your
latest integration of these atmel_serial patches, and I only know that
your set of the end of December works fine. I do not know the changes
you made since posting that set and your latest patch-set.

But let me clear some things up:
The original patchset I posted, existed of a set of patches suitable
for the mainline kernel, plus an additional patch for Preempt-RT only.
So, the splitup  of the interrupt handler was also needed for
Preempt-RT, but it was not the only patch that was needed on
preempt-rt.

Now looking at this problem:
> >  * Drop the lock here since it might end up calling
> >  * uart_start(), which takes the lock.
> >spin_unlock(>lock);
> >  */
> > tty_flip_buffer_push(port->info->tty);
> > /*
> > spin_lock(>lock);
> >  */
> > The same code with this comments out runs

I expect the UART generating the problem is the DBGU port. The DBGU
shares its interrupt line with the timer interrupt with the IRQF_TIMER
flag set, and thus the DBGU interrupt handler is running in
IRQF_NODELAY context. Within this context it is forbidden to lock a
normal spinlock, because a normal spinlock is converted to a mutex on
Preempt-RT; a mutex can sleep which is forbidden in interrupt context.
So, to get around this problem, this lock spinlock has to be of the
raw_spinlock_t type. The raw_spinlock_t is the normal mainline-kernel
spinlock, and as such it is not converted to a mutex, and will
therefor never sleep.

Attached a patch that changes this spinlock type. I used it in my
patchset, but your updates of December last year do not need this
patch anymore, so apparantly you changed something that has a
regression on Preempt-RT...

> > Complete Preemption (Real-Time) ok but the serials is just unusable due
> > to too many overruns (just using lrz)

This problem is not there in the December-set either. It works like a charm...

I believe I have to look at the latest set of patches, and try to find
any regressions. Do you have a location somewhere where I can download
the latest versions? Or do I need to dig through LKML to find the
latest... ;-)

Kind Regards,

Remy



2008/1/30, Haavard Skinnemoen <[EMAIL PROTECTED]>:
> On Wed, 30 Jan 2008 00:12:23 +0100
> michael <[EMAIL PROTECTED]> wrote:
>
> > I'm testing this patch on an at91sam9260 on 2.6.24-rt. I'm using this
> > patch with the tclib support for hrtimer and the clocksource pit_clk.
> > These are the results:
> >
> > - Voluntary Kernel Preemption the system (crash)
> > - Preemptible  Kernel (crash)
>
> Ouch. I'm assuming this is with DMA disabled?
>
> > /*
> >  * Drop the lock here since it might end up calling
> >  * uart_start(), which takes the lock.
> >spin_unlock(>lock);
> >  */
> > tty_flip_buffer_push(port->info->tty);
> > /*
> > spin_lock(>lock);
> >  */
> > The same code with this comments out runs
>
> Now, _that_ is strange. I can't see anything that needs protection
> across that call; in fact, I think we can lock a lot less than what we
> currently do.
>
> > Complete Preemption (Real-Time) ok but the serials is just unusable due
> > to too many overruns (just using lrz)
>
> Is it worse than before? IIRC Remy mentioned something about
> IRQF_NODELAY being the reason for moving all this code to softirq
> context in the first place; does the interrupt handler run in hardirq
> context?
>
> > The system is stable and doesn't crash reverting this patch.
> > I don't test with the thread hardirqs active.
>
> Ok.
>
> > >> +  ret = -ENOMEM;
> > >> +  data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
> > >> +  if (!data)
> > >> +  goto err_alloc_ring;
> > >> +  port->rx_ring.buf = data;
> > >> +
> > >>ret = uart_add_one_port(_uart, >uart);
> > >>
> > Is the kmalloc correct?
> > maybe:
> > data = kmalloc(ATMEL_SERIAL_RINGSIZE * sizeof(struct atmel_uart_char),
> > GFP_KERNEL);
>
> I think you're right. Can you change it and see if it helps?
>
> I guess I didn't test it thoroughly enough with DMA
> disabled...slub_debug ought to catch such things, but not until we
> receive enough data to actually overflow the buffer.
>
> > >> @@ -1033,6 +1165,9 @@ static int __devexit atmel_serial_remove(struct 
> > >> platform_device *pdev)
> > >>
> > >>ret = uart_remove_one_port(_uart, port);
> > >>
> > >> +  tasklet_kill(_port->tasklet);
> > >> +  kfree(atmel_port->rx_ring.buf);
> > >> +
> > >>
&g

Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

 The code above is from the tasklet, so I don't think that's the problem.
 The interrupt handler doesn't take any locks.
 Or are spinlocks not allowed in softirq context either?

They are allowed there...
So, it has to be something different.

  I believe I have to look at the latest set of patches, and try to find
  any regressions. Do you have a location somewhere where I can download
  the latest versions? Or do I need to dig through LKML to find the
  latest... ;-)
 They are in -mm. You were Cc'ed I think...

Yes, I saw them, but I did not know if there were any updates in the
mean time, because I had seen some discussions, which confused me a
bit about the current status of the complete set.

 - with full preemption  it runs but the serial line can't be used for
 receiving at high bit rate (using lrz)

A few questions arise here to me:
* What serial port is used here? (DBGU, or something else)
* No DMA was used, was flow-control enabled? (cannot with DBGU)
* If some other UART, why not using DMA?

Notice that the DBGU has no flow control, and just a 1 byte FIFO (thus
no fifo at all).
At high speeds (e.g. =115200) it is _likely_ that you will miss
characters, nothing can prevent that. DBGU should only be used at
lower speeds, or just as text console. 115200 is running fine here as
text-console.
I would not expect that the behaviour is worse than without the
patchset, because without it it does not work at all on Preempt-RT,
but also: there was done much more in interrupt context previously, so
the chance of buffer overruns was much more likely in the old
situation.
The real interrupt handler (doing the reading from the fifo) must be
as short as possible, to be able to keep up with the data flow.

A simple calculation: 115200bps results in approx. 11520 bytes per second.
This means that the interrupt handler must be capable of handling each
byte on DBGU within 87us. With a worst case interrupt latency of about
85us, and average between 2us and 54us (on Preempt-RT and AT91RM9200),
you can simply understand that this will not match, how good/fast the
interrupt handling will ever be.

So, I suggest to either use flow-control, or DMA for bulkdata... (thus not DBGU)

Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard (and Michael),

First I want to mention that I did not found the time yet to test your
latest integration of these atmel_serial patches, and I only know that
your set of the end of December works fine. I do not know the changes
you made since posting that set and your latest patch-set.

But let me clear some things up:
The original patchset I posted, existed of a set of patches suitable
for the mainline kernel, plus an additional patch for Preempt-RT only.
So, the splitup  of the interrupt handler was also needed for
Preempt-RT, but it was not the only patch that was needed on
preempt-rt.

Now looking at this problem:
   * Drop the lock here since it might end up calling
   * uart_start(), which takes the lock.
 spin_unlock(port-lock);
   */
  tty_flip_buffer_push(port-info-tty);
  /*
  spin_lock(port-lock);
   */
  The same code with this comments out runs

I expect the UART generating the problem is the DBGU port. The DBGU
shares its interrupt line with the timer interrupt with the IRQF_TIMER
flag set, and thus the DBGU interrupt handler is running in
IRQF_NODELAY context. Within this context it is forbidden to lock a
normal spinlock, because a normal spinlock is converted to a mutex on
Preempt-RT; a mutex can sleep which is forbidden in interrupt context.
So, to get around this problem, this lock spinlock has to be of the
raw_spinlock_t type. The raw_spinlock_t is the normal mainline-kernel
spinlock, and as such it is not converted to a mutex, and will
therefor never sleep.

Attached a patch that changes this spinlock type. I used it in my
patchset, but your updates of December last year do not need this
patch anymore, so apparantly you changed something that has a
regression on Preempt-RT...

  Complete Preemption (Real-Time) ok but the serials is just unusable due
  to too many overruns (just using lrz)

This problem is not there in the December-set either. It works like a charm...

I believe I have to look at the latest set of patches, and try to find
any regressions. Do you have a location somewhere where I can download
the latest versions? Or do I need to dig through LKML to find the
latest... ;-)

Kind Regards,

Remy



2008/1/30, Haavard Skinnemoen [EMAIL PROTECTED]:
 On Wed, 30 Jan 2008 00:12:23 +0100
 michael [EMAIL PROTECTED] wrote:

  I'm testing this patch on an at91sam9260 on 2.6.24-rt. I'm using this
  patch with the tclib support for hrtimer and the clocksource pit_clk.
  These are the results:
 
  - Voluntary Kernel Preemption the system (crash)
  - Preemptible  Kernel (crash)

 Ouch. I'm assuming this is with DMA disabled?

  /*
   * Drop the lock here since it might end up calling
   * uart_start(), which takes the lock.
 spin_unlock(port-lock);
   */
  tty_flip_buffer_push(port-info-tty);
  /*
  spin_lock(port-lock);
   */
  The same code with this comments out runs

 Now, _that_ is strange. I can't see anything that needs protection
 across that call; in fact, I think we can lock a lot less than what we
 currently do.

  Complete Preemption (Real-Time) ok but the serials is just unusable due
  to too many overruns (just using lrz)

 Is it worse than before? IIRC Remy mentioned something about
 IRQF_NODELAY being the reason for moving all this code to softirq
 context in the first place; does the interrupt handler run in hardirq
 context?

  The system is stable and doesn't crash reverting this patch.
  I don't test with the thread hardirqs active.

 Ok.

   +  ret = -ENOMEM;
   +  data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
   +  if (!data)
   +  goto err_alloc_ring;
   +  port-rx_ring.buf = data;
   +
  ret = uart_add_one_port(atmel_uart, port-uart);
  
  Is the kmalloc correct?
  maybe:
  data = kmalloc(ATMEL_SERIAL_RINGSIZE * sizeof(struct atmel_uart_char),
  GFP_KERNEL);

 I think you're right. Can you change it and see if it helps?

 I guess I didn't test it thoroughly enough with DMA
 disabled...slub_debug ought to catch such things, but not until we
 receive enough data to actually overflow the buffer.

   @@ -1033,6 +1165,9 @@ static int __devexit atmel_serial_remove(struct 
   platform_device *pdev)
  
  ret = uart_remove_one_port(atmel_uart, port);
  
   +  tasklet_kill(atmel_port-tasklet);
   +  kfree(atmel_port-rx_ring.buf);
   +
  
  Why the tasklet_kill is not done in atmel_shutdown?

 Why should it be? If it should, we must move the call to tasklet_init
 into atmel_startup too, and I don't really see the point.

 Haavard

On PREEMPT-RT we may not block on a normal spinlock in IRQ/IRQF_NODELAY-context.
This patch fixes this by making the lock a raw_spinlock_t for the
Atmel serial console.

This patch demands the following patches to be installed first:
* atmel_serial_cleanup.patch
* atmel_serial_irq_splitup.patch

Signed-off-by: Remy Bohmer [EMAIL PROTECTED]
---
 include/linux/serial_core.h |7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Index: linux-2.6.23/include/linux/serial_core.h

Re: [PATCH] kthread: always create the kernel threads with normal priority

[Remy Bohmer]

Hello Michal,

> > Maybe we can find a way to use a similar mechanism as I used in my
> > patchset for the priorities of the remaining kthreads.
> > I do not like the way of forcing userland to change the priorities,
> > because that would require a userland with the chrt tool installed,
> > and that is not that practical for embedded systems (in which there
> > could be cases that there is no userland at all, or the init-process
> > is the whole embedded application). In that case an option to do it on
> > the kernel commandline is more practical.
> >
> > I propose this kernel cmd-line option:
> > kthread_pmap=somethread:50,otherthread:12,34
>
> I see. kthreadd would look up the priority for itself and
> kthread_create would consult the map for all other kernel threads.
> That should work.
> Your sirq_pmap would not be needed anymore, as kthread_pmap could be
> used for softirq threads too, right?

That is correct. The soft-irqs are just ordinary kernel-threads, but
irq_pmap is still needed, to set the priority of a certain interrupt
handler.
In this case it also possible to set the prio of the IRQ-kthreads as
well as the prio of a certain interrupt handler. This might give some
conflicts, and I have to check how to resolve these.

Kind Regards,

Remy


2008/1/7, Michal Schmidt <[EMAIL PROTECTED]>:
> On Mon, 7 Jan 2008 12:22:51 +0100
> "Remy Bohmer" <[EMAIL PROTECTED]> wrote:
>
> > Hello Michal and Andrew,
> >
> > > Let's not make the decision for the user. Just allow the
> > > administrator to change kthreadd's priority safely if he chooses to
> > > do it. Ensure that the kernel threads are created with the usual
> > > nice level even if kthreadd's priority is changed from the default.
> >
> > Last year, I posted a patchset (that was meant for Preempt-RT at that
> > time) to be able to prioritise the interrupt-handler-threads (which
> > are kthreads) and softirq-threads from the kernel commandline. See
> > http://lkml.org/lkml/2007/12/19/208
> >
> > Maybe we can find a way to use a similar mechanism as I used in my
> > patchset for the priorities of the remaining kthreads.
> > I do not like the way of forcing userland to change the priorities,
> > because that would require a userland with the chrt tool installed,
> > and that is not that practical for embedded systems (in which there
> > could be cases that there is no userland at all, or the init-process
> > is the whole embedded application). In that case an option to do it on
> > the kernel commandline is more practical.
> >
> > I propose this kernel cmd-line option:
> > kthread_pmap=somethread:50,otherthread:12,34
>
> I see. kthreadd would look up the priority for itself and
> kthread_create would consult the map for all other kernel threads.
> That should work.
> Your sirq_pmap would not be needed anymore, as kthread_pmap could be
> used for softirq threads too, right?
>
> Michal
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] kthread: always create the kernel threads with normal priority

[Remy Bohmer]

Hello Michal and Andrew,

> Let's not make the decision for the user. Just allow the administrator to
> change kthreadd's priority safely if he chooses to do it. Ensure that the
> kernel threads are created with the usual nice level even if kthreadd's
> priority is changed from the default.

Last year, I posted a patchset (that was meant for Preempt-RT at that
time) to be able to prioritise the interrupt-handler-threads (which
are kthreads) and softirq-threads from the kernel commandline. See
http://lkml.org/lkml/2007/12/19/208

Maybe we can find a way to use a similar mechanism as I used in my
patchset for the priorities of the remaining kthreads.
I do not like the way of forcing userland to change the priorities,
because that would require a userland with the chrt tool installed,
and that is not that practical for embedded systems (in which there
could be cases that there is no userland at all, or the init-process
is the whole embedded application). In that case an option to do it on
the kernel commandline is more practical.

I propose this kernel cmd-line option:
kthread_pmap=somethread:50,otherthread:12,34

Then threads can be started as SCHED_NORMAL, and when overruled inside
the kthread-sources itself, or by the kernel commandline, the user can
set them to something else.

What do you think of this?

(notice that I am reworking the review comments I received on this
patch-series right now, and that I can take such change into account
immediately)

Kind Regards,

Remy

2008/1/7, Michal Schmidt <[EMAIL PROTECTED]>:
> On Sat, 22 Dec 2007 01:30:21 -0800
> Andrew Morton <[EMAIL PROTECTED]> wrote:
>
> > On Mon, 17 Dec 2007 23:43:14 +0100 Michal Schmidt
> > <[EMAIL PROTECTED]> wrote:
> >
> > > kthreadd, the creator of other kernel threads, runs as a normal
> > > priority task. This is a potential for priority inversion when a
> > > task wants to spawn a high-priority kernel thread. A middle priority
> > > SCHED_FIFO task can block kthreadd's execution indefinitely and thus
> > > prevent the timely creation of the high-priority kernel thread.
> > >
> > > This causes a practical problem. When a runaway real-time task is
> > > eating 100% CPU and we attempt to put the CPU offline, sometimes we
> > > block while waiting for the creation of the highest-priority
> > > "kstopmachine" thread.
> > >
> > > The fix is to run kthreadd with the highest possible SCHED_FIFO
> > > priority. Its children must still run as slightly negatively reniced
> > > SCHED_NORMAL tasks.
> >
> > Did you hit this problem with the stock kernel, or have you been
> > working on other stuff?
>
> This was with RHEL5 and with current Fedora kernels.
>
> > A locked-up SCHED_FIFO process will cause kernel threads all sorts of
> > problems.  You've hit one instance, but there will be others.
> > (pdflush stops working, for one).
> >
> > The general approach we've taken to this is "don't do that".  Yes, we
> > could boost lots of kernel threads in the way which this patch does
> > but this actually takes control *away* from userspace.  Userspace no
> > longer has the ability to guarantee itself minimum possible latency
> > without getting preempted by kernel threads.
> >
> > And yes, giving userspace this minimum-latency capability does imply
> > that userspace has a responsibility to not 100% starve kernel
> > threads.  It's a reasonable compromise, I think?
>
> You're right. We should not run kthreadd with SCHED_FIFO by default.
> But the user should be able to change it using chrt if he wants to
> avoid this particular problem. So how about this instead?:
>
>
>
> kthreadd, the creator of other kernel threads, runs as a normal priority task.
> This is a potential for priority inversion when a task wants to spawn a
> high-priority kernel thread. A middle priority SCHED_FIFO task can block
> kthreadd's execution indefinitely and thus prevent the timely creation of the
> high-priority kernel thread.
>
> This causes a practical problem. When a runaway real-time task is eating 100%
> CPU and we attempt to put the CPU offline, sometimes we block while waiting 
> for
> the creation of the highest-priority "kstopmachine" thread.
>
> This could be solved by always running kthreadd with the highest possible
> SCHED_FIFO priority, but that would be undesirable policy decision in the
> kernel. kthreadd would cause unwanted latencies even for the realtime users 
> who
> know what they're doing.
>
> Let's not make the decision for the user. Just allow the administrator to
> change kthreadd's priority safely if he chooses to do it. Ensure that the
> kernel threads are created with the usual nice level even if kthreadd's
> priority is changed from the default.
>
> Signed-off-by: Michal Schmidt <[EMAIL PROTECTED]>
> ---
>  kernel/kthread.c |   11 +++
>  1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index dcfe724..e832a85 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -94,10 +94,21 

Re: [PATCH] kthread: always create the kernel threads with normal priority

[Remy Bohmer]

Hello Michal and Andrew,

 Let's not make the decision for the user. Just allow the administrator to
 change kthreadd's priority safely if he chooses to do it. Ensure that the
 kernel threads are created with the usual nice level even if kthreadd's
 priority is changed from the default.

Last year, I posted a patchset (that was meant for Preempt-RT at that
time) to be able to prioritise the interrupt-handler-threads (which
are kthreads) and softirq-threads from the kernel commandline. See
http://lkml.org/lkml/2007/12/19/208

Maybe we can find a way to use a similar mechanism as I used in my
patchset for the priorities of the remaining kthreads.
I do not like the way of forcing userland to change the priorities,
because that would require a userland with the chrt tool installed,
and that is not that practical for embedded systems (in which there
could be cases that there is no userland at all, or the init-process
is the whole embedded application). In that case an option to do it on
the kernel commandline is more practical.

I propose this kernel cmd-line option:
kthread_pmap=somethread:50,otherthread:12,34

Then threads can be started as SCHED_NORMAL, and when overruled inside
the kthread-sources itself, or by the kernel commandline, the user can
set them to something else.

What do you think of this?

(notice that I am reworking the review comments I received on this
patch-series right now, and that I can take such change into account
immediately)

Kind Regards,

Remy

2008/1/7, Michal Schmidt [EMAIL PROTECTED]:
 On Sat, 22 Dec 2007 01:30:21 -0800
 Andrew Morton [EMAIL PROTECTED] wrote:

  On Mon, 17 Dec 2007 23:43:14 +0100 Michal Schmidt
  [EMAIL PROTECTED] wrote:
 
   kthreadd, the creator of other kernel threads, runs as a normal
   priority task. This is a potential for priority inversion when a
   task wants to spawn a high-priority kernel thread. A middle priority
   SCHED_FIFO task can block kthreadd's execution indefinitely and thus
   prevent the timely creation of the high-priority kernel thread.
  
   This causes a practical problem. When a runaway real-time task is
   eating 100% CPU and we attempt to put the CPU offline, sometimes we
   block while waiting for the creation of the highest-priority
   kstopmachine thread.
  
   The fix is to run kthreadd with the highest possible SCHED_FIFO
   priority. Its children must still run as slightly negatively reniced
   SCHED_NORMAL tasks.
 
  Did you hit this problem with the stock kernel, or have you been
  working on other stuff?

 This was with RHEL5 and with current Fedora kernels.

  A locked-up SCHED_FIFO process will cause kernel threads all sorts of
  problems.  You've hit one instance, but there will be others.
  (pdflush stops working, for one).
 
  The general approach we've taken to this is don't do that.  Yes, we
  could boost lots of kernel threads in the way which this patch does
  but this actually takes control *away* from userspace.  Userspace no
  longer has the ability to guarantee itself minimum possible latency
  without getting preempted by kernel threads.
 
  And yes, giving userspace this minimum-latency capability does imply
  that userspace has a responsibility to not 100% starve kernel
  threads.  It's a reasonable compromise, I think?

 You're right. We should not run kthreadd with SCHED_FIFO by default.
 But the user should be able to change it using chrt if he wants to
 avoid this particular problem. So how about this instead?:



 kthreadd, the creator of other kernel threads, runs as a normal priority task.
 This is a potential for priority inversion when a task wants to spawn a
 high-priority kernel thread. A middle priority SCHED_FIFO task can block
 kthreadd's execution indefinitely and thus prevent the timely creation of the
 high-priority kernel thread.

 This causes a practical problem. When a runaway real-time task is eating 100%
 CPU and we attempt to put the CPU offline, sometimes we block while waiting 
 for
 the creation of the highest-priority kstopmachine thread.

 This could be solved by always running kthreadd with the highest possible
 SCHED_FIFO priority, but that would be undesirable policy decision in the
 kernel. kthreadd would cause unwanted latencies even for the realtime users 
 who
 know what they're doing.

 Let's not make the decision for the user. Just allow the administrator to
 change kthreadd's priority safely if he chooses to do it. Ensure that the
 kernel threads are created with the usual nice level even if kthreadd's
 priority is changed from the default.

 Signed-off-by: Michal Schmidt [EMAIL PROTECTED]
 ---
  kernel/kthread.c |   11 +++
  1 files changed, 11 insertions(+), 0 deletions(-)

 diff --git a/kernel/kthread.c b/kernel/kthread.c
 index dcfe724..e832a85 100644
 --- a/kernel/kthread.c
 +++ b/kernel/kthread.c
 @@ -94,10 +94,21 @@ static void create_kthread(struct kthread_create_info 
 *create)
 if (pid  0) {
 create-result = ERR_PTR(pid);
 

Re: [PATCH] kthread: always create the kernel threads with normal priority

[Remy Bohmer]

Hello Michal,

  Maybe we can find a way to use a similar mechanism as I used in my
  patchset for the priorities of the remaining kthreads.
  I do not like the way of forcing userland to change the priorities,
  because that would require a userland with the chrt tool installed,
  and that is not that practical for embedded systems (in which there
  could be cases that there is no userland at all, or the init-process
  is the whole embedded application). In that case an option to do it on
  the kernel commandline is more practical.
 
  I propose this kernel cmd-line option:
  kthread_pmap=somethread:50,otherthread:12,34

 I see. kthreadd would look up the priority for itself and
 kthread_create would consult the map for all other kernel threads.
 That should work.
 Your sirq_pmap would not be needed anymore, as kthread_pmap could be
 used for softirq threads too, right?

That is correct. The soft-irqs are just ordinary kernel-threads, but
irq_pmap is still needed, to set the priority of a certain interrupt
handler.
In this case it also possible to set the prio of the IRQ-kthreads as
well as the prio of a certain interrupt handler. This might give some
conflicts, and I have to check how to resolve these.

Kind Regards,

Remy


2008/1/7, Michal Schmidt [EMAIL PROTECTED]:
 On Mon, 7 Jan 2008 12:22:51 +0100
 Remy Bohmer [EMAIL PROTECTED] wrote:

  Hello Michal and Andrew,
 
   Let's not make the decision for the user. Just allow the
   administrator to change kthreadd's priority safely if he chooses to
   do it. Ensure that the kernel threads are created with the usual
   nice level even if kthreadd's priority is changed from the default.
 
  Last year, I posted a patchset (that was meant for Preempt-RT at that
  time) to be able to prioritise the interrupt-handler-threads (which
  are kthreads) and softirq-threads from the kernel commandline. See
  http://lkml.org/lkml/2007/12/19/208
 
  Maybe we can find a way to use a similar mechanism as I used in my
  patchset for the priorities of the remaining kthreads.
  I do not like the way of forcing userland to change the priorities,
  because that would require a userland with the chrt tool installed,
  and that is not that practical for embedded systems (in which there
  could be cases that there is no userland at all, or the init-process
  is the whole embedded application). In that case an option to do it on
  the kernel commandline is more practical.
 
  I propose this kernel cmd-line option:
  kthread_pmap=somethread:50,otherthread:12,34

 I see. kthreadd would look up the priority for itself and
 kthread_create would consult the map for all other kernel threads.
 That should work.
 Your sirq_pmap would not be needed anymore, as kthread_pmap could be
 used for softirq threads too, right?

 Michal

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/3] Enable setting of IRQ-thread priorities from kernel cmdline. (repost:CC to LKML)

[Remy Bohmer]

> By default, all softirq-threads or IRQs will be RT task but if there
> is some option user can switch it to NON-RT task then it will be good.

Okay, understood.
I will think about how to add that too. But, probably we can add it
seperately from this patchset.

Kind Regards,

Remy


2007/12/20, Jaswinder Singh <[EMAIL PROTECTED]>:
> Hello Remy,
>
> On 12/20/07, Remy Bohmer <[EMAIL PROTECTED]> wrote:
> > So, Is this a serious requirement? Should this be possible?
>
> I have noticed this problem:
> [EMAIL PROTECTED]:~# cat /proc/loadavgrt
> 1.00 1.00 1.00 0/52 1158
> [EMAIL PROTECTED]:~# cat /proc/loadavg
> 0.00 0.00 0.02 1/52 1159
> [EMAIL PROTECTED]:~#
>
> So I am curious, if possible, user can switch softirq-threads or IRQs
> RT tasks to non-RT tasks for slow hardware or least important hardware
> for NON-RT tasks. So this will improve RT behaviour.
>
> By default, all softirq-threads or IRQs will be RT task but if there
> is some option user can switch it to NON-RT task then it will be good.
>
> Thank you,
>
> Jaswinder Singh.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/3] Enable setting of IRQ-thread priorities from kernel cmdline. (repost:CC to LKML)

[Remy Bohmer]

Hello Jaswinder,

> Is it possible to run softirq-threads or IRQs as a non-realtime tasks
> in RT kernel.

Currently the answer is no. (Unless overruled from userspace) I did
not take this requirement into account.
So, Is this a serious requirement? Should this be possible?
I cannot imagine a purpose in which that is wanted, it would cause
unpredictable interrupt handling, and I can imagine that many
drivers/devices will not like that.

The purpose is usually to spread the threads across several
priorities, to create room for some RT-process to become more
important than an IRQ that generates large latencies, and each
RT-system designer shall divide the priorities different, due to
different system requirements.

Kind Regards,

Remy

2007/12/20, Jaswinder Singh <[EMAIL PROTECTED]>:
> On 12/20/07, Remy Bohmer <[EMAIL PROTECTED]> wrote:
> > The RT-patch originally creates all its softirq-threads at
> > priority 50.
>
> Is it possible to run softirq-threads or IRQs as a non-realtime tasks
> in RT kernel.
>
> If yes, then how.
>
> Thank you,
>
> Jaswinder Singh.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/3] Enable setting of IRQ-thread priorities from kernel cmdline. (repost:CC to LKML)

[Remy Bohmer]

Hello Jaswinder,

 Is it possible to run softirq-threads or IRQs as a non-realtime tasks
 in RT kernel.

Currently the answer is no. (Unless overruled from userspace) I did
not take this requirement into account.
So, Is this a serious requirement? Should this be possible?
I cannot imagine a purpose in which that is wanted, it would cause
unpredictable interrupt handling, and I can imagine that many
drivers/devices will not like that.

The purpose is usually to spread the threads across several
priorities, to create room for some RT-process to become more
important than an IRQ that generates large latencies, and each
RT-system designer shall divide the priorities different, due to
different system requirements.

Kind Regards,

Remy

2007/12/20, Jaswinder Singh [EMAIL PROTECTED]:
 On 12/20/07, Remy Bohmer [EMAIL PROTECTED] wrote:
  The RT-patch originally creates all its softirq-threads at
  priority 50.

 Is it possible to run softirq-threads or IRQs as a non-realtime tasks
 in RT kernel.

 If yes, then how.

 Thank you,

 Jaswinder Singh.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/3] Enable setting of IRQ-thread priorities from kernel cmdline. (repost:CC to LKML)

[Remy Bohmer]

 By default, all softirq-threads or IRQs will be RT task but if there
 is some option user can switch it to NON-RT task then it will be good.

Okay, understood.
I will think about how to add that too. But, probably we can add it
seperately from this patchset.

Kind Regards,

Remy


2007/12/20, Jaswinder Singh [EMAIL PROTECTED]:
 Hello Remy,

 On 12/20/07, Remy Bohmer [EMAIL PROTECTED] wrote:
  So, Is this a serious requirement? Should this be possible?

 I have noticed this problem:
 [EMAIL PROTECTED]:~# cat /proc/loadavgrt
 1.00 1.00 1.00 0/52 1158
 [EMAIL PROTECTED]:~# cat /proc/loadavg
 0.00 0.00 0.02 1/52 1159
 [EMAIL PROTECTED]:~#

 So I am curious, if possible, user can switch softirq-threads or IRQs
 RT tasks to non-RT tasks for slow hardware or least important hardware
 for NON-RT tasks. So this will improve RT behaviour.

 By default, all softirq-threads or IRQs will be RT task but if there
 is some option user can switch it to NON-RT task then it will be good.

 Thank you,

 Jaswinder Singh.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 1/3] Add generic routine for parsing map-like options on kernel cmd-line (repost:CC to LKML)

[Remy Bohmer]

Hello Randy,

Sorry for the language errors, English is not my Native language, so I
make these stupid errors...

> > + *   get_map_option - Parse integer from an option map
> The @param lines (below) need to go here, immediately following the
> function short description (the line above).  No intervening blank
> lines.

OK, I will adapt that.

> > +int get_map_option(const char *str, const char *key, int *pint)
> > +{
> > + char  buf[COMMAND_LINE_SIZE];
>
> COMMAND_LINE_SIZE varies from 256 to 2048, depending on $ARCH.
> That's a bit too much to declare on a function's local stack --
> unless you are very certain of the call tree to this point and
> that the total stack size is safe.  Can you just kmalloc() this
> buf?

I know it is big on a 4k stack, and I also think it is not very nice...
But kmalloc() panics the kernel if I do it as soon as in the __setup() code.

The problem is that the original string may not be modified by the
cmdline-parser, and I do not know the length of the command up front,
(except that it cannot be longer than this define). Allocating a
global buffer is not safe and needs locking. So actually only what
left for me was the stack... or rewrite the used C-library-routines
completely myself, for this purpose only, which is also not nice.

I hope someone could point to me to another possibilty, that I did not
think of yet.


Kind Regards,

Remy

>
> > + char  *p, *substr;
> > + int   found = 0;
> > +
> > + /* We must copy the string to the stack, because strsep()
> > +changes it.*/
> > + strncpy(buf, str, COMMAND_LINE_SIZE);
> > + buf[COMMAND_LINE_SIZE-1] = '\0';
> > +
> > + p = buf;
> > + substr = strsep(, ",");
> > + while ((!found) && (substr != NULL)) {
> > + if (strlen(substr) != 0) {
> > + if (key == NULL) {
> > + /* Check for the absence of any ':' */
> > + if (strchr(substr, ':') == NULL) {
> > + sscanf(substr, "%d", pint);
> > + found = 1;
> > + }
> > + } else {
> > + /* check if the first part of the key matches 
> > */
> > + if (!strncmp(substr, key, strlen(key))) {
> > + substr += strlen(key);
> > + /* Now the next char must be a ':',
> > +if not, search for the next match 
> > */
> > + if (*substr == ':') {
> > + substr++;
> > + sscanf(substr, "%d", pint);
> > + found = 1;
> > + }
> > + }
> > + }
> > + }
> > + substr = strsep(, ",");
> > + }
> > + return found;
> > +}
>
> ---
> ~Randy
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/6] atmel_serial: Cleanups, irq handler splitup & DMA

[Remy Bohmer]

Hello,

> preempt-rt can disable interrupts for 300 us?

I am not sure if it really is an interrupt lock that long, but on
these ARM cores I still run into latencies that large. I tried using
latency_trace to figure out where they come from, but that tool even
reports interrupt locks up to 10 ms while those surely do not exist
(the 2nd in size reported is about 300us). So, those tools are buggy
on AT91 as well, and I do not trust them completely yet.. (They even
do not compile cleanly, conflicts in headers, warnings etc. Maybe
those tools are not used very much outside x86)

So, I still have a long way to go to make Preempt-RT to run on these
cores just as good as preempt-rt runs on X86, although I am sure that
I never will reach the <30us as I have on x86.

> If so, then I guess there's really no way to avoid a few overruns.

And if DBGU is used as a terminal, it should be no problem at all, or
someone has to slow down the baudrate a bit.
I only get this overrun under very heavy system load (CPU load 99%,
with one single thread running at RT-prio 99, taking chunks away of
99/100ms.) and then throw in a big file of many MBs in the terminal.
But, as long as the read part still runs in hard-irq context, it
should keep up, unless interrupts are locked too long.

> > Notice that without these interrupt handler splitup, it was much, much
> > much worse...
>
> Ok, that's good I guess.

Yep, that _is_ good.

> > So, for me it is not a big deal, because it is just a terminal, and
> > with my shaky fingers I usually do not type that fast ;-))
> If you do, you just need to switch to one of the USARTs instead of the
> DBGU so that you can use DMA :-)

:-)

> We need to fix the break- and error handling though. But my vacation
> starts tomorrow, so I probably won't be able to fix it until next year.

In that case, I wish you very good holiday, and a happy new-year.
It was very pleasant working with you on this driver.

I probably see you again on the list next year.


Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[patch 3/3] Enable setting of IRQ-thread priorities from kernel cmdline. (repost:CC to LKML)

[Remy Bohmer]

The RT-patch originally creates all its softirq-threads at
priority 50. Of course, this is not a usable default for many
realtime systems and therefor these priorities has to be tuneable
for each RT-system. But, currently there is no way within the
kernel to adjust this to the needs of the user. Some scripts
are floating around to do this from userspace, but for several
applications the priorities should already be set properly by
the kernel itself before userland is started.

This patch changes this by adding a kernel cmd-line option that
can handle a map of priorities.

Remarks:
* Priorities are only set at creation time of the softirq.
* Priorities has to be set per-cpu.
* If userland overrules, it is NOT restored by this code.
* if no new kernel cmdline options are given, the kernel works
  as before, and all softirqs start at 50.
* No wildcards are supported on the cmdline.

See kernel/Documentation/kernel-parameters.txt for usage info.

Signed-off-by: Remy Bohmer <[EMAIL PROTECTED]>

---
 Documentation/kernel-parameters.txt |   11 +++
 kernel/softirq.c|  105 ++--
 2 files changed, 99 insertions(+), 17 deletions(-)

Index: linux-2.6.24-rc5-rt1/Documentation/kernel-parameters.txt
===
--- linux-2.6.24-rc5-rt1.orig/Documentation/kernel-parameters.txt   
2007-12-18 22:06:11.0 +0100
+++ linux-2.6.24-rc5-rt1/Documentation/kernel-parameters.txt2007-12-18 
22:07:19.0 +0100
@@ -806,6 +806,17 @@ and is between 256 and 4096 characters. 
If this cmdline argument is ommitted, every thread
runs at prio 50.
 
+   sirq_pmap=  [IRQ-threading] List of priorities each softirq
+   thread must have.
+   Format: sirq_pmap=block/0:90,sched/0:75,50
+   The priorities have to be specified per-cpu.
+   The first field without ':', is the default prio.
+   The names have to match the softirq_names[] table in
+   kernel/softirq.c, (thus without 'softirq-' prefix) to
+   keep the cmd-line short.
+   If this cmdline argument is ommitted, every softirq
+   runs at prio 50.
+
ports=  [IP_VS_FTP] IPVS ftp helper module
Default is 21.
Up to 8 (IP_VS_APP_MAX_PORTS) ports
Index: linux-2.6.24-rc5-rt1/kernel/softirq.c
===
--- linux-2.6.24-rc5-rt1.orig/kernel/softirq.c  2007-12-18 22:06:11.0 
+0100
+++ linux-2.6.24-rc5-rt1/kernel/softirq.c   2007-12-18 22:08:54.0 
+0100
@@ -66,6 +66,10 @@ struct softirqdata {
 
 static DEFINE_PER_CPU(struct softirqdata [MAX_SOFTIRQ], ksoftirqd);
 
+static char *cmdline;
+static int  default_prio = MAX_USER_RT_PRIO/2;
+
+
 #ifdef CONFIG_PREEMPT_SOFTIRQS
 /*
  * Preempting the softirq causes cases that would not be a
@@ -770,10 +774,28 @@ EXPORT_SYMBOL(tasklet_unlock_wait);
 
 #endif
 
+static const char *softirq_names [] =
+{
+  [HI_SOFTIRQ] = "high",
+  [SCHED_SOFTIRQ]  = "sched",
+  [TIMER_SOFTIRQ]  = "timer",
+  [NET_TX_SOFTIRQ] = "net-tx",
+  [NET_RX_SOFTIRQ] = "net-rx",
+  [BLOCK_SOFTIRQ]  = "block",
+  [TASKLET_SOFTIRQ]= "tasklet",
+#ifdef CONFIG_HIGH_RES_TIMERS
+  [HRTIMER_SOFTIRQ]= "hrtimer",
+#endif
+  [RCU_SOFTIRQ]= "rcu",
+};
+
+static int get_softirq_prio(const char *name);
+
 static int ksoftirqd(void * __data)
 {
-   struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO/2 };
+   struct sched_param param = { 0, };
struct softirqdata *data = __data;
+   char buf[50];
u32 softirq_mask = (1 << data->nr);
struct softirq_action *h;
int cpu = data->cpu;
@@ -781,8 +803,12 @@ static int ksoftirqd(void * __data)
 #ifdef CONFIG_PREEMPT_SOFTIRQS
init_waitqueue_head(>wait);
 #endif
-
+   /* Lookup the priority of this softirq, and set the prio accordingly */
+   snprintf(buf, sizeof(buf), "%s/%lu",
+softirq_names[data->nr], data->cpu);
+   param.sched_priority = get_softirq_prio(buf);
sys_sched_setscheduler(current->pid, SCHED_FIFO, );
+
current->flags |= PF_SOFTIRQ;
set_current_state(TASK_INTERRUPTIBLE);
 
@@ -911,21 +937,6 @@ void takeover_tasklets(unsigned int cpu)
 }
 #endif /* CONFIG_HOTPLUG_CPU */
 
-static const char *softirq_names [] =
-{
-  [HI_SOFTIRQ] = "high",
-  [SCHED_SOFTIRQ]  = "sched",
-  [TIMER_SOFTIRQ]  = "timer",
-  [NET_TX_SOFTIRQ] = "net-tx",
-  [NET_RX_SOFTIRQ] = "net-rx",
-  [BLOCK_SOFTIRQ] 

[patch 2/3] Enable setting of IRQ-thread priorities from kernel cmdline (repost:CC to LKML)

[Remy Bohmer]

The RT-patch originally creates all its IRQ-threads at priority 50.
Of course, this is not a usable default for many realtime systems
and therefor these priorities has to be tuneable for each RT-system.
But, currently there is no way within the kernel to adjust this to
the needs of the user. Some scripts are floating around to do this
from userspace, but for several applications the priorities should
already be set properly by the kernel itself before userland is
started.

This patch changes this by adding a kernel cmd-line option that can
handle a map of priorities.
It is based on the name of the driver that requests an interrupt, it
does NOT use the IRQ-ID to prioritize a certain IRQ thread.
This is designed this way because across several hardware-boards the
IRQ-IDs can change, but not necessarily the drivers 
(interrupt handlers) name.


Remarks:
* There is a check for conflicts added, for situations where 
  IRQ-sharing is detected with different priorities requested
  on the kernel cmd-line.
* Priorities are only set at creation time of the IRQ-thread.
* If userland overrules, it is NOT restored by this code.
* if no new kernel cmdline options are given, the kernel works
  as before, and all IRQ-threads start at 50.
* No wildcards are supported on the cmdline.

See kernel/Documentation/kernel-parameters.txt for usage info.

Signed-off-by: Remy Bohmer <[EMAIL PROTECTED]>

---
 Documentation/kernel-parameters.txt |7 ++
 include/linux/irq.h |1 
 kernel/irq/manage.c |  101 
 3 files changed, 98 insertions(+), 11 deletions(-)

Index: linux-2.6.24-rc5-rt1/Documentation/kernel-parameters.txt
===
--- linux-2.6.24-rc5-rt1.orig/Documentation/kernel-parameters.txt   
2007-12-18 22:09:13.0 +0100
+++ linux-2.6.24-rc5-rt1/Documentation/kernel-parameters.txt2007-12-18 
22:09:16.0 +0100
@@ -799,6 +799,13 @@ and is between 256 and 4096 characters. 
ips=[HW,SCSI] Adaptec / IBM ServeRAID controller
See header of drivers/scsi/ips.c.
 
+   irq_pmap=   [IRQ-threading] List of priorities each interrupt
+   thread must have.
+   Format: irq_pmap=megasas:90,eth0:40,50
+   The first field without ':', is the default prio.
+   If this cmdline argument is ommitted, every thread
+   runs at prio 50.
+
ports=  [IP_VS_FTP] IPVS ftp helper module
Default is 21.
Up to 8 (IP_VS_APP_MAX_PORTS) ports
Index: linux-2.6.24-rc5-rt1/include/linux/irq.h
===
--- linux-2.6.24-rc5-rt1.orig/include/linux/irq.h   2007-12-18 
22:09:13.0 +0100
+++ linux-2.6.24-rc5-rt1/include/linux/irq.h2007-12-18 22:09:16.0 
+0100
@@ -169,6 +169,7 @@ struct irq_desc {
unsigned intirqs_unhandled;
unsigned long   last_unhandled; /* Aging timer for unhandled 
count */
struct task_struct  *thread;
+   int rt_prio;
wait_queue_head_t   wait_for_handler;
cycles_ttimestamp;
raw_spinlock_t  lock;
Index: linux-2.6.24-rc5-rt1/kernel/irq/manage.c
===
--- linux-2.6.24-rc5-rt1.orig/kernel/irq/manage.c   2007-12-18 
22:09:13.0 +0100
+++ linux-2.6.24-rc5-rt1/kernel/irq/manage.c2007-12-18 22:15:43.0 
+0100
@@ -13,9 +13,19 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "internals.h"
 
+
+#ifdef CONFIG_PREEMPT_HARDIRQS
+
+static char *cmdline;
+static int  default_prio = MAX_USER_RT_PRIO/2;
+
+#endif
+
+
 #ifdef CONFIG_SMP
 
 /**
@@ -260,7 +270,7 @@ void recalculate_desc_flags(struct irq_d
desc->status |= IRQ_NODELAY;
 }
 
-static int start_irq_thread(int irq, struct irq_desc *desc);
+static int start_irq_thread(int irq, struct irq_desc *desc, int prio);
 
 /*
  * Internal function that tells the architecture code whether a
@@ -293,6 +303,8 @@ void compat_irq_chip_set_default_handler
desc->handle_irq = NULL;
 }
 
+static int get_irq_prio(const char *name);
+
 /*
  * Internal function to register an irqaction - typically used to
  * allocate special interrupts that are part of the architecture.
@@ -328,7 +340,7 @@ int setup_irq(unsigned int irq, struct i
}
 
if (!(new->flags & IRQF_NODELAY))
-   if (start_irq_thread(irq, desc))
+   if (start_irq_thread(irq, desc, get_irq_prio(new->name)))
return -ENOMEM;
/*
 * The following block of code has to be executed atomically
@@ -811,11 +823,7 @@ static int do_irqd(void * __desc)
 #endif
current-&g

[patch 1/3] Add generic routine for parsing map-like options on kernel cmd-line (repost:CC to LKML)

[Remy Bohmer]

This patch adds a generic routine to the kernel, so that a map of
settings can be entered on the kernel commandline.

Signed-off-by: Remy Bohmer <[EMAIL PROTECTED]>

---
---
 include/linux/kernel.h |1 
 lib/cmdline.c  |   58 +
 2 files changed, 59 insertions(+)

Index: linux-2.6.24-rc5-rt1/include/linux/kernel.h
===
--- linux-2.6.24-rc5-rt1.orig/include/linux/kernel.h2007-12-18 
21:32:09.0 +0100
+++ linux-2.6.24-rc5-rt1/include/linux/kernel.h 2007-12-18 21:33:58.0 
+0100
@@ -164,6 +164,7 @@ extern int vsscanf(const char *, const c
 
 extern int get_option(char **str, int *pint);
 extern char *get_options(const char *str, int nints, int *ints);
+extern int get_map_option(const char *str, const char *key, int *pint);
 extern unsigned long long memparse(char *ptr, char **retptr);
 
 extern int core_kernel_text(unsigned long addr);
Index: linux-2.6.24-rc5-rt1/lib/cmdline.c
===
--- linux-2.6.24-rc5-rt1.orig/lib/cmdline.c 2007-10-09 22:31:38.0 
+0200
+++ linux-2.6.24-rc5-rt1/lib/cmdline.c  2007-12-18 21:33:58.0 +0100
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /*
  * If a hyphen was found in get_option, this will handle the
@@ -114,6 +115,63 @@ char *get_options(const char *str, int n
 }
 
 /**
+ * get_map_option - Parse integer from an option map
+ *
+ * This function parses an integer from an option map like
+ *   some_map=key1:99,key2:98,...,keyN:NN,NN
+ * Only the value of the first match is returned, or if no
+ * key option is given (key = NULL) the value of the first
+ * field without a ':' is returned.
+ *
+ * @str: option string
+ * @key: The key inside the map, can be NULL
+ * @pint: (output) integer value parsed from the map @str
+ *
+ * Return values:
+ * 0 - no int in string
+ * 1 - int found
+ */
+int get_map_option(const char *str, const char *key, int *pint)
+{
+   char  buf[COMMAND_LINE_SIZE];
+   char  *p, *substr;
+   int   found = 0;
+
+   /* We must copy the string to the stack, because strsep()
+  changes it.*/
+   strncpy(buf, str, COMMAND_LINE_SIZE);
+   buf[COMMAND_LINE_SIZE-1] = '\0';
+
+   p = buf;
+   substr = strsep(, ",");
+   while ((!found) && (substr != NULL)) {
+   if (strlen(substr) != 0) {
+   if (key == NULL) {
+   /* Check for the absence of any ':' */
+   if (strchr(substr, ':') == NULL) {
+   sscanf(substr, "%d", pint);
+   found = 1;
+   }
+   } else {
+   /* check if the first part of the key matches */
+   if (!strncmp(substr, key, strlen(key))) {
+   substr += strlen(key);
+   /* Now the next char must be a ':',
+  if not, search for the next match */
+   if (*substr == ':') {
+   substr++;
+   sscanf(substr, "%d", pint);
+   found = 1;
+   }
+   }
+   }
+   }
+   substr = strsep(, ",");
+   }
+   return found;
+}
+
+/**
  * memparse - parse a string with mem suffixes into a number
  * @ptr: Where parse begins
  * @retptr: (output) Pointer to next char after parse completes

-- 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[patch 0/3] add kernel-cmdline support for interrupt thread priorities (repost:CC to LKML)

[Remy Bohmer]

Hello All,

Back in August we had a discussion about the default priorities of
IRQ-threads and softirqs, and the lack of a mechanism to configure them from
within the kernel.
(see http://www.mail-archive.com/[EMAIL PROTECTED]/msg01022.html)

Here is a patchset that implements such a nice feature from the kernel cmdline,
similar to what we all agreed on, back then.

The set exist of 3 parts:
1) add a generic routine to the kernel commandline-parse-library to parse the
format of the map as we agreed on.
2) Add a __setup(irq_pmap=) routine to kernel/irq/manage.c
3) Add a __setup(sirq_pmap=) routine to kernel/softirq.c

I hope you can find the time to review this set.


Kind Regards,

Remy Bohmer
-- 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/6] atmel_serial: Cleanups, irq handler splitup & DMA

[Remy Bohmer]

> > but I noticed that I sometimes get an input overrun (ttyS0: 1
> > input overrun(s) ) during stress conditions.
> > This is something I did not notice before, maybe it was already there,
> > or has something changed in this area that it is now more sensitive
> > for this?
> Hmm...is this with or without DMA?

DBGU is without DMA.

> If it's without DMA, something very strange is going on -- the non-DMA
> RX code is almost the only thing left in the hardirq handler, so I
> would really expect overruns to be much less likely to occur now than
> before...

As mentioned, maybe it was already there, but I did not run into it
earlier. I have to figure that out. But, at 115200 and a 1 byte
receive-'fifo' on DBGU, and still interrupt locks somewhere in the
tree up to 300us, it is a simple calculation that we can run into
overrun conditions...
Notice that without these interrupt handler splitup, it was much, much
much worse...

So, for me it is not a big deal, because it is just a terminal, and
with my shaky fingers I usually do not type that fast ;-))

Remy

Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/6] atmel_serial: Cleanups, irq handler splitup & DMA

[Remy Bohmer]

Hello Haavard,

> Could you try the patch below? It's a bit strange that you got an oops
> though...

It is not really strange... spinlocks are mutexes on preempt-rt, and
recursive mutex locking is not allowed, this is one differences with
the mainline spinlock.

But... I tried that patch, and it works a lot better, no oopses
anymore, but I noticed that I sometimes get an input overrun (ttyS0: 1
input overrun(s) ) during stress conditions.
This is something I did not notice before, maybe it was already there,
or has something changed in this area that it is now more sensitive
for this?

Kind Regards,

Remy

2007/12/19, Haavard Skinnemoen <[EMAIL PROTECTED]>:
> On Wed, 19 Dec 2007 16:57:04 +0100
> "Remy Bohmer" <[EMAIL PROTECTED]> wrote:
>
> > Hello Haavard,
> >
> > Sorry.. But I get an Oops on Preempt-RT with the latest set of
> > patches. I did not see it earlier today with the other set of patches.
>
> Hmm...from the backtrace, it looks like lock recursion -- port->lock is
> held for the whole duration of the tasklet, but we somehow end up in
> uart_start(), which grabs the lock again.
>
> Could you try the patch below? It's a bit strange that you got an oops
> though...
>
> Haavard
>
> diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
> index 7967054..948c643 100644
> --- a/drivers/serial/atmel_serial.c
> +++ b/drivers/serial/atmel_serial.c
> @@ -666,7 +666,13 @@ static void atmel_rx_from_ring(struct uart_port *port)
> uart_insert_char(port, status, ATMEL_US_OVRE, c.ch, flg);
> }
>
> +   /*
> +* Drop the lock here since it might end up calling
> +* uart_start(), which takes the lock.
> +*/
> +   spin_unlock(>lock);
> tty_flip_buffer_push(port->info->tty);
> +   spin_lock(>lock);
>  }
>
>  static void atmel_rx_from_dma(struct uart_port *port)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/6] atmel_serial: Cleanups, irq handler splitup & DMA

[Remy Bohmer]

)
[   14.217766] [] (__might_sleep+0x0/0x104) from
[] (__rt_spin_lock+0x30/0x6c)
[   14.226555]  r4:c02e
[   14.228508] [] (__rt_spin_lock+0x0/0x6c) from
[] (rt_spin_lock+0x10/0x14)
[   14.237297]  r5:c02dd5a0 r4:c02dd818
[   14.240227] [] (rt_spin_lock+0x0/0x14) from []
(do_exit+0x210/0x7e0)
[   14.249016] [] (do_exit+0x0/0x7e0) from []
(die+0x1c8/0x214)
[   14.255852] [] (die+0x0/0x214) from []
(__do_kernel_fault+0x6c/0x7c)
[   14.263664] [] (__do_kernel_fault+0x0/0x7c) from
[] (do_page_fault+0x21c/0x238)
[   14.272453]  r7:c02e1cf0 r6:c02dd5a0 r5:c01f0370 r4:
[   14.278313] [] (do_page_fault+0x0/0x238) from
[] (do_DataAbort+0x3c/0xa0)
[   14.287102] [] (do_DataAbort+0x0/0xa0) from []
(__dabt_svc+0x40/0x60)
[   14.294914] Exception stack(0xc02e1cf0 to 0xc02e1d38)
[   14.299797] 1ce0: 
c0212afc c02dd5a0 c02dd5a0
[   14.308586] 1d00: a013 c02e c0212aec c001e800 0001
c001ec58 c3c3cc21 c02e1d94
[   14.316398] 1d20: c02dd5a0 c02e1d38 c01a46c4 c01a46f4 6093 
[   14.324211]  r8:0001 r7:c001e800 r6:c0212aec r5:c02e1d24 r4:
[   14.331047] [] (rt_spin_lock_slowlock+0x0/0x1c8) from
[] (__rt_spin_lock+0x60/0x6c)
[   14.340812] [] (__rt_spin_lock+0x0/0x6c) from
[] (rt_spin_lock+0x10/0x14)
[   14.348625]  r5:c0212aec r4:c001e800
[   14.352531] [] (rt_spin_lock+0x0/0x14) from []
(uart_start+0x20/0x34)
[   14.360344] [] (uart_start+0x0/0x34) from []
(uart_flush_chars+0x10/0x14)
[   14.369133]  r5:c3c3cc1c r4:001b
[   14.372062] [] (uart_flush_chars+0x0/0x14) from
[] (n_tty_receive_buf+0xd8c/0xe70)
[   14.381828] [] (n_tty_receive_buf+0x0/0xe70) from
[] (flush_to_ldisc+0xfc/0x184)
[   14.390617] [] (flush_to_ldisc+0x0/0x184) from
[] (tty_flip_buffer_push+0x3c/0x40)
[   14.399406] [] (tty_flip_buffer_push+0x0/0x40) from
[] (atmel_tasklet_func+0x5ec/0x5fc)
[   14.409172]  r5:1a1b r4:c0212aec
[   14.413078] [] (atmel_tasklet_func+0x0/0x5fc) from
[] (__tasklet_action+0xc8/0x194)
[   14.421867] [] (__tasklet_action+0x0/0x194) from
[] (tasklet_action+0x38/0x40)
[   14.431633]  r8:c02e r7:c0209e20 r6:ffdf r5:c0209c20 r4:c0209c20
[   14.437492] [] (tasklet_action+0x0/0x40) from
[] (ksoftirqd+0x178/0x228)
[   14.446281] [] (ksoftirqd+0x0/0x228) from []
(kthread+0x60/0x94)
[   14.454094] [] (kthread+0x0/0x94) from []
(do_exit+0x0/0x7e0)
[   14.460930]  r6: r5: r4:


2007/12/19, Haavard Skinnemoen <[EMAIL PROTECTED]>:
> The following patchset cleans up the atmel_serial driver a bit,
> moves a significant portion of the interrupt handler into a tasklet,
> and adds DMA support. This is the result of a combined effort by Chip
> Coldwell, Remy Bohmer and me. The patches should apply cleanly onto
> Linus' latest git tree.
>
> With DMA, I see transfer rates around 92 kbps when transferring a big
> file using ZModem (both directions are roughly the same.)
>
> Note that break and error handling doesn't work too well with DMA
> enabled. This is a common problem with all the efforts I've seen
> adding DMA support to this driver (including my own). The PDC error
> handling also accesses icount without locking. I'm tempted to just
> ignore the problem for now and hopefully come up with a solution
> later.
>
> Everyone, please give it a try and/or review the code.
>
> Chip Coldwell (1):
>   atmel_serial: Add DMA support
>
> Haavard Skinnemoen (3):
>   atmel_serial: Use cpu_relax() when busy-waiting
>   atmel_serial: Use existing console options only if BRG is running
>   atmel_serial: Fix bugs in probe() error path and remove()
>
> Remy Bohmer (2):
>   atmel_serial: Clean up the code
>   atmel_serial: Split the interrupt handler
>
>  drivers/serial/atmel_serial.c |  938 
> -
>  1 files changed, 738 insertions(+), 200 deletions(-)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

> Hrm. We probably need to lock while updating icount. That's a problem
> since we do that from the tx interrupt handler...and I don't suppose we
> want to move most of the atmel_tx_chars() code into the tasklet too...?

I do not see a reason why not moving transmit to a tasklet. It is only
time critical to read in time. If the transmit is postponed for a
while, it will only slow down transmission, while not reading in time
results in lost data.

Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

> Hmm...perhaps we can eliminate the locking in the status handler
> too...? Does anyone see a problem with this patch?

I have not seen any problem so far, besides, I am very happy with a
lockless interrupt handling, because this helps reducing latencies.

Tested it on top of the other 5 patches, and everything still works,
also tested under stress conditions.

So:
Acked-by: Remy Bohmer <[EMAIL PROTECTED]>


Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

 Hmm...perhaps we can eliminate the locking in the status handler
 too...? Does anyone see a problem with this patch?

I have not seen any problem so far, besides, I am very happy with a
lockless interrupt handling, because this helps reducing latencies.

Tested it on top of the other 5 patches, and everything still works,
also tested under stress conditions.

So:
Acked-by: Remy Bohmer [EMAIL PROTECTED]


Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

 Hrm. We probably need to lock while updating icount. That's a problem
 since we do that from the tx interrupt handler...and I don't suppose we
 want to move most of the atmel_tx_chars() code into the tasklet too...?

I do not see a reason why not moving transmit to a tasklet. It is only
time critical to read in time. If the transmit is postponed for a
while, it will only slow down transmission, while not reading in time
results in lost data.

Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/6] atmel_serial: Cleanups, irq handler splitup DMA

[Remy Bohmer]

 function called from invalid context
softirq-tasklet(9) at kernel/rtmutex.c:637
[   14.204094] in_atomic():1 [0001], irqs_disabled():0
[   14.208977] [c0023d48] (dump_stack+0x0/0x14) from [c002a5a8]
(__might_sleep+0xe0/0x104)
[   14.217766] [c002a4c8] (__might_sleep+0x0/0x104) from
[c01a4af8] (__rt_spin_lock+0x30/0x6c)
[   14.226555]  r4:c02e
[   14.228508] [c01a4ac8] (__rt_spin_lock+0x0/0x6c) from
[c01a4b44] (rt_spin_lock+0x10/0x14)
[   14.237297]  r5:c02dd5a0 r4:c02dd818
[   14.240227] [c01a4b34] (rt_spin_lock+0x0/0x14) from [c00338e8]
(do_exit+0x210/0x7e0)
[   14.249016] [c00336d8] (do_exit+0x0/0x7e0) from [c0023c4c]
(die+0x1c8/0x214)
[   14.255852] [c0023a84] (die+0x0/0x214) from [c0024f38]
(__do_kernel_fault+0x6c/0x7c)
[   14.263664] [c0024ecc] (__do_kernel_fault+0x0/0x7c) from
[c0025164] (do_page_fault+0x21c/0x238)
[   14.272453]  r7:c02e1cf0 r6:c02dd5a0 r5:c01f0370 r4:
[   14.278313] [c0024f48] (do_page_fault+0x0/0x238) from
[c001f250] (do_DataAbort+0x3c/0xa0)
[   14.287102] [c001f214] (do_DataAbort+0x0/0xa0) from [c001fa60]
(__dabt_svc+0x40/0x60)
[   14.294914] Exception stack(0xc02e1cf0 to 0xc02e1d38)
[   14.299797] 1ce0: 
c0212afc c02dd5a0 c02dd5a0
[   14.308586] 1d00: a013 c02e c0212aec c001e800 0001
c001ec58 c3c3cc21 c02e1d94
[   14.316398] 1d20: c02dd5a0 c02e1d38 c01a46c4 c01a46f4 6093 
[   14.324211]  r8:0001 r7:c001e800 r6:c0212aec r5:c02e1d24 r4:
[   14.331047] [c01a4670] (rt_spin_lock_slowlock+0x0/0x1c8) from
[c01a4b28] (__rt_spin_lock+0x60/0x6c)
[   14.340812] [c01a4ac8] (__rt_spin_lock+0x0/0x6c) from
[c01a4b44] (rt_spin_lock+0x10/0x14)
[   14.348625]  r5:c0212aec r4:c001e800
[   14.352531] [c01a4b34] (rt_spin_lock+0x0/0x14) from [c010e298]
(uart_start+0x20/0x34)
[   14.360344] [c010e278] (uart_start+0x0/0x34) from [c010e2bc]
(uart_flush_chars+0x10/0x14)
[   14.369133]  r5:c3c3cc1c r4:001b
[   14.372062] [c010e2ac] (uart_flush_chars+0x0/0x14) from
[c01007e0] (n_tty_receive_buf+0xd8c/0xe70)
[   14.381828] [c00ffa54] (n_tty_receive_buf+0x0/0xe70) from
[c00fb38c] (flush_to_ldisc+0xfc/0x184)
[   14.390617] [c00fb290] (flush_to_ldisc+0x0/0x184) from
[c00fb450] (tty_flip_buffer_push+0x3c/0x40)
[   14.399406] [c00fb414] (tty_flip_buffer_push+0x0/0x40) from
[c011188c] (atmel_tasklet_func+0x5ec/0x5fc)
[   14.409172]  r5:1a1b r4:c0212aec
[   14.413078] [c01112a0] (atmel_tasklet_func+0x0/0x5fc) from
[c0035858] (__tasklet_action+0xc8/0x194)
[   14.421867] [c0035790] (__tasklet_action+0x0/0x194) from
[c003599c] (tasklet_action+0x38/0x40)
[   14.431633]  r8:c02e r7:c0209e20 r6:ffdf r5:c0209c20 r4:c0209c20
[   14.437492] [c0035964] (tasklet_action+0x0/0x40) from
[c00363b4] (ksoftirqd+0x178/0x228)
[   14.446281] [c003623c] (ksoftirqd+0x0/0x228) from [c004510c]
(kthread+0x60/0x94)
[   14.454094] [c00450ac] (kthread+0x0/0x94) from [c00336d8]
(do_exit+0x0/0x7e0)
[   14.460930]  r6: r5: r4:


2007/12/19, Haavard Skinnemoen [EMAIL PROTECTED]:
 The following patchset cleans up the atmel_serial driver a bit,
 moves a significant portion of the interrupt handler into a tasklet,
 and adds DMA support. This is the result of a combined effort by Chip
 Coldwell, Remy Bohmer and me. The patches should apply cleanly onto
 Linus' latest git tree.

 With DMA, I see transfer rates around 92 kbps when transferring a big
 file using ZModem (both directions are roughly the same.)

 Note that break and error handling doesn't work too well with DMA
 enabled. This is a common problem with all the efforts I've seen
 adding DMA support to this driver (including my own). The PDC error
 handling also accesses icount without locking. I'm tempted to just
 ignore the problem for now and hopefully come up with a solution
 later.

 Everyone, please give it a try and/or review the code.

 Chip Coldwell (1):
   atmel_serial: Add DMA support

 Haavard Skinnemoen (3):
   atmel_serial: Use cpu_relax() when busy-waiting
   atmel_serial: Use existing console options only if BRG is running
   atmel_serial: Fix bugs in probe() error path and remove()

 Remy Bohmer (2):
   atmel_serial: Clean up the code
   atmel_serial: Split the interrupt handler

  drivers/serial/atmel_serial.c |  938 
 -
  1 files changed, 738 insertions(+), 200 deletions(-)

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/6] atmel_serial: Cleanups, irq handler splitup DMA

[Remy Bohmer]

Hello Haavard,

 Could you try the patch below? It's a bit strange that you got an oops
 though...

It is not really strange... spinlocks are mutexes on preempt-rt, and
recursive mutex locking is not allowed, this is one differences with
the mainline spinlock.

But... I tried that patch, and it works a lot better, no oopses
anymore, but I noticed that I sometimes get an input overrun (ttyS0: 1
input overrun(s) ) during stress conditions.
This is something I did not notice before, maybe it was already there,
or has something changed in this area that it is now more sensitive
for this?

Kind Regards,

Remy

2007/12/19, Haavard Skinnemoen [EMAIL PROTECTED]:
 On Wed, 19 Dec 2007 16:57:04 +0100
 Remy Bohmer [EMAIL PROTECTED] wrote:

  Hello Haavard,
 
  Sorry.. But I get an Oops on Preempt-RT with the latest set of
  patches. I did not see it earlier today with the other set of patches.

 Hmm...from the backtrace, it looks like lock recursion -- port-lock is
 held for the whole duration of the tasklet, but we somehow end up in
 uart_start(), which grabs the lock again.

 Could you try the patch below? It's a bit strange that you got an oops
 though...

 Haavard

 diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
 index 7967054..948c643 100644
 --- a/drivers/serial/atmel_serial.c
 +++ b/drivers/serial/atmel_serial.c
 @@ -666,7 +666,13 @@ static void atmel_rx_from_ring(struct uart_port *port)
 uart_insert_char(port, status, ATMEL_US_OVRE, c.ch, flg);
 }

 +   /*
 +* Drop the lock here since it might end up calling
 +* uart_start(), which takes the lock.
 +*/
 +   spin_unlock(port-lock);
 tty_flip_buffer_push(port-info-tty);
 +   spin_lock(port-lock);
  }

  static void atmel_rx_from_dma(struct uart_port *port)

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/6] atmel_serial: Cleanups, irq handler splitup DMA

[Remy Bohmer]

  but I noticed that I sometimes get an input overrun (ttyS0: 1
  input overrun(s) ) during stress conditions.
  This is something I did not notice before, maybe it was already there,
  or has something changed in this area that it is now more sensitive
  for this?
 Hmm...is this with or without DMA?

DBGU is without DMA.

 If it's without DMA, something very strange is going on -- the non-DMA
 RX code is almost the only thing left in the hardirq handler, so I
 would really expect overruns to be much less likely to occur now than
 before...

As mentioned, maybe it was already there, but I did not run into it
earlier. I have to figure that out. But, at 115200 and a 1 byte
receive-'fifo' on DBGU, and still interrupt locks somewhere in the
tree up to 300us, it is a simple calculation that we can run into
overrun conditions...
Notice that without these interrupt handler splitup, it was much, much
much worse...

So, for me it is not a big deal, because it is just a terminal, and
with my shaky fingers I usually do not type that fast ;-))

Remy

Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[patch 0/3] add kernel-cmdline support for interrupt thread priorities (repost:CC to LKML)

[Remy Bohmer]

Hello All,

Back in August we had a discussion about the default priorities of
IRQ-threads and softirqs, and the lack of a mechanism to configure them from
within the kernel.
(see http://www.mail-archive.com/[EMAIL PROTECTED]/msg01022.html)

Here is a patchset that implements such a nice feature from the kernel cmdline,
similar to what we all agreed on, back then.

The set exist of 3 parts:
1) add a generic routine to the kernel commandline-parse-library to parse the
format of the map as we agreed on.
2) Add a __setup(irq_pmap=) routine to kernel/irq/manage.c
3) Add a __setup(sirq_pmap=) routine to kernel/softirq.c

I hope you can find the time to review this set.


Kind Regards,

Remy Bohmer
-- 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[patch 1/3] Add generic routine for parsing map-like options on kernel cmd-line (repost:CC to LKML)

[Remy Bohmer]

This patch adds a generic routine to the kernel, so that a map of
settings can be entered on the kernel commandline.

Signed-off-by: Remy Bohmer [EMAIL PROTECTED]

---
---
 include/linux/kernel.h |1 
 lib/cmdline.c  |   58 +
 2 files changed, 59 insertions(+)

Index: linux-2.6.24-rc5-rt1/include/linux/kernel.h
===
--- linux-2.6.24-rc5-rt1.orig/include/linux/kernel.h2007-12-18 
21:32:09.0 +0100
+++ linux-2.6.24-rc5-rt1/include/linux/kernel.h 2007-12-18 21:33:58.0 
+0100
@@ -164,6 +164,7 @@ extern int vsscanf(const char *, const c
 
 extern int get_option(char **str, int *pint);
 extern char *get_options(const char *str, int nints, int *ints);
+extern int get_map_option(const char *str, const char *key, int *pint);
 extern unsigned long long memparse(char *ptr, char **retptr);
 
 extern int core_kernel_text(unsigned long addr);
Index: linux-2.6.24-rc5-rt1/lib/cmdline.c
===
--- linux-2.6.24-rc5-rt1.orig/lib/cmdline.c 2007-10-09 22:31:38.0 
+0200
+++ linux-2.6.24-rc5-rt1/lib/cmdline.c  2007-12-18 21:33:58.0 +0100
@@ -15,6 +15,7 @@
 #include linux/module.h
 #include linux/kernel.h
 #include linux/string.h
+#include asm/setup.h
 
 /*
  * If a hyphen was found in get_option, this will handle the
@@ -114,6 +115,63 @@ char *get_options(const char *str, int n
 }
 
 /**
+ * get_map_option - Parse integer from an option map
+ *
+ * This function parses an integer from an option map like
+ *   some_map=key1:99,key2:98,...,keyN:NN,NN
+ * Only the value of the first match is returned, or if no
+ * key option is given (key = NULL) the value of the first
+ * field without a ':' is returned.
+ *
+ * @str: option string
+ * @key: The key inside the map, can be NULL
+ * @pint: (output) integer value parsed from the map @str
+ *
+ * Return values:
+ * 0 - no int in string
+ * 1 - int found
+ */
+int get_map_option(const char *str, const char *key, int *pint)
+{
+   char  buf[COMMAND_LINE_SIZE];
+   char  *p, *substr;
+   int   found = 0;
+
+   /* We must copy the string to the stack, because strsep()
+  changes it.*/
+   strncpy(buf, str, COMMAND_LINE_SIZE);
+   buf[COMMAND_LINE_SIZE-1] = '\0';
+
+   p = buf;
+   substr = strsep(p, ,);
+   while ((!found)  (substr != NULL)) {
+   if (strlen(substr) != 0) {
+   if (key == NULL) {
+   /* Check for the absence of any ':' */
+   if (strchr(substr, ':') == NULL) {
+   sscanf(substr, %d, pint);
+   found = 1;
+   }
+   } else {
+   /* check if the first part of the key matches */
+   if (!strncmp(substr, key, strlen(key))) {
+   substr += strlen(key);
+   /* Now the next char must be a ':',
+  if not, search for the next match */
+   if (*substr == ':') {
+   substr++;
+   sscanf(substr, %d, pint);
+   found = 1;
+   }
+   }
+   }
+   }
+   substr = strsep(p, ,);
+   }
+   return found;
+}
+
+/**
  * memparse - parse a string with mem suffixes into a number
  * @ptr: Where parse begins
  * @retptr: (output) Pointer to next char after parse completes

-- 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[patch 2/3] Enable setting of IRQ-thread priorities from kernel cmdline (repost:CC to LKML)

[Remy Bohmer]

The RT-patch originally creates all its IRQ-threads at priority 50.
Of course, this is not a usable default for many realtime systems
and therefor these priorities has to be tuneable for each RT-system.
But, currently there is no way within the kernel to adjust this to
the needs of the user. Some scripts are floating around to do this
from userspace, but for several applications the priorities should
already be set properly by the kernel itself before userland is
started.

This patch changes this by adding a kernel cmd-line option that can
handle a map of priorities.
It is based on the name of the driver that requests an interrupt, it
does NOT use the IRQ-ID to prioritize a certain IRQ thread.
This is designed this way because across several hardware-boards the
IRQ-IDs can change, but not necessarily the drivers 
(interrupt handlers) name.


Remarks:
* There is a check for conflicts added, for situations where 
  IRQ-sharing is detected with different priorities requested
  on the kernel cmd-line.
* Priorities are only set at creation time of the IRQ-thread.
* If userland overrules, it is NOT restored by this code.
* if no new kernel cmdline options are given, the kernel works
  as before, and all IRQ-threads start at 50.
* No wildcards are supported on the cmdline.

See kernel/Documentation/kernel-parameters.txt for usage info.

Signed-off-by: Remy Bohmer [EMAIL PROTECTED]

---
 Documentation/kernel-parameters.txt |7 ++
 include/linux/irq.h |1 
 kernel/irq/manage.c |  101 
 3 files changed, 98 insertions(+), 11 deletions(-)

Index: linux-2.6.24-rc5-rt1/Documentation/kernel-parameters.txt
===
--- linux-2.6.24-rc5-rt1.orig/Documentation/kernel-parameters.txt   
2007-12-18 22:09:13.0 +0100
+++ linux-2.6.24-rc5-rt1/Documentation/kernel-parameters.txt2007-12-18 
22:09:16.0 +0100
@@ -799,6 +799,13 @@ and is between 256 and 4096 characters. 
ips=[HW,SCSI] Adaptec / IBM ServeRAID controller
See header of drivers/scsi/ips.c.
 
+   irq_pmap=   [IRQ-threading] List of priorities each interrupt
+   thread must have.
+   Format: irq_pmap=megasas:90,eth0:40,50
+   The first field without ':', is the default prio.
+   If this cmdline argument is ommitted, every thread
+   runs at prio 50.
+
ports=  [IP_VS_FTP] IPVS ftp helper module
Default is 21.
Up to 8 (IP_VS_APP_MAX_PORTS) ports
Index: linux-2.6.24-rc5-rt1/include/linux/irq.h
===
--- linux-2.6.24-rc5-rt1.orig/include/linux/irq.h   2007-12-18 
22:09:13.0 +0100
+++ linux-2.6.24-rc5-rt1/include/linux/irq.h2007-12-18 22:09:16.0 
+0100
@@ -169,6 +169,7 @@ struct irq_desc {
unsigned intirqs_unhandled;
unsigned long   last_unhandled; /* Aging timer for unhandled 
count */
struct task_struct  *thread;
+   int rt_prio;
wait_queue_head_t   wait_for_handler;
cycles_ttimestamp;
raw_spinlock_t  lock;
Index: linux-2.6.24-rc5-rt1/kernel/irq/manage.c
===
--- linux-2.6.24-rc5-rt1.orig/kernel/irq/manage.c   2007-12-18 
22:09:13.0 +0100
+++ linux-2.6.24-rc5-rt1/kernel/irq/manage.c2007-12-18 22:15:43.0 
+0100
@@ -13,9 +13,19 @@
 #include linux/kthread.h
 #include linux/syscalls.h
 #include linux/interrupt.h
+#include linux/list.h
 
 #include internals.h
 
+
+#ifdef CONFIG_PREEMPT_HARDIRQS
+
+static char *cmdline;
+static int  default_prio = MAX_USER_RT_PRIO/2;
+
+#endif
+
+
 #ifdef CONFIG_SMP
 
 /**
@@ -260,7 +270,7 @@ void recalculate_desc_flags(struct irq_d
desc-status |= IRQ_NODELAY;
 }
 
-static int start_irq_thread(int irq, struct irq_desc *desc);
+static int start_irq_thread(int irq, struct irq_desc *desc, int prio);
 
 /*
  * Internal function that tells the architecture code whether a
@@ -293,6 +303,8 @@ void compat_irq_chip_set_default_handler
desc-handle_irq = NULL;
 }
 
+static int get_irq_prio(const char *name);
+
 /*
  * Internal function to register an irqaction - typically used to
  * allocate special interrupts that are part of the architecture.
@@ -328,7 +340,7 @@ int setup_irq(unsigned int irq, struct i
}
 
if (!(new-flags  IRQF_NODELAY))
-   if (start_irq_thread(irq, desc))
+   if (start_irq_thread(irq, desc, get_irq_prio(new-name)))
return -ENOMEM;
/*
 * The following block of code has to be executed atomically
@@ -811,11 +823,7 @@ static int do_irqd(void * __desc)
 #endif

[patch 3/3] Enable setting of IRQ-thread priorities from kernel cmdline. (repost:CC to LKML)

[Remy Bohmer]

The RT-patch originally creates all its softirq-threads at
priority 50. Of course, this is not a usable default for many
realtime systems and therefor these priorities has to be tuneable
for each RT-system. But, currently there is no way within the
kernel to adjust this to the needs of the user. Some scripts
are floating around to do this from userspace, but for several
applications the priorities should already be set properly by
the kernel itself before userland is started.

This patch changes this by adding a kernel cmd-line option that
can handle a map of priorities.

Remarks:
* Priorities are only set at creation time of the softirq.
* Priorities has to be set per-cpu.
* If userland overrules, it is NOT restored by this code.
* if no new kernel cmdline options are given, the kernel works
  as before, and all softirqs start at 50.
* No wildcards are supported on the cmdline.

See kernel/Documentation/kernel-parameters.txt for usage info.

Signed-off-by: Remy Bohmer [EMAIL PROTECTED]

---
 Documentation/kernel-parameters.txt |   11 +++
 kernel/softirq.c|  105 ++--
 2 files changed, 99 insertions(+), 17 deletions(-)

Index: linux-2.6.24-rc5-rt1/Documentation/kernel-parameters.txt
===
--- linux-2.6.24-rc5-rt1.orig/Documentation/kernel-parameters.txt   
2007-12-18 22:06:11.0 +0100
+++ linux-2.6.24-rc5-rt1/Documentation/kernel-parameters.txt2007-12-18 
22:07:19.0 +0100
@@ -806,6 +806,17 @@ and is between 256 and 4096 characters. 
If this cmdline argument is ommitted, every thread
runs at prio 50.
 
+   sirq_pmap=  [IRQ-threading] List of priorities each softirq
+   thread must have.
+   Format: sirq_pmap=block/0:90,sched/0:75,50
+   The priorities have to be specified per-cpu.
+   The first field without ':', is the default prio.
+   The names have to match the softirq_names[] table in
+   kernel/softirq.c, (thus without 'softirq-' prefix) to
+   keep the cmd-line short.
+   If this cmdline argument is ommitted, every softirq
+   runs at prio 50.
+
ports=  [IP_VS_FTP] IPVS ftp helper module
Default is 21.
Up to 8 (IP_VS_APP_MAX_PORTS) ports
Index: linux-2.6.24-rc5-rt1/kernel/softirq.c
===
--- linux-2.6.24-rc5-rt1.orig/kernel/softirq.c  2007-12-18 22:06:11.0 
+0100
+++ linux-2.6.24-rc5-rt1/kernel/softirq.c   2007-12-18 22:08:54.0 
+0100
@@ -66,6 +66,10 @@ struct softirqdata {
 
 static DEFINE_PER_CPU(struct softirqdata [MAX_SOFTIRQ], ksoftirqd);
 
+static char *cmdline;
+static int  default_prio = MAX_USER_RT_PRIO/2;
+
+
 #ifdef CONFIG_PREEMPT_SOFTIRQS
 /*
  * Preempting the softirq causes cases that would not be a
@@ -770,10 +774,28 @@ EXPORT_SYMBOL(tasklet_unlock_wait);
 
 #endif
 
+static const char *softirq_names [] =
+{
+  [HI_SOFTIRQ] = high,
+  [SCHED_SOFTIRQ]  = sched,
+  [TIMER_SOFTIRQ]  = timer,
+  [NET_TX_SOFTIRQ] = net-tx,
+  [NET_RX_SOFTIRQ] = net-rx,
+  [BLOCK_SOFTIRQ]  = block,
+  [TASKLET_SOFTIRQ]= tasklet,
+#ifdef CONFIG_HIGH_RES_TIMERS
+  [HRTIMER_SOFTIRQ]= hrtimer,
+#endif
+  [RCU_SOFTIRQ]= rcu,
+};
+
+static int get_softirq_prio(const char *name);
+
 static int ksoftirqd(void * __data)
 {
-   struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO/2 };
+   struct sched_param param = { 0, };
struct softirqdata *data = __data;
+   char buf[50];
u32 softirq_mask = (1  data-nr);
struct softirq_action *h;
int cpu = data-cpu;
@@ -781,8 +803,12 @@ static int ksoftirqd(void * __data)
 #ifdef CONFIG_PREEMPT_SOFTIRQS
init_waitqueue_head(data-wait);
 #endif
-
+   /* Lookup the priority of this softirq, and set the prio accordingly */
+   snprintf(buf, sizeof(buf), %s/%lu,
+softirq_names[data-nr], data-cpu);
+   param.sched_priority = get_softirq_prio(buf);
sys_sched_setscheduler(current-pid, SCHED_FIFO, param);
+
current-flags |= PF_SOFTIRQ;
set_current_state(TASK_INTERRUPTIBLE);
 
@@ -911,21 +937,6 @@ void takeover_tasklets(unsigned int cpu)
 }
 #endif /* CONFIG_HOTPLUG_CPU */
 
-static const char *softirq_names [] =
-{
-  [HI_SOFTIRQ] = high,
-  [SCHED_SOFTIRQ]  = sched,
-  [TIMER_SOFTIRQ]  = timer,
-  [NET_TX_SOFTIRQ] = net-tx,
-  [NET_RX_SOFTIRQ] = net-rx,
-  [BLOCK_SOFTIRQ]  = block,
-  [TASKLET_SOFTIRQ]= tasklet,
-#ifdef CONFIG_HIGH_RES_TIMERS
-  [HRTIMER_SOFTIRQ]= hrtimer,
-#endif
-  [RCU_SOFTIRQ]= rcu,
-};
-
 static int __cpuinit cpu_callback(struct notifier_block

Re: [PATCH v2 0/6] atmel_serial: Cleanups, irq handler splitup DMA

[Remy Bohmer]

Hello,

 preempt-rt can disable interrupts for 300 us?

I am not sure if it really is an interrupt lock that long, but on
these ARM cores I still run into latencies that large. I tried using
latency_trace to figure out where they come from, but that tool even
reports interrupt locks up to 10 ms while those surely do not exist
(the 2nd in size reported is about 300us). So, those tools are buggy
on AT91 as well, and I do not trust them completely yet.. (They even
do not compile cleanly, conflicts in headers, warnings etc. Maybe
those tools are not used very much outside x86)

So, I still have a long way to go to make Preempt-RT to run on these
cores just as good as preempt-rt runs on X86, although I am sure that
I never will reach the 30us as I have on x86.

 If so, then I guess there's really no way to avoid a few overruns.

And if DBGU is used as a terminal, it should be no problem at all, or
someone has to slow down the baudrate a bit.
I only get this overrun under very heavy system load (CPU load 99%,
with one single thread running at RT-prio 99, taking chunks away of
99/100ms.) and then throw in a big file of many MBs in the terminal.
But, as long as the read part still runs in hard-irq context, it
should keep up, unless interrupts are locked too long.

  Notice that without these interrupt handler splitup, it was much, much
  much worse...

 Ok, that's good I guess.

Yep, that _is_ good.

  So, for me it is not a big deal, because it is just a terminal, and
  with my shaky fingers I usually do not type that fast ;-))
 If you do, you just need to switch to one of the USARTs instead of the
 DBGU so that you can use DMA :-)

:-)

 We need to fix the break- and error handling though. But my vacation
 starts tomorrow, so I probably won't be able to fix it until next year.

In that case, I wish you very good holiday, and a happy new-year.
It was very pleasant working with you on this driver.

I probably see you again on the list next year.


Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 1/3] Add generic routine for parsing map-like options on kernel cmd-line (repost:CC to LKML)

[Remy Bohmer]

Hello Randy,

Sorry for the language errors, English is not my Native language, so I
make these stupid errors...

  + *   get_map_option - Parse integer from an option map
 The @param lines (below) need to go here, immediately following the
 function short description (the line above).  No intervening blank
 lines.

OK, I will adapt that.

  +int get_map_option(const char *str, const char *key, int *pint)
  +{
  + char  buf[COMMAND_LINE_SIZE];

 COMMAND_LINE_SIZE varies from 256 to 2048, depending on $ARCH.
 That's a bit too much to declare on a function's local stack --
 unless you are very certain of the call tree to this point and
 that the total stack size is safe.  Can you just kmalloc() this
 buf?

I know it is big on a 4k stack, and I also think it is not very nice...
But kmalloc() panics the kernel if I do it as soon as in the __setup() code.

The problem is that the original string may not be modified by the
cmdline-parser, and I do not know the length of the command up front,
(except that it cannot be longer than this define). Allocating a
global buffer is not safe and needs locking. So actually only what
left for me was the stack... or rewrite the used C-library-routines
completely myself, for this purpose only, which is also not nice.

I hope someone could point to me to another possibilty, that I did not
think of yet.


Kind Regards,

Remy


  + char  *p, *substr;
  + int   found = 0;
  +
  + /* We must copy the string to the stack, because strsep()
  +changes it.*/
  + strncpy(buf, str, COMMAND_LINE_SIZE);
  + buf[COMMAND_LINE_SIZE-1] = '\0';
  +
  + p = buf;
  + substr = strsep(p, ,);
  + while ((!found)  (substr != NULL)) {
  + if (strlen(substr) != 0) {
  + if (key == NULL) {
  + /* Check for the absence of any ':' */
  + if (strchr(substr, ':') == NULL) {
  + sscanf(substr, %d, pint);
  + found = 1;
  + }
  + } else {
  + /* check if the first part of the key matches 
  */
  + if (!strncmp(substr, key, strlen(key))) {
  + substr += strlen(key);
  + /* Now the next char must be a ':',
  +if not, search for the next match 
  */
  + if (*substr == ':') {
  + substr++;
  + sscanf(substr, %d, pint);
  + found = 1;
  + }
  + }
  + }
  + }
  + substr = strsep(p, ,);
  + }
  + return found;
  +}

 ---
 ~Randy

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

> > BTW: Attached I have added a 2nd patch that I use for Preempt-RT. (For
> > cleaner startup, and to get rid of useless IRQ-threads.
>
> Hrm. That assumption isn't valid on AVR32...on AP7000, for example,
> IRQ1 is used by the LCD controller.

In that case, forget that (dirty) patch completely for now. It does
not break things on Preempt-RT, we only will have a IRQ-1 kernel
thread that is never scheduled. (Of which I think is actually a
irq-manage bug, if there are no handlers that are scheduled on that
thread, it should not even start the thread.)

I will verify/test the complete patchset tomorrow.

Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

A few remarks:

> From: Remy Bohmer <[EMAIL PROTECTED]>

My name, at your address ;-)))

> This patch splits up the interrupt handler of the serial port
> into a interrupt top-half and a tasklet.

I see you moved the handling of the sysrq-key to the tasklet. This was
actually a very nice feature in the IRQ-top half on preempt-RT. This
helps debugging running away RT-processes.

> In this version of the patch, we try to only do things that are
> absolutely necessary in the interrupt handler, storing away the
> status register along with the received character and letting the
> tasklet handle break, sysrq, error flags, etc.

Preempt-RT now absolutely requires my (4th) IRQ_NODELAY patch, because
the spinlock now is always inside the code, and not only in
theexception path, and thus without my NO_DELAY patch we have a panic
during boot.
On preempt-RT this spinlock must be a raw-spinlock. (If this type is
known in the mainline kernel, you can apply that patch it anyway)

BTW: Attached I have added a 2nd patch that I use for Preempt-RT. (For
cleaner startup, and to get rid of useless IRQ-threads.

> This patch should apply on top of the cleanup patch I sent earlier

For the cleanup patch:
Acked-by: Remy Bohmer <[EMAIL PROTECTED]>

> today. Or at least I think so...I'll send the full series once
> everyone are happy.

So, for this patch: I am almost happy ;-)


Kind Regards,

Remy
This patch prevents starting up a useless IRQ-thread for IRQ-1,
because this IRQ is running in IRQF_NODELAY context due to the 
timer interrupt.

Signed-off-by: Remy Bohmer <[EMAIL PROTECTED]>
---
 drivers/serial/atmel_serial.c |   10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

Index: linux-2.6.23/drivers/serial/atmel_serial.c
===
--- linux-2.6.23.orig/drivers/serial/atmel_serial.c	2007-12-18 15:49:02.0 +0100
+++ linux-2.6.23/drivers/serial/atmel_serial.c	2007-12-18 15:56:37.0 +0100
@@ -549,6 +549,7 @@ static int atmel_startup(struct uart_por
 {
 	struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
 	int retval;
+	unsigned long irqflags = IRQF_SHARED;
 
 	/*
 	 * Ensure that no interrupts are enabled otherwise when
@@ -557,10 +558,17 @@ static int atmel_startup(struct uart_por
 	 */
 	UART_PUT_IDR(port, -1);
 
+#ifdef CONFIG_PREEMPT_RT
+	/* IRQ1 is the System IRQ, which is shared with the Timer-IRQ, and
+	   thus it is running in IRQF_NODELAY context. By setting this flag
+	   here also, we prevent starting up a useless IRQ-thread.*/
+	if (port->irq == 1)
+		irqflags |= IRQF_NODELAY;
+#endif
 	/*
 	 * Allocate the IRQ
 	 */
-	retval = request_irq(port->irq, atmel_interrupt, IRQF_SHARED,
+	retval = request_irq(port->irq, atmel_interrupt, irqflags,
 			"atmel_serial", port);
 	if (retval) {
 		printk("atmel_serial: atmel_startup - Can't get irq\n");

Re: [PATCH]: Atmel Serial Console interrupt handler splitup

[Remy Bohmer]

Hello Haavard,

> I don't think so, but I don't feel all that strongly about it. I'd
> actually prefer if we used at_writel() and at_readl() throughout the
> code and killed those UART_PUT/UART_GET macros.

I completely agree.

Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] atmel_serial: Clean up the code

[Remy Bohmer]

Hello Haavard,

> Please note that I'm not trying to steal the show here -- I just want

That did not even come to my mind at all...
I am happy with everything that helps making this driver better.

What shall we do first from here, splitup of the interrupt handler? Or
DMA patch?


Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] atmel_serial: Clean up the code

[Remy Bohmer]

Hello Haavard,

 Please note that I'm not trying to steal the show here -- I just want

That did not even come to my mind at all...
I am happy with everything that helps making this driver better.

What shall we do first from here, splitup of the interrupt handler? Or
DMA patch?


Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH]: Atmel Serial Console interrupt handler splitup

[Remy Bohmer]

Hello Haavard,

 I don't think so, but I don't feel all that strongly about it. I'd
 actually prefer if we used at_writel() and at_readl() throughout the
 code and killed those UART_PUT/UART_GET macros.

I completely agree.

Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

A few remarks:

 From: Remy Bohmer [EMAIL PROTECTED]

My name, at your address ;-)))

 This patch splits up the interrupt handler of the serial port
 into a interrupt top-half and a tasklet.

I see you moved the handling of the sysrq-key to the tasklet. This was
actually a very nice feature in the IRQ-top half on preempt-RT. This
helps debugging running away RT-processes.

 In this version of the patch, we try to only do things that are
 absolutely necessary in the interrupt handler, storing away the
 status register along with the received character and letting the
 tasklet handle break, sysrq, error flags, etc.

Preempt-RT now absolutely requires my (4th) IRQ_NODELAY patch, because
the spinlock now is always inside the code, and not only in
theexception path, and thus without my NO_DELAY patch we have a panic
during boot.
On preempt-RT this spinlock must be a raw-spinlock. (If this type is
known in the mainline kernel, you can apply that patch it anyway)

BTW: Attached I have added a 2nd patch that I use for Preempt-RT. (For
cleaner startup, and to get rid of useless IRQ-threads.

 This patch should apply on top of the cleanup patch I sent earlier

For the cleanup patch:
Acked-by: Remy Bohmer [EMAIL PROTECTED]

 today. Or at least I think so...I'll send the full series once
 everyone are happy.

So, for this patch: I am almost happy ;-)


Kind Regards,

Remy
This patch prevents starting up a useless IRQ-thread for IRQ-1,
because this IRQ is running in IRQF_NODELAY context due to the 
timer interrupt.

Signed-off-by: Remy Bohmer [EMAIL PROTECTED]
---
 drivers/serial/atmel_serial.c |   10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

Index: linux-2.6.23/drivers/serial/atmel_serial.c
===
--- linux-2.6.23.orig/drivers/serial/atmel_serial.c	2007-12-18 15:49:02.0 +0100
+++ linux-2.6.23/drivers/serial/atmel_serial.c	2007-12-18 15:56:37.0 +0100
@@ -549,6 +549,7 @@ static int atmel_startup(struct uart_por
 {
 	struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
 	int retval;
+	unsigned long irqflags = IRQF_SHARED;
 
 	/*
 	 * Ensure that no interrupts are enabled otherwise when
@@ -557,10 +558,17 @@ static int atmel_startup(struct uart_por
 	 */
 	UART_PUT_IDR(port, -1);
 
+#ifdef CONFIG_PREEMPT_RT
+	/* IRQ1 is the System IRQ, which is shared with the Timer-IRQ, and
+	   thus it is running in IRQF_NODELAY context. By setting this flag
+	   here also, we prevent starting up a useless IRQ-thread.*/
+	if (port-irq == 1)
+		irqflags |= IRQF_NODELAY;
+#endif
 	/*
 	 * Allocate the IRQ
 	 */
-	retval = request_irq(port-irq, atmel_interrupt, IRQF_SHARED,
+	retval = request_irq(port-irq, atmel_interrupt, irqflags,
 			atmel_serial, port);
 	if (retval) {
 		printk(atmel_serial: atmel_startup - Can't get irq\n);

Re: [PATCH] atmel_serial: Split the interrupt handler

[Remy Bohmer]

Hello Haavard,

  BTW: Attached I have added a 2nd patch that I use for Preempt-RT. (For
  cleaner startup, and to get rid of useless IRQ-threads.

 Hrm. That assumption isn't valid on AVR32...on AP7000, for example,
 IRQ1 is used by the LCD controller.

In that case, forget that (dirty) patch completely for now. It does
not break things on Preempt-RT, we only will have a IRQ-1 kernel
thread that is never scheduled. (Of which I think is actually a
irq-manage bug, if there are no handlers that are scheduled on that
thread, it should not even start the thread.)

I will verify/test the complete patchset tomorrow.

Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH]: Atmel Serial Console interrupt handler splitup

[Remy Bohmer]

Hello Haavard,

> > Yep. All calls that block on a Mutex somehow on Preempt-RT. (such as
> > spinlocks, wakeup_interruptible() and many of its friends.)
> Right. Looks like the DMA patch call these functions from irq context
> too...I guess it'll need the same treatment?

That is correct. DMA code does do that, but the DMA code is not used
for DBGU, and _only_ the interrupt handler of DBGU runs in
IRQF_NODELAY context (because it is part of the System-IRQ and thus
shared with the timer-irq). The DMA code always runs in IRQ-thread
context, where it is safe to block on a mutex.
So, it is not mandatory to change that also. (It may be nice to do it anyway)

> > > > +struct atmel_uart_char {
> > > > + unsigned int status;
> > > > + unsigned int overrun;
> > > > + unsigned int ch;
> > > > + unsigned int flg;
> > > > +};
> > >
> > > Hmm. 16 bytes per char is a bit excessive, isn't it? How about
> > >
> > > struct atmel_uart_char {
> > > u16 ch;
> > > u16 status;
> > > };
> > >
> > > where ch is the received character (up to 9 bits) and status is the
> > > lowest 16 bits of the status register?
> > I used the NODELAY patch for the 8250 serial port as reference, it
> > contains similar code, and I tried to make both look the same.
> Ok, I see. But...
> > > > +#define ATMEL_SERIAL_RINGSIZE 1024
> This means that the buffer ends up at 16K. Since the buffer itself is
> kept in a struct along with a few other pieces of data, we end up
> kmalloc()ing 32KB of memory...

Oops... 32kB on X86 is not much, relatively, on ARM it is somewhat different.
We can also decrease the total RingSize, 128 would be good enough also.
I can look at it later this week.

> I'm a bit tempted to just go ahead and do the changes we agree on and
> post the result. Then I'll see if I can make the DMA stuff behave. I've
> got a different driver lying around which is DMA-only and has a few
> problems on its own.

Beware that, according to several older discussions, DBGU cannot make
use of DMA... (If I understood it correctly, it was because the buffer
first have to be full, before an interupt is generated. That would be
very annoying while typing 1 character at the time ;-)

> Also, I think the DMA patch needs to be more integrated with your
> "interrupt handler splitup" patch. No point in keeping two RX buffers
> around, and the DMA code needs to obey the same rules as the rest of
> the driver if it's going to work in -rt.

As mentioned, not necessarily...

> And I'd really like to have working DMA support on avr32 before
> christmas ;-)

Me too ;-)


Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH]: Atmel Serial Console interrupt handler splitup

[Remy Bohmer]

Hello Haavard,

> I'll give it a shot, but first I have some comments on your other
> patches.

Good news someone is working on this bug again. Also good news you
already found a bug in there.

> Btw, it would be nice if patches that affect more or less
> architecture-independent drivers were posted to linux-kernel (added to
> Cc.)

Not really architecture independant, I believe, because thos are
drivers for peripherals _inside_ Atmel Cores ;-)

> Also, it would be easier to review if you posted just one patch per
> e-mail. I'm going to cut & paste a bit from your attachments.

I know, but I have some troubles to get 'quilt mail' to work from
behind a proxy server, and attaching to a mail, at least it does not
corrupt the contents of the patches.

> > 4) For RT only: atmel_serial_irqf_nodelay.patch can be applied
> > anywhere after 1 and 2
>
> I'll ignore this for now.

OK.

> > +#define lread(port)  __raw_readl(port)
> > +#define lwrite(v, port)  __raw_writel(v, port)
>
> Why is this necessary, and what does 'l' stand for?

There is a huge list of macros below these definitions. By doing it
this way, the macros still fit on 80 characters wide, while without
them, I had split up the macros over several lines, which does not
make it more readable. That's all.
'l' refers at the last letter of __raw_readl, and writel. Nothing special.

>
> > - struct uart_portuart;   /* uart */
> > - struct clk  *clk;   /* uart clock */
> > - unsigned short  suspended;  /* is port suspended? */
> > - int break_active;   /* break being received */
> > + struct uart_portuart;   /* uart */
> > + struct clk  *clk;   /* uart clock */
> > + unsigned short  suspended;  /* is port suspended? */
> > + int break_active;   /* break being received */
>
> Looks like you're adding one or more spaces before each TAB here. Why
> is that an improvement?

I used scripts/Lindent to reformat the file, and then I removed the
quirks Lindent put in the file. Apparantly I missed that one.

> These conflict with David Brownell's "atmel_serial build warnings
> begone" patch which was merged into mainline a few weeks ago.

Hmm, I seem to have missed that one. Why is it not there in a
big-AT91-patch from Andrew?

> >  /*
> > + * receive interrupt handler.
> > + */
> > +static inline void
> > +atmel_handle_receive(struct uart_port *port, unsigned int pending)
>
> Please drop "inline" here. The compiler will do it automatically if it
> has only one caller, and if it at some point gets several callers, we
> might not want to inline it after all.

Funny, This was the first thing that Andrew started complaining about.
He suggested to put an inline there which I had not. I already
mentioned that this was against the CodingStyle, but I also mentioned
that I did not wanted to start a fight about this :-)
So, to prevent a discussion, I added the inline...

> > @@ -422,7 +454,9 @@ static int atmel_startup(struct uart_por
> >   /*
> >* Allocate the IRQ
> >*/
> > - retval = request_irq(port->irq, atmel_interrupt, IRQF_SHARED, 
> > "atmel_serial", port);
> > + retval =
> > + request_irq(port->irq, atmel_interrupt, IRQF_SHARED,
> > + "atmel_serial", port);
>
> I think request_irq() belongs on the same line as "retval =".

I blame scripts/Lindent ;-)

> Please use TABs, not spaces. Might as well remove those comments...they
> don't seem all that useful.

Go ahead...

I did not remove any comment, even if they appear useless to me. I am
not the maintainer of this driver, and just wanted to improve it, so
that it was able of running on Preempt-RT.
Before being able to submit the change that really mattered to me, I
had to make the driver pass the scripts/checkpatch.pl check, otherwise
the patch-that-matters would be completely unreadable.

>
> > + while (!(UART_GET_CSR(port) & ATMEL_US_TXEMPTY))
> > + barrier();
>
> Should probably use cpu_relax(), but it's probably out of scope for a
> codingstyle cleanup patch (and I don't think it matters on AVR32 or
> ARM.)

Agree.

> >   /*
> > -  *  First, save IMR and then disable interrupts
> > +  *  First, save IMR and then disable interrupts
> >*/
> >   imr = UART_GET_IMR(port);   /* get interrupt mask */
> >   UART_PUT_IDR(port, ATMEL_US_RXRDY | ATMEL_US_TXRDY);
> > @@ -790,30 +828,32 @@ static void atmel_console_write(struct c
> >   uart_console_write(port, s, count, atmel_console_putchar);
> >
> >   /*
> > -  *  Finally, wait for transmitter to become empty
> > -  *  and restore IMR
> > +  *  Finally, wait for transmitter to become empty
> > +  *  and restore IMR
> >*/
>
> Looks like you're replacing TABs with spaces. Why?



> > -// TODO: CR is a write-only register
> > -//   

Re: [PATCH]: Atmel Serial Console interrupt handler splitup

[Remy Bohmer]

Hello Haavard,

 I'll give it a shot, but first I have some comments on your other
 patches.

Good news someone is working on this bug again. Also good news you
already found a bug in there.

 Btw, it would be nice if patches that affect more or less
 architecture-independent drivers were posted to linux-kernel (added to
 Cc.)

Not really architecture independant, I believe, because thos are
drivers for peripherals _inside_ Atmel Cores ;-)

 Also, it would be easier to review if you posted just one patch per
 e-mail. I'm going to cut  paste a bit from your attachments.

I know, but I have some troubles to get 'quilt mail' to work from
behind a proxy server, and attaching to a mail, at least it does not
corrupt the contents of the patches.

  4) For RT only: atmel_serial_irqf_nodelay.patch can be applied
  anywhere after 1 and 2

 I'll ignore this for now.

OK.

  +#define lread(port)  __raw_readl(port)
  +#define lwrite(v, port)  __raw_writel(v, port)

 Why is this necessary, and what does 'l' stand for?

There is a huge list of macros below these definitions. By doing it
this way, the macros still fit on 80 characters wide, while without
them, I had split up the macros over several lines, which does not
make it more readable. That's all.
'l' refers at the last letter of __raw_readl, and writel. Nothing special.


  - struct uart_portuart;   /* uart */
  - struct clk  *clk;   /* uart clock */
  - unsigned short  suspended;  /* is port suspended? */
  - int break_active;   /* break being received */
  + struct uart_portuart;   /* uart */
  + struct clk  *clk;   /* uart clock */
  + unsigned short  suspended;  /* is port suspended? */
  + int break_active;   /* break being received */

 Looks like you're adding one or more spaces before each TAB here. Why
 is that an improvement?

I used scripts/Lindent to reformat the file, and then I removed the
quirks Lindent put in the file. Apparantly I missed that one.

 These conflict with David Brownell's atmel_serial build warnings
 begone patch which was merged into mainline a few weeks ago.

Hmm, I seem to have missed that one. Why is it not there in a
big-AT91-patch from Andrew?

   /*
  + * receive interrupt handler.
  + */
  +static inline void
  +atmel_handle_receive(struct uart_port *port, unsigned int pending)

 Please drop inline here. The compiler will do it automatically if it
 has only one caller, and if it at some point gets several callers, we
 might not want to inline it after all.

Funny, This was the first thing that Andrew started complaining about.
He suggested to put an inline there which I had not. I already
mentioned that this was against the CodingStyle, but I also mentioned
that I did not wanted to start a fight about this :-)
So, to prevent a discussion, I added the inline...

  @@ -422,7 +454,9 @@ static int atmel_startup(struct uart_por
/*
 * Allocate the IRQ
 */
  - retval = request_irq(port-irq, atmel_interrupt, IRQF_SHARED, 
  atmel_serial, port);
  + retval =
  + request_irq(port-irq, atmel_interrupt, IRQF_SHARED,
  + atmel_serial, port);

 I think request_irq() belongs on the same line as retval =.

I blame scripts/Lindent ;-)

 Please use TABs, not spaces. Might as well remove those comments...they
 don't seem all that useful.

Go ahead...

I did not remove any comment, even if they appear useless to me. I am
not the maintainer of this driver, and just wanted to improve it, so
that it was able of running on Preempt-RT.
Before being able to submit the change that really mattered to me, I
had to make the driver pass the scripts/checkpatch.pl check, otherwise
the patch-that-matters would be completely unreadable.


  + while (!(UART_GET_CSR(port)  ATMEL_US_TXEMPTY))
  + barrier();

 Should probably use cpu_relax(), but it's probably out of scope for a
 codingstyle cleanup patch (and I don't think it matters on AVR32 or
 ARM.)

Agree.

/*
  -  *  First, save IMR and then disable interrupts
  +  *  First, save IMR and then disable interrupts
 */
imr = UART_GET_IMR(port);   /* get interrupt mask */
UART_PUT_IDR(port, ATMEL_US_RXRDY | ATMEL_US_TXRDY);
  @@ -790,30 +828,32 @@ static void atmel_console_write(struct c
uart_console_write(port, s, count, atmel_console_putchar);
 
/*
  -  *  Finally, wait for transmitter to become empty
  -  *  and restore IMR
  +  *  Finally, wait for transmitter to become empty
  +  *  and restore IMR
 */

 Looks like you're replacing TABs with spaces. Why?



  -// TODO: CR is a write-only register
  -//   unsigned int cr;
  +/* TODO: CR is a write-only register
  +//  unsigned int cr;
   //
  -//   cr = UART_GET_CR(port)  

Re: [PATCH]: Atmel Serial Console interrupt handler splitup

[Remy Bohmer]

Hello Haavard,

  Yep. All calls that block on a Mutex somehow on Preempt-RT. (such as
  spinlocks, wakeup_interruptible() and many of its friends.)
 Right. Looks like the DMA patch call these functions from irq context
 too...I guess it'll need the same treatment?

That is correct. DMA code does do that, but the DMA code is not used
for DBGU, and _only_ the interrupt handler of DBGU runs in
IRQF_NODELAY context (because it is part of the System-IRQ and thus
shared with the timer-irq). The DMA code always runs in IRQ-thread
context, where it is safe to block on a mutex.
So, it is not mandatory to change that also. (It may be nice to do it anyway)

+struct atmel_uart_char {
+ unsigned int status;
+ unsigned int overrun;
+ unsigned int ch;
+ unsigned int flg;
+};
  
   Hmm. 16 bytes per char is a bit excessive, isn't it? How about
  
   struct atmel_uart_char {
   u16 ch;
   u16 status;
   };
  
   where ch is the received character (up to 9 bits) and status is the
   lowest 16 bits of the status register?
  I used the NODELAY patch for the 8250 serial port as reference, it
  contains similar code, and I tried to make both look the same.
 Ok, I see. But...
+#define ATMEL_SERIAL_RINGSIZE 1024
 This means that the buffer ends up at 16K. Since the buffer itself is
 kept in a struct along with a few other pieces of data, we end up
 kmalloc()ing 32KB of memory...

Oops... 32kB on X86 is not much, relatively, on ARM it is somewhat different.
We can also decrease the total RingSize, 128 would be good enough also.
I can look at it later this week.

 I'm a bit tempted to just go ahead and do the changes we agree on and
 post the result. Then I'll see if I can make the DMA stuff behave. I've
 got a different driver lying around which is DMA-only and has a few
 problems on its own.

Beware that, according to several older discussions, DBGU cannot make
use of DMA... (If I understood it correctly, it was because the buffer
first have to be full, before an interupt is generated. That would be
very annoying while typing 1 character at the time ;-)

 Also, I think the DMA patch needs to be more integrated with your
 interrupt handler splitup patch. No point in keeping two RX buffers
 around, and the DMA code needs to obey the same rules as the rest of
 the driver if it's going to work in -rt.

As mentioned, not necessarily...

 And I'd really like to have working DMA support on avr32 before
 christmas ;-)

Me too ;-)


Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Revert lazy irq disable for simple irqs

[Remy Bohmer]

Hello Steven,

> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21 lazy irq disabling
> was implemented, and the simple irq handler had a masking set to it.
>
> Remy Bohmer discovered that some devices in the ARM architecture
> would trigger the mask, but never unmask it. His patch to do the
> unmasking was questioned by Russell King about masking simple irqs
> to begin with. Looking further, it was discovered that the problems
> Remy was seeing was due to improper use of the simple handler by
> devices, and he later submitted patches to fix those. But the issue
> that was uncovered was that the simple handler should never mask.
>
> This patch reverts the masking in the simple handler.
>
Acked-by: Remy Bohmer <[EMAIL PROTECTED]>




> ---
>  kernel/irq/chip.c |9 ++---
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> Index: linux-compile.git/kernel/irq/chip.c
> ===
> --- linux-compile.git.orig/kernel/irq/chip.c2007-10-19 12:37:51.0 
> -0400
> +++ linux-compile.git/kernel/irq/chip.c 2007-12-12 15:03:43.0 -0500
> @@ -297,18 +297,13 @@ handle_simple_irq(unsigned int irq, stru
>
> if (unlikely(desc->status & IRQ_INPROGRESS))
> goto out_unlock;
> +   desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
> kstat_cpu(cpu).irqs[irq]++;
>
> action = desc->action;
> -   if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
> -   if (desc->chip->mask)
> -   desc->chip->mask(irq);
> -   desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
> -   desc->status |= IRQ_PENDING;
> +   if (unlikely(!action || (desc->status & IRQ_DISABLED)))
> goto out_unlock;
> -   }
>
> -   desc->status &= ~(IRQ_REPLAY | IRQ_WAITING | IRQ_PENDING);
> desc->status |= IRQ_INPROGRESS;
> spin_unlock(>lock);
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23.9-rt13

[Remy Bohmer]

Hello Steven,

If I compile -rt13 I get some compile warnings on ARM (AT91):
1) This one did not exist in rt1:
In file included from kernel/sched.c:911:
kernel/sched_rt.c: In function 'dec_rt_tasks':
kernel/sched_rt.c:88: warning: unused variable 'highest_prio'
2)  This one is there already for a much longer time:
CC  kernel/rcupreempt.o
kernel/rcupreempt.c:1001: warning: 'per_cpu__rcu_dyntick_snapshot'
defined but not used

Both warnings are fixed by the attached patch, but warning 2 needs some review.
This var is defined twice in this file, 1 in the NO_HZ ifdef, and 1
which seems to be not used.

Kind Regards,

Remy


2007/12/13, Steven Rostedt <[EMAIL PROTECTED]>:
> We are pleased to announce the 2.6.23.9-rt13 tree, which can be
> downloaded from the location:
>
>  http://www.kernel.org/pub/linux/kernel/projects/rt/
>
> Changes since 2.6.23.9-rt12
>
>   - Backported the new RT Balancing code from sched-devel
>New changes by Steven Rostedt, Gregory Haskins,
> Ingo Molnar, and Dmitry Adamushko
>
>   - 2 dimension CPU Prio RT balancing search (Gregory Haskins)
>
>   - ARM compile fix (Kevin Hilman)
>
>   - Disable HPET legacy replacement for kdump (OGAWA Hirofumi)
>
>   - disable HPET on shutdown (OGAWA Hirofumi)
>
>   - fix for futex_wait signal stack corruption (Steven Rostedt)
>
>   - Handle IRQ_PENDING for simple irq thread (Steven Rostedt)
>
>   - latency tracer updates (Daniel Walker)
>
>   - Remove warning in local_bh_enable (Kevin Hilman)
>
>   - use real time pcp locking for page draining during cpu (Andi Kleen)
>
>   - Revert lazy disable irq from simple irq handler (Steven Rostedt)
>
>   - AT91 switch to edge from simple irq (Remy Bohmer)
>
>
> to build a 2.6.23.9-rt13 tree, the following patches should be applied:
>
>   http://www.kernel.org/pub/linux/kernel/v2.6/linux-2.6.23.9.tar.bz2
>   http://www.kernel.org/pub/linux/kernel/projects/rt/patch-2.6.23.9-rt13.bz2
>
> And like always, my RT version of Matt Mackall's ketchup will get this
> for you nicely:
>
>   http://people.redhat.com/srostedt/rt/tools/ketchup-0.9.8-rt2
>
>
> The broken out patches are also available.
>
> -- Steve
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
> the body of a message to [EMAIL PROTECTED]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Fix a compile warning on -rt13, and UP (ARM AT91rm9200)

Signed-off-by: Remy Bohmer <[EMAIL PROTECTED]>

---
 kernel/rcupreempt.c |2 --
 kernel/sched_rt.c   |3 ++-
 2 files changed, 2 insertions(+), 3 deletions(-)

Index: linux-2.6.23/kernel/sched_rt.c
===
--- linux-2.6.23.orig/kernel/sched_rt.c	2007-12-13 10:59:54.0 +0100
+++ linux-2.6.23/kernel/sched_rt.c	2007-12-13 11:06:41.0 +0100
@@ -85,8 +85,9 @@ static inline void inc_rt_tasks(struct t
 
 static inline void dec_rt_tasks(struct task_struct *p, struct rq *rq)
 {
+#ifdef CONFIG_SMP
 	int highest_prio = rq->rt.highest_prio;
-
+#endif
 	WARN_ON(!rt_task(p));
 	WARN_ON(!rq->rt.rt_nr_running);
 	rq->rt.rt_nr_running--;
Index: linux-2.6.23/kernel/rcupreempt.c
===
--- linux-2.6.23.orig/kernel/rcupreempt.c	2007-12-13 10:59:54.0 +0100
+++ linux-2.6.23/kernel/rcupreempt.c	2007-12-13 11:38:28.0 +0100
@@ -998,8 +998,6 @@ void __init rcu_init_rt(void)
 	rcu_preempt_boost_init();
 }
 
-static DEFINE_PER_CPU(long, rcu_dyntick_snapshot);
-
 /*
  * Deprecated, use synchronize_rcu() or synchronize_sched() instead.
  */

Re: 2.6.23.9-rt13

[Remy Bohmer]

Hello Steven,

If I compile -rt13 I get some compile warnings on ARM (AT91):
1) This one did not exist in rt1:
In file included from kernel/sched.c:911:
kernel/sched_rt.c: In function 'dec_rt_tasks':
kernel/sched_rt.c:88: warning: unused variable 'highest_prio'
2)  This one is there already for a much longer time:
CC  kernel/rcupreempt.o
kernel/rcupreempt.c:1001: warning: 'per_cpu__rcu_dyntick_snapshot'
defined but not used

Both warnings are fixed by the attached patch, but warning 2 needs some review.
This var is defined twice in this file, 1 in the NO_HZ ifdef, and 1
which seems to be not used.

Kind Regards,

Remy


2007/12/13, Steven Rostedt [EMAIL PROTECTED]:
 We are pleased to announce the 2.6.23.9-rt13 tree, which can be
 downloaded from the location:

  http://www.kernel.org/pub/linux/kernel/projects/rt/

 Changes since 2.6.23.9-rt12

   - Backported the new RT Balancing code from sched-devel
New changes by Steven Rostedt, Gregory Haskins,
 Ingo Molnar, and Dmitry Adamushko

   - 2 dimension CPU Prio RT balancing search (Gregory Haskins)

   - ARM compile fix (Kevin Hilman)

   - Disable HPET legacy replacement for kdump (OGAWA Hirofumi)

   - disable HPET on shutdown (OGAWA Hirofumi)

   - fix for futex_wait signal stack corruption (Steven Rostedt)

   - Handle IRQ_PENDING for simple irq thread (Steven Rostedt)

   - latency tracer updates (Daniel Walker)

   - Remove warning in local_bh_enable (Kevin Hilman)

   - use real time pcp locking for page draining during cpu (Andi Kleen)

   - Revert lazy disable irq from simple irq handler (Steven Rostedt)

   - AT91 switch to edge from simple irq (Remy Bohmer)


 to build a 2.6.23.9-rt13 tree, the following patches should be applied:

   http://www.kernel.org/pub/linux/kernel/v2.6/linux-2.6.23.9.tar.bz2
   http://www.kernel.org/pub/linux/kernel/projects/rt/patch-2.6.23.9-rt13.bz2

 And like always, my RT version of Matt Mackall's ketchup will get this
 for you nicely:

   http://people.redhat.com/srostedt/rt/tools/ketchup-0.9.8-rt2


 The broken out patches are also available.

 -- Steve


 -
 To unsubscribe from this list: send the line unsubscribe linux-rt-users in
 the body of a message to [EMAIL PROTECTED]
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

Fix a compile warning on -rt13, and UP (ARM AT91rm9200)

Signed-off-by: Remy Bohmer [EMAIL PROTECTED]

---
 kernel/rcupreempt.c |2 --
 kernel/sched_rt.c   |3 ++-
 2 files changed, 2 insertions(+), 3 deletions(-)

Index: linux-2.6.23/kernel/sched_rt.c
===
--- linux-2.6.23.orig/kernel/sched_rt.c	2007-12-13 10:59:54.0 +0100
+++ linux-2.6.23/kernel/sched_rt.c	2007-12-13 11:06:41.0 +0100
@@ -85,8 +85,9 @@ static inline void inc_rt_tasks(struct t
 
 static inline void dec_rt_tasks(struct task_struct *p, struct rq *rq)
 {
+#ifdef CONFIG_SMP
 	int highest_prio = rq-rt.highest_prio;
-
+#endif
 	WARN_ON(!rt_task(p));
 	WARN_ON(!rq-rt.rt_nr_running);
 	rq-rt.rt_nr_running--;
Index: linux-2.6.23/kernel/rcupreempt.c
===
--- linux-2.6.23.orig/kernel/rcupreempt.c	2007-12-13 10:59:54.0 +0100
+++ linux-2.6.23/kernel/rcupreempt.c	2007-12-13 11:38:28.0 +0100
@@ -998,8 +998,6 @@ void __init rcu_init_rt(void)
 	rcu_preempt_boost_init();
 }
 
-static DEFINE_PER_CPU(long, rcu_dyntick_snapshot);
-
 /*
  * Deprecated, use synchronize_rcu() or synchronize_sched() instead.
  */

Re: [PATCH] Revert lazy irq disable for simple irqs

[Remy Bohmer]

Hello Steven,

 In commit 76d2160147f43f982dfe881404cfde9fd0a9da21 lazy irq disabling
 was implemented, and the simple irq handler had a masking set to it.

 Remy Bohmer discovered that some devices in the ARM architecture
 would trigger the mask, but never unmask it. His patch to do the
 unmasking was questioned by Russell King about masking simple irqs
 to begin with. Looking further, it was discovered that the problems
 Remy was seeing was due to improper use of the simple handler by
 devices, and he later submitted patches to fix those. But the issue
 that was uncovered was that the simple handler should never mask.

 This patch reverts the masking in the simple handler.

Acked-by: Remy Bohmer [EMAIL PROTECTED]




 ---
  kernel/irq/chip.c |9 ++---
  1 file changed, 2 insertions(+), 7 deletions(-)

 Index: linux-compile.git/kernel/irq/chip.c
 ===
 --- linux-compile.git.orig/kernel/irq/chip.c2007-10-19 12:37:51.0 
 -0400
 +++ linux-compile.git/kernel/irq/chip.c 2007-12-12 15:03:43.0 -0500
 @@ -297,18 +297,13 @@ handle_simple_irq(unsigned int irq, stru

 if (unlikely(desc-status  IRQ_INPROGRESS))
 goto out_unlock;
 +   desc-status = ~(IRQ_REPLAY | IRQ_WAITING);
 kstat_cpu(cpu).irqs[irq]++;

 action = desc-action;
 -   if (unlikely(!action || (desc-status  IRQ_DISABLED))) {
 -   if (desc-chip-mask)
 -   desc-chip-mask(irq);
 -   desc-status = ~(IRQ_REPLAY | IRQ_WAITING);
 -   desc-status |= IRQ_PENDING;
 +   if (unlikely(!action || (desc-status  IRQ_DISABLED)))
 goto out_unlock;
 -   }

 -   desc-status = ~(IRQ_REPLAY | IRQ_WAITING | IRQ_PENDING);
 desc-status |= IRQ_INPROGRESS;
 spin_unlock(desc-lock);



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RT] Revert Softdisable for simple irqs.

[Remy Bohmer]

> No problem. Could you also ACK the one I sent for mainline.

I will test it first tomorrow morning.

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RT] Revert Softdisable for simple irqs.

[Remy Bohmer]

Hello Steven,

>
> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21 lazy irq disabling
> was implemented, and the simple irq handler had a masking set to it.
>
> Remy Bohmer discovered that some devices in the ARM architecture
> would trigger the mask, but never unmask it. His patch to do the
> unmasking was questioned by Russell King about masking simple irqs
> to begin with. Looking further, it was discovered that the problems
> Remy was seeing was due to improper use of the simple handler by
> devices, and he later submitted patches to fix those. But the issue
> that was uncovered was that the simple handler should never mask.
>
> This patch reverts the masking in the simple handler.

Also:
Acked-by: Remy Bohmer <[EMAIL PROTECTED]>

Thanks for the effort also, I still had it on my todo list, but that
is needed anymore...

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RT] Revert Softdisable for simple irqs.

[Remy Bohmer]

Hello Steven,


 In commit 76d2160147f43f982dfe881404cfde9fd0a9da21 lazy irq disabling
 was implemented, and the simple irq handler had a masking set to it.

 Remy Bohmer discovered that some devices in the ARM architecture
 would trigger the mask, but never unmask it. His patch to do the
 unmasking was questioned by Russell King about masking simple irqs
 to begin with. Looking further, it was discovered that the problems
 Remy was seeing was due to improper use of the simple handler by
 devices, and he later submitted patches to fix those. But the issue
 that was uncovered was that the simple handler should never mask.

 This patch reverts the masking in the simple handler.

Also:
Acked-by: Remy Bohmer [EMAIL PROTECTED]

Thanks for the effort also, I still had it on my todo list, but that
is needed anymore...

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RT] Revert Softdisable for simple irqs.

[Remy Bohmer]

 No problem. Could you also ACK the one I sent for mainline.

I will test it first tomorrow morning.

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: lockdep problem conversion semaphore->mutex (dev->sem)

[Remy Bohmer]

Peter,

Thanks for this clear answer.

Remy

2007/12/8, Peter Zijlstra <[EMAIL PROTECTED]>:
>
> On Sat, 2007-12-08 at 21:33 +0100, Remy Bohmer wrote:
>
> > Which problems? I did not see any special things, it looked rather
> > straight forward. What have I overlooked?
>
> On suspend it locks the whole device tree, this means it has 'unbounded'
> nesting and holds an 'unbounded' number of locks. Neither things are
> easy to annotate (remember that mutex_lock_nested can handle up to 8
> nestings and current->held_locks has a max of 30).
>
> In fact, converting this will be the hardest part, it would require
> reworking the locking and introduction of a hard limit on the device
> tree depth - this might upset some people, but I suspect that 16 or 24
> should be deep enough for pretty much anything. Of course, if people
> prove me wrong, I'll have to reconsider. The up-side of the locking
> scheme I'm thinking of will be that locking the whole tree will only
> take 'depth' number of opterations vs the total number of tree elements.
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [EMAIL PROTECTED]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: lockdep problem conversion semaphore->mutex (dev->sem)

[Remy Bohmer]

Hello Ingo,

> no, you are wrong. If you want to do complex locking, you can still do
> it: take a look at the dev->sem conversion patches from Peter which
> correctly do this. Lockdep has all the facilities for that.
> (you just dont know about them)

Ok.

> the general policy message here is: do not implement complex locking. It
> hurts. It's hard to maintain. It's easy to mess up and leads to bugs.

Agree

> Lockdep just makes that plain obvious.
> Your mail and your frustration shows this general concept in happy

Please, do not see it as frustration, because it isn't...
I just want to understand it better.

> action: judging from your comments you have little clue about dev->sem
> locking and its implications and you'd happily go along and pollute the
> kernel with complex and hard to maintain nested locking constructs.

Now, you assume that i would _like_ complex locking. This is not true.

I just want to understand what was so wrong with dev->sem, I even read
in the discussions before about dev->sem, that it still was on the old
semaphore interface to get around lockdep issues, and _that_ is wrong.
That is bug-hiding from either the code or the tool. I just wanted to
understand if this was a lockdep bug, or a real code bug.

> Lockdep prevents you from doing it mindlessly, it _forces_ you to first
> understand the data structures, their locking and their relationship
> with each other. Then you can implement complexity, if you still want
> it.
>
> That, Sir, is a Good Thing (tm).

Completely agree.


Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: lockdep problem conversion semaphore->mutex (dev->sem)

[Remy Bohmer]

Hello Peter,

> And while you might not see it in-tree anymore, lockdep does help out
> tremendously while developing new code. I'm sure that without it the
> locking would be in a much worse state than it is today.

I am not arguing that, I am also convinced it has done a good job.

> I have a good idea on how to annotate this, but not yet the time to do
> so.

Sounds good...

> Aside from the nesting problems, the dev->sem has other problems as
> well. Converting this is rather non-trivial.

Which problems? I did not see any special things, it looked rather
straight forward. What have I overlooked?

> I'd not put it as harshly as you put it though, lockdep makes some
> assumptions which can lead to false positives -

By putting it this black and white, it usually helps to get all the
opinions clear ;-)
(By staying in the middle, everybody usually tend to agree ;-)

> otoh these assumptions
> often end up pointing out 'curious' locking coupled to 'curious' data
> structures. And fixing up these things often leads to better and simpler
> code.
> The emphasis is on often, this is one of the cases where this is not so.
> So while it does restain the creativity of locking it often ends up
> being for the better.

Ack.

Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: lockdep problem conversion semaphore->mutex (dev->sem)

[Remy Bohmer]

Hello Peter and Daniel,

> Yeah, it are different lock instances, however by virtue of sharing the
> same lock init site, they belong to the same lock class. Lockdep works
> by tracking class dependancies, not instance dependancies.

The device and its parent both indeed have different
pointers/instances. I saw that during debugging yesterday, so I
already expected this was not really a bug, but a false positive of
lockdep.

> By generalizing to classes it can detect locking errors before they
> actually occur.

Basically that is a good thing...
But... now we do not transfer the dev->sem to a mutex, because lockdep
will start generating false positives, and thus we mask the lockdep
error, by not converting the dev->sem to what it really is...

This does give me a bad feeling about this...  In short, we are
implementing workarounds in good code code to hide bugs in
debug-tooling... ?!

So, any suggestions on how to fix lockdep? Anyone some good hints
where I can start?
Is it that fundamental to lockdep that it cannot be fixed without
breaking it, or do we have to instrument the code that tells lockdep
to ignore this particular mutex?


Kind Regards,

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: lockdep problem conversion semaphore-mutex (dev-sem)

[Remy Bohmer]

Hello Peter and Daniel,

 Yeah, it are different lock instances, however by virtue of sharing the
 same lock init site, they belong to the same lock class. Lockdep works
 by tracking class dependancies, not instance dependancies.

The device and its parent both indeed have different
pointers/instances. I saw that during debugging yesterday, so I
already expected this was not really a bug, but a false positive of
lockdep.

 By generalizing to classes it can detect locking errors before they
 actually occur.

Basically that is a good thing...
But... now we do not transfer the dev-sem to a mutex, because lockdep
will start generating false positives, and thus we mask the lockdep
error, by not converting the dev-sem to what it really is...

This does give me a bad feeling about this...  In short, we are
implementing workarounds in good code code to hide bugs in
debug-tooling... ?!

So, any suggestions on how to fix lockdep? Anyone some good hints
where I can start?
Is it that fundamental to lockdep that it cannot be fixed without
breaking it, or do we have to instrument the code that tells lockdep
to ignore this particular mutex?


Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: lockdep problem conversion semaphore-mutex (dev-sem)

[Remy Bohmer]

Hello Peter,

 And while you might not see it in-tree anymore, lockdep does help out
 tremendously while developing new code. I'm sure that without it the
 locking would be in a much worse state than it is today.

I am not arguing that, I am also convinced it has done a good job.

 I have a good idea on how to annotate this, but not yet the time to do
 so.

Sounds good...

 Aside from the nesting problems, the dev-sem has other problems as
 well. Converting this is rather non-trivial.

Which problems? I did not see any special things, it looked rather
straight forward. What have I overlooked?

 I'd not put it as harshly as you put it though, lockdep makes some
 assumptions which can lead to false positives -

By putting it this black and white, it usually helps to get all the
opinions clear ;-)
(By staying in the middle, everybody usually tend to agree ;-)

 otoh these assumptions
 often end up pointing out 'curious' locking coupled to 'curious' data
 structures. And fixing up these things often leads to better and simpler
 code.
 The emphasis is on often, this is one of the cases where this is not so.
 So while it does restain the creativity of locking it often ends up
 being for the better.

Ack.

Kind Regards,

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: lockdep problem conversion semaphore-mutex (dev-sem)

[Remy Bohmer]

Hello Ingo,

 no, you are wrong. If you want to do complex locking, you can still do
 it: take a look at the dev-sem conversion patches from Peter which
 correctly do this. Lockdep has all the facilities for that.
 (you just dont know about them)

Ok.

 the general policy message here is: do not implement complex locking. It
 hurts. It's hard to maintain. It's easy to mess up and leads to bugs.

Agree

 Lockdep just makes that plain obvious.
 Your mail and your frustration shows this general concept in happy

Please, do not see it as frustration, because it isn't...
I just want to understand it better.

 action: judging from your comments you have little clue about dev-sem
 locking and its implications and you'd happily go along and pollute the
 kernel with complex and hard to maintain nested locking constructs.

Now, you assume that i would _like_ complex locking. This is not true.

I just want to understand what was so wrong with dev-sem, I even read
in the discussions before about dev-sem, that it still was on the old
semaphore interface to get around lockdep issues, and _that_ is wrong.
That is bug-hiding from either the code or the tool. I just wanted to
understand if this was a lockdep bug, or a real code bug.

 Lockdep prevents you from doing it mindlessly, it _forces_ you to first
 understand the data structures, their locking and their relationship
 with each other. Then you can implement complexity, if you still want
 it.

 That, Sir, is a Good Thing (tm).

Completely agree.


Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: lockdep problem conversion semaphore-mutex (dev-sem)

[Remy Bohmer]

Peter,

Thanks for this clear answer.

Remy

2007/12/8, Peter Zijlstra [EMAIL PROTECTED]:

 On Sat, 2007-12-08 at 21:33 +0100, Remy Bohmer wrote:

  Which problems? I did not see any special things, it looked rather
  straight forward. What have I overlooked?

 On suspend it locks the whole device tree, this means it has 'unbounded'
 nesting and holds an 'unbounded' number of locks. Neither things are
 easy to annotate (remember that mutex_lock_nested can handle up to 8
 nestings and current-held_locks has a max of 30).

 In fact, converting this will be the hardest part, it would require
 reworking the locking and introduction of a hard limit on the device
 tree depth - this might upset some people, but I suspect that 16 or 24
 should be deep enough for pretty much anything. Of course, if people
 prove me wrong, I'll have to reconsider. The up-side of the locking
 scheme I'm thinking of will be that locking the whole tree will only
 take 'depth' number of opterations vs the total number of tree elements.



 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to [EMAIL PROTECTED]
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

lockdep problem conversion semaphore->mutex (dev->sem)

[Remy Bohmer]

Hello Peter,

> > What specifically is wrong with dev->sem ?
>
> Nothing really, other than that they use semaphores to avoid lockdep :-/
>
> I think I know how to annotate this, after Alan Stern explained all the
> use cases, but I haven't come around to implementing it. Hope to do that
> soonish.

I was looking for an easy semaphore I could convert to a mutex, and I
ran into one that was widely spread and interesting, and which seemed
quite doable at first sight.
So, I started working on it, but was forgotten this discussion, (until
Daniel made me remember it this afternoon). So, I (stupid me ;-) )
tried to convert dev->sem...

After doing the monkey part of the conversion I can boot the kernel
completely on X86 and ARM, and everything works fine, except after
enabling lockdep, lockdep starts complaining...

Is this the problem you were pointing at?
===
Setting up standard PCI resources
ACPI: EC: Look up EC in DSDT
ACPI: Interpreter enabled
ACPI: (supports S0 S1 S3 S4 S5)
ACPI: Using IOAPIC for interrupt routing

=
[ INFO: possible recursive locking detected ]
2.6.24-rc4 #5
-
swapper/1 is trying to acquire lock:
 (>mut){--..}, at: [] __driver_attach+0x2c/0x5f

but task is already holding lock:
 (>mut){--..}, at: [] __driver_attach+0x1d/0x5f

other info that might help us debug this:
1 lock held by swapper/1:
 #0:  (>mut){--..}, at: [] __driver_attach+0x1d/0x5f

stack backtrace:
Pid: 1, comm: swapper Not tainted 2.6.24-rc4 #5
 [] __lock_acquire+0x17b/0xb83
 [] trace_hardirqs_on+0x11a/0x13d
 [] lock_acquire+0x5f/0x77
 [] __driver_attach+0x2c/0x5f
 [] mutex_lock_nested+0x105/0x26b
 [] __driver_attach+0x2c/0x5f
 [] __driver_attach+0x2c/0x5f
 [] __driver_attach+0x0/0x5f
 [] __driver_attach+0x2c/0x5f
 [] bus_for_each_dev+0x3a/0x5c
 [] driver_attach+0x16/0x18
 [] __driver_attach+0x0/0x5f
 [] bus_add_driver+0x6d/0x19a
 [] acpi_ec_init+0x33/0x51
 [] kernel_init+0x148/0x2af
 [] kernel_init+0x0/0x2af
 [] kernel_init+0x0/0x2af
 [] kernel_thread_helper+0x7/0x10
 ===
ACPI: PCI Root Bridge [PCI0] (:00)
Force enabled HPET at base address 0xfed0
===

I tried debugging it, and I have not found a recursive mutex locking
so far, only locking of 2 different mutexes in a row prior to this
warning, which IMO should be valid.

What is your opinion?

BTW: I attached my patch for dev->sem as I have it now, that generates
this lockdep warning ( for if you want to look at it yourself also, so
you do not have to do the monkey part yourself anymore ;-)


Kind Regards,

Remy


patch_replace_sem_by_mutex_in_device_h
Description: Binary data

lockdep problem conversion semaphore-mutex (dev-sem)

[Remy Bohmer]

Hello Peter,

  What specifically is wrong with dev-sem ?

 Nothing really, other than that they use semaphores to avoid lockdep :-/

 I think I know how to annotate this, after Alan Stern explained all the
 use cases, but I haven't come around to implementing it. Hope to do that
 soonish.

I was looking for an easy semaphore I could convert to a mutex, and I
ran into one that was widely spread and interesting, and which seemed
quite doable at first sight.
So, I started working on it, but was forgotten this discussion, (until
Daniel made me remember it this afternoon). So, I (stupid me ;-) )
tried to convert dev-sem...

After doing the monkey part of the conversion I can boot the kernel
completely on X86 and ARM, and everything works fine, except after
enabling lockdep, lockdep starts complaining...

Is this the problem you were pointing at?
===
Setting up standard PCI resources
ACPI: EC: Look up EC in DSDT
ACPI: Interpreter enabled
ACPI: (supports S0 S1 S3 S4 S5)
ACPI: Using IOAPIC for interrupt routing

=
[ INFO: possible recursive locking detected ]
2.6.24-rc4 #5
-
swapper/1 is trying to acquire lock:
 (dev-mut){--..}, at: [c056760c] __driver_attach+0x2c/0x5f

but task is already holding lock:
 (dev-mut){--..}, at: [c05675fd] __driver_attach+0x1d/0x5f

other info that might help us debug this:
1 lock held by swapper/1:
 #0:  (dev-mut){--..}, at: [c05675fd] __driver_attach+0x1d/0x5f

stack backtrace:
Pid: 1, comm: swapper Not tainted 2.6.24-rc4 #5
 [c0448a25] __lock_acquire+0x17b/0xb83
 [c0448491] trace_hardirqs_on+0x11a/0x13d
 [c04497f9] lock_acquire+0x5f/0x77
 [c056760c] __driver_attach+0x2c/0x5f
 [c06210de] mutex_lock_nested+0x105/0x26b
 [c056760c] __driver_attach+0x2c/0x5f
 [c056760c] __driver_attach+0x2c/0x5f
 [c05675e0] __driver_attach+0x0/0x5f
 [c056760c] __driver_attach+0x2c/0x5f
 [c0566ba9] bus_for_each_dev+0x3a/0x5c
 [c05673ba] driver_attach+0x16/0x18
 [c05675e0] __driver_attach+0x0/0x5f
 [c0566ea0] bus_add_driver+0x6d/0x19a
 [c0762613] acpi_ec_init+0x33/0x51
 [c0749491] kernel_init+0x148/0x2af
 [c0749349] kernel_init+0x0/0x2af
 [c0749349] kernel_init+0x0/0x2af
 [c0405bd7] kernel_thread_helper+0x7/0x10
 ===
ACPI: PCI Root Bridge [PCI0] (:00)
Force enabled HPET at base address 0xfed0
===

I tried debugging it, and I have not found a recursive mutex locking
so far, only locking of 2 different mutexes in a row prior to this
warning, which IMO should be valid.

What is your opinion?

BTW: I attached my patch for dev-sem as I have it now, that generates
this lockdep warning ( for if you want to look at it yourself also, so
you do not have to do the monkey part yourself anymore ;-)


Kind Regards,

Remy


patch_replace_sem_by_mutex_in_device_h
Description: Binary data

Re: [PATCH 3/3] printer port driver: semaphore to mutex

[Remy Bohmer]

Hello Daniel,

> I looked at the console_sem , but i was going to leave that as last..
>
> The problem with the console_sem is that it can get locked from
> interrupt context, which is discouraged with mutex types.. I think it
> will be complicated to convert..

At first it looked simple, but after I sent my mail, I saw also the
interrupt issue...
But nevertheless, I am going to look at it anyway... (I think not today)

Further, I looked also at include/linux/device.h where in the 'struct
device', a semaphore is listed that is used at several places as
mutex. I was wondering what was wise here:
* replacing the semaphore with a mutex at once, and replace it
everywhere in the kernel also, or
* just add a new mutex to the struct, and change the the semaphore to
a mutex per driver, and eventually remove the semaphore from the
struct.

The first option creates the biggest patch, the 2nd leaves the risk
for never removing the semaphore from the struct device...

What is your opinion?


BTW: I also will look at the dvb_frontend.c semaphore->mutex.
I also want to look at the mutex in the PS3 code, but I cannot test it
before I receive my new PS3 ;-)

Remy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] printer port driver: semaphore to mutex

[Remy Bohmer]

Hello Matthias,

Which do you have exactly on your list? (good to know, it prevents
double work...)

Remy

2007/12/6, Matthias Kaehlcke <[EMAIL PROTECTED]>:
> El Thu, Dec 06, 2007 at 08:34:06AM -0800 Daniel Walker ha dit:
>
> > On Thu, 2007-12-06 at 11:23 +0100, Ingo Molnar wrote:
> > > * Daniel Walker <[EMAIL PROTECTED]> wrote:
> > >
> > > > The port_mutex is actually a semaphore, so easily converted to a
> > > > struct mutex.
> > > >
> > > > Signed-off-by: Daniel Walker <[EMAIL PROTECTED]>
> > >
> > > Acked-by: Ingo Molnar <[EMAIL PROTECTED]>
> > >
> > > cool. How far away are we from being able to remove all the semaphore
> > > code? :-)
> >
> > I wish my 7 patches made a dent, but it's hasn't done much. ;(
> >
> > I would guess at least a week just to mop up the relatively easy ones..
> > I've got 12 in my queue, and there still ~50 hopefully trivial ones
> > still to be looked at.. Then another ~30 more difficult ones (that use
> > init_MUTEX_LOCKED, or sema_init with 0 instead of 1) ..
>
> I've also more patches of this type on my list, though i work in a
> slower pace
>
> --
> Matthias Kaehlcke
> Linux Application Developer
> Barcelona
>
>  You must have a plan. If you don't have a plan,
>you'll become part of somebody else's plan
>  .''`.
> using free software / Debian GNU/Linux | http://debian.org  : :'  :
> `. `'`
> gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4  `-
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] printer port driver: semaphore to mutex

[Remy Bohmer]

Daniel,

FYI: I am working on the conversion of the 2 sem->mutex in kernel/printk.c

Remy

2007/12/6, Daniel Walker <[EMAIL PROTECTED]>:
> On Thu, 2007-12-06 at 11:23 +0100, Ingo Molnar wrote:
> > * Daniel Walker <[EMAIL PROTECTED]> wrote:
> >
> > > The port_mutex is actually a semaphore, so easily converted to a
> > > struct mutex.
> > >
> > > Signed-off-by: Daniel Walker <[EMAIL PROTECTED]>
> >
> > Acked-by: Ingo Molnar <[EMAIL PROTECTED]>
> >
> > cool. How far away are we from being able to remove all the semaphore
> > code? :-)
>
> I wish my 7 patches made a dent, but it's hasn't done much. ;(
>
> I would guess at least a week just to mop up the relatively easy ones..
> I've got 12 in my queue, and there still ~50 hopefully trivial ones
> still to be looked at.. Then another ~30 more difficult ones (that use
> init_MUTEX_LOCKED, or sema_init with 0 instead of 1) ..
>
> Daniel
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] printer port driver: semaphore to mutex

[Remy Bohmer]

Daniel,

FYI: I am working on the conversion of the 2 sem-mutex in kernel/printk.c

Remy

2007/12/6, Daniel Walker [EMAIL PROTECTED]:
 On Thu, 2007-12-06 at 11:23 +0100, Ingo Molnar wrote:
  * Daniel Walker [EMAIL PROTECTED] wrote:
 
   The port_mutex is actually a semaphore, so easily converted to a
   struct mutex.
  
   Signed-off-by: Daniel Walker [EMAIL PROTECTED]
 
  Acked-by: Ingo Molnar [EMAIL PROTECTED]
 
  cool. How far away are we from being able to remove all the semaphore
  code? :-)

 I wish my 7 patches made a dent, but it's hasn't done much. ;(

 I would guess at least a week just to mop up the relatively easy ones..
 I've got 12 in my queue, and there still ~50 hopefully trivial ones
 still to be looked at.. Then another ~30 more difficult ones (that use
 init_MUTEX_LOCKED, or sema_init with 0 instead of 1) ..

 Daniel


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] printer port driver: semaphore to mutex

[Remy Bohmer]

Hello Matthias,

Which do you have exactly on your list? (good to know, it prevents
double work...)

Remy

2007/12/6, Matthias Kaehlcke [EMAIL PROTECTED]:
 El Thu, Dec 06, 2007 at 08:34:06AM -0800 Daniel Walker ha dit:

  On Thu, 2007-12-06 at 11:23 +0100, Ingo Molnar wrote:
   * Daniel Walker [EMAIL PROTECTED] wrote:
  
The port_mutex is actually a semaphore, so easily converted to a
struct mutex.
   
Signed-off-by: Daniel Walker [EMAIL PROTECTED]
  
   Acked-by: Ingo Molnar [EMAIL PROTECTED]
  
   cool. How far away are we from being able to remove all the semaphore
   code? :-)
 
  I wish my 7 patches made a dent, but it's hasn't done much. ;(
 
  I would guess at least a week just to mop up the relatively easy ones..
  I've got 12 in my queue, and there still ~50 hopefully trivial ones
  still to be looked at.. Then another ~30 more difficult ones (that use
  init_MUTEX_LOCKED, or sema_init with 0 instead of 1) ..

 I've also more patches of this type on my list, though i work in a
 slower pace

 --
 Matthias Kaehlcke
 Linux Application Developer
 Barcelona

  You must have a plan. If you don't have a plan,
you'll become part of somebody else's plan
  .''`.
 using free software / Debian GNU/Linux | http://debian.org  : :'  :
 `. `'`
 gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4  `-

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] printer port driver: semaphore to mutex

[Remy Bohmer]

Hello Daniel,

 I looked at the console_sem , but i was going to leave that as last..

 The problem with the console_sem is that it can get locked from
 interrupt context, which is discouraged with mutex types.. I think it
 will be complicated to convert..

At first it looked simple, but after I sent my mail, I saw also the
interrupt issue...
But nevertheless, I am going to look at it anyway... (I think not today)

Further, I looked also at include/linux/device.h where in the 'struct
device', a semaphore is listed that is used at several places as
mutex. I was wondering what was wise here:
* replacing the semaphore with a mutex at once, and replace it
everywhere in the kernel also, or
* just add a new mutex to the struct, and change the the semaphore to
a mutex per driver, and eventually remove the semaphore from the
struct.

The first option creates the biggest patch, the 2nd leaves the risk
for never removing the semaphore from the struct device...

What is your opinion?


BTW: I also will look at the dvb_frontend.c semaphore-mutex.
I also want to look at the mutex in the PS3 code, but I cannot test it
before I receive my new PS3 ;-)

Remy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello All,

I tested some more with the edge_triggered interrupt handler on AT91,
and I had already a long time a problem with the AT91SAM9261-EK kit,
that the DM9000 Ethernet controller did not work _at all_ on RT. I
just tried if the edge triggered interrupt handler works on that board
also. And now that board also works properly on RT. (my first patch
did not solve this problem either, so apparantly that one still misses
interrupts.)

This proves again that the simple_irq usage on AT91 is just crap...

Russell, Thanks again for the latest (and greatest) hint. This saves
me a lot of debug time on that board.

Kind Regards,

Remy

2007/11/29, Remy Bohmer <[EMAIL PROTECTED]>:
> Hello Russell,
>
> > While I realise that, I'm telling you that the _problem_ is being
> > caused by the wrong handler being used.
>
> Okay, I believe you... (someone told me once, Russell is right, and if
> you do not believe him, he is still right ;-)
>
> > SA1100 and PXA have exactly the same setup.  They use the correct
> > handler.  Why is AT91 special?
>
> This remark is what convinced me :-))
>
> I changed the interrupt handler from the simple_irq to the edge_irq,
> and it works...!!
> (I added a noop routine for that .ack part, because there is no ack)
>
> I believe I was too focussed on the masking bug in the RT kernel on
> the simple_irq() that I did not see that for the AT91 series the edge
> type interrupt handler also works... (even better...) What I thought
> was 1 single bug in the RT-kernel turned out to be a number of things
> together that aren't correct, even for mainline.
>
> So, to come to a conclusion: The masking bug in RT is still there in
> the simple_irq path, and masking has to be removed from the simple_irq
> code. Also for mainline. AT91 can live without simple_irq.
> I think we are in sync again...
>
> I will post a patch for the AT91 later on, after some more testing.
>
>
> Kind Regards,
>
> Remy
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello Russell,

> While I realise that, I'm telling you that the _problem_ is being
> caused by the wrong handler being used.

Okay, I believe you... (someone told me once, Russell is right, and if
you do not believe him, he is still right ;-)

> SA1100 and PXA have exactly the same setup.  They use the correct
> handler.  Why is AT91 special?

This remark is what convinced me :-))

I changed the interrupt handler from the simple_irq to the edge_irq,
and it works...!!
(I added a noop routine for that .ack part, because there is no ack)

I believe I was too focussed on the masking bug in the RT kernel on
the simple_irq() that I did not see that for the AT91 series the edge
type interrupt handler also works... (even better...) What I thought
was 1 single bug in the RT-kernel turned out to be a number of things
together that aren't correct, even for mainline.

So, to come to a conclusion: The masking bug in RT is still there in
the simple_irq path, and masking has to be removed from the simple_irq
code. Also for mainline. AT91 can live without simple_irq.
I think we are in sync again...

I will post a patch for the AT91 later on, after some more testing.


Kind Regards,

Remy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello Russell,

> If people insist on adding the mask/unmask crap to it, the function
> might as well be deleted and be an alias for handle_level_IRQ.  Because
> that's _precisely_ what you lot are turning it into.

First, I want to make clear that I am just debugging a problem on RT
that does not exist on mainline, and I am trying to find a way to get
it solved nicely _on RT_, and preferable in a way that it works for
everybody with the least chance for regression.

I already mentioned that RT is doing masking in this code during
normal use, where the mainline kernel does not do this, **except** in
an error situation.

I also mentioned that there are 2 ways of solving it on RT:
1) do not do masking at all, (just as on mainline), and only mask it
when there is an error (just as on mainline)
2) Fix it by adding an unmask, which I proposed in my first patch, and
which others also did in their patches. (not knowing your opinion, as
I do know)

Still, I believe, that the fact if a interrupt **can** be masked is
not a reason to forbid the simple_irq type(), and surely does not make
it automatically a level type interrupt.
The interrupt type I talk about is actually edge triggered (the
interrupt triggers on _BOTH_ edges of the input-line), but there is no
way of 'acknowledging' the interrupt, which makes the edge type
handler unsuitable, and much too heavy.
As mentioned, this type of irq is never pending, and unmasking it will
never lead to get a interrupt request immediately; the interrupt that
occurs during the masked time, is just lost.

So, as far as I can tell , the type really used on at91 for the GPIO
stuff _is_ a simple_irq as you describe, but one that can be
masked/unmasked **in case of errors**. It should never be masked
during normal use.

So, I propose option 1 to solve it on RT, and thus to trigger Steven
to NACK my first patch. I will try and see if I can make it work
_without_ masking on RT (except in case of errors, just as in
mainline).
...and probably add some clear description about the purpose of
simple_irq, especially related to masking...

Do you agree on this Russell?

> Ah, and looking at the changes to the file, the addition of the mask
> and unmask was done by someone who didn't understand what this was
> trying to do.  So that change should be backed out.

Maybe he was trying to mask the irq during an error situation?


Kind regards,

Remy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello Russell,

> If 'no' then it's the right handler and the mask/unmask methods associated
> with the interrupt will be no-ops.

I completely understand what you keep on saying, but that would imply
that the following piece of code in chip.c is completely bogus anyway!
(snip from mainline 2.6.23)

handle_simple_irq(unsigned int irq, struct irq_desc *desc)
...
if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
if (desc->chip->mask)
desc->chip->mask(irq);
...
}

Why trying to mask the interrupt if that is illegal use of the interrupt type?
This piece of code assumes that there CAN be a handler for masking
interrupts for the simple_irq type. This is in contradiction what you
are trying to explain.

If what you say is (completely) true the code is better to be:

if (desc->chip->mask)
 BUG();


IMO, interrupts can still be simple_irq types if the interrupt source
does **not have to** be disabled to be able to handle them. These
(GPIO) interrupts signal an event, that is already gone when the
interrupt handler starts, they are **never** really pending, waiting
for some device driver to handle the event. The device does not really
care if the interrupt is really handled, it just generates a new
interrupt on the next event.
And yes, it is possible that these interrupts can be seperately
masked/unmasked, but it is not necessary, and therefor others chose
apparantly for the simple_irq() types.

So, I do not think that the argument of **can** masked/unmasked is
valid, but more the **need** to be masked/unmasked is valid.

I think we should not argue if this is correct use, or misuse.
Fact is now that there is a bug in RT, that this type of 'misuse' does
not work on RT and breaks the interrupt handling. There are even more
patches floating around to fix this.

According to what Daniel also mentions, there are already several
architectures that use this type of interrupt with a masking/unmasking
implementation, apparantly all without the **need** to masking them.

So, the code must prevent this type of 'misuse' (and thus not
supporting it as it is doing now) or fully support it on RT, just like
on mainline.

So, in short: IMO, only RT is broken, not mainline -> thus fix RT,
otherwise we will have regression compared to mainline.

Kind Regards,

Remy


2007/11/29, Russell King - ARM Linux <[EMAIL PROTECTED]>:
> On Thu, Nov 29, 2007 at 11:14:30AM +0100, Remy Bohmer wrote:
> > I do not think Russell is right here with assuming that the wrong
> > interrupt handler type is used. Looking at the behaviour of the
> > mainline kernel (non-RT), the implementation is quite different: On
> > mainline the handle_simple_irq() in chip.c is not re-entrant, the
> > masking is **only** done in case of errors, and therefor never
> > unmasked again, of course.
>
> The issue comes down to a very simple question:
>
>   Are you using handle_simple_irq() with an interrupt which can be masked?
>
> If the answer to that question is 'yes' then you're using the wrong handler.
> If 'no' then it's the right handler and the mask/unmask methods associated
> with the interrupt will be no-ops.
>
> Therefore, if you have mask/unmask methods for a simple IRQ which actually
> do something, clearly the first answer is the one which applies.  You're
> using the wrong handler.  Use the level or edge handlers instead.
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello Steven,

> Remy, sorry about this round-a-bout. But I don't have any of the hardware
> that this affects, and I'm just being cautious.

No problem, I expected this discussion when I submitted the patch. It
is logical that everybody is cautious on this subject. But still,
there is a bug here. The question is how to figure out the best
solution. (with the least chance of regression).

I do not think Russell is right here with assuming that the wrong
interrupt handler type is used. Looking at the behaviour of the
mainline kernel (non-RT), the implementation is quite different: On
mainline the handle_simple_irq() in chip.c is not re-entrant, the
masking is **only** done in case of errors, and therefor never
unmasked again, of course.
On RT the masking is done when the next interrupt arrives, while the
1st interrupt is in progress by the interrupt thread, so it is masked
under normal valid conditions, and never unmasked. So, this is where
the real bug is.

So, I see 2 solutions:
* Never mask the interrupt in the first place. (should also work for
this type of interrupt)
* Add an unmask to the interrupt, once it is handled. (I chose this
route with my patch)

It is definitely only related to interrupt threading, and thus only RT related.

Kind Regards,

Remy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello Steven,

 Remy, sorry about this round-a-bout. But I don't have any of the hardware
 that this affects, and I'm just being cautious.

No problem, I expected this discussion when I submitted the patch. It
is logical that everybody is cautious on this subject. But still,
there is a bug here. The question is how to figure out the best
solution. (with the least chance of regression).

I do not think Russell is right here with assuming that the wrong
interrupt handler type is used. Looking at the behaviour of the
mainline kernel (non-RT), the implementation is quite different: On
mainline the handle_simple_irq() in chip.c is not re-entrant, the
masking is **only** done in case of errors, and therefor never
unmasked again, of course.
On RT the masking is done when the next interrupt arrives, while the
1st interrupt is in progress by the interrupt thread, so it is masked
under normal valid conditions, and never unmasked. So, this is where
the real bug is.

So, I see 2 solutions:
* Never mask the interrupt in the first place. (should also work for
this type of interrupt)
* Add an unmask to the interrupt, once it is handled. (I chose this
route with my patch)

It is definitely only related to interrupt threading, and thus only RT related.

Kind Regards,

Remy
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello Russell,

 If 'no' then it's the right handler and the mask/unmask methods associated
 with the interrupt will be no-ops.

I completely understand what you keep on saying, but that would imply
that the following piece of code in chip.c is completely bogus anyway!
(snip from mainline 2.6.23)

handle_simple_irq(unsigned int irq, struct irq_desc *desc)
...
if (unlikely(!action || (desc-status  IRQ_DISABLED))) {
if (desc-chip-mask)
desc-chip-mask(irq);
...
}

Why trying to mask the interrupt if that is illegal use of the interrupt type?
This piece of code assumes that there CAN be a handler for masking
interrupts for the simple_irq type. This is in contradiction what you
are trying to explain.

If what you say is (completely) true the code is better to be:

if (desc-chip-mask)
 BUG();


IMO, interrupts can still be simple_irq types if the interrupt source
does **not have to** be disabled to be able to handle them. These
(GPIO) interrupts signal an event, that is already gone when the
interrupt handler starts, they are **never** really pending, waiting
for some device driver to handle the event. The device does not really
care if the interrupt is really handled, it just generates a new
interrupt on the next event.
And yes, it is possible that these interrupts can be seperately
masked/unmasked, but it is not necessary, and therefor others chose
apparantly for the simple_irq() types.

So, I do not think that the argument of **can** masked/unmasked is
valid, but more the **need** to be masked/unmasked is valid.

I think we should not argue if this is correct use, or misuse.
Fact is now that there is a bug in RT, that this type of 'misuse' does
not work on RT and breaks the interrupt handling. There are even more
patches floating around to fix this.

According to what Daniel also mentions, there are already several
architectures that use this type of interrupt with a masking/unmasking
implementation, apparantly all without the **need** to masking them.

So, the code must prevent this type of 'misuse' (and thus not
supporting it as it is doing now) or fully support it on RT, just like
on mainline.

So, in short: IMO, only RT is broken, not mainline - thus fix RT,
otherwise we will have regression compared to mainline.

Kind Regards,

Remy


2007/11/29, Russell King - ARM Linux [EMAIL PROTECTED]:
 On Thu, Nov 29, 2007 at 11:14:30AM +0100, Remy Bohmer wrote:
  I do not think Russell is right here with assuming that the wrong
  interrupt handler type is used. Looking at the behaviour of the
  mainline kernel (non-RT), the implementation is quite different: On
  mainline the handle_simple_irq() in chip.c is not re-entrant, the
  masking is **only** done in case of errors, and therefor never
  unmasked again, of course.

 The issue comes down to a very simple question:

   Are you using handle_simple_irq() with an interrupt which can be masked?

 If the answer to that question is 'yes' then you're using the wrong handler.
 If 'no' then it's the right handler and the mask/unmask methods associated
 with the interrupt will be no-ops.

 Therefore, if you have mask/unmask methods for a simple IRQ which actually
 do something, clearly the first answer is the one which applies.  You're
 using the wrong handler.  Use the level or edge handlers instead.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello Russell,

 If people insist on adding the mask/unmask crap to it, the function
 might as well be deleted and be an alias for handle_level_IRQ.  Because
 that's _precisely_ what you lot are turning it into.

First, I want to make clear that I am just debugging a problem on RT
that does not exist on mainline, and I am trying to find a way to get
it solved nicely _on RT_, and preferable in a way that it works for
everybody with the least chance for regression.

I already mentioned that RT is doing masking in this code during
normal use, where the mainline kernel does not do this, **except** in
an error situation.

I also mentioned that there are 2 ways of solving it on RT:
1) do not do masking at all, (just as on mainline), and only mask it
when there is an error (just as on mainline)
2) Fix it by adding an unmask, which I proposed in my first patch, and
which others also did in their patches. (not knowing your opinion, as
I do know)

Still, I believe, that the fact if a interrupt **can** be masked is
not a reason to forbid the simple_irq type(), and surely does not make
it automatically a level type interrupt.
The interrupt type I talk about is actually edge triggered (the
interrupt triggers on _BOTH_ edges of the input-line), but there is no
way of 'acknowledging' the interrupt, which makes the edge type
handler unsuitable, and much too heavy.
As mentioned, this type of irq is never pending, and unmasking it will
never lead to get a interrupt request immediately; the interrupt that
occurs during the masked time, is just lost.

So, as far as I can tell , the type really used on at91 for the GPIO
stuff _is_ a simple_irq as you describe, but one that can be
masked/unmasked **in case of errors**. It should never be masked
during normal use.

So, I propose option 1 to solve it on RT, and thus to trigger Steven
to NACK my first patch. I will try and see if I can make it work
_without_ masking on RT (except in case of errors, just as in
mainline).
...and probably add some clear description about the purpose of
simple_irq, especially related to masking...

Do you agree on this Russell?

 Ah, and looking at the changes to the file, the addition of the mask
 and unmask was done by someone who didn't understand what this was
 trying to do.  So that change should be backed out.

Maybe he was trying to mask the irq during an error situation?


Kind regards,

Remy
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello Russell,

 While I realise that, I'm telling you that the _problem_ is being
 caused by the wrong handler being used.

Okay, I believe you... (someone told me once, Russell is right, and if
you do not believe him, he is still right ;-)

 SA1100 and PXA have exactly the same setup.  They use the correct
 handler.  Why is AT91 special?

This remark is what convinced me :-))

I changed the interrupt handler from the simple_irq to the edge_irq,
and it works...!!
(I added a noop routine for that .ack part, because there is no ack)

I believe I was too focussed on the masking bug in the RT kernel on
the simple_irq() that I did not see that for the AT91 series the edge
type interrupt handler also works... (even better...) What I thought
was 1 single bug in the RT-kernel turned out to be a number of things
together that aren't correct, even for mainline.

So, to come to a conclusion: The masking bug in RT is still there in
the simple_irq path, and masking has to be removed from the simple_irq
code. Also for mainline. AT91 can live without simple_irq.
I think we are in sync again...

I will post a patch for the AT91 later on, after some more testing.


Kind Regards,

Remy
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello All,

I tested some more with the edge_triggered interrupt handler on AT91,
and I had already a long time a problem with the AT91SAM9261-EK kit,
that the DM9000 Ethernet controller did not work _at all_ on RT. I
just tried if the edge triggered interrupt handler works on that board
also. And now that board also works properly on RT. (my first patch
did not solve this problem either, so apparantly that one still misses
interrupts.)

This proves again that the simple_irq usage on AT91 is just crap...

Russell, Thanks again for the latest (and greatest) hint. This saves
me a lot of debug time on that board.

Kind Regards,

Remy

2007/11/29, Remy Bohmer [EMAIL PROTECTED]:
 Hello Russell,

  While I realise that, I'm telling you that the _problem_ is being
  caused by the wrong handler being used.

 Okay, I believe you... (someone told me once, Russell is right, and if
 you do not believe him, he is still right ;-)

  SA1100 and PXA have exactly the same setup.  They use the correct
  handler.  Why is AT91 special?

 This remark is what convinced me :-))

 I changed the interrupt handler from the simple_irq to the edge_irq,
 and it works...!!
 (I added a noop routine for that .ack part, because there is no ack)

 I believe I was too focussed on the masking bug in the RT kernel on
 the simple_irq() that I did not see that for the AT91 series the edge
 type interrupt handler also works... (even better...) What I thought
 was 1 single bug in the RT-kernel turned out to be a number of things
 together that aren't correct, even for mainline.

 So, to come to a conclusion: The masking bug in RT is still there in
 the simple_irq path, and masking has to be removed from the simple_irq
 code. Also for mainline. AT91 can live without simple_irq.
 I think we are in sync again...

 I will post a patch for the AT91 later on, after some more testing.


 Kind Regards,

 Remy

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello Kevin,

Just copied your mail to the list, maybe your solution is also worth looking at.

Remy

> I had a similar issue when using the chained GPIO interrupts on OMAP
> under PREEMPT_RT.
>
> I believe the chained handler itself is supposed to be doing the
> ack/unmask instead of the simple_handler.
>
> However, currently there is no way for the chained handler to know
> when the threaded handler has acutally run.  So the way I fixed this
> was to have the simple handler call the chip->end hook so that the
> chained handler could do the ack/unmask after the threaded handler has
> actually run.
>
> I've been using this against the -rt patch since 2.6.21, and submitted
> and RFC a while back, but got no comments.
>
> This patch is against 2.6.24-rc2-rt1.
>
> Signed-off-by: Kevin Hilman <[EMAIL PROTECTED]>
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index b35d209..2f9e09e 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -661,6 +661,8 @@ static void thread_simple_irq(irq_desc_t *desc)
> note_interrupt(irq, desc, action_ret);
> }
> desc->status &= ~IRQ_INPROGRESS;
> +   if (desc->chip->end)
> +   desc->chip->end(irq);
>  }
>
>  /*
>
>
>
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello Daniel,

>  *  Note: The caller is expected to handle the ack, clear, mask and
>  *  unmask issues if necessary.
> So we shouldn't need any flow control unless there is some other
> factors..

This comment can be misinterpreted, I think. Who is assumed to be the
caller in this context? The 2 other routines in the driver that
actually do the unmasking stuff besides only calling this routine? Is
it allowed to call it directly or should it always be done through a
wrapper that does all these special things?

Either way, only masking interrupts, and never unmasking it, is a bug.
If interrupts come and go slow enough you never run into this problem,
and if this type is not used often, nobody will notice it.
Usually interrupts needs clearence of the source before the hardware
can generate a new one. GPIO interrupts are different, they are
generated whenever a IO-level changes, there is no acknowledge or
clearing of the interupt needed. These types of interrupts are never
'pending' from hardware point of view. So, with these type of
interrupts, a new one can occur while the interrupt handler has not
handled the previous one yet, and therefor these interrupt-types will
show this bug.

>
> Additionally, we have a patch in the real time tree called
> "irq-mask-fix.patch" which adds an "unmask" to handle_simple_irq, but as
> the note says we don't need flow control..

You mean the Montavista real time tree?

Kind Regards,

Remy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello Daniel,

  *  Note: The caller is expected to handle the ack, clear, mask and
  *  unmask issues if necessary.
 So we shouldn't need any flow control unless there is some other
 factors..

This comment can be misinterpreted, I think. Who is assumed to be the
caller in this context? The 2 other routines in the driver that
actually do the unmasking stuff besides only calling this routine? Is
it allowed to call it directly or should it always be done through a
wrapper that does all these special things?

Either way, only masking interrupts, and never unmasking it, is a bug.
If interrupts come and go slow enough you never run into this problem,
and if this type is not used often, nobody will notice it.
Usually interrupts needs clearence of the source before the hardware
can generate a new one. GPIO interrupts are different, they are
generated whenever a IO-level changes, there is no acknowledge or
clearing of the interupt needed. These types of interrupts are never
'pending' from hardware point of view. So, with these type of
interrupts, a new one can occur while the interrupt handler has not
handled the previous one yet, and therefor these interrupt-types will
show this bug.


 Additionally, we have a patch in the real time tree called
 irq-mask-fix.patch which adds an unmask to handle_simple_irq, but as
 the note says we don't need flow control..

You mean the Montavista real time tree?

Kind Regards,

Remy
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello Kevin,

Just copied your mail to the list, maybe your solution is also worth looking at.

Remy

 I had a similar issue when using the chained GPIO interrupts on OMAP
 under PREEMPT_RT.

 I believe the chained handler itself is supposed to be doing the
 ack/unmask instead of the simple_handler.

 However, currently there is no way for the chained handler to know
 when the threaded handler has acutally run.  So the way I fixed this
 was to have the simple handler call the chip-end hook so that the
 chained handler could do the ack/unmask after the threaded handler has
 actually run.

 I've been using this against the -rt patch since 2.6.21, and submitted
 and RFC a while back, but got no comments.

 This patch is against 2.6.24-rc2-rt1.

 Signed-off-by: Kevin Hilman [EMAIL PROTECTED]

 diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
 index b35d209..2f9e09e 100644
 --- a/kernel/irq/manage.c
 +++ b/kernel/irq/manage.c
 @@ -661,6 +661,8 @@ static void thread_simple_irq(irq_desc_t *desc)
 note_interrupt(irq, desc, action_ret);
 }
 desc-status = ~IRQ_INPROGRESS;
 +   if (desc-chip-end)
 +   desc-chip-end(irq);
  }

  /*




-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Attached the same patch, but it also cleans the manage.c code a bit,
because the IRQ types 'simple IRQ', 'level-IRQ' and 'FastEOI' were
handled differently while they should be handled the same.

Kind Regards,

Remy

2007/11/26, Remy Bohmer <[EMAIL PROTECTED]>:
> Hello,
>
> I use 2.6.23.1-rt5 on the Atmel AT91 series.
> Interrupt threading on Preempt-RT and ARM works fine, except for
> (edge-triggered) GPIO interrupts. There is a problem when a new
> interrupt arives while the interrupt thread is handling the previous
> interrupt. If this occurs the interrupt handling stalls forever.
>
> This is caused by a unbalanced interrupt mask/unmask problem in the kernel.
> The attached patch fixes this. More information about this problem is
> documented inside the patch itself.
>
> This patch is meant for Preempt-RT only.
>
> Kind Regards,
>
> Remy Bohmer
>
>
On ARM there is a problem where the interrupt handler stalls when they are 
coming faster than the kernel can handle.

The problem occurs when the routine handle_simple_irq() masks the interrupt 
when an IRQ-thread is handling the interrupt at the same time. (IRQ_INPROGRESS
is set). The interrupt thread, however does **never** a 
desc->chip->unmask(), so the interrupt becomes disabled forever.

IRQ_DISABLED is usually not set for this interrupt

This is in kernel/irq/chip.c, where the irq is masked when a IRQ-thread is
running: 
--
void fastcall
handle_simple_irq(unsigned int irq, struct irq_desc *desc)
{




	if (unlikely(!action || (desc->status & (IRQ_INPROGRESS |
		 IRQ_DISABLED {
(!!)->		if (desc->chip->mask)
(!!)->			desc->chip->mask(irq);
		desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
		desc->status |= IRQ_PENDING;
		goto out_unlock;
	}



}
--

Masking the interrupt seems valid, because the interrupt handler thread is 
still running, so it can handle the new pending interrupt. But, it has to be
umasked somewhere. The logical place is to do this in kernel/irq/manage.c, 
because this situation is also handled for the thread_level_irq() and 
thread_fasteoi_irq(), but not for thread_simple_irq(). This patch adds this
for these kind of interrupts also.

Signed-off-by: Remy Bohmer <[EMAIL PROTECTED]>
---
 kernel/irq/manage.c |   29 +++--
 1 file changed, 3 insertions(+), 26 deletions(-)

Index: linux-2.6.23/kernel/irq/manage.c
===
--- linux-2.6.23.orig/kernel/irq/manage.c	2007-11-26 13:46:58.0 +0100
+++ linux-2.6.23/kernel/irq/manage.c	2007-11-26 14:43:32.0 +0100
@@ -646,28 +646,7 @@ static void thread_simple_irq(irq_desc_t
 			note_interrupt(irq, desc, action_ret);
 	}
 	desc->status &= ~IRQ_INPROGRESS;
-}
-
-/*
- * threaded level type irq handler
- */
-static void thread_level_irq(irq_desc_t *desc)
-{
-	unsigned int irq = desc - irq_desc;
-
-	thread_simple_irq(desc);
-	if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
-		desc->chip->unmask(irq);
-}
-
-/*
- * threaded fasteoi type irq handler
- */
-static void thread_fasteoi_irq(irq_desc_t *desc)
-{
-	unsigned int irq = desc - irq_desc;
 
-	thread_simple_irq(desc);
 	if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
 		desc->chip->unmask(irq);
 }
@@ -747,12 +726,10 @@ static void do_hardirq(struct irq_desc *
 	if (!(desc->status & IRQ_INPROGRESS))
 		goto out;
 
-	if (desc->handle_irq == handle_simple_irq)
+	if ((desc->handle_irq == handle_simple_irq) ||
+	(desc->handle_irq == handle_level_irq)  ||
+	(desc->handle_irq == handle_fasteoi_irq))
 		thread_simple_irq(desc);
-	else if (desc->handle_irq == handle_level_irq)
-		thread_level_irq(desc);
-	else if (desc->handle_irq == handle_fasteoi_irq)
-		thread_fasteoi_irq(desc);
 	else if (desc->handle_irq == handle_edge_irq)
 		thread_edge_irq(desc);
 	else

[PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello,

I use 2.6.23.1-rt5 on the Atmel AT91 series.
Interrupt threading on Preempt-RT and ARM works fine, except for
(edge-triggered) GPIO interrupts. There is a problem when a new
interrupt arives while the interrupt thread is handling the previous
interrupt. If this occurs the interrupt handling stalls forever.

This is caused by a unbalanced interrupt mask/unmask problem in the kernel.
The attached patch fixes this. More information about this problem is
documented inside the patch itself.

This patch is meant for Preempt-RT only.

Kind Regards,

Remy Bohmer
On ARM there is a problem where the interrupt handler stalls when they are 
coming faster than the kernel can handle.

The problem occurs when the routine handle_simple_irq() masks the interrupt 
when an IRQ-thread is handling the interrupt at the same time. (IRQ_INPROGRESS
is set). The interrupt thread, however does **never** a 
desc->chip->unmask(), so the interrupt becomes disabled forever.

IRQ_DISABLED is usually not set for this interrupt

This is in kernel/irq/chip.c, where the irq is masked when a IRQ-thread is
running: 
--
void fastcall
handle_simple_irq(unsigned int irq, struct irq_desc *desc)
{




	if (unlikely(!action || (desc->status & (IRQ_INPROGRESS |
		 IRQ_DISABLED {
(!!)->		if (desc->chip->mask)
(!!)->			desc->chip->mask(irq);
		desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
		desc->status |= IRQ_PENDING;
		goto out_unlock;
	}



}
--

Masking the interrupt seems valid, because the interrupt handler thread is 
still running, so it can handle the new pending interrupt. But, it has to be
umasked somewhere. The logical place is to do this in kernel/irq/manage.c, 
because this situation is also handled for the thread_level_irq() and 
thread_fasteoi_irq(), but not for thread_simple_irq(). This patch adds this
for these kind of interrupts also.

Signed-off-by: Remy Bohmer <[EMAIL PROTECTED]>
---
 kernel/irq/manage.c |3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6.23/kernel/irq/manage.c
===
--- linux-2.6.23.orig/kernel/irq/manage.c	2007-11-26 13:46:58.0 +0100
+++ linux-2.6.23/kernel/irq/manage.c	2007-11-26 13:48:30.0 +0100
@@ -646,6 +646,9 @@ static void thread_simple_irq(irq_desc_t
 			note_interrupt(irq, desc, action_ret);
 	}
 	desc->status &= ~IRQ_INPROGRESS;
+
+	if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
+		desc->chip->unmask(irq);
 }
 
 /*

[PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Hello,

I use 2.6.23.1-rt5 on the Atmel AT91 series.
Interrupt threading on Preempt-RT and ARM works fine, except for
(edge-triggered) GPIO interrupts. There is a problem when a new
interrupt arives while the interrupt thread is handling the previous
interrupt. If this occurs the interrupt handling stalls forever.

This is caused by a unbalanced interrupt mask/unmask problem in the kernel.
The attached patch fixes this. More information about this problem is
documented inside the patch itself.

This patch is meant for Preempt-RT only.

Kind Regards,

Remy Bohmer
On ARM there is a problem where the interrupt handler stalls when they are 
coming faster than the kernel can handle.

The problem occurs when the routine handle_simple_irq() masks the interrupt 
when an IRQ-thread is handling the interrupt at the same time. (IRQ_INPROGRESS
is set). The interrupt thread, however does **never** a 
desc-chip-unmask(), so the interrupt becomes disabled forever.

IRQ_DISABLED is usually not set for this interrupt

This is in kernel/irq/chip.c, where the irq is masked when a IRQ-thread is
running: 
--
void fastcall
handle_simple_irq(unsigned int irq, struct irq_desc *desc)
{




	if (unlikely(!action || (desc-status  (IRQ_INPROGRESS |
		 IRQ_DISABLED {
(!!)-		if (desc-chip-mask)
(!!)-			desc-chip-mask(irq);
		desc-status = ~(IRQ_REPLAY | IRQ_WAITING);
		desc-status |= IRQ_PENDING;
		goto out_unlock;
	}



}
--

Masking the interrupt seems valid, because the interrupt handler thread is 
still running, so it can handle the new pending interrupt. But, it has to be
umasked somewhere. The logical place is to do this in kernel/irq/manage.c, 
because this situation is also handled for the thread_level_irq() and 
thread_fasteoi_irq(), but not for thread_simple_irq(). This patch adds this
for these kind of interrupts also.

Signed-off-by: Remy Bohmer [EMAIL PROTECTED]
---
 kernel/irq/manage.c |3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6.23/kernel/irq/manage.c
===
--- linux-2.6.23.orig/kernel/irq/manage.c	2007-11-26 13:46:58.0 +0100
+++ linux-2.6.23/kernel/irq/manage.c	2007-11-26 13:48:30.0 +0100
@@ -646,6 +646,9 @@ static void thread_simple_irq(irq_desc_t
 			note_interrupt(irq, desc, action_ret);
 	}
 	desc-status = ~IRQ_INPROGRESS;
+
+	if (!(desc-status  IRQ_DISABLED)  desc-chip-unmask)
+		desc-chip-unmask(irq);
 }
 
 /*

Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever

[Remy Bohmer]

Attached the same patch, but it also cleans the manage.c code a bit,
because the IRQ types 'simple IRQ', 'level-IRQ' and 'FastEOI' were
handled differently while they should be handled the same.

Kind Regards,

Remy

2007/11/26, Remy Bohmer [EMAIL PROTECTED]:
 Hello,

 I use 2.6.23.1-rt5 on the Atmel AT91 series.
 Interrupt threading on Preempt-RT and ARM works fine, except for
 (edge-triggered) GPIO interrupts. There is a problem when a new
 interrupt arives while the interrupt thread is handling the previous
 interrupt. If this occurs the interrupt handling stalls forever.

 This is caused by a unbalanced interrupt mask/unmask problem in the kernel.
 The attached patch fixes this. More information about this problem is
 documented inside the patch itself.

 This patch is meant for Preempt-RT only.

 Kind Regards,

 Remy Bohmer


On ARM there is a problem where the interrupt handler stalls when they are 
coming faster than the kernel can handle.

The problem occurs when the routine handle_simple_irq() masks the interrupt 
when an IRQ-thread is handling the interrupt at the same time. (IRQ_INPROGRESS
is set). The interrupt thread, however does **never** a 
desc-chip-unmask(), so the interrupt becomes disabled forever.

IRQ_DISABLED is usually not set for this interrupt

This is in kernel/irq/chip.c, where the irq is masked when a IRQ-thread is
running: 
--
void fastcall
handle_simple_irq(unsigned int irq, struct irq_desc *desc)
{




	if (unlikely(!action || (desc-status  (IRQ_INPROGRESS |
		 IRQ_DISABLED {
(!!)-		if (desc-chip-mask)
(!!)-			desc-chip-mask(irq);
		desc-status = ~(IRQ_REPLAY | IRQ_WAITING);
		desc-status |= IRQ_PENDING;
		goto out_unlock;
	}



}
--

Masking the interrupt seems valid, because the interrupt handler thread is 
still running, so it can handle the new pending interrupt. But, it has to be
umasked somewhere. The logical place is to do this in kernel/irq/manage.c, 
because this situation is also handled for the thread_level_irq() and 
thread_fasteoi_irq(), but not for thread_simple_irq(). This patch adds this
for these kind of interrupts also.

Signed-off-by: Remy Bohmer [EMAIL PROTECTED]
---
 kernel/irq/manage.c |   29 +++--
 1 file changed, 3 insertions(+), 26 deletions(-)

Index: linux-2.6.23/kernel/irq/manage.c
===
--- linux-2.6.23.orig/kernel/irq/manage.c	2007-11-26 13:46:58.0 +0100
+++ linux-2.6.23/kernel/irq/manage.c	2007-11-26 14:43:32.0 +0100
@@ -646,28 +646,7 @@ static void thread_simple_irq(irq_desc_t
 			note_interrupt(irq, desc, action_ret);
 	}
 	desc-status = ~IRQ_INPROGRESS;
-}
-
-/*
- * threaded level type irq handler
- */
-static void thread_level_irq(irq_desc_t *desc)
-{
-	unsigned int irq = desc - irq_desc;
-
-	thread_simple_irq(desc);
-	if (!(desc-status  IRQ_DISABLED)  desc-chip-unmask)
-		desc-chip-unmask(irq);
-}
-
-/*
- * threaded fasteoi type irq handler
- */
-static void thread_fasteoi_irq(irq_desc_t *desc)
-{
-	unsigned int irq = desc - irq_desc;
 
-	thread_simple_irq(desc);
 	if (!(desc-status  IRQ_DISABLED)  desc-chip-unmask)
 		desc-chip-unmask(irq);
 }
@@ -747,12 +726,10 @@ static void do_hardirq(struct irq_desc *
 	if (!(desc-status  IRQ_INPROGRESS))
 		goto out;
 
-	if (desc-handle_irq == handle_simple_irq)
+	if ((desc-handle_irq == handle_simple_irq) ||
+	(desc-handle_irq == handle_level_irq)  ||
+	(desc-handle_irq == handle_fasteoi_irq))
 		thread_simple_irq(desc);
-	else if (desc-handle_irq == handle_level_irq)
-		thread_level_irq(desc);
-	else if (desc-handle_irq == handle_fasteoi_irq)
-		thread_fasteoi_irq(desc);
 	else if (desc-handle_irq == handle_edge_irq)
 		thread_edge_irq(desc);
 	else

Re: [BUG on PREEMPT_RT, 2.6.23.1-rt5] in rt-mutex code and signals

[Remy Bohmer]

Hello Daniel,

Can we coordinate who is doing what somehow, to prevent double work?

Remy

2007/11/19, Daniel Walker <[EMAIL PROTECTED]>:
> On Mon, 2007-11-19 at 02:24 -0500, Jon Masters wrote:
> > On Sat, 2007-11-17 at 09:55 -0800, Daniel Walker wrote:
> >
> > > Sure, you want to split the list?
> >
> > I'm happy to grab a few of these too. Let me know if either of you or
> > Ingo is working on the whole lot and about to dump it on us ;-)
>
> I was planning to just try a few, so I think your safe..
>
> Daniel
>
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [BUG on PREEMPT_RT, 2.6.23.1-rt5] in rt-mutex code and signals

[Remy Bohmer]

Hello Steven,

> > OK, I wont be able to work on this this weekend, but I'll try to get to it
> > on Monday.  A better example to show the bug you are looking for is simply
> > create a mutex and create a thread that grabs that mutex and goes to
> > sleep. Have your driver read grab that mutex with
> > mutex_lock_interruptible. And if the signal code is broken with this, then
> > you definitely got a point that the inerruptible code is broken.

I removed the 'struct semaphore' completely from my driver, using real
mutexes now instead, replace the signalling semaphores by 'struct
completion' mechanisms and got rid of the possible race I saw with the
completions in a different way, and now the problem is completely
gone!

Posix Signals work properly now (no OOPS anymore), so the problem was
likely related to the way I used the 'struct semaphore' types, which
is thus different compared to the non-RT kernel and therefor quite
confusing.

So, thank you (and Daniel) for pointing me into the right direction.

Now lets get rid of the 'struct semaphore' completely in the kernel :-))


Kind Regards,

Remy Bohmer
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [BUG on PREEMPT_RT, 2.6.23.1-rt5] in rt-mutex code and signals

[Remy Bohmer]

Hello Steven,

  OK, I wont be able to work on this this weekend, but I'll try to get to it
  on Monday.  A better example to show the bug you are looking for is simply
  create a mutex and create a thread that grabs that mutex and goes to
  sleep. Have your driver read grab that mutex with
  mutex_lock_interruptible. And if the signal code is broken with this, then
  you definitely got a point that the inerruptible code is broken.

I removed the 'struct semaphore' completely from my driver, using real
mutexes now instead, replace the signalling semaphores by 'struct
completion' mechanisms and got rid of the possible race I saw with the
completions in a different way, and now the problem is completely
gone!

Posix Signals work properly now (no OOPS anymore), so the problem was
likely related to the way I used the 'struct semaphore' types, which
is thus different compared to the non-RT kernel and therefor quite
confusing.

So, thank you (and Daniel) for pointing me into the right direction.

Now lets get rid of the 'struct semaphore' completely in the kernel :-))


Kind Regards,

Remy Bohmer
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [BUG on PREEMPT_RT, 2.6.23.1-rt5] in rt-mutex code and signals

[Remy Bohmer]

Hello Daniel,

Can we coordinate who is doing what somehow, to prevent double work?

Remy

2007/11/19, Daniel Walker [EMAIL PROTECTED]:
 On Mon, 2007-11-19 at 02:24 -0500, Jon Masters wrote:
  On Sat, 2007-11-17 at 09:55 -0800, Daniel Walker wrote:
 
   Sure, you want to split the list?
 
  I'm happy to grab a few of these too. Let me know if either of you or
  Ingo is working on the whole lot and about to dump it on us ;-)

 I was planning to just try a few, so I think your safe..

 Daniel


-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/