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

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

[Kalle Valo]

greearb@candelatech.com writes:

> From: Ben Greear <greearb@candelatech.com>
>
> I put a v1 NIC from an TP-LINK AC 1750 AP in
> a 64-bit PC, and the OS crashes on bootup.  I'm not
> sure how broken my hardware is (possibly completely non
> functional), but at least with this patch it will no longer
> crash the OS.  Not sure it ever got far enough to try,
> but I also do not have firmware for the NIC.
>
> With this patch I get this info on module load:
>
> ath10k_pci 0000:05:00.0: BAR 0: assigned [mem 0xf4400000-0xf45fffff 64bit]
> ath10k_pci 0000:05:00.0: BAR 0: error updating (0xf4400004 != 0xffffffff)
> ath10k_pci 0000:05:00.0: BAR 0: error updating (high 0x000000 != 0xffffffff)
> ath10k_pci 0000:05:00.0: Refused to change power state, currently in D3
> ath10k: MSI-X interrupt handling (8 intrs)
> ath10k: Unable to wakeup target
> ath10k: target takes too long to wake up (awake count 1)
> ath10k: src_ring ffff88020c0d0a00:  write_index is out of bounds: 4294967295  nentries_mask: 15.
> ath10k: dest_ring ffff88020db2c000:  write_index is out of bounds: 4294967295  nentries_mask: 511.
> ath10k: dest_ring ffff880210d56400:  write_index is out of bounds: 4294967295  nentries_mask: 31.
> ath10k: src_ring ffff880210d57600:  write_index is out of bounds: 4294967295  nentries_mask: 31.
> ath10k: src_ring ffff88020fe70000:  write_index is out of bounds: 4294967295  nentries_mask: 2047.
> ath10k: src_ring ffff880212989b40:  write_index is out of bounds: 4294967295  nentries_mask: 1.
> ath10k: dest_ring ffff880212989960:  write_index is out of bounds: 4294967295  nentries_mask: 1.
> ath10k: Failed to get pcie state addr: -5
> ath10k: early firmware event indicated
> ------------[ cut here ]------------
> WARNING: at /home/greearb/git/linux.wireless-testing/drivers/net/wireless/ath/ath10k/ce.c:771 ath10k_ce_per_engine_service+0x53/0x1b4 [ath10k_pci]()
> ....
> (it hits the warning case about 5-6 times and then seems to quiesce OK).

I haven't seen this myself so it might be a hw problem, but difficult to
say.

