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

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

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/