[PATCH 1/3] band: return -ENOTSUP when RSSI is too low for rate estimation

[PATCH 1/3] band: return -ENOTSUP when RSSI is too low for rate estimation

[James Prestwood]

This was overlooked in a prior patch and causes the rate estimation
to return -ENETUNREACH if the RSSI is too low to support the
various capabilities. This return was unhandled and was treated as
if the IE was invalid which then printed a warning.

The low RSSI case should just be ignored, similar to if the IE was
not provided at all. In this case return -ENOTSUP so the caller
moves on to the next capability set.

Note: this does result in most of the estimation functions only
      returning 0 or -ENOTSUP as they do little to no validation
      on the frame, but rather just test bits. Additional
      validation could be added in the future which would be
      handled by this patch.
---
 src/band.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/band.c b/src/band.c
index 11cd965e..d71380ae 100644
--- a/src/band.c
+++ b/src/band.c
@@ -121,7 +121,7 @@ int band_estimate_nonht_rate(const struct band *band,
 	}
 
 	if (!max_rate)
-		return -ENETUNREACH;
+		return -ENOTSUP;
 
 	*out_data_rate = max_rate * 500000;
 	return 0;
@@ -319,7 +319,7 @@ int band_estimate_ht_rx_rate(const struct band *band,
 				rssi, sgi, out_data_rate))
 		return 0;
 
-	return -ENETUNREACH;
+	return -ENOTSUP;
 }
 
 static bool find_best_mcs_vht(uint8_t max_index, enum ofdm_channel_width width,
@@ -502,7 +502,7 @@ try_vht80:
 				rssi, nss, sgi, out_data_rate))
 		return 0;
 
-	return -ENETUNREACH;
+	return -ENOTSUP;
 }
 
 /*
@@ -678,7 +678,7 @@ int band_estimate_he_rx_rate(const struct band *band, const uint8_t *hec,
 	}
 
 	if (!rate)
-		return -EBADMSG;
+		return -ENOTSUP;
 
 	*out_data_rate = rate;
 
-- 
2.34.1

[PATCH 2/3] scan: replace -ENETUNREACH with -ENOTSUP for rate estimation return

[James Prestwood]

The rate estimation functions now just return 0 or -ENOTSUP for
success or if no estimation could be made due to missing IEs. Only
in the case of other errors (such as malformed IEs) should a
warning be printed.
---
 src/scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/scan.c b/src/scan.c
index f48ffdef..a22cc27a 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -1608,7 +1608,7 @@ static struct scan_bss *scan_parse_attr_bss(struct l_genl_attr *attr,
 
 		ret = wiphy_estimate_data_rate(wiphy, ies, ies_len, bss,
 						&bss->data_rate);
-		if (ret < 0 && ret != -ENETUNREACH)
+		if (ret < 0 && ret != -ENOTSUP)
 			l_warn("wiphy_estimate_data_rate() failed");
 	}
 
-- 
2.34.1

[PATCH 3/3] wiphy: include MAC of BSS with invalid HE capabilities

[James Prestwood]

The prior print was not very descriptive, and now will log the
MAC of the offending BSS.
---
 src/wiphy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/wiphy.c b/src/wiphy.c
index 712d20cf..8f5e46bf 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -1039,7 +1039,8 @@ int wiphy_estimate_data_rate(struct wiphy *wiphy,
 			break;
 		case IE_TYPE_HE_CAPABILITIES:
 			if (!ie_validate_he_capabilities(iter.data, iter.len)) {
-				l_warn("invalid HE capabilities");
+				l_warn("invalid HE capabilities for "MAC,
+					MAC_STR(bss->addr));
 				continue;
 			}
 
-- 
2.34.1

Re: [PATCH 1/3] band: return -ENOTSUP when RSSI is too low for rate estimation

[Denis Kenzior]

Hi James,

On 4/15/24 10:11, James Prestwood wrote:
> This was overlooked in a prior patch and causes the rate estimation
> to return -ENETUNREACH if the RSSI is too low to support the
> various capabilities. This return was unhandled and was treated as
> if the IE was invalid which then printed a warning.
> 
> The low RSSI case should just be ignored, similar to if the IE was
> not provided at all. In this case return -ENOTSUP so the caller
> moves on to the next capability set.

Err, why?  Just handle the error code.

> 
> Note: this does result in most of the estimation functions only
>        returning 0 or -ENOTSUP as they do little to no validation
>        on the frame, but rather just test bits. Additional
>        validation could be added in the future which would be
>        handled by this patch.
> ---
>   src/band.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

Hmm, this set is failing CI for some reason?

Regards,
-Denis

Re: [PATCH 1/3] band: return -ENOTSUP when RSSI is too low for rate estimation

[James Prestwood]

Hi Denis,

On 4/15/24 11:47 AM, Denis Kenzior wrote:
> Hi James,
>
> On 4/15/24 10:11, James Prestwood wrote:
>> This was overlooked in a prior patch and causes the rate estimation
>> to return -ENETUNREACH if the RSSI is too low to support the
>> various capabilities. This return was unhandled and was treated as
>> if the IE was invalid which then printed a warning.
>>
>> The low RSSI case should just be ignored, similar to if the IE was
>> not provided at all. In this case return -ENOTSUP so the caller
>> moves on to the next capability set.
>
> Err, why?  Just handle the error code.

Seemed better to do this than checking for 2 return codes. And IMO "Not 
Supported" actually describes the situation better than "Network is 
unreachable".

But I don't have a strong preference either way, if you prefer checking 
both we can do that too.

>
>>
>> Note: this does result in most of the estimation functions only
>>        returning 0 or -ENOTSUP as they do little to no validation
>>        on the frame, but rather just test bits. Additional
>>        validation could be added in the future which would be
>>        handled by this patch.
>> ---
>>   src/band.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> Hmm, this set is failing CI for some reason?
Ah, good catch CI :) its because test-band was expecting -EBADMSG when 
all MCS' were unsupported. Really this is a valid IE so we shouldn't 
expect -EBADMSG there but we do. I'll have to change this, or check for 
-ENETUNREACH if we go back to that.
>
> Regards,
> -Denis

Re: [PATCH 1/3] band: return -ENOTSUP when RSSI is too low for rate estimation

[Denis Kenzior]

Hi James,

> 
> Seemed better to do this than checking for 2 return codes. And IMO "Not 
> Supported" actually describes the situation better than "Network is unreachable".

Well, it is supported, just not reachable ;)

> 
> But I don't have a strong preference either way, if you prefer checking both we 
> can do that too.

I rather we check both, since unit tests in particular would probably use the 
extra information.

>> Hmm, this set is failing CI for some reason?
> Ah, good catch CI :) its because test-band was expecting -EBADMSG when all MCS' 
> were unsupported. Really this is a valid IE so we shouldn't expect -EBADMSG 
> there but we do. I'll have to change this, or check for -ENETUNREACH if we go 
> back to that.

Heh :)

Regards,
-Denis