Re: 2.6.24-git: kmap_atomic() WARN_ON()

2008-02-26 Thread Andrew Morton 
On Tue, 26 Feb 2008 11:32:24 -0500 Jeff Garzik <[EMAIL PROTECTED]> wrote:

> Ingo Molnar wrote:
> > * Jeff Garzik <[EMAIL PROTECTED]> wrote:
> > 
> >> +  unsigned long flags;
> >> +
> >> +  local_irq_save(flags);
> > 
> > hm, couldnt we attach the irq disabling to some spinlock, in a natural 
> > way? Explicit flags fiddling is a PITA once we do things like threaded 
> > irq handlers, -rt, etc.
> 
> Attaching the irq disabling to some spinlock is what would be 
> artificial...  See the ahci.c patch earlier in this thread.  It is taken 
> without spin_lock_irqsave() in the interrupt handler, and there is no 
> reason to disable interrupts for the entirety of the interrupt handler 
> run -- only the part where we call kmap.
> 
> This is only being done to satisfy kmap_atomic's requirements, not libata's.
> 
> I could add a "kmap lock" but that just seems silly.
> 

It's a bit sad to disable interupts across a memset (how big is it?) just
for the small proportion of cases which are accessing a highmem page.

What you could do is to add an `unsigned long *flags' arg to
ata_scsi_rbuf_get() and ata_scsi_rbuf_put(), and then, in
ata_scsi_rbuf_get() do

if (PageHighmem(page))
local_irq_disable(*flags);


--
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.24-git: kmap_atomic() WARN_ON()

2008-02-26 Thread Jeff Garzik 

Ingo Molnar wrote:

* Jeff Garzik <[EMAIL PROTECTED]> wrote:


+   unsigned long flags;
+
+   local_irq_save(flags);


hm, couldnt we attach the irq disabling to some spinlock, in a natural 
way? Explicit flags fiddling is a PITA once we do things like threaded 
irq handlers, -rt, etc.


Attaching the irq disabling to some spinlock is what would be 
artificial...  See the ahci.c patch earlier in this thread.  It is taken 
without spin_lock_irqsave() in the interrupt handler, and there is no 
reason to disable interrupts for the entirety of the interrupt handler 
run -- only the part where we call kmap.


This is only being done to satisfy kmap_atomic's requirements, not libata's.

I could add a "kmap lock" but that just seems silly.

Jeff




--
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.24-git: kmap_atomic() WARN_ON()

2008-02-26 Thread Thomas Gleixner 
On Mon, 25 Feb 2008, Jeff Garzik wrote:

> Welcome to test this... (attached, not tested nor even compiled, really)

Works, but I agree with Ingo vs. the stand alone irq_en/disable.

Thanks,
tglx
--
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.24-git: kmap_atomic() WARN_ON()

2008-02-26 Thread Ingo Molnar 

* Jeff Garzik <[EMAIL PROTECTED]> wrote:

> + unsigned long flags;
> +
> + local_irq_save(flags);

hm, couldnt we attach the irq disabling to some spinlock, in a natural 
way? Explicit flags fiddling is a PITA once we do things like threaded 
irq handlers, -rt, etc.

Ingo
--
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.24-git: kmap_atomic() WARN_ON()

2008-02-26 Thread Ingo Molnar 

* Jeff Garzik [EMAIL PROTECTED] wrote:

 + unsigned long flags;
 +
 + local_irq_save(flags);

hm, couldnt we attach the irq disabling to some spinlock, in a natural 
way? Explicit flags fiddling is a PITA once we do things like threaded 
irq handlers, -rt, etc.

Ingo
--
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.24-git: kmap_atomic() WARN_ON()

2008-02-26 Thread Thomas Gleixner 
On Mon, 25 Feb 2008, Jeff Garzik wrote:

 Welcome to test this... (attached, not tested nor even compiled, really)

Works, but I agree with Ingo vs. the stand alone irq_en/disable.

Thanks,
tglx
--
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.24-git: kmap_atomic() WARN_ON()

2008-02-26 Thread Jeff Garzik 

Ingo Molnar wrote:

* Jeff Garzik [EMAIL PROTECTED] wrote:


+   unsigned long flags;
+
+   local_irq_save(flags);


hm, couldnt we attach the irq disabling to some spinlock, in a natural 
way? Explicit flags fiddling is a PITA once we do things like threaded 
irq handlers, -rt, etc.


Attaching the irq disabling to some spinlock is what would be 
artificial...  See the ahci.c patch earlier in this thread.  It is taken 
without spin_lock_irqsave() in the interrupt handler, and there is no 
reason to disable interrupts for the entirety of the interrupt handler 
run -- only the part where we call kmap.


This is only being done to satisfy kmap_atomic's requirements, not libata's.

I could add a kmap lock but that just seems silly.

Jeff




--
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.24-git: kmap_atomic() WARN_ON()

2008-02-26 Thread Andrew Morton 
On Tue, 26 Feb 2008 11:32:24 -0500 Jeff Garzik [EMAIL PROTECTED] wrote:

 Ingo Molnar wrote:
  * Jeff Garzik [EMAIL PROTECTED] wrote:
  
  +  unsigned long flags;
  +
  +  local_irq_save(flags);
  
  hm, couldnt we attach the irq disabling to some spinlock, in a natural 
  way? Explicit flags fiddling is a PITA once we do things like threaded 
  irq handlers, -rt, etc.
 
 Attaching the irq disabling to some spinlock is what would be 
 artificial...  See the ahci.c patch earlier in this thread.  It is taken 
 without spin_lock_irqsave() in the interrupt handler, and there is no 
 reason to disable interrupts for the entirety of the interrupt handler 
 run -- only the part where we call kmap.
 
 This is only being done to satisfy kmap_atomic's requirements, not libata's.
 
 I could add a kmap lock but that just seems silly.
 

It's a bit sad to disable interupts across a memset (how big is it?) just
for the small proportion of cases which are accessing a highmem page.

What you could do is to add an `unsigned long *flags' arg to
ata_scsi_rbuf_get() and ata_scsi_rbuf_put(), and then, in
ata_scsi_rbuf_get() do

if (PageHighmem(page))
local_irq_disable(*flags);


--
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.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Jeff Garzik 

Welcome to test this... (attached, not tested nor even compiled, really)

Jeff



diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0562b0a..7b1f1ee 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1694,12 +1694,17 @@ void ata_scsi_rbuf_fill(struct ata_scsi_args *args,
u8 *rbuf;
unsigned int buflen, rc;
struct scsi_cmnd *cmd = args->cmd;
+   unsigned long flags;
+
+   local_irq_save(flags);
 
buflen = ata_scsi_rbuf_get(cmd, );
memset(rbuf, 0, buflen);
rc = actor(args, rbuf, buflen);
ata_scsi_rbuf_put(cmd, rbuf);
 
+   local_irq_restore(flags);
+
if (rc == 0)
cmd->result = SAM_STAT_GOOD;
args->done(cmd);
@@ -2473,6 +2478,9 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
if ((scsicmd[0] == INQUIRY) && ((scsicmd[1] & 0x03) == 0)) {
u8 *buf = NULL;
unsigned int buflen;
+   unsigned long flags;
+
+   local_irq_save(flags);
 
buflen = ata_scsi_rbuf_get(cmd, );
 
@@ -2490,6 +2498,8 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
}
 
ata_scsi_rbuf_put(cmd, buf);
+
+   local_irq_restore(flags);
}
 
cmd->result = SAM_STAT_GOOD;

Re: 2.6.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Andrew Morton 
On Mon, 25 Feb 2008 23:01:59 +0100 (CET) Thomas Gleixner <[EMAIL PROTECTED]> 
wrote:

> On Mon, 25 Feb 2008, Andrew Morton wrote:
> > On Mon, 25 Feb 2008 15:08:35 -0500 Jeff Garzik <[EMAIL PROTECTED]> wrote:
> > > There are plenty of drivers that do the same thing that ahci does, in 
> > > terms of interrupt handler locking...  and I will definitely push back 
> > > on efforts to convert otherwise-100%-safe spin_lock() into 
> > > spin_lock_irqsave() just to quiet lockdep.
> > > 
> > > Very interesting email, thanks...
> > > 
> > 
> > I suspect this is a bug in my old kmap_atomic debugging patch.  It doesn't
> > know about the implicit irq-disablememnt which interrupt handlers enjoy.  I
> > don't think...
> 
> I suspect here is confusion. The implicit irq-disablement of lockdep
> is actually hiding the warning.
> 
> The code which emits the warning is:
> 
> if (type == KM_IRQ0 || type == KM_IRQ1 || type == KM_BOUNCE_READ ||
> type == KM_BIO_SRC_IRQ || type == KM_BIO_DST_IRQ) {
> if (!irqs_disabled()) {
> WARN_ON(1);
> warn_count--;
> }
> 
> It checks for _NOT_ irqs_disabled. The calling code is
> ata_scsi_rbuf_get() which calls with:
> 
>  buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;
> 
> This happens with interrupts enabled. So the warning is according to
> the well documented km_type enum and the equally well documented
> highmem debug code correct.
> 
> Bjoern decoded it very well, just Jeff jumped to very interesting
> conclusions.
> 

Ah, OK, yes.  ata is wrong.  It must disable interrupts here.  Otherwise
this CPU could get interrupted by some other device whose handler also uses
KM_IRQ0, resulting in data corruption.

--
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.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Thomas Gleixner 
On Mon, 25 Feb 2008, Andrew Morton wrote:
> On Mon, 25 Feb 2008 15:08:35 -0500 Jeff Garzik <[EMAIL PROTECTED]> wrote:
> > There are plenty of drivers that do the same thing that ahci does, in 
> > terms of interrupt handler locking...  and I will definitely push back 
> > on efforts to convert otherwise-100%-safe spin_lock() into 
> > spin_lock_irqsave() just to quiet lockdep.
> > 
> > Very interesting email, thanks...
> > 
> 
> I suspect this is a bug in my old kmap_atomic debugging patch.  It doesn't
> know about the implicit irq-disablememnt which interrupt handlers enjoy.  I
> don't think...

I suspect here is confusion. The implicit irq-disablement of lockdep
is actually hiding the warning.

The code which emits the warning is:

if (type == KM_IRQ0 || type == KM_IRQ1 || type == KM_BOUNCE_READ ||
type == KM_BIO_SRC_IRQ || type == KM_BIO_DST_IRQ) {
if (!irqs_disabled()) {
WARN_ON(1);
warn_count--;
}

It checks for _NOT_ irqs_disabled. The calling code is
ata_scsi_rbuf_get() which calls with:

 buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;

This happens with interrupts enabled. So the warning is according to
the well documented km_type enum and the equally well documented
highmem debug code correct.

Bjoern decoded it very well, just Jeff jumped to very interesting
conclusions.

Thanks,

tglx
--
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.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Andrew Morton 
On Mon, 25 Feb 2008 15:08:35 -0500 Jeff Garzik <[EMAIL PROTECTED]> wrote:

> Bj__rn Steinbrink wrote:
> > On 2008.02.07 00:58:42 +0100, Thomas Gleixner wrote:
> >> current mainline triggers:
> >>
> >> WARNING: at 
> >> /home/tglx/work/kernel/x86/linux-2.6/arch/x86/mm/highmem_32.c:52 
> >> kmap_atomic_prot+0xe5/0x19b()
> >> Modules linked in: ahci(+) sata_sil libata sd_mod scsi_mod raid1 ext3 jbd 
> >> ehci_hcd ohci_hcd uhci_hcd
> >> Pid: 0, comm: swapper Not tainted 2.6.24 #173
> >>  [] warn_on_slowpath+0x41/0x51
> >>  [] ? __enqueue_entity+0x9c/0xa4
> >>  [] ? enqueue_entity+0x124/0x13b
> >>  [] ? enqueue_task_fair+0x41/0x4c
> >>  [] ? _spin_lock_irqsave+0x14/0x2e
> >>  [] ? lock_timer_base+0x1f/0x3e
> >>  [] kmap_atomic_prot+0xe5/0x19b
> >>  [] kmap_atomic+0x14/0x16
> >>  [] ata_scsi_rbuf_get+0x1e/0x2c [libata]
> >>  [] atapi_qc_complete+0x23f/0x289 [libata]
> >>  [] __ata_qc_complete+0x8e/0x93 [libata]
> >>  [] ata_qc_complete+0x115/0x128 [libata]
> >>  [] ata_qc_complete_multiple+0x86/0xa0 [libata]
> >>  [] ahci_interrupt+0x370/0x40d [ahci]
> >>  [] handle_IRQ_event+0x21/0x48
> >>  [] handle_edge_irq+0xc9/0x10a
> >>  [] ? handle_edge_irq+0x0/0x10a
> >>  [] do_IRQ+0x8b/0xb7
> >>  [] common_interrupt+0x23/0x28
> >>  [] ? init_chipset_cmd64x+0xb/0x93
> >>  [] ? mwait_idle_with_hints+0x39/0x3d
> >>  [] ? mwait_idle+0x0/0xf
> >>  [] mwait_idle+0xd/0xf
> >>  [] cpu_idle+0xb0/0xe4
> >>  [] rest_init+0x5d/0x5f
> >>
> >> This is not a new problem. It was pointed out some time ago already,
> >> but now the WARN_ON() finally made it into mainline :)
> >>
> >> The fix is not obvious, as this code seems to be called from various
> >> call sites.
> > 
> > Hm, do you have lockdep enabled? If not, does lockdep make this go away?
> > Because lockdep will set IRQF_DISABLED for all interrupt handlers, and
> > unless that flag is set, handle_IRQ_event will reenable interrupts while
> > the handler is running. And ahci_interrupt only uses a plain spin_lock,
> > so interrupts keep being enabled. The patch below should help with that.
> > 
> > Hmhm, maybe that also solves the deadlock you saw? Dunno...
> > 
> > I can't come up with an useful commit message right now, but I'll resend
> > in suitable form for submission if that thing actually works.
> > 
> > Bj__rn
> > 
> > 
> > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> > index 1db93b6..ae3dbc8 100644
> > --- a/drivers/ata/ahci.c
> > +++ b/drivers/ata/ahci.c
> > @@ -1739,6 +1739,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
> > *dev_instance)
> > unsigned int i, handled = 0;
> > void __iomem *mmio;
> > u32 irq_stat, irq_ack = 0;
> > +   unsigned long flags;
> >  
> > VPRINTK("ENTER\n");
> >  
> > @@ -1751,7 +1752,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
> > *dev_instance)
> > if (!irq_stat)
> > return IRQ_NONE;
> >  
> > -   spin_lock(>lock);
> > +   spin_lock_irqsave(>lock, flags);
> >  
> > for (i = 0; i < host->n_ports; i++) {
> > struct ata_port *ap;
> > @@ -1778,7 +1779,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
> > *dev_instance)
> > handled = 1;
> > }
> >  
> > -   spin_unlock(>lock);
> > +   spin_unlock_irqrestore(>lock, flags);
> 
> If this truly fixes the problem, then lockdep is definitely the problem 
> source.
> 
> There are plenty of drivers that do the same thing that ahci does, in 
> terms of interrupt handler locking...  and I will definitely push back 
> on efforts to convert otherwise-100%-safe spin_lock() into 
> spin_lock_irqsave() just to quiet lockdep.
> 
> Very interesting email, thanks...
> 