> +	/* 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()?

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

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Ben Greear]

On 07/11/2013 02:36 AM, Kalle Valo wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> I put a v1 NIC from an TP-LINK AC 1750 AP in
>> a 64-bit PC, and the OS crashes on bootup.  I'm not
>> sure how broken my hardware is (possibly completely non
>> functional), but at least with this patch it will no longer
>> crash the OS.  Not sure it ever got far enough to try,
>> but I also do not have firmware for the NIC.
>>
>> With this patch I get this info on module load:
>>
>> ath10k_pci 0000:05:00.0: BAR 0: assigned [mem 0xf4400000-0xf45fffff 64bit]
>> ath10k_pci 0000:05:00.0: BAR 0: error updating (0xf4400004 != 0xffffffff)
>> ath10k_pci 0000:05:00.0: BAR 0: error updating (high 0x000000 != 0xffffffff)
>> ath10k_pci 0000:05:00.0: Refused to change power state, currently in D3
>> ath10k: MSI-X interrupt handling (8 intrs)
>> ath10k: Unable to wakeup target
>> ath10k: target takes too long to wake up (awake count 1)
>> ath10k: src_ring ffff88020c0d0a00:  write_index is out of bounds: 4294967295  nentries_mask: 15.
>> ath10k: dest_ring ffff88020db2c000:  write_index is out of bounds: 4294967295  nentries_mask: 511.
>> ath10k: dest_ring ffff880210d56400:  write_index is out of bounds: 4294967295  nentries_mask: 31.
>> ath10k: src_ring ffff880210d57600:  write_index is out of bounds: 4294967295  nentries_mask: 31.
>> ath10k: src_ring ffff88020fe70000:  write_index is out of bounds: 4294967295  nentries_mask: 2047.
>> ath10k: src_ring ffff880212989b40:  write_index is out of bounds: 4294967295  nentries_mask: 1.
>> ath10k: dest_ring ffff880212989960:  write_index is out of bounds: 4294967295  nentries_mask: 1.
>> ath10k: Failed to get pcie state addr: -5
>> ath10k: early firmware event indicated
>> ------------[ cut here ]------------
>> WARNING: at /home/greearb/git/linux.wireless-testing/drivers/net/wireless/ath/ath10k/ce.c:771 ath10k_ce_per_engine_service+0x53/0x1b4 [ath10k_pci]()
>> ....
>> (it hits the warning case about 5-6 times and then seems to quiesce OK).
>
> I haven't seen this myself so it might be a hw problem, but difficult to
> say.
>
>> +	/* 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()?

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.


> 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,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Kalle Valo]

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;


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Ben Greear]

On 07/12/2013 07:51 AM, Kalle Valo wrote:
> 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?

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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Michal Kazior]

Hi,

On 11 July 2013 11:36, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> I put a v1 NIC from an TP-LINK AC 1750 AP in
>> a 64-bit PC, and the OS crashes on bootup.  I'm not
>> sure how broken my hardware is (possibly completely non
>> functional), but at least with this patch it will no longer
>> crash the OS.  Not sure it ever got far enough to try,
>> but I also do not have firmware for the NIC.
>>
>> With this patch I get this info on module load:
>>
>> ath10k_pci 0000:05:00.0: BAR 0: assigned [mem 0xf4400000-0xf45fffff 64bit]
>> ath10k_pci 0000:05:00.0: BAR 0: error updating (0xf4400004 != 0xffffffff)
>> ath10k_pci 0000:05:00.0: BAR 0: error updating (high 0x000000 != 0xffffffff)
>> ath10k_pci 0000:05:00.0: Refused to change power state, currently in D3
>> ath10k: MSI-X interrupt handling (8 intrs)
>> ath10k: Unable to wakeup target
>> ath10k: target takes too long to wake up (awake count 1)
>> ath10k: src_ring ffff88020c0d0a00:  write_index is out of bounds: 4294967295  nentries_mask: 15.
>> ath10k: dest_ring ffff88020db2c000:  write_index is out of bounds: 4294967295  nentries_mask: 511.
>> ath10k: dest_ring ffff880210d56400:  write_index is out of bounds: 4294967295  nentries_mask: 31.
>> ath10k: src_ring ffff880210d57600:  write_index is out of bounds: 4294967295  nentries_mask: 31.
>> ath10k: src_ring ffff88020fe70000:  write_index is out of bounds: 4294967295  nentries_mask: 2047.
>> ath10k: src_ring ffff880212989b40:  write_index is out of bounds: 4294967295  nentries_mask: 1.
>> ath10k: dest_ring ffff880212989960:  write_index is out of bounds: 4294967295  nentries_mask: 1.
>> ath10k: Failed to get pcie state addr: -5
>> ath10k: early firmware event indicated
>> ------------[ cut here ]------------
>> WARNING: at /home/greearb/git/linux.wireless-testing/drivers/net/wireless/ath/ath10k/ce.c:771 ath10k_ce_per_engine_service+0x53/0x1b4 [ath10k_pci]()
>> ....
>> (it hits the warning case about 5-6 times and then seems to quiesce OK).
>
> I haven't seen this myself so it might be a hw problem, but difficult to
> say.
>
>> +     /* 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()?

The teardown sequence is broken. Interrupts aren't
disabled/unregistered soon enough. I have a pending patch that should
fix this (although it's still more of a workaround than a real fix
since we need proper variable init code/ hw init code separation to be
done). However the patch depends on my recovery patchset so I haven't
posted it yet.


Pozdrawiam / Best regards,
Michał Kazior.

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Kalle Valo]

Ben Greear <greearb@candelatech.com> writes:

> Actually, I have no idea what type of hardware this is.  It was
> suggested earlier to me that this AP had a v1 hardware in it, but
> lspci shows this fairly unpromising thing, and the ath10k driver
> appears to call 003c the V2 hardware....
>
> 05:00.0 Network controller: Atheros Communications Inc. Device 003c (rev ff) (prog-if ff)
> 	!!! Unknown header type 7f
> 	Kernel modules: ath10k_pci

That unknown header type looks scary. I'm starting to suspect even more
that you have a hw problem of some sort.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Ben Greear]

On 07/11/2013 02:37 AM, Kalle Valo wrote:
> Ben Greear <greearb@candelatech.com> writes:
>
>> Actually, I have no idea what type of hardware this is.  It was
>> suggested earlier to me that this AP had a v1 hardware in it, but
>> lspci shows this fairly unpromising thing, and the ath10k driver
>> appears to call 003c the V2 hardware....
>>
>> 05:00.0 Network controller: Atheros Communications Inc. Device 003c (rev ff) (prog-if ff)
>> 	!!! Unknown header type 7f
>> 	Kernel modules: ath10k_pci
>
> That unknown header type looks scary. I'm starting to suspect even more
> that you have a hw problem of some sort.

I tried two different NICS extracted from two different APs and the behaviour
is the same, so it is probably not just a manufacturing error.

Please note that I do NOT have v1 firmware, though from what I could tell,
this NIC was showing up as v2 hardware anyway...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k