On 31 July 2013 07:50, Michal Kazior <michal.kazior@xxxxxxxxx> wrote: > On 30 July 2013 20:35, Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> wrote: >> Michal Kazior <michal.kazior@xxxxxxxxx> writes: >> >>> There was a slight race during PCI shutdown. Since >>> interrupts weren't really stopped (only Copy >>> Engine interrupts were disabled through device hw >>> registers) it was possible for a firmware >>> indication (crash) interrupt to come in after >>> tasklets were synced/killed. This would cause >>> memory corruption and a panic in most cases. It >>> was also possible for interrupt to come before CE >>> was initialized during device probing. >>> >>> Interrupts are required for BMI phase so they are enabled as soon as >>> power_up() is called but are freed upon both power_down() and stop() >>> so there's asymmetry here. As by design stop() cannot be followed by >>> start() it is okay. Both power_down() and stop() should be merged >>> later on to avoid confusion. >> >> Why are the interrupts freed both in power_down() and stop()? I don't >> get that. >> >> What if we call disable_irq() in power_down() instead? > > power_down() must call free_irq(), because power_up() calls > request_irq() (if you want the symmetry). If anything, the stop() > should call disable_irq(), but wouldn't that mean start() should call > enable_irq()? But than, irqs are needed before start().. > > I did think about disable_irq() but AFAIR you need to enable_irq() > later on (so either way you need to keep track of the irq state or > you'll get a ton of WARN_ONs from the system). I'll double check that > and report back later enable/disable_irq must be balanced as well. There are two cases of power cycle: * power_up, power_down * power_up, start, stop, power_down If irq setup is moved from pci_probe/remove to power_up/power_down, then stop() still needs irqs to be halted - either disable_irq, or free_irq. In the latter case power_down must be prepared and not issue free_irq again. If irq setup remains in pci_probe/remove then both stop() and power_down() need irqs to be halted too. Same issue applies. If stop/power_down is merged than the whole problem is solved. This seems like the sane solution to the whole problem but requires some refactoring to be done first. Pozdrawiam / Best regards, Michał Kazior. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