I suspect this is a bug in my old kmap_atomic debugging patch.  It doesn't
know about the implicit irq-disablememnt which interrupt handlers enjoy.  I
don't think...

--
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.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Björn Steinbrink 
On 2008.02.25 15:08:35 -0500, Jeff Garzik wrote:
> Björn Steinbrink wrote:
>> Hm, do you have lockdep enabled? If not, does lockdep make this go away?
>> Because lockdep will set IRQF_DISABLED for all interrupt handlers, and
>> unless that flag is set, handle_IRQ_event will reenable interrupts while
>> the handler is running. And ahci_interrupt only uses a plain spin_lock,
>> so interrupts keep being enabled. The patch below should help with that.
>>
>> Hmhm, maybe that also solves the deadlock you saw? Dunno...
>>
>> I can't come up with an useful commit message right now, but I'll resend
>> in suitable form for submission if that thing actually works.
>>
>> Björn
>>
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 1db93b6..ae3dbc8 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -1739,6 +1739,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
>> *dev_instance)
>>  unsigned int i, handled = 0;
>>  void __iomem *mmio;
>>  u32 irq_stat, irq_ack = 0;
>> +unsigned long flags;
>>  VPRINTK("ENTER\n");
>>  @@ -1751,7 +1752,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
>> *dev_instance)
>>  if (!irq_stat)
>>  return IRQ_NONE;
>>  -   spin_lock(>lock);
>> +spin_lock_irqsave(>lock, flags);
>>  for (i = 0; i < host->n_ports; i++) {
>>  struct ata_port *ap;
>> @@ -1778,7 +1779,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
>> *dev_instance)
>>  handled = 1;
>>  }
>>  -   spin_unlock(>lock);
>> +spin_unlock_irqrestore(>lock, flags);
>
> If this truly fixes the problem, then lockdep is definitely the problem  
> source.

Hm, lockdep keeps the interrupts _disabled_. The code that reenables the
interrupts was already in the first revision of Linus' git tree. So
lockdep would actually just hide the problem, not cause it.

Björn
--
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.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Jeff Garzik 

Björn Steinbrink wrote:

On 2008.02.07 00:58:42 +0100, Thomas Gleixner wrote:

current mainline triggers:

WARNING: at /home/tglx/work/kernel/x86/linux-2.6/arch/x86/mm/highmem_32.c:52 
kmap_atomic_prot+0xe5/0x19b()
Modules linked in: ahci(+) sata_sil libata sd_mod scsi_mod raid1 ext3 jbd 
ehci_hcd ohci_hcd uhci_hcd
Pid: 0, comm: swapper Not tainted 2.6.24 #173
 [] warn_on_slowpath+0x41/0x51
 [] ? __enqueue_entity+0x9c/0xa4
 [] ? enqueue_entity+0x124/0x13b
 [] ? enqueue_task_fair+0x41/0x4c
 [] ? _spin_lock_irqsave+0x14/0x2e
 [] ? lock_timer_base+0x1f/0x3e
 [] kmap_atomic_prot+0xe5/0x19b
 [] kmap_atomic+0x14/0x16
 [] ata_scsi_rbuf_get+0x1e/0x2c [libata]
 [] atapi_qc_complete+0x23f/0x289 [libata]
 [] __ata_qc_complete+0x8e/0x93 [libata]
 [] ata_qc_complete+0x115/0x128 [libata]
 [] ata_qc_complete_multiple+0x86/0xa0 [libata]
 [] ahci_interrupt+0x370/0x40d [ahci]
 [] handle_IRQ_event+0x21/0x48
 [] handle_edge_irq+0xc9/0x10a
 [] ? handle_edge_irq+0x0/0x10a
 [] do_IRQ+0x8b/0xb7
 [] common_interrupt+0x23/0x28
 [] ? init_chipset_cmd64x+0xb/0x93
 [] ? mwait_idle_with_hints+0x39/0x3d
 [] ? mwait_idle+0x0/0xf
 [] mwait_idle+0xd/0xf
 [] cpu_idle+0xb0/0xe4
 [] rest_init+0x5d/0x5f

