[PATCH] ath9k: Check for errors when reading SREV register

[PATCH] ath9k: Check for errors when reading SREV register

[Tim Schumacher]

Right now, if an error is encountered during the SREV register
read (i.e. an EIO in ath9k_regread()), that error code gets
passed all the way to __ath9k_hw_init(), where it is visible
during the "Chip rev not supported" message.

    ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits
    ath: phy2: Mac Chip Rev 0x0f.3 is not supported by this driver
    ath: phy2: Unable to initialize hardware; initialization status: -95
    ath: phy2: Unable to initialize hardware; initialization status: -95
    ath9k_htc: Failed to initialize the device

Check for -EIO explicitly in ath9k_hw_read_revisions() and return
a boolean based on the success of the operation. Check for that in
__ath9k_hw_init() and abort with a more debugging-friendly message
if reading the revisions wasn't successful.

    ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits
    ath: phy2: Failed to read SREV register
    ath: phy2: Could not read hardware revision
    ath: phy2: Unable to initialize hardware; initialization status: -95
    ath: phy2: Unable to initialize hardware; initialization status: -95
    ath9k_htc: Failed to initialize the device

This helps when debugging by directly showing the first point of
failure and it could prevent possible errors if a 0x0f.3 revision
is ever supported.

Signed-off-by: Tim Schumacher <timschumi@gmx.de>
---
 drivers/net/wireless/ath/ath9k/hw.c | 32 +++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 8581d917635a..b6773d613f0c 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -252,8 +252,9 @@ void ath9k_hw_get_channel_centers(struct ath_hw *ah,
 /* Chip Revisions */
 /******************/
 
-static void ath9k_hw_read_revisions(struct ath_hw *ah)
+static bool ath9k_hw_read_revisions(struct ath_hw *ah)
 {
+	u32 srev;
 	u32 val;
 
 	if (ah->get_mac_revision)
@@ -269,25 +270,33 @@ static void ath9k_hw_read_revisions(struct ath_hw *ah)
 			val = REG_READ(ah, AR_SREV);
 			ah->hw_version.macRev = MS(val, AR_SREV_REVISION2);
 		}
-		return;
+		return true;
 	case AR9300_DEVID_AR9340:
 		ah->hw_version.macVersion = AR_SREV_VERSION_9340;
-		return;
+		return true;
 	case AR9300_DEVID_QCA955X:
 		ah->hw_version.macVersion = AR_SREV_VERSION_9550;
-		return;
+		return true;
 	case AR9300_DEVID_AR953X:
 		ah->hw_version.macVersion = AR_SREV_VERSION_9531;
-		return;
+		return true;
 	case AR9300_DEVID_QCA956X:
 		ah->hw_version.macVersion = AR_SREV_VERSION_9561;
-		return;
+		return true;
 	}
 
-	val = REG_READ(ah, AR_SREV) & AR_SREV_ID;
+	srev = REG_READ(ah, AR_SREV);
+
+	if (srev == -EIO) {
+		ath_err(ath9k_hw_common(ah),
+			"Failed to read SREV register");
+		return false;
+	}
+
+	val = srev & AR_SREV_ID;
 
 	if (val == 0xFF) {
-		val = REG_READ(ah, AR_SREV);
+		val = srev;
 		ah->hw_version.macVersion =
 			(val & AR_SREV_VERSION2) >> AR_SREV_TYPE2_S;
 		ah->hw_version.macRev = MS(val, AR_SREV_REVISION2);
@@ -306,6 +315,8 @@ static void ath9k_hw_read_revisions(struct ath_hw *ah)
 		if (ah->hw_version.macVersion == AR_SREV_VERSION_5416_PCIE)
 			ah->is_pciexpress = true;
 	}
+
+	return true;
 }
 
 /************************************/
@@ -559,7 +570,10 @@ static int __ath9k_hw_init(struct ath_hw *ah)
 	struct ath_common *common = ath9k_hw_common(ah);
 	int r = 0;
 
