Peter Seiderer <ps.report@xxxxxxx> writes:
> Hello Toke,
>
> On Mon, 04 Apr 2022 20:19:39 +0200, Toke Høiland-Jørgensen <toke@xxxxxxx> wrote:
>
>> Peter Seiderer <ps.report@xxxxxxx> writes:
>>
>> > Stop reading (and copying) from ieee80211_tx_rate to ath_tx_info.rates
>> > after list end tag (count == 0, idx < 0), prevents copying of garbage
>> > to card registers.
>>
>> In the normal case I don't think this patch does anything, since any
>> invalid rate entries will already be skipped (just one at a time instead
>> of all at once). So this comment is a bit misleading.
>
> Save some (minimal) compute time? Found it something misleading while
> debugging to see random values written out to the card and found this
> comment in net/mac80211/rate.c:
>
> 648 /*
> 649 * make sure there's no valid rate following
> 650 * an invalid one, just in case drivers don't
> 651 * take the API seriously to stop at -1.
> 652 */
>
> and multiple places doing the same check (count == 0, idx < 0) for validation
> e.g.:
>
> 723 if (i < ARRAY_SIZE(info->control.rates) &&
> 724 info->control.rates[i].idx >= 0 &&
> 725 info->control.rates[i].count) {
>
> or
>
> 742 if (rates[i].idx < 0 || !rates[i].count)
> 743 break;
>
>>
>> Also, Minstrel could in principle produce a rate sequence where the
>> indexes are all positive, but there's one in the middle with a count of
>> 0, couldn't it? With this patch, the last entries of such a sequence
>> would now be skipped...
>
> According to net/mac80211/rc80211_minstrel_ht.c:
>
> 1128 static bool
> 1129 minstrel_ht_txstat_valid(struct minstrel_priv *mp, struct minstrel_ht_sta * mi,
> 1130 struct ieee80211_tx_rate *rate)
> 1131 {
> 1132 int i;
> 1133
> 1134 if (rate->idx < 0)
> 1135 return false;
> 1136
> 1137 if (!rate->count)
> 1138 return false;
> 1139
>
> minstrel although evaluates a rate count of zero as invalid...
So my concern was mostly that the documentation (in mac80211.h) says
that an idx of -1 indicates the end, but says nothing about the count.
Which implies that in principle you could have a rate table of { idx,
count } like { 1, 1 }, { 2, 0 }, { 3, 1 } which would mean all three
rates was valid but the second one would just be "skipped" due to a
count of zero.
But it seems that the code populating the rate table that you linked
above (lines 742/743) actually do abort on either condition, so I guess
it's safe to do so in the driver as well...
-Toke