This is not a new problem. It was pointed out some time ago already,
but now the WARN_ON() finally made it into mainline :)

The fix is not obvious, as this code seems to be called from various
call sites.


Hm, do you have lockdep enabled? If not, does lockdep make this go away?
Because lockdep will set IRQF_DISABLED for all interrupt handlers, and
unless that flag is set, handle_IRQ_event will reenable interrupts while
the handler is running. And ahci_interrupt only uses a plain spin_lock,
so interrupts keep being enabled. The patch below should help with that.

Hmhm, maybe that also solves the deadlock you saw? Dunno...

I can't come up with an useful commit message right now, but I'll resend
in suitable form for submission if that thing actually works.

Björn


diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 1db93b6..ae3dbc8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1739,6 +1739,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
*dev_instance)
unsigned int i, handled = 0;
void __iomem *mmio;
u32 irq_stat, irq_ack = 0;
+   unsigned long flags;
 
 	VPRINTK("ENTER\n");
 
@@ -1751,7 +1752,7 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)

if (!irq_stat)
return IRQ_NONE;
 
-	spin_lock(>lock);

+   spin_lock_irqsave(>lock, flags);
 
 	for (i = 0; i < host->n_ports; i++) {

struct ata_port *ap;
@@ -1778,7 +1779,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
*dev_instance)
handled = 1;
}
 
-	spin_unlock(>lock);

+   spin_unlock_irqrestore(>lock, flags);


If this truly fixes the problem, then lockdep is definitely the problem 
source.


There are plenty of drivers that do the same thing that ahci does, in 
terms of interrupt handler locking...  and I will definitely push back 
on efforts to convert otherwise-100%-safe spin_lock() into 
spin_lock_irqsave() just to quiet lockdep.


Very interesting email, thanks...

Jeff



--
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.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Björn Steinbrink 
On 2008.02.07 00:58:42 +0100, Thomas Gleixner wrote:
> current mainline triggers:
> 
> WARNING: at /home/tglx/work/kernel/x86/linux-2.6/arch/x86/mm/highmem_32.c:52 
> kmap_atomic_prot+0xe5/0x19b()
> Modules linked in: ahci(+) sata_sil libata sd_mod scsi_mod raid1 ext3 jbd 
> ehci_hcd ohci_hcd uhci_hcd
> Pid: 0, comm: swapper Not tainted 2.6.24 #173
>  [] warn_on_slowpath+0x41/0x51
>  [] ? __enqueue_entity+0x9c/0xa4
>  [] ? enqueue_entity+0x124/0x13b
>  [] ? enqueue_task_fair+0x41/0x4c
>  [] ? _spin_lock_irqsave+0x14/0x2e
>  [] ? lock_timer_base+0x1f/0x3e
>  [] kmap_atomic_prot+0xe5/0x19b
>  [] kmap_atomic+0x14/0x16
>  [] ata_scsi_rbuf_get+0x1e/0x2c [libata]
>  [] atapi_qc_complete+0x23f/0x289 [libata]
>  [] __ata_qc_complete+0x8e/0x93 [libata]
>  [] ata_qc_complete+0x115/0x128 [libata]
>  [] ata_qc_complete_multiple+0x86/0xa0 [libata]
>  [] ahci_interrupt+0x370/0x40d [ahci]
>  [] handle_IRQ_event+0x21/0x48
>  [] handle_edge_irq+0xc9/0x10a
>  [] ? handle_edge_irq+0x0/0x10a
>  [] do_IRQ+0x8b/0xb7
>  [] common_interrupt+0x23/0x28
>  [] ? init_chipset_cmd64x+0xb/0x93
>  [] ? mwait_idle_with_hints+0x39/0x3d
>  [] ? mwait_idle+0x0/0xf
>  [] mwait_idle+0xd/0xf
>  [] cpu_idle+0xb0/0xe4
>  [] rest_init+0x5d/0x5f
> 
> This is not a new problem. It was pointed out some time ago already,
> but now the WARN_ON() finally made it into mainline :)
> 
> The fix is not obvious, as this code seems to be called from various
> call sites.

Hm, do you have lockdep enabled? If not, does lockdep make this go away?
Because lockdep will set IRQF_DISABLED for all interrupt handlers, and
unless that flag is set, handle_IRQ_event will reenable interrupts while
the handler is running. And ahci_interrupt only uses a plain spin_lock,
so interrupts keep being enabled. The patch below should help with that.

Hmhm, maybe that also solves the deadlock you saw? Dunno...

I can't come up with an useful commit message right now, but I'll resend
in suitable form for submission if that thing actually works.

Björn


diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 1db93b6..ae3dbc8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1739,6 +1739,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
*dev_instance)
unsigned int i, handled = 0;
void __iomem *mmio;
u32 irq_stat, irq_ack = 0;
+   unsigned long flags;
 
VPRINTK("ENTER\n");
 
@@ -1751,7 +1752,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
*dev_instance)
if (!irq_stat)
return IRQ_NONE;
 
-   spin_lock(>lock);
+   spin_lock_irqsave(>lock, flags);
 
for (i = 0; i < host->n_ports; i++) {
struct ata_port *ap;
@@ -1778,7 +1779,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
*dev_instance)
handled = 1;
}
 
-   spin_unlock(>lock);
+   spin_unlock_irqrestore(>lock, flags);
 
VPRINTK("EXIT\n");
 
