| Message ID | 20240415151130.40389-1-prestwoj@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] band: return -ENOTSUP when RSSI is too low for rate estimation | expand |
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
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
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
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;
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(-)