From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 29 Jan 2013 00:01:49 +0100 From: Stefan Richter To: Mark Einon Cc: linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH] firewire: Fix ohci free_irq() warning Message-ID: <20130129000149.5fa5b0c3@stein> In-Reply-To: <1359410988-3740-1-git-send-email-mark.einon@gmail.com> References: <1359410988-3740-1-git-send-email-mark.einon@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: 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] [] warn_slowpath_common+0x7f/0xc0 > [ 262.299545] [] warn_slowpath_fmt+0x46/0x50 > [ 262.299548] [] ? default_spin_lock_flags+0x9/0x10 > [ 262.299551] [] __free_irq+0xa3/0x1e0 > [ 262.299554] [] free_irq+0x54/0xc0 > [ 262.299558] [] pci_remove+0x6e/0x210 [firewire_ohci] > [ 262.299564] [] pci_device_remove+0x3f/0x110 > [ 262.299569] [] __device_release_driver+0x7c/0xe0 > [ 262.299573] [] device_release_driver+0x2c/0x40 > [ 262.299576] [] bus_remove_device+0xe1/0x120 > [ 262.299578] [] device_del+0x12a/0x1c0 > [ 262.299581] [] device_unregister+0x16/0x30 > [ 262.299583] [] pci_stop_bus_device+0x94/0xa0 > [ 262.299588] [] acpiphp_disable_slot+0xb7/0x1a0 [acpiphp] > [ 262.299594] [] ? get_slot_status+0x46/0xc0 [acpiphp] > [ 262.299599] [] acpiphp_check_bridge.isra.15+0x2d/0xf0 [acpiphp] > [ 262.299604] [] _handle_hotplug_event_bridge+0x372/0x4d0 [acpiphp] > [ 262.299608] [] ? acpi_os_execute_deferred+0x2f/0x34 > [ 262.299612] [] ? kfree+0xed/0x110 > [ 262.299617] [] process_one_work+0x12a/0x420 > [ 262.299620] [] ? _handle_hotplug_event_func+0x1d0/0x1d0 [acpiphp] > [ 262.299625] [] worker_thread+0x12e/0x2f0 > [ 262.299627] [] ? manage_workers.isra.26+0x200/0x200 > [ 262.299629] [] kthread+0x93/0xa0 > [ 262.299632] [] kernel_thread_helper+0x4/0x10 > [ 262.299636] [] ? kthread_freezable_should_stop+0x70/0x70 > [ 262.299639] [] ? gs_change+0x13/0x13 > [ 262.299641] ---[ end trace 3f830890e076911f ]--- > > Signed-off-by: Mark Einon > --- > 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/