--
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.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Björn Steinbrink 
On 2008.02.07 00:58:42 +0100, Thomas Gleixner wrote:
 current mainline triggers:
 
 WARNING: at /home/tglx/work/kernel/x86/linux-2.6/arch/x86/mm/highmem_32.c:52 
 kmap_atomic_prot+0xe5/0x19b()
 Modules linked in: ahci(+) sata_sil libata sd_mod scsi_mod raid1 ext3 jbd 
 ehci_hcd ohci_hcd uhci_hcd
 Pid: 0, comm: swapper Not tainted 2.6.24 #173
  [c0126b60] warn_on_slowpath+0x41/0x51
  [c011c5eb] ? __enqueue_entity+0x9c/0xa4
  [c011c717] ? enqueue_entity+0x124/0x13b
  [c011cedb] ? enqueue_task_fair+0x41/0x4c
  [c032ce88] ? _spin_lock_irqsave+0x14/0x2e
  [c012e885] ? lock_timer_base+0x1f/0x3e
  [c011ad6d] kmap_atomic_prot+0xe5/0x19b
  [c011ae37] kmap_atomic+0x14/0x16
  [f88ea218] ata_scsi_rbuf_get+0x1e/0x2c [libata]
  [f88ea821] atapi_qc_complete+0x23f/0x289 [libata]
  [f88e3d00] __ata_qc_complete+0x8e/0x93 [libata]
  [f88e3fc7] ata_qc_complete+0x115/0x128 [libata]
  [f88e8d47] ata_qc_complete_multiple+0x86/0xa0 [libata]
  [f8841a5d] ahci_interrupt+0x370/0x40d [ahci]
  [c01512c6] handle_IRQ_event+0x21/0x48
  [c01521ca] handle_edge_irq+0xc9/0x10a
  [c0152101] ? handle_edge_irq+0x0/0x10a
  [c0107518] do_IRQ+0x8b/0xb7
  [c01055db] common_interrupt+0x23/0x28
  [c032007b] ? init_chipset_cmd64x+0xb/0x93
  [c010316e] ? mwait_idle_with_hints+0x39/0x3d
  [c0103172] ? mwait_idle+0x0/0xf
  [c010317f] mwait_idle+0xd/0xf
  [c0103761] cpu_idle+0xb0/0xe4
  [c031b509] rest_init+0x5d/0x5f
 
 This is not a new problem. It was pointed out some time ago already,
 but now the WARN_ON() finally made it into mainline :)
 
 The fix is not obvious, as this code seems to be called from various
 call sites.

Hm, do you have lockdep enabled? If not, does lockdep make this go away?
Because lockdep will set IRQF_DISABLED for all interrupt handlers, and
unless that flag is set, handle_IRQ_event will reenable interrupts while
the handler is running. And ahci_interrupt only uses a plain spin_lock,
so interrupts keep being enabled. The patch below should help with that.

Hmhm, maybe that also solves the deadlock you saw? Dunno...

I can't come up with an useful commit message right now, but I'll resend
in suitable form for submission if that thing actually works.

Björn


diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 1db93b6..ae3dbc8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1739,6 +1739,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
*dev_instance)
unsigned int i, handled = 0;
void __iomem *mmio;
u32 irq_stat, irq_ack = 0;
+   unsigned long flags;
 
VPRINTK(ENTER\n);
 
@@ -1751,7 +1752,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
*dev_instance)
if (!irq_stat)
return IRQ_NONE;
 
-   spin_lock(host-lock);
+   spin_lock_irqsave(host-lock, flags);
 
for (i = 0; i  host-n_ports; i++) {
struct ata_port *ap;
@@ -1778,7 +1779,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
*dev_instance)
handled = 1;
}
 
-   spin_unlock(host-lock);
+   spin_unlock_irqrestore(host-lock, flags);
 
VPRINTK(EXIT\n);
 
--
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.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Jeff Garzik 

Björn Steinbrink wrote:

On 2008.02.07 00:58:42 +0100, Thomas Gleixner wrote:

current mainline triggers:

WARNING: at /home/tglx/work/kernel/x86/linux-2.6/arch/x86/mm/highmem_32.c:52 
kmap_atomic_prot+0xe5/0x19b()
Modules linked in: ahci(+) sata_sil libata sd_mod scsi_mod raid1 ext3 jbd 
ehci_hcd ohci_hcd uhci_hcd
Pid: 0, comm: swapper Not tainted 2.6.24 #173
 [c0126b60] warn_on_slowpath+0x41/0x51
 [c011c5eb] ? __enqueue_entity+0x9c/0xa4
 [c011c717] ? enqueue_entity+0x124/0x13b
 [c011cedb] ? enqueue_task_fair+0x41/0x4c
 [c032ce88] ? _spin_lock_irqsave+0x14/0x2e
 [c012e885] ? lock_timer_base+0x1f/0x3e
 [c011ad6d] kmap_atomic_prot+0xe5/0x19b
 [c011ae37] kmap_atomic+0x14/0x16
 [f88ea218] ata_scsi_rbuf_get+0x1e/0x2c [libata]
 [f88ea821] atapi_qc_complete+0x23f/0x289 [libata]
 [f88e3d00] __ata_qc_complete+0x8e/0x93 [libata]
 [f88e3fc7] ata_qc_complete+0x115/0x128 [libata]
 [f88e8d47] ata_qc_complete_multiple+0x86/0xa0 [libata]
 [f8841a5d] ahci_interrupt+0x370/0x40d [ahci]
 [c01512c6] handle_IRQ_event+0x21/0x48
 [c01521ca] handle_edge_irq+0xc9/0x10a
 [c0152101] ? handle_edge_irq+0x0/0x10a
 [c0107518] do_IRQ+0x8b/0xb7
 [c01055db] common_interrupt+0x23/0x28
 [c032007b] ? init_chipset_cmd64x+0xb/0x93
 [c010316e] ? mwait_idle_with_hints+0x39/0x3d
 [c0103172] ? mwait_idle+0x0/0xf
 [c010317f] mwait_idle+0xd/0xf
 [c0103761] cpu_idle+0xb0/0xe4
 [c031b509] rest_init+0x5d/0x5f

This is not a new problem. It was pointed out some time ago already,
but now the WARN_ON() finally made it into mainline :)

The fix is not obvious, as this code seems to be called from various
call sites.


