[PATCH] firewire: Fix ohci free_irq() warning

[PATCH] firewire: Fix ohci free_irq() warning

[Mark Einon]

This patch fixes the kernel warning generated when putting an MSI MS-1727
GT740 laptop into suspend mode. The call sequence in this case calls
free_irq() twice, once in pci_remove() and once then in pci_suspend().

[  262.299486] WARNING: at /build/buildd/linux-3.5.0/kernel/irq/manage.c:1198 __free_irq+0xa3/0x1e0()
[  262.299487] Hardware name: MS-1727
[  262.299488] Trying to free already-free IRQ 16
[  262.299488] Modules linked in: ip6table_filter ip6_tables ebtable_nat ebtables xt_state ipt_REJECT xt_CHECKSUM iptable_mangle xt_tcpudp iptable_filter snd_hda_codec_hdmi ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 ip_tables x_tables bridge stp llc joydev arc4 parport_pc ppdev coretemp kvm_intel snd_hda_codec_realtek kvm microcode snd_seq_midi snd_rawmidi psmouse snd_seq_midi_event serio_raw nvidia(PO) i7core_edac snd_seq edac_core snd_hda_intel iwlwifi snd_hda_codec jmb38x_ms snd_hwdep mac80211 snd_pcm lpc_ich memstick snd_seq_device cfg80211 snd_timer snd soundcore snd_page_alloc ir_lirc_codec lirc_dev ir_mce_kbd_decoder ir_sanyo_decoder ir_sony_decoder ir_jvc_decoder ir_rc6_decoder ir_rc5_decoder ir_nec_decoder rfcomm bnep rc_rc6_mce mxm_wmi ene_ir rc_core bluetooth wmi video acpiphp mac_hid lp parport binfmt_misc hid_generic usbhid hid firewire_ohci sdhci_pci r8169 firewire_core sdhci crc_itu_t
[  262.299537] Pid: 4, comm: kworker/0:0 Tainted: P           O 3.5.0-22-generic #34-Ubuntu
[  262.299538] Call Trace:
[  262.299540]  [<ffffffff81051c1f>] warn_slowpath_common+0x7f/0xc0
[  262.299545]  [<ffffffff81051d16>] warn_slowpath_fmt+0x46/0x50
[  262.299548]  [<ffffffff8103fa39>] ? default_spin_lock_flags+0x9/0x10
[  262.299551]  [<ffffffff810df6b3>] __free_irq+0xa3/0x1e0
[  262.299554]  [<ffffffff810df844>] free_irq+0x54/0xc0
[  262.299558]  [<ffffffffa005a27e>] pci_remove+0x6e/0x210 [firewire_ohci]
[  262.299564]  [<ffffffff8135ae7f>] pci_device_remove+0x3f/0x110
[  262.299569]  [<ffffffff8141fdbc>] __device_release_driver+0x7c/0xe0
[  262.299573]  [<ffffffff8141fe4c>] device_release_driver+0x2c/0x40
[  262.299576]  [<ffffffff8141f5f1>] bus_remove_device+0xe1/0x120
[  262.299578]  [<ffffffff8141cd1a>] device_del+0x12a/0x1c0
[  262.299581]  [<ffffffff8141cdc6>] device_unregister+0x16/0x30
[  262.299583]  [<ffffffff81354784>] pci_stop_bus_device+0x94/0xa0
[  262.299588]  [<ffffffffa0091c67>] acpiphp_disable_slot+0xb7/0x1a0 [acpiphp]
[  262.299594]  [<ffffffffa0090716>] ? get_slot_status+0x46/0xc0 [acpiphp]
[  262.299599]  [<ffffffffa0091d7d>] acpiphp_check_bridge.isra.15+0x2d/0xf0 [acpiphp]
[  262.299604]  [<ffffffffa0092442>] _handle_hotplug_event_bridge+0x372/0x4d0 [acpiphp]
[  262.299608]  [<ffffffff81390f8c>] ? acpi_os_execute_deferred+0x2f/0x34
[  262.299612]  [<ffffffff8116e22d>] ? kfree+0xed/0x110
[  262.299617]  [<ffffffff8107086a>] process_one_work+0x12a/0x420
[  262.299620]  [<ffffffffa00920d0>] ? _handle_hotplug_event_func+0x1d0/0x1d0 [acpiphp]
[  262.299625]  [<ffffffff8107141e>] worker_thread+0x12e/0x2f0
[  262.299627]  [<ffffffff810712f0>] ? manage_workers.isra.26+0x200/0x200
[  262.299629]  [<ffffffff81075f13>] kthread+0x93/0xa0
[  262.299632]  [<ffffffff8168d024>] kernel_thread_helper+0x4/0x10
[  262.299636]  [<ffffffff81075e80>] ? kthread_freezable_should_stop+0x70/0x70
[  262.299639]  [<ffffffff8168d020>] ? gs_change+0x13/0x13
[  262.299641] ---[ end trace 3f830890e076911f ]---

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/firewire/ohci.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 45912e6..029c407 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -3766,7 +3766,7 @@ static int pci_suspend(struct pci_dev *dev, pm_message_t state)
 	int err;
 
 	software_reset(ohci);
-	free_irq(dev->irq, ohci);
+	disable_irq(dev->irq);
 	pci_disable_msi(dev);
 	err = pci_save_state(dev);
 	if (err) {
@@ -3807,6 +3807,7 @@ static int pci_resume(struct pci_dev *dev)
 		return err;
 
 	ohci_resume_iso_dma(ohci);
+	enable_irq(dev->irq);
 
 	return 0;
 }
-- 
1.7.10.4

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Stefan Richter]

On Jan 28 Mark Einon wrote:
> This patch fixes the kernel warning generated when putting an MSI MS-1727
> GT740 laptop into suspend mode. The call sequence in this case calls
> free_irq() twice, once in pci_remove() and once then in pci_suspend().

You mean /first/ in pci_suspend() and /then/ in pci_remove() on the
already suspended devices, right?

Because the other way around, first pci_remove(), then pci_suspend(),
surely must not appen.  And if it does, the bug is elsewhere but not in
firewire-ohci.

On that note, is pci_suspend() -> pci_remove() actually a legal state
transition that must be handled by drivers?

Another comment below.

> [  262.299486] WARNING: at /build/buildd/linux-3.5.0/kernel/irq/manage.c:1198 __free_irq+0xa3/0x1e0()
> [  262.299487] Hardware name: MS-1727
> [  262.299488] Trying to free already-free IRQ 16
> [  262.299488] Modules linked in: ip6table_filter ip6_tables ebtable_nat ebtables xt_state ipt_REJECT xt_CHECKSUM iptable_mangle xt_tcpudp iptable_filter snd_hda_codec_hdmi ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 ip_tables x_tables bridge stp llc joydev arc4 parport_pc ppdev coretemp kvm_intel snd_hda_codec_realtek kvm microcode snd_seq_midi snd_rawmidi psmouse snd_seq_midi_event serio_raw nvidia(PO) i7core_edac snd_seq edac_core snd_hda_intel iwlwifi snd_hda_codec jmb38x_ms snd_hwdep mac80211 snd_pcm lpc_ich memstick snd_seq_device cfg80211 snd_timer snd soundcore snd_page_alloc ir_lirc_codec lirc_dev ir_mce_kbd_decoder ir_sanyo_decoder ir_sony_decoder ir_jvc_decoder ir_rc6_decoder ir_rc5_decoder ir_nec_decoder rfcomm bnep rc_rc6_mce mxm_wmi ene_ir rc_core bluetooth wmi video acpiphp mac_hid lp parport binfmt_misc hid_generic usbhid hid firewire_ohci sdhci_pci r8169 firewire_core sdhci crc_itu_t
> [  262.299537] Pid: 4, comm: kworker/0:0 Tainted: P           O 3.5.0-22-generic #34-Ubuntu
> [  262.299538] Call Trace:
> [  262.299540]  [<ffffffff81051c1f>] warn_slowpath_common+0x7f/0xc0
> [  262.299545]  [<ffffffff81051d16>] warn_slowpath_fmt+0x46/0x50
> [  262.299548]  [<ffffffff8103fa39>] ? default_spin_lock_flags+0x9/0x10
> [  262.299551]  [<ffffffff810df6b3>] __free_irq+0xa3/0x1e0
> [  262.299554]  [<ffffffff810df844>] free_irq+0x54/0xc0
> [  262.299558]  [<ffffffffa005a27e>] pci_remove+0x6e/0x210 [firewire_ohci]
> [  262.299564]  [<ffffffff8135ae7f>] pci_device_remove+0x3f/0x110
> [  262.299569]  [<ffffffff8141fdbc>] __device_release_driver+0x7c/0xe0
> [  262.299573]  [<ffffffff8141fe4c>] device_release_driver+0x2c/0x40
> [  262.299576]  [<ffffffff8141f5f1>] bus_remove_device+0xe1/0x120
> [  262.299578]  [<ffffffff8141cd1a>] device_del+0x12a/0x1c0
> [  262.299581]  [<ffffffff8141cdc6>] device_unregister+0x16/0x30
> [  262.299583]  [<ffffffff81354784>] pci_stop_bus_device+0x94/0xa0
> [  262.299588]  [<ffffffffa0091c67>] acpiphp_disable_slot+0xb7/0x1a0 [acpiphp]
> [  262.299594]  [<ffffffffa0090716>] ? get_slot_status+0x46/0xc0 [acpiphp]
> [  262.299599]  [<ffffffffa0091d7d>] acpiphp_check_bridge.isra.15+0x2d/0xf0 [acpiphp]
> [  262.299604]  [<ffffffffa0092442>] _handle_hotplug_event_bridge+0x372/0x4d0 [acpiphp]
> [  262.299608]  [<ffffffff81390f8c>] ? acpi_os_execute_deferred+0x2f/0x34
> [  262.299612]  [<ffffffff8116e22d>] ? kfree+0xed/0x110
> [  262.299617]  [<ffffffff8107086a>] process_one_work+0x12a/0x420
> [  262.299620]  [<ffffffffa00920d0>] ? _handle_hotplug_event_func+0x1d0/0x1d0 [acpiphp]
> [  262.299625]  [<ffffffff8107141e>] worker_thread+0x12e/0x2f0
> [  262.299627]  [<ffffffff810712f0>] ? manage_workers.isra.26+0x200/0x200
> [  262.299629]  [<ffffffff81075f13>] kthread+0x93/0xa0
> [  262.299632]  [<ffffffff8168d024>] kernel_thread_helper+0x4/0x10
> [  262.299636]  [<ffffffff81075e80>] ? kthread_freezable_should_stop+0x70/0x70
> [  262.299639]  [<ffffffff8168d020>] ? gs_change+0x13/0x13
> [  262.299641] ---[ end trace 3f830890e076911f ]---
> 
> Signed-off-by: Mark Einon <mark.einon@gmail.com>
> ---
>  drivers/firewire/ohci.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
> index 45912e6..029c407 100644
> --- a/drivers/firewire/ohci.c
> +++ b/drivers/firewire/ohci.c
> @@ -3766,7 +3766,7 @@ static int pci_suspend(struct pci_dev *dev, pm_message_t state)
>  	int err;
>  
>  	software_reset(ohci);
> -	free_irq(dev->irq, ohci);
> +	disable_irq(dev->irq);
>  	pci_disable_msi(dev);
>  	err = pci_save_state(dev);
>  	if (err) {
> @@ -3807,6 +3807,7 @@ static int pci_resume(struct pci_dev *dev)
>  		return err;
>  
>  	ohci_resume_iso_dma(ohci);
> +	enable_irq(dev->irq);
>  
>  	return 0;
>  }

firewire-ohci's pci_resume() calls request_irq() a little bit before that,
in ohci_enable().  Is the sequence pci_probe/request_irq() ->
pci_suspend/disable_irq() -> pci_resume/request_irq() ->
pci_resume/enable_irq() legal?
-- 
Stefan Richter
-=====-===-= ---= ===--
http://arcgraph.de/sr/

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Greg KH]

On Mon, Jan 28, 2013 at 10:09:48PM +0000, Mark Einon wrote:
> This patch fixes the kernel warning generated when putting an MSI MS-1727
> GT740 laptop into suspend mode. The call sequence in this case calls
> free_irq() twice, once in pci_remove() and once then in pci_suspend().
> 
> [  262.299486] WARNING: at /build/buildd/linux-3.5.0/kernel/irq/manage.c:1198 __free_irq+0xa3/0x1e0()
> [  262.299487] Hardware name: MS-1727
> [  262.299488] Trying to free already-free IRQ 16
> [  262.299488] Modules linked in: ip6table_filter ip6_tables ebtable_nat ebtables xt_state ipt_REJECT xt_CHECKSUM iptable_mangle xt_tcpudp iptable_filter snd_hda_codec_hdmi ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 ip_tables x_tables bridge stp llc joydev arc4 parport_pc ppdev coretemp kvm_intel snd_hda_codec_realtek kvm microcode snd_seq_midi snd_rawmidi psmouse snd_seq_midi_event serio_raw nvidia(PO) i7core_edac snd_seq edac_core snd_hda_intel iwlwifi snd_hda_codec jmb38x_ms snd_hwdep mac80211 snd_pcm lpc_ich memstick snd_seq_device cfg80211 snd_timer snd soundcore snd_page_alloc ir_lirc_codec lirc_dev ir_mce_kbd_decoder ir_sanyo_decoder ir_sony_decoder ir_jvc_decoder ir_rc6_decoder ir_rc5_decoder ir_nec_decoder rfcomm bnep rc_rc6_mce mxm_wmi ene_ir rc_core bluetooth wmi video acpiphp mac_hid lp parport binfmt_misc hid_generic usbhid hid firewire_ohci sdhci_pci r8169 firewire_core sdhci crc_itu_t
> [  262.299537] Pid: 4, comm: kworker/0:0 Tainted: P           O 3.5.0-22-generic #34-Ubuntu
> [  262.299538] Call Trace:
> [  262.299540]  [<ffffffff81051c1f>] warn_slowpath_common+0x7f/0xc0
> [  262.299545]  [<ffffffff81051d16>] warn_slowpath_fmt+0x46/0x50
> [  262.299548]  [<ffffffff8103fa39>] ? default_spin_lock_flags+0x9/0x10
> [  262.299551]  [<ffffffff810df6b3>] __free_irq+0xa3/0x1e0
> [  262.299554]  [<ffffffff810df844>] free_irq+0x54/0xc0
> [  262.299558]  [<ffffffffa005a27e>] pci_remove+0x6e/0x210 [firewire_ohci]
> [  262.299564]  [<ffffffff8135ae7f>] pci_device_remove+0x3f/0x110
> [  262.299569]  [<ffffffff8141fdbc>] __device_release_driver+0x7c/0xe0
> [  262.299573]  [<ffffffff8141fe4c>] device_release_driver+0x2c/0x40
> [  262.299576]  [<ffffffff8141f5f1>] bus_remove_device+0xe1/0x120
> [  262.299578]  [<ffffffff8141cd1a>] device_del+0x12a/0x1c0
> [  262.299581]  [<ffffffff8141cdc6>] device_unregister+0x16/0x30
> [  262.299583]  [<ffffffff81354784>] pci_stop_bus_device+0x94/0xa0
> [  262.299588]  [<ffffffffa0091c67>] acpiphp_disable_slot+0xb7/0x1a0 [acpiphp]
> [  262.299594]  [<ffffffffa0090716>] ? get_slot_status+0x46/0xc0 [acpiphp]
> [  262.299599]  [<ffffffffa0091d7d>] acpiphp_check_bridge.isra.15+0x2d/0xf0 [acpiphp]
> [  262.299604]  [<ffffffffa0092442>] _handle_hotplug_event_bridge+0x372/0x4d0 [acpiphp]
> [  262.299608]  [<ffffffff81390f8c>] ? acpi_os_execute_deferred+0x2f/0x34
> [  262.299612]  [<ffffffff8116e22d>] ? kfree+0xed/0x110
> [  262.299617]  [<ffffffff8107086a>] process_one_work+0x12a/0x420
> [  262.299620]  [<ffffffffa00920d0>] ? _handle_hotplug_event_func+0x1d0/0x1d0 [acpiphp]
> [  262.299625]  [<ffffffff8107141e>] worker_thread+0x12e/0x2f0
> [  262.299627]  [<ffffffff810712f0>] ? manage_workers.isra.26+0x200/0x200
> [  262.299629]  [<ffffffff81075f13>] kthread+0x93/0xa0
> [  262.299632]  [<ffffffff8168d024>] kernel_thread_helper+0x4/0x10
> [  262.299636]  [<ffffffff81075e80>] ? kthread_freezable_should_stop+0x70/0x70
> [  262.299639]  [<ffffffff8168d020>] ? gs_change+0x13/0x13
> [  262.299641] ---[ end trace 3f830890e076911f ]---
> 
> Signed-off-by: Mark Einon <mark.einon@gmail.com>
> ---
>  drivers/firewire/ohci.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Mark Einon]

