From: Johannes Berg <johannes@sipsolutions.net> To: Ben Greear <greearb@candelatech.com>, "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>, ath10k <ath10k@lists.infradead.org>, kirtika@google.com Subject: Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable" Date: Wed, 18 Oct 2017 09:33:53 +0200 [thread overview] Message-ID: <1508312033.2674.9.camel@sipsolutions.net> (raw) In-Reply-To: <2c293255-aa79-75a0-1c28-994f864cddf4@candelatech.com> 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. > 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 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. johannes
WARNING: multiple messages have this Message-ID (diff)
From: Johannes Berg <johannes@sipsolutions.net> To: Ben Greear <greearb@candelatech.com>, "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>, ath10k <ath10k@lists.infradead.org>, kirtika@google.com Subject: Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable" Date: Wed, 18 Oct 2017 09:33:53 +0200 [thread overview] Message-ID: <1508312033.2674.9.camel@sipsolutions.net> (raw) In-Reply-To: <2c293255-aa79-75a0-1c28-994f864cddf4@candelatech.com> 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. > 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 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. johannes _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
next prev parent reply other threads:[~2017-10-18 7:33 UTC|newest] Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-10-10 20:54 Setting single rate in ath10k broken by "reject/clear user rate mask if not usable" Ben Greear 2017-10-10 20:54 ` Ben Greear 2017-10-11 4:37 ` Not able to set single rate in ath10k (backports-4.14-rc2-1) KAVITA MATHUR 2017-10-11 8:02 ` Setting single rate in ath10k broken by "reject/clear user rate mask if not usable" Johannes Berg 2017-10-11 8:02 ` Johannes Berg 2017-10-11 8:07 ` Johannes Berg 2017-10-11 8:07 ` Johannes Berg 2017-10-11 14:51 ` Ben Greear 2017-10-11 14:51 ` Ben Greear 2017-10-18 7:33 ` Johannes Berg [this message] 2017-10-18 7:33 ` Johannes Berg 2017-10-18 14:50 ` Ben Greear 2017-10-18 14:50 ` Ben Greear 2017-10-18 17:56 ` Oleksij Rempel 2017-10-18 17:56 ` Oleksij Rempel 2017-10-18 20:34 ` Johannes Berg 2017-10-18 20:34 ` Johannes Berg 2017-10-18 20:51 ` Ben Greear 2017-10-18 20:51 ` Ben Greear 2017-10-18 21:02 ` Johannes Berg 2017-10-18 21:02 ` Johannes Berg 2017-10-18 21:30 ` Ben Greear 2017-10-18 21:30 ` Ben Greear 2017-10-25 15:17 ` Johannes Berg 2017-10-25 15:17 ` Johannes Berg 2017-10-25 16:13 ` Ben Greear 2017-10-25 16:13 ` Ben Greear 2017-10-27 20:15 ` Johannes Berg 2017-10-27 20:15 ` Johannes Berg 2017-10-27 20:41 ` Ben Greear 2017-10-27 20:41 ` Ben Greear 2017-11-13 10:09 ` Johannes Berg 2017-11-13 10:09 ` Johannes Berg 2017-11-13 17:05 ` Ben Greear 2017-11-13 17:05 ` Ben Greear
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1508312033.2674.9.camel@sipsolutions.net \ --to=johannes@sipsolutions.net \ --cc=ath10k@lists.infradead.org \ --cc=greearb@candelatech.com \ --cc=kirtika@google.com \ --cc=linux-wireless@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.