Hm, do you have lockdep enabled? If not, does lockdep make this go away?
Because lockdep will set IRQF_DISABLED for all interrupt handlers, and
unless that flag is set, handle_IRQ_event will reenable interrupts while
the handler is running. And ahci_interrupt only uses a plain spin_lock,
so interrupts keep being enabled. The patch below should help with that.

Hmhm, maybe that also solves the deadlock you saw? Dunno...

I can't come up with an useful commit message right now, but I'll resend
in suitable form for submission if that thing actually works.

Björn


diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 1db93b6..ae3dbc8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1739,6 +1739,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
*dev_instance)
unsigned int i, handled = 0;
void __iomem *mmio;
u32 irq_stat, irq_ack = 0;
+   unsigned long flags;
 
 	VPRINTK(ENTER\n);
 
@@ -1751,7 +1752,7 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)

if (!irq_stat)
return IRQ_NONE;
 
-	spin_lock(host-lock);

+   spin_lock_irqsave(host-lock, flags);
 
 	for (i = 0; i  host-n_ports; i++) {

struct ata_port *ap;
@@ -1778,7 +1779,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
*dev_instance)
handled = 1;
}
 
-	spin_unlock(host-lock);

+   spin_unlock_irqrestore(host-lock, flags);


If this truly fixes the problem, then lockdep is definitely the problem 
source.


There are plenty of drivers that do the same thing that ahci does, in 
terms of interrupt handler locking...  and I will definitely push back 
on efforts to convert otherwise-100%-safe spin_lock() into 
spin_lock_irqsave() just to quiet lockdep.


Very interesting email, thanks...

Jeff



--
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.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Björn Steinbrink 
On 2008.02.25 15:08:35 -0500, Jeff Garzik wrote:
 Björn Steinbrink wrote:
 Hm, do you have lockdep enabled? If not, does lockdep make this go away?
 Because lockdep will set IRQF_DISABLED for all interrupt handlers, and
 unless that flag is set, handle_IRQ_event will reenable interrupts while
 the handler is running. And ahci_interrupt only uses a plain spin_lock,
 so interrupts keep being enabled. The patch below should help with that.

 Hmhm, maybe that also solves the deadlock you saw? Dunno...

 I can't come up with an useful commit message right now, but I'll resend
 in suitable form for submission if that thing actually works.

 Björn


 diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
 index 1db93b6..ae3dbc8 100644
 --- a/drivers/ata/ahci.c
 +++ b/drivers/ata/ahci.c
 @@ -1739,6 +1739,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
 *dev_instance)
  unsigned int i, handled = 0;
  void __iomem *mmio;
  u32 irq_stat, irq_ack = 0;
 +unsigned long flags;
  VPRINTK(ENTER\n);
  @@ -1751,7 +1752,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
 *dev_instance)
  if (!irq_stat)
  return IRQ_NONE;
  -   spin_lock(host-lock);
 +spin_lock_irqsave(host-lock, flags);
  for (i = 0; i  host-n_ports; i++) {
  struct ata_port *ap;
 @@ -1778,7 +1779,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
 *dev_instance)
  handled = 1;
  }
  -   spin_unlock(host-lock);
 +spin_unlock_irqrestore(host-lock, flags);

 If this truly fixes the problem, then lockdep is definitely the problem  
 source.

Hm, lockdep keeps the interrupts _disabled_. The code that reenables the
interrupts was already in the first revision of Linus' git tree. So
lockdep would actually just hide the problem, not cause it.

Björn
--
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.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Andrew Morton 
On Mon, 25 Feb 2008 15:08:35 -0500 Jeff Garzik [EMAIL PROTECTED] wrote:

 Bj__rn Steinbrink wrote:
  On 2008.02.07 00:58:42 +0100, Thomas Gleixner wrote:
  current mainline triggers:
 
  WARNING: at 
  /home/tglx/work/kernel/x86/linux-2.6/arch/x86/mm/highmem_32.c:52 
  kmap_atomic_prot+0xe5/0x19b()
  Modules linked in: ahci(+) sata_sil libata sd_mod scsi_mod raid1 ext3 jbd 
  ehci_hcd ohci_hcd uhci_hcd
  Pid: 0, comm: swapper Not tainted 2.6.24 #173
   [c0126b60] warn_on_slowpath+0x41/0x51
   [c011c5eb] ? __enqueue_entity+0x9c/0xa4
   [c011c717] ? enqueue_entity+0x124/0x13b
   [c011cedb] ? enqueue_task_fair+0x41/0x4c
   [c032ce88] ? _spin_lock_irqsave+0x14/0x2e
   [c012e885] ? lock_timer_base+0x1f/0x3e
   [c011ad6d] kmap_atomic_prot+0xe5/0x19b
   [c011ae37] kmap_atomic+0x14/0x16
   [f88ea218] ata_scsi_rbuf_get+0x1e/0x2c [libata]
   [f88ea821] atapi_qc_complete+0x23f/0x289 [libata]
   [f88e3d00] __ata_qc_complete+0x8e/0x93 [libata]
   [f88e3fc7] ata_qc_complete+0x115/0x128 [libata]
   [f88e8d47] ata_qc_complete_multiple+0x86/0xa0 [libata]
   [f8841a5d] ahci_interrupt+0x370/0x40d [ahci]
   [c01512c6] handle_IRQ_event+0x21/0x48
   [c01521ca] handle_edge_irq+0xc9/0x10a
   [c0152101] ? handle_edge_irq+0x0/0x10a
   [c0107518] do_IRQ+0x8b/0xb7
   [c01055db] common_interrupt+0x23/0x28
   [c032007b] ? init_chipset_cmd64x+0xb/0x93
   [c010316e] ? mwait_idle_with_hints+0x39/0x3d
   [c0103172] ? mwait_idle+0x0/0xf
   [c010317f] mwait_idle+0xd/0xf
   [c0103761] cpu_idle+0xb0/0xe4
   [c031b509] rest_init+0x5d/0x5f
 
  This is not a new problem. It was pointed out some time ago already,
  but now the WARN_ON() finally made it into mainline :)
 
  The fix is not obvious, as this code seems to be called from various
  call sites.
  
  Hm, do you have lockdep enabled? If not, does lockdep make this go away?
  Because lockdep will set IRQF_DISABLED for all interrupt handlers, and
  unless that flag is set, handle_IRQ_event will reenable interrupts while
  the handler is running. And ahci_interrupt only uses a plain spin_lock,
  so interrupts keep being enabled. The patch below should help with that.
  
  Hmhm, maybe that also solves the deadlock you saw? Dunno...
  
  I can't come up with an useful commit message right now, but I'll resend
  in suitable form for submission if that thing actually works.
  
  Bj__rn
  
  
  diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
  index 1db93b6..ae3dbc8 100644
  --- a/drivers/ata/ahci.c
  +++ b/drivers/ata/ahci.c
  @@ -1739,6 +1739,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
  *dev_instance)
  unsigned int i, handled = 0;
  void __iomem *mmio;
  u32 irq_stat, irq_ack = 0;
  +   unsigned long flags;
   
  VPRINTK(ENTER\n);
   
  @@ -1751,7 +1752,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
  *dev_instance)
  if (!irq_stat)
  return IRQ_NONE;
   
  -   spin_lock(host-lock);
  +   spin_lock_irqsave(host-lock, flags);
   
  for (i = 0; i  host-n_ports; i++) {
  struct ata_port *ap;
  @@ -1778,7 +1779,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
  *dev_instance)
  handled = 1;
  }
   
  -   spin_unlock(host-lock);
  +   spin_unlock_irqrestore(host-lock, flags);
 
 If this truly fixes the problem, then lockdep is definitely the problem 
 source.
 
 There are plenty of drivers that do the same thing that ahci does, in 
 terms of interrupt handler locking...  and I will definitely push back 
 on efforts to convert otherwise-100%-safe spin_lock() into 
 spin_lock_irqsave() just to quiet lockdep.
 
 Very interesting email, thanks...
 

