Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"

[Ben Greear]

On 10/18/2017 12:33 AM, Johannes Berg wrote:
> Hi,
>
>> The call to set the rate in the driver comes before the error
>> check.
>>
>> 	if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL)) {
>> 		ret = drv_set_bitrate_mask(local, sdata, mask);
>> 		if (ret) {
>> 			pr_err("%s: drv-set-bitrate-mask had error
>> return: %d\n",
>> 			       sdata->dev->name, ret);
>> 			return ret;
>> 		}
>> 	}
>>
>> 	/*
>> 	 * If active validate the setting and reject it if it doesn't
>> leave
>> 	 * at least one basic rate usable, since we really have to be
>> able
>> 	 * to send something, and if we're an AP we have to be able to
>> do
>> 	 * so at a basic rate so that all clients can receive it.
>> 	 */
>> 	if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
>> 	    sdata->vif.bss_conf.chandef.chan) {
>> 		u32 basic_rates = sdata->vif.bss_conf.basic_rates;
>> 		enum nl80211_band band = sdata-
>>> vif.bss_conf.chandef.chan->band;
>>
>> 		if (!(mask->control[band].legacy & basic_rates)) {
>> 			#### I changed this code so I could set a
>> single rate... --Ben
>> 			pr_err("%s:  WARNING: no legacy rates for
>> band[%d] in set-bitrate-mask.\n",
>> 			       sdata->dev->name, band);
>> 		}
>> 	}
>
> Heh, that's just dumb. I guess I'll fix that by putting the test first,
> no idea how that happened.
>
>>>
>>>> So, I think we should relax this check, at least for ath10k.
>>>
>>> Well, yes and no. I don't think we should make ath10k special here,
>>> and
>>> this fixes a real problem - namely that you can set up the system
>>> so
>>> that you have no usable rates at all, and then you just get a
>>> WARN_ON
>>> and start using the lowest possible rate...
>>
>> Well, there are a million ways to set up a broken system,
>
> True, but this one actually happened in practice, for some reason, and
> stopping the user from constantly shooting themselves in the foot seems
> like a good idea to me. Especially if the user (or application) can't
> really even know what they're getting into.
>
> Now, the case in question was _client_ mode, but still.
>
>> and setting a single rate has a useful purpose, especially with
>> ath10k since it has such limited rate-setting capabilities.
>
> You're stretching the definition of "useful purpose" a bit here though,
> you're about the only one who's ever going to need to set a single
> rate.

People trying to do regulatory testing want this feature, and other people
that are not me also like to test with specific rates.  Still a small-ish set
of people, but bigger than just me at least.

This used to work, and now is broken, so it is a regression of at least somewhat
useful feature.  I think we need a way to re-enable this, even if it requires
poking some sysfs or debugfs file to allow this check to be relaxed.

And for that matter, it might be nice to allow purposefully broken ratesets
(with part of basic missing, for instance), in order to test peer devices (including
other Linux stacks).

>> There is basically never going to be a case where setting a single
>> tx-rate on an AP is a good idea in a general production environment,
>> so maybe a possible WARN-ON is fine?
>
> A WARN_ON() for a user configuration is never fine.
>
> You're assuming that there's actually a user sitting there and doing
> this, which is not necessarily the case.
>
> Even rejecting a single rate setting wouldn't be enough because you can
> get into problems even when you enable multiple rates, e.g. if you
> enable all the CCK rates while connected on 5 GHz.
>
>> If you *do* set up an AP with a limited rateset, then it should
>> simply fail to allow a station to connect if it does not have any
>> matching rates.
>
> That's what requiring at least one basic rate to remain does. If you
> want to have basic rates 6,9,12 and then configure only 18, how would
> the client get rejected? Just configure basic rates differently
> beforehand, and then you can do this easily, and the right thing with
> rejecting clients will happen automatically (in fact, clients might not
> even attempt to connect - even better!)
>
>> This might go back to a previous idea I had (and patches I posted and
>> carry in my tree) to allow setting a different advertise rateset vs
>> usable tx-rateset.
>
> You still have the same problem with which clients support them and
> require them etc.

For regulatory testing purposes:  You can advertise normal basic rateset,
and you can accept those (and even transmit mgt and bcast frames on them),
but for data tx, you use a single fixed rate.  That is one reason it is nice
to have the advertised rateset different from the tx rateset.


>> For existing stations that might not match the new fixed rate, we
>> could purposefully kick them off I guess, but seems like a lot of
>> work for a case that seems pretty irrelevant.
>
> For better or worse, there are people using this API programmatically
> without a user baby-sitting it, so we need to make it work in all
> cases. We can let the user shoot themselves in the foot and have only a
> single usable rate left, but we can't let them hang themselves and have
> no rate left at all.

Out of curiosity, couldn't 'iw' do the same checks?  It is a lot easier to
return a useful error code/message from a user-space app anyway, in case that
can work.

Thanks,
Ben


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