On 28 January 2013 23:01, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> On Jan 28 Mark Einon wrote:
>> This patch fixes the kernel warning generated when putting an MSI MS-1727
>> GT740 laptop into suspend mode. The call sequence in this case calls
>> free_irq() twice, once in pci_remove() and once then in pci_suspend().
>
> You mean /first/ in pci_suspend() and /then/ in pci_remove() on the
> already suspended devices, right?

Yes, I did. The call sequence is suspend then resume. My bad.

>
> Because the other way around, first pci_remove(), then pci_suspend(),
> surely must not appen.  And if it does, the bug is elsewhere but not in
> firewire-ohci.
>
> On that note, is pci_suspend() -> pci_remove() actually a legal state
> transition that must be handled by drivers?

It's happening on the Ubuntu 12.10 distro which performs the suspend
then remove sequence. I assumed that was normal.

>
> Another comment below.
<snip>
>
> firewire-ohci's pci_resume() calls request_irq() a little bit before that,
> in ohci_enable().  Is the sequence pci_probe/request_irq() ->
> pci_suspend/disable_irq() -> pci_resume/request_irq() ->
> pci_resume/enable_irq() legal?

I missed the call to request_irq(). Perhaps the simplest way to handle
the fix would be to use a flag that holds the irq state? I'm open to
suggestions.

Cheers,

Mark

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Stefan Richter]

Added Cc: linux-pm.

On Jan 29 Mark Einon wrote:
> On 28 January 2013 23:01, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> > On Jan 28 Mark Einon wrote:
> >> This patch fixes the kernel warning generated when putting an MSI MS-1727
> >> GT740 laptop into suspend mode. The call sequence in this case calls
> >> free_irq() twice, once in pci_remove() and once then in pci_suspend().
> >
> > You mean /first/ in pci_suspend() and /then/ in pci_remove() on the
> > already suspended devices, right?
> 
> Yes, I did. The call sequence is suspend then resume. My bad.
> 
> >
> > Because the other way around, first pci_remove(), then pci_suspend(),
> > surely must not appen.  And if it does, the bug is elsewhere but not in
> > firewire-ohci.
> >
> > On that note, is pci_suspend() -> pci_remove() actually a legal state
> > transition that must be handled by drivers?
> 
> It's happening on the Ubuntu 12.10 distro which performs the suspend
> then remove sequence. I assumed that was normal.

Maybe a parent device (PCI bridge or the likes) of the OHCI is
requested to be removed during suspend or is being removed during
resume, thereby causing a removal request to the OHCI?

Or the suspend method of firewire-ohci exited with an error return code...
but then the suspend sequence would be rolled back, not the offending
device be removed, right?

In any case, could you check the dmesg right before the warning which you
already posted --- and right after it too --- whether there is an
indication what else was going on?

Also, what are the parent devices of the OHCI according to "lspci -tv" (or
what else can show PCI topology)?

> > Another comment below.
> <snip>
> >
> > firewire-ohci's pci_resume() calls request_irq() a little bit before that,
> > in ohci_enable().  Is the sequence pci_probe/request_irq() ->
> > pci_suspend/disable_irq() -> pci_resume/request_irq() ->
> > pci_resume/enable_irq() legal?
> 
> I missed the call to request_irq(). Perhaps the simplest way to handle
> the fix would be to use a flag that holds the irq state? I'm open to
> suggestions.

Not sure what extent of state tracking the PCI core or driver core already
offer; I'll have to look.
-- 
Stefan Richter
-=====-===-= ---= ===-=
http://arcgraph.de/sr/

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Alan Stern]

On Tue, 29 Jan 2013, Stefan Richter wrote:

> Added Cc: linux-pm.
> 
> On Jan 29 Mark Einon wrote:
> > On 28 January 2013 23:01, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> > > On Jan 28 Mark Einon wrote:
> > >> This patch fixes the kernel warning generated when putting an MSI MS-1727
> > >> GT740 laptop into suspend mode. The call sequence in this case calls
> > >> free_irq() twice, once in pci_remove() and once then in pci_suspend().
> > >
> > > You mean /first/ in pci_suspend() and /then/ in pci_remove() on the
> > > already suspended devices, right?
> > 
> > Yes, I did. The call sequence is suspend then resume. My bad.

Why does the pci_suspend routine call free_irq() at all?  As far as I 
know, it's not supposed to do that.  Won't the device continue to use 
the same IRQ after it is resumed?

Alan Stern

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Alan Stern]

On Tue, 29 Jan 2013, Stefan Richter wrote:

> Added Cc: linux-pm.
> 
> On Jan 29 Mark Einon wrote:
> > On 28 January 2013 23:01, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> > > On Jan 28 Mark Einon wrote:
> > >> This patch fixes the kernel warning generated when putting an MSI MS-1727
> > >> GT740 laptop into suspend mode. The call sequence in this case calls
> > >> free_irq() twice, once in pci_remove() and once then in pci_suspend().
> > >
> > > You mean /first/ in pci_suspend() and /then/ in pci_remove() on the
> > > already suspended devices, right?
> > 
> > Yes, I did. The call sequence is suspend then resume. My bad.

Why does the pci_suspend routine call free_irq() at all?  As far as I 
know, it's not supposed to do that.  Won't the device continue to use 
the same IRQ after it is resumed?

Alan Stern

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Mark Einon]

On 29 January 2013 16:04, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> Added Cc: linux-pm.
>
> On Jan 29 Mark Einon wrote:
>> On 28 January 2013 23:01, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>> > On Jan 28 Mark Einon wrote:
>> >> This patch fixes the kernel warning generated when putting an MSI MS-1727
>> >> GT740 laptop into suspend mode. The call sequence in this case calls
>> >> free_irq() twice, once in pci_remove() and once then in pci_suspend().
>> >
>> > You mean /first/ in pci_suspend() and /then/ in pci_remove() on the
>> > already suspended devices, right?
>>
>> Yes, I did. The call sequence is suspend then resume. My bad.

Correction above : s/resume/remove

>>
>> >
>> > Because the other way around, first pci_remove(), then pci_suspend(),
>> > surely must not appen.  And if it does, the bug is elsewhere but not in
>> > firewire-ohci.
>> >
>> > On that note, is pci_suspend() -> pci_remove() actually a legal state
>> > transition that must be handled by drivers?
>>
>> It's happening on the Ubuntu 12.10 distro which performs the suspend
>> then remove sequence. I assumed that was normal.
>
> Maybe a parent device (PCI bridge or the likes) of the OHCI is
> requested to be removed during suspend or is being removed during
> resume, thereby causing a removal request to the OHCI?
>
> Or the suspend method of firewire-ohci exited with an error return code...
> but then the suspend sequence would be rolled back, not the offending
> device be removed, right?
>
> In any case, could you check the dmesg right before the warning which you
> already posted --- and right after it too --- whether there is an
> indication what else was going on?

It looks like the remove call actually happens on the wakeup sequence
- although I've not yet found the time to investigate further. The
lines preceding the crash are:

Jan 30 23:22:53 msilap kernel: [ 7682.288190] ACPI: Low-level resume complete
Jan 30 23:22:53 msilap kernel: [ 7682.288228] PM: Restoring platform NVS memory
Jan 30 23:22:53 msilap kernel: [ 7682.288644] Enabling non-boot CPUs ...
Jan 30 23:22:53 msilap kernel: [ 7682.288713] smpboot: Booting Node 0
Processor 1 APIC 0x2
Jan 30 23:22:53 msilap kernel: [ 7682.301963] hpet: hpet3 irq 41 for MSI
Jan 30 23:22:53 msilap kernel: [ 7682.301978] CPU1 is up
Jan 30 23:22:53 msilap kernel: [ 7682.302040] smpboot: Booting Node 0
Processor 2 APIC 0x4
Jan 30 23:22:53 msilap kernel: [ 7682.315394] hpet: hpet4 irq 42 for MSI
Jan 30 23:22:53 msilap kernel: [ 7682.315408] CPU2 is up
Jan 30 23:22:53 msilap kernel: [ 7682.315470] smpboot: Booting Node 0
Processor 3 APIC 0x6
Jan 30 23:22:53 msilap kernel: [ 7682.328815] hpet: hpet5 irq 43 for MSI
Jan 30 23:22:53 msilap kernel: [ 7682.328828] CPU3 is up
Jan 30 23:22:53 msilap kernel: [ 7682.328882] smpboot: Booting Node 0
Processor 4 APIC 0x1
Jan 30 23:22:53 msilap kernel: [ 7682.342256] hpet: hpet6 irq 44 for MSI
Jan 30 23:22:53 msilap kernel: [ 7682.342269] CPU4 is up
Jan 30 23:22:53 msilap kernel: [ 7682.342327] smpboot: Booting Node 0
Processor 5 APIC 0x3
Jan 30 23:22:53 msilap kernel: [ 7682.355737] CPU5 is up
Jan 30 23:22:53 msilap kernel: [ 7682.355797] smpboot: Booting Node 0
Processor 6 APIC 0x5
Jan 30 23:22:53 msilap kernel: [ 7682.369213] CPU6 is up
Jan 30 23:22:53 msilap kernel: [ 7682.369272] smpboot: Booting Node 0
Processor 7 APIC 0x7
Jan 30 23:22:53 msilap kernel: [ 7682.382720] CPU7 is up
Jan 30 23:22:53 msilap kernel: [ 7682.388801] ACPI: Waking up from
system sleep state S3
Jan 30 23:22:53 msilap kernel: [ 7682.403997] ------------[ cut here
]------------
Jan 30 23:22:53 msilap kernel: [ 7682.404007] WARNING: at
/home/mark/Source/linux/kernel/irq/manage.c:1248
__free_irq+0xa3/0x1e0()
<snip>

then after the first crash trace:

Jan 30 23:22:53 msilap kernel: [ 7682.404181] ---[ end trace
1a4f0a37b1022aad ]---
Jan 30 23:22:53 msilap kernel: [ 7682.404211] firewire_ohci
0000:07:00.0: removed fw-ohci device
Jan 30 23:22:53 msilap kernel: [ 7682.404327] ------------[ cut here
]------------
Jan 30 23:22:53 msilap kernel: [ 7682.404330] WARNING: at
/home/mark/Source/linux/kernel/irq/manage.c:1248
__free_irq+0xa3/0x1e0()
Jan 30 23:22:53 msilap kernel: [ 7682.404330] Hardware name: MS-1727
Jan 30 23:22:53 msilap kernel: [ 7682.404331] Trying to free already-free IRQ 16
Jan 30 23:22:53 msilap kernel: [ 7682.404363] Modules linked in:
ip6table_filter ip6_tables ebtable_nat ebtables nf_conntrack_ipv4
nf_defrag_ipv4 xt_state nf_conntrack ipt_REJECT xt_CHECKSUM
iptable_mangle xt_tcpudp iptable_filter bnep rfcomm ip_tables
bluetooth x_tables bridge stp llc parport_pc ppdev binfmt_misc arc4
nouveau iwldvm mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek
snd_hda_intel snd_hda_codec iwlwifi coretemp kvm_intel snd_hwdep kvm
snd_pcm ttm cfg80211 snd_seq_midi snd_rawmidi drm_kms_helper
snd_seq_midi_event drm snd_seq snd_timer snd_seq_device joydev snd
microcode i7core_edac i2c_algo_bit psmouse soundcore edac_core
jmb38x_ms msi_wmi serio_raw video memstick mxm_wmi lpc_ich
sparse_keymap mac_hid wmi snd_page_alloc acpiphp lp parport r8169
firewire_ohci(O) sdhci_pci firewire_core(O) sdhci crc_itu_t
Jan 30 23:22:53 msilap kernel: [ 7682.404365] Pid: 5968, comm:
kworker/0:2 Tainted: G        W  O 3.8.0-rc5-next-20130128+ #6
Jan 30 23:22:53 msilap kernel: [ 7682.404366] Call Trace:
Jan 30 23:22:53 msilap kernel: [ 7682.404369]  [<ffffffff810589af>]
warn_slowpath_common+0x7f/0xc0
Jan 30 23:22:53 msilap kernel: [ 7682.404371]  [<ffffffff81058aa6>]
warn_slowpath_fmt+0x46/0x50
Jan 30 23:22:53 msilap kernel: [ 7682.404374]  [<ffffffff81044c69>] ?
default_spin_lock_flags+0x9/0x10
Jan 30 23:22:53 msilap kernel: [ 7682.404376]  [<ffffffff810eabc3>]
__free_irq+0xa3/0x1e0
Jan 30 23:22:53 msilap kernel: [ 7682.404378]  [<ffffffff810ead54>]
free_irq+0x54/0xc0
Jan 30 23:22:53 msilap kernel: [ 7682.404384]  [<ffffffffa000968a>]
sdhci_remove_host+0x6a/0x190 [sdhci]
Jan 30 23:22:53 msilap kernel: [ 7682.404389]  [<ffffffffa002f3ff>]
sdhci_pci_remove_slot+0x4f/0xc0 [sdhci_pci]
Jan 30 23:22:53 msilap kernel: [ 7682.404394]  [<ffffffffa002fa70>]
sdhci_pci_remove+0x50/0xa0 [sdhci_pci]
Jan 30 23:22:53 msilap kernel: [ 7682.404398]  [<ffffffff8137ff26>]
pci_device_remove+0x46/0xc0
Jan 30 23:22:53 msilap kernel: [ 7682.404402]  [<ffffffff81456f3c>]
__device_release_driver+0x7c/0xe0
Jan 30 23:22:53 msilap kernel: [ 7682.404404]  [<ffffffff814571ac>]
device_release_driver+0x2c/0x40
Jan 30 23:22:53 msilap kernel: [ 7682.404406]  [<ffffffff814569b1>]
bus_remove_device+0xe1/0x120
Jan 30 23:22:53 msilap kernel: [ 7682.404409]  [<ffffffff8145403a>]
device_del+0x11a/0x1b0
Jan 30 23:22:53 msilap kernel: [ 7682.404411]  [<ffffffff81379d94>]
pci_stop_bus_device+0x94/0xa0
Jan 30 23:22:53 msilap kernel: [ 7682.404414]  [<ffffffff81379f36>]
pci_stop_and_remove_bus_device+0x16/0x30
Jan 30 23:22:53 msilap kernel: [ 7682.404419]  [<ffffffffa0028b36>]
acpiphp_disable_slot+0xf6/0x1a0 [acpiphp]
Jan 30 23:22:53 msilap kernel: [ 7682.404424]  [<ffffffffa0027716>] ?
get_slot_status+0x46/0xc0 [acpiphp]
Jan 30 23:22:53 msilap kernel: [ 7682.404428]  [<ffffffffa0028c0d>]
acpiphp_check_bridge.isra.13+0x2d/0xf0 [acpiphp]
Jan 30 23:22:53 msilap kernel: [ 7682.404433]  [<ffffffffa0029247>]
_handle_hotplug_event_bridge+0x2e7/0x410 [acpiphp]
Jan 30 23:22:53 msilap kernel: [ 7682.404436]  [<ffffffff813bdc64>] ?
acpi_os_execute_deferred+0x2d/0x32
Jan 30 23:22:53 msilap kernel: [ 7682.404439]  [<ffffffff8117df25>] ?
kfree+0x105/0x140
Jan 30 23:22:53 msilap kernel: [ 7682.404441]  [<ffffffff81077249>]
process_one_work+0x159/0x4c0
Jan 30 23:22:53 msilap kernel: [ 7682.404444]  [<ffffffff8107895e>]
worker_thread+0x15e/0x4a0
Jan 30 23:22:53 msilap kernel: [ 7682.404446]  [<ffffffff81078800>] ?
manage_workers+0x2a0/0x2a0
Jan 30 23:22:53 msilap kernel: [ 7682.404448]  [<ffffffff8107df60>]
kthread+0xc0/0xd0
Jan 30 23:22:53 msilap kernel: [ 7682.404450]  [<ffffffff8107dea0>] ?
kthread_create_on_node+0x130/0x130
Jan 30 23:22:53 msilap kernel: [ 7682.404452]  [<ffffffff816d756c>]
ret_from_fork+0x7c/0xb0
Jan 30 23:22:53 msilap kernel: [ 7682.404454]  [<ffffffff8107dea0>] ?
kthread_create_on_node+0x130/0x130
Jan 30 23:22:53 msilap kernel: [ 7682.404456] ---[ end trace
1a4f0a37b1022aae ]---


>
> Also, what are the parent devices of the OHCI according to "lspci -tv" (or
> what else can show PCI topology)?

I ran 'lshw' on the machine, which may be of more use. The relevant
output is (removing parts not associated with the firewire device):

msilap
    description: Notebook
    product: MS-1727 (To be filled by O.E.M.)
    vendor: MICRO-STAR INTERNATIONAL CO., LTD
    version: REV:1.0
    serial: FFFFFFFF
    width: 64 bits
    capabilities: smbios-2.6 dmi-2.6 vsyscall32
    configuration: boot=normal chassis=notebook family=To be filled by
O.E.M. sku=To be filled by O.E.M.
uuid=00020003-0004-0005-0006-000700080009
  *-core
       description: Motherboard
       product: MS-1727
       vendor: MICRO-STAR INTERNATIONAL CO., LTD
       physical id: 0
       version: REV:1.0
       serial: BSS-0123456789
       slot: To be filled by O.E.M.
     *-pci:0
          description: Host bridge
          product: Core Processor DMI
          vendor: Intel Corporation
          physical id: 100
          bus info: pci@0000:00:00.0
          version: 11
          width: 32 bits
          clock: 33MHz
        *-pci:4
             description: PCI bridge
             product: 5 Series/3400 Series Chipset PCI Express Root Port 5
             vendor: Intel Corporation
             physical id: 1c.4
             bus info: pci@0000:00:1c.4
             version: 05
             width: 32 bits
             clock: 33MHz
             capabilities: pci pciexpress msi pm normal_decode
bus_master cap_list
             configuration: driver=pcieport
             resources: irq:16 ioport:9000(size=4096)
memory:d4600000-d59fffff ioport:d9d00000(size=2097152)
           *-firewire
                description: FireWire (IEEE 1394)
                product: IEEE 1394 Host Controller
                vendor: JMicron Technology Corp.
                physical id: 0
                bus info: pci@0000:07:00.0
                version: 00
                width: 32 bits
                clock: 33MHz
                capabilities: pm pciexpress msi ohci bus_master cap_list
                configuration: driver=firewire_ohci latency=0
                resources: irq:16 memory:d4607000-d46077ff
memory:d4606000-d460607f memory:d4605000-d460507f
memory:d4604000-d460407f
           *-generic:0
                description: System peripheral
                product: SD/MMC Host Controller
                vendor: JMicron Technology Corp.
                physical id: 0.1
                bus info: pci@0000:07:00.1
                version: 00
                width: 32 bits
                clock: 33MHz
                capabilities: pm pciexpress msi bus_master cap_list
                configuration: driver=sdhci-pci latency=0
                resources: irq:16 memory:d4603000-d46030ff
           *-generic:1 UNCLAIMED
                description: SD Host controller
                product: Standard SD Host Controller
                vendor: JMicron Technology Corp.
                physical id: 0.2
                bus info: pci@0000:07:00.2
                version: 00
                width: 32 bits
                clock: 33MHz
                capabilities: pm pciexpress msi cap_list
                configuration: latency=0
                resources: memory:d4602000-d46020ff
           *-generic:2
                description: System peripheral
                product: MS Host Controller
                vendor: JMicron Technology Corp.
                physical id: 0.3
                bus info: pci@0000:07:00.3
                version: 00
                width: 32 bits
                clock: 33MHz
                capabilities: pm pciexpress msi bus_master cap_list
                configuration: driver=jmb38x_ms latency=0
                resources: irq:16 memory:d4601000-d46010ff
           *-generic:3 UNCLAIMED
                description: System peripheral
                product: xD Host Controller
                vendor: JMicron Technology Corp.
                physical id: 0.4
                bus info: pci@0000:07:00.4
                version: 00
                width: 32 bits
                clock: 33MHz
                capabilities: pm pciexpress msi bus_master cap_list
                configuration: latency=0
                resources: memory:d4600000-d46000ff


>
>> > Another comment below.
>> <snip>
>> >
>> > firewire-ohci's pci_resume() calls request_irq() a little bit before that,
>> > in ohci_enable().  Is the sequence pci_probe/request_irq() ->
>> > pci_suspend/disable_irq() -> pci_resume/request_irq() ->
>> > pci_resume/enable_irq() legal?
>>
>> I missed the call to request_irq(). Perhaps the simplest way to handle
>> the fix would be to use a flag that holds the irq state? I'm open to
>> suggestions.
>
> Not sure what extent of state tracking the PCI core or driver core already
> offer; I'll have to look.
> --
> Stefan Richter
> -=====-===-= ---= ===-=
> http://arcgraph.de/sr/

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Mark Einon]

On 29 January 2013 17:01, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 29 Jan 2013, Stefan Richter wrote:
>
>> Added Cc: linux-pm.
>>
>> On Jan 29 Mark Einon wrote:
>> > On 28 January 2013 23:01, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>> > > On Jan 28 Mark Einon wrote:
>> > >> This patch fixes the kernel warning generated when putting an MSI MS-1727
>> > >> GT740 laptop into suspend mode. The call sequence in this case calls
>> > >> free_irq() twice, once in pci_remove() and once then in pci_suspend().
>> > >
>> > > You mean /first/ in pci_suspend() and /then/ in pci_remove() on the
>> > > already suspended devices, right?
>> >
>> > Yes, I did. The call sequence is suspend then resume. My bad.
>
> Why does the pci_suspend routine call free_irq() at all?  As far as I
> know, it's not supposed to do that.  Won't the device continue to use
> the same IRQ after it is resumed?

This sounds reasonable to me - I think we could probably get rid of
the request_irq() call from resume, and use
disable_irq()/enable_irq()?
I'll attempt a patch - but unfortunately I don't have a firewire device to test.

Cheers,

Mark

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Alan Stern]

On Wed, 30 Jan 2013, Mark Einon wrote:

> >> > >> This patch fixes the kernel warning generated when putting an MSI MS-1727
> >> > >> GT740 laptop into suspend mode. The call sequence in this case calls
> >> > >> free_irq() twice, once in pci_remove() and once then in pci_suspend().
> >> > >
> >> > > You mean /first/ in pci_suspend() and /then/ in pci_remove() on the
> >> > > already suspended devices, right?
> >> >
> >> > Yes, I did. The call sequence is suspend then resume. My bad.
> >
> > Why does the pci_suspend routine call free_irq() at all?  As far as I
> > know, it's not supposed to do that.  Won't the device continue to use
> > the same IRQ after it is resumed?
> 
> This sounds reasonable to me - I think we could probably get rid of
> the request_irq() call from resume, and use
> disable_irq()/enable_irq()?

Why mess around with IRQ settings at all?  Just have the suspend
routine tell the controller to stop generating them.

Alan Stern

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Alan Stern]

On Wed, 30 Jan 2013, Mark Einon wrote:

> >> > >> This patch fixes the kernel warning generated when putting an MSI MS-1727
> >> > >> GT740 laptop into suspend mode. The call sequence in this case calls
> >> > >> free_irq() twice, once in pci_remove() and once then in pci_suspend().
> >> > >
> >> > > You mean /first/ in pci_suspend() and /then/ in pci_remove() on the
> >> > > already suspended devices, right?
> >> >
> >> > Yes, I did. The call sequence is suspend then resume. My bad.
> >
> > Why does the pci_suspend routine call free_irq() at all?  As far as I
> > know, it's not supposed to do that.  Won't the device continue to use
> > the same IRQ after it is resumed?
> 
> This sounds reasonable to me - I think we could probably get rid of
> the request_irq() call from resume, and use
> disable_irq()/enable_irq()?

Why mess around with IRQ settings at all?  Just have the suspend
routine tell the controller to stop generating them.

Alan Stern

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Mark Einon]

On 31 January 2013 15:04, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 30 Jan 2013, Mark Einon wrote:
>
>> >> > >> This patch fixes the kernel warning generated when putting an MSI MS-1727
>> >> > >> GT740 laptop into suspend mode. The call sequence in this case calls
>> >> > >> free_irq() twice, once in pci_remove() and once then in pci_suspend().
>> >> > >
>> >> > > You mean /first/ in pci_suspend() and /then/ in pci_remove() on the
>> >> > > already suspended devices, right?
>> >> >
>> >> > Yes, I did. The call sequence is suspend then resume. My bad.
>> >
>> > Why does the pci_suspend routine call free_irq() at all?  As far as I
>> > know, it's not supposed to do that.  Won't the device continue to use
>> > the same IRQ after it is resumed?
>>
>> This sounds reasonable to me - I think we could probably get rid of
>> the request_irq() call from resume, and use
>> disable_irq()/enable_irq()?
>
> Why mess around with IRQ settings at all?  Just have the suspend
> routine tell the controller to stop generating them.
>
> Alan Stern
>

I looked into doing this; using context_stop() to stop the controller running.