I suspect this is a bug in my old kmap_atomic debugging patch.  It doesn't
know about the implicit irq-disablememnt which interrupt handlers enjoy.  I
don't think...

--
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.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Thomas Gleixner 
On Mon, 25 Feb 2008, Andrew Morton wrote:
 On Mon, 25 Feb 2008 15:08:35 -0500 Jeff Garzik [EMAIL PROTECTED] wrote:
  There are plenty of drivers that do the same thing that ahci does, in 
  terms of interrupt handler locking...  and I will definitely push back 
  on efforts to convert otherwise-100%-safe spin_lock() into 
  spin_lock_irqsave() just to quiet lockdep.
  
  Very interesting email, thanks...
  
 
 I suspect this is a bug in my old kmap_atomic debugging patch.  It doesn't
 know about the implicit irq-disablememnt which interrupt handlers enjoy.  I
 don't think...

I suspect here is confusion. The implicit irq-disablement of lockdep
is actually hiding the warning.

The code which emits the warning is:

if (type == KM_IRQ0 || type == KM_IRQ1 || type == KM_BOUNCE_READ ||
type == KM_BIO_SRC_IRQ || type == KM_BIO_DST_IRQ) {
if (!irqs_disabled()) {
WARN_ON(1);
warn_count--;
}

It checks for _NOT_ irqs_disabled. The calling code is
ata_scsi_rbuf_get() which calls with:

 buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg-offset;

This happens with interrupts enabled. So the warning is according to
the well documented km_type enum and the equally well documented
highmem debug code correct.

Bjoern decoded it very well, just Jeff jumped to very interesting
conclusions.

Thanks,

tglx
--
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.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Andrew Morton 
On Mon, 25 Feb 2008 23:01:59 +0100 (CET) Thomas Gleixner [EMAIL PROTECTED] 
wrote:

 On Mon, 25 Feb 2008, Andrew Morton wrote:
  On Mon, 25 Feb 2008 15:08:35 -0500 Jeff Garzik [EMAIL PROTECTED] wrote:
   There are plenty of drivers that do the same thing that ahci does, in 
   terms of interrupt handler locking...  and I will definitely push back 
   on efforts to convert otherwise-100%-safe spin_lock() into 
   spin_lock_irqsave() just to quiet lockdep.
   
   Very interesting email, thanks...
   
  
  I suspect this is a bug in my old kmap_atomic debugging patch.  It doesn't
  know about the implicit irq-disablememnt which interrupt handlers enjoy.  I
  don't think...
 
 I suspect here is confusion. The implicit irq-disablement of lockdep
 is actually hiding the warning.
 
 The code which emits the warning is:
 
 if (type == KM_IRQ0 || type == KM_IRQ1 || type == KM_BOUNCE_READ ||
 type == KM_BIO_SRC_IRQ || type == KM_BIO_DST_IRQ) {
 if (!irqs_disabled()) {
 WARN_ON(1);
 warn_count--;
 }
 
 It checks for _NOT_ irqs_disabled. The calling code is
 ata_scsi_rbuf_get() which calls with:
 
  buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg-offset;
 
 This happens with interrupts enabled. So the warning is according to
 the well documented km_type enum and the equally well documented
 highmem debug code correct.
 
 Bjoern decoded it very well, just Jeff jumped to very interesting
 conclusions.
 

Ah, OK, yes.  ata is wrong.  It must disable interrupts here.  Otherwise
this CPU could get interrupted by some other device whose handler also uses
KM_IRQ0, resulting in data corruption.

--
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.24-git: kmap_atomic() WARN_ON()

2008-02-25 Thread Jeff Garzik 

Welcome to test this... (attached, not tested nor even compiled, really)

Jeff



diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0562b0a..7b1f1ee 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1694,12 +1694,17 @@ void ata_scsi_rbuf_fill(struct ata_scsi_args *args,
u8 *rbuf;
unsigned int buflen, rc;
struct scsi_cmnd *cmd = args-cmd;
+   unsigned long flags;
+
+   local_irq_save(flags);
 
buflen = ata_scsi_rbuf_get(cmd, rbuf);
memset(rbuf, 0, buflen);
rc = actor(args, rbuf, buflen);
ata_scsi_rbuf_put(cmd, rbuf);
 
+   local_irq_restore(flags);
+
if (rc == 0)
cmd-result = SAM_STAT_GOOD;
args-done(cmd);
@@ -2473,6 +2478,9 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
if ((scsicmd[0] == INQUIRY)  ((scsicmd[1]  0x03) == 0)) {
u8 *buf = NULL;
unsigned int buflen;
+   unsigned long flags;
+
+   local_irq_save(flags);
 
buflen = ata_scsi_rbuf_get(cmd, buf);
 
@@ -2490,6 +2498,8 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
}
 
ata_scsi_rbuf_put(cmd, buf);
+
+   local_irq_restore(flags);
}
 
cmd-result = SAM_STAT_GOOD;

Re: 2.6.24-git: kmap_atomic() WARN_ON()

2008-02-13 Thread Thomas Gleixner 
On Wed, 13 Feb 2008, Rafael J. Wysocki wrote:

> Hi Thomas,
> 
> On Thursday, 7 of February 2008, Thomas Gleixner wrote:
> > current mainline triggers:
> 
> Has the issue been fixed in the meantime?

Nope. 

Just for reference: http://lkml.org/lkml/2007/1/14/38 looks
frighteningly similar.

Thanks,

tglx
--
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.24-git: kmap_atomic() WARN_ON()

2008-02-13 Thread Rafael J. Wysocki 
Hi Thomas,

On Thursday, 7 of February 2008, Thomas Gleixner wrote:
> current mainline triggers:

Has the issue been fixed in the meantime?

> WARNING: at /home/tglx/work/kernel/x86/linux-2.6/arch/x86/mm/highmem_32.c:52 
> kmap_atomic_prot+0xe5/0x19b()
> Modules linked in: ahci(+) sata_sil libata sd_mod scsi_mod raid1 ext3 jbd 
> ehci_hcd ohci_hcd uhci_hcd
> Pid: 0, comm: swapper Not tainted 2.6.24 #173
>  [] warn_on_slowpath+0x41/0x51
>  [] ? __enqueue_entity+0x9c/0xa4
>  [] ? enqueue_entity+0x124/0x13b
>  [] ? enqueue_task_fair+0x41/0x4c
>  [] ? _spin_lock_irqsave+0x14/0x2e
>  [] ? lock_timer_base+0x1f/0x3e
>  [] kmap_atomic_prot+0xe5/0x19b
>  [] kmap_atomic+0x14/0x16
>  [] ata_scsi_rbuf_get+0x1e/0x2c [libata]
>  [] atapi_qc_complete+0x23f/0x289 [libata]
>  [] __ata_qc_complete+0x8e/0x93 [libata]
>  [] ata_qc_complete+0x115/0x128 [libata]
>  [] ata_qc_complete_multiple+0x86/0xa0 [libata]
>  [] ahci_interrupt+0x370/0x40d [ahci]
>  [] handle_IRQ_event+0x21/0x48
>  [] handle_edge_irq+0xc9/0x10a
>  [] ? handle_edge_irq+0x0/0x10a
>  [] do_IRQ+0x8b/0xb7
>  [] common_interrupt+0x23/0x28
>  [] ? init_chipset_cmd64x+0xb/0x93
>  [] ? mwait_idle_with_hints+0x39/0x3d
>  [] ? mwait_idle+0x0/0xf
>  [] mwait_idle+0xd/0xf
>  [] cpu_idle+0xb0/0xe4
>  [] rest_init+0x5d/0x5f
> 
> This is not a new problem. It was pointed out some time ago already,
> but now the WARN_ON() finally made it into mainline :)
> 
> The fix is not obvious, as this code seems to be called from various
> call sites.
> 
> Thanks,
>   tglx
--
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.24-git: kmap_atomic() WARN_ON()

2008-02-13 Thread Rafael J. Wysocki 
Hi Thomas,

On Thursday, 7 of February 2008, Thomas Gleixner wrote:
 current mainline triggers:

Has the issue been fixed in the meantime?

 WARNING: at /home/tglx/work/kernel/x86/linux-2.6/arch/x86/mm/highmem_32.c:52 
 kmap_atomic_prot+0xe5/0x19b()
 Modules linked in: ahci(+) sata_sil libata sd_mod scsi_mod raid1 ext3 jbd 
 ehci_hcd ohci_hcd uhci_hcd
 Pid: 0, comm: swapper Not tainted 2.6.24 #173
  [c0126b60] warn_on_slowpath+0x41/0x51
  [c011c5eb] ? __enqueue_entity+0x9c/0xa4
  [c011c717] ? enqueue_entity+0x124/0x13b
  [c011cedb] ? enqueue_task_fair+0x41/0x4c
  [c032ce88] ? _spin_lock_irqsave+0x14/0x2e
  [c012e885] ? lock_timer_base+0x1f/0x3e
  [c011ad6d] kmap_atomic_prot+0xe5/0x19b
  [c011ae37] kmap_atomic+0x14/0x16
  [f88ea218] ata_scsi_rbuf_get+0x1e/0x2c [libata]
  [f88ea821] atapi_qc_complete+0x23f/0x289 [libata]
  [f88e3d00] __ata_qc_complete+0x8e/0x93 [libata]
  [f88e3fc7] ata_qc_complete+0x115/0x128 [libata]
  [f88e8d47] ata_qc_complete_multiple+0x86/0xa0 [libata]
  [f8841a5d] ahci_interrupt+0x370/0x40d [ahci]
  [c01512c6] handle_IRQ_event+0x21/0x48
  [c01521ca] handle_edge_irq+0xc9/0x10a
  [c0152101] ? handle_edge_irq+0x0/0x10a
  [c0107518] do_IRQ+0x8b/0xb7
  [c01055db] common_interrupt+0x23/0x28
  [c032007b] ? init_chipset_cmd64x+0xb/0x93
  [c010316e] ? mwait_idle_with_hints+0x39/0x3d
  [c0103172] ? mwait_idle+0x0/0xf
  [c010317f] mwait_idle+0xd/0xf
  [c0103761] cpu_idle+0xb0/0xe4
  [c031b509] rest_init+0x5d/0x5f
 
 This is not a new problem. It was pointed out some time ago already,
 but now the WARN_ON() finally made it into mainline :)
 
 The fix is not obvious, as this code seems to be called from various
 call sites.
 
 Thanks,
   tglx
--
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.24-git: kmap_atomic() WARN_ON()

2008-02-13 Thread Thomas Gleixner 
On Wed, 13 Feb 2008, Rafael J. Wysocki wrote:

 Hi Thomas,
 
 On Thursday, 7 of February 2008, Thomas Gleixner wrote:
  current mainline triggers:
 
 Has the issue been fixed in the meantime?

Nope. 

Just for reference: http://lkml.org/lkml/2007/1/14/38 looks
frighteningly similar.

Thanks,

tglx
--
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/