Re: [ath9k-devel] [PATCH] ath10k: Fix crash when using v1 hardware.

[Kalle Valo <kvalo@qca.qualcomm.com>]

Ben Greear <greearb@candelatech.com> writes:

> On 07/11/2013 02:36 AM, Kalle Valo wrote:
>> greearb@candelatech.com writes:
>>
>>> +	/* On v1 hardware at least, setup can fail, causing ce_id_state to
>>> +	 * be cleaned up, but this method is still called a few times.  Check
>>> +	 * for NULL here so we don't crash.  Probably a better fix is to stop
>>> +	 * the ath10k_pci_ce_tasklet sooner.
>>> +	 */
>>> +	if (WARN_ONCE(!ce_state, "ce_id_to_state[%i] is NULL\n", ce_id))
>>> +		return;
>>> +
>>> +	ctrl_addr = ce_state->ctrl_addr;
>>> +
>>
>> The tests you add look like workarounds. I would prefer to try fix these
>> by going to the source of the problem. Maybe we should add
>> ath10k_pci_wake() and ath10k_do_pci_wake()?

Doh, dropped an essential word from a sentence, again. That's what I get
when trying to do multiple things at the same time.

What I was trying to say: Maybe we should add proper error hanling to
ath10k_pci_wake() and ath10k_do_pci_wake() and that way avoid this?

> These are work-arounds, but you should not let a bad piece of
> hardware/firmware crash the entire OS just because you don't want to
> do sanity checking on the values you get from the firmware. Perhaps
> there is a better fix for the code above, but the warning splat should
> still provide incentive to get it right, while not crashing the OS in
> the meantime.

Sure, the driver should not crash if HW is not functioning correctly.
I'm just saying that adding odd checks at random places is not the
"kernel way" to do things, only GTK people do that ;)

I was thinking that the proper way is to check that wakeup succeeds and
not enable interrupts or something like that.

>> Can you enable few debug logs, like ATH10K_DBG_PCI, and post them? That
>> would give more hint there things are going wrong.
>
> Yes, I can do that.

Thanks.

Oh, did you mention something that ath10k detect the device as hw2.0?
Maybe the PCI ids are wrong? Then you could also force the same
workaround for hw2.0 as hw1.0 has:

--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2145,10 +2145,8 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
 
        switch (pci_dev->device) {
        case QCA988X_1_0_DEVICE_ID:
-               set_bit(ATH10K_PCI_FEATURE_HW_1_0_WARKAROUND, ar_pci->features);
-               break;
        case QCA988X_2_0_DEVICE_ID:
-               set_bit(ATH10K_PCI_FEATURE_MSI_X, ar_pci->features);
+               set_bit(ATH10K_PCI_FEATURE_HW_1_0_WARKAROUND, ar_pci->features);
                break;
        default:
                ret = -ENODEV;