-	ath9k_hw_read_revisions(ah);
+	if (!ath9k_hw_read_revisions(ah)) {
+		ath_err(common, "Could not read hardware revisions");
+		return -EOPNOTSUPP;
+	}
 
 	switch (ah->hw_version.macVersion) {
 	case AR_SREV_VERSION_5416_PCI:
-- 
2.21.0

Re: [PATCH] ath9k: Check for errors when reading SREV register

[Tom Psyborg]

On 18/03/2019, Tim Schumacher <timschumi@gmx.de> wrote:
> Right now, if an error is encountered during the SREV register
> read (i.e. an EIO in ath9k_regread()), that error code gets
> passed all the way to __ath9k_hw_init(), where it is visible
> during the "Chip rev not supported" message.
>
>     ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits
>     ath: phy2: Mac Chip Rev 0x0f.3 is not supported by this driver
>     ath: phy2: Unable to initialize hardware; initialization status: -95
>     ath: phy2: Unable to initialize hardware; initialization status: -95
>     ath9k_htc: Failed to initialize the device
>
> Check for -EIO explicitly in ath9k_hw_read_revisions() and return
> a boolean based on the success of the operation. Check for that in
> __ath9k_hw_init() and abort with a more debugging-friendly message
> if reading the revisions wasn't successful.
>

you are still passing it all the way from ath9k_hw_init

>     ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits
>     ath: phy2: Failed to read SREV register
>     ath: phy2: Could not read hardware revision
>     ath: phy2: Unable to initialize hardware; initialization status: -95
>     ath: phy2: Unable to initialize hardware; initialization status: -95
>     ath9k_htc: Failed to initialize the device
>
> This helps when debugging by directly showing the first point of
> failure and it could prevent possible errors if a 0x0f.3 revision
> is ever supported.
>

I don't think this is required. Mac Chip Rev 0x0f.3 at least prints
what the problem is instead of generic SREV read failure. Either wrong
define in driver or address overlap caused by large/bad firmware in
ath9k_htc case.

Re: [PATCH] ath9k: Check for errors when reading SREV register

[Tim Schumacher]

On 18.03.19 23:35, Tom Psyborg wrote:
> On 18/03/2019, Tim Schumacher <timschumi@gmx.de> wrote:
>> Right now, if an error is encountered during the SREV register
>> read (i.e. an EIO in ath9k_regread()), that error code gets
>> passed all the way to __ath9k_hw_init(), where it is visible
>> during the "Chip rev not supported" message.
>>
>>     ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits
>>     ath: phy2: Mac Chip Rev 0x0f.3 is not supported by this driver
>>     ath: phy2: Unable to initialize hardware; initialization status: -95
>>     ath: phy2: Unable to initialize hardware; initialization status: -95
>>     ath9k_htc: Failed to initialize the device
>>
>> Check for -EIO explicitly in ath9k_hw_read_revisions() and return
>> a boolean based on the success of the operation. Check for that in
>> __ath9k_hw_init() and abort with a more debugging-friendly message
>> if reading the revisions wasn't successful.
>>
> 
> you are still passing it all the way from ath9k_hw_init
> 

I meant the actual error code (i.e. -EIO) there, which gets checked
for right after the REG_READ() call in ath9k_hw_read_revisions(). If
it's present, it immediately prints the "SREV register" message and
returns false instead of true.

>>     ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits
>>     ath: phy2: Failed to read SREV register
>>     ath: phy2: Could not read hardware revision
>>     ath: phy2: Unable to initialize hardware; initialization status: -95
>>     ath: phy2: Unable to initialize hardware; initialization status: -95
>>     ath9k_htc: Failed to initialize the device
>>
>> This helps when debugging by directly showing the first point of
>> failure and it could prevent possible errors if a 0x0f.3 revision
>> is ever supported.
>>
> 
> I don't think this is required. Mac Chip Rev 0x0f.3 at least prints
> what the problem is instead of generic SREV read failure.

The point of this patch is that the "Mac Chip Rev" (in this case)
is a false message and that it actually _doesn't_ show what the
problem is. There is no Rev that is 0x0f.3. It just falsely takes
the -EIO from ath9k_regread() as a valid return value (since it
is never caught earlier) and tries to extract a revision from it,
which results in 0x0f.3.

The case in where the revision succeeded to read, but it simply
isn't supported by the driver, is untouched and it still prints
the original message.

> Either wrong
> define in driver or address overlap caused by large/bad firmware in
> ath9k_htc case.
>

I don't know why it fails to read the SREV register in my specific
case (I tracked it down to a WMI command timeout, which seems to
only happen on a Raspberry Pi 3), but having the SREV error message
(which points to the actual issue) instead of the false-positive
"Rev not supported" message would have saved me quite some time that
I spent with debugging the issue, searching for the source of the
wrong value.

Re: [PATCH] ath9k: Check for errors when reading SREV register

[Tom Psyborg]

On 19/03/2019, Tim Schumacher <timschumi@gmx.de> wrote:
>
> The case in where the revision succeeded to read, but it simply
> isn't supported by the driver, is untouched and it still prints
> the original message.
>

In that case this change is fine.

> I don't know why it fails to read the SREV register in my specific
> case (I tracked it down to a WMI command timeout, which seems to
> only happen on a Raspberry Pi 3), but having the SREV error message
> (which points to the actual issue) instead of the false-positive
> "Rev not supported" message would have saved me quite some time that
> I spent with debugging the issue, searching for the source of the
> wrong value.
>

Did you try some other device than RPI3 ? I've noticed that on router
and laptop while doing some htc fw builds.

Re: [PATCH] ath9k: Check for errors when reading SREV register

[Tim Schumacher]

On 20.03.19 00:38, Tom Psyborg wrote:
> On 19/03/2019, Tim Schumacher <timschumi@gmx.de> wrote:
>>
>> The case in where the revision succeeded to read, but it simply
>> isn't supported by the driver, is untouched and it still prints
>> the original message.
>>
>
> In that case this change is fine.
>
>> I don't know why it fails to read the SREV register in my specific
>> case (I tracked it down to a WMI command timeout, which seems to
>> only happen on a Raspberry Pi 3), but having the SREV error message
>> (which points to the actual issue) instead of the false-positive
>> "Rev not supported" message would have saved me quite some time that
>> I spent with debugging the issue, searching for the source of the
>> wrong value.
>>
>
> Did you try some other device than RPI3 ? I've noticed that on router
> and laptop while doing some htc fw builds.
>

I don't have that many other devices to test, but everything worked
fine on both of my PCs and it was fine on the RPi3 that was running
a raspbian-based kernel instead of mainline.

Re: [PATCH] ath9k: Check for errors when reading SREV register

[Kalle Valo]

Tim Schumacher <timschumi@gmx.de> writes:

> Right now, if an error is encountered during the SREV register
> read (i.e. an EIO in ath9k_regread()), that error code gets
> passed all the way to __ath9k_hw_init(), where it is visible
> during the "Chip rev not supported" message.
>
>     ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits
>     ath: phy2: Mac Chip Rev 0x0f.3 is not supported by this driver
>     ath: phy2: Unable to initialize hardware; initialization status: -95
>     ath: phy2: Unable to initialize hardware; initialization status: -95
>     ath9k_htc: Failed to initialize the device
>
> Check for -EIO explicitly in ath9k_hw_read_revisions() and return
> a boolean based on the success of the operation. Check for that in
> __ath9k_hw_init() and abort with a more debugging-friendly message
> if reading the revisions wasn't successful.
>
>     ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits
>     ath: phy2: Failed to read SREV register
>     ath: phy2: Could not read hardware revision
>     ath: phy2: Unable to initialize hardware; initialization status: -95
>     ath: phy2: Unable to initialize hardware; initialization status: -95
>     ath9k_htc: Failed to initialize the device
>
> This helps when debugging by directly showing the first point of
> failure and it could prevent possible errors if a 0x0f.3 revision
> is ever supported.
>
> Signed-off-by: Tim Schumacher <timschumi@gmx.de>

[...]

> -	val = REG_READ(ah, AR_SREV) & AR_SREV_ID;
> +	srev = REG_READ(ah, AR_SREV);
> +
> +	if (srev == -EIO) {
> +		ath_err(ath9k_hw_common(ah),
> +			"Failed to read SREV register");
> +		return false;
> +	}

I really don't like how the error handling is implemented in REG_READ().
If the register has value 0xfffffffb (= -EIO ==-5) won't this interpret
that as an error?

-- 
Kalle Valo

Re: [PATCH] ath9k: Check for errors when reading SREV register

[Tim Schumacher]

On 21.03.19 11:02, Kalle Valo wrote:
> Tim Schumacher <timschumi@gmx.de> writes:
>
>> Right now, if an error is encountered during the SREV register
>> read (i.e. an EIO in ath9k_regread()), that error code gets
>> passed all the way to __ath9k_hw_init(), where it is visible
>> during the "Chip rev not supported" message.
>>
>>     ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits
>>     ath: phy2: Mac Chip Rev 0x0f.3 is not supported by this driver
>>     ath: phy2: Unable to initialize hardware; initialization status: -95
>>     ath: phy2: Unable to initialize hardware; initialization status: -95
>>     ath9k_htc: Failed to initialize the device
>>
>> Check for -EIO explicitly in ath9k_hw_read_revisions() and return
>> a boolean based on the success of the operation. Check for that in
>> __ath9k_hw_init() and abort with a more debugging-friendly message
>> if reading the revisions wasn't successful.
>>
>>     ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits
>>     ath: phy2: Failed to read SREV register
>>     ath: phy2: Could not read hardware revision
>>     ath: phy2: Unable to initialize hardware; initialization status: -95
>>     ath: phy2: Unable to initialize hardware; initialization status: -95
>>     ath9k_htc: Failed to initialize the device
>>
>> This helps when debugging by directly showing the first point of
>> failure and it could prevent possible errors if a 0x0f.3 revision
>> is ever supported.
>>
>> Signed-off-by: Tim Schumacher <timschumi@gmx.de>
>
> [...]
>
>> -	val = REG_READ(ah, AR_SREV) & AR_SREV_ID;
>> +	srev = REG_READ(ah, AR_SREV);
>> +
>> +	if (srev == -EIO) {
>> +		ath_err(ath9k_hw_common(ah),
>> +			"Failed to read SREV register");
>> +		return false;
>> +	}
>
> I really don't like how the error handling is implemented in REG_READ().
> If the register has value 0xfffffffb (= -EIO ==-5) won't this interpret
> that as an error?
>

If the register had that value, it would indeed report an error. However
(at least if I read the current code and the data sheet correctly), to make
use of the higher 24 bits of the register, the "small"/old version of the
SREV_ID (i.e. the rightmost 8 bit) need to be set to 0xFF. Therefore, a
register read of 0xfffffffb should never happen in this register.

But the error handling is indeed a bit weird. Making the return value a pure
status indicator and saving the value from the register by passing a
reference would probably be the best solution to fixing this up.

Re: [PATCH] ath9k: Check for errors when reading SREV register

[Kalle Valo]

Tim Schumacher <timschumi@gmx.de> writes:

> On 21.03.19 11:02, Kalle Valo wrote:
>> Tim Schumacher <timschumi@gmx.de> writes:
>>
>>> -	val = REG_READ(ah, AR_SREV) & AR_SREV_ID;
>>> +	srev = REG_READ(ah, AR_SREV);
>>> +
>>> +	if (srev == -EIO) {
>>> +		ath_err(ath9k_hw_common(ah),
>>> +			"Failed to read SREV register");
>>> +		return false;
>>> +	}
>>
>> I really don't like how the error handling is implemented in REG_READ().
>> If the register has value 0xfffffffb (= -EIO ==-5) won't this interpret
>> that as an error?
>>
>
> If the register had that value, it would indeed report an error. However
> (at least if I read the current code and the data sheet correctly), to make
> use of the higher 24 bits of the register, the "small"/old version of the
> SREV_ID (i.e. the rightmost 8 bit) need to be set to 0xFF. Therefore, a
> register read of 0xfffffffb should never happen in this register.

Good, thanks for checking.

> But the error handling is indeed a bit weird. Making the return value a pure
> status indicator and saving the value from the register by passing a
> reference would probably be the best solution to fixing this up.

Yeah, that would be so much better. But that can fixed in another patch,
no need to do that here.

-- 
Kalle Valo

Re: [PATCH] ath9k: Check for errors when reading SREV register

[Kalle Valo]

Tim Schumacher <timschumi@gmx.de> wrote:

> Right now, if an error is encountered during the SREV register
> read (i.e. an EIO in ath9k_regread()), that error code gets
> passed all the way to __ath9k_hw_init(), where it is visible
> during the "Chip rev not supported" message.
> 
>     ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits
>     ath: phy2: Mac Chip Rev 0x0f.3 is not supported by this driver
>     ath: phy2: Unable to initialize hardware; initialization status: -95
>     ath: phy2: Unable to initialize hardware; initialization status: -95
>     ath9k_htc: Failed to initialize the device
> 
> Check for -EIO explicitly in ath9k_hw_read_revisions() and return
> a boolean based on the success of the operation. Check for that in
> __ath9k_hw_init() and abort with a more debugging-friendly message
> if reading the revisions wasn't successful.
> 
>     ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits
>     ath: phy2: Failed to read SREV register
>     ath: phy2: Could not read hardware revision
>     ath: phy2: Unable to initialize hardware; initialization status: -95
>     ath: phy2: Unable to initialize hardware; initialization status: -95
>     ath9k_htc: Failed to initialize the device
> 
> This helps when debugging by directly showing the first point of
> failure and it could prevent possible errors if a 0x0f.3 revision
> is ever supported.
> 
> Signed-off-by: Tim Schumacher <timschumi@gmx.de>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

2f90c7e5d094 ath9k: Check for errors when reading SREV register

-- 
https://patchwork.kernel.org/patch/10858399/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches