On 2 August 2013 09:41, Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> wrote:
> Michal Kazior <michal.kazior@xxxxxxxxx> writes:
>
>> This fixes interrupt-related issue when no
>> interfaces were running thus the device was
>> considered powered down.
>>
>> The power_down() function isn't really powering
>> down the device. It simply assumed it won't
>> interrupt. This wasn't true in some cases and
>> could lead to paging failures upon FW indication
>> interrupt (i.e. FW crash) because some structures
>> aren't allocated in that device state.
>>
>> One reason for that was that ar_pci->started
>> wasn't reset. The other is interrupts should've
>> been masked when teardown starts.
>>
>> The patch reorganized interrupt setup and makes
>> sure ar_pci->started is reset accordingly.
>>
>> Reported-by: Ben Greear <greearb@xxxxxxxxxxxxxxx>
>> Signed-off-by: Michal Kazior <michal.kazior@xxxxxxxxx>
>> ---
>> v2:
>> * updated commit message
>> * added Reported-By: Ben
>> * added disable_irq() in hif_stop()
>> * added ar_pci->started resetting
>> * removed ar_pci->intr_started
>
> Thanks, this looks much better now.
>
> But I still have one question:
>
>> @@ -1742,6 +1761,12 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
>> {
>> int ret;
>>
>> + ret = ath10k_pci_start_intr(ar);
>> + if (ret) {
>> + ath10k_err("could not start interrupt handling (%d)\n", ret);
>> + goto err;
>> + }
>
> So now we call start_intr() during power_up(), which means that we do
> the request_irq() calls during every interface up event. Does that cause
> any meaningful overhead?
I don't think so.
> For me it looks better to do all resource allocation in
> ath10k_pci_probe(), like request_irq(), and free the resources in
> ath10k_pci_remove(). But then we would need to immeadiately call
> disable_irq() and then enable_irq() from power_up() so I'm not sure if
> that's any better.
Not only that. Since disable/enable_irq must be balanced we'd need
some way to track whether we have irqs enabled/disabled - either with
an extra bool variable, additional ath10k_states or new pci-specific
states.
The patch assumes disable_irq is followed by free_irq (which it is)
and possibly request_irq later on.
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