However, removing the enable_irq() from pci_resume() involves not
calling ohci_enable() (as it is also the fw_card_driver.enable
function, and can't easily be modified). As this call involves a lot
of register writes and I have no devices to test, I decided against
it.

I'll send an updated patch for consideration that merely uses a bool
to stop the irq being freed twice - crude, but it works without
changing too much code.

Cheers,

Mark

[PATCH v2] firewire: Fix ohci free_irq() warning

[Mark Einon]

This patch fixes the kernel warning generated when putting an MSI MS-1727
GT740 laptop into suspend mode. The call sequence in this case calls
free_irq() twice, once in pci_remove() and once then in pci_suspend().

[  262.299486] WARNING: at /build/buildd/linux-3.5.0/kernel/irq/manage.c:1198 __free_irq+0xa3/0x1e0()
[  262.299487] Hardware name: MS-1727
[  262.299488] Trying to free already-free IRQ 16
[  262.299488] Modules linked in: ip6table_filter ip6_tables ebtable_nat ebtables xt_state ipt_REJECT xt_CHECKSUM iptable_mangle xt_tcpudp iptable_filter snd_hda_codec_hdmi ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 ip_tables x_tables bridge stp llc joydev arc4 parport_pc ppdev coretemp kvm_intel snd_hda_codec_realtek kvm microcode snd_seq_midi snd_rawmidi psmouse snd_seq_midi_event serio_raw nvidia(PO) i7core_edac snd_seq edac_core snd_hda_intel iwlwifi snd_hda_codec jmb38x_ms snd_hwdep mac80211 snd_pcm lpc_ich memstick snd_seq_device cfg80211 snd_timer snd soundcore snd_page_alloc ir_lirc_codec lirc_dev ir_mce_kbd_decoder ir_sanyo_decoder ir_sony_decoder ir_jvc_decoder ir_rc6_decoder ir_rc5_decoder ir_nec_decoder rfcomm bnep rc_rc6_mce mxm_wmi ene_ir rc_core bluetooth wmi video acpiphp mac_hid lp parport binfmt_misc hid_generic usbhid hid firewire_ohci sdhci_pci r8169 firewire_core sdhci crc_itu_t
[  262.299537] Pid: 4, comm: kworker/0:0 Tainted: P           O 3.5.0-22-generic #34-Ubuntu
[  262.299538] Call Trace:
[  262.299540]  [<ffffffff81051c1f>] warn_slowpath_common+0x7f/0xc0
[  262.299545]  [<ffffffff81051d16>] warn_slowpath_fmt+0x46/0x50
[  262.299548]  [<ffffffff8103fa39>] ? default_spin_lock_flags+0x9/0x10
[  262.299551]  [<ffffffff810df6b3>] __free_irq+0xa3/0x1e0
[  262.299554]  [<ffffffff810df844>] free_irq+0x54/0xc0
[  262.299558]  [<ffffffffa005a27e>] pci_remove+0x6e/0x210 [firewire_ohci]
[  262.299564]  [<ffffffff8135ae7f>] pci_device_remove+0x3f/0x110
[  262.299569]  [<ffffffff8141fdbc>] __device_release_driver+0x7c/0xe0
[  262.299573]  [<ffffffff8141fe4c>] device_release_driver+0x2c/0x40
[  262.299576]  [<ffffffff8141f5f1>] bus_remove_device+0xe1/0x120
[  262.299578]  [<ffffffff8141cd1a>] device_del+0x12a/0x1c0
[  262.299581]  [<ffffffff8141cdc6>] device_unregister+0x16/0x30
[  262.299583]  [<ffffffff81354784>] pci_stop_bus_device+0x94/0xa0
[  262.299588]  [<ffffffffa0091c67>] acpiphp_disable_slot+0xb7/0x1a0 [acpiphp]
[  262.299594]  [<ffffffffa0090716>] ? get_slot_status+0x46/0xc0 [acpiphp]
[  262.299599]  [<ffffffffa0091d7d>] acpiphp_check_bridge.isra.15+0x2d/0xf0 [acpiphp]
[  262.299604]  [<ffffffffa0092442>] _handle_hotplug_event_bridge+0x372/0x4d0 [acpiphp]
[  262.299608]  [<ffffffff81390f8c>] ? acpi_os_execute_deferred+0x2f/0x34
[  262.299612]  [<ffffffff8116e22d>] ? kfree+0xed/0x110
[  262.299617]  [<ffffffff8107086a>] process_one_work+0x12a/0x420
[  262.299620]  [<ffffffffa00920d0>] ? _handle_hotplug_event_func+0x1d0/0x1d0 [acpiphp]
[  262.299625]  [<ffffffff8107141e>] worker_thread+0x12e/0x2f0
[  262.299627]  [<ffffffff810712f0>] ? manage_workers.isra.26+0x200/0x200
[  262.299629]  [<ffffffff81075f13>] kthread+0x93/0xa0
[  262.299632]  [<ffffffff8168d024>] kernel_thread_helper+0x4/0x10
[  262.299636]  [<ffffffff81075e80>] ? kthread_freezable_should_stop+0x70/0x70
[  262.299639]  [<ffffffff8168d020>] ? gs_change+0x13/0x13
[  262.299641] ---[ end trace 3f830890e076911f ]---

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/firewire/ohci.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 45912e6..8e1c7a9 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -196,6 +196,7 @@ struct fw_ohci {
 	bool csr_state_setclear_abdicate;
 	int n_ir;
 	int n_it;
+	bool irq_requested; /* pci dev irq request successful, not freed */
 	/*
 	 * Spinlock for accessing fw_ohci data.  Never call out of
 	 * this driver with this lock held.
@@ -2384,7 +2385,7 @@ static int ohci_enable(struct fw_card *card,
 
 	if (!(ohci->quirks & QUIRK_NO_MSI))
 		pci_enable_msi(dev);
-	if (request_irq(dev->irq, irq_handler,
+	if (!ohci->irq_requested && request_irq(dev->irq, irq_handler,
 			pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED,
 			ohci_driver_name, ohci)) {
 		dev_err(card->device, "failed to allocate interrupt %d\n",
@@ -2398,6 +2399,8 @@ static int ohci_enable(struct fw_card *card,
 			ohci->next_config_rom = NULL;
 		}
 		return -EIO;
+	} else {
+		ohci->irq_requested = true;
 	}
 
 	irqs =	OHCI1394_reqTxComplete | OHCI1394_respTxComplete |
@@ -3733,8 +3736,10 @@ static void pci_remove(struct pci_dev *dev)
 	 */
 
 	software_reset(ohci);
-	free_irq(dev->irq, ohci);
-
+	if (ohci->irq_requested) {
+		free_irq(dev->irq, ohci);
+		ohci->irq_requested = false;
+	}
 	if (ohci->next_config_rom && ohci->next_config_rom != ohci->config_rom)
 		dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE,
 				  ohci->next_config_rom, ohci->next_config_rom_bus);
@@ -3766,7 +3771,10 @@ static int pci_suspend(struct pci_dev *dev, pm_message_t state)
 	int err;
 
 	software_reset(ohci);
-	free_irq(dev->irq, ohci);
+	if (ohci->irq_requested) {
+		free_irq(dev->irq, ohci);
+		ohci->irq_requested = false;
+	}
 	pci_disable_msi(dev);
 	err = pci_save_state(dev);
 	if (err) {
-- 
1.7.10.4

[PATCH v2] firewire: Fix ohci free_irq() warning

[Mark Einon]

This patch fixes the kernel warning generated when putting an MSI MS-1727
GT740 laptop into suspend mode. The call sequence in this case calls
free_irq() twice, once in pci_remove() and once then in pci_suspend().

[  262.299486] WARNING: at /build/buildd/linux-3.5.0/kernel/irq/manage.c:1198 __free_irq+0xa3/0x1e0()
[  262.299487] Hardware name: MS-1727
[  262.299488] Trying to free already-free IRQ 16
[  262.299488] Modules linked in: ip6table_filter ip6_tables ebtable_nat ebtables xt_state ipt_REJECT xt_CHECKSUM iptable_mangle xt_tcpudp iptable_filter snd_hda_codec_hdmi ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 ip_tables x_tables bridge stp llc joydev arc4 parport_pc ppdev coretemp kvm_intel snd_hda_codec_realtek kvm microcode snd_seq_midi snd_rawmidi psmouse snd_seq_midi_event serio_raw nvidia(PO) i7core_edac snd_seq edac_core snd_hda_intel iwlwifi snd_hda_codec jmb38x_ms snd_hwdep mac80211 snd_pcm lpc_ich memstick snd_seq_device cfg80211 snd_timer snd soundcore snd_page_alloc ir_lirc_codec lirc_dev ir_mce_kbd_decoder ir_sanyo_decoder ir_sony_decoder ir_jvc_decoder ir_rc6_decoder ir_rc5_decoder ir_nec_decoder rfcomm bnep rc_rc6_mce mxm_wmi ene_ir 
 rc_core bluetooth wmi video acpiphp mac_hid lp parport binfmt_misc hid_generic usbhid hid firewire_ohci sdhci_pci r8169 firewire_core sdhci crc_itu_t
[  262.299537] Pid: 4, comm: kworker/0:0 Tainted: P           O 3.5.0-22-generic #34-Ubuntu
[  262.299538] Call Trace:
[  262.299540]  [<ffffffff81051c1f>] warn_slowpath_common+0x7f/0xc0
[  262.299545]  [<ffffffff81051d16>] warn_slowpath_fmt+0x46/0x50
[  262.299548]  [<ffffffff8103fa39>] ? default_spin_lock_flags+0x9/0x10
[  262.299551]  [<ffffffff810df6b3>] __free_irq+0xa3/0x1e0
[  262.299554]  [<ffffffff810df844>] free_irq+0x54/0xc0
[  262.299558]  [<ffffffffa005a27e>] pci_remove+0x6e/0x210 [firewire_ohci]
[  262.299564]  [<ffffffff8135ae7f>] pci_device_remove+0x3f/0x110
[  262.299569]  [<ffffffff8141fdbc>] __device_release_driver+0x7c/0xe0
[  262.299573]  [<ffffffff8141fe4c>] device_release_driver+0x2c/0x40
[  262.299576]  [<ffffffff8141f5f1>] bus_remove_device+0xe1/0x120
[  262.299578]  [<ffffffff8141cd1a>] device_del+0x12a/0x1c0
[  262.299581]  [<ffffffff8141cdc6>] device_unregister+0x16/0x30
[  262.299583]  [<ffffffff81354784>] pci_stop_bus_device+0x94/0xa0
[  262.299588]  [<ffffffffa0091c67>] acpiphp_disable_slot+0xb7/0x1a0 [acpiphp]
[  262.299594]  [<ffffffffa0090716>] ? get_slot_status+0x46/0xc0 [acpiphp]
[  262.299599]  [<ffffffffa0091d7d>] acpiphp_check_bridge.isra.15+0x2d/0xf0 [acpiphp]
[  262.299604]  [<ffffffffa0092442>] _handle_hotplug_event_bridge+0x372/0x4d0 [acpiphp]
[  262.299608]  [<ffffffff81390f8c>] ? acpi_os_execute_deferred+0x2f/0x34
[  262.299612]  [<ffffffff8116e22d>] ? kfree+0xed/0x110
[  262.299617]  [<ffffffff8107086a>] process_one_work+0x12a/0x420
[  262.299620]  [<ffffffffa00920d0>] ? _handle_hotplug_event_func+0x1d0/0x1d0 [acpiphp]
[  262.299625]  [<ffffffff8107141e>] worker_thread+0x12e/0x2f0
[  262.299627]  [<ffffffff810712f0>] ? manage_workers.isra.26+0x200/0x200
[  262.299629]  [<ffffffff81075f13>] kthread+0x93/0xa0
[  262.299632]  [<ffffffff8168d024>] kernel_thread_helper+0x4/0x10
[  262.299636]  [<ffffffff81075e80>] ? kthread_freezable_should_stop+0x70/0x70
[  262.299639]  [<ffffffff8168d020>] ? gs_change+0x13/0x13
[  262.299641] ---[ end trace 3f830890e076911f ]---

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/firewire/ohci.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 45912e6..8e1c7a9 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -196,6 +196,7 @@ struct fw_ohci {
 	bool csr_state_setclear_abdicate;
 	int n_ir;
 	int n_it;
+	bool irq_requested; /* pci dev irq request successful, not freed */
 	/*
 	 * Spinlock for accessing fw_ohci data.  Never call out of
 	 * this driver with this lock held.
@@ -2384,7 +2385,7 @@ static int ohci_enable(struct fw_card *card,
 
 	if (!(ohci->quirks & QUIRK_NO_MSI))
 		pci_enable_msi(dev);
-	if (request_irq(dev->irq, irq_handler,
+	if (!ohci->irq_requested && request_irq(dev->irq, irq_handler,
 			pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED,
 			ohci_driver_name, ohci)) {
 		dev_err(card->device, "failed to allocate interrupt %d\n",
@@ -2398,6 +2399,8 @@ static int ohci_enable(struct fw_card *card,
 			ohci->next_config_rom = NULL;
 		}
 		return -EIO;
+	} else {
+		ohci->irq_requested = true;
 	}
 
 	irqs =	OHCI1394_reqTxComplete | OHCI1394_respTxComplete |
@@ -3733,8 +3736,10 @@ static void pci_remove(struct pci_dev *dev)
 	 */
 
 	software_reset(ohci);
-	free_irq(dev->irq, ohci);
-
+	if (ohci->irq_requested) {
+		free_irq(dev->irq, ohci);
+		ohci->irq_requested = false;
+	}
 	if (ohci->next_config_rom && ohci->next_config_rom != ohci->config_rom)
 		dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE,
 				  ohci->next_config_rom, ohci->next_config_rom_bus);
@@ -3766,7 +3771,10 @@ static int pci_suspend(struct pci_dev *dev, pm_message_t state)
 	int err;
 
 	software_reset(ohci);
-	free_irq(dev->irq, ohci);
+	if (ohci->irq_requested) {
+		free_irq(dev->irq, ohci);
+		ohci->irq_requested = false;
+	}
 	pci_disable_msi(dev);
 	err = pci_save_state(dev);
 	if (err) {
-- 
1.7.10.4


------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_jan

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Peter Hurley]

On Fri, 2013-02-01 at 19:13 +0000, Mark Einon wrote:
> On 31 January 2013 15:04, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 30 Jan 2013, Mark Einon wrote:
> >
> >> >> > >> This patch fixes the kernel warning generated when putting an MSI MS-1727
> >> >> > >> GT740 laptop into suspend mode. The call sequence in this case calls
> >> >> > >> free_irq() twice, once in pci_remove() and once then in pci_suspend().
> >> >> > >
> >> >> > > You mean /first/ in pci_suspend() and /then/ in pci_remove() on the
> >> >> > > already suspended devices, right?
> >> >> >
> >> >> > Yes, I did. The call sequence is suspend then resume. My bad.
> >> >
> >> > Why does the pci_suspend routine call free_irq() at all?  As far as I
> >> > know, it's not supposed to do that.  Won't the device continue to use
> >> > the same IRQ after it is resumed?
> >>
> >> This sounds reasonable to me - I think we could probably get rid of
> >> the request_irq() call from resume, and use
> >> disable_irq()/enable_irq()?
> >
> > Why mess around with IRQ settings at all?  Just have the suspend
> > routine tell the controller to stop generating them.
> >
> > Alan Stern
> >
> 
> I looked into doing this; using context_stop() to stop the controller running.
> 
> However, removing the enable_irq() from pci_resume() involves not
> calling ohci_enable() (as it is also the fw_card_driver.enable
> function, and can't easily be modified). As this call involves a lot
> of register writes and I have no devices to test, I decided against
> it.
> 
> I'll send an updated patch for consideration that merely uses a bool
> to stop the irq being freed twice - crude, but it works without
> changing too much code.

Hi Mark,

I think what Alan means is that the suspend/resume code should just
mask/unmask interrupts at the OHCI controller, via the OHCI
IntEventClear/Set registers (naturally, saving the current mask and
restoring it on resume).

Of course, there's a lot more to do with an OHCI controller -- as you
note. Like stopping running DMA contexts :)  And restarting them on
resume.

I'd do it, but I'm buried to my eyeballs in tty right now -- not fun. I
can _eventually_ do this as I need to address problems with the FW643
anyway at some point, but it's going to be a little while.

In the meantime, I'm a little confused: you say you can't test this code
because you have no hardware; but then how'd you trip this bug?

Regards,
Peter Hurley

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Peter Hurley]

On Fri, 2013-02-01 at 16:09 -0500, Peter Hurley wrote:
> On Fri, 2013-02-01 at 19:13 +0000, Mark Einon wrote:
> > On 31 January 2013 15:04, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > On Wed, 30 Jan 2013, Mark Einon wrote:
> > >
> > >> >> > >> This patch fixes the kernel warning generated when putting an MSI MS-1727
> > >> >> > >> GT740 laptop into suspend mode. The call sequence in this case calls
> > >> >> > >> free_irq() twice, once in pci_remove() and once then in pci_suspend().
> > >> >> > >
> > >> >> > > You mean /first/ in pci_suspend() and /then/ in pci_remove() on the
> > >> >> > > already suspended devices, right?
> > >> >> >
> > >> >> > Yes, I did. The call sequence is suspend then resume. My bad.
> > >> >
> > >> > Why does the pci_suspend routine call free_irq() at all?  As far as I
> > >> > know, it's not supposed to do that.  Won't the device continue to use
> > >> > the same IRQ after it is resumed?
> > >>
> > >> This sounds reasonable to me - I think we could probably get rid of
> > >> the request_irq() call from resume, and use
> > >> disable_irq()/enable_irq()?
> > >
> > > Why mess around with IRQ settings at all?  Just have the suspend
> > > routine tell the controller to stop generating them.
> > >
> > > Alan Stern
> > >
> > 
> > I looked into doing this; using context_stop() to stop the controller running.
> > 
> > However, removing the enable_irq() from pci_resume() involves not
> > calling ohci_enable() (as it is also the fw_card_driver.enable
> > function, and can't easily be modified). As this call involves a lot
> > of register writes and I have no devices to test, I decided against
> > it.
> > 
> > I'll send an updated patch for consideration that merely uses a bool
> > to stop the irq being freed twice - crude, but it works without
> > changing too much code.

Sorry that should read...

> Hi Mark,
> 
> I think what Alan means is that the suspend/resume code should just
> mask/unmask interrupts at the OHCI controller, via the OHCI
> IntEventClear/Set registers (naturally, saving the current mask and
     ^^^^^^^^
  IntMaskClear/Set

by clearing/setting masterIntEnable

> restoring it on resume).
> 
> Of course, there's a lot more to do with an OHCI controller -- as you
> note. Like stopping running DMA contexts :)  And restarting them on
> resume.
> 
> I'd do it, but I'm buried to my eyeballs in tty right now -- not fun. I
> can _eventually_ do this as I need to address problems with the FW643
> anyway at some point, but it's going to be a little while.
> 
> In the meantime, I'm a little confused: you say you can't test this code
> because you have no hardware; but then how'd you trip this bug?
> 
> Regards,
> Peter Hurley

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Mark Einon]

