* [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL @ 2014-04-14 21:47 David Milburn 2014-04-15 16:33 ` Tejun Heo 0 siblings, 1 reply; 15+ messages in thread From: David Milburn @ 2014-04-14 21:47 UTC (permalink / raw) To: tj; +Cc: linux-ide System may crash in ahci_hw_interrupt() or ahci_thread_fn() when accessing the interrupt status in a port's private_data if the port is actually a DUMMY port. # lspci | grep ATA 00:1f.2 SATA controller: Intel Corporation 82801JI (ICH10 Family) SATA AHCI Controller # systemctl status kdump (to ensure kdump service is running) # echo c > /proc/sysrq-trigger <snip console output for linux-3.15-rc1> [ 9.352080] ahci 0000:00:1f.2: AHCI 0001.0200 32 slots 6 ports 3 Gbps 0x1 impl SATA mode [ 9.352084] ahci 0000:00:1f.2: flags: 64bit ncq sntf pm led clo pio slum part ccc [ 9.368155] Console: switching to colour frame buffer device 128x48 [ 9.439759] mgag200 0000:11:00.0: fb0: mgadrmfb frame buffer device [ 9.446765] mgag200 0000:11:00.0: registered panic notifier [ 9.470166] scsi1 : ahci [ 9.479166] scsi2 : ahci [ 9.488172] scsi3 : ahci [ 9.497174] scsi4 : ahci [ 9.506175] scsi5 : ahci [ 9.515174] scsi6 : ahci [ 9.518181] ata1: SATA max UDMA/133 abar m2048@0x95c00000 port 0x95c00100 irq 91 [ 9.526448] ata2: DUMMY [ 9.529182] ata3: DUMMY [ 9.531916] ata4: DUMMY [ 9.534650] ata5: DUMMY [ 9.537382] ata6: DUMMY [ 9.576196] [drm] Initialized mgag200 1.0.0 20110418 for 0000:11:00.0 on minor 0 [ 9.845257] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) [ 9.865161] ata1.00: ATAPI: Optiarc DVD RW AD-7580S, FX04, max UDMA/100 [ 9.891407] ata1.00: configured for UDMA/100 [ 9.900525] scsi 1:0:0:0: CD-ROM Optiarc DVD RW AD-7580S FX04 PQ: 0 ANSI: 5 [ 10.247399] iTCO_vendor_support: vendor-support=0 [ 10.261572] iTCO_wdt: Intel TCO WatchDog Timer Driver v1.11 [ 10.269764] iTCO_wdt: unable to reset NO_REBOOT flag, device disabled by hardware/BIOS [ 10.301932] sd 0:2:0:0: [sda] 570310656 512-byte logical blocks: (291 GB/271 GiB) [ 10.317085] sd 0:2:0:0: [sda] Write Protect is off [ 10.328326] sd 0:2:0:0: [sda] Write cache: disabled, read cache: disabled, supports DPO and FUA [ 10.375452] BUG: unable to handle kernel NULL pointer dereference at 000000000000003c [ 10.384217] IP: [<ffffffffa0133df0>] ahci_hw_interrupt+0x100/0x130 [libahci] [ 10.392101] PGD 0 [ 10.394353] Oops: 0000 [#1] SMP [ 10.397978] Modules linked in: sr_mod(+) cdrom sd_mod iTCO_wdt crc_t10dif iTCO_vendor_support crct10dif_common ahci libahci libata lpc_ich mfd_core mgag200 syscopyarea sysfillrect sysimgblt i2c_algo_bit drm_kms_helper ttm drm i2c_core megaraid_sas dm_mirror dm_region_hash dm_log dm_mod [ 10.426499] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.15.0-rc1 #1 [ 10.433495] Hardware name: QCI QSSC-S4R/QSSC-S4R, BIOS QSSC-S4R.QCI.01.00.S013.032920111005 03/29/2011 [ 10.443886] task: ffffffff81906460 ti: ffffffff818f0000 task.ti: ffffffff818f0000 [ 10.452239] RIP: 0010:[<ffffffffa0133df0>] [<ffffffffa0133df0>] ahci_hw_interrupt+0x100/0x130 [libahci] [ 10.462838] RSP: 0018:ffff880033c03d98 EFLAGS: 00010046 [ 10.468767] RAX: 0000000000a400a4 RBX: ffff880029a6bc18 RCX: 00000000fffffffa [ 10.476731] RDX: 00000000000000a4 RSI: ffff880029bb0000 RDI: ffff880029a6bc18 [ 10.484696] RBP: ffff880033c03dc8 R08: 0000000000000000 R09: ffff88002f800490 [ 10.492661] R10: 0000000000000000 R11: 0000000000000005 R12: 0000000000000000 [ 10.500625] R13: ffff880029a6bd98 R14: 0000000000000000 R15: ffffc90000194000 [ 10.508590] FS: 0000000000000000(0000) GS:ffff880033c00000(0000) knlGS:0000000000000000 [ 10.517623] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b [ 10.524035] CR2: 000000000000003c CR3: 00000000328ff000 CR4: 00000000000007b0 [ 10.531999] Stack: [ 10.534241] 0000000000000017 ffff880031ba7d00 000000000000005c ffff880031ba7d00 [ 10.542535] 0000000000000000 000000000000005c ffff880033c03e10 ffffffff810c2a1e [ 10.550827] ffff880031ae2900 000000008108fb4f ffff880031ae2900 ffff880031ae2984 [ 10.559121] Call Trace: [ 10.561849] <IRQ> [ 10.563994] [<ffffffff810c2a1e>] handle_irq_event_percpu+0x3e/0x1a0 [ 10.571309] [<ffffffff810c2bbd>] handle_irq_event+0x3d/0x60 [ 10.577631] [<ffffffff810c4fdd>] try_one_irq.isra.6+0x8d/0xf0 [ 10.584142] [<ffffffff810c5313>] note_interrupt+0x173/0x1f0 [ 10.590460] [<ffffffff810c2a8e>] handle_irq_event_percpu+0xae/0x1a0 [ 10.597554] [<ffffffff810c2bbd>] handle_irq_event+0x3d/0x60 [ 10.603872] [<ffffffff810c5727>] handle_edge_irq+0x77/0x130 [ 10.610199] [<ffffffff81014b8f>] handle_irq+0xbf/0x150 [ 10.616040] [<ffffffff8109ff4e>] ? vtime_account_idle+0xe/0x50 [ 10.622654] [<ffffffff815fca1a>] ? atomic_notifier_call_chain+0x1a/0x20 [ 10.630140] [<ffffffff816038cf>] do_IRQ+0x4f/0xf0 [ 10.635490] [<ffffffff815f8aed>] common_interrupt+0x6d/0x6d [ 10.641805] <EOI> [ 10.643950] [<ffffffff8149ca9f>] ? cpuidle_enter_state+0x4f/0xc0 [ 10.650972] [<ffffffff8149ca98>] ? cpuidle_enter_state+0x48/0xc0 [ 10.657775] [<ffffffff8149cb47>] cpuidle_enter+0x17/0x20 [ 10.663807] [<ffffffff810b0070>] cpu_startup_entry+0x2c0/0x3d0 [ 10.670423] [<ffffffff815dfcc7>] rest_init+0x77/0x80 [ 10.676065] [<ffffffff81a60f47>] start_kernel+0x40f/0x41a [ 10.682190] [<ffffffff81a60941>] ? repair_env_string+0x5c/0x5c [ 10.688799] [<ffffffff81a60120>] ? early_idt_handlers+0x120/0x120 [ 10.695699] [<ffffffff81a605ee>] x86_64_start_reservations+0x2a/0x2c [ 10.702889] [<ffffffff81a60733>] x86_64_start_kernel+0x143/0x152 [ 10.709689] Code: a0 fc ff 85 c0 8b 4d d4 74 c3 48 8b 7b 08 89 ca 48 c7 c6 60 66 13 a0 31 c0 e8 9d 70 28 e1 8b 4d d4 eb aa 0f 1f 84 00 00 00 00 00 <45> 8b 64 24 3c 48 89 df e8 23 47 4c e1 41 83 fc 01 19 c0 48 83 [ 10.731470] RIP [<ffffffffa0133df0>] ahci_hw_interrupt+0x100/0x130 [libahci] [ 10.739441] RSP <ffff880033c03d98> [ 10.743333] CR2: 000000000000003c [ 10.747032] ---[ end trace b6e82636970e2690 ]--- [ 10.760190] Kernel panic - not syncing: Fatal exception in interrupt [ 10.767291] Kernel Offset: 0x0 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffff9fffffff) Signed-off-by: David Milburn <dmilburn@redhat.com> --- drivers/ata/libahci.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index 6bd4f66..401924e 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -1791,9 +1791,14 @@ irqreturn_t ahci_thread_fn(int irq, void *dev_instance) u32 status; spin_lock_irqsave(&ap->host->lock, flags); - status = pp->intr_status; - if (status) - pp->intr_status = 0; + if (!ata_port_is_dummy(ap)) { + status = pp->intr_status; + if (status) + pp->intr_status = 0; + } else { + spin_unlock_irqrestore(&ap->host->lock, flags); + return IRQ_HANDLED; + } spin_unlock_irqrestore(&ap->host->lock, flags); spin_lock_bh(ap->lock); @@ -1833,8 +1838,11 @@ irqreturn_t ahci_hw_interrupt(int irq, void *dev_instance) irq_stat = readl(mmio + HOST_IRQ_STAT); if (!irq_stat) { - u32 status = pp->intr_status; + u32 status = 0; + if (!ata_port_is_dummy(ap_this)) + status = pp->intr_status; + spin_unlock(&host->lock); VPRINTK("EXIT\n"); ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-14 21:47 [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL David Milburn @ 2014-04-15 16:33 ` Tejun Heo 2014-04-15 18:18 ` David Milburn 0 siblings, 1 reply; 15+ messages in thread From: Tejun Heo @ 2014-04-15 16:33 UTC (permalink / raw) To: David Milburn; +Cc: linux-ide, Alexander Gordeev (cc'ing Alexander) On Mon, Apr 14, 2014 at 04:47:18PM -0500, David Milburn wrote: > System may crash in ahci_hw_interrupt() or ahci_thread_fn() when accessing > the interrupt status in a port's private_data if the port is actually a > DUMMY port. > > # lspci | grep ATA > 00:1f.2 SATA controller: Intel Corporation 82801JI (ICH10 Family) SATA AHCI Controller > > # systemctl status kdump (to ensure kdump service is running) > # echo c > /proc/sysrq-trigger > > <snip console output for linux-3.15-rc1> ... > [ 10.375452] BUG: unable to handle kernel NULL pointer dereference at 000000000000003c > [ 10.384217] IP: [<ffffffffa0133df0>] ahci_hw_interrupt+0x100/0x130 [libahci] ... > [ 10.559121] Call Trace: > [ 10.561849] <IRQ> > [ 10.563994] [<ffffffff810c2a1e>] handle_irq_event_percpu+0x3e/0x1a0 > [ 10.571309] [<ffffffff810c2bbd>] handle_irq_event+0x3d/0x60 > [ 10.577631] [<ffffffff810c4fdd>] try_one_irq.isra.6+0x8d/0xf0 > [ 10.584142] [<ffffffff810c5313>] note_interrupt+0x173/0x1f0 > [ 10.590460] [<ffffffff810c2a8e>] handle_irq_event_percpu+0xae/0x1a0 > [ 10.597554] [<ffffffff810c2bbd>] handle_irq_event+0x3d/0x60 > [ 10.603872] [<ffffffff810c5727>] handle_edge_irq+0x77/0x130 > [ 10.610199] [<ffffffff81014b8f>] handle_irq+0xbf/0x150 > [ 10.616040] [<ffffffff8109ff4e>] ? vtime_account_idle+0xe/0x50 > [ 10.622654] [<ffffffff815fca1a>] ? atomic_notifier_call_chain+0x1a/0x20 > [ 10.630140] [<ffffffff816038cf>] do_IRQ+0x4f/0xf0 > [ 10.635490] [<ffffffff815f8aed>] common_interrupt+0x6d/0x6d > [ 10.641805] <EOI> > [ 10.643950] [<ffffffff8149ca9f>] ? cpuidle_enter_state+0x4f/0xc0 > [ 10.650972] [<ffffffff8149ca98>] ? cpuidle_enter_state+0x48/0xc0 > [ 10.657775] [<ffffffff8149cb47>] cpuidle_enter+0x17/0x20 > [ 10.663807] [<ffffffff810b0070>] cpu_startup_entry+0x2c0/0x3d0 > [ 10.670423] [<ffffffff815dfcc7>] rest_init+0x77/0x80 > [ 10.676065] [<ffffffff81a60f47>] start_kernel+0x40f/0x41a > [ 10.682190] [<ffffffff81a60941>] ? repair_env_string+0x5c/0x5c > [ 10.688799] [<ffffffff81a60120>] ? early_idt_handlers+0x120/0x120 > [ 10.695699] [<ffffffff81a605ee>] x86_64_start_reservations+0x2a/0x2c > [ 10.702889] [<ffffffff81a60733>] x86_64_start_kernel+0x143/0x152 ... > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > index 6bd4f66..401924e 100644 > --- a/drivers/ata/libahci.c > +++ b/drivers/ata/libahci.c > @@ -1791,9 +1791,14 @@ irqreturn_t ahci_thread_fn(int irq, void *dev_instance) > u32 status; > > spin_lock_irqsave(&ap->host->lock, flags); > - status = pp->intr_status; > - if (status) > - pp->intr_status = 0; > + if (!ata_port_is_dummy(ap)) { > + status = pp->intr_status; > + if (status) > + pp->intr_status = 0; > + } else { > + spin_unlock_irqrestore(&ap->host->lock, flags); > + return IRQ_HANDLED; > + } > spin_unlock_irqrestore(&ap->host->lock, flags); > > spin_lock_bh(ap->lock); > @@ -1833,8 +1838,11 @@ irqreturn_t ahci_hw_interrupt(int irq, void *dev_instance) > irq_stat = readl(mmio + HOST_IRQ_STAT); > > if (!irq_stat) { > - u32 status = pp->intr_status; > + u32 status = 0; > > + if (!ata_port_is_dummy(ap_this)) > + status = pp->intr_status; > + > spin_unlock(&host->lock); > > VPRINTK("EXIT\n"); I kinda really dislike that we're littering interrupt handling path instead of ensuring that this doesn't happen for dummy ports. Alexander, do we need to request irqs for dummy ports? If so, wouldn't it be better to use noop handlers than doing the above? Thanks. -- tejun ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-15 16:33 ` Tejun Heo @ 2014-04-15 18:18 ` David Milburn 2014-04-15 18:20 ` Tejun Heo 0 siblings, 1 reply; 15+ messages in thread From: David Milburn @ 2014-04-15 18:18 UTC (permalink / raw) To: Tejun Heo; +Cc: linux-ide, agordeev On Tue, Apr 15, 2014 at 12:33:09PM -0400, Tejun Heo wrote: > (cc'ing Alexander) > > On Mon, Apr 14, 2014 at 04:47:18PM -0500, David Milburn wrote: > > System may crash in ahci_hw_interrupt() or ahci_thread_fn() when accessing > > the interrupt status in a port's private_data if the port is actually a > > DUMMY port. > > > > # lspci | grep ATA > > 00:1f.2 SATA controller: Intel Corporation 82801JI (ICH10 Family) SATA AHCI Controller > > > > # systemctl status kdump (to ensure kdump service is running) > > # echo c > /proc/sysrq-trigger > > > > <snip console output for linux-3.15-rc1> > ... > > [ 10.375452] BUG: unable to handle kernel NULL pointer dereference at 000000000000003c > > [ 10.384217] IP: [<ffffffffa0133df0>] ahci_hw_interrupt+0x100/0x130 [libahci] > ... > > [ 10.559121] Call Trace: > > [ 10.561849] <IRQ> > > [ 10.563994] [<ffffffff810c2a1e>] handle_irq_event_percpu+0x3e/0x1a0 > > [ 10.571309] [<ffffffff810c2bbd>] handle_irq_event+0x3d/0x60 > > [ 10.577631] [<ffffffff810c4fdd>] try_one_irq.isra.6+0x8d/0xf0 > > [ 10.584142] [<ffffffff810c5313>] note_interrupt+0x173/0x1f0 > > [ 10.590460] [<ffffffff810c2a8e>] handle_irq_event_percpu+0xae/0x1a0 > > [ 10.597554] [<ffffffff810c2bbd>] handle_irq_event+0x3d/0x60 > > [ 10.603872] [<ffffffff810c5727>] handle_edge_irq+0x77/0x130 > > [ 10.610199] [<ffffffff81014b8f>] handle_irq+0xbf/0x150 > > [ 10.616040] [<ffffffff8109ff4e>] ? vtime_account_idle+0xe/0x50 > > [ 10.622654] [<ffffffff815fca1a>] ? atomic_notifier_call_chain+0x1a/0x20 > > [ 10.630140] [<ffffffff816038cf>] do_IRQ+0x4f/0xf0 > > [ 10.635490] [<ffffffff815f8aed>] common_interrupt+0x6d/0x6d > > [ 10.641805] <EOI> > > [ 10.643950] [<ffffffff8149ca9f>] ? cpuidle_enter_state+0x4f/0xc0 > > [ 10.650972] [<ffffffff8149ca98>] ? cpuidle_enter_state+0x48/0xc0 > > [ 10.657775] [<ffffffff8149cb47>] cpuidle_enter+0x17/0x20 > > [ 10.663807] [<ffffffff810b0070>] cpu_startup_entry+0x2c0/0x3d0 > > [ 10.670423] [<ffffffff815dfcc7>] rest_init+0x77/0x80 > > [ 10.676065] [<ffffffff81a60f47>] start_kernel+0x40f/0x41a > > [ 10.682190] [<ffffffff81a60941>] ? repair_env_string+0x5c/0x5c > > [ 10.688799] [<ffffffff81a60120>] ? early_idt_handlers+0x120/0x120 > > [ 10.695699] [<ffffffff81a605ee>] x86_64_start_reservations+0x2a/0x2c > > [ 10.702889] [<ffffffff81a60733>] x86_64_start_kernel+0x143/0x152 > ... > > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > > index 6bd4f66..401924e 100644 > > --- a/drivers/ata/libahci.c > > +++ b/drivers/ata/libahci.c > > @@ -1791,9 +1791,14 @@ irqreturn_t ahci_thread_fn(int irq, void *dev_instance) > > u32 status; > > > > spin_lock_irqsave(&ap->host->lock, flags); > > - status = pp->intr_status; > > - if (status) > > - pp->intr_status = 0; > > + if (!ata_port_is_dummy(ap)) { > > + status = pp->intr_status; > > + if (status) > > + pp->intr_status = 0; > > + } else { > > + spin_unlock_irqrestore(&ap->host->lock, flags); > > + return IRQ_HANDLED; > > + } > > spin_unlock_irqrestore(&ap->host->lock, flags); > > > > spin_lock_bh(ap->lock); > > @@ -1833,8 +1838,11 @@ irqreturn_t ahci_hw_interrupt(int irq, void *dev_instance) > > irq_stat = readl(mmio + HOST_IRQ_STAT); > > > > if (!irq_stat) { > > - u32 status = pp->intr_status; > > + u32 status = 0; > > > > + if (!ata_port_is_dummy(ap_this)) > > + status = pp->intr_status; > > + > > spin_unlock(&host->lock); > > > > VPRINTK("EXIT\n"); > > I kinda really dislike that we're littering interrupt handling path > instead of ensuring that this doesn't happen for dummy ports. > Alexander, do we need to request irqs for dummy ports? If so, > wouldn't it be better to use noop handlers than doing the above? > > Thanks. > > -- > tejun Hi Tejun, Alexander, This patch also solves the problem, would this better? Thanks, David drivers/ata/ahci.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index 5a0bf8e..831b1b4 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -1236,14 +1236,14 @@ int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis) struct ahci_port_priv *pp = host->ports[i]->private_data; /* pp is NULL for dummy ports */ - if (pp) + if (pp) { desc = pp->irq_desc; - else - desc = dev_driver_string(host->dev); - rc = devm_request_threaded_irq(host->dev, - irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED, - desc, host->ports[i]); + rc = devm_request_threaded_irq(host->dev, + irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED, + desc, host->ports[i]); + } + if (rc) goto out_free_irqs; } ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-15 18:18 ` David Milburn @ 2014-04-15 18:20 ` Tejun Heo 2014-04-16 7:39 ` Alexander Gordeev 0 siblings, 1 reply; 15+ messages in thread From: Tejun Heo @ 2014-04-15 18:20 UTC (permalink / raw) To: David Milburn; +Cc: linux-ide, agordeev On Tue, Apr 15, 2014 at 01:18:12PM -0500, David Milburn wrote: > This patch also solves the problem, would this better? Yes, this is a lot better. Alexander, does this look good to you? Also, shouldn't this cc stable? > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > index 5a0bf8e..831b1b4 100644 > --- a/drivers/ata/ahci.c > +++ b/drivers/ata/ahci.c > @@ -1236,14 +1236,14 @@ int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis) > struct ahci_port_priv *pp = host->ports[i]->private_data; > > /* pp is NULL for dummy ports */ > - if (pp) > + if (pp) { > desc = pp->irq_desc; > - else > - desc = dev_driver_string(host->dev); > > - rc = devm_request_threaded_irq(host->dev, > - irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED, > - desc, host->ports[i]); > + rc = devm_request_threaded_irq(host->dev, > + irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED, > + desc, host->ports[i]); > + } > + > if (rc) > goto out_free_irqs; > } -- tejun ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-15 18:20 ` Tejun Heo @ 2014-04-16 7:39 ` Alexander Gordeev 2014-04-16 15:19 ` Tejun Heo 0 siblings, 1 reply; 15+ messages in thread From: Alexander Gordeev @ 2014-04-16 7:39 UTC (permalink / raw) To: Tejun Heo; +Cc: David Milburn, linux-ide On Tue, Apr 15, 2014 at 02:20:01PM -0400, Tejun Heo wrote: > On Tue, Apr 15, 2014 at 01:18:12PM -0500, David Milburn wrote: > > This patch also solves the problem, would this better? > > Yes, this is a lot better. Alexander, does this look good to you? Yep, apart from a minor comment below. Also, we're not going to see complains on spurious interrupts, aren't we? > Also, shouldn't this cc stable? > > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > > index 5a0bf8e..831b1b4 100644 > > --- a/drivers/ata/ahci.c > > +++ b/drivers/ata/ahci.c > > @@ -1236,14 +1236,14 @@ int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis) > > struct ahci_port_priv *pp = host->ports[i]->private_data; > > > > /* pp is NULL for dummy ports */ > > - if (pp) > > + if (pp) { > > desc = pp->irq_desc; > > - else > > - desc = dev_driver_string(host->dev); > > > > - rc = devm_request_threaded_irq(host->dev, > > - irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED, > > - desc, host->ports[i]); > > + rc = devm_request_threaded_irq(host->dev, > > + irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED, > > + desc, host->ports[i]); We could dereference 'pp->irq_desc' here and get rid of 'desc' variable. > > + } > > + > > if (rc) > > goto out_free_irqs; > > } > > -- > tejun -- Regards, Alexander Gordeev agordeev@redhat.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-16 7:39 ` Alexander Gordeev @ 2014-04-16 15:19 ` Tejun Heo 2014-04-16 16:28 ` David Milburn 2014-04-16 18:51 ` Alexander Gordeev 0 siblings, 2 replies; 15+ messages in thread From: Tejun Heo @ 2014-04-16 15:19 UTC (permalink / raw) To: Alexander Gordeev; +Cc: David Milburn, linux-ide Hello, On Wed, Apr 16, 2014 at 09:39:19AM +0200, Alexander Gordeev wrote: > On Tue, Apr 15, 2014 at 02:20:01PM -0400, Tejun Heo wrote: > > On Tue, Apr 15, 2014 at 01:18:12PM -0500, David Milburn wrote: > > > This patch also solves the problem, would this better? > > > > Yes, this is a lot better. Alexander, does this look good to you? > > Yep, apart from a minor comment below. > Also, we're not going to see complains on spurious interrupts, aren't we? The irq isn't even requested, it won't reach the kernel at all. > > Also, shouldn't this cc stable? Yeap, I think so. > > > - rc = devm_request_threaded_irq(host->dev, > > > - irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED, > > > - desc, host->ports[i]); > > > + rc = devm_request_threaded_irq(host->dev, > > > + irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED, > > > + desc, host->ports[i]); > > We could dereference 'pp->irq_desc' here and get rid of 'desc' variable. David, can you please update the patch accordingly? Thanks! -- tejun ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-16 15:19 ` Tejun Heo @ 2014-04-16 16:28 ` David Milburn 2014-04-16 18:51 ` Alexander Gordeev 1 sibling, 0 replies; 15+ messages in thread From: David Milburn @ 2014-04-16 16:28 UTC (permalink / raw) To: Tejun Heo; +Cc: Alexander Gordeev, linux-ide On 04/16/2014 10:19 AM, Tejun Heo wrote: > Hello, > > On Wed, Apr 16, 2014 at 09:39:19AM +0200, Alexander Gordeev wrote: >> On Tue, Apr 15, 2014 at 02:20:01PM -0400, Tejun Heo wrote: >>> On Tue, Apr 15, 2014 at 01:18:12PM -0500, David Milburn wrote: >>>> This patch also solves the problem, would this better? >>> >>> Yes, this is a lot better. Alexander, does this look good to you? >> >> Yep, apart from a minor comment below. >> Also, we're not going to see complains on spurious interrupts, aren't we? > > The irq isn't even requested, it won't reach the kernel at all. > >>> Also, shouldn't this cc stable? > > Yeap, I think so. > >>>> - rc = devm_request_threaded_irq(host->dev, >>>> - irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED, >>>> - desc, host->ports[i]); >>>> + rc = devm_request_threaded_irq(host->dev, >>>> + irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED, >>>> + desc, host->ports[i]); >> >> We could dereference 'pp->irq_desc' here and get rid of 'desc' variable. > > David, can you please update the patch accordingly? Hi, Sure, I have successfully re-tested, I will re-submit the patch. Thanks, David ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-16 15:19 ` Tejun Heo 2014-04-16 16:28 ` David Milburn @ 2014-04-16 18:51 ` Alexander Gordeev 2014-04-16 19:14 ` Tejun Heo 1 sibling, 1 reply; 15+ messages in thread From: Alexander Gordeev @ 2014-04-16 18:51 UTC (permalink / raw) To: Tejun Heo; +Cc: David Milburn, linux-ide On Wed, Apr 16, 2014 at 11:19:32AM -0400, Tejun Heo wrote: > > Also, we're not going to see complains on spurious interrupts, aren't we? > > The irq isn't even requested, it won't reach the kernel at all. The crash occured in ahci_hw_interrupt() which means multiple MSIs were enabled. The fact driver does not request IRQ does not mean the PCI device does not send an MSI interrupt (and it does as we're observing the crash). So my question if the dummy port interrupt does not end up in handle_bad_irq() or some? Thanks! -- Regards, Alexander Gordeev agordeev@redhat.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-16 18:51 ` Alexander Gordeev @ 2014-04-16 19:14 ` Tejun Heo 2014-04-17 8:29 ` Alexander Gordeev 0 siblings, 1 reply; 15+ messages in thread From: Tejun Heo @ 2014-04-16 19:14 UTC (permalink / raw) To: Alexander Gordeev; +Cc: David Milburn, linux-ide Hello, On Wed, Apr 16, 2014 at 08:51:05PM +0200, Alexander Gordeev wrote: > The crash occured in ahci_hw_interrupt() which means multiple MSIs > were enabled. The fact driver does not request IRQ does not mean > the PCI device does not send an MSI interrupt (and it does as we're > observing the crash). So my question if the dummy port interrupt > does not end up in handle_bad_irq() or some? My memory is kinda fuzzy now but several stray interrupts don't trigger anything. Thanks. -- tejun ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-16 19:14 ` Tejun Heo @ 2014-04-17 8:29 ` Alexander Gordeev 2014-04-17 13:29 ` Tejun Heo 0 siblings, 1 reply; 15+ messages in thread From: Alexander Gordeev @ 2014-04-17 8:29 UTC (permalink / raw) To: Tejun Heo; +Cc: David Milburn, linux-ide On Wed, Apr 16, 2014 at 03:14:54PM -0400, Tejun Heo wrote: > Hello, > > On Wed, Apr 16, 2014 at 08:51:05PM +0200, Alexander Gordeev wrote: > > The crash occured in ahci_hw_interrupt() which means multiple MSIs > > were enabled. The fact driver does not request IRQ does not mean > > the PCI device does not send an MSI interrupt (and it does as we're > > observing the crash). So my question if the dummy port interrupt > > does not end up in handle_bad_irq() or some? > > My memory is kinda fuzzy now but several stray interrupts don't > trigger anything. Mine too :) Just thought may be this way would be better: diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index e45b18e..9f1169a 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -1240,6 +1240,8 @@ int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis) irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED, pp->irq_desc, host->ports[i]); + else + disable_irq(irq + i); if (rc) goto out_free_irqs; } > Thanks. > > -- > tejun -- Regards, Alexander Gordeev agordeev@redhat.com ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-17 8:29 ` Alexander Gordeev @ 2014-04-17 13:29 ` Tejun Heo 2014-04-17 12:44 ` Alexander Gordeev 0 siblings, 1 reply; 15+ messages in thread From: Tejun Heo @ 2014-04-17 13:29 UTC (permalink / raw) To: Alexander Gordeev; +Cc: David Milburn, linux-ide On Thu, Apr 17, 2014 at 10:29:35AM +0200, Alexander Gordeev wrote: > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > index e45b18e..9f1169a 100644 > --- a/drivers/ata/ahci.c > +++ b/drivers/ata/ahci.c > @@ -1240,6 +1240,8 @@ int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis) > irq + i, ahci_hw_interrupt, > ahci_thread_fn, IRQF_SHARED, > pp->irq_desc, host->ports[i]); > + else > + disable_irq(irq + i); Hmmm? Request failed. Why would it require an explicit disable? -- tejun ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-17 13:29 ` Tejun Heo @ 2014-04-17 12:44 ` Alexander Gordeev 2014-04-17 13:45 ` Tejun Heo 0 siblings, 1 reply; 15+ messages in thread From: Alexander Gordeev @ 2014-04-17 12:44 UTC (permalink / raw) To: Tejun Heo; +Cc: David Milburn, linux-ide On Thu, Apr 17, 2014 at 09:29:42AM -0400, Tejun Heo wrote: > On Thu, Apr 17, 2014 at 10:29:35AM +0200, Alexander Gordeev wrote: > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > > index e45b18e..9f1169a 100644 > > --- a/drivers/ata/ahci.c > > +++ b/drivers/ata/ahci.c > > @@ -1240,6 +1240,8 @@ int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis) > > irq + i, ahci_hw_interrupt, > > ahci_thread_fn, IRQF_SHARED, > > pp->irq_desc, host->ports[i]); > > + else > > + disable_irq(irq + i); > > Hmmm? Request failed. Why would it require an explicit disable? if (pp) rc = devm_request_threaded_irq(..., irq + i, ...) else disable_irq(irq + i); > -- > tejun -- Regards, Alexander Gordeev agordeev@redhat.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-17 12:44 ` Alexander Gordeev @ 2014-04-17 13:45 ` Tejun Heo 2014-04-17 13:07 ` Alexander Gordeev 0 siblings, 1 reply; 15+ messages in thread From: Tejun Heo @ 2014-04-17 13:45 UTC (permalink / raw) To: Alexander Gordeev; +Cc: David Milburn, linux-ide On Thu, Apr 17, 2014 at 02:44:36PM +0200, Alexander Gordeev wrote: > On Thu, Apr 17, 2014 at 09:29:42AM -0400, Tejun Heo wrote: > > On Thu, Apr 17, 2014 at 10:29:35AM +0200, Alexander Gordeev wrote: > > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > > > index e45b18e..9f1169a 100644 > > > --- a/drivers/ata/ahci.c > > > +++ b/drivers/ata/ahci.c > > > @@ -1240,6 +1240,8 @@ int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis) > > > irq + i, ahci_hw_interrupt, > > > ahci_thread_fn, IRQF_SHARED, > > > pp->irq_desc, host->ports[i]); > > > + else > > > + disable_irq(irq + i); > > > > Hmmm? Request failed. Why would it require an explicit disable? > > if (pp) > rc = devm_request_threaded_irq(..., irq + i, ...) > else > disable_irq(irq + i); Ah, was looking at the old code. But again, we didn't even request the irq, why would we disable it? Thanks. -- tejun ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-17 13:45 ` Tejun Heo @ 2014-04-17 13:07 ` Alexander Gordeev 2014-04-17 14:09 ` Tejun Heo 0 siblings, 1 reply; 15+ messages in thread From: Alexander Gordeev @ 2014-04-17 13:07 UTC (permalink / raw) To: Tejun Heo; +Cc: David Milburn, linux-ide On Thu, Apr 17, 2014 at 09:45:43AM -0400, Tejun Heo wrote: > > if (pp) > > rc = devm_request_threaded_irq(..., irq + i, ...) > > else > > disable_irq(irq + i); > > Ah, was looking at the old code. But again, we didn't even request > the irq, why would we disable it? The driver does not request IRQs for dummy ports, but it still requests MSIs for them. The MSI framework initializes the resources for all MSIs (IRQ #s, CPU interrupt vectors). Hence, although we do not request a IRQ #, it still exists - just with no handler associated with the driver. IOW - with a default handler, which would probably complain loudly once an interrupt from a dummy port comes (I suppose David could check it). So if we just disable dummy port's IRQ #s we should never hear from them. > Thanks. > > -- > tejun -- Regards, Alexander Gordeev agordeev@redhat.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL 2014-04-17 13:07 ` Alexander Gordeev @ 2014-04-17 14:09 ` Tejun Heo 0 siblings, 0 replies; 15+ messages in thread From: Tejun Heo @ 2014-04-17 14:09 UTC (permalink / raw) To: Alexander Gordeev; +Cc: David Milburn, linux-ide On Thu, Apr 17, 2014 at 03:07:32PM +0200, Alexander Gordeev wrote: > On Thu, Apr 17, 2014 at 09:45:43AM -0400, Tejun Heo wrote: > > > if (pp) > > > rc = devm_request_threaded_irq(..., irq + i, ...) > > > else > > > disable_irq(irq + i); > > > > Ah, was looking at the old code. But again, we didn't even request > > the irq, why would we disable it? > > The driver does not request IRQs for dummy ports, but it still requests > MSIs for them. The MSI framework initializes the resources for all MSIs > (IRQ #s, CPU interrupt vectors). Hence, although we do not request a > IRQ #, it still exists - just with no handler associated with the driver. > IOW - with a default handler, which would probably complain loudly once > an interrupt from a dummy port comes (I suppose David could check it). > > So if we just disable dummy port's IRQ #s we should never hear from them. Hmmm.... can you please prepare a proper patch with a comment explaining what's going on? I'll apply it once David verifies it. Thanks. -- tejun ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2014-04-17 14:09 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-04-14 21:47 [PATCH] libahci: ahci interrupt check for disabled port since private_data may be NULL David Milburn 2014-04-15 16:33 ` Tejun Heo 2014-04-15 18:18 ` David Milburn 2014-04-15 18:20 ` Tejun Heo 2014-04-16 7:39 ` Alexander Gordeev 2014-04-16 15:19 ` Tejun Heo 2014-04-16 16:28 ` David Milburn 2014-04-16 18:51 ` Alexander Gordeev 2014-04-16 19:14 ` Tejun Heo 2014-04-17 8:29 ` Alexander Gordeev 2014-04-17 13:29 ` Tejun Heo 2014-04-17 12:44 ` Alexander Gordeev 2014-04-17 13:45 ` Tejun Heo 2014-04-17 13:07 ` Alexander Gordeev 2014-04-17 14:09 ` Tejun Heo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.