LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jeff Garzik <jgarzik@pobox.com>
To: "Björn Steinbrink" <B.Steinbrink@gmx.de>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: 2.6.24-git: kmap_atomic() WARN_ON()
Date: Mon, 25 Feb 2008 15:08:35 -0500	[thread overview]
Message-ID: <47C32043.6090809@pobox.com> (raw)
In-Reply-To: <20080225195924.GA23176@atjola.homenet>

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




  reply	other threads:[~2008-02-25 20:08 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-06 23:58 Thomas Gleixner
2008-02-13 22:39 ` Rafael J. Wysocki
2008-02-14  1:13   ` Thomas Gleixner
2008-02-25 19:59 ` Björn Steinbrink
2008-02-25 20:08   ` Jeff Garzik [this message]
2008-02-25 20:35     ` Björn Steinbrink
2008-02-25 20:40     ` Andrew Morton
2008-02-25 22:01       ` Thomas Gleixner
2008-02-25 22:17         ` Andrew Morton
2008-02-25 23:19         ` Jeff Garzik
2008-02-26  8:39           ` Ingo Molnar
2008-02-26 16:32             ` Jeff Garzik
2008-02-26 18:19               ` Andrew Morton
2008-02-26 20:49                 ` Ingo Molnar
2008-02-26 21:37                   ` Andrew Morton
2008-02-26 22:59                     ` Jeff Garzik
2008-02-27  0:02                       ` Alan Cox
2008-02-26 23:49                   ` Nick Piggin
2008-02-26  8:50           ` Thomas Gleixner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=47C32043.6090809@pobox.com \
    --to=jgarzik@pobox.com \
    --cc=B.Steinbrink@gmx.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --subject='Re: 2.6.24-git: kmap_atomic() WARN_ON()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).