On 1 February 2013 21:09, Peter Hurley <peter@hurleysoftware.com> wrote:
> Hi Mark,
>
> I think what Alan means is that the suspend/resume code should just
> mask/unmask interrupts at the OHCI controller, via the OHCI
> IntEventClear/Set registers (naturally, saving the current mask and
> restoring it on resume).
>
> Of course, there's a lot more to do with an OHCI controller -- as you
> note. Like stopping running DMA contexts :)  And restarting them on
> resume.
>
> I'd do it, but I'm buried to my eyeballs in tty right now -- not fun. I
> can _eventually_ do this as I need to address problems with the FW643
> anyway at some point, but it's going to be a little while.

Hi Peter,

Ok, understood. I can certainly attempt a patch if I get time.

>
> In the meantime, I'm a little confused: you say you can't test this code
> because you have no hardware; but then how'd you trip this bug?

I can test the code in that I have a firewire port on my laptop, but
haven't got anything to plug into the port.
I assume that any large changes I make are quite capable of breaking
something there...

Cheers,

Mark

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Stefan Richter]

On Jan 30 Mark Einon wrote:
> On 29 January 2013 16:04, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> > Added Cc: linux-pm.
> >
> > On Jan 29 Mark Einon wrote:
> >> On 28 January 2013 23:01, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> >> > On Jan 28 Mark Einon wrote:
> >> >> This patch fixes the kernel warning generated when putting an MSI MS-1727
> >> >> GT740 laptop into suspend mode. The call sequence in this case calls
> >> >> free_irq() twice, once in pci_remove() and once then in pci_suspend().
> >> >
> >> > You mean /first/ in pci_suspend() and /then/ in pci_remove() on the
> >> > already suspended devices, right?
> >>
> >> Yes, I did. The call sequence is suspend then resume. My bad.
> 
> Correction above : s/resume/remove
> 
> >>
> >> >
> >> > Because the other way around, first pci_remove(), then pci_suspend(),
> >> > surely must not appen.  And if it does, the bug is elsewhere but not in
> >> > firewire-ohci.
> >> >
> >> > On that note, is pci_suspend() -> pci_remove() actually a legal state
> >> > transition that must be handled by drivers?
> >>
> >> It's happening on the Ubuntu 12.10 distro which performs the suspend
> >> then remove sequence. I assumed that was normal.
> >
> > Maybe a parent device (PCI bridge or the likes) of the OHCI is
> > requested to be removed during suspend or is being removed during
> > resume, thereby causing a removal request to the OHCI?
> >
> > Or the suspend method of firewire-ohci exited with an error return code...
> > but then the suspend sequence would be rolled back, not the offending
> > device be removed, right?
> >
> > In any case, could you check the dmesg right before the warning which you
> > already posted --- and right after it too --- whether there is an
> > indication what else was going on?
> 
> It looks like the remove call actually happens on the wakeup sequence
> - although I've not yet found the time to investigate further.

OK.  I, as a naive driver maintainer, expect a sequence like
   [suspend request] -> suspend() -> [if ret==0, suspended state] ->
   [resume request] -> resume() -> ...
but what apparently happens is
   [suspend request] -> suspend() -> [if ret==0, suspended state] ->
   [resume request] -> remove() -> ...

If you had an ExpressCard or CardBus controller, then it could of course
happen that the PC is suspended, then the controller card physically
removed, and then a resume been requested.  Question to all:  Would the
driver's resume() method be called on the MIA device be called, or would
the driver's remove() method be called in such a case?

However, this is not what happened in your case:  According to the
hardware information which you provided below, you have a PCI Express
controller which is apparently soldered onto the mainboard, and it is not
obvious to me why the ACPI wake-up is causing this controller attempted to
be removed.

Still, I am not clear on the following two questions:

  - Why is this happening on your laptop at all?

  - Must Linux PCI kernel drivers (or any kernel device drivers supporting
    PM suspend/ remove) be prepared to handle direct state transitions from
    [suspended] to [remove]?

> The lines preceding the crash are:
> 
> Jan 30 23:22:53 msilap kernel: [ 7682.288190] ACPI: Low-level resume complete
> Jan 30 23:22:53 msilap kernel: [ 7682.288228] PM: Restoring platform NVS memory
> Jan 30 23:22:53 msilap kernel: [ 7682.288644] Enabling non-boot CPUs ...
> Jan 30 23:22:53 msilap kernel: [ 7682.288713] smpboot: Booting Node 0 Processor 1 APIC 0x2
> Jan 30 23:22:53 msilap kernel: [ 7682.301963] hpet: hpet3 irq 41 for MSI
> Jan 30 23:22:53 msilap kernel: [ 7682.301978] CPU1 is up
> Jan 30 23:22:53 msilap kernel: [ 7682.302040] smpboot: Booting Node 0 Processor 2 APIC 0x4
> Jan 30 23:22:53 msilap kernel: [ 7682.315394] hpet: hpet4 irq 42 for MSI
> Jan 30 23:22:53 msilap kernel: [ 7682.315408] CPU2 is up
> Jan 30 23:22:53 msilap kernel: [ 7682.315470] smpboot: Booting Node 0 Processor 3 APIC 0x6
> Jan 30 23:22:53 msilap kernel: [ 7682.328815] hpet: hpet5 irq 43 for MSI
> Jan 30 23:22:53 msilap kernel: [ 7682.328828] CPU3 is up
> Jan 30 23:22:53 msilap kernel: [ 7682.328882] smpboot: Booting Node 0 Processor 4 APIC 0x1
> Jan 30 23:22:53 msilap kernel: [ 7682.342256] hpet: hpet6 irq 44 for MSI
> Jan 30 23:22:53 msilap kernel: [ 7682.342269] CPU4 is up
> Jan 30 23:22:53 msilap kernel: [ 7682.342327] smpboot: Booting Node 0 Processor 5 APIC 0x3
> Jan 30 23:22:53 msilap kernel: [ 7682.355737] CPU5 is up
> Jan 30 23:22:53 msilap kernel: [ 7682.355797] smpboot: Booting Node 0 Processor 6 APIC 0x5
> Jan 30 23:22:53 msilap kernel: [ 7682.369213] CPU6 is up
> Jan 30 23:22:53 msilap kernel: [ 7682.369272] smpboot: Booting Node 0 Processor 7 APIC 0x7
> Jan 30 23:22:53 msilap kernel: [ 7682.382720] CPU7 is up
> Jan 30 23:22:53 msilap kernel: [ 7682.388801] ACPI: Waking up from system sleep state S3
> Jan 30 23:22:53 msilap kernel: [ 7682.403997] ------------[ cut here ]------------
> Jan 30 23:22:53 msilap kernel: [ 7682.404007] WARNING: at /home/mark/Source/linux/kernel/irq/manage.c:1248 __free_irq+0xa3/0x1e0()
> <snip>
> 
> then after the first crash trace:
> 
> Jan 30 23:22:53 msilap kernel: [ 7682.404181] ---[ end trace 1a4f0a37b1022aad ]---
> Jan 30 23:22:53 msilap kernel: [ 7682.404211] firewire_ohci 0000:07:00.0: removed fw-ohci device
> Jan 30 23:22:53 msilap kernel: [ 7682.404327] ------------[ cut here ]------------
> Jan 30 23:22:53 msilap kernel: [ 7682.404330] WARNING: at /home/mark/Source/linux/kernel/irq/manage.c:1248 __free_irq+0xa3/0x1e0()
> Jan 30 23:22:53 msilap kernel: [ 7682.404330] Hardware name: MS-1727
> Jan 30 23:22:53 msilap kernel: [ 7682.404331] Trying to free already-free IRQ 16
> Jan 30 23:22:53 msilap kernel: [ 7682.404363] Modules linked in: ip6table_filter ip6_tables ebtable_nat ebtables nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack ipt_REJECT xt_CHECKSUM iptable_mangle xt_tcpudp iptable_filter bnep rfcomm ip_tables bluetooth x_tables bridge stp llc parport_pc ppdev binfmt_misc arc4 nouveau iwldvm mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec iwlwifi coretemp kvm_intel snd_hwdep kvm
> snd_pcm ttm cfg80211 snd_seq_midi snd_rawmidi drm_kms_helper snd_seq_midi_event drm snd_seq snd_timer snd_seq_device joydev snd
> microcode i7core_edac i2c_algo_bit psmouse soundcore edac_core jmb38x_ms msi_wmi serio_raw video memstick mxm_wmi lpc_ich
> sparse_keymap mac_hid wmi snd_page_alloc acpiphp lp parport r8169 firewire_ohci(O) sdhci_pci firewire_core(O) sdhci crc_itu_t
> Jan 30 23:22:53 msilap kernel: [ 7682.404365] Pid: 5968, comm: kworker/0:2 Tainted: G        W  O 3.8.0-rc5-next-20130128+ #6
> Jan 30 23:22:53 msilap kernel: [ 7682.404366] Call Trace:
> Jan 30 23:22:53 msilap kernel: [ 7682.404369]  [<ffffffff810589af>] warn_slowpath_common+0x7f/0xc0
> Jan 30 23:22:53 msilap kernel: [ 7682.404371]  [<ffffffff81058aa6>] warn_slowpath_fmt+0x46/0x50
> Jan 30 23:22:53 msilap kernel: [ 7682.404374]  [<ffffffff81044c69>] ? default_spin_lock_flags+0x9/0x10
> Jan 30 23:22:53 msilap kernel: [ 7682.404376]  [<ffffffff810eabc3>] __free_irq+0xa3/0x1e0
> Jan 30 23:22:53 msilap kernel: [ 7682.404378]  [<ffffffff810ead54>] free_irq+0x54/0xc0
> Jan 30 23:22:53 msilap kernel: [ 7682.404384]  [<ffffffffa000968a>] sdhci_remove_host+0x6a/0x190 [sdhci]
> Jan 30 23:22:53 msilap kernel: [ 7682.404389]  [<ffffffffa002f3ff>] sdhci_pci_remove_slot+0x4f/0xc0 [sdhci_pci]
> Jan 30 23:22:53 msilap kernel: [ 7682.404394]  [<ffffffffa002fa70>] sdhci_pci_remove+0x50/0xa0 [sdhci_pci]
> Jan 30 23:22:53 msilap kernel: [ 7682.404398]  [<ffffffff8137ff26>] pci_device_remove+0x46/0xc0
> Jan 30 23:22:53 msilap kernel: [ 7682.404402]  [<ffffffff81456f3c>] __device_release_driver+0x7c/0xe0
> Jan 30 23:22:53 msilap kernel: [ 7682.404404]  [<ffffffff814571ac>] device_release_driver+0x2c/0x40
> Jan 30 23:22:53 msilap kernel: [ 7682.404406]  [<ffffffff814569b1>] bus_remove_device+0xe1/0x120
> Jan 30 23:22:53 msilap kernel: [ 7682.404409]  [<ffffffff8145403a>] device_del+0x11a/0x1b0
> Jan 30 23:22:53 msilap kernel: [ 7682.404411]  [<ffffffff81379d94>] pci_stop_bus_device+0x94/0xa0
> Jan 30 23:22:53 msilap kernel: [ 7682.404414]  [<ffffffff81379f36>] pci_stop_and_remove_bus_device+0x16/0x30
> Jan 30 23:22:53 msilap kernel: [ 7682.404419]  [<ffffffffa0028b36>] acpiphp_disable_slot+0xf6/0x1a0 [acpiphp]
> Jan 30 23:22:53 msilap kernel: [ 7682.404424]  [<ffffffffa0027716>] ? get_slot_status+0x46/0xc0 [acpiphp]
> Jan 30 23:22:53 msilap kernel: [ 7682.404428]  [<ffffffffa0028c0d>] acpiphp_check_bridge.isra.13+0x2d/0xf0 [acpiphp]
> Jan 30 23:22:53 msilap kernel: [ 7682.404433]  [<ffffffffa0029247>] _handle_hotplug_event_bridge+0x2e7/0x410 [acpiphp]
> Jan 30 23:22:53 msilap kernel: [ 7682.404436]  [<ffffffff813bdc64>] ? acpi_os_execute_deferred+0x2d/0x32
> Jan 30 23:22:53 msilap kernel: [ 7682.404439]  [<ffffffff8117df25>] ? kfree+0x105/0x140
> Jan 30 23:22:53 msilap kernel: [ 7682.404441]  [<ffffffff81077249>] process_one_work+0x159/0x4c0
> Jan 30 23:22:53 msilap kernel: [ 7682.404444]  [<ffffffff8107895e>] worker_thread+0x15e/0x4a0
> Jan 30 23:22:53 msilap kernel: [ 7682.404446]  [<ffffffff81078800>] ? manage_workers+0x2a0/0x2a0
> Jan 30 23:22:53 msilap kernel: [ 7682.404448]  [<ffffffff8107df60>] kthread+0xc0/0xd0
> Jan 30 23:22:53 msilap kernel: [ 7682.404450]  [<ffffffff8107dea0>] ? kthread_create_on_node+0x130/0x130
> Jan 30 23:22:53 msilap kernel: [ 7682.404452]  [<ffffffff816d756c>] ret_from_fork+0x7c/0xb0
> Jan 30 23:22:53 msilap kernel: [ 7682.404454]  [<ffffffff8107dea0>] ? kthread_create_on_node+0x130/0x130
> Jan 30 23:22:53 msilap kernel: [ 7682.404456] ---[ end trace 1a4f0a37b1022aae ]---

