Re: [PATCH] ath10k: move irq setup

[Michal Kazior <michal.kazior@tieto.com>]

On 31 July 2013 07:50, Michal Kazior <michal.kazior@tieto.com> wrote:
> On 30 July 2013 20:35, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@tieto.com> 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.