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

Commit Message

Kalle Valo July 12, 2013, 2:51 p.m. UTC

Ben Greear <[e-mail]> writes:

> On 07/11/2013 02:36 AM, Kalle Valo wrote:
>> [e-mail] 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:


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ben Greear July 12, 2013, 3:34 p.m. UTC | #1

On 07/12/2013 07:51 AM, Kalle Valo wrote:
> Ben Greear <[e-mail]> writes:
>
>> On 07/11/2013 02:36 AM, Kalle Valo wrote:
>>> [e-mail] 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?

Maybe..I haven't spent much time in the driver yet, so not sure
the best place to fix the null ce_state issue.

>> 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.

With the array-index crashes, you are asking firmware for some
value, and not sanity checking the return value to make sure it
is bounded by the array length.  In my case, it's returning something very
wrong because PCI isn't talking, or something like that.

But, even with functional PCI, you could get a value that for
whatever reason doesn't match the expected value (ie, buggy firmware).
I think you must sanity check those values that can crash the OS
with stray memory access, at least.

>>> 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:

I tried just changing the #defines so that it was treated as V1,
but that did not help.  I can try your suggestion when I get
a chance.  I hope to have some time to do some more
twiddling on these NICs later today or early next week.

Thanks,
Ben

Patch

--- 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;