As a side note:  The sdhci driver (which is used by the sdhci-pci
driver here) is featuring the same issue as firewire-ohci does:  It is
calling free_irq() in its suspend() method, and this is in conflict with
the free_irq() in its remove() method.

> >
> > Also, what are the parent devices of the OHCI according to "lspci -tv" (or
> > what else can show PCI topology)?
> 
> I ran 'lshw' on the machine, which may be of more use. The relevant
> output is (removing parts not associated with the firewire device):
> 
> msilap
>     description: Notebook
>     product: MS-1727 (To be filled by O.E.M.)
>     vendor: MICRO-STAR INTERNATIONAL CO., LTD
>     version: REV:1.0
>     serial: FFFFFFFF
>     width: 64 bits
>     capabilities: smbios-2.6 dmi-2.6 vsyscall32
>     configuration: boot=normal chassis=notebook family=To be filled by O.E.M. sku=To be filled by O.E.M. uuid=00020003-0004-0005-0006-000700080009
>   *-core
>        description: Motherboard
>        product: MS-1727
>        vendor: MICRO-STAR INTERNATIONAL CO., LTD
>        physical id: 0
>        version: REV:1.0
>        serial: BSS-0123456789
>        slot: To be filled by O.E.M.
>      *-pci:0
>           description: Host bridge
>           product: Core Processor DMI
>           vendor: Intel Corporation
>           physical id: 100
>           bus info: pci@0000:00:00.0
>           version: 11
>           width: 32 bits
>           clock: 33MHz
>         *-pci:4
>              description: PCI bridge
>              product: 5 Series/3400 Series Chipset PCI Express Root Port 5
>              vendor: Intel Corporation
>              physical id: 1c.4
>              bus info: pci@0000:00:1c.4
>              version: 05
>              width: 32 bits
>              clock: 33MHz
>              capabilities: pci pciexpress msi pm normal_decode bus_master cap_list
>              configuration: driver=pcieport
>              resources: irq:16 ioport:9000(size=4096) memory:d4600000-d59fffff ioport:d9d00000(size=2097152)

So, is this Intel PCIe Root Complex acting up and losing contact to other
endpoints at resume, or is the JMicron endpoint pretending to be
nonexistent at resume, or what else is happening?

>            *-firewire
>                 description: FireWire (IEEE 1394)
>                 product: IEEE 1394 Host Controller
>                 vendor: JMicron Technology Corp.
>                 physical id: 0
>                 bus info: pci@0000:07:00.0
>                 version: 00
>                 width: 32 bits
>                 clock: 33MHz
>                 capabilities: pm pciexpress msi ohci bus_master cap_list
>                 configuration: driver=firewire_ohci latency=0
>                 resources: irq:16 memory:d4607000-d46077ff memory:d4606000-d460607f memory:d4605000-d460507f memory:d4604000-d460407f
>            *-generic:0
>                 description: System peripheral
>                 product: SD/MMC Host Controller
>                 vendor: JMicron Technology Corp.
>                 physical id: 0.1
>                 bus info: pci@0000:07:00.1
>                 version: 00
>                 width: 32 bits
>                 clock: 33MHz
>                 capabilities: pm pciexpress msi bus_master cap_list
>                 configuration: driver=sdhci-pci latency=0
>                 resources: irq:16 memory:d4603000-d46030ff
>            *-generic:1 UNCLAIMED
>                 description: SD Host controller
>                 product: Standard SD Host Controller
>                 vendor: JMicron Technology Corp.
>                 physical id: 0.2
>                 bus info: pci@0000:07:00.2
>                 version: 00
>                 width: 32 bits
>                 clock: 33MHz
>                 capabilities: pm pciexpress msi cap_list
>                 configuration: latency=0
>                 resources: memory:d4602000-d46020ff
>            *-generic:2
>                 description: System peripheral
>                 product: MS Host Controller
>                 vendor: JMicron Technology Corp.
>                 physical id: 0.3
>                 bus info: pci@0000:07:00.3
>                 version: 00
>                 width: 32 bits
>                 clock: 33MHz
>                 capabilities: pm pciexpress msi bus_master cap_list
>                 configuration: driver=jmb38x_ms latency=0
>                 resources: irq:16 memory:d4601000-d46010ff
>            *-generic:3 UNCLAIMED
>                 description: System peripheral
>                 product: xD Host Controller
>                 vendor: JMicron Technology Corp.
>                 physical id: 0.4
>                 bus info: pci@0000:07:00.4
>                 version: 00
>                 width: 32 bits
>                 clock: 33MHz
>                 capabilities: pm pciexpress msi bus_master cap_list
>                 configuration: latency=0
>                 resources: memory:d4600000-d46000ff

This seems to be a JMicron JMB388, which is a PCIe combination device with
OHCI-1394 1.1 link and 1394A:2000 PHY, and a 4-in-1 memory card reader
for MMC/SD...SDXC/MS/xD cards. 
-- 
Stefan Richter
-=====-===-= --=- ---=-
http://arcgraph.de/sr/

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Stefan Richter]

On Feb 01 Mark Einon wrote:
> On 1 February 2013 21:09, Peter Hurley <peter@hurleysoftware.com> wrote:
> >>>> On Jan 29 Alan Stern wrote:
> >>>>> Why does the pci_suspend routine call free_irq() at all?  As far as I 
> >>>>> know, it's not supposed to do that.  Won't the device continue to use 
> >>>>> the same IRQ after it is resumed?

As far as I can tell, it happened to be done that way as a side effect of
how the probe() and resume() methods share code.  It has remained like
this since the initial implementation:
http://git.kernel.org/linus/2aef469a35a2

Still, at this point I would like to learn whether .suspend() followed
by .remove() is a valid order of sequence which drivers must support
before I prepare myself to get comfortable with a refactoring of
firewire-ohci's .probe()/.resume()/suspend()/remove().  Obviously, so far
my assumption was that a successful .suspend() can only ever be followed
by .resume().

> > I think what Alan means is that the suspend/resume code should just
> > mask/unmask interrupts at the OHCI controller, via the OHCI
> > IntEventClear/Set registers (naturally, saving the current mask and
> > restoring it on resume).
> >
> > Of course, there's a lot more to do with an OHCI controller -- as you
> > note. Like stopping running DMA contexts :)  And restarting them on
> > resume.
> >
> > I'd do it, but I'm buried to my eyeballs in tty right now -- not fun. I
> > can _eventually_ do this as I need to address problems with the FW643
> > anyway at some point, but it's going to be a little while.
> 
> Hi Peter,
> 
> Ok, understood. I can certainly attempt a patch if I get time.
> 
> >
> > In the meantime, I'm a little confused: you say you can't test this code
> > because you have no hardware; but then how'd you trip this bug?
> 
> I can test the code in that I have a firewire port on my laptop, but
> haven't got anything to plug into the port.
> I assume that any large changes I make are quite capable of breaking
> something there...

This is a valid assumption.  Some years ago I caused a regression in
stable kernel branches in exactly this way myself.
-- 
Stefan Richter
-=====-===-= --=- ---=-
http://arcgraph.de/sr/

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Alan Stern]

On Sat, 2 Feb 2013, Stefan Richter wrote:

> On Feb 01 Mark Einon wrote:
> > On 1 February 2013 21:09, Peter Hurley <peter@hurleysoftware.com> wrote:
> > >>>> On Jan 29 Alan Stern wrote:
> > >>>>> Why does the pci_suspend routine call free_irq() at all?  As far as I 
> > >>>>> know, it's not supposed to do that.  Won't the device continue to use 
> > >>>>> the same IRQ after it is resumed?
> 
> As far as I can tell, it happened to be done that way as a side effect of
> how the probe() and resume() methods share code.  It has remained like
> this since the initial implementation:
> http://git.kernel.org/linus/2aef469a35a2

At one point, quite a few years ago, Linus complained about drivers the 
release IRQs during suspend only to reacquire them during resume.  A 
little refactoring should be able to separate out resource 
acquisition/release (done only during probe and remove) from activation 
and shutdown (also done during resume and suspend).

> Still, at this point I would like to learn whether .suspend() followed
> by .remove() is a valid order of sequence which drivers must support
> before I prepare myself to get comfortable with a refactoring of
> firewire-ohci's .probe()/.resume()/suspend()/remove().  Obviously, so far
> my assumption was that a successful .suspend() can only ever be followed
> by .resume().

It depends on the subsystem.  Some subsystems do have suspend -> remove
transitions and others don't.  In general, it's a good idea for drivers
to be prepared for removal while the system is asleep.  Presumably any
hot-unpluggable bus (which includes most of the important buses these
days) would have to support it.

Alan Stern

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Alan Stern]

On Sat, 2 Feb 2013, Stefan Richter wrote:

> On Feb 01 Mark Einon wrote:
> > On 1 February 2013 21:09, Peter Hurley <peter@hurleysoftware.com> wrote:
> > >>>> On Jan 29 Alan Stern wrote:
> > >>>>> Why does the pci_suspend routine call free_irq() at all?  As far as I 
> > >>>>> know, it's not supposed to do that.  Won't the device continue to use 
> > >>>>> the same IRQ after it is resumed?
> 
> As far as I can tell, it happened to be done that way as a side effect of
> how the probe() and resume() methods share code.  It has remained like
> this since the initial implementation:
> http://git.kernel.org/linus/2aef469a35a2

At one point, quite a few years ago, Linus complained about drivers the 
release IRQs during suspend only to reacquire them during resume.  A 
little refactoring should be able to separate out resource 
acquisition/release (done only during probe and remove) from activation 
and shutdown (also done during resume and suspend).

> Still, at this point I would like to learn whether .suspend() followed
> by .remove() is a valid order of sequence which drivers must support
> before I prepare myself to get comfortable with a refactoring of
> firewire-ohci's .probe()/.resume()/suspend()/remove().  Obviously, so far
> my assumption was that a successful .suspend() can only ever be followed
> by .resume().

It depends on the subsystem.  Some subsystems do have suspend -> remove
transitions and others don't.  In general, it's a good idea for drivers
to be prepared for removal while the system is asleep.  Presumably any
hot-unpluggable bus (which includes most of the important buses these
days) would have to support it.

Alan Stern

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Alan Stern]

On Sat, 2 Feb 2013, Stefan Richter wrote:

> OK.  I, as a naive driver maintainer, expect a sequence like
>    [suspend request] -> suspend() -> [if ret==0, suspended state] ->
>    [resume request] -> resume() -> ...
> but what apparently happens is
>    [suspend request] -> suspend() -> [if ret==0, suspended state] ->
>    [resume request] -> remove() -> ...
> 
> If you had an ExpressCard or CardBus controller, then it could of course
> happen that the PC is suspended, then the controller card physically
> removed, and then a resume been requested.  Question to all:  Would the
> driver's resume() method be called on the MIA device be called, or would
> the driver's remove() method be called in such a case?

It depends on the subsystem.  While processing a resume callback for a
device (or for the device's parent), if the subsystem detects that the
device has been removed then most likely it would skip calling the 
driver's resume method and would call the remove method instead.

Alan Stern

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Alan Stern]

On Sat, 2 Feb 2013, Stefan Richter wrote:

> OK.  I, as a naive driver maintainer, expect a sequence like
>    [suspend request] -> suspend() -> [if ret==0, suspended state] ->
>    [resume request] -> resume() -> ...
> but what apparently happens is
>    [suspend request] -> suspend() -> [if ret==0, suspended state] ->
>    [resume request] -> remove() -> ...
> 
> If you had an ExpressCard or CardBus controller, then it could of course
> happen that the PC is suspended, then the controller card physically
> removed, and then a resume been requested.  Question to all:  Would the
> driver's resume() method be called on the MIA device be called, or would
> the driver's remove() method be called in such a case?

It depends on the subsystem.  While processing a resume callback for a
device (or for the device's parent), if the subsystem detects that the
device has been removed then most likely it would skip calling the 
driver's resume method and would call the remove method instead.

Alan Stern

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Stefan Richter]

On Feb 02 Alan Stern wrote:
> On Sat, 2 Feb 2013, Stefan Richter wrote:
> 
> > On Feb 01 Mark Einon wrote:
> > > On 1 February 2013 21:09, Peter Hurley <peter@hurleysoftware.com> wrote:
> > > >>>> On Jan 29 Alan Stern wrote:
> > > >>>>> Why does the pci_suspend routine call free_irq() at all?  As far as I 
> > > >>>>> know, it's not supposed to do that.  Won't the device continue to use 
> > > >>>>> the same IRQ after it is resumed?
> > 
> > As far as I can tell, it happened to be done that way as a side effect of
> > how the probe() and resume() methods share code.  It has remained like
> > this since the initial implementation:
> > http://git.kernel.org/linus/2aef469a35a2
> 
> At one point, quite a few years ago, Linus complained about drivers the 
> release IRQs during suspend only to reacquire them during resume.  A 
> little refactoring should be able to separate out resource 
> acquisition/release (done only during probe and remove) from activation 
> and shutdown (also done during resume and suspend).
> 
> > Still, at this point I would like to learn whether .suspend() followed
> > by .remove() is a valid order of sequence which drivers must support
> > before I prepare myself to get comfortable with a refactoring of
> > firewire-ohci's .probe()/.resume()/suspend()/remove().  Obviously, so far
> > my assumption was that a successful .suspend() can only ever be followed
> > by .resume().
> 
> It depends on the subsystem.  Some subsystems do have suspend -> remove
> transitions and others don't.  In general, it's a good idea for drivers
> to be prepared for removal while the system is asleep.  Presumably any
> hot-unpluggable bus (which includes most of the important buses these
> days) would have to support it.

OK, thank you.  In this case we are of course dealing with the pci
subsystem (and with PCI/ CardBus/ PCI Express/ ExpressCard attached
hardware).  Maybe I should have addressed my question to linux-pci
instead of linux-pm; however, if this is the general expectation,
then I too prefer firewire-ohci to be able to handle it even if the pci
subsystem wouldn't require it presently (which now sounds unlikely).
-- 
Stefan Richter
-=====-===-= --=- ---=-
http://arcgraph.de/sr/

Re: [PATCH] firewire: Fix ohci free_irq() warning

[Stefan Richter]

On Feb 02 Alan Stern wrote:
> On Sat, 2 Feb 2013, Stefan Richter wrote:
> 
> > On Feb 01 Mark Einon wrote:
> > > On 1 February 2013 21:09, Peter Hurley <peter@hurleysoftware.com> wrote:
> > > >>>> On Jan 29 Alan Stern wrote:
> > > >>>>> Why does the pci_suspend routine call free_irq() at all?  As far as I 
> > > >>>>> know, it's not supposed to do that.  Won't the device continue to use 
> > > >>>>> the same IRQ after it is resumed?
> > 
> > As far as I can tell, it happened to be done that way as a side effect of
> > how the probe() and resume() methods share code.  It has remained like
> > this since the initial implementation:
> > http://git.kernel.org/linus/2aef469a35a2
> 
> At one point, quite a few years ago, Linus complained about drivers the 
> release IRQs during suspend only to reacquire them during resume.  A 
> little refactoring should be able to separate out resource 
> acquisition/release (done only during probe and remove) from activation 
> and shutdown (also done during resume and suspend).
> 
> > Still, at this point I would like to learn whether .suspend() followed
> > by .remove() is a valid order of sequence which drivers must support
> > before I prepare myself to get comfortable with a refactoring of
> > firewire-ohci's .probe()/.resume()/suspend()/remove().  Obviously, so far
> > my assumption was that a successful .suspend() can only ever be followed
> > by .resume().
> 
> It depends on the subsystem.  Some subsystems do have suspend -> remove
> transitions and others don't.  In general, it's a good idea for drivers
> to be prepared for removal while the system is asleep.  Presumably any
> hot-unpluggable bus (which includes most of the important buses these
> days) would have to support it.

OK, thank you.  In this case we are of course dealing with the pci
subsystem (and with PCI/ CardBus/ PCI Express/ ExpressCard attached
hardware).  Maybe I should have addressed my question to linux-pci
instead of linux-pm; however, if this is the general expectation,
then I too prefer firewire-ohci to be able to handle it even if the pci
subsystem wouldn't require it presently (which now sounds unlikely).
-- 
Stefan Richter
-=====-===-= --=- ---=-
http://arcgraph.de/sr/

[PATCH v3] firewire: Fix ohci free_irq() warning

[Mark Einon]

This patch fixes the kernel warning below, generated when putting an
MSI MS-1727 GT740 laptop into suspend mode. The call sequence in this
case calls free_irq() twice, once in pci_suspend() and once then in
pci_remove().

The fix breaks up the ohci_enable() call into four separate calls -
ohci_enable_regs(), ohci_create_irq(), ohci_enable_irq() and
ohci_enable_run(). The original call sequence of ohci_enable() is replaced
by the four calls, but the ohci_enable() called from pci_resume() is
replaced by three of the calls, missing out ohci_create_irq().

Finally, a new call, ohci_disable_irq(), replaces the free_irq() in
pci_suspend() which sets the irq mask to zero.

[  262.299486] WARNING: at /build/buildd/linux-3.5.0/kernel/irq/manage.c:1198 __free_irq+0xa3/0x1e0()
[  262.299487] Hardware name: MS-1727
[  262.299488] Trying to free already-free IRQ 16
[  262.299488] Modules linked in: ip6table_filter ip6_tables ebtable_nat ebtables xt_state ipt_REJECT xt_CHECKSUM iptable_mangle xt_tcpudp iptable_filter snd_hda_codec_hdmi ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 ip_tables x_tables bridge stp llc joydev arc4 parport_pc ppdev coretemp kvm_intel snd_hda_codec_realtek kvm microcode snd_seq_midi snd_rawmidi psmouse snd_seq_midi_event serio_raw nvidia(PO) i7core_edac snd_seq edac_core snd_hda_intel iwlwifi snd_hda_codec jmb38x_ms snd_hwdep mac80211 snd_pcm lpc_ich memstick snd_seq_device cfg80211 snd_timer snd soundcore snd_page_alloc ir_lirc_codec lirc_dev ir_mce_kbd_decoder ir_sanyo_decoder ir_sony_decoder ir_jvc_decoder ir_rc6_decoder ir_rc5_decoder ir_nec_decoder rfcomm bnep rc_rc6_mce mxm_wmi ene_ir rc_core bluetooth wmi video acpiphp mac_hid lp parport binfmt_misc hid_generic usbhid hid firewire_ohci sdhci_pci r8169 firewire_core sdhci crc_itu_t
[  262.299537] Pid: 4, comm: kworker/0:0 Tainted: P           O 3.5.0-22-generic #34-Ubuntu
[  262.299538] Call Trace:
[  262.299540]  [<ffffffff81051c1f>] warn_slowpath_common+0x7f/0xc0
[  262.299545]  [<ffffffff81051d16>] warn_slowpath_fmt+0x46/0x50
[  262.299548]  [<ffffffff8103fa39>] ? default_spin_lock_flags+0x9/0x10
[  262.299551]  [<ffffffff810df6b3>] __free_irq+0xa3/0x1e0
[  262.299554]  [<ffffffff810df844>] free_irq+0x54/0xc0
[  262.299558]  [<ffffffffa005a27e>] pci_remove+0x6e/0x210 [firewire_ohci]
[  262.299564]  [<ffffffff8135ae7f>] pci_device_remove+0x3f/0x110
[  262.299569]  [<ffffffff8141fdbc>] __device_release_driver+0x7c/0xe0
[  262.299573]  [<ffffffff8141fe4c>] device_release_driver+0x2c/0x40
[  262.299576]  [<ffffffff8141f5f1>] bus_remove_device+0xe1/0x120
[  262.299578]  [<ffffffff8141cd1a>] device_del+0x12a/0x1c0
[  262.299581]  [<ffffffff8141cdc6>] device_unregister+0x16/0x30
[  262.299583]  [<ffffffff81354784>] pci_stop_bus_device+0x94/0xa0
[  262.299588]  [<ffffffffa0091c67>] acpiphp_disable_slot+0xb7/0x1a0 [acpiphp]
[  262.299594]  [<ffffffffa0090716>] ? get_slot_status+0x46/0xc0 [acpiphp]
[  262.299599]  [<ffffffffa0091d7d>] acpiphp_check_bridge.isra.15+0x2d/0xf0 [acpiphp]
[  262.299604]  [<ffffffffa0092442>] _handle_hotplug_event_bridge+0x372/0x4d0 [acpiphp]
[  262.299608]  [<ffffffff81390f8c>] ? acpi_os_execute_deferred+0x2f/0x34
[  262.299612]  [<ffffffff8116e22d>] ? kfree+0xed/0x110
[  262.299617]  [<ffffffff8107086a>] process_one_work+0x12a/0x420
[  262.299620]  [<ffffffffa00920d0>] ? _handle_hotplug_event_func+0x1d0/0x1d0 [acpiphp]
[  262.299625]  [<ffffffff8107141e>] worker_thread+0x12e/0x2f0
[  262.299627]  [<ffffffff810712f0>] ? manage_workers.isra.26+0x200/0x200
[  262.299629]  [<ffffffff81075f13>] kthread+0x93/0xa0
[  262.299632]  [<ffffffff8168d024>] kernel_thread_helper+0x4/0x10
[  262.299636]  [<ffffffff81075e80>] ? kthread_freezable_should_stop+0x70/0x70
[  262.299639]  [<ffffffff8168d020>] ? gs_change+0x13/0x13
[  262.299641] ---[ end trace 3f830890e076911f ]---

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---

Note - This patch has only been tested with a firewire controller, and
not subsequently with any devices plugged into the controller.

 drivers/firewire/ohci.c |  134 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 91 insertions(+), 43 deletions(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 45912e6..56814b8 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -2242,12 +2242,69 @@ static int probe_tsb41ba3d(struct fw_ohci *ohci)
 	return 1;
 }
 
-static int ohci_enable(struct fw_card *card,
-		       const __be32 *config_rom, size_t length)
+static int ohci_create_irq(struct fw_card *card)
 {
 	struct fw_ohci *ohci = fw_ohci(card);
 	struct pci_dev *dev = to_pci_dev(card->device);
-	u32 lps, version, irqs;
+
+	if (!(ohci->quirks & QUIRK_NO_MSI))
+		pci_enable_msi(dev);
+	if (request_irq(dev->irq, irq_handler,
+			pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED,
+			ohci_driver_name, ohci)) {
+		dev_err(card->device, "failed to allocate interrupt %d\n",
+			dev->irq);
+		pci_disable_msi(dev);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void ohci_disable_irq(struct fw_ohci *ohci)
+{
+	reg_write(ohci, OHCI1394_IntMaskSet, 0);
+
+	reg_write(ohci, OHCI1394_HCControlSet,
+		  ~(OHCI1394_HCControl_linkEnable &
+		  OHCI1394_HCControl_BIBimageValid));
+
+	reg_write(ohci, OHCI1394_LinkControlSet,
+		  ~(OHCI1394_LinkControl_rcvSelfID &
+		  OHCI1394_LinkControl_rcvPhyPkt));
+}
+
+static void ohci_enable_irq(struct fw_ohci *ohci)
+{
+	int irqs = OHCI1394_reqTxComplete | OHCI1394_respTxComplete |
+		   OHCI1394_RQPkt | OHCI1394_RSPkt |
+		   OHCI1394_isochTx | OHCI1394_isochRx |
+		   OHCI1394_postedWriteErr |
+		   OHCI1394_selfIDComplete |
+		   OHCI1394_regAccessFail |
+		   OHCI1394_cycleInconsistent |
+		   OHCI1394_unrecoverableError |
+		   OHCI1394_cycleTooLong |
+		   OHCI1394_masterIntEnable;
+
+	if (param_debug & OHCI_PARAM_DEBUG_BUSRESETS)
+		irqs |= OHCI1394_busReset;
+	reg_write(ohci, OHCI1394_IntMaskSet, irqs);
+
+	reg_write(ohci, OHCI1394_HCControlSet,
+		  OHCI1394_HCControl_linkEnable |
+		  OHCI1394_HCControl_BIBimageValid);
+
+	reg_write(ohci, OHCI1394_LinkControlSet,
+		  OHCI1394_LinkControl_rcvSelfID |
+		  OHCI1394_LinkControl_rcvPhyPkt);
+}
+
+static int ohci_enable_regs(struct fw_card *card,
+			    const __be32 *config_rom, size_t length)
+{
+	struct fw_ohci *ohci = fw_ohci(card);
+	u32 lps, version;
 	int i, ret;
 
 	if (software_reset(ohci)) {
@@ -2382,53 +2439,41 @@ static int ohci_enable(struct fw_card *card,
 
 	reg_write(ohci, OHCI1394_AsReqFilterHiSet, 0x80000000);
 
-	if (!(ohci->quirks & QUIRK_NO_MSI))
-		pci_enable_msi(dev);
-	if (request_irq(dev->irq, irq_handler,
-			pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED,
-			ohci_driver_name, ohci)) {
-		dev_err(card->device, "failed to allocate interrupt %d\n",
-			dev->irq);
-		pci_disable_msi(dev);
+	return 0;
+}
+
+static void ohci_enable_run(struct fw_ohci *ohci)
+{
+	ar_context_run(&ohci->ar_request_ctx);
+	ar_context_run(&ohci->ar_response_ctx);
 
+	flush_writes(ohci);
+
+	/* We are ready to go, reset bus to finish initialization. */
+	fw_schedule_bus_reset(&ohci->card, false, true);
+}
+
+static int ohci_enable(struct fw_card *card,
+		       const __be32 *config_rom, size_t length)
+{
+	struct fw_ohci *ohci = fw_ohci(card);
+	int ret;
+
+	ohci_enable_regs(card, config_rom, length);
+
+	ret = ohci_create_irq(card);
+	if (ret) {
 		if (config_rom) {
 			dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE,
 					  ohci->next_config_rom,
 					  ohci->next_config_rom_bus);
 			ohci->next_config_rom = NULL;
 		}
-		return -EIO;
+		return ret;
 	}
 
-	irqs =	OHCI1394_reqTxComplete | OHCI1394_respTxComplete |
-		OHCI1394_RQPkt | OHCI1394_RSPkt |
-		OHCI1394_isochTx | OHCI1394_isochRx |
-		OHCI1394_postedWriteErr |
-		OHCI1394_selfIDComplete |
-		OHCI1394_regAccessFail |
-		OHCI1394_cycleInconsistent |
-		OHCI1394_unrecoverableError |
-		OHCI1394_cycleTooLong |
-		OHCI1394_masterIntEnable;
-	if (param_debug & OHCI_PARAM_DEBUG_BUSRESETS)
-		irqs |= OHCI1394_busReset;
-	reg_write(ohci, OHCI1394_IntMaskSet, irqs);
-
-	reg_write(ohci, OHCI1394_HCControlSet,
-		  OHCI1394_HCControl_linkEnable |
-		  OHCI1394_HCControl_BIBimageValid);
-
-	reg_write(ohci, OHCI1394_LinkControlSet,
-		  OHCI1394_LinkControl_rcvSelfID |
-		  OHCI1394_LinkControl_rcvPhyPkt);
-
-	ar_context_run(&ohci->ar_request_ctx);
-	ar_context_run(&ohci->ar_response_ctx);
-
-	flush_writes(ohci);
-
-	/* We are ready to go, reset bus to finish initialization. */
-	fw_schedule_bus_reset(&ohci->card, false, true);
+	ohci_enable_irq(ohci);
+	ohci_enable_run(ohci);
 
 	return 0;
 }
@@ -3766,7 +3811,7 @@ static int pci_suspend(struct pci_dev *dev, pm_message_t state)
 	int err;
 
 	software_reset(ohci);
-	free_irq(dev->irq, ohci);
+	ohci_disable_irq(ohci);
 	pci_disable_msi(dev);
 	err = pci_save_state(dev);
 	if (err) {
@@ -3802,10 +3847,13 @@ static int pci_resume(struct pci_dev *dev)
 		reg_write(ohci, OHCI1394_GUIDHi, (u32)(ohci->card.guid >> 32));
 	}
 
-	err = ohci_enable(&ohci->card, NULL, 0);
+	err = ohci_enable_regs(&ohci->card, NULL, 0);
 	if (err)
 		return err;
 
+	ohci_enable_irq(ohci);
+	ohci_enable_run(ohci);
+
 	ohci_resume_iso_dma(ohci);
 
 	return 0;
-- 
1.7.10.4

[PATCH v3] firewire: Fix ohci free_irq() warning

[Mark Einon]

This patch fixes the kernel warning below, generated when putting an
MSI MS-1727 GT740 laptop into suspend mode. The call sequence in this
case calls free_irq() twice, once in pci_suspend() and once then in
pci_remove().

The fix breaks up the ohci_enable() call into four separate calls -
ohci_enable_regs(), ohci_create_irq(), ohci_enable_irq() and
ohci_enable_run(). The original call sequence of ohci_enable() is replaced
by the four calls, but the ohci_enable() called from pci_resume() is
replaced by three of the calls, missing out ohci_create_irq().

Finally, a new call, ohci_disable_irq(), replaces the free_irq() in
pci_suspend() which sets the irq mask to zero.

[  262.299486] WARNING: at /build/buildd/linux-3.5.0/kernel/irq/manage.c:1198 __free_irq+0xa3/0x1e0()
[  262.299487] Hardware name: MS-1727
[  262.299488] Trying to free already-free IRQ 16
[  262.299488] Modules linked in: ip6table_filter ip6_tables ebtable_nat ebtables xt_state ipt_REJECT xt_CHECKSUM iptable_mangle xt_tcpudp iptable_filter snd_hda_codec_hdmi ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 ip_tables x_tables bridge stp llc joydev arc4 parport_pc ppdev coretemp kvm_intel snd_hda_codec_realtek kvm microcode snd_seq_midi snd_rawmidi psmouse snd_seq_midi_event serio_raw nvidia(PO) i7core_edac snd_seq edac_core snd_hda_intel iwlwifi snd_hda_codec jmb38x_ms snd_hwdep mac80211 snd_pcm lpc_ich memstick snd_seq_device cfg80211 snd_timer snd soundcore snd_page_alloc ir_lirc_codec lirc_dev ir_mce_kbd_decoder ir_sanyo_decoder ir_sony_decoder ir_jvc_decoder ir_rc6_decoder ir_rc5_decoder ir_nec_decoder rfcomm bnep rc_rc6_mce mxm_wmi ene_ir 
 rc_core bluetooth wmi video acpiphp mac_hid lp parport binfmt_misc hid_generic usbhid hid firewire_ohci sdhci_pci r8169 firewire_core sdhci crc_itu_t
[  262.299537] Pid: 4, comm: kworker/0:0 Tainted: P           O 3.5.0-22-generic #34-Ubuntu
[  262.299538] Call Trace:
[  262.299540]  [<ffffffff81051c1f>] warn_slowpath_common+0x7f/0xc0
[  262.299545]  [<ffffffff81051d16>] warn_slowpath_fmt+0x46/0x50
[  262.299548]  [<ffffffff8103fa39>] ? default_spin_lock_flags+0x9/0x10
[  262.299551]  [<ffffffff810df6b3>] __free_irq+0xa3/0x1e0
[  262.299554]  [<ffffffff810df844>] free_irq+0x54/0xc0
[  262.299558]  [<ffffffffa005a27e>] pci_remove+0x6e/0x210 [firewire_ohci]
[  262.299564]  [<ffffffff8135ae7f>] pci_device_remove+0x3f/0x110
[  262.299569]  [<ffffffff8141fdbc>] __device_release_driver+0x7c/0xe0
[  262.299573]  [<ffffffff8141fe4c>] device_release_driver+0x2c/0x40
[  262.299576]  [<ffffffff8141f5f1>] bus_remove_device+0xe1/0x120
[  262.299578]  [<ffffffff8141cd1a>] device_del+0x12a/0x1c0
[  262.299581]  [<ffffffff8141cdc6>] device_unregister+0x16/0x30
[  262.299583]  [<ffffffff81354784>] pci_stop_bus_device+0x94/0xa0
[  262.299588]  [<ffffffffa0091c67>] acpiphp_disable_slot+0xb7/0x1a0 [acpiphp]
[  262.299594]  [<ffffffffa0090716>] ? get_slot_status+0x46/0xc0 [acpiphp]
[  262.299599]  [<ffffffffa0091d7d>] acpiphp_check_bridge.isra.15+0x2d/0xf0 [acpiphp]
[  262.299604]  [<ffffffffa0092442>] _handle_hotplug_event_bridge+0x372/0x4d0 [acpiphp]
[  262.299608]  [<ffffffff81390f8c>] ? acpi_os_execute_deferred+0x2f/0x34
[  262.299612]  [<ffffffff8116e22d>] ? kfree+0xed/0x110
[  262.299617]  [<ffffffff8107086a>] process_one_work+0x12a/0x420
[  262.299620]  [<ffffffffa00920d0>] ? _handle_hotplug_event_func+0x1d0/0x1d0 [acpiphp]
[  262.299625]  [<ffffffff8107141e>] worker_thread+0x12e/0x2f0
[  262.299627]  [<ffffffff810712f0>] ? manage_workers.isra.26+0x200/0x200
[  262.299629]  [<ffffffff81075f13>] kthread+0x93/0xa0
[  262.299632]  [<ffffffff8168d024>] kernel_thread_helper+0x4/0x10
[  262.299636]  [<ffffffff81075e80>] ? kthread_freezable_should_stop+0x70/0x70
[  262.299639]  [<ffffffff8168d020>] ? gs_change+0x13/0x13
[  262.299641] ---[ end trace 3f830890e076911f ]---

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---

Note - This patch has only been tested with a firewire controller, and
not subsequently with any devices plugged into the controller.

 drivers/firewire/ohci.c |  134 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 91 insertions(+), 43 deletions(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 45912e6..56814b8 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -2242,12 +2242,69 @@ static int probe_tsb41ba3d(struct fw_ohci *ohci)
 	return 1;
 }
 
-static int ohci_enable(struct fw_card *card,
-		       const __be32 *config_rom, size_t length)
+static int ohci_create_irq(struct fw_card *card)
 {
 	struct fw_ohci *ohci = fw_ohci(card);
 	struct pci_dev *dev = to_pci_dev(card->device);
-	u32 lps, version, irqs;
+
+	if (!(ohci->quirks & QUIRK_NO_MSI))
+		pci_enable_msi(dev);
+	if (request_irq(dev->irq, irq_handler,
+			pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED,
+			ohci_driver_name, ohci)) {
+		dev_err(card->device, "failed to allocate interrupt %d\n",
+			dev->irq);
+		pci_disable_msi(dev);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void ohci_disable_irq(struct fw_ohci *ohci)
+{
+	reg_write(ohci, OHCI1394_IntMaskSet, 0);
+
+	reg_write(ohci, OHCI1394_HCControlSet,
+		  ~(OHCI1394_HCControl_linkEnable &
+		  OHCI1394_HCControl_BIBimageValid));
+
+	reg_write(ohci, OHCI1394_LinkControlSet,
+		  ~(OHCI1394_LinkControl_rcvSelfID &
+		  OHCI1394_LinkControl_rcvPhyPkt));
+}
+
+static void ohci_enable_irq(struct fw_ohci *ohci)
+{
+	int irqs = OHCI1394_reqTxComplete | OHCI1394_respTxComplete |
+		   OHCI1394_RQPkt | OHCI1394_RSPkt |
+		   OHCI1394_isochTx | OHCI1394_isochRx |
+		   OHCI1394_postedWriteErr |
+		   OHCI1394_selfIDComplete |
+		   OHCI1394_regAccessFail |
+		   OHCI1394_cycleInconsistent |
+		   OHCI1394_unrecoverableError |
+		   OHCI1394_cycleTooLong |
+		   OHCI1394_masterIntEnable;
+
+	if (param_debug & OHCI_PARAM_DEBUG_BUSRESETS)
+		irqs |= OHCI1394_busReset;
+	reg_write(ohci, OHCI1394_IntMaskSet, irqs);
+
+	reg_write(ohci, OHCI1394_HCControlSet,
+		  OHCI1394_HCControl_linkEnable |
+		  OHCI1394_HCControl_BIBimageValid);
+
+	reg_write(ohci, OHCI1394_LinkControlSet,
+		  OHCI1394_LinkControl_rcvSelfID |
+		  OHCI1394_LinkControl_rcvPhyPkt);
+}
+
+static int ohci_enable_regs(struct fw_card *card,
+			    const __be32 *config_rom, size_t length)
+{
+	struct fw_ohci *ohci = fw_ohci(card);
+	u32 lps, version;
 	int i, ret;
 
 	if (software_reset(ohci)) {
@@ -2382,53 +2439,41 @@ static int ohci_enable(struct fw_card *card,
 
 	reg_write(ohci, OHCI1394_AsReqFilterHiSet, 0x80000000);
 
-	if (!(ohci->quirks & QUIRK_NO_MSI))
-		pci_enable_msi(dev);
-	if (request_irq(dev->irq, irq_handler,
-			pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED,
-			ohci_driver_name, ohci)) {
-		dev_err(card->device, "failed to allocate interrupt %d\n",
-			dev->irq);
-		pci_disable_msi(dev);
+	return 0;
+}
+
+static void ohci_enable_run(struct fw_ohci *ohci)
+{
+	ar_context_run(&ohci->ar_request_ctx);
+	ar_context_run(&ohci->ar_response_ctx);
 
+	flush_writes(ohci);
+
+	/* We are ready to go, reset bus to finish initialization. */
+	fw_schedule_bus_reset(&ohci->card, false, true);
+}
+
+static int ohci_enable(struct fw_card *card,
+		       const __be32 *config_rom, size_t length)
+{
+	struct fw_ohci *ohci = fw_ohci(card);
+	int ret;
+
+	ohci_enable_regs(card, config_rom, length);
+
+	ret = ohci_create_irq(card);
+	if (ret) {
 		if (config_rom) {
 			dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE,
 					  ohci->next_config_rom,
 					  ohci->next_config_rom_bus);
 			ohci->next_config_rom = NULL;
 		}
-		return -EIO;
+		return ret;
 	}
 
-	irqs =	OHCI1394_reqTxComplete | OHCI1394_respTxComplete |
-		OHCI1394_RQPkt | OHCI1394_RSPkt |
-		OHCI1394_isochTx | OHCI1394_isochRx |
-		OHCI1394_postedWriteErr |
-		OHCI1394_selfIDComplete |
-		OHCI1394_regAccessFail |
-		OHCI1394_cycleInconsistent |
-		OHCI1394_unrecoverableError |
-		OHCI1394_cycleTooLong |
-		OHCI1394_masterIntEnable;
-	if (param_debug & OHCI_PARAM_DEBUG_BUSRESETS)
-		irqs |= OHCI1394_busReset;
-	reg_write(ohci, OHCI1394_IntMaskSet, irqs);
-
-	reg_write(ohci, OHCI1394_HCControlSet,
-		  OHCI1394_HCControl_linkEnable |
-		  OHCI1394_HCControl_BIBimageValid);
-
-	reg_write(ohci, OHCI1394_LinkControlSet,
-		  OHCI1394_LinkControl_rcvSelfID |
-		  OHCI1394_LinkControl_rcvPhyPkt);
-
-	ar_context_run(&ohci->ar_request_ctx);
-	ar_context_run(&ohci->ar_response_ctx);
-
-	flush_writes(ohci);
-
-	/* We are ready to go, reset bus to finish initialization. */
-	fw_schedule_bus_reset(&ohci->card, false, true);
+	ohci_enable_irq(ohci);
+	ohci_enable_run(ohci);
 
 	return 0;
 }
@@ -3766,7 +3811,7 @@ static int pci_suspend(struct pci_dev *dev, pm_message_t state)
 	int err;
 
 	software_reset(ohci);
-	free_irq(dev->irq, ohci);
+	ohci_disable_irq(ohci);
 	pci_disable_msi(dev);
 	err = pci_save_state(dev);
 	if (err) {
@@ -3802,10 +3847,13 @@ static int pci_resume(struct pci_dev *dev)
 		reg_write(ohci, OHCI1394_GUIDHi, (u32)(ohci->card.guid >> 32));
 	}
 
-	err = ohci_enable(&ohci->card, NULL, 0);
+	err = ohci_enable_regs(&ohci->card, NULL, 0);
 	if (err)
 		return err;
 
+	ohci_enable_irq(ohci);
+	ohci_enable_run(ohci);
+
 	ohci_resume_iso_dma(ohci);
 
 	return 0;
-- 
1.7.10.4


------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb

Re: [PATCH v3] firewire: Fix ohci free_irq() warning

[Stefan Richter]

On Feb 05 Mark Einon wrote:
> This patch fixes the kernel warning below, generated when putting an
> MSI MS-1727 GT740 laptop into suspend mode. The call sequence in this
> case calls free_irq() twice, once in pci_suspend() and once then in
> pci_remove().
> 
> The fix breaks up the ohci_enable() call into four separate calls -
> ohci_enable_regs(), ohci_create_irq(), ohci_enable_irq() and
> ohci_enable_run(). The original call sequence of ohci_enable() is replaced
> by the four calls, but the ohci_enable() called from pci_resume() is
> replaced by three of the calls, missing out ohci_create_irq().
> 
> Finally, a new call, ohci_disable_irq(), replaces the free_irq() in
> pci_suspend() which sets the irq mask to zero.
[...]

Hi,
I haven't forgotten about this patch; just didn't fully check it yet
because I still wanted to look whether there is a more minimalistic
way.  Perhaps I'll post something in the next days.

> @@ -3766,7 +3811,7 @@ static int pci_suspend(struct pci_dev *dev, pm_message_t state)
>  	int err;
>  
>  	software_reset(ohci);
> -	free_irq(dev->irq, ohci);
> +	ohci_disable_irq(ohci);
>  	pci_disable_msi(dev);
>  	err = pci_save_state(dev);
>  	if (err) {
> @@ -3802,10 +3847,13 @@ static int pci_resume(struct pci_dev *dev)
>  		reg_write(ohci, OHCI1394_GUIDHi, (u32)(ohci->card.guid >> 32));
>  	}
>  
> -	err = ohci_enable(&ohci->card, NULL, 0);
> +	err = ohci_enable_regs(&ohci->card, NULL, 0);
>  	if (err)
>  		return err;
>  
> +	ohci_enable_irq(ohci);
> +	ohci_enable_run(ohci);
> +
>  	ohci_resume_iso_dma(ohci);
>  
>  	return 0;

Looks like pci_{en,dis}able_msi are now unbalanced.  I suppose we better
place them immediately around {request,free}_irq.
-- 
Stefan Richter
-=====-===-= --=- =---=
http://arcgraph.de/sr/