Re: [PATCH v2 4/6] rtw88: update regulatory settings implementaion
On Wed, Oct 16, 2019 at 7:55 PM Tony Chuang wrote:
>
> From: Brian Norris
> >
> > On Wed, Oct 16, 2019 at 5:33 AM wrote:
> > > This also supports user regulatory hints, and it should only be
> > > enabled for specific distributions that need this to correct
> > > the cards reglutory.
...
> > There should be a pretty high bar for introducing either new CONFIG_*
> > options or module parameters, in my opinion, and I'm not sure you
> > really satisfied it. Why "should only be enabled" by certain
> > distributions? Your opinion? If it's the technical limitation you
> > refer to ("efuse settings"), then just detect the efuse and prevent
> > user hints only on those modules.
> >
>
> Because the efuse/module does not contain the information if the
> user's hint is allowed. But sometimes distributions require to set the
> regulatory via "NL80211_CMD_SET_REG".
> So we are leaving the CONFIG_* here for some reason that needs it.
Is there ever a case where user hint is not allowed? For what reason?
If not efuse, then what?
Or alternatively: if someone sets CONFIG_RTW88_REGD_USER_REG_HINTS=y,
then what problems will they have? Technical problems (e.g., firmware
will crash on certain modules) or problems?
(If "" means "legal", then just blink twice to acknowledge. Or
just don't answer.)
Brian
Re: [PATCH v2 1/6] rtw88: use macro to check the current band
On Wed, Oct 16, 2019 at 5:33 AM wrote:
> index 4759d6a0ca6e..492a2bfc0d5a 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.h
> +++ b/drivers/net/wireless/realtek/rtw88/main.h
> @@ -58,6 +58,19 @@ struct rtw_hci {
> u8 bulkout_num;
> };
>
> +#define IS_CH_5G_BAND_1(channel) ((channel) >= 36 && (channel <= 48))
> +#define IS_CH_5G_BAND_2(channel) ((channel) >= 52 && (channel <= 64))
> +#define IS_CH_5G_BAND_3(channel) ((channel) >= 100 && (channel <= 144))
> +#define IS_CH_5G_BAND_4(channel) ((channel) >= 149 && (channel <= 177))
There are channels between 48 and 52, 64 and 100, and 144 and 149.
What are you doing with those?
> +#define IS_CH_5G_BAND_MID(channel) \
> + (IS_CH_5G_BAND_2(channel) || IS_CH_5G_BAND_3(channel))
> +
> +#define IS_CH_2G_BAND(channel) ((channel) <= 14)
> +#define IS_CH_5G_BAND(channel) \
> + (IS_CH_5G_BAND_1(channel) || IS_CH_5G_BAND_2(channel) || \
> +IS_CH_5G_BAND_3(channel) || IS_CH_5G_BAND_4(channel))
Given the above (you have major holes in 5G_BAND{1,2,3,4}), this macro
seems like a regression.
Brian
Re: [PATCH v2 3/6] rtw88: Enable 802.11ac beamformee support
Hi, On Wed, Oct 16, 2019 at 08:32:58PM +0800, yhchu...@realtek.com wrote: ... > diff --git a/drivers/net/wireless/realtek/rtw88/bf.h > b/drivers/net/wireless/realtek/rtw88/bf.h > new file mode 100644 > index ..96a8216dd11f > --- /dev/null > +++ b/drivers/net/wireless/realtek/rtw88/bf.h ... > +void rtw_bf_disassoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif, > + struct ieee80211_bss_conf *bss_conf); > +void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif, > + struct ieee80211_bss_conf *bss_conf); > +void rtw_bf_init_bfer_entry_mu(struct rtw_dev *rtwdev, > +struct mu_bfer_init_para *param); Not used outside bf.c, so you should make it static in bf.c and drop the declaration here. > +void rtw_bf_cfg_sounding(struct rtw_dev *rtwdev, struct rtw_vif *vif, > + enum rtw_trx_desc_rate rate); Same. > +void rtw_bf_cfg_mu_bfee(struct rtw_dev *rtwdev, struct cfg_mumimo_para > *param); Same. > +void rtw_bf_del_bfer_entry_mu(struct rtw_dev *rtwdev); Same. > +void rtw_bf_del_sounding(struct rtw_dev *rtwdev); Same. > +void rtw_bf_enable_bfee_su(struct rtw_dev *rtwdev, struct rtw_vif *vif, > +struct rtw_bfee *bfee); > +void rtw_bf_enable_bfee_mu(struct rtw_dev *rtwdev, struct rtw_vif *vif, > +struct rtw_bfee *bfee); > +void rtw_bf_remove_bfee_su(struct rtw_dev *rtwdev, struct rtw_bfee *bfee); > +void rtw_bf_remove_bfee_mu(struct rtw_dev *rtwdev, struct rtw_bfee *bfee); > +void rtw_bf_set_gid_table(struct rtw_dev *rtwdev, struct ieee80211_vif *vif, > + struct ieee80211_bss_conf *conf); > +void rtw_bf_phy_init(struct rtw_dev *rtwdev); > +void rtw_bf_cfg_csi_rate(struct rtw_dev *rtwdev, u8 rssi, u8 cur_rate, > + u8 fixrate_en, u8 *new_rate); > +#endif Brian
Re: [PATCH v2 4/6] rtw88: update regulatory settings implementaion
On Wed, Oct 16, 2019 at 5:33 AM wrote:
> This also supports user regulatory hints, and it should only be
> enabled for specific distributions that need this to correct
> the cards reglutory.
s/cards/card's/
s/reglutory/regulatory/
There should be a pretty high bar for introducing either new CONFIG_*
options or module parameters, in my opinion, and I'm not sure you
really satisfied it. Why "should only be enabled" by certain
distributions? Your opinion? If it's the technical limitation you
refer to ("efuse settings"), then just detect the efuse and prevent
user hints only on those modules.
Brian
[PATCH] rtw88: mark rtw_fw_hdr __packed
The use of u8 and __le16 in this struct assumes that it's going to be
packed to byte alignment. C doesn't guarantee that, so we should mark
this __packed.
Fixes: cc20a7139836 ("rtw88: use struct rtw_fw_hdr to access firmware header")
Cc: Ping-Ke Shih
Signed-off-by: Brian Norris
---
drivers/net/wireless/realtek/rtw88/fw.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtw88/fw.h
b/drivers/net/wireless/realtek/rtw88/fw.h
index 4f7999394235..73d1b9ca8efc 100644
--- a/drivers/net/wireless/realtek/rtw88/fw.h
+++ b/drivers/net/wireless/realtek/rtw88/fw.h
@@ -127,7 +127,7 @@ struct rtw_fw_hdr {
__le32 emem_size;
__le32 emem_addr;
__le32 imem_addr;
-};
+} __packed;
/* C2H */
#define GET_CCX_REPORT_SEQNUM(c2h_payload) (c2h_payload[8] & 0xfc)
--
2.23.0.700.g56cf767bdb-goog
[PATCH] rtw88: include interrupt.h for tasklet_struct
Depending on implicit header includes, we might see this compilation
error:
.../main.h:1391:24: error: field has incomplete type 'struct tasklet_struct'
struct tasklet_struct tx_tasklet;
^
Fixes: 3745d3e550d1 ("rtw88: add driver TX queue support")
Signed-off-by: Brian Norris
---
drivers/net/wireless/realtek/rtw88/main.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/wireless/realtek/rtw88/main.h
b/drivers/net/wireless/realtek/rtw88/main.h
index f3eab96dba86..4759d6a0ca6e 100644
--- a/drivers/net/wireless/realtek/rtw88/main.h
+++ b/drivers/net/wireless/realtek/rtw88/main.h
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include "util.h"
--
2.23.0.700.g56cf767bdb-goog
[PATCH] rtw88: use a for loop in rtw_power_mode_change(), not goto
No change in logic.
Signed-off-by: Brian Norris
---
drivers/net/wireless/realtek/rtw88/ps.c | 61 -
1 file changed, 29 insertions(+), 32 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/ps.c
b/drivers/net/wireless/realtek/rtw88/ps.c
index e81de3ddc8c5..820e0a3a141c 100644
--- a/drivers/net/wireless/realtek/rtw88/ps.c
+++ b/drivers/net/wireless/realtek/rtw88/ps.c
@@ -69,24 +69,26 @@ void rtw_power_mode_change(struct rtw_dev *rtwdev, bool
enter)
u8 polling_cnt;
u8 retry_cnt = 0;
-retry:
- request = rtw_read8(rtwdev, rtwdev->hci.rpwm_addr);
- confirm = rtw_read8(rtwdev, rtwdev->hci.cpwm_addr);
-
- /* toggle to request power mode, others remain 0 */
- request ^= request | BIT_RPWM_TOGGLE;
- if (!enter) {
- request |= POWER_MODE_ACK;
- } else {
- request |= POWER_MODE_LCLK;
- if (rtw_fw_lps_deep_mode == LPS_DEEP_MODE_PG)
- request |= POWER_MODE_PG;
- }
+ for (retry_cnt = 0; retry_cnt < 3; retry_cnt++) {
+ request = rtw_read8(rtwdev, rtwdev->hci.rpwm_addr);
+ confirm = rtw_read8(rtwdev, rtwdev->hci.cpwm_addr);
+
+ /* toggle to request power mode, others remain 0 */
+ request ^= request | BIT_RPWM_TOGGLE;
+ if (!enter) {
+ request |= POWER_MODE_ACK;
+ } else {
+ request |= POWER_MODE_LCLK;
+ if (rtw_fw_lps_deep_mode == LPS_DEEP_MODE_PG)
+ request |= POWER_MODE_PG;
+ }
- rtw_write8(rtwdev, rtwdev->hci.rpwm_addr, request);
+ rtw_write8(rtwdev, rtwdev->hci.rpwm_addr, request);
+
+ if (enter)
+ return;
- /* check confirm power mode has left power save state */
- if (!enter) {
+ /* check confirm power mode has left power save state */
for (polling_cnt = 0; polling_cnt < 3; polling_cnt++) {
polling = rtw_read8(rtwdev, rtwdev->hci.cpwm_addr);
if ((polling ^ confirm) & BIT_RPWM_TOGGLE)
@@ -94,23 +96,18 @@ void rtw_power_mode_change(struct rtw_dev *rtwdev, bool
enter)
mdelay(20);
}
- /* in case of fw/hw missed the request, retry 3 times */
- if (retry_cnt < 3) {
- rtw_warn(rtwdev, "failed to leave deep PS, retry=%d\n",
-retry_cnt);
- retry_cnt++;
- goto retry;
- }
-
- /* Hit here means that driver failed to change hardware
-* power mode to active state after retry 3 times.
-* If the power state is locked at Deep sleep, most of
-* the hardware circuits is not working, even register
-* read/write. It should be treated as fatal error and
-* requires an entire analysis about the firmware/hardware
-*/
- WARN(1, "Hardware power state locked\n");
+ /* in case of fw/hw missed the request, retry */
+ rtw_warn(rtwdev, "failed to leave deep PS, retry=%d\n",
+retry_cnt);
}
+
+ /* Hit here means that driver failed to change hardware power mode to
+* active state after retry 3 times. If the power state is locked at
+* Deep sleep, most of the hardware circuits is not working, even
+* register read/write. It should be treated as fatal error and
+* requires an entire analysis about the firmware/hardware
+*/
+ WARN(1, "Hardware power state locked\n");
}
EXPORT_SYMBOL(rtw_power_mode_change);
--
2.23.0.700.g56cf767bdb-goog
Re: ath10k: Poor performance with kernel 5.3 fixed
On Wed, Sep 25, 2019 at 2:16 AM Federico Cuello wrote: > When upgrading to 5.3 my AP started to work really slow. I tracked the > problem to 4504f0e5b5714d9d26b1a80bf1fc133c95830588 and fixed the issue. For the record, that's: 4504f0e5b571 ath10k: sdio: workaround firmware UART pin configuration bug > The attached patch fixes the issue when uart_print is false and > uart_pin_workaround also false. -ENOPATCH Brian
Re: [PATCH 05/15] rtw88: pci: release tx skbs DMAed when stop
On Fri, Sep 20, 2019 at 1:29 AM Tony Chuang wrote: > > Brian Norris writes: > > > Ah, I was a bit confused. So it does get called from "PS" routines: > > I thought you're talking about IEEE80211_CONF_PS instead of > IEEE80211_CONF_IDLE. Like I said, I was confused :) On first glance, I just saw the codepath showing up in ps.c, but then I noticed it's only for IDLE, not PS. > Also if possible you should queue patch 2, that reordering will cause > two H2C skbs not be released because HCI hasn't started, everytime > enter/leave IDLE state (rtw_power_[on|off]). That patch also looks good to me, FWIW. Side note: it's a little bit strange that your driver can silently misbehave so badly, just by TX'ing in the wrong state. Would this be a good case to add some WARN_ON() or WARN_ON_ONCE() (e.g., in functions like rtw_fw_send_h2c_packet()), to check for the appropriate "started" state? Brian
Re: [PATCH 05/15] rtw88: pci: release tx skbs DMAed when stop
On Tue, Sep 17, 2019 at 7:10 PM Tony Chuang wrote: > > On Mon, Sep 16, 2019 at 12:03 AM wrote: > > > > > > From: Yan-Hsuan Chuang > > > > > > Interrupt is disabled to stop PCI, which means the skbs > > > queued for each TX ring will not be released via DMA > > > interrupt. > > > > In what cases do you hit this? I think you do this when entering PS > > mode, no? But then, see below. > > I'll hit this when ieee80211_ops::stop, or rtw_power_off. > Both are to turn off the device, so there's no more DMA activities. > If we don't release the SKBs that are not released by DMA interrupt > when powering off, these could be leaked. Ah, I was a bit confused. So it does get called from "PS" routines: rtw_enter_ips() -> rtw_core_stop() but that "IPS" mode means "Inactive" Power Save, and it's only used when transitioning into idle states (IEEE80211_CONF_IDLE). Incidentally, I think this also may explain many of the leaks I've been seeing elsewhere, when I leave a device sitting and scanning for a very long time -- each scan attempt is making a single transition out-and-back to IPS mode, which meant it may be leaking any outstanding TX DMA. And testing confirms this: if I just bring up the interface, run a scan, then bring it down, I see many fewer unmaps than maps. Doing this enough times, I run out of contiguous DMA memory and the device stops working. This fixes that problem for me. So: Reviewed-by: Brian Norris Tested-by: Brian Norris I wonder if, given the problems I've seen (the driver can become totally ineroperable), this patch and the previous patch (its only real dependency) should be fast-tracked to the current release. Brian
Re: [PATCH 05/15] rtw88: pci: release tx skbs DMAed when stop
May be a dumb question but: On Mon, Sep 16, 2019 at 12:03 AM wrote: > > From: Yan-Hsuan Chuang > > Interrupt is disabled to stop PCI, which means the skbs > queued for each TX ring will not be released via DMA > interrupt. In what cases do you hit this? I think you do this when entering PS mode, no? But then, see below. > To avoid those skbs remained being left in > the skb queue until PCI has been removed, driver needs > to release skbs by itself. Doesn't that also mean your dropping these packets? Shouldn't you be delaying PS transitions until you've finished TX'ing? Brian
Re: Flag for detecting 802.11r Fast BSS Transition support
On Sat, Aug 17, 2019 at 6:40 AM Marcel Holtmann wrote: > And if you have a distribution or an OEM that cares, then that is what is > going to happen. I can see where you might not be particularly sympathetic to this concern, but where I re-started this thread, I added in Jouni, due to his mention of: "There are such drivers [supporting FT] especially in number of Android devices and I'd rather not break those use cases.." [1] That doesn't exactly sound like a case where he's willing to break compatibility with older kernels in new wpa_supplicant. But maybe I shouldn't put words in his mouth. (On the other hand, Android systems are likely to not ever get either kernel *or* wpa_supplicant version upgrades, so maybe it's not really a problem at all!) Anyway, I'll just cook a patch, and then figure out whether/how I can teach wpa_supplicant to respect it. (Or, continue forking wpa_supplicant as we have been wont to do...) Brian [1] http://lists.infradead.org/pipermail/hostap/2019-April/039951.html
[PATCH 5.4] rtw88: drop unused rtw_coex_coex_dm_reset()
From: Guenter Roeck
0day reports:
sparse warnings:
drivers/net/wireless/realtek/rtw88/coex.c:2457:6: sparse:
symbol 'rtw_coex_coex_dm_reset' was not declared. Should it be static?
rtw_coex_coex_dm_reset() is not called. Remove it.
Fixes: 4136214f7c46 ("rtw88: add BT co-existence support")
Reported-by: kbuild test robot
Signed-off-by: Guenter Roeck
Signed-off-by: Brian Norris
---
drivers/net/wireless/realtek/rtw88/coex.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/coex.c
b/drivers/net/wireless/realtek/rtw88/coex.c
index 4577fceddc5e..9ee860db651a 100644
--- a/drivers/net/wireless/realtek/rtw88/coex.c
+++ b/drivers/net/wireless/realtek/rtw88/coex.c
@@ -2454,11 +2454,6 @@ void rtw_coex_wl_fwdbginfo_notify(struct rtw_dev
*rtwdev, u8 *buf, u8 length)
rtw_coex_wl_ccklock_detect(rtwdev);
}
-void rtw_coex_coex_dm_reset(struct rtw_dev *rtwdev)
-{
- __rtw_coex_init_hw_config(rtwdev, false);
-}
-
void rtw_coex_wl_status_change_notify(struct rtw_dev *rtwdev)
{
struct rtw_coex *coex = &rtwdev->coex;
--
2.23.0.rc1.153.gdeed80330f-goog
Re: [PATCH v3 2/3] rtw88: enclose c2h cmd handle with mutex
On Thu, Aug 15, 2019 at 7:45 PM Tony Chuang wrote: > I think the problem here is I should give a better comment to better > describe the usage of the mutex. And I also want to keep it short. I don't think we have limits on comments in source code -- better to document too much than not enough. Brian
Re: Flag for detecting 802.11r Fast BSS Transition support
Hi Marcel, On Fri, Aug 16, 2019 at 11:54 AM Marcel Holtmann wrote: > > Well, I guess we could just run the command and look for EOPNOTSUPP... > > this kind of API design and usage is bad. Try-and-error approach is just not > sustainable. Sure. That "suggestion" was quite literally an afterthought. Not really a proper suggestion. > Even while it is late to add a proper flag that indicates support, we need to > do this to make nl80211 better for the future. I suppose. I'm not quite sure how I would make use of that properly though, given the corpus of kernels out there where the flag doesn't exist (but the feature does). Some other heurestic for determining kernel recency? Compile-time flags for the relevant user space, such that one builds it for "new kernel API (w/ flag)" vs. "old kernel API" (with the latter not even trying to look for the flag)? Or I guess a more proactive approach: implement both a "supported" and an "unsupported" flag, so user space can figure out a tristate: flag not available (old kernel -- user space is left to guess) vs. command supported flag vs. command not supported flag. That seems a bit awkward though. Brian
Re: [PATCH v3 2/3] rtw88: enclose c2h cmd handle with mutex
I understand this is already queued up, but I still have a question: On Wed, Jul 31, 2019 at 5:23 AM wrote: > C2H commands that cannot be handled in IRQ context should > be protected by rtwdev->mutex. Because they might have a > sequece of hardware operations that does not want to be > interfered. Can you elaborate on what interference you're looking at, exactly? I'm not a big fan of defensive addition of global locks, and this particular mutex isn't very targeted. It claims to be for mac80211 callbacks, but you use it in quite a few places (some of which clearly don't make sense), and many of them are not related to mac80211 callbacks AFAICT. To the contrary: this handler is called from the mac80211 work queue, which is ordered and therefore shouldn't be getting "interrupted" (e.g., conflicting commands). But then, you added the 'irqsafe' command, which gets run from the ISR...and doesn't hold this lock, obviously. It may well be that you're correct here, but I'd like to see a better explanation for stuff like this. And maybe an update to the rtw_dev::mutex comments. Brian
Re: [PATCH] check for driver SME support when selecting FT suites
+ Johannes, linux-wireless I see Matthew resent this with the proper sign-off, but it's not going anywhere, likely because of the below: On Mon, Apr 15, 2019 at 3:38 PM Jouni Malinen wrote: > That said, I'd also like to understand how this has been tested with > drivers that do not use wpa_supplicant for SME, but that do support FT. > There are such drivers especially in number of Android devices and I'd > rather not break those use cases.. I see exactly 1 upstream driver that implements NL80211_CMD_UPDATE_FT_IES. We didn't really get anywhere so far on this thread: https://lore.kernel.org/linux-wireless/211816ff03cf188d834a21b1fbc98b4f8c5b81f4.ca...@sipsolutions.net/ Brian
Re: Flag for detecting 802.11r Fast BSS Transition support
+ Jouni I forgot this was dangling: On Mon, Apr 8, 2019 at 12:52 PM Johannes Berg wrote: > So I guess you'd have to figure out what operations the drivers need to > support then? I'm not even sure how wpa_s would handle this for SME > offload devices. I'm not intimately familiar with FT, but it looks like the only thing wpa_supplicant is asking for is NL80211_CMD_UPDATE_FT_IES. I see exactly one driver that implements this, but there's no flag for it. Well, I guess we could just run the command and look for EOPNOTSUPP... Jouni also claims [1] that there are plenty of out-of-tree non-SME drivers that support FT. Android drivers may not be directly relevant to upstream, but this topic does affect our ability to use wpa_supplicant generically. Brian [1] http://lists.infradead.org/pipermail/hostap/2019-April/039951.html
Re: [PATCH] rtw88: pci: enable MSI interrupt
On Thu, Aug 1, 2019 at 2:21 AM Tony Chuang wrote: > > Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt > > On Tue, Jul 30, 2019 at 07:50:14PM +0800, yhchu...@realtek.com wrote: > > > --- a/drivers/net/wireless/realtek/rtw88/pci.c > > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > > > @@ -874,6 +878,7 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, > > void *dev) > > > if (!rtwpci->irq_enabled) > > > goto out; > > > > > > + rtw_pci_disable_interrupt(rtwdev, rtwpci); > > > > Why exactly do you have to mask interrupts during the ISR? Is there a > > race in rtw_pci_irq_recognized() or something? > > > I think there is a race between SW and HW, if we do not stop the > IRQ first, write 1 clear will make the interrupt to be lost. This doesn't need to slow down this patch (I think v2 is fine), but I still don't quite understand. Before this addition, the sequence is: (a) read out your IRQ status (b) ack the un-masked IRQs you see (c) operate on those IRQs Even if a new IRQ comes in the middle of (b), shouldn't it be sufficient to move on to (c), where you're still prepared to handle that IRQ? Or if the IRQ comes after (b), you won't ACK it, and you should immediately get a new IRQ after you return? I guess that's assuming that these registers are Write 1 to Clear. But if so, that means rtw_pci_irq_recognized() is effectively atomic, no? Also, somewhat unrelated: but why do you unmask HIMR1, when you're not actually handling any of its IRQ bits? Brian > > > > > rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); > > > > > > if (irq_status[0] & IMR_MGNTDOK)
Re: [PATCH] rtw88: pci: enable MSI interrupt
Hi,
On Tue, Jul 30, 2019 at 07:50:14PM +0800, yhchu...@realtek.com wrote:
> From: Yu-Yen Ting
>
> MSI interrupt should be enabled on certain platform.
>
> Add a module parameter disable_msi to disable MSI interrupt,
> driver will then use legacy interrupt instead.
> And the interrupt mode is not able to change at run-time, so
> the module parameter is read only.
Well, if we unbind/rebind the device, probe() will pick up the new
value. e.g.:
echo ':01:00.0' > /sys/bus/pci/drivers/rtw_pci/unbind
echo ':01:00.0' > /sys/bus/pci/drivers/rtw_pci/bind
So is it really necessary to mark read-only? I think there's a general
understanding that module parameters are not always "immediately
effective."
> Tested-by: Ján Veselý
> Signed-off-by: Yu-Yen Ting
> Signed-off-by: Yan-Hsuan Chuang
> ---
> drivers/net/wireless/realtek/rtw88/pci.c | 51
> ++--
> drivers/net/wireless/realtek/rtw88/pci.h | 1 +
> 2 files changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> b/drivers/net/wireless/realtek/rtw88/pci.c
> index 23dd06a..25410f6 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -10,6 +10,10 @@
> #include "rx.h"
> #include "debug.h"
>
> +static bool rtw_disable_msi;
> +module_param_named(disable_msi, rtw_disable_msi, bool, 0444);
> +MODULE_PARM_DESC(disable_msi, "Set Y to disable MSI interrupt support");
> +
> static u32 rtw_pci_tx_queue_idx_addr[] = {
> [RTW_TX_QUEUE_BK] = RTK_PCI_TXBD_IDX_BKQ,
> [RTW_TX_QUEUE_BE] = RTK_PCI_TXBD_IDX_BEQ,
> @@ -874,6 +878,7 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq,
> void *dev)
> if (!rtwpci->irq_enabled)
> goto out;
>
> + rtw_pci_disable_interrupt(rtwdev, rtwpci);
Why exactly do you have to mask interrupts during the ISR? Is there a
race in rtw_pci_irq_recognized() or something?
> rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status);
>
> if (irq_status[0] & IMR_MGNTDOK)
...
> @@ -1103,6 +1110,45 @@ static struct rtw_hci_ops rtw_pci_ops = {
> .write_data_h2c = rtw_pci_write_data_h2c,
> };
>
> +static int rtw_pci_request_irq(struct rtw_dev *rtwdev, struct pci_dev *pdev)
> +{
> + struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> + int ret;
> +
> + if (!rtw_disable_msi) {
> + ret = pci_enable_msi(pdev);
> + if (ret) {
> + rtw_warn(rtwdev, "failed to enable msi, using legacy
> irq\n");
> + } else {
> + rtw_warn(rtwdev, "pci msi enabled\n");
> + rtwpci->msi_enabled = true;
> + }
> + }
> +
> + ret = request_irq(pdev->irq, &rtw_pci_interrupt_handler, IRQF_SHARED,
> + KBUILD_MODNAME, rtwdev);
> + if (ret) {
> + rtw_err(rtwdev, "failed to request irq\n");
Print out 'ret' here?
> + if (rtwpci->msi_enabled) {
> + pci_disable_msi(pdev);
> + rtwpci->msi_enabled = false;
> + }
> + }
> +
> + return ret;
> +}
Otherwise, looks fine:
Reviewed-by: Brian Norris
Re: [5.2 regression] rtwpci + amd iommu
Hi Tony, Can you please submit that patch upstream soon? Thanks, Brian On Tue, Jul 9, 2019 at 11:10 PM Ján Veselý wrote: ... > feel free to add my tested-by, ... > On Wed, Jul 10, 2019 at 1:12 AM Tony Chuang wrote: ... > > I am not sure if enabling MSI interrupt is going to help. > > You can try the patch attached, if it works, I will send it to upstream. > > Thanks > > > > Yan-Hsuan
Re: [PATCH for v5.2] iwlwifi: mvm: disable TX-AMSDU on older NICs
On Wed, Jul 3, 2019 at 4:46 AM Kalle Valo wrote: > Luca Coelho writes: > > Hi Dave, > > > > This is an important fix for a bug that has been reported by several > > users in bugzilla (and elsewhere). It fixes FW crashes that disrupt > > throughput and connectivity in general in very popular devices (Intel's > > WiFi 7000 and 8000 series). > > > > I know it's a bit late for v5.2, but if possible, it would be great to > > take this. Kalle is on vacation, so we agreed that I would send it > > directly to you. > > Acked-by: Kalle Valo 5.2 has come and gone, and I believe Kalle has awoken from vacation. Can we get this for 5.3? Brian
Re: [PATCH] rtw88: make functions static
On Sun, Jul 14, 2019 at 7:24 PM Tony Chuang wrote: > I am sorry I have to NACK it. > > Nothing wrong about this patch. Because in the last patch set I sent has > 11 patches, but one of them is not applied by Kalle. > And I am going to resend it, which will use rtw_get_tx_power_params > in debug.c I suppose that's fine with me. I've marked it as Rejected in Patchwork. (Normally, I would say: it's your fault for leaving these things unused. But it was in the middle of the series, where you were aiming to start using it by the end.) It would still be nice to see the promised v2: https://patchwork.kernel.org/patch/10966403/ Regards, Brian
[PATCH] rtw88: make functions static
They're only used in phy.c.
Signed-off-by: Brian Norris
---
drivers/net/wireless/realtek/rtw88/phy.c | 13 ++---
drivers/net/wireless/realtek/rtw88/phy.h | 13 -
2 files changed, 10 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/phy.c
b/drivers/net/wireless/realtek/rtw88/phy.c
index 4ec8dcf17361..4bb36eba7080 100644
--- a/drivers/net/wireless/realtek/rtw88/phy.c
+++ b/drivers/net/wireless/realtek/rtw88/phy.c
@@ -140,7 +140,7 @@ void rtw_phy_init(struct rtw_dev *rtwdev)
dm_info->igi_history[0] = rtw_read32_mask(rtwdev, addr, mask);
}
-void rtw_phy_dig_write(struct rtw_dev *rtwdev, u8 igi)
+static void rtw_phy_dig_write(struct rtw_dev *rtwdev, u8 igi)
{
struct rtw_chip_info *chip = rtwdev->chip;
struct rtw_hal *hal = &rtwdev->hal;
@@ -1603,8 +1603,15 @@ static s8 rtw_phy_get_tx_power_limit(struct rtw_dev
*rtwdev, u8 band,
return (s8)rtwdev->chip->max_power_index;
}
-void rtw_get_tx_power_params(struct rtw_dev *rtwdev, u8 path, u8 rate, u8 bw,
-u8 ch, u8 regd, struct rtw_power_params *pwr_param)
+struct rtw_power_params {
+ u8 pwr_base;
+ s8 pwr_offset;
+ s8 pwr_limit;
+};
+
+static void rtw_get_tx_power_params(struct rtw_dev *rtwdev, u8 path, u8 rate,
+ u8 bw, u8 ch, u8 regd,
+ struct rtw_power_params *pwr_param)
{
struct rtw_hal *hal = &rtwdev->hal;
struct rtw_txpwr_idx *pwr_idx;
diff --git a/drivers/net/wireless/realtek/rtw88/phy.h
b/drivers/net/wireless/realtek/rtw88/phy.h
index 7c8eb732b13c..0f90ea24c6d7 100644
--- a/drivers/net/wireless/realtek/rtw88/phy.h
+++ b/drivers/net/wireless/realtek/rtw88/phy.h
@@ -103,19 +103,6 @@ static inline int rtw_check_supported_rfe(struct rtw_dev
*rtwdev)
return 0;
}
-void rtw_phy_dig_write(struct rtw_dev *rtwdev, u8 igi);
-
-struct rtw_power_params {
- u8 pwr_base;
- s8 pwr_offset;
- s8 pwr_limit;
-};
-
-void
-rtw_get_tx_power_params(struct rtw_dev *rtwdev, u8 path,
- u8 rate, u8 bw, u8 ch, u8 regd,
- struct rtw_power_params *pwr_param);
-
#defineMASKBYTE0 0xff
#defineMASKBYTE1 0xff00
#defineMASKBYTE2 0xff
--
2.22.0.510.g264f2c817a-goog
[RFC PATCH] rtw88: use txpwr_lmt_cfg_pair struct, not arrays
We're just trusting that these tables are of the right dimensions, when
we could do better by just using the struct directly. Let's expose the
struct txpwr_lmt_cfg_pair instead.
The table changes were made by using some Vim macros, so that should
help prevent any translation mistakes along the way.
Remaining work: get the 'void *data' out of the generic struct
rtw_table; all of these tables really deserve to be their own data
structure, with proper type fields.
Signed-off-by: Brian Norris
---
I think this patch works fine on its own, but it also seems desirable to
do more similar refactoring for the other tables. I'll leave it up to
Yan-Hsuan as to
(a) whether this seems like a decent refactoring and
(b) whether this should be taken further, to refactor some of the other
tables.
Hence the "RFC" label.
drivers/net/wireless/realtek/rtw88/phy.c | 15 +-
drivers/net/wireless/realtek/rtw88/phy.h |9 +
.../wireless/realtek/rtw88/rtw8822b_table.c | 1564 +++---
.../wireless/realtek/rtw88/rtw8822c_table.c | 2635 +++--
4 files changed, 2939 insertions(+), 1284 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/phy.c
b/drivers/net/wireless/realtek/rtw88/phy.c
index 4bb36eba7080..027b7b6fbc57 100644
--- a/drivers/net/wireless/realtek/rtw88/phy.c
+++ b/drivers/net/wireless/realtek/rtw88/phy.c
@@ -29,15 +29,6 @@ struct phy_pg_cfg_pair {
u32 data;
};
-struct txpwr_lmt_cfg_pair {
- u8 regd;
- u8 band;
- u8 bw;
- u8 rs;
- u8 ch;
- s8 txpwr_lmt;
-};
-
static const u32 db_invert_table[12][8] = {
{10,13, 16, 20,
25,32, 40, 50},
@@ -1267,10 +1258,8 @@ static void rtw_xref_txpwr_lmt(struct rtw_dev *rtwdev)
void rtw_parse_tbl_txpwr_lmt(struct rtw_dev *rtwdev,
const struct rtw_table *tbl)
{
- const struct txpwr_lmt_cfg_pair *p = tbl->data;
- const struct txpwr_lmt_cfg_pair *end = p + tbl->size / 6;
-
- BUILD_BUG_ON(sizeof(struct txpwr_lmt_cfg_pair) != sizeof(u8) * 6);
+ const struct rtw_txpwr_lmt_cfg_pair *p = tbl->data;
+ const struct rtw_txpwr_lmt_cfg_pair *end = p + tbl->size;
for (; p < end; p++) {
rtw_phy_set_tx_power_limit(rtwdev, p->regd, p->band,
diff --git a/drivers/net/wireless/realtek/rtw88/phy.h
b/drivers/net/wireless/realtek/rtw88/phy.h
index 0f90ea24c6d7..2748437f8e72 100644
--- a/drivers/net/wireless/realtek/rtw88/phy.h
+++ b/drivers/net/wireless/realtek/rtw88/phy.h
@@ -45,6 +45,15 @@ void rtw_phy_set_tx_power_level(struct rtw_dev *rtwdev, u8
channel);
void rtw_phy_tx_power_by_rate_config(struct rtw_hal *hal);
void rtw_phy_tx_power_limit_config(struct rtw_hal *hal);
+struct rtw_txpwr_lmt_cfg_pair {
+ u8 regd;
+ u8 band;
+ u8 bw;
+ u8 rs;
+ u8 ch;
+ s8 txpwr_lmt;
+};
+
#define RTW_DECL_TABLE_PHY_COND_CORE(name, cfg, path) \
const struct rtw_table name ## _tbl = {\
.data = name, \
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822b_table.c
b/drivers/net/wireless/realtek/rtw88/rtw8822b_table.c
index 2d2dfb495ce1..465f58411cab 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8822b_table.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8822b_table.c
@@ -20382,402 +20382,1182 @@ static const u32 rtw8822b_rf_b[] = {
RTW_DECL_TABLE_RF_RADIO(rtw8822b_rf_b, B);
-static const u8 rtw8822b_txpwr_lmt_type2[] = {
- 0, 0, 0, 0, 1, 32, 2, 0, 0, 0, 1, 28, 1, 0, 0, 0, 1, 30,
- 0, 0, 0, 0, 2, 32, 2, 0, 0, 0, 2, 28, 1, 0, 0, 0, 2, 30,
- 0, 0, 0, 0, 3, 32, 2, 0, 0, 0, 3, 28, 1, 0, 0, 0, 3, 30,
- 0, 0, 0, 0, 4, 32, 2, 0, 0, 0, 4, 28, 1, 0, 0, 0, 4, 30,
- 0, 0, 0, 0, 5, 32, 2, 0, 0, 0, 5, 28, 1, 0, 0, 0, 5, 30,
- 0, 0, 0, 0, 6, 32, 2, 0, 0, 0, 6, 28, 1, 0, 0, 0, 6, 30,
- 0, 0, 0, 0, 7, 32, 2, 0, 0, 0, 7, 28, 1, 0, 0, 0, 7, 30,
- 0, 0, 0, 0, 8, 32, 2, 0, 0, 0, 8, 28, 1, 0, 0, 0, 8, 30,
- 0, 0, 0, 0, 9, 32, 2, 0, 0, 0, 9, 28, 1, 0, 0, 0, 9, 30,
- 0, 0, 0, 0, 10, 32, 2, 0, 0, 0, 10, 28, 1, 0, 0, 0, 10, 30,
- 0, 0, 0, 0, 11, 32, 2, 0, 0, 0, 11, 28, 1, 0, 0, 0, 11, 30,
- 0, 0, 0, 0, 12, 26, 2, 0, 0, 0, 12, 28, 1, 0, 0, 0, 12, 30,
- 0, 0, 0, 0, 13, 20, 2, 0, 0, 0, 13, 28, 1, 0, 0, 0, 13, 28,
- 0, 0, 0, 0, 14, 63, 2, 0, 0, 0, 14, 63, 1, 0, 0, 0, 14, 32,
- 0, 0, 0, 1, 1, 26, 2, 0, 0, 1, 1, 30, 1, 0, 0, 1, 1, 34,
- 0, 0, 0, 1, 2, 30, 2, 0, 0, 1, 2, 30, 1, 0, 0, 1, 2, 34,
- 0, 0, 0, 1, 3, 32, 2, 0, 0, 1, 3, 30, 1, 0, 0, 1, 3, 34,
- 0, 0, 0, 1, 4, 34, 2, 0, 0, 1, 4, 30, 1, 0, 0, 1, 4, 34,
- 0, 0, 0, 1, 5, 34, 2, 0, 0, 1, 5, 30, 1, 0, 0, 1, 5, 34,
- 0, 0, 0, 1, 6, 34, 2, 0, 0, 1, 6, 30, 1, 0, 0, 1, 6, 34,
- 0, 0, 0, 1, 7, 34, 2, 0, 0, 1, 7, 30, 1, 0, 0, 1, 7, 34,
-
Re: [PATCH 09/11] rtw88: remove all RTW_MAX_POWER_INDEX macro
I realize this is already upstream, but I thought I'd ask here, since I was going back and reviewing some of this: On Wed, May 29, 2019 at 12:55 AM wrote: > > From: Tzu-En Huang > > Since this macro definition has different values in different chipset, > the current defined macro value is for 8822b. This will cause the > settings of 8822c be incorrect. ^^ Is this actually correct, that 8822b was correct and 8822c was wrong? Because I see RTW_MAX_POWER_INDEX used to be defined as 0x7f, and rtw8822c_hw_spec.max_power_index also appears to be 0x7f. Which would sound like 8822b (*b*, not c) was wrong, as it lists 0x3f. Anyway, I'm going to assume you got the change right, and you just mis-spoke in the description. Regards, Brian > Remove RTW_MAX_POWER_INDEX and use max_power_index in struct rtw_chip_info > to make sure the value of different chipset is right. > > Signed-off-by: Tzu-En Huang > Signed-off-by: Yan-Hsuan Chuang
Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element
Hi, On Thu, Jun 13, 2019 at 10:49 AM Brian Norris wrote: > "element_len + 2" would be much more readable as "total_ie_len". (Same for > several other usages in this patch.) I can send such a patch myself as a > follow-up I suppose. [...] > It seems like we should only be validating the standard pieces (e.g., up to > the > length/OUI), and only after an appropriate OUI match, *then* validating the > rest of > the vendor element (the pieces we'll use later). So I just decided to send some patches myself, for both of my notes: [PATCH 5.2 1/2] mwifiex: Don't abort on small, spec-compliant vendor IEs https://patchwork.kernel.org/patch/10996895/ [PATCH 2/2] mwifiex: use 'total_ie_len' in mwifiex_update_bss_desc_with_ie() https://patchwork.kernel.org/patch/10996893/ I'd appreciate your review. Regards, Brian
Re: [PATCH] mwifiex: avoid deleting uninitialized timer during USB cleanup
Hi Ganapathi,
This looks kinda wrong, but I'm not totally sure, as I'm not very familiar with
your USB driver.
On Wed, Jun 12, 2019 at 09:24:33PM +0530, Ganapathi Bhat wrote:
> Driver calls del_timer_sync(hold_timer), in unregister_dev(), but
> there exists is a case when the timer is yet to be initialized. A
> restructure of init and cleanup is needed to synchronize timer
> creation and delee. Make use of init_if() / cleanup_if() handlers
s/delee/delete/
> to get this done.
>
> Reported-by: syzbot+373e6719b49912399...@syzkaller.appspotmail.com
> Signed-off-by: Ganapathi Bhat
> ---
> drivers/net/wireless/marvell/mwifiex/usb.c | 32
> +++---
> 1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c
> b/drivers/net/wireless/marvell/mwifiex/usb.c
> index c2365ee..939f1e9 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -1348,6 +1348,8 @@ static void mwifiex_usb_cleanup_tx_aggr(struct
> mwifiex_adapter *adapter)
>
> for (idx = 0; idx < MWIFIEX_TX_DATA_PORT; idx++) {
> port = &card->port[idx];
> + if (!port->tx_data_ep)
> + continue;
It's not clear to me what this is about. Are you sure you're not just cleaning
stuff up in the wrong order?
> if (adapter->bus_aggr.enable)
> while ((skb_tmp =
> skb_dequeue(&port->tx_aggr.aggr_list)))
...
> @@ -1584,7 +1580,29 @@ static void mwifiex_usb_submit_rem_rx_urbs(struct
> mwifiex_adapter *adapter)
> return 0;
> }
>
> +static int mwifiex_init_usb(struct mwifiex_adapter *adapter)
> +{
> + struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
> + int ret = 0;
> +
> + if (card->usb_boot_state == USB8XXX_FW_DNLD)
> + return 0;
This looks wrong. You don't want to skip your basic initialization just because
firmware isn't loaded yet. In fact, init_if() always gets called before FW
init, so haven't you basically stubbed out this function most of the time?
I guess the question is: is this step supposed to go before, or after firmware
initilization? Based on that answer, we can make an appropriate patch.
(The original code does this after FW initialization, and now you're only sort
of moving it before.)
> +
> + ret = mwifiex_usb_rx_init(adapter);
> + if (!ret)
> + ret = mwifiex_usb_tx_init(adapter);
> +
> + return ret;
> +}
Brian
Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element
On Thu, Jun 13, 2019 at 11:38 AM Brian Norris wrote: > So, I might say: > > /* Vendor IEs must at least contain the OUI. */ > if (total_ie_len < offsetof(struct ieee80211_vendor_ie, oui_type)) > return -EINVAL; > > /* If the IE still isn't long enough, it's not a match. */ > if (element_len < sizeof(wpa_oui)) > continue; That would of course need to be break, not continue, to properly skip to the next IE. Brian
Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element
On Thu, Jun 13, 2019 at 08:12:39PM +0200, Takashi Iwai wrote: > On Thu, 13 Jun 2019 19:49:40 +0200, > Brian Norris wrote: > > On Wed, May 29, 2019 at 02:52:20PM +0200, Takashi Iwai wrote: > > > --- a/drivers/net/wireless/marvell/mwifiex/scan.c > > > +++ b/drivers/net/wireless/marvell/mwifiex/scan.c > > > @@ -1269,6 +1269,8 @@ int mwifiex_update_bss_desc_with_ie(struct > > > mwifiex_adapter *adapter, > > > break; > > > > > > case WLAN_EID_FH_PARAMS: > > > + if (element_len + 2 < sizeof(*fh_param_set)) > > > > "element_len + 2" would be much more readable as "total_ie_len". (Same for > > several other usages in this patch.) I can send such a patch myself as a > > follow-up I suppose. > > Yes, please. I'll wait until we straighten out the other (potentially) real bug. Don't want to make needless conflicts. > > > + return -EINVAL; > > > fh_param_set = > > > (struct ieee_types_fh_param_set *) current_ptr; > > > memcpy(&bss_entry->phy_param_set.fh_param_set, > > > > [...] > > > > > @@ -1349,6 +1361,9 @@ int mwifiex_update_bss_desc_with_ie(struct > > > mwifiex_adapter *adapter, > > > break; > > > > > > case WLAN_EID_VENDOR_SPECIFIC: > > > + if (element_len + 2 < sizeof(vendor_ie->vend_hdr)) > > > > Why 'sizeof(vendor_ie->vend_hdr)'? The (mwifiex-specific compare with the > > ieee80211.h generic struct ieee80211_vendor_ie) ieee_types_vendor_header > > struct > > includes the 'oui_subtype' and 'version' fields, which are not standard > > requirements for the vendor header (in fact, even the 4th byte of the > > OUI -- "oui_type" -- doesn't appear to be in the 802.11 specification). > > So it looks to me like you might be rejecting valid vendor headers (that > > we should just be skipping) that might have vendor-specific content with > > length 0 or 1 bytes. > > > > It seems like we should only be validating the standard pieces (e.g., up to > > the > > length/OUI), and only after an appropriate OUI match, *then* validating the > > rest of > > the vendor element (the pieces we'll use later). > > Hm, right, that looks too strict. Instead we need to check right > before both memcmp()'s of OUI. I think this is the right place for one check (the memcmp() is right after this line anyway) -- the minimum length just should be smaller. e.g.: sizeof(struct ieee80211_vendor_ie) // this is still technically 1 byte too large I think or offsetof(struct ieee80211_vendor_ie, oui_type) // not sure if this is the cleanest... If it's smaller than that, we can still say -EINVAL. Then, we can go with: if (element_len < sizeof(wpa_oui) continue; or similar. So, I might say: /* Vendor IEs must at least contain the OUI. */ if (total_ie_len < offsetof(struct ieee80211_vendor_ie, oui_type)) return -EINVAL; /* If the IE still isn't long enough, it's not a match. */ if (element_len < sizeof(wpa_oui)) continue; Brian
Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element
Hi Takashi, On Wed, May 29, 2019 at 02:52:20PM +0200, Takashi Iwai wrote: > Currently mwifiex_update_bss_desc_with_ie() implicitly assumes that > the source descriptor entries contain the enough size for each type > and performs copying without checking the source size. This may lead > to read over boundary. > > Fix this by putting the source size check in appropriate places. > > Signed-off-by: Takashi Iwai > --- > drivers/net/wireless/marvell/mwifiex/scan.c | 15 +++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c > b/drivers/net/wireless/marvell/mwifiex/scan.c > index 64ab6fe78c0d..c269a0de9413 100644 > --- a/drivers/net/wireless/marvell/mwifiex/scan.c > +++ b/drivers/net/wireless/marvell/mwifiex/scan.c > @@ -1269,6 +1269,8 @@ int mwifiex_update_bss_desc_with_ie(struct > mwifiex_adapter *adapter, > break; > > case WLAN_EID_FH_PARAMS: > + if (element_len + 2 < sizeof(*fh_param_set)) "element_len + 2" would be much more readable as "total_ie_len". (Same for several other usages in this patch.) I can send such a patch myself as a follow-up I suppose. > + return -EINVAL; > fh_param_set = > (struct ieee_types_fh_param_set *) current_ptr; > memcpy(&bss_entry->phy_param_set.fh_param_set, [...] > @@ -1349,6 +1361,9 @@ int mwifiex_update_bss_desc_with_ie(struct > mwifiex_adapter *adapter, > break; > > case WLAN_EID_VENDOR_SPECIFIC: > + if (element_len + 2 < sizeof(vendor_ie->vend_hdr)) Why 'sizeof(vendor_ie->vend_hdr)'? The (mwifiex-specific compare with the ieee80211.h generic struct ieee80211_vendor_ie) ieee_types_vendor_header struct includes the 'oui_subtype' and 'version' fields, which are not standard requirements for the vendor header (in fact, even the 4th byte of the OUI -- "oui_type" -- doesn't appear to be in the 802.11 specification). So it looks to me like you might be rejecting valid vendor headers (that we should just be skipping) that might have vendor-specific content with length 0 or 1 bytes. It seems like we should only be validating the standard pieces (e.g., up to the length/OUI), and only after an appropriate OUI match, *then* validating the rest of the vendor element (the pieces we'll use later). Brian > + return -EINVAL; > + > vendor_ie = (struct ieee_types_vendor_specific *) > current_ptr; > > -- > 2.16.4 >
Re: [PATCH 1/2] mwifiex: add support for host wakeup via PCIE wake#
(You really should re-send your patches, as they didn't make it to the list. But while you're here:) On Thu, May 30, 2019 at 3:01 AM Ganapathi Bhat wrote: > > From: Sharvari Harisangam > > > > PCIE WAKE# is asserted by firmware, when WoWLAN conditions are > > matched. Current driver does not enable PME bit needed for WAKE# > > assertion, causing host to remain in sleep even after WoWLAN conditions are > > matched. This commit fixes it by enabling wakeup (PME bit) in suspend > > handler. Are you sure said devices actually have their 'wakeup' attribute set to 'enabled' (e.g., in sysfs)? I think the PCI core should probably be taking care of this for you already. See below. > > Signed-off-by: Sharvari Harisangam > > Signed-off-by: Ganapathi Bhat > > --- > > drivers/net/wireless/marvell/mwifiex/pcie.c | 27 > > +++ > > 1 file changed, 15 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c > > b/drivers/net/wireless/marvell/mwifiex/pcie.c > > index 3fe81b2..0bd81d4 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c ... > > @@ -181,6 +180,10 @@ static int mwifiex_pcie_suspend(struct device *dev) > > set_bit(MWIFIEX_IS_SUSPENDED, &adapter->work_flags); > > clear_bit(MWIFIEX_IS_HS_ENABLING, &adapter->work_flags); > > > > + pci_enable_wake(pdev, pci_choose_state(pdev, state), 1); > > + pci_save_state(pdev); > > + pci_set_power_state(pdev, pci_choose_state(pdev, state)); >From Documentation/power/pci.txt: """ 3.1.2. suspend() ... This callback is expected to quiesce the device and prepare it to be put into a low-power state by the PCI subsystem. It is not required (in fact it even is not recommended) that a PCI driver's suspend() callback save the standard configuration registers of the device, prepare it for waking up the system, or put it into a low-power state. All of these operations can very well be taken care of by the PCI subsystem, without the driver's participation. However, in some rare case it is convenient to carry out these operations in a PCI driver. [...] """ I think you need to do a little more work to justify why you are one of those rare cases. On a related note: some of the existing "configure wakeup" stuff we do here should probably be gated behind a call to device_may_wakeup(). That would help make it more obvious that such configuration is futile, if the device was marked as wake-disabled. Brian > > + > > return 0; > > } > >
Re: [PATCH v9 04/14] rtw88: trx files
On Wed, May 1, 2019 at 11:30 AM Kalle Valo wrote: > My comment was about handling firmware commands and events as a byte > array, not about bitfields. So that instead of accessing 'index + 1' and > 'index + 4' you should create a proper struct for the command and access > it using 'cmd->foo' and 'cmd->bar'. Sure, bitfields you still need to > access using FIELD_GET() or similar but having a struct for commands is > a lot cleaner approach. And most upstream drivers do this: ath10k, > ath6kl, iwlwifi, p54 and whatnot. I think I pushed Tony away from the bitfields (he was using a struct plus some ugly bitfields / #ifdefs pre-v8), and he ended up with this (in v8). I noted on the v8 cover letter that one can still use a struct, but just avoid using bitfields -- so you would still have 'u8' and '__le32' fields (or similar), and do the right le32_to_cpu() accessors (sparse will help you) plus FIELD_GET() (for any necessary bitfields). Brian
Re: [PATCH v8 00/14] rtw88: mac80211 driver for Realtek 802.11ac wireless network chips
On Fri, Apr 19, 2019 at 1:23 AM Tony Chuang wrote: > > -Original Message- > > From: Kalle Valo [mailto:kv...@codeaurora.org] > > Please submit a new version with the new license. But don't any other > > changes than change the license and possibly remove the default line in > > Kconfig. > > > > Hi Kalle, > > I have sent PATCH v9 with license change and remove default y. > But for somehow reason the patches did not reach linux-wireless. > So I can not see them appear on the patchwork. > I do not know if there is anything wrong, should I resend? > > I am sure I've CCed linux-wireless, just checked by Ping-Ke. Checking with people who were CC'd is not a good choice -- that circumvents the mailing list. I didn't see it on Patchwork at first either, but now it's mostly there: https://patchwork.kernel.org/cover/10909117/ Also on the kernel.org archive: https://lkml.kernel.org/linux-wireless/1555653004-1795-1-git-send-email-yhchu...@realtek.com/ Maybe it was just delayed. Patch 14 is also still missing though. Brian
Re: Flag for detecting 802.11r Fast BSS Transition support
+ Johannes On Thu, Oct 04, 2018 at 12:06:50PM -0700, Matthew Wang wrote: > Hi, > > I'm wondering if there is a flag for detecting firmware/driver support > for FT. It seems like checking for SME support is a pretty good proxy > for this, but there is a non-mac80211 driver that can do FT as well > (ath/wil6210). It would be great if anyone knows of a feature flag > specifically for FT. I chatted with Johannes, and he agreed that there was no such flag today. It also sounded like he was open to adding one, even if it's several years too late. I don't think there's any useful way people could (generically) use FT support today. Brian
Re: [PATCH] mwifiex: add a bounds check in mwifiex_process_sta_rx_packet()
On Tue, Apr 2, 2019 at 12:03 AM Dan Carpenter wrote: > > Smatch complains that "local_rx_pd->priority" can't be trusted because > it comes from skb->data and it can go up to 255 instead of being capped > in the 0-7 range. A few lines earlier, on the other side of the if > statement, we cap priority so it seems harmless to add a bounds check > here as well. > > Signed-off-by: Dan Carpenter Seems right to me: Reviewed-by: Brian Norris
Re: [PATCH v8 00/14] rtw88: mac80211 driver for Realtek 802.11ac wireless network chips
On Tue, Mar 26, 2019 at 12:54 AM Stanislaw Gruszka wrote: > I also tested on RTL8822CE we generously got from Tony. I replaced wifi > card on my laptop by the 8822CE device. I have warning on resume from > suspend (see below, it could be mac80211 issue though). But network > connection was established after the warning and in general device > works stable for 2 days now. > > Tested-by: Stanislaw Gruszka > > [16158.826873] rtw_pci :02:00.0: start vif 00:e0:4c:09:94:0f on port 0 > [16158.827693] [ cut here ] > [16158.827694] wlan0: invalid CW_min/CW_max: 0/0 I didn't test system suspend/resume with this driver yet (oops), but FWIW I've seen reports of this warning on every other mac80211 driver I've supported. I don't think we've ever totally resolved them and I haven't looked closely into the reason for the WARNING. Brian
Re: [PATCH v8 00/14] rtw88: mac80211 driver for Realtek 802.11ac wireless network chips
Hi, On Wed, Mar 13, 2019 at 12:13:49PM +0800, yhchu...@realtek.com wrote: > From: Yan-Hsuan Chuang > > This is a new mac80211 driver for Realtek 802.11ac wireless network chips. > rtw88 now supports RTL8822BE/RTL8822CE now, with basic station mode > functionalities. The firmware for both can be found at linux-firmware. > > https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git > For RTL8822BE: rtw88/rtw8822b_fw.bin > For RTL8822CE: rtw88/rtw8822c_fw.bin > Potential problems: > * TRX statictics misleading, as we are not reporting status correctly, >or say, not reporting for "every" packet. By the way, I see earlier RFC versions had similar comments/questions from Stanislaw and Kalle that I had too. So at least Kalle didn't have a great answer either on what to do with hardware where it is inefficient to get ACK reports for everything :D But least you did add a little bit of comments this time, and fixed up some of the reporting. ... > v5 ... > - use macro instead of ugly struct layout with #ifdef __LITTLE_ENDIAN BTW, you technically could still use a struct there if you'd like, but removing the bitfield / __LITTLE_ENDIAN handling was nice, thank you. > - simplify efuse logical map parsing function Much better, thank you! ... > - enable DMA sync to avoid pci bus timeout ... > v8 > > - add prefix "rtw_" to *_rates and corresponding *_sizes, otherwise >the extern will confuse the linker there are multiple definitions >with brcmsmac driver I can't say I've closely reviewed every bit of this driver, but I did tackle significant portions of it. All my comments have been addressed, apart from one relatively insignificant piece that I re-commented on this version. So: Reviewed-by: Brian Norris I also gave 8822CE some moderate testing, and while I'm sure it's not perfect, it's at least stable, and it's in much better than when I tested approximately v2 (as Tony has rolled in a lot of fixes since then): Tested-by: Brian Norris
Re: [PATCH v8 03/14] rtw88: hci files
Hi,
On Wed, Mar 13, 2019 at 12:13:52PM +0800, yhchu...@realtek.com wrote:
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> b/drivers/net/wireless/realtek/rtw88/pci.c
> new file mode 100644
> index 000..cf3bffb
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -0,0 +1,1211 @@
...
> +static u8 ac_to_hwq[] = {
> + [0] = RTW_TX_QUEUE_VO,
> + [1] = RTW_TX_QUEUE_VI,
> + [2] = RTW_TX_QUEUE_BE,
> + [3] = RTW_TX_QUEUE_BK,
> +};
I don't want to dig up and repeat the details of my comments, but I
think you missed my earlier suggestion here: you should use the
ieee80211_ac_numbers enum instead of bare 0-3 indexing.
> +static u8 rtw_hw_queue_mapping(struct sk_buff *skb)
> +{
> + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> + __le16 fc = hdr->frame_control;
> + u8 q_mapping = skb_get_queue_mapping(skb);
> + u8 queue;
> +
> + if (unlikely(ieee80211_is_beacon(fc)))
> + queue = RTW_TX_QUEUE_BCN;
> + else if (unlikely(ieee80211_is_mgmt(fc) || ieee80211_is_ctl(fc)))
> + queue = RTW_TX_QUEUE_MGMT;
> + else
> + queue = ac_to_hwq[q_mapping];
It also seems wise to be defensive about the values of 'q_mapping', so
we don't accidentally overstep the array if for some reason the
implementation details of mac80211 change some day. e.g.:
else if (WARN_ON_ONCE(q_mapping >= ARRAY_SIZE(ac_to_hwq)))
// Pick a default.
Brian
> +
> + return queue;
> +}
> +
Re: [RFC v2 03/12] rtw88: hci files
+ Grant
I'm sorry (a little) to drudge up old revisions. But I happened to be
looking back at prior reviews, since I'm reviewing the latest, and I
realized I had this same (then-unresolved) confusion on later versions.
Maybe I should have read more before reviewing... Oh well. Comments
below:
On Mon, Oct 08, 2018 at 11:34:36AM +0200, Stanislaw Gruszka wrote:
> On Fri, Oct 05, 2018 at 12:07:06PM +, Tony Chuang wrote:
> > > > +static void rtw_pci_dma_check(struct rtw_dev *rtwdev,
> > > > + struct rtw_pci_rx_ring *rx_ring,
> > > > + u32 idx)
> > > > +{
> > > > + struct rtw_chip_info *chip = rtwdev->chip;
> > > > + struct rtw_pci_rx_buffer_desc *buf_desc;
> > > > + u32 desc_sz = chip->rx_buf_desc_sz;
> > > > + u16 total_pkt_size;
> > > > + int i;
> > > > +
> > > > + buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
> > > > +idx * desc_sz);
> > > > + for (i = 0; i < 20; i++) {
> > > > + total_pkt_size = le16_to_cpu(buf_desc->total_pkt_size);
> > > > + if (total_pkt_size)
> > > > + return;
> > > > + }
> > > > +
> > > > + if (i >= 20)
> > > > + rtw_warn(rtwdev, "pci bus timeout, drop packet\n");
> > > This is not right, most likely you need to use
Indeed, this was not right. Tony ended up removing it entirely later on,
based on my independent (sorry, didn't read this!) suggestion.
> > > dma_sync_single_for_cpu() .
> >
> > Not really understand how dma_sync_single_for_cpu() works.
> > Can you show me if possible?
>
> dma_sync_single_for_cpu() and dma_sync_single_for_device()
> transfer dma buffer ownership to respectivly CPU and device.
> It is well documented in:
> Documentation/DMA-API-HOWTO.txt:
> Documentation/DMA-API.txt
Yes, if we expect to need a sort of polling loop like this here, then it
would be important to flush any caches (and, as Grant noted later, it
would probably help to ensure some kind of consistent time deadline --
udelay(), or maybe just check against ktime_get()).
I'll also note here as I did on the later series: these rings are
allocated with consistent memory, so as long as the device is only
signalling this interrupt after it has completed writing to memory, then
we should be reading the latest copy and there should be no need for a
polling loop.
And Tony apparently agreed with me; the loop is gone in the latest, and
instead, we simply read once before emitting a warning.
Regards,
Brian
Re: [PATCH v4 03/13] rtw88: hci files
[Following up a little late on this one] Hi Grant and Tony, On Wed, Feb 20, 2019 at 11:19:10AM +, Tony Chuang wrote: > > -Original Message- > > From: Grant Grundler [mailto:grund...@chromium.org] > > This is another bug in the code actually: the code accessing tx > > descriptor rings should be declared volatile since they are updated by > > the device which is not visible to the compiler. Compiler won't > > optimize (or reorder) volatile code (by "volatile code" I mean code > > that is accessing data marked "volatile"). If I'm understanding the code correctly, the TX ISR assumes that everything between the last entry we processed (ring->r.rp) and a certain point (cur_rp) is complete. The former index was saved during the previous interrupt and the latter is determined via a register read. So assuming properly synchronized interrupts (such as with PCIe MSI) and no device-/firmware-related ordering bugs, then I think we're OK -- the only "volatile" piece is the register read, which already embeds volatile in the IO accessors. > This is fixed in PATCH v5. We reset and use rx tag to sync dma. > So the polling can be removed and the bus timeout has not happened again. Yes, I tested the later versions of this series, and I didn't see this problem so far. I'll follow up there with my Review/Test information eventually. Thanks, Brian
Re: [PATCH v2 00/24] rtw88: major fixes for 8822c to have stable functionalities
On Fri, Mar 8, 2019 at 2:40 AM Kalle Valo wrote: > Once everyone are happy about the driver, at least it needs a review > from Johannes and me but reviews from others is more than welcome. I > also saw that Brian gave good comments but not sure if he thinks the > driver is ready for upstream? I still have it on my plate to take another look-and-test at this, but I don't think that should stop you or Johannes from looking. At some point, if there aren't major structural or conceptual issues, it makes sense to merge and iterate. I'll try to get to it within the next week if not sooner. Regards, Brian
Re: [4.20 PATCH] Revert "mwifiex: restructure rx_reorder_tbl_lock usage"
Hi again Ganapathi,
By the way, I was a little curious about what went wrong here, so I dug
in a little further:
On Fri, Nov 30, 2018 at 09:59:57AM -0800, Brian Norris wrote:
> This reverts commit 5188d5453bc9380ccd4ae1086138dd485d13aef2, because it
> introduced lock recursion:
>
> BUG: spinlock recursion on CPU#2, kworker/u13:1/395
>lock: 0xffc0e28a47f0, .magic: dead4ead, .owner: kworker/u13:1/395,
> .owner_cpu: 2
> CPU: 2 PID: 395 Comm: kworker/u13:1 Not tainted 4.20.0-rc4+ #2
> Hardware name: Google Kevin (DT)
> Workqueue: MWIFIEX_RX_WORK_QUEUE mwifiex_rx_work_queue [mwifiex]
> Call trace:
>dump_backtrace+0x0/0x140
>show_stack+0x20/0x28
>dump_stack+0x84/0xa4
>spin_bug+0x98/0xa4
>do_raw_spin_lock+0x5c/0xdc
>_raw_spin_lock_irqsave+0x38/0x48
>mwifiex_flush_data+0x2c/0xa4 [mwifiex]
>call_timer_fn+0xcc/0x1c4
>run_timer_softirq+0x264/0x4f0
>__do_softirq+0x1a8/0x35c
>do_softirq+0x54/0x64
>netif_rx_ni+0xe8/0x120
>mwifiex_recv_packet+0xfc/0x10c [mwifiex]
>mwifiex_process_rx_packet+0x1d4/0x238 [mwifiex]
>mwifiex_11n_dispatch_pkt+0x190/0x1ac [mwifiex]
>mwifiex_11n_rx_reorder_pkt+0x28c/0x354 [mwifiex]
TL;DR: the problem was right here ^^^
where you started running mwifiex_11n_dispatch_pkt() (via
mwifiex_11n_scan_and_dispatch()) while holding a spinlock.
When you do that, you eventually call netif_rx_ni(), which specifically
defers to softirq contexts. Then, if you happen to have your flush timer
expiring just before that, you end up in mwifiex_flush_data(), which
also needs that spinlock.
There are a few possible ways to handle this:
(a) prevent processing softirqs in that context; e.g., with
local_bh_disable(). This seems somewhat of a hack.
(Side note: I think most of the locks in this driver really could be
spin_lock_bh(), not spin_lock_irqsave() -- we don't really care
about hardirq context for 99% of these locks.)
(b) restructure so that packet processing (e.g., netif_rx_ni()) is done
outside of the spinlock.
It's actually not that hard to do (b). You can just queue your skb's up
in a temporary sk_buff_head list and process them all at once after
you've finished processing the reorder table. I have a local patch to do
this, and I might send it your way if I can give it a bit more testing.
Brian
>mwifiex_process_sta_rx_packet+0x204/0x26c [mwifiex]
>mwifiex_handle_rx_packet+0x15c/0x16c [mwifiex]
>mwifiex_rx_work_queue+0x104/0x134 [mwifiex]
>worker_thread+0x4cc/0x72c
>kthread+0x134/0x13c
>ret_from_fork+0x10/0x18
>
> This was clearly not tested well at all. I simply performed 'wget' in a
> loop and it fell over within a few seconds.
>
> Fixes: 5188d5453bc9 ("mwifiex: restructure rx_reorder_tbl_lock usage")
> Cc:
> Cc: Ganapathi Bhat
> Signed-off-by: Brian Norris
Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
Hi Rafael, On Wed, Feb 27, 2019 at 3:04 PM Rafael J. Wysocki wrote: > On Wed, Feb 27, 2019 at 9:58 PM Brian Norris wrote: > > On Wed, Feb 27, 2019 at 11:16:12AM +0100, Ard Biesheuvel wrote: > > > So I'd argue that we should add an optional 'wake-gpio' DT property > > > instead to the generic PCI device binding, and leave the interrupt > > > binding and discovery alone. > > > > So I think Mark Rutland already shot that one down; it's conceptually an > > interrupt from the device's perspective. Perhaps I shouldn't speak for Mark, but I am basically quoting him off IRC. > Which device are you talking about? The one that signals wakeup? If > so, then I beg to differ. Yes, the endpoint device. > On ACPI platforms WAKE# is represented as an ACPI GPE that is signaled > through SCI and handled at a different level (on HW-reduced ACPI it > actually can be a GPIO interrupt, but it still is handled with the > help of AML). The driver of the device signaling wakeup need not even > be aware that WAKE# has been asserted. Frankly, ACPI is not relevant to how we represent WAKE# in DT, IMO. Also, we're talking about the *device*, not the driver. When talking about Device Tree, that distinction is relevant. So while the driver need not be aware (and I agree! it only needs to care about enabling/disabling wake), *something* should be aware, and the signal that "something" should be receiving is simply "did WAKE happen"? That sounds basically like the device is signalling an interrupt to me. Maybe this goes back to some confusion we had elsewhere: what is the meaning of "interrupt" in device tree? > > We just need to figure out a good way of representing it that doesn't stomp > > on the existing INTx > > definitions. > > WAKE# is a signal that is converted into an interrupt, but that > interrupt may arrive at some place your driver has nothing to do with. I could agree with that, perhaps. But that's also what Device Tree is all about, really. We describe the relation between devices. So some other handles events that are triggered by , so we use a phandle to relate to . > It generally doesn't make sense to represent it as an interrupt for > the target device. What would you suggest then? I'm not clearly understanding how you think we should (a) describe (in DT) and (b) implement this WAKE# handling. Brian
Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
Hi Ard, On Wed, Feb 27, 2019 at 11:16:12AM +0100, Ard Biesheuvel wrote: > On Wed, 27 Feb 2019 at 11:02, Marc Zyngier wrote: > > On 26/02/2019 23:28, Brian Norris wrote: > > > You're not the first person to notice this. All the motivations are not > > > necessarily painted clearly in their cover letter, but here are some > > > previous attempts at solving this problem: > > > > > > [RFC PATCH v11 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core > > > https://lkml.kernel.org/lkml/20171225114742.18920-1-jeffy.c...@rock-chips.com/ > > > http://lkml.kernel.org/lkml/20171226023646.17722-1-jeffy.c...@rock-chips.com/ > > > > > > As you can see by the 12th iteration, it wasn't left unsolved for lack > > > of trying... > > > > I wasn't aware of this. That's definitely a better approach than my > > hack, and I would really like this to be revived. > > > > I don't think this approach is entirely sound either. (I'm sure there may be problems with the above series. We probably should give it another shot though someday, as I think it's closer to the mark.) > From the side of the PCI device, WAKE# is just a GPIO line, and how it > is wired into the system is an entirely separate matter. So I don't > think it is justified to overload the notion of legacy interrupts with > some other pin that may behave in a way that is vaguely similar to how > a true wake-up capable interrupt works. I think you've conflated INTx with WAKE# just a bit (and to be fair, that's exactly what the bad binding we're trying to replace did, accidentally). We're not trying to claim this WAKE# signal replaces the typical PCI interrupts, but it *is* an interrupt in some sense -- "depending on your definition of interrupt", per our IRC conversation ;) > So I'd argue that we should add an optional 'wake-gpio' DT property > instead to the generic PCI device binding, and leave the interrupt > binding and discovery alone. So I think Mark Rutland already shot that one down; it's conceptually an interrupt from the device's perspective. We just need to figure out a good way of representing it that doesn't stomp on the existing INTx definitions. Brian
Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
Hi Marc,
On Wed, Feb 27, 2019 at 10:02:16AM +, Marc Zyngier wrote:
> On 26/02/2019 23:28, Brian Norris wrote:
> > On Sun, Feb 24, 2019 at 02:04:22PM +, Marc Zyngier wrote:
> >> Note how the interrupt is part of the properties directly attached to the
> >> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
> >> interrupt, as it is attached to the wake-up widget that bypasses the PCIe
> >> RC
> >> altogether (Yay for the broken design!). This is in total violation of the
> >> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
> >> specifiers describe the PCI device interrupts, and must obey the
> >> INT-{A,B,C,D} mapping. Oops!
> >
> > You're not the first person to notice this. All the motivations are not
> > necessarily painted clearly in their cover letter, but here are some
> > previous attempts at solving this problem:
> >
> > [RFC PATCH v11 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core
> > https://lkml.kernel.org/lkml/20171225114742.18920-1-jeffy.c...@rock-chips.com/
> > http://lkml.kernel.org/lkml/20171226023646.17722-1-jeffy.c...@rock-chips.com/
> >
> > As you can see by the 12th iteration, it wasn't left unsolved for lack
> > of trying...
>
> I wasn't aware of this. That's definitely a better approach than my
> hack, and I would really like this to be revived.
Well, in some respects it may be better (mostly, handling in the PCI
core rather than each driver). But I'm still unsure about the DT
binding.
And while perhaps I could find time to revive it, it's probably more
expedient to kill the bad binding first.
> > Frankly, if a proper DT replacement to the admittedly bad binding isn't
> > agreed upon quickly, I'd be very happy to just have WAKE# support
> > removed from the DTS for now, and the existing mwifiex binding should
> > just be removed. (Wake-on-WiFi was never properly vetted on these
> > platforms anyway.) It mostly serves to just cause problems like you've
> > noticed.
>
> Agreed. If there is no actual use for this, and that we can build a case
> for a better solution, let's remove the wakeup support from the Gru DT
> (it is invalid anyway), and bring it back if and when we get the right
> level of support.
+1
Today, something simple like NL80211_WOWLAN_TRIG_DISCONNECT and
NL80211_WOWLAN_TRIG_NET_DETECT may work OK, but I'm not confident that
anything more complicated is really a compelling story today (well,
outside of Android, which has a massively more complicated--and not
upstream--setup for this stuff).
> [...]
>
> > One problem Rockchip authors were also trying to resolve here is that
> > PCIe WAKE# handling should not really be something the PCI device driver
> > has to handle directly. Despite your complaints about not using in-band
> > TLP wakeup, a separate WAKE# pin is in fact a documented part of the
> > PCIe standard, and it so happens that the Rockchip RC does not support
> > handling TLPs in S3, if you want to have decent power consumption. (Your
> > "bad hardware" complaints could justifiably fall here, I suppose.)
> >
> > Additionally, I've had pushback from PCI driver authors/maintainers on
> > adding more special handling for this sort of interrupt property (not
> > the binding specifically, but just the concept of handling WAKE# in the
> > driver), as they claim this should be handled by the system firmware,
> > when they set the appropriate wakeup flags, which filter down to
> > __pci_enable_wake() -> platform_pci_set_wakeup(). That's how x86 systems
> > do it (note: I know for a fact that many have a very similar
> > architecture -- WAKE# is not routed to the RC, because, why does it need
> > to? and they *don't* use TLP wakeup either -- they just hide it in
> > firmware better), and it Just Works.
>
> Even on an arm64 platform, there is no reason why a wakeup interrupt
> couldn't be handled by FW rather than the OS. It just need to be wired
> to the right spot (so that it generates a secure interrupt that can be
> handled by FW).
True...but then you also need a configuration (enable/disable) API for
it too. I don't think we have such a per-device API? So it would be a
pretty similar problem to solve anyway.
> > So, we basically concluded that we should standardize on a way to
> > describe WAKE# interrupts such that PCI drivers don't have to deal with
> > it at all, and the PCI core can do it for us. 12 revisions later
> > and...we still never agreed on a good device tree binding for this.
>
> Is the DT binding the only proble
Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
Hi, On Tue, Feb 26, 2019 at 05:14:00PM +, Marc Zyngier wrote: > On 26/02/2019 16:21, Ard Biesheuvel wrote: > > On Mon, 25 Feb 2019 at 15:53, Marc Zyngier wrote: > >> It outlines one thing: If you have to interpret per-device PCI > >> properties from DT, you're in for serious trouble. I should get some > >> better HW. > >> > > > > Yeah, it obviously makes no sense at all for the interrupt parent of a > > PCI device to deviate from the host bridge's interrupt parent, and > > it's quite unfortunate that we can't simply ban it now that the cat is > > out of the bag already. > > > > Arguably, the wake up widget is not part of the PCI device, but I have > > no opinion as to whether it is better modeling it as a sub device as > > you are proposing or as an entirely separate device referenced via a > > phandle. > > It is not that clear. The widget seems to be an integral part of the > device, as it is the same basic IP that is used for SDIO and USB. It's not really a widget specific to this IP. It's just a GPIO. It so happens that both SDIO and PCIe designs have wanted to use a GPIO for wakeup, as many other devices do. (Note: it's not just cheap ARM devices; pulling up some Intel Chromebook designs, I see the exact same WAKE# GPIO on their PCIe WiFi as well.) > It looks like the good old pre-PCI-2.2 days, where you had to have a > separate cable between your network card and the base-board for the > wake-up interrupt to be delivered. Starting with PCI-2.2, the bus can > carry the signal just fine. With PCIe, it should just be an interrupt > TLP sent to the RC, but that's obviously not within the capabilities of > the HW. You should search the PCI Express specification for WAKE#. There is a clearly-documented "side-band wake" feature that is part of the standard, as an alternative to in-band TLP wakeup. While you claim this is an ancient thing, it in fact still in use on many systems -- it's just usually abstracted better by ACPI firmware, whereas the dirty laundry is aired a bit more on a Device Tree system. And we got it wrong. > Anyway, it'd be good if the Marvell people could chime in and let us > know how they'd prefer to handle this. I'm not sure this is really a Marvell-specific problem. (Well, except for the marvell,wakeup-pin silliness, which is somewhat orthogonal.) In fact, if we cared a little more about Wake-on-WiFi, we'd be trying to support the same (out-of-band WAKE#) with other WiFi drivers. Brian
Re: [PATCH 3/4] mwifiex: Flag wake-up interrupt as IRQ_NOAUTOEN rather than disabling it too late
On Tue, Feb 26, 2019 at 03:31:31PM -0800, Brian Norris wrote:
> Hi Marc,
>
> On Sun, Feb 24, 2019 at 02:04:25PM +, Marc Zyngier wrote:
> > The mwifiex driver makes unsafe assumptions about the state of the
> > wake-up interrupt. It requests it and only then disable it. Of
> > course, the interrupt may be screaming for whatever reason at that
> > time, and the handler will then be called without the interrupt
> > having been registered with the PM/wakeup subsystem. Oops.
> >
> > The right way to handle this kind of situation is to flag the
> > interrupt with IRQ_NOAUTOEN before requesting it. It will then
> > stay disabled until someone (the wake-up subsystem) enables it.
> >
> > Signed-off-by: Marc Zyngier
>
> This could be:
>
> Fixes: 853402a00823 ("mwifiex: Enable WoWLAN for both sdio and pcie")
Also, this comes after a different change (that's not quite as clearly a
backport-able bugfix). Perhaps it should go first in the series?
Brian
> although, that was just borrowing the existing antipattern from SDIO
> code..
>
> Reviewed-by: Brian Norris
>
> Thanks.
>
> > ---
> > drivers/net/wireless/marvell/mwifiex/main.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c
> > b/drivers/net/wireless/marvell/mwifiex/main.c
> > index 2105c2b7c627..82cf35e50579 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -1610,6 +1610,7 @@ static void mwifiex_probe_of(struct mwifiex_adapter
> > *adapter)
> > "wake-up interrupt outside 'wake-up' subnode of
> > %pOF\n",
> > adapter->dt_node);
> >
> > + irq_set_status_flags(adapter->irq_wakeup, IRQ_NOAUTOEN);
> > ret = devm_request_irq(dev, adapter->irq_wakeup,
> >mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
> >"wifi_wake", adapter);
> > @@ -1619,7 +1620,6 @@ static void mwifiex_probe_of(struct mwifiex_adapter
> > *adapter)
> > goto err_exit;
> > }
> >
> > - disable_irq(adapter->irq_wakeup);
> > if (device_init_wakeup(dev, true)) {
> > dev_err(dev, "fail to init wakeup for mwifiex\n");
> > goto err_exit;
> > --
> > 2.20.1
> >
Re: [PATCH 3/4] mwifiex: Flag wake-up interrupt as IRQ_NOAUTOEN rather than disabling it too late
Hi Marc,
On Sun, Feb 24, 2019 at 02:04:25PM +, Marc Zyngier wrote:
> The mwifiex driver makes unsafe assumptions about the state of the
> wake-up interrupt. It requests it and only then disable it. Of
> course, the interrupt may be screaming for whatever reason at that
> time, and the handler will then be called without the interrupt
> having been registered with the PM/wakeup subsystem. Oops.
>
> The right way to handle this kind of situation is to flag the
> interrupt with IRQ_NOAUTOEN before requesting it. It will then
> stay disabled until someone (the wake-up subsystem) enables it.
>
> Signed-off-by: Marc Zyngier
This could be:
Fixes: 853402a00823 ("mwifiex: Enable WoWLAN for both sdio and pcie")
although, that was just borrowing the existing antipattern from SDIO
code..
Reviewed-by: Brian Norris
Thanks.
> ---
> drivers/net/wireless/marvell/mwifiex/main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index 2105c2b7c627..82cf35e50579 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1610,6 +1610,7 @@ static void mwifiex_probe_of(struct mwifiex_adapter
> *adapter)
>"wake-up interrupt outside 'wake-up' subnode of
> %pOF\n",
>adapter->dt_node);
>
> + irq_set_status_flags(adapter->irq_wakeup, IRQ_NOAUTOEN);
> ret = devm_request_irq(dev, adapter->irq_wakeup,
> mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
> "wifi_wake", adapter);
> @@ -1619,7 +1620,6 @@ static void mwifiex_probe_of(struct mwifiex_adapter
> *adapter)
> goto err_exit;
> }
>
> - disable_irq(adapter->irq_wakeup);
> if (device_init_wakeup(dev, true)) {
> dev_err(dev, "fail to init wakeup for mwifiex\n");
> goto err_exit;
> --
> 2.20.1
>
Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
+ others
Hi Marc,
Thanks for the series. I have a few bits of history to add to this, and
some comments.
On Sun, Feb 24, 2019 at 02:04:22PM +, Marc Zyngier wrote:
> For quite some time, I wondered why the PCI mwifiex device built in my
> Chromebook was unable to use the good old legacy interrupts. But as MSIs
> were working fine, I never really bothered investigating. I finally had a
> look, and the result isn't very pretty.
>
> On this machine (rk3399-based kevin), the wake-up interrupt is described as
> such:
>
> &pci_rootport {
> mvl_wifi: wifi@0,0 {
> compatible = "pci1b4b,2b42";
> reg = <0x8301 0x0 0x 0x0 0x0010
> 0x8301 0x0 0x0010 0x0 0x0010>;
> interrupt-parent = <&gpio0>;
> interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> pinctrl-names = "default";
> pinctrl-0 = <&wlan_host_wake_l>;
> wakeup-source;
> };
> };
>
> Note how the interrupt is part of the properties directly attached to the
> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
> interrupt, as it is attached to the wake-up widget that bypasses the PCIe RC
> altogether (Yay for the broken design!). This is in total violation of the
> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
> specifiers describe the PCI device interrupts, and must obey the
> INT-{A,B,C,D} mapping. Oops!
You're not the first person to notice this. All the motivations are not
necessarily painted clearly in their cover letter, but here are some
previous attempts at solving this problem:
[RFC PATCH v11 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core
https://lkml.kernel.org/lkml/20171225114742.18920-1-jeffy.c...@rock-chips.com/
http://lkml.kernel.org/lkml/20171226023646.17722-1-jeffy.c...@rock-chips.com/
As you can see by the 12th iteration, it wasn't left unsolved for lack
of trying...
Frankly, if a proper DT replacement to the admittedly bad binding isn't
agreed upon quickly, I'd be very happy to just have WAKE# support
removed from the DTS for now, and the existing mwifiex binding should
just be removed. (Wake-on-WiFi was never properly vetted on these
platforms anyway.) It mostly serves to just cause problems like you've
noticed.
> The net effect of the above is that Linux tries to do something vaguely
> sensible, and uses the same interrupt for both the wake-up widget and the
> PCI device. This doesn't work for two reasons: (1) the wake-up widget grabs
> the interrupt in exclusive mode, and (2) the PCI interrupt is still routed
> to the RC, leading to a screaming interrupt. This simply cannot work.
>
> To sort out this mess, we need to lift the confusion between the two
> interrupts. This is done by extending the DT binding to allow the wake-up
> interrupt to be described in a 'wake-up' subnode, sidestepping the issue
> completely. On my Chromebook, it now looks like this:
>
> &pci_rootport {
> mvl_wifi: wifi@0,0 {
> compatible = "pci1b4b,2b42";
> reg = <0x8301 0x0 0x 0x0 0x0010
> 0x8301 0x0 0x0010 0x0 0x0010>;
> pinctrl-names = "default";
> pinctrl-0 = <&wlan_host_wake_l>;
> wake-up {
> interrupt-parent = <&gpio0>;
> interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> wakeup-source;
> };
> };
> };
One problem Rockchip authors were also trying to resolve here is that
PCIe WAKE# handling should not really be something the PCI device driver
has to handle directly. Despite your complaints about not using in-band
TLP wakeup, a separate WAKE# pin is in fact a documented part of the
PCIe standard, and it so happens that the Rockchip RC does not support
handling TLPs in S3, if you want to have decent power consumption. (Your
"bad hardware" complaints could justifiably fall here, I suppose.)
Additionally, I've had pushback from PCI driver authors/maintainers on
adding more special handling for this sort of interrupt property (not
the binding specifically, but just the concept of handling WAKE# in the
driver), as they claim this should be handled by the system firmware,
when they set the appropriate wakeup flags, which filter down to
__pci_enable_wake() -> platform_pci_set_wakeup(). That's how x86 systems
do it (note: I know for a fact that many have a very similar
architecture -- WAKE# is not routed to the RC, because, why does it need
to? and they *don't* use TLP wakeup either -- they just hide it in
firmware better), and it Just Works.
So, we basically concluded that we should standardize on a way to
describe WAKE# interrupts such that PCI drivers don't have to deal with
it at all, and the PCI core can do it for us. 12 revisions later
and...we still never agreed on a good device tree binding for this.
IOW, maybe your wake-up sub-node is the best way to side-
[PATCH] mwifiex: don't advertise IBSS features without FW support
As it is, doing something like
# iw phy phy0 interface add foobar type ibss
on a firmware that doesn't have ad-hoc support just yields failures of
HostCmd_CMD_SET_BSS_MODE, which happened to return a '-1' error code
(-EPERM? not really right...) and sometimes may even crash the firmware
along the way.
Let's parse the firmware capability flag while registering the wiphy, so
we don't allow attempting IBSS at all, and we get a proper -EOPNOTSUPP
from nl80211 instead.
Fixes: e267e71e68ae ("mwifiex: Disable adhoc feature based on firmware
capability")
Signed-off-by: Brian Norris
---
drivers/net/wireless/marvell/mwifiex/cfg80211.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 1467af22e394..883752f640b4 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -4310,11 +4310,13 @@ int mwifiex_register_cfg80211(struct mwifiex_adapter
*adapter)
wiphy->mgmt_stypes = mwifiex_mgmt_stypes;
wiphy->max_remain_on_channel_duration = 5000;
wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) |
-BIT(NL80211_IFTYPE_ADHOC) |
BIT(NL80211_IFTYPE_P2P_CLIENT) |
BIT(NL80211_IFTYPE_P2P_GO) |
BIT(NL80211_IFTYPE_AP);
+ if (ISSUPP_ADHOC_ENABLED(adapter->fw_cap_info))
+ wiphy->interface_modes |= BIT(NL80211_IFTYPE_ADHOC);
+
wiphy->bands[NL80211_BAND_2GHZ] = &mwifiex_band_2ghz;
if (adapter->config_bands & BAND_A)
wiphy->bands[NL80211_BAND_5GHZ] = &mwifiex_band_5ghz;
@@ -4374,11 +4376,13 @@ int mwifiex_register_cfg80211(struct mwifiex_adapter
*adapter)
wiphy->available_antennas_tx = BIT(adapter->number_of_antenna) - 1;
wiphy->available_antennas_rx = BIT(adapter->number_of_antenna) - 1;
- wiphy->features |= NL80211_FEATURE_HT_IBSS |
- NL80211_FEATURE_INACTIVITY_TIMER |
+ wiphy->features |= NL80211_FEATURE_INACTIVITY_TIMER |
NL80211_FEATURE_LOW_PRIORITY_SCAN |
NL80211_FEATURE_NEED_OBSS_SCAN;
+ if (ISSUPP_ADHOC_ENABLED(adapter->fw_cap_info))
+ wiphy->features |= NL80211_FEATURE_HT_IBSS;
+
if (ISSUPP_RANDOM_MAC(adapter->fw_cap_info))
wiphy->features |= NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR |
NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR |
--
2.21.0.rc0.258.g878e2cd30e-goog
Re: [PATCH v4 03/13] rtw88: hci files
On Wed, Feb 13, 2019 at 3:08 AM Tony Chuang wrote: > > On Behalf Of Tony Chuang > > > From: Brian Norris [mailto:briannor...@chromium.org] > > > Do you have some kind of memory mapping/ordering issue or something? I > > > wouldn't think you should expect to just drop packets on the floor so > > > often like this. > > > > > > > Looks like the cause is that the DMA might not have done when the interrupt > > arrived. Need to do more test to figure it out. > > > I tested and checked again. I found that I didn't enable DMA sync for > the TRX path. After some work it can be resolved if I turn DMA sync on. > So I will include this in the PATCH v5. Great! I'd also highly recommend dropping that loop. It just pretends to do something that it doesn't really do. Leaving a single iteration as a sort of debugging check is OK, if it's truly a bug any time we fail it. Regards, Brian
Re: [PATCH v4 03/13] rtw88: hci files
On Mon, Feb 11, 2019 at 10:19 PM Tony Chuang wrote:
> > From: Brian Norris [mailto:briannor...@chromium.org]
> > On Wed, Jan 30, 2019 at 12:02:10PM +0800, yhchu...@realtek.com wrote:
> > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> > b/drivers/net/wireless/realtek/rtw88/pci.c
> > > new file mode 100644
> > > index 000..ef3c9bb
> > > --- /dev/null
> > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > > @@ -0,0 +1,1210 @@
> >
> > ...
> >
> > > +static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
> > > + struct rtw_pci_rx_ring *rx_ring,
> > > + u8 desc_size, u32 len)
> > > +{
> > > + struct pci_dev *pdev = to_pci_dev(rtwdev->dev);
> > > + struct sk_buff *skb = NULL;
> > > + dma_addr_t dma;
> > > + u8 *head;
> > > + int ring_sz = desc_size * len;
> > > + int buf_sz = RTK_PCI_RX_BUF_SIZE;
> > > + int i, allocated;
> > > + int ret = 0;
> > > +
> > > + head = pci_zalloc_consistent(pdev, ring_sz, &dma);
> > > + if (!head) {
> > > + rtw_err(rtwdev, "failed to allocate rx ring\n");
> > > + return -ENOMEM;
> > > + }
> > > + rx_ring->r.head = head;
> > > +
> > > + for (i = 0; i < len; i++) {
> > > + skb = dev_alloc_skb(buf_sz);
> > > + if (!skb) {
> > > + allocated = i;
> > > + ret = -ENOMEM;
> > > + goto err_out;
> > > + }
> > > +
> > > + memset(skb->data, 0, buf_sz);
> > > + rx_ring->buf[i] = skb;
> > > + ret = rtw_pci_reset_rx_desc(rtwdev, skb, rx_ring, i,
> > > desc_size);
> > > + if (ret) {
> > > + allocated = i;
> > > + goto err_out;
> > > + }
> > > + }
> > > +
> > > + rx_ring->r.dma = dma;
> > > + rx_ring->r.len = len;
> > > + rx_ring->r.desc_size = desc_size;
> > > + rx_ring->r.wp = 0;
> > > + rx_ring->r.rp = 0;
> > > +
> > > + return 0;
> > > +
> > > +err_out:
> > > + dev_kfree_skb_any(skb);
> >
> > Maybe I'm misreading but...shouldn't this line not be here? You properly
> > iterate over the allocated SKBs below, and this looks like you're just
> > going to be double-freeing (or, negative ref-counting).
>
> Yeah, I think I should move this line above after reset_rx_desc failed.
Ah yes, so it's not technically incorrect, it's just a bit strange to
read. Yes, moving it up to the failure location would make it clearer
IMO. Thanks.
> > > + for (i = 0; i < allocated; i++) {
> > > + skb = rx_ring->buf[i];
> > > + if (!skb)
> > > + continue;
> > > + dma = *((dma_addr_t *)skb->cb);
> > > + pci_unmap_single(pdev, dma, buf_sz, PCI_DMA_FROMDEVICE);
> > > + dev_kfree_skb_any(skb);
> > > + rx_ring->buf[i] = NULL;
> > > + }
> > > + pci_free_consistent(pdev, ring_sz, head, dma);
> > > +
> > > + rtw_err(rtwdev, "failed to init rx buffer\n");
> > > +
> > > + return ret;
> > > +}
> > > +
...
> After some testing, this function I think can be removed.
> (rtw_pci_parse_configuration)
Why was it here in the first place? Were there any hardware quirks
that this was covering? Or, are you just saying that the default
values are already correct (plus, the PCI framework already brings you
out of D3)? I do also see that removing this function doesn't actually
have a net effect on the the PCI configuration, judging by lspci
-vvv.
Brian
Re: [PATCH 01/24] rtw88: report correct tx status if mac80211 requested one
On Sun, Feb 10, 2019 at 8:31 PM Tony Chuang wrote: > To fix `iw wlan0 station dump` display, I think I can just restore one line > in pci.c. That is, restore IEEE80211_TX_STAT_ACK flag line: > > + continue; > + } > ieee80211_tx_info_clear_status(info); > - info->flags |= IEEE80211_TX_STAT_ACK; > ieee80211_tx_status_irqsafe(hw, skb) > > And with some modifications, such as IEEE80211_TX_CTL_NO_ACK check. > Then we can better reporting ACK status for data frames without > IEEE80211_TX_CTL_REQ_TX_STATUS. This way we can also ensure the > connection monitor can work. (but it will be no loss) This seems like a small improvement. It means you will almost never actually report a "drop", but that's still probably better than *always* reporting drops I think. I also see that there are handful of older (likely poorly-maintained? or perhaps similarly-constrained) drivers that have a similar default behavior -- they report ACK by default, apparently without any particular notice that the packet was actually ACKed by the receiver. But I'm not really a mac80211 expert, so someone else may have better ideas here. > It's not buggy I think, if firmware is not reporting status, something must > go wrong. And after some test I know why you feel it's unreliable. > > For WOW implementation, we modified a lot in fw.c functions. > And correct some driver-firmware interface behaviors. To make sure the > firmware is running as expected. But the patches are still holding in my hand. > I can attach them in this patch set, and apparently I should. I will separate > them out of WOW patch set and resend again. Ah, well I don't know at what point we should cut things off, but I'll stick to my stated opinion from previously: all bugfixes will help someone like me. It's tough to differentiate firmware bugs from driver bugs, especially when I have absolutely no firmware documentation :) BTW, while we're at it: is this still a reasonable firmware to reference? https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/commit/?id=338684a0c7760644031483311464c7cf5b3aac94 rtw88: Add firmware file for driver rtw88 > I think we should keep this feature. Because we actually can report status, > despite not for every packet. The only problem is when we use `iw wlan0 > station dump` we could get *no* packet loss (like I've mentioned above, > report TX_STAT_ACK for every other packets not have > IEEE80211_TX_CTL_REQ_TX_STATUS). We cannot accurately report > everything by firmware report, it takes too many tx bandwidth, and > the performance will degrade severely. If we really cannot accept reporting Yeah, it does seem like it would be pretty heavyweight to do this C2H reporting for every frame. > tx status this way, we need to find another way to solve it. That means I > need some time to investigate and test connect monitor system and get > a better report logic if we drop the REPORTS_TX_ACK_STATUS feature. > Or if you have a point of view, we can discuss about it. > Thanks! Regards, Brian
Re: [PATCH v4 03/13] rtw88: hci files
On Sun, Feb 10, 2019 at 9:48 PM Tony Chuang wrote: > > From: Brian Norris [mailto:briannor...@chromium.org] > > > + tx_data = rtw_pci_get_tx_data(skb); > > > + tx_data->dma = dma; > > > + skb_queue_tail(&ring->queue, skb); > > > > IIUC, you have no locking for this queue. That seems like a bad idea. It > > then gets pulled off this queue in your ISR, again without a lock. So > > for example, if the only packet in your queue gets completed while you > > are trying to queue another one, you might corrupt the list. > > > > I think skb_queue_tail already has its own spinlock to protect the queue? > Cannot see why the list might be corrupted. Or I misunderstand you. Ah, no of course you're correct. I think I kept looking at the definition of __skb_queue_tail() instead. It also doesn't help that, in skbuff.h, the kerneldoc comments for __skb_queue_tail() are put above skb_queue_tail(). So it's extra easy to confuse them... And to be clear, so far I don't think I've seen actual corruption of this queue yet. Just bad TX status reporting, including plenty of driver WARN()s. So my comment was only theoretical (and incorrect). Regards, Brian
Re: [PATCH 01/24] rtw88: report correct tx status if mac80211 requested one
On Thu, Jan 31, 2019 at 08:21:14PM +0800, yhchu...@realtek.com wrote:
> From: Yan-Hsuan Chuang
>
> Before this commit, driver always reports IEEE80211_TX_STAT_ACK for
> every tx packet, but it will confuse the mac80211 stack for connection
> monitor system. mac80211 stack needs correct ack information about some
> specific packets such as prop_req, null, auth, assoc, in order to know
> if AP is alive. And for such packets, mac80211 will pass a tx flag
> IEEE80211_TX_CTL_REQ_TX_STATUS to driver. Driver then need to request a
> tx report from hardware.
I think you're misinterpreting the mac80211 semantics here. This flag
isn't for the driver to determine whether or not it should report ACKs
-- it's to help ensure that status reports *really* make it back up to
the upper layers (and don't get dropped).
On the contrary, if you look at __ieee80211_tx_status(), it's expecting
that everything that has IEEE80211_HW_REPORTS_TX_ACK_STATUS will report
an appropriate IEEE80211_TX_STAT_ACK status. The logic is basically:
if (ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS))
if (!(info->flags & IEEE80211_TX_STAT_ACK))
ieee80211_lost_packet(sta, info);
That explains why I see almost every packet get reported as lost in `iw
wlan0 station dump`.
> The tx report is not passed by hardware with the tx'ed packet, it is
> passed through C2H. So driver need to queue the packets that require
> correct tx report and upon the tx report is received, report to mac80211
> stack, with the frame is acked or not.
>
> In case of driver missed the C2H report, setup a 500ms timer to purge
> the tx report skb queue (500ms is time mac80211 used as probe_time).
>
> Signed-off-by: Yan-Hsuan Chuang
> ---
> drivers/net/wireless/realtek/rtw88/fw.c | 21 ++-
> drivers/net/wireless/realtek/rtw88/fw.h | 8 +++
> drivers/net/wireless/realtek/rtw88/main.c | 10
> drivers/net/wireless/realtek/rtw88/main.h | 13 +
> drivers/net/wireless/realtek/rtw88/pci.c | 8 ++-
> drivers/net/wireless/realtek/rtw88/pci.h | 1 +
> drivers/net/wireless/realtek/rtw88/tx.c | 96
> +++
> drivers/net/wireless/realtek/rtw88/tx.h | 8 +++
> 8 files changed, 163 insertions(+), 2 deletions(-)
...
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> b/drivers/net/wireless/realtek/rtw88/pci.c
> index ef3c9bb..7de4638 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -585,6 +585,7 @@ static int rtw_pci_xmit(struct rtw_dev *rtwdev,
>
> tx_data = rtw_pci_get_tx_data(skb);
> tx_data->dma = dma;
> + tx_data->sn = pkt_info->sn;
> skb_queue_tail(&ring->queue, skb);
>
> /* kick off tx queue */
> @@ -716,8 +717,13 @@ static void rtw_pci_tx_isr(struct rtw_dev *rtwdev,
> struct rtw_pci *rtwpci,
> skb_pull(skb, rtwdev->chip->tx_pkt_desc_sz);
>
> info = IEEE80211_SKB_CB(skb);
> +
> + /* enqueue to wait for tx report */
> + if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) {
> + rtw_tx_report_enqueue(rtwdev, skb, tx_data->sn);
This reporting code appears to be very buggy. At least, it's extremely
easy to hit the WARN() you've inserted ("purge skb(s) not reported by
firmware"), which means that the TX reporting queue is not getting
responses for a lot of packets.
So it's not clear if you should be trying to accurately report
everything (even if your firmware status reports are unreliable), or if
you should just drop the REPORTS_TX_ACK_STATUS feature.
> + continue;
> + }
> ieee80211_tx_info_clear_status(info);
> - info->flags |= IEEE80211_TX_STAT_ACK;
> ieee80211_tx_status_irqsafe(hw, skb);
One other problem with your code is that it doesn't check for
IEEE80211_TX_CTL_NO_ACK anywhere. With that flag, you should be
reporting IEEE80211_TX_STAT_NOACK_TRANSMITTED instead of
IEEE80211_TX_STAT_ACK.
> }
>
Brian
Re: [PATCH v4 03/13] rtw88: hci files
FYI, I have some more review comments because I'm trying to see why your
TX path doesn't work all that well. At least, it's not reporting things
correctly. (I know there's one ACK reporting bug you fixed in a
follow-up patch, but then, that patch is buggy too I think.)
On Wed, Jan 30, 2019 at 12:02:10PM +0800, yhchu...@realtek.com wrote:
> From: Yan-Hsuan Chuang
>
> hci files for Realtek 802.11ac wireless network chips
>
> For now there is only PCI bus supported by rtwlan, in the future it
> will also have USB/SDIO
>
> Signed-off-by: Yan-Hsuan Chuang
> ---
> drivers/net/wireless/realtek/rtw88/hci.h | 211 ++
> drivers/net/wireless/realtek/rtw88/pci.c | 1210
> ++
> drivers/net/wireless/realtek/rtw88/pci.h | 229 ++
> 3 files changed, 1650 insertions(+)
> create mode 100644 drivers/net/wireless/realtek/rtw88/hci.h
> create mode 100644 drivers/net/wireless/realtek/rtw88/pci.c
> create mode 100644 drivers/net/wireless/realtek/rtw88/pci.h
>
> diff --git a/drivers/net/wireless/realtek/rtw88/hci.h
> b/drivers/net/wireless/realtek/rtw88/hci.h
> new file mode 100644
> index 000..91b15ef
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/hci.h
> @@ -0,0 +1,211 @@
...
> +static int rtw_pci_xmit(struct rtw_dev *rtwdev,
> + struct rtw_tx_pkt_info *pkt_info,
> + struct sk_buff *skb, u8 queue)
> +{
> + struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> + struct rtw_chip_info *chip = rtwdev->chip;
> + struct rtw_pci_tx_ring *ring;
> + struct rtw_pci_tx_data *tx_data;
> + dma_addr_t dma;
> + u32 tx_pkt_desc_sz = chip->tx_pkt_desc_sz;
> + u32 tx_buf_desc_sz = chip->tx_buf_desc_sz;
> + u32 size;
> + u32 psb_len;
> + u8 *pkt_desc;
> + struct rtw_pci_tx_buffer_desc *buf_desc;
> + u32 bd_idx;
> +
> + ring = &rtwpci->tx_rings[queue];
> +
> + size = skb->len;
> +
> + if (queue == RTW_TX_QUEUE_BCN)
> + rtw_pci_release_rsvd_page(rtwpci, ring);
> + else if (!avail_desc(ring->r.wp, ring->r.rp, ring->r.len))
> + return -ENOSPC;
> +
> + pkt_desc = skb_push(skb, chip->tx_pkt_desc_sz);
> + memset(pkt_desc, 0, tx_pkt_desc_sz);
> + pkt_info->qsel = rtw_pci_get_tx_qsel(skb, queue);
> + rtw_tx_fill_tx_desc(pkt_info, skb);
> + dma = pci_map_single(rtwpci->pdev, skb->data, skb->len,
> + PCI_DMA_TODEVICE);
> + if (pci_dma_mapping_error(rtwpci->pdev, dma))
> + return -EBUSY;
> +
> + /* after this we got dma mapped, there is no way back */
> + buf_desc = get_tx_buffer_desc(ring, tx_buf_desc_sz);
> + memset(buf_desc, 0, tx_buf_desc_sz);
> + psb_len = (skb->len - 1) / 128 + 1;
> + if (queue == RTW_TX_QUEUE_BCN)
> + psb_len |= 1 << RTK_PCI_TXBD_OWN_OFFSET;
> +
> + buf_desc[0].psb_len = cpu_to_le16(psb_len);
> + buf_desc[0].buf_size = cpu_to_le16(tx_pkt_desc_sz);
> + buf_desc[0].dma = cpu_to_le32(dma);
> + buf_desc[1].buf_size = cpu_to_le16(size);
> + buf_desc[1].dma = cpu_to_le32(dma + tx_pkt_desc_sz);
> +
> + tx_data = rtw_pci_get_tx_data(skb);
> + tx_data->dma = dma;
> + skb_queue_tail(&ring->queue, skb);
IIUC, you have no locking for this queue. That seems like a bad idea. It
then gets pulled off this queue in your ISR, again without a lock. So
for example, if the only packet in your queue gets completed while you
are trying to queue another one, you might corrupt the list.
Brian
> +
> + /* kick off tx queue */
> + if (queue != RTW_TX_QUEUE_BCN) {
> + if (++ring->r.wp >= ring->r.len)
> + ring->r.wp = 0;
> + bd_idx = rtw_pci_tx_queue_idx_addr[queue];
> + rtw_write16(rtwdev, bd_idx, ring->r.wp & 0xfff);
> + } else {
> + u32 reg_bcn_work;
> +
> + reg_bcn_work = rtw_read8(rtwdev, RTK_PCI_TXBD_BCN_WORK);
> + reg_bcn_work |= BIT_PCI_BCNQ_FLAG;
> + rtw_write8(rtwdev, RTK_PCI_TXBD_BCN_WORK, reg_bcn_work);
> + }
> +
> + return 0;
> +}
...
Re: [PATCH v4 03/13] rtw88: hci files
Hi,
One more comment for now, below:
On Wed, Jan 30, 2019 at 12:02:10PM +0800, yhchu...@realtek.com wrote:
> From: Yan-Hsuan Chuang
>
> hci files for Realtek 802.11ac wireless network chips
>
> For now there is only PCI bus supported by rtwlan, in the future it
> will also have USB/SDIO
>
> Signed-off-by: Yan-Hsuan Chuang
> ---
> drivers/net/wireless/realtek/rtw88/hci.h | 211 ++
> drivers/net/wireless/realtek/rtw88/pci.c | 1210
> ++
> drivers/net/wireless/realtek/rtw88/pci.h | 229 ++
> 3 files changed, 1650 insertions(+)
> create mode 100644 drivers/net/wireless/realtek/rtw88/hci.h
> create mode 100644 drivers/net/wireless/realtek/rtw88/pci.c
> create mode 100644 drivers/net/wireless/realtek/rtw88/pci.h
...
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> b/drivers/net/wireless/realtek/rtw88/pci.c
> new file mode 100644
> index 000..ef3c9bb
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -0,0 +1,1210 @@
> +static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
> +u8 hw_queue)
> +{
> + struct rtw_chip_info *chip = rtwdev->chip;
> + struct rtw_pci_rx_ring *ring;
> + struct rtw_rx_pkt_stat pkt_stat;
> + struct ieee80211_rx_status rx_status;
> + struct sk_buff *skb, *new;
> + u32 cur_wp, cur_rp, tmp;
> + u32 count;
> + u32 pkt_offset;
> + u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
> + u32 buf_desc_sz = chip->rx_buf_desc_sz;
> + u8 *rx_desc;
> + dma_addr_t dma;
> +
> + ring = &rtwpci->rx_rings[RTW_RX_QUEUE_MPDU];
> +
> + tmp = rtw_read32(rtwdev, RTK_PCI_RXBD_IDX_MPDUQ);
> + cur_wp = tmp >> 16;
> + cur_wp &= 0xfff;
> + if (cur_wp >= ring->r.wp)
> + count = cur_wp - ring->r.wp;
> + else
> + count = ring->r.len - (ring->r.wp - cur_wp);
> +
> + cur_rp = ring->r.rp;
> + while (count--) {
> + rtw_pci_dma_check(rtwdev, ring, cur_rp);
> + skb = ring->buf[cur_rp];
> + dma = *((dma_addr_t *)skb->cb);
> + pci_unmap_single(rtwpci->pdev, dma, RTK_PCI_RX_BUF_SIZE,
> + PCI_DMA_FROMDEVICE);
> + rx_desc = skb->data;
> + chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
> &rx_status);
> +
> + /* offset from rx_desc to payload */
> + pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> + pkt_stat.shift;
> +
> + if (pkt_stat.is_c2h) {
> + /* keep rx_desc, halmac needs it */
> + skb_put(skb, pkt_stat.pkt_len + pkt_offset);
> +
> + /* pass offset for further operation */
> + *((u32 *)skb->cb) = pkt_offset;
> + skb_queue_tail(&rtwdev->c2h_queue, skb);
> + ieee80211_queue_work(rtwdev->hw, &rtwdev->c2h_work);
> + } else {
> + /* remove rx_desc, maybe use skb_pull? */
> + skb_put(skb, pkt_stat.pkt_len);
> + skb_reserve(skb, pkt_offset);
> +
> + /* alloc a smaller skb to mac80211 */
> + new = dev_alloc_skb(pkt_stat.pkt_len);
> + if (!new) {
> + new = skb;
> + } else {
> + skb_put_data(new, skb->data, skb->len);
> + dev_kfree_skb_any(skb);
> + }
> + /* TODO: merge into rx.c */
> + rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> + memcpy(new->cb, &rx_status, sizeof(rx_status));
> + ieee80211_rx_irqsafe(rtwdev->hw, new);
> + }
> +
> + /* skb delivered to mac80211, alloc a new one in rx ring */
> + new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE);
> + if (WARN(!new, "rx routine starvation\n"))
> + return;
> +
> + ring->buf[cur_rp] = new;
> + rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz);
You aren't handling failures from this function. It's not quite clear to
me whether that will just leak the SKB, or if it will end in an
unbalanced unmap later.
Brian
> +
> + /* host read next element in ring */
> + if (++cur_rp >= ring->r.len)
> + cur_rp = 0;
> + }
> +
> + ring->r.rp = cur_rp;
> + ring->r.wp = cur_wp;
> + rtw_write16(rtwdev, RTK_PCI_RXBD_IDX_MPDUQ, ring->r.rp);
> +}
Re: [PATCH] ath10k: support PCIe enter L1 state
On Fri, Feb 8, 2019 at 5:42 AM Kalle Valo wrote: > No replies from anyone (including Wen) for 3 months about testing this > patch on anything else than QCA6174. So I'll drop this now, please > resubmit once test coverage is better. I know this isn't exactly what you're asking for, but FWIW we've been using this since late November on all our QCA6174 products. No issues seen as far as I know, and we have seen some power savings. Regards, Brian
[PATCH] ath10k: pci: use mutex for diagnostic window CE polling
The DIAG copy engine is only used via polling, but it holds a spinlock
with softirqs disabled. Each iteration of our read/write loops can
theoretically take 20ms (two 10ms timeout loops), and this loop can be
run an unbounded number of times while holding the spinlock -- dependent
on the request size given by the caller.
As of commit 39501ea64116 ("ath10k: download firmware via diag Copy
Engine for QCA6174 and QCA9377."), we transfer large chunks of firmware
memory using this mechanism. With large enough firmware segments, this
becomes an exceedingly long period for disabling soft IRQs. For example,
with a 500KiB firmware segment, in testing QCA6174A, I see 200 loop
iterations of about 50-100us each, which can total about 10-20ms.
In reality, we don't really need to block softirqs for this duration.
The DIAG CE is only used in polling mode, and we only need to hold
ce_lock to make sure any CE bookkeeping is done without screwing up
another CE. Otherwise, we only need to ensure exclusion between
ath10k_pci_diag_{read,write}_mem() contexts.
This patch moves to use fine-grained locking for the shared ce_lock,
while adding a new mutex just to ensure mutual exclusion of diag
read/write operations.
Tested on QCA6174A, firmware version WLAN.RM.4.4.1-00132-QCARMSWPZ-1.
Fixes: 39501ea64116 ("ath10k: download firmware via diag Copy Engine for
QCA6174 and QCA9377.")
Signed-off-by: Brian Norris
---
I'm not quite sure if this is -stable material. It's technically an
existing bug, but it looks like previously, the DIAG interface was only
exposed via debugfs -- you could block softirqs by playing with
/sys/kernel/debug/ieee80211/phyX/ath10k/mem_value. The new usage (for
loading firmware, on QCA6174 and QCA9377) might constitute a significant
regression.
Also, I'd appreciate some review from Qualcomm folks as always.
---
drivers/net/wireless/ath/ath10k/pci.c | 41 ++-
drivers/net/wireless/ath/ath10k/pci.h | 3 ++
2 files changed, 18 insertions(+), 26 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c
b/drivers/net/wireless/ath/ath10k/pci.c
index 39e0b1cc2a12..f8356b3bf150 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -913,7 +913,6 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32
address, void *data,
int nbytes)
{
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
- struct ath10k_ce *ce = ath10k_ce_priv(ar);
int ret = 0;
u32 *buf;
unsigned int completed_nbytes, alloc_nbytes, remaining_bytes;
@@ -924,8 +923,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32
address, void *data,
void *data_buf = NULL;
int i;
- spin_lock_bh(&ce->ce_lock);
-
+ mutex_lock(&ar_pci->ce_diag_mutex);
ce_diag = ar_pci->ce_diag;
/*
@@ -960,19 +958,17 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar,
u32 address, void *data,
nbytes = min_t(unsigned int, remaining_bytes,
DIAG_TRANSFER_LIMIT);
- ret = ce_diag->ops->ce_rx_post_buf(ce_diag, &ce_data, ce_data);
+ ret = ath10k_ce_rx_post_buf(ce_diag, &ce_data, ce_data);
if (ret != 0)
goto done;
/* Request CE to send from Target(!) address to Host buffer */
- ret = ath10k_ce_send_nolock(ce_diag, NULL, (u32)address,
nbytes, 0,
- 0);
+ ret = ath10k_ce_send(ce_diag, NULL, (u32)address, nbytes, 0, 0);
if (ret)
goto done;
i = 0;
- while (ath10k_ce_completed_send_next_nolock(ce_diag,
- NULL) != 0) {
+ while (ath10k_ce_completed_send_next(ce_diag, NULL) != 0) {
udelay(DIAG_ACCESS_CE_WAIT_US);
i += DIAG_ACCESS_CE_WAIT_US;
@@ -983,10 +979,8 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32
address, void *data,
}
i = 0;
- while (ath10k_ce_completed_recv_next_nolock(ce_diag,
- (void **)&buf,
- &completed_nbytes)
- != 0) {
+ while (ath10k_ce_completed_recv_next(ce_diag, (void **)&buf,
+&completed_nbytes) != 0) {
udelay(DIAG_ACCESS_CE_WAIT_US);
i += DIAG_ACCESS_CE_WAIT_US;
@@ -1019,7 +1013,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar,
u32 address, void *data,
dma_free_coherent(ar->dev, alloc_nbytes, data_b
Re: [PATCH] ath10k: Enable Factory Test Mode for WCN3990
On Wed, Aug 29, 2018 at 12:30:38PM +0530, Rakesh Pillai wrote: > The support to put WCN3990 firmware into Factory > test mode is not present currently. The WCN3990 > firmware can operate in Factory test mode based > on the mode it receives in the wlan enable message > from the host driver. > > When the host driver is started in testmode send > the operating mode as UTF mode, to the WCN3990 > firmware, in the wlan enable message to start the > firmware in Factory test mode. > > Tested on: WCN3990 > Tested FW: WLAN.HL.2.0-01192-QCAHLSWMTPLZ-1. > > Signed-off-by: Rakesh Pillai > --- > drivers/net/wireless/ath/ath10k/ahb.c | 3 ++- > drivers/net/wireless/ath/ath10k/core.c | 2 +- > drivers/net/wireless/ath/ath10k/hif.h | 7 --- > drivers/net/wireless/ath/ath10k/mac.c | 2 +- > drivers/net/wireless/ath/ath10k/pci.c | 3 ++- > drivers/net/wireless/ath/ath10k/sdio.c | 3 ++- > drivers/net/wireless/ath/ath10k/snoc.c | 20 > drivers/net/wireless/ath/ath10k/testmode.c | 2 +- > drivers/net/wireless/ath/ath10k/usb.c | 3 ++- > 9 files changed, 31 insertions(+), 14 deletions(-) What ever happened here? I'm told this is useful, but I see no comments and it's marked Deferred in patchwork: https://patchwork.kernel.org/patch/10579559/ FWIW, it looks OK to me: Reviewed-by: Brian Norris
Re: [PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"
On Mon, Feb 4, 2019 at 8:43 AM Kalle Valo wrote:
> Brian Norris wrote:
>
> > This reverts commit cfb353c0dc058bc1619cc226d3cbbda1f360bdd3.
> >
> > WCN3990 firmware does not yet implement this feature, and so it crashes
> > like this:
> >
> > fatal error received: err_qdi.c:456:EX:wlan_process:1:WLAN
> > RT:207a:PC=b001b4f0
> >
> > This feature can be re-implemented with a proper service bitmap or other
> > feature-discovery mechanism in the future. But it should not break
> > working boards.
> >
> > Fixes: cfb353c0dc05 ("ath10k: add quiet mode support for QCA6174/QCA9377")
> > Cc: Yu Wang
> > Cc: Rakesh Pillai
> > Cc: Govind Singh
> > Cc:
> > Signed-off-by: Brian Norris
>
> This regression should be fixed by (which is in v4.20):
>
> 53884577fbce ath10k: skip sending quiet mode cmd for WCN3990
>
> So this revert should not be needed anymore and I'll drop this.
Yes, that seems to work for me. I'll probably replace the revert with
the above in my downstream. Thanks!
Can't really be fixed now, but it would have been nice to have a Fixes
tag on that one.
Brian
Re: [PATCH v2] ath10k: implement set_base_macaddr to fix rx-bssid mask in multiple APs conf
Hi,
On Mon, Feb 04, 2019 at 09:57:14PM +0100, Christian Lamparter wrote:
> Many integrated QCA9984 WiFis in various IPQ806x platform routers
> from various vendors (Netgear R7800, ZyXEL NBG6817, TP-LINK C2600,
> etc.) have either blank, bogus or non-unique MAC-addresses in
> their calibration data.
>
> As a result, OpenWrt utilizes a discouraged binary calibration data
> patching method that allows to modify the device's MAC-addresses right
> at the source. This is because the ath10k' firmware extracts the MAC
> address from the supplied radio/calibration data and issues a response
> to the ath10k linux driver. Which was designed to take the main MAC in
> ath10k_wmi_event_ready().
>
> Part of the "setting an alternate MAC" issue was already tackled by a
> patch from Brian Norris:
> commit 9d5804662ce1
> ("ath10k: retrieve MAC address from system firmware if provided")
> by allowing the option to specify an alternate MAC-address with the
> established device_get_mac_address() function which extracts the right
> address from DeviceTree/fwnode mac-address or local-mac-address
> properties and saves it for later.
>
> However, Ben Greear noted that the Qualcomm's ath10k firmware is liable
> to not properly calculate its rx-bssid mask in this case. This can cause
> issues in the popluar "multiple AP with a single ath10k instance"
> configurations.
>
> To improve MAC address handling, Felix Fietkau suggested to call
> pdev_set_base_macaddr_cmdid before bringing up the first vif and
> use the first vif MAC address there. Which is in ath10k_core_start().
>
> This patch implement Felix Fietkau's request to
> "call pdev_set_base_macaddr_cmdid before bringing up the first vif".
> The pdev_set_base_macaddr_cmdid is already declared for all devices
> and version. The driver just needed the support code for this
> function.
>
> Tested on:
> QCA9880/CUS223, firmwares: 10.2.4.13-2, 10.2.4.70.44, 10.2.4-1.0-00041
> QCA9887/MR33 firmware:10.2.4-1.0-00033
> QCA4019/RT-AC58U firmware: 10.4-3.4-00104, 10.4-3.5.3-00057
> QCA9984/R7800 firmware: Candela Technologies (CT) Firmware
>
> BugLink:
> https://lists.openwrt.org/pipermail/openwrt-devel/2018-November/014595.html
> Fixes: 9d5804662ce1 ("ath10k: retrieve MAC address from system firmware if
> provided")
> Cc: Brian Norris
> Cc: Ben Greear
> Cc: Felix Fietkau
> Cc: Mathias Kresin
> Signed-off-by: Christian Lamparter
>
> ---
>
> Changed from v1:
> - removed support for obsolete, untested firmwares
> - removed unsupported TLV ops
> - don't error-out on unsupported platforms
This doesn't break WCN3990 this time (which doesn't support this
command), and I correctly hit the EOPNOTSUPP path:
Tested-by: Brian Norris
Re: [PATCH] ath10k: implement set_base_macaddr to fix rx-bssid mask in multiple APs conf
On Mon, Feb 4, 2019 at 10:10 AM Christian Lamparter wrote:
> On Monday, February 4, 2019 4:45:12 PM CET Kalle Valo wrote:
> > Christian Lamparter writes:
> > > --- a/drivers/net/wireless/ath/ath10k/core.c
> > > +++ b/drivers/net/wireless/ath/ath10k/core.c
> > > @@ -2649,6 +2649,13 @@ int ath10k_core_start(struct ath10k *ar, enum
> > > ath10k_firmware_mode mode,
> > > goto err_hif_stop;
> > > }
> > >
> > > + status = ath10k_wmi_pdev_set_base_macaddr(ar, ar->mac_addr);
> > > + if (status) {
> > > + ath10k_err(ar,
> > > + "failed to set base mac address: %d\n", status);
> > > + goto err_hif_stop;
> > > + }
> >
> > Oh, and as the new parameter is not supported with WMI TLV interface
> > (QCA6174, WCN3990 etc) this will print an error on those.
>
> This also means that Brian won't be able to test/verify this anyway?
Well, I'd be able to tell you if this started dumping new errors to the log :)
And in fact, it seems this crashes my firmware:
[ 150.091401] qcom-q6v5-mss 408.remoteproc: fatal error received:
err_qdi.c:456:EF:wlan_process:1:cmnos_thread.c:3921:Asserted in
wlan_dev.c:WLAN_GET_MAC_ID_FROM_WMI_PDEV_ID:536
Note that I'm running WCN3990, and I haven't configure any sort of PD
restart -- so any WiFi firmware assertions/crashes take down the
entire modem/WiFi.
IOW:
Tested-and-found-wanting-by: Brian Norris
Willing to test a v2!
Regards,
Brian
Re: [PATCH] ath10k: implement set_base_macaddr to fix rx-bssid mask in multiple APs conf
Hello!
On Mon, Jan 14, 2019 at 7:36 AM Christian Lamparter wrote:
> Part of the "setting an alternate MAC" issue was already tackled by a
> patch from Brian Norris:
> commit 9d5804662ce1
> ("ath10k: retrieve MAC address from system firmware if provided")
> by allowing the option to specify an alternate MAC-address with the
> established device_get_mac_address() function which extracts the right
> address from DeviceTree/fwnode mac-address or local-mac-address
> properties and saves it for later.
>
> However, Ben Greear noted that the Qualcomm's ath10k firmware is liable
> to not properly calculate its rx-bssid mask in this case. This can cause
> issues in the popluar "multiple AP with a single ath10k instance"
> configurations.
I'll admit that
(a) I wasn't testing AP mode, or any similar configuration to this and
(b) I'm not super familiar with the firmware features involved here.
That's to say, this patch could very well be completely justified and
correct, but I may not the right person to analyze it.
I do have it on my list to test out though, so I'll hopefully chime in
again eventually.
Regards,
Brian
Re: iw release cadence?
On Fri, Feb 1, 2019 at 2:09 PM Johannes Berg wrote: > On Fri, 2019-02-01 at 22:54 +0100, Johannes Berg wrote: > > > > I'll just tag one now :-) > > Done. I updated a few things and pulled in our FTM support code too, now > that all the nl80211.h api is upstream. Awesome, thanks! Presumably a tarball is on its way? Brian
iw release cadence?
Hi Johannes, Is there any sort of release cadence to the iw tool? Or is it pretty arbitrary? I'm curious, since it's been more than a year since the "v4.14" release. I'd like to package up bugfixes and a few features, but it'd be much nicer to just pull a new official tag/tarball than to drop a bunch of individual patches into my build system. If you're going to tag something soon though, I can...just wait for you :) More bluntly: can I haz release please? Regards, Brian
Re: [PATCH v4 08/13] rtw88: debug files
Hi,
On Wed, Jan 30, 2019 at 12:02:15PM +0800, yhchu...@realtek.com wrote:
> From: Yan-Hsuan Chuang
>
> debug files for Realtek 802.11ac wireless network chips
>
> Signed-off-by: Yan-Hsuan Chuang
> ---
> drivers/net/wireless/realtek/rtw88/debug.c | 631
> +
> drivers/net/wireless/realtek/rtw88/debug.h | 35 ++
> 2 files changed, 666 insertions(+)
> create mode 100644 drivers/net/wireless/realtek/rtw88/debug.c
> create mode 100644 drivers/net/wireless/realtek/rtw88/debug.h
>
> diff --git a/drivers/net/wireless/realtek/rtw88/debug.c
> b/drivers/net/wireless/realtek/rtw88/debug.c
> new file mode 100644
> index 000..d0cb9d3
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/debug.c
> @@ -0,0 +1,631 @@
...
> +#ifdef CONFIG_RTW88_DEBUG
> +
> +void __rtw_dbg(struct rtw_dev *rtwdev, const char *fmt, ...)
> +{
> + struct va_format vaf = {
> + .fmt = fmt,
> + };
> + va_list args;
> +
> + va_start(args, fmt);
> + vaf.va = &args;
> +
> + if (net_ratelimit())
> + dev_dbg(rtwdev->dev, "%pV", &vaf);
I understand some questions came up about this dbg() interface
previously, without the most constructive result, but...
...I do find one particular aspect of this interface a little weird: it
has its own separate Kconfig flag, and yet it's still implicitly
dependent on CONFIG_DYNAMIC_DEBUG. Note how dev_dbg() gets completely
stubbed out if !defined(CONFIG_DYNAMIC_DEBUG) && !defined(DEBUG).
I think some other similar loggers end up just using
dev_printk(KERN_DEBUG, ...) for this piece of the equation, so that if
somebody has bothered to enable CONFIG_RTW88_DEBUG, they can be sure
their log messages are at least compiled in.
But then, other drivers that have used dev_printk() *also* have dynamic
methods to enable/disable their dbg-level messages (e.g., with a 'mask'
module parameter, for classifying different types of messages). That
also gives us the option of compiling in the messages while leaving them
disabled for printing by default. IOW, they basically implement a
categorized version of CONFIG_DYNAMIC_DEBUG.
(Also note: my systems generally have DYNAMIC_DEBUG disabled, but we
*do* like to have a few driver-specific debug options enabled for WiFi,
so we can debug problems in the field via runtime enable/disable.)
Altogether, I think this means you should either:
(a) alias RTW88_DEBUG with DYNAMIC_DEBUG (e.g., RTW88_DEBUG selects or
depends on DYNAMIC_DEBUG?) or, remove your own special Kconfig
entirely; or
(b) implement runtime controls to enable/disable your dbg() messages,
and do not depend on DYNAMIC_DEBUG
I kinda lean toward (b), since that's how many other WiFi drivers work,
and it prevents me from having to enable all of DYNAMIC_DEBUG (although,
it may be time to reevaluate why Chrome OS has it disabled...probably
just for mild savings on size). It also gives you the option of
classifying your debug messages even further.
Regards,
Brian
> +
> + va_end(args);
> +}
> +EXPORT_SYMBOL(__rtw_dbg);
> +
> +#endif /* CONFIG_RTW88_DEBUG */
> diff --git a/drivers/net/wireless/realtek/rtw88/debug.h
> b/drivers/net/wireless/realtek/rtw88/debug.h
> new file mode 100644
> index 000..231e4e8
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/debug.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2018 Realtek Corporation.
> + */
> +
> +#ifndef __RTW_DEBUG_H
> +#define __RTW_DEBUG_H
> +
> +#ifdef CONFIG_RTW88_DEBUGFS
> +
> +void rtw_debugfs_init(struct rtw_dev *rtwdev);
> +
> +#else
> +
> +static inline void rtw_debugfs_init(struct rtw_dev *rtwdev) {}
> +
> +#endif /* CONFIG_RTW88_DEBUGFS */
> +
> +#ifdef CONFIG_RTW88_DEBUG
> +
> +__printf(2, 3)
> +void __rtw_dbg(struct rtw_dev *rtwdev, const char *fmt, ...);
> +
> +#define rtw_dbg(rtwdev, a...) __rtw_dbg(rtwdev, ##a)
> +
> +#else
> +
> +static inline void rtw_dbg(struct rtw_dev *rtwdev, const char *fmt, ...) {}
> +
> +#endif /* CONFIG_RTW88_DEBUG */
> +
> +#define rtw_info(rtwdev, a...) dev_info(rtwdev->dev, ##a)
> +#define rtw_warn(rtwdev, a...) dev_warn(rtwdev->dev, ##a)
> +#define rtw_err(rtwdev, a...) dev_err(rtwdev->dev, ##a)
> +
> +#endif
> --
> 2.7.4
>
Re: [PATCH 00/24] rtw88: major fixes for 8822c to have stable functionalities
On Thu, Jan 31, 2019 at 08:21:13PM +0800, yhchu...@realtek.com wrote: > From: Yan-Hsuan Chuang > > Note this patch set is based on the original patch set "rtw88: mac80211 > driver for Realtek 802.11ac wireless network chips". The latest would be here, for reference: https://patchwork.kernel.org/cover/10787651/ http://lkml.kernel.org/linux-wireless/1548820940-15237-1-git-send-email-yhchu...@realtek.com > These patches are mean to make sure 8822c chip to operate normal for most > of the basic functionalities, such as power on, switch channel, scan, > connection establish and connection monitor. > > As the original patch set was sent 3 months ago, progress has been made > by Realtek during the past months. Now we have tested on more chips and > released tables and parameters for them. Also the chips are all > programed with efuse map released for 8822c. > > Most of the changes are about BB and RF, both control the tx/rx path. > PHY parameters/seq and efuse info make sure the hardware is powered on > correctly. And channel setting updates help driver to switch to target > channel accurately. Then trx mode setting and DACK will make hardware to > have stable performance to tx/rx to connect to AP. > > Here tx power control is also required to transmit with a precise power. > Otherwise if the power is too high or too low, the peer might not be able > to receive the signal. > > Finally, we need to report correct tx status for mac80211's connection > monitor system, this requires firmware's C2H of tx status report. After > this, users can use 8822c chips for more stable wireless communication. Besides the comments I added (and needing to fix the out-of-bounds reads), this series helpfully adds RFE 1 support, so the 8822C chip I have works for some basic functions -- it's not too snappy, and I feel like there's still plenty of room for improvement but: Tested-by: Brian Norris And except for a handful of patches (should look at patch 1 closer), these all look pretty sane and helpful. Reviewed-by: Brian Norris I still need to pass back over the original patchset and try a few more things out. Regards, Brian
Re: [PATCH 07/24] rtw88: 8822c: update efuse table as released
Hi,
On Thu, Jan 31, 2019 at 08:21:20PM +0800, yhchu...@realtek.com wrote:
> From: Yan-Hsuan Chuang
>
> Update efuse table layout as the document released.
> From the newest released document, 8822c has RFE type 1 and type 2.
> Without adding RFE 1 and 2 the user's efuse-programed chips will failed
> to pass the chip info check and cannot power on hardware.
> And 8822c has only 512 bytes for physical efuse.
>
> Signed-off-by: Yan-Hsuan Chuang
> ---
> drivers/net/wireless/realtek/rtw88/rtw8822c.c | 12 ++
> drivers/net/wireless/realtek/rtw88/rtw8822c.h | 53
> ++-
> 2 files changed, 31 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> index 71f2af0..5c06e32 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> @@ -1137,7 +1133,7 @@ struct rtw_chip_info rtw8822c_hw_spec = {
> .tx_buf_desc_sz = 16,
> .rx_pkt_desc_sz = 24,
> .rx_buf_desc_sz = 8,
> - .phy_efuse_size = 1024,
> + .phy_efuse_size = 512,
I realized that technically, I'm blaming your memory errors (writing
past the phy efuse buffer) on the original patchset, when you really
create the error here, when you shrink phy_efuse_size.
Regardless, the original series should be fixed to do better bounds
checking or to stop accessing a fixed value beyond 512 anyway.
Brian
> .log_efuse_size = 768,
> .ptct_efuse_size = 124,
> .txff_size = 262144,
Re: [PATCH 24/24] rtw88: 8822b: turn rtw_write32s_mask into macro
Hi,
On Thu, Jan 31, 2019 at 08:21:37PM +0800, yhchu...@realtek.com wrote:
> From: Yan-Hsuan Chuang
>
> The inlined rtw_write32s_mask has to check range of addr with
> BUILD_BUG_ON. But with some variants of gcc version the function might
> not get inlined, and it will have no idea to know how to do, then
> results in a compile error. Turn it into a macro to make sure the values
> are known when compile time.
>
> Signed-off-by: Yan-Hsuan Chuang
> ---
> drivers/net/wireless/realtek/rtw88/rtw8822b.c | 10 --
> drivers/net/wireless/realtek/rtw88/rtw8822b.h | 15 +++
> 2 files changed, 15 insertions(+), 10 deletions(-)
>
...
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822b.h
> b/drivers/net/wireless/realtek/rtw88/rtw8822b.h
> index 311fe8a..4cf193b1 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw8822b.h
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8822b.h
> @@ -97,6 +97,21 @@ struct rtw8822b_efuse {
> };
> };
>
> +static inline void
> +_rtw_write32s_mask(struct rtw_dev *rtwdev, u32 addr, u32 mask, u32 data)
> +{
> + rtw_write32_mask(rtwdev, addr, mask, data);
> + rtw_write32_mask(rtwdev, addr + 0x200, mask, data);
> +}
> +
> +/* 0xC00-0xCFF and 0xE00-0xEFF have the same layout */
Feels like this belongs with _rtw_write32s_mask() now, not here?
> +#define rtw_write32s_mask(rtwdev, addr, mask, data) \
> + do { \
> + BUILD_BUG_ON(addr < 0xC00 || addr >= 0xD00); \
You probably want parentheses around the 'addr'. You *probably* won't
run into trouble with this particular macro, but if the caller is doing
the wrong kinds of comparisons or arithmetic, this might not work they
way you want.
Brian
> +\
> + _rtw_write32s_mask(rtwdev, addr, mask, data); \
> + } while (0)
> +
> /* phy status page0 */
> #define GET_PHY_STAT_P0_PWDB(phy_stat)
> \
> le32_get_bits(*((__le32 *)(phy_stat) + 0x00), GENMASK(15, 8))
> --
> 2.7.4
>
Re: [PATCH v4 06/13] rtw88: fw and efuse files
Hi,
On Wed, Jan 30, 2019 at 12:02:13PM +0800, yhchu...@realtek.com wrote:
> From: Yan-Hsuan Chuang
>
> fw and efuse files for Realtek 802.11ac wireless network chips
>
> Signed-off-by: Yan-Hsuan Chuang
> ---
> drivers/net/wireless/realtek/rtw88/efuse.c | 150 +++
> drivers/net/wireless/realtek/rtw88/efuse.h | 53 +++
> drivers/net/wireless/realtek/rtw88/fw.c| 611
> +
> drivers/net/wireless/realtek/rtw88/fw.h| 213 ++
> 4 files changed, 1027 insertions(+)
> create mode 100644 drivers/net/wireless/realtek/rtw88/efuse.c
> create mode 100644 drivers/net/wireless/realtek/rtw88/efuse.h
> create mode 100644 drivers/net/wireless/realtek/rtw88/fw.c
> create mode 100644 drivers/net/wireless/realtek/rtw88/fw.h
>
> diff --git a/drivers/net/wireless/realtek/rtw88/efuse.c
> b/drivers/net/wireless/realtek/rtw88/efuse.c
> new file mode 100644
> index 000..7c1b782
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/efuse.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2018 Realtek Corporation.
> + */
> +
> +#include "main.h"
> +#include "efuse.h"
> +#include "reg.h"
> +#include "debug.h"
> +
> +#define RTW_EFUSE_BANK_WIFI 0x0
> +
> +static void switch_efuse_bank(struct rtw_dev *rtwdev)
> +{
> + rtw_write32_mask(rtwdev, REG_LDO_EFUSE_CTRL, BIT_MASK_EFUSE_BANK_SEL,
> + RTW_EFUSE_BANK_WIFI);
> +}
> +
> +static int rtw_dump_logical_efuse_map(struct rtw_dev *rtwdev, u8 *phy_map,
> + u8 *log_map)
> +{
> + u32 physical_size = rtwdev->efuse.physical_size;
> + u32 protect_size = rtwdev->efuse.protect_size;
> + u32 logical_size = rtwdev->efuse.logical_size;
> + u32 phy_idx, log_idx;
> + u8 hdr1, hdr2;
> + u8 blk_idx;
> + u8 valid;
> + u8 word_en;
> + int i;
> +
> + phy_idx = 0;
> +
> + do {
See my comments below about termination, but I think you need some
bounds checks up front to ensure you're not running over the buffers.
You have some checks at the end of the embedded for-loop, but it's not
clear you will always run them.
> + hdr1 = *(phy_map + phy_idx);
> + if ((hdr1 & 0x1f) == 0xf) {
> + phy_idx++;
> + hdr2 = *(phy_map + phy_idx);
> + if (hdr2 == 0xff)
> + break;
> + blk_idx = ((hdr2 & 0xf0) >> 1) | ((hdr1 >> 5) & 0x07);
> + word_en = hdr2 & 0x0f;
> + } else {
> + blk_idx = (hdr1 & 0xf0) >> 4;
> + word_en = hdr1 & 0x0f;
> + }
> +
> + if (hdr1 == 0xff)
> + break;
> +
> + phy_idx++;
> + for (i = 0; i < 4; i++) {
> + valid = (~(word_en >> i)) & 0x1;
> + if (valid != 0x1)
> + continue;
> + log_idx = (blk_idx << 3) + (i << 1);
> + *(log_map + log_idx) = *(phy_map + phy_idx);
> + log_idx++;
> + phy_idx++;
> + *(log_map + log_idx) = *(phy_map + phy_idx);
> + phy_idx++;
> + if (phy_idx > physical_size - protect_size ||
> + log_idx > logical_size)
> + return -EINVAL;
> + }
> + } while (1);
This is a complicated and ugly loop. Can you make this easier to read?
Comments? Describe the layout in words or a diagram? Macros? At the
moment, I can't even guarantee that this while(1) loop is guaranteed to
terminate, let alone actually determine what exactly you're trying to
parse.
> +
> + return 0;
> +}
> +
> +static int rtw_dump_physical_efuse_map(struct rtw_dev *rtwdev, u8 *map)
> +{
> + struct rtw_chip_info *chip = rtwdev->chip;
> + u32 size = rtwdev->efuse.physical_size;
> + u32 efuse_ctl;
> + u32 addr;
> + u32 cnt;
> +
> + switch_efuse_bank(rtwdev);
> +
> + /* disable 2.5V LDO */
> + chip->ops->cfg_ldo25(rtwdev, false);
> +
> + efuse_ctl = rtw_read32(rtwdev, REG_EFUSE_CTRL);
> +
> + for (addr = 0; addr < size; addr++) {
> + efuse_ctl &= ~(BIT_MASK_EF_DATA | BITS_EF_ADDR);
> + efuse_ctl |= (addr & BIT_MASK_EF_ADDR) << BIT_SHIFT_EF_ADDR;
> + rtw_write32(rtwdev, REG_EFUSE_CTRL, efuse_ctl & (~BIT_EF_FLAG));
> +
> + cnt = 100;
> + do {
> + udelay(1);
> + efuse_ctl = rtw_read32(rtwdev, REG_EFUSE_CTRL);
> + if (--cnt == 0)
> + return -EBUSY;
> + } while (!(efuse_ctl & BIT_EF_FLAG));
> +
> + *(map + addr) = (u8)(efuse_ctl & BIT_MASK_EF_DATA);
> + }
> +
> + return 0;
> +}
> +
> +int rtw_parse_efuse_map(struct rtw_dev *rtwdev)
> +{
> + struct rtw_chip_info *chip = r
Re: [PATCH v4 03/13] rtw88: hci files
Hi,
A few scattered comments:
On Wed, Jan 30, 2019 at 12:02:10PM +0800, yhchu...@realtek.com wrote:
> From: Yan-Hsuan Chuang
>
> hci files for Realtek 802.11ac wireless network chips
>
> For now there is only PCI bus supported by rtwlan, in the future it
> will also have USB/SDIO
>
> Signed-off-by: Yan-Hsuan Chuang
> ---
> drivers/net/wireless/realtek/rtw88/hci.h | 211 ++
> drivers/net/wireless/realtek/rtw88/pci.c | 1210
> ++
> drivers/net/wireless/realtek/rtw88/pci.h | 229 ++
> 3 files changed, 1650 insertions(+)
> create mode 100644 drivers/net/wireless/realtek/rtw88/hci.h
> create mode 100644 drivers/net/wireless/realtek/rtw88/pci.c
> create mode 100644 drivers/net/wireless/realtek/rtw88/pci.h
>
...
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> b/drivers/net/wireless/realtek/rtw88/pci.c
> new file mode 100644
> index 000..ef3c9bb
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -0,0 +1,1210 @@
...
> +static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
> + struct rtw_pci_rx_ring *rx_ring,
> + u8 desc_size, u32 len)
> +{
> + struct pci_dev *pdev = to_pci_dev(rtwdev->dev);
> + struct sk_buff *skb = NULL;
> + dma_addr_t dma;
> + u8 *head;
> + int ring_sz = desc_size * len;
> + int buf_sz = RTK_PCI_RX_BUF_SIZE;
> + int i, allocated;
> + int ret = 0;
> +
> + head = pci_zalloc_consistent(pdev, ring_sz, &dma);
> + if (!head) {
> + rtw_err(rtwdev, "failed to allocate rx ring\n");
> + return -ENOMEM;
> + }
> + rx_ring->r.head = head;
> +
> + for (i = 0; i < len; i++) {
> + skb = dev_alloc_skb(buf_sz);
> + if (!skb) {
> + allocated = i;
> + ret = -ENOMEM;
> + goto err_out;
> + }
> +
> + memset(skb->data, 0, buf_sz);
> + rx_ring->buf[i] = skb;
> + ret = rtw_pci_reset_rx_desc(rtwdev, skb, rx_ring, i, desc_size);
> + if (ret) {
> + allocated = i;
> + goto err_out;
> + }
> + }
> +
> + rx_ring->r.dma = dma;
> + rx_ring->r.len = len;
> + rx_ring->r.desc_size = desc_size;
> + rx_ring->r.wp = 0;
> + rx_ring->r.rp = 0;
> +
> + return 0;
> +
> +err_out:
> + dev_kfree_skb_any(skb);
Maybe I'm misreading but...shouldn't this line not be here? You properly
iterate over the allocated SKBs below, and this looks like you're just
going to be double-freeing (or, negative ref-counting).
> + for (i = 0; i < allocated; i++) {
> + skb = rx_ring->buf[i];
> + if (!skb)
> + continue;
> + dma = *((dma_addr_t *)skb->cb);
> + pci_unmap_single(pdev, dma, buf_sz, PCI_DMA_FROMDEVICE);
> + dev_kfree_skb_any(skb);
> + rx_ring->buf[i] = NULL;
> + }
> + pci_free_consistent(pdev, ring_sz, head, dma);
> +
> + rtw_err(rtwdev, "failed to init rx buffer\n");
> +
> + return ret;
> +}
> +
...
> +static void rtw_pci_dma_check(struct rtw_dev *rtwdev,
> + struct rtw_pci_rx_ring *rx_ring,
> + u32 idx)
> +{
> + struct rtw_chip_info *chip = rtwdev->chip;
> + struct rtw_pci_rx_buffer_desc *buf_desc;
> + u32 desc_sz = chip->rx_buf_desc_sz;
> + u16 total_pkt_size;
> + int i;
> +
> + buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
> + idx * desc_sz);
> + for (i = 0; i < 20; i++) {
> + total_pkt_size = le16_to_cpu(buf_desc->total_pkt_size);
> + if (total_pkt_size)
> + return;
> + }
Umm, what are you trying to do here? This is a non-sensical loop. I
*imagine* you're trying to do some kind of timeout loop here, but since
there's nothing telling the compiler that this is anything but normal
memory, this loop gets flattened by the compiler into a single check of
->total_pkt_size (I checked; my compiler gets rid of the loop).
So, at a minimum, you should just remove the loop. But I'm not sure if
this "check" function has any value at all...
> +
> + if (i >= 20)
> + rtw_warn(rtwdev, "pci bus timeout, drop packet\n");
...BTW, I'm seeing this get triggered quite a bit.
Do you have some kind of memory mapping/ordering issue or something? I
wouldn't think you should expect to just drop packets on the floor so
often like this.
> +}
> +
...
> +
> +static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
> +u8 hw_queue)
> +{
> + struct rtw_chip_info *chip = rtwdev->chip;
> + struct rtw_pci_rx_ring *ring;
> + struct rtw_rx_pkt_stat pkt_stat;
> + struct ieee80211_rx_status rx_status;
> + struct sk_buff *skb, *new;
> + u32 cur_wp, cur_rp, tmp
Re: [PATCH v4 09/13] rtw88: chip files
Hi,
On Wed, Jan 30, 2019 at 12:02:16PM +0800, yhchu...@realtek.com wrote:
> From: Yan-Hsuan Chuang
>
> chip files Realtek 802.11ac wireless network chips
> 8822B & 8822C series files
>
> Signed-off-by: Yan-Hsuan Chuang
> ---
> drivers/net/wireless/realtek/rtw88/rtw8822b.c | 1590
>
> drivers/net/wireless/realtek/rtw88/rtw8822b.h | 155 ++
> .../net/wireless/realtek/rtw88/rtw8822b_table.h| 18 +
> drivers/net/wireless/realtek/rtw88/rtw8822c.c | 1169 ++
> drivers/net/wireless/realtek/rtw88/rtw8822c.h | 171 +++
> .../net/wireless/realtek/rtw88/rtw8822c_table.h| 16 +
> 6 files changed, 3119 insertions(+)
> create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822b.c
> create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822b.h
> create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822b_table.h
> create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822c.c
> create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822c.h
> create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822c_table.h
>
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822b.c
> b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
> new file mode 100644
> index 000..0339041
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
> @@ -0,0 +1,1590 @@
...
> +static inline void
> +rtw_write32s_mask(struct rtw_dev *rtwdev, u32 addr, u32 mask, u32 data)
> +{
> + BUILD_BUG_ON(addr < 0xC00 || addr >= 0xD00);
This seems to be a non-traditional use of BUILD_BUG_ON(). Normally, I
see this used for stuff that's guaranteed to be known at compile time --
structure offsets, constants, etc. This is usually (always?) a constant,
but it passes through a function parameter, so I'm not sure if that's
really guaranteed.
Anyway...this is failing confusingly for me when I try to build:
drivers/net/wireless/realtek/rtw88/rtw8822b.c: In function ‘rtw_write32s_mask’:
drivers/net/wireless/realtek/rtw88/rtw8822b.c:230:176: error: call to
‘__compiletime_assert_230’ declared with attribute error: BUILD_BUG_ON failed:
addr < 0xC00 || addr >= 0xD00
BUILD_BUG_ON(addr < 0xC00 || addr >= 0xD00);
^
I tried to pinpoint which call yielded this, and I think once I deleted
enough calls to rtw_write32s_mask() it came down to this one:
rtw_write32s_mask(rtwdev, REG_RFEINV, BIT(11) | BIT(10) | 0x3f, 0x0);
which doesn't really make sense, as that's a value of 0xcbc.
What I really think it comes down to is that you can't guarantee
rtw_write32s_mask() will get inlined, and so BUILD_BUG_ON() may not know
what to do with it.
I think you should either drop the BUILD_BUG_ON(), or else turn
rtw_write32s_mask() into a macro.
BTW, I'm using a variant of gcc version 4.9.2. I normally use clang for
kernel builds (as that's the current Chromium OS toolchain), but clang
still regularly has breakage upstream, so I'm still building with an old
GCC toolchain. It's technically still supported in
Documentation/process/changes.rst though!
Brian
> +
> + rtw_write32_mask(rtwdev, addr, mask, data);
> + /* 0xC00-0xCFF and 0xE00-0xEFF have the same layout */
> + rtw_write32_mask(rtwdev, addr + 0x200, mask, data);
> +}
> +
[PATCH] ath10k: sdio: add .owner field
sdio_register_driver() doesn't do this for us, unlike (for example)
platform_driver_register(). This is important for helping track
module-to-device relationships.
Signed-off-by: Brian Norris
---
drivers/net/wireless/ath/ath10k/sdio.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c
b/drivers/net/wireless/ath/ath10k/sdio.c
index 983ecfef1d28..2fc0c17c9672 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -2088,7 +2088,10 @@ static struct sdio_driver ath10k_sdio_driver = {
.id_table = ath10k_sdio_devices,
.probe = ath10k_sdio_probe,
.remove = ath10k_sdio_remove,
- .drv.pm = ATH10K_SDIO_PM_OPS,
+ .drv = {
+ .owner = THIS_MODULE,
+ .pm = ATH10K_SDIO_PM_OPS,
+ },
};
static int __init ath10k_sdio_init(void)
--
2.20.1.495.gaa96b0ce6b-goog
[PATCH for-5.0] ath10k: correct bus type for WCN3990
WCN3990 is SNOC, not PCI. This prevents probing WCN3990.
Fixes: 367c899f622c ("ath10k: add bus type check in ath10k_init_hw_params")
Signed-off-by: Brian Norris
---
This was a regression in 4.20.
drivers/net/wireless/ath/ath10k/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.c
b/drivers/net/wireless/ath/ath10k/core.c
index 399b501f3c3c..e8891f5fc83a 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -548,7 +548,7 @@ static const struct ath10k_hw_params
ath10k_hw_params_list[] = {
{
.id = WCN3990_HW_1_0_DEV_VERSION,
.dev_id = 0,
- .bus = ATH10K_BUS_PCI,
+ .bus = ATH10K_BUS_SNOC,
.name = "wcn3990 hw1.0",
.continuous_frag_desc = true,
.tx_chain_mask = 0x7,
--
2.20.1.495.gaa96b0ce6b-goog
Re: [PATCH] mwifiex: don't print error message on coex event
On Mon, Jan 28, 2019 at 7:43 AM Stefan Agner wrote: > > The BT coex event is not an error condition. Don't print an error > message in this case. The same even in sta_event.c prints a > message using the debug level already. For some reason, that one (and a handful of others) uses dev_dbg() instead of mwifiex_dbg() with an appropriate mask (e.g., EVENT). But mwifiex_dbg() seems preferable for consistency. > Signed-off-by: Stefan Agner Reviewed-by: Brian Norris > --- > drivers/net/wireless/marvell/mwifiex/uap_event.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/uap_event.c > b/drivers/net/wireless/marvell/mwifiex/uap_event.c > index e86217a6b9ca..ca759d9c0253 100644 > --- a/drivers/net/wireless/marvell/mwifiex/uap_event.c > +++ b/drivers/net/wireless/marvell/mwifiex/uap_event.c > @@ -300,7 +300,7 @@ int mwifiex_process_uap_event(struct mwifiex_private > *priv) > mwifiex_11h_handle_radar_detected(priv, adapter->event_skb); > break; > case EVENT_BT_COEX_WLAN_PARA_CHANGE: > - dev_err(adapter->dev, "EVENT: BT coex wlan param update\n"); > + mwifiex_dbg(adapter, EVENT, "event: BT coex wlan param > update\n"); > mwifiex_bt_coex_wlan_param_update_event(priv, > adapter->event_skb); > break; > -- > 2.20.1 >
Re: [PATCH] ath10k: snoc: remove set but not used variable 'ar_snoc'
On Mon, Jan 28, 2019 at 9:53 PM YueHaibing wrote: > > ping... For some reason, your patch shows up as Deferred in patchwork: https://patchwork.kernel.org/patch/10589789/ So the maintainers have accidentally (?) ignored it. I'm not what the official suggestion is for that, but you might just resend. In any case... > On 2018/9/6 10:29, YueHaibing wrote: > > From: Yue Haibing > > > > Fixes gcc '-Wunused-but-set-variable' warning: > > > > drivers/net/wireless/ath/ath10k/snoc.c: In function > > 'ath10k_snoc_tx_pipe_cleanup': > > drivers/net/wireless/ath/ath10k/snoc.c:681:22: warning: > > variable 'ar_snoc' set but not used [-Wunused-but-set-variable] > > > > Signed-off-by: Yue Haibing ...patch looks fine to me: Reviewed-by: Brian Norris
Re: [PATCH v3 00/13] rtw88: mac80211 driver for Realtek 802.11ac wireless network chips
On Mon, Jan 28, 2019 at 6:15 PM Tony Chuang wrote: > This NULL pointer was found months ago and has been fixed already. > Thanks for your test :). > I am holding the patch to fix it for the next patchsets. > > BTW, since rtw88 has not been accepted, could I send next patch set based on > this patch set as long as I explicitly mark that the next patch is based on > the previous one? I'd normally expect that if you find major bugs in your initial driver submission that still isn't reviewed/merged, you might as well just roll the fix into latest version and describe it in the cover-letter/changelog. This particular change is so trivial it doesn't really seem to deserve a separate patch. (It would also help people like me, who may very well run into the same bug when they get around to testing/reviewing the driver.) I also don't know what the contents of the "next patch set" is -- if it's a lot of new features, maybe they don't deserve to clutter the initial submission, but if they're bugfixes like this, it seems like you could just fix the original patch set. But that's just my opinion. Brian
Re: [PATCH v3 00/13] rtw88: mac80211 driver for Realtek 802.11ac wireless network chips
On Mon, Jan 28, 2019 at 2:16 AM Stanislaw Gruszka wrote: > On Mon, Jan 28, 2019 at 09:25:32AM +, Tony Chuang wrote: > > Sorry for the typo here. Yes, I mean " wireless-drivers-next ". > > In MAINTAINERS file, I do change to "wireless-drivers-next" instead of > > "wireless-next". > > The git entry in MAINTAINER file is not necessary and should point > the driver specific tree, which rtw88 do not have. Just remove this > entry. Yes, it seems Tony mis-interpreted my suggestion when I said he was using the wrong tree. I agree this driver does not need a git entry at all. Regards, Brian P.S. I do plan to give this driver a spin this week and hopefully give it a little closer review. No guarantees I actually get the time for that, but I hope to...
Re: [PATCH v2 13/13] rtw88: add support for Realtek 802.11ac wireless chips
Hi, On Sun, Jan 27, 2019 at 7:36 PM Tony Chuang wrote: > > -Original Message- > > From: Brian Norris [mailto:briannor...@chromium.org] > > > > On Fri, Nov 16, 2018 at 07:31:19PM +0800, yhchu...@realtek.com wrote: > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index 9ad052a..138515b 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -12546,6 +12546,14 @@ T: git > > git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-testing.g > > > S: Maintained > > > F: drivers/net/wireless/realtek/rtlwifi/ > > > > > > +REALTEK WIRELESS DRIVER (rtw88) > > > +M: Yan-Hsuan Chuang > > > +L: linux-wireless@vger.kernel.org > > > +W: http://wireless.kernel.org/ > > > +T: git > > git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-testing.git > > > > That tree hasn't been updated in years. I expect that's not relevant, so > > perhaps you should remove it? I think these days, unless you have a > > large amount of patches that deserves its own tree / pull-request, > > you're probably just going to be covered by the wireless-drivers / > > wireless-drivers-next trees: > > > > NETWORKING DRIVERS (WIRELESS) > > M: Kalle Valo > > L: linux-wireless@vger.kernel.org > > Q: http://patchwork.kernel.org/project/linux-wireless/list/ > > T: git > > git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git > > T: git > > git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git > > OK, I can change to wireless-drivers tree and send PATCHv3. I wouldn't list wireless-drivers directly -- you don't need to list any tree at all, and the "NETWORKING DRIVERS (WIRELESS)" entry will cover you anyway. Notice how none of these list their own trees either: MARVELL MWIFIEX WIRELESS DRIVER MARVELL MWL8K WIRELESS DRIVER MEDIATEK MT76 WIRELESS LAN DRIVER ORINOCO DRIVER P54 WIRELESS DRIVER ... It's probably not worth another resend. If Kalle cares, I'm sure he can delete your 'T:' line. Brian
Re: [PATCH v2 13/13] rtw88: add support for Realtek 802.11ac wireless chips
Hi Yan-Hsuan, On Fri, Nov 16, 2018 at 07:31:19PM +0800, yhchu...@realtek.com wrote: > From: Yan-Hsuan Chuang > > Signed-off-by: Yan-Hsuan Chuang > --- > MAINTAINERS | 8 > 1 file changed, 8 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9ad052a..138515b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12546,6 +12546,14 @@ T: git > git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-testing.g > S: Maintained > F: drivers/net/wireless/realtek/rtlwifi/ > > +REALTEK WIRELESS DRIVER (rtw88) > +M: Yan-Hsuan Chuang > +L: linux-wireless@vger.kernel.org > +W: http://wireless.kernel.org/ > +T: git > git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-testing.git That tree hasn't been updated in years. I expect that's not relevant, so perhaps you should remove it? I think these days, unless you have a large amount of patches that deserves its own tree / pull-request, you're probably just going to be covered by the wireless-drivers / wireless-drivers-next trees: NETWORKING DRIVERS (WIRELESS) M: Kalle Valo L: linux-wireless@vger.kernel.org Q: http://patchwork.kernel.org/project/linux-wireless/list/ T: git git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git T: git git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git Regards, Brian > +S: Maintained > +F: drivers/net/wireless/realtek/rtw88/ > + > RTL8XXXU WIRELESS DRIVER (rtl8xxxu) > M: Jes Sorensen > L: linux-wireless@vger.kernel.org
Re: [PATCH] ath10k: Move non-fatal warn logs to dbg level
On Thu, Dec 20, 2018 at 06:40:43PM +0200, Kalle Valo wrote: > Brian Norris writes: > > Alternatively, you could add these ignored commands to wmi-tlv.h, and > > just ignore them specifically. That's better documentation, in case > > new WMI events come along that *are* important to notice when we > > ignore them. There are other recent examples of that sort of patch > > being merged. > > I have actually tried to do that (what Brian describes above) during the > last years but the problem is that there are just so many different > firmware branches that it's difficult to keep up in ath10k. So in the > end these warnings just confused the users and they mistakenly thought > this warning message is the cause of their problems. That's why I think > it's best to remove this warning. Thanks for the background. To be clear, I'm perfectly fine with this patch too. I was mostly just volunteering an alternative, in case it *was* your intention as maintainer to keep it this way. Regards, Brian
Re: [EXT] Re: [PATCH] mwifiex: fix nested rtnl locking on BG_SCAN_STOPPED
Hi Ganapathi, For some reason, I'm daft enough to reply to this ancient thread. It appears as if you probably have not resolved this issue yet though, so I figured I could lend some advice. On Wed, Apr 25, 2018 at 1:06 AM Ganapathi Bhat wrote: > > Ganapathi Bhat writes: > > > --- a/drivers/net/wireless/marvell/mwifiex/sta_event.c > > > +++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c > > > @@ -848,7 +848,10 @@ int mwifiex_process_sta_event(struct > > > mwifiex_private *priv) > > > > > > case EVENT_BG_SCAN_STOPPED: > > > dev_dbg(adapter->dev, "event: BGS_STOPPED\n"); > > > - cfg80211_sched_scan_stopped(priv->wdev.wiphy, 0); > > > + if (rtnl_is_locked()) > > > + cfg80211_sched_scan_stopped_rtnl(priv- > > >wdev.wiphy, 0); > > > + else > > > + cfg80211_sched_scan_stopped(priv->wdev.wiphy, > > 0); > > > > IMHO checking if a lock is taking is rather ugly and an indication there's a > > problem with the locking. Instead making an ugly workaround like this I > > would rather investigate who is holding the rtnl and solve that. Agreed, this is not good. You are now bound to hit ASSERT_RTNL() warnings occasionally, since you might see rtnl locked here, but a split second later, when you're running in __cfg80211_stop_sched_scan(), it could be released by some other thread. > There can be applications trying to access driver(via cfg80211), holding > rtnl_lock. > For example(in our case): > 1. "iw dev" was called, when BG_SCAN was active. > 2. NL80211_CMD_GET_INTERFACE requires rtnl_lock to be hold(specified in > internal_flags) > 3. cfg80211 will hold rtnl_lock before calling "get_tx_power"(in pre_doit). > 4. mwifiex will download RF_TX_PWR command to firmware > 5. firmware will send BG_SCAN_STOPPED event(since BG_SCAN was active). > 6. mwifiex will call "cfg80211_sched_scan_stopped" causing nested rtnl > locking. IIUC, it's not exactly a nested lock, but a lock inversion issue. #1 NL80211_CMD_GET_INTERFACE thread is holding rtnl lock and later waiting on one of its HostCmd_CMD_* to complete In the meantime: #2 a EVENT_BG_SCAN_STOPPED is queued, and it grabs the rtnl lock Because events are served on the same thread as commands, you get #1 waiting on #2, which is waiting on the lock held in #1. i.e., ABBA. The way to resolve this is to either move event processing to a different thread than command processing (that looks it would be very difficult to do correctly, given the ossified structure of your driver; but that might be a wise move in the long term)... ...or else maybe you could defer this specific piece of work to its own thread. That might require yet another workqueue? Anyway, the key point is that you really don't want to hold rtnl_lock in your main workqueue, or in any other way that might block the rest of the operation of your driver. Brian
Re: [PATCH v2] mwifiex: debugfs: correct histogram spacing, formatting
On Tue, Dec 04, 2018 at 08:37:30AM +0200, Kalle Valo wrote: > Brian Norris writes: > > > Here's a v2 that combines both sets of strings in that way. I'm not > > resending the other patches, since they were only related in concept > > (since I was referring to debugfs for implementing the nl80211 stuff), > > but have no real dependency. > > > > @Kalle: I can resend the others as a new series if that helps. > > Yeah, it does. Trying to pick patches from different places in patchwork > is just too much of a hassle for me. So please do resend the whole > series. I've split them up and resent the other 2 unmodified now (as 'RFC PATCH v2 X/2'), as this patch is trivial and not related to the others directly. They can be applied in any order. Hopefully that's sufficient. Brian
[RFC PATCH v2 1/2] mwifiex: refactor mwifiex_parse_htinfo() for reuse
This function converts some firmware-specific parameters into cfg80211
'rate_info' structures. It currently assumes it's dealing only with TX
bitrate, but the RX bitrate looks to be the same, so refactor this
function to be reusable.
Signed-off-by: Brian Norris
---
v2:
* no change - just split unrelated (debugfs) patch to its own series
---
.../net/wireless/marvell/mwifiex/cfg80211.c | 36 ++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index adc88433faa8..02b80ea232a7 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -1275,27 +1275,27 @@ mwifiex_cfg80211_change_virtual_intf(struct wiphy
*wiphy,
}
static void
-mwifiex_parse_htinfo(struct mwifiex_private *priv, u8 tx_htinfo,
+mwifiex_parse_htinfo(struct mwifiex_private *priv, u8 rateinfo, u8 htinfo,
struct rate_info *rate)
{
struct mwifiex_adapter *adapter = priv->adapter;
if (adapter->is_hw_11ac_capable) {
/* bit[1-0]: 00=LG 01=HT 10=VHT */
- if (tx_htinfo & BIT(0)) {
+ if (htinfo & BIT(0)) {
/* HT */
- rate->mcs = priv->tx_rate;
+ rate->mcs = rateinfo;
rate->flags |= RATE_INFO_FLAGS_MCS;
}
- if (tx_htinfo & BIT(1)) {
+ if (htinfo & BIT(1)) {
/* VHT */
- rate->mcs = priv->tx_rate & 0x0F;
+ rate->mcs = rateinfo & 0x0F;
rate->flags |= RATE_INFO_FLAGS_VHT_MCS;
}
- if (tx_htinfo & (BIT(1) | BIT(0))) {
+ if (htinfo & (BIT(1) | BIT(0))) {
/* HT or VHT */
- switch (tx_htinfo & (BIT(3) | BIT(2))) {
+ switch (htinfo & (BIT(3) | BIT(2))) {
case 0:
rate->bw = RATE_INFO_BW_20;
break;
@@ -1310,26 +1310,26 @@ mwifiex_parse_htinfo(struct mwifiex_private *priv, u8
tx_htinfo,
break;
}
- if (tx_htinfo & BIT(4))
+ if (htinfo & BIT(4))
rate->flags |= RATE_INFO_FLAGS_SHORT_GI;
- if ((priv->tx_rate >> 4) == 1)
+ if ((rateinfo >> 4) == 1)
rate->nss = 2;
else
rate->nss = 1;
}
} else {
/*
-* Bit 0 in tx_htinfo indicates that current Tx rate
-* is 11n rate. Valid MCS index values for us are 0 to 15.
+* Bit 0 in htinfo indicates that current rate is 11n. Valid
+* MCS index values for us are 0 to 15.
*/
- if ((tx_htinfo & BIT(0)) && (priv->tx_rate < 16)) {
- rate->mcs = priv->tx_rate;
+ if ((htinfo & BIT(0)) && (rateinfo < 16)) {
+ rate->mcs = rateinfo;
rate->flags |= RATE_INFO_FLAGS_MCS;
rate->bw = RATE_INFO_BW_20;
- if (tx_htinfo & BIT(1))
+ if (htinfo & BIT(1))
rate->bw = RATE_INFO_BW_40;
- if (tx_htinfo & BIT(2))
+ if (htinfo & BIT(2))
rate->flags |= RATE_INFO_FLAGS_SHORT_GI;
}
}
@@ -1375,7 +1375,8 @@ mwifiex_dump_station_info(struct mwifiex_private *priv,
sinfo->tx_packets = node->stats.tx_packets;
sinfo->tx_failed = node->stats.tx_failed;
- mwifiex_parse_htinfo(priv, node->stats.last_tx_htinfo,
+ mwifiex_parse_htinfo(priv, priv->tx_rate,
+node->stats.last_tx_htinfo,
&sinfo->txrate);
sinfo->txrate.legacy = node->stats.last_tx_rate * 5;
@@ -1401,7 +1402,8 @@ mwifiex_dump_station_info(struct mwifiex_private *priv,
HostCmd_ACT_GEN_GET, DTIM_PERIOD_I,
&priv->dtim_period, true);
- mwifiex_parse_htinfo(priv, priv->tx_htinfo, &sinfo->txrate);
+ mwifiex_parse_htinfo(priv, priv->tx_rate, priv->tx_htinfo,
+&sinfo->txrate);
sinfo->signal_avg = priv->bcn_rssi_avg;
sinfo->rx_bytes = priv->stats.rx_bytes;
--
2.20.0.rc2.403.gdbc3b29805-goog
[RFC PATCH v2 2/2] mwifiex: add NL80211_STA_INFO_RX_BITRATE support
Comparing the existing TX_BITRATE parsing code (in
mwifiex_parse_htinfo()) with the RX bitrate histograms in debugfs.c, it
appears that the rxpd_rate and rxpd_htinfo fields have the same format.
At least, they give reasonable results when I parse them this way.
So this patch adds support for RX_BITRATE to our station info dump.
Along the way, I add legacy bitrate parsing into the same function,
using the debugfs code (mwifiex_histogram_read() and
mwifiex_adjust_data_rate()) as reference.
Additionally, to satisfy the requirements of
NL80211_STA_INFO_RX_BITRATE, I skip logging the bitrate of multicast
packets. This shouldn't add a lot of overhead to the RX path, as there
are already several similar 802.3 header checks in this same codepath.
We can also bias the branch behavior to favor unicast, as that's the
common performance-sensitive case.
I'd consider this support somewhat experimental, as I have zero
documentation from Marvell. But the existing driver code gives me good
reason to think this is correct.
I've tested this on a few different 802.11{a,b,g,n,ac} networks, and the
reported bitrates look good to me.
Signed-off-by: Brian Norris
---
RFC: I'd appreciate it if someone from Marvell could double check my work
here.
v2:
* no change - just split unrelated (debugfs) patch to its own series
---
.../net/wireless/marvell/mwifiex/cfg80211.c | 26 +++
drivers/net/wireless/marvell/mwifiex/sta_rx.c | 13 ++
2 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 02b80ea232a7..1467af22e394 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -1333,6 +1333,28 @@ mwifiex_parse_htinfo(struct mwifiex_private *priv, u8
rateinfo, u8 htinfo,
rate->flags |= RATE_INFO_FLAGS_SHORT_GI;
}
}
+
+ /* Decode legacy rates for non-HT. */
+ if (!(htinfo & (BIT(0) | BIT(1 {
+ /* Bitrates in multiples of 100kb/s. */
+ static const int legacy_rates[] = {
+ [0] = 10,
+ [1] = 20,
+ [2] = 55,
+ [3] = 110,
+ [4] = 60, /* MWIFIEX_RATE_INDEX_OFDM0 */
+ [5] = 60,
+ [6] = 90,
+ [7] = 120,
+ [8] = 180,
+ [9] = 240,
+ [10] = 360,
+ [11] = 480,
+ [12] = 540,
+ };
+ if (rateinfo < ARRAY_SIZE(legacy_rates))
+ rate->legacy = legacy_rates[rateinfo];
+ }
}
/*
@@ -1414,6 +1436,10 @@ mwifiex_dump_station_info(struct mwifiex_private *priv,
/* bit rate is in 500 kb/s units. Convert it to 100kb/s units */
sinfo->txrate.legacy = rate * 5;
+ sinfo->filled |= BIT(NL80211_STA_INFO_RX_BITRATE);
+ mwifiex_parse_htinfo(priv, priv->rxpd_rate, priv->rxpd_htinfo,
+&sinfo->rxrate);
+
if (priv->bss_mode == NL80211_IFTYPE_STATION) {
sinfo->filled |= BIT_ULL(NL80211_STA_INFO_BSS_PARAM);
sinfo->bss_param.flags = 0;
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_rx.c
b/drivers/net/wireless/marvell/mwifiex/sta_rx.c
index 00fcbda09349..fb28a5c7f441 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_rx.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_rx.c
@@ -152,14 +152,17 @@ int mwifiex_process_rx_packet(struct mwifiex_private
*priv,
mwifiex_process_tdls_action_frame(priv, offset, rx_pkt_len);
}
- priv->rxpd_rate = local_rx_pd->rx_rate;
-
- priv->rxpd_htinfo = local_rx_pd->ht_info;
+ /* Only stash RX bitrate for unicast packets. */
+ if (likely(!is_multicast_ether_addr(rx_pkt_hdr->eth803_hdr.h_dest))) {
+ priv->rxpd_rate = local_rx_pd->rx_rate;
+ priv->rxpd_htinfo = local_rx_pd->ht_info;
+ }
if (GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_STA ||
GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_UAP) {
- adj_rx_rate = mwifiex_adjust_data_rate(priv, priv->rxpd_rate,
- priv->rxpd_htinfo);
+ adj_rx_rate = mwifiex_adjust_data_rate(priv,
+ local_rx_pd->rx_rate,
+ local_rx_pd->ht_info);
mwifiex_hist_data_add(priv, adj_rx_rate, local_rx_pd->snr,
local_rx_pd->nf);
}
--
2.20.0.rc2.403.gdbc3b29805-goog
[PATCH v2] mwifiex: debugfs: correct histogram spacing, formatting
Currently, snippets of this file look like:
rx rates (in Mbps): 0=1M 1=2M2=5.5M 3=11M 4=6M 5=9M 6=12M
7=18M 8=24M 9=36M 10=48M 11=54M12-27=MCS0-15(BW20) 28-43=MCS0-15(BW40)
44-53=MCS0-9(VHT:BW20)54-63=MCS0-9(VHT:BW40)64-73=MCS0-9(VHT:BW80)
...
noise_flr[--96dBm] = 22
noise_flr[--95dBm] = 149
noise_flr[--94dBm] = 9
noise_flr[--93dBm] = 2
We're missing some spaces, and we're adding a minus sign ('-') on values
that are already negative signed integers.
Signed-off-by: Brian Norris
---
v2:
* combine strings to improve readability and save a tiny bit of code
On Mon, Dec 03, 2018 at 10:32:52AM -0800, Joe Perches wrote:
> It's be smaller object code and more intelligible source to use
> a single coalesced string with one sprintf
Sure, I can do that.
Here's a v2 that combines both sets of strings in that way. I'm not
resending the other patches, since they were only related in concept
(since I was referring to debugfs for implementing the nl80211 stuff),
but have no real dependency.
@Kalle: I can resend the others as a new series if that helps.
drivers/net/wireless/marvell/mwifiex/debugfs.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c
b/drivers/net/wireless/marvell/mwifiex/debugfs.c
index cce70252fd96..cbe4493b3266 100644
--- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
+++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
@@ -273,15 +273,13 @@ mwifiex_histogram_read(struct file *file, char __user
*ubuf,
"total samples = %d\n",
atomic_read(&phist_data->num_samples));
- p += sprintf(p, "rx rates (in Mbps): 0=1M 1=2M");
- p += sprintf(p, "2=5.5M 3=11M 4=6M 5=9M 6=12M\n");
- p += sprintf(p, "7=18M 8=24M 9=36M 10=48M 11=54M");
- p += sprintf(p, "12-27=MCS0-15(BW20) 28-43=MCS0-15(BW40)\n");
+ p += sprintf(p,
+"rx rates (in Mbps): 0=1M 1=2M 2=5.5M 3=11M 4=6M
5=9M 6=12M\n"
+"7=18M 8=24M 9=36M 10=48M 11=54M 12-27=MCS0-15(BW20)
28-43=MCS0-15(BW40)\n");
if (ISSUPP_11ACENABLED(priv->adapter->fw_cap_info)) {
- p += sprintf(p, "44-53=MCS0-9(VHT:BW20)");
- p += sprintf(p, "54-63=MCS0-9(VHT:BW40)");
- p += sprintf(p, "64-73=MCS0-9(VHT:BW80)\n\n");
+ p += sprintf(p,
+"44-53=MCS0-9(VHT:BW20) 54-63=MCS0-9(VHT:BW40)
64-73=MCS0-9(VHT:BW80)\n\n");
} else {
p += sprintf(p, "\n");
}
@@ -310,7 +308,7 @@ mwifiex_histogram_read(struct file *file, char __user *ubuf,
for (i = 0; i < MWIFIEX_MAX_NOISE_FLR; i++) {
value = atomic_read(&phist_data->noise_flr[i]);
if (value)
- p += sprintf(p, "noise_flr[-%02ddBm] = %d\n",
+ p += sprintf(p, "noise_flr[%02ddBm] = %d\n",
(int)(i-128), value);
}
for (i = 0; i < MWIFIEX_MAX_SIG_STRENGTH; i++) {
--
2.20.0.rc1.387.gf8505762e3-goog
Re: [EXT] [4.20 PATCH] Revert "mwifiex: restructure rx_reorder_tbl_lock usage"
On Fri, Nov 30, 2018 at 07:04:49PM +, Ganapathi Bhat wrote: > > This was clearly not tested well at all. I simply performed 'wget' in a > > loop and > > it fell over within a few seconds. > Sorry. I had run a iperf test before sharing the patch (no regression > observed). > It looks I failed to get this right for second time I will check this. Were you running on an 11n/ac network? IIUC, the particular code that's being blamed is only active with 11n+ features. You might also make sure you test with stuff like lockdep enabled, as that gives nice warnings and can even preemptively warn about potential problems before they even occur, if you use the right settings. (Some lockdep configurations can slow things down significantly, BTW.) Brian
[PATCH 1/3] mwifiex: debugfs: correct histogram spacing, formatting
Currently, snippets of this file look like:
rx rates (in Mbps): 0=1M 1=2M2=5.5M 3=11M 4=6M 5=9M 6=12M
7=18M 8=24M 9=36M 10=48M 11=54M12-27=MCS0-15(BW20) 28-43=MCS0-15(BW40)
44-53=MCS0-9(VHT:BW20)54-63=MCS0-9(VHT:BW40)64-73=MCS0-9(VHT:BW80)
...
noise_flr[--96dBm] = 22
noise_flr[--95dBm] = 149
noise_flr[--94dBm] = 9
noise_flr[--93dBm] = 2
We're missing some spaces, and we're adding a minus sign ('-') on values
that are already negative signed integers.
Signed-off-by: Brian Norris
---
drivers/net/wireless/marvell/mwifiex/debugfs.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c
b/drivers/net/wireless/marvell/mwifiex/debugfs.c
index cce70252fd96..65e48e16cb49 100644
--- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
+++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
@@ -273,14 +273,14 @@ mwifiex_histogram_read(struct file *file, char __user
*ubuf,
"total samples = %d\n",
atomic_read(&phist_data->num_samples));
- p += sprintf(p, "rx rates (in Mbps): 0=1M 1=2M");
+ p += sprintf(p, "rx rates (in Mbps): 0=1M 1=2M ");
p += sprintf(p, "2=5.5M 3=11M 4=6M 5=9M 6=12M\n");
- p += sprintf(p, "7=18M 8=24M 9=36M 10=48M 11=54M");
+ p += sprintf(p, "7=18M 8=24M 9=36M 10=48M 11=54M ");
p += sprintf(p, "12-27=MCS0-15(BW20) 28-43=MCS0-15(BW40)\n");
if (ISSUPP_11ACENABLED(priv->adapter->fw_cap_info)) {
- p += sprintf(p, "44-53=MCS0-9(VHT:BW20)");
- p += sprintf(p, "54-63=MCS0-9(VHT:BW40)");
+ p += sprintf(p, "44-53=MCS0-9(VHT:BW20) ");
+ p += sprintf(p, "54-63=MCS0-9(VHT:BW40) ");
p += sprintf(p, "64-73=MCS0-9(VHT:BW80)\n\n");
} else {
p += sprintf(p, "\n");
@@ -310,7 +310,7 @@ mwifiex_histogram_read(struct file *file, char __user *ubuf,
for (i = 0; i < MWIFIEX_MAX_NOISE_FLR; i++) {
value = atomic_read(&phist_data->noise_flr[i]);
if (value)
- p += sprintf(p, "noise_flr[-%02ddBm] = %d\n",
+ p += sprintf(p, "noise_flr[%02ddBm] = %d\n",
(int)(i-128), value);
}
for (i = 0; i < MWIFIEX_MAX_SIG_STRENGTH; i++) {
--
2.20.0.rc1.387.gf8505762e3-goog
[PATCH 3/3] mwifiex: add NL80211_STA_INFO_RX_BITRATE support
Comparing the existing TX_BITRATE parsing code (in
mwifiex_parse_htinfo()) with the RX bitrate histograms in debugfs.c, it
appears that the rxpd_rate and rxpd_htinfo fields have the same format.
At least, they give reasonable results when I parse them this way.
So this patch adds support for RX_BITRATE to our station info dump.
Along the way, I add legacy bitrate parsing into the same function,
using the debugfs code (mwifiex_histogram_read() and
mwifiex_adjust_data_rate()) as reference.
Additionally, to satisfy the requirements of
NL80211_STA_INFO_RX_BITRATE, I skip logging the bitrate of multicast
packets. This shouldn't add a lot of overhead to the RX path, as there
are already several similar 802.3 header checks in this same codepath.
We can also bias the branch behavior to favor unicast, as that's the
common performance-sensitive case.
I'd consider this support somewhat experimental, as I have zero
documentation from Marvell. But the existing driver code gives me good
reason to think this is correct.
I've tested this on a few different 802.11{a,b,g,n,ac} networks, and the
reported bitrates look good to me.
Signed-off-by: Brian Norris
---
I'd appreciate it if someone from Marvell could double check my work
here.
.../net/wireless/marvell/mwifiex/cfg80211.c | 26 +++
drivers/net/wireless/marvell/mwifiex/sta_rx.c | 13 ++
2 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 02b80ea232a7..1467af22e394 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -1333,6 +1333,28 @@ mwifiex_parse_htinfo(struct mwifiex_private *priv, u8
rateinfo, u8 htinfo,
rate->flags |= RATE_INFO_FLAGS_SHORT_GI;
}
}
+
+ /* Decode legacy rates for non-HT. */
+ if (!(htinfo & (BIT(0) | BIT(1 {
+ /* Bitrates in multiples of 100kb/s. */
+ static const int legacy_rates[] = {
+ [0] = 10,
+ [1] = 20,
+ [2] = 55,
+ [3] = 110,
+ [4] = 60, /* MWIFIEX_RATE_INDEX_OFDM0 */
+ [5] = 60,
+ [6] = 90,
+ [7] = 120,
+ [8] = 180,
+ [9] = 240,
+ [10] = 360,
+ [11] = 480,
+ [12] = 540,
+ };
+ if (rateinfo < ARRAY_SIZE(legacy_rates))
+ rate->legacy = legacy_rates[rateinfo];
+ }
}
/*
@@ -1414,6 +1436,10 @@ mwifiex_dump_station_info(struct mwifiex_private *priv,
/* bit rate is in 500 kb/s units. Convert it to 100kb/s units */
sinfo->txrate.legacy = rate * 5;
+ sinfo->filled |= BIT(NL80211_STA_INFO_RX_BITRATE);
+ mwifiex_parse_htinfo(priv, priv->rxpd_rate, priv->rxpd_htinfo,
+&sinfo->rxrate);
+
if (priv->bss_mode == NL80211_IFTYPE_STATION) {
sinfo->filled |= BIT_ULL(NL80211_STA_INFO_BSS_PARAM);
sinfo->bss_param.flags = 0;
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_rx.c
b/drivers/net/wireless/marvell/mwifiex/sta_rx.c
index 00fcbda09349..fb28a5c7f441 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_rx.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_rx.c
@@ -152,14 +152,17 @@ int mwifiex_process_rx_packet(struct mwifiex_private
*priv,
mwifiex_process_tdls_action_frame(priv, offset, rx_pkt_len);
}
- priv->rxpd_rate = local_rx_pd->rx_rate;
-
- priv->rxpd_htinfo = local_rx_pd->ht_info;
+ /* Only stash RX bitrate for unicast packets. */
+ if (likely(!is_multicast_ether_addr(rx_pkt_hdr->eth803_hdr.h_dest))) {
+ priv->rxpd_rate = local_rx_pd->rx_rate;
+ priv->rxpd_htinfo = local_rx_pd->ht_info;
+ }
if (GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_STA ||
GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_UAP) {
- adj_rx_rate = mwifiex_adjust_data_rate(priv, priv->rxpd_rate,
- priv->rxpd_htinfo);
+ adj_rx_rate = mwifiex_adjust_data_rate(priv,
+ local_rx_pd->rx_rate,
+ local_rx_pd->ht_info);
mwifiex_hist_data_add(priv, adj_rx_rate, local_rx_pd->snr,
local_rx_pd->nf);
}
--
2.20.0.rc1.387.gf8505762e3-goog
[PATCH 2/3] mwifiex: refactor mwifiex_parse_htinfo() for reuse
This function converts some firmware-specific parameters into cfg80211
'rate_info' structures. It currently assumes it's dealing only with TX
bitrate, but the RX bitrate looks to be the same, so refactor this
function to be reusable.
Signed-off-by: Brian Norris
---
.../net/wireless/marvell/mwifiex/cfg80211.c | 36 ++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index adc88433faa8..02b80ea232a7 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -1275,27 +1275,27 @@ mwifiex_cfg80211_change_virtual_intf(struct wiphy
*wiphy,
}
static void
-mwifiex_parse_htinfo(struct mwifiex_private *priv, u8 tx_htinfo,
+mwifiex_parse_htinfo(struct mwifiex_private *priv, u8 rateinfo, u8 htinfo,
struct rate_info *rate)
{
struct mwifiex_adapter *adapter = priv->adapter;
if (adapter->is_hw_11ac_capable) {
/* bit[1-0]: 00=LG 01=HT 10=VHT */
- if (tx_htinfo & BIT(0)) {
+ if (htinfo & BIT(0)) {
/* HT */
- rate->mcs = priv->tx_rate;
+ rate->mcs = rateinfo;
rate->flags |= RATE_INFO_FLAGS_MCS;
}
- if (tx_htinfo & BIT(1)) {
+ if (htinfo & BIT(1)) {
/* VHT */
- rate->mcs = priv->tx_rate & 0x0F;
+ rate->mcs = rateinfo & 0x0F;
rate->flags |= RATE_INFO_FLAGS_VHT_MCS;
}
- if (tx_htinfo & (BIT(1) | BIT(0))) {
+ if (htinfo & (BIT(1) | BIT(0))) {
/* HT or VHT */
- switch (tx_htinfo & (BIT(3) | BIT(2))) {
+ switch (htinfo & (BIT(3) | BIT(2))) {
case 0:
rate->bw = RATE_INFO_BW_20;
break;
@@ -1310,26 +1310,26 @@ mwifiex_parse_htinfo(struct mwifiex_private *priv, u8
tx_htinfo,
break;
}
- if (tx_htinfo & BIT(4))
+ if (htinfo & BIT(4))
rate->flags |= RATE_INFO_FLAGS_SHORT_GI;
- if ((priv->tx_rate >> 4) == 1)
+ if ((rateinfo >> 4) == 1)
rate->nss = 2;
else
rate->nss = 1;
}
} else {
/*
-* Bit 0 in tx_htinfo indicates that current Tx rate
-* is 11n rate. Valid MCS index values for us are 0 to 15.
+* Bit 0 in htinfo indicates that current rate is 11n. Valid
+* MCS index values for us are 0 to 15.
*/
- if ((tx_htinfo & BIT(0)) && (priv->tx_rate < 16)) {
- rate->mcs = priv->tx_rate;
+ if ((htinfo & BIT(0)) && (rateinfo < 16)) {
+ rate->mcs = rateinfo;
rate->flags |= RATE_INFO_FLAGS_MCS;
rate->bw = RATE_INFO_BW_20;
- if (tx_htinfo & BIT(1))
+ if (htinfo & BIT(1))
rate->bw = RATE_INFO_BW_40;
- if (tx_htinfo & BIT(2))
+ if (htinfo & BIT(2))
rate->flags |= RATE_INFO_FLAGS_SHORT_GI;
}
}
@@ -1375,7 +1375,8 @@ mwifiex_dump_station_info(struct mwifiex_private *priv,
sinfo->tx_packets = node->stats.tx_packets;
sinfo->tx_failed = node->stats.tx_failed;
- mwifiex_parse_htinfo(priv, node->stats.last_tx_htinfo,
+ mwifiex_parse_htinfo(priv, priv->tx_rate,
+node->stats.last_tx_htinfo,
&sinfo->txrate);
sinfo->txrate.legacy = node->stats.last_tx_rate * 5;
@@ -1401,7 +1402,8 @@ mwifiex_dump_station_info(struct mwifiex_private *priv,
HostCmd_ACT_GEN_GET, DTIM_PERIOD_I,
&priv->dtim_period, true);
- mwifiex_parse_htinfo(priv, priv->tx_htinfo, &sinfo->txrate);
+ mwifiex_parse_htinfo(priv, priv->tx_rate, priv->tx_htinfo,
+&sinfo->txrate);
sinfo->signal_avg = priv->bcn_rssi_avg;
sinfo->rx_bytes = priv->stats.rx_bytes;
--
2.20.0.rc1.387.gf8505762e3-goog
Re: [PATCH] ath10k: Move non-fatal warn logs to dbg level
On Wed, Nov 28, 2018 at 8:25 PM Govind Singh wrote: > > During driver load below warn logs are printed in the console. > Since driver may not implement all wmi events sent by fw and > all of them are non-fatal, move this log to debug level to > remove un-necessary warn message on console. > > [ 361.887230] ath10k_snoc a00.wifi: Unknown eventid: 16393 > [ 361.907037] ath10k_snoc a00.wifi: Unknown eventid: 237569 > > Signed-off-by: Govind Singh Alternatively, you could add these ignored commands to wmi-tlv.h, and just ignore them specifically. That's better documentation, in case new WMI events come along that *are* important to notice when we ignore them. There are other recent examples of that sort of patch being merged. Brian
[4.20 PATCH] Revert "mwifiex: restructure rx_reorder_tbl_lock usage"
This reverts commit 5188d5453bc9380ccd4ae1086138dd485d13aef2, because it
introduced lock recursion:
BUG: spinlock recursion on CPU#2, kworker/u13:1/395
lock: 0xffc0e28a47f0, .magic: dead4ead, .owner: kworker/u13:1/395,
.owner_cpu: 2
CPU: 2 PID: 395 Comm: kworker/u13:1 Not tainted 4.20.0-rc4+ #2
Hardware name: Google Kevin (DT)
Workqueue: MWIFIEX_RX_WORK_QUEUE mwifiex_rx_work_queue [mwifiex]
Call trace:
dump_backtrace+0x0/0x140
show_stack+0x20/0x28
dump_stack+0x84/0xa4
spin_bug+0x98/0xa4
do_raw_spin_lock+0x5c/0xdc
_raw_spin_lock_irqsave+0x38/0x48
mwifiex_flush_data+0x2c/0xa4 [mwifiex]
call_timer_fn+0xcc/0x1c4
run_timer_softirq+0x264/0x4f0
__do_softirq+0x1a8/0x35c
do_softirq+0x54/0x64
netif_rx_ni+0xe8/0x120
mwifiex_recv_packet+0xfc/0x10c [mwifiex]
mwifiex_process_rx_packet+0x1d4/0x238 [mwifiex]
mwifiex_11n_dispatch_pkt+0x190/0x1ac [mwifiex]
mwifiex_11n_rx_reorder_pkt+0x28c/0x354 [mwifiex]
mwifiex_process_sta_rx_packet+0x204/0x26c [mwifiex]
mwifiex_handle_rx_packet+0x15c/0x16c [mwifiex]
mwifiex_rx_work_queue+0x104/0x134 [mwifiex]
worker_thread+0x4cc/0x72c
kthread+0x134/0x13c
ret_from_fork+0x10/0x18
This was clearly not tested well at all. I simply performed 'wget' in a
loop and it fell over within a few seconds.
Fixes: 5188d5453bc9 ("mwifiex: restructure rx_reorder_tbl_lock usage")
Cc:
Cc: Ganapathi Bhat
Signed-off-by: Brian Norris
---
This is a 4.19 regression, but IMO the revert should be merged ASAP.
drivers/net/wireless/marvell/mwifiex/11n.c| 5 +-
.../wireless/marvell/mwifiex/11n_rxreorder.c | 96 ++-
.../net/wireless/marvell/mwifiex/uap_txrx.c | 3 -
3 files changed, 51 insertions(+), 53 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c
b/drivers/net/wireless/marvell/mwifiex/11n.c
index e2addd8b878b..5d75c971004b 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n.c
@@ -696,11 +696,10 @@ void mwifiex_11n_delba(struct mwifiex_private *priv, int
tid)
"Send delba to tid=%d, %pM\n",
tid, rx_reor_tbl_ptr->ta);
mwifiex_send_delba(priv, tid, rx_reor_tbl_ptr->ta, 0);
- spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
- flags);
- return;
+ goto exit;
}
}
+exit:
spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
}
diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
index 8e63d14c1e1c..5380fba652cc 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
@@ -103,8 +103,6 @@ static int mwifiex_11n_dispatch_pkt(struct mwifiex_private
*priv, void *payload)
* There could be holes in the buffer, which are skipped by the function.
* Since the buffer is linear, the function uses rotation to simulate
* circular buffer.
- *
- * The caller must hold rx_reorder_tbl_lock spinlock.
*/
static void
mwifiex_11n_dispatch_pkt_until_start_win(struct mwifiex_private *priv,
@@ -113,21 +111,25 @@ mwifiex_11n_dispatch_pkt_until_start_win(struct
mwifiex_private *priv,
{
int pkt_to_send, i;
void *rx_tmp_ptr;
+ unsigned long flags;
pkt_to_send = (start_win > tbl->start_win) ?
min((start_win - tbl->start_win), tbl->win_size) :
tbl->win_size;
for (i = 0; i < pkt_to_send; ++i) {
+ spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
rx_tmp_ptr = NULL;
if (tbl->rx_reorder_ptr[i]) {
rx_tmp_ptr = tbl->rx_reorder_ptr[i];
tbl->rx_reorder_ptr[i] = NULL;
}
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
if (rx_tmp_ptr)
mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
}
+ spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
/*
* We don't have a circular buffer, hence use rotation to simulate
* circular buffer
@@ -138,6 +140,7 @@ mwifiex_11n_dispatch_pkt_until_start_win(struct
mwifiex_private *priv,
}
tbl->start_win = start_win;
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
}
/*
@@ -147,8 +150,6 @@ mwifiex_11n_dispatch_pkt_until_start_win(struct
mwifiex_private *priv,
* The start window is adjusted automatically when a hole is located.
* Since the buffer is linear, the function uses rotation to simulate
* circular buffer.
- *
- * The caller must hold rx_reorder_tbl_lock spinlock.
*/
static void
mwifiex_11n_scan_and_
[PATCH] iw: dump 'rx bitrate' in link stats
We include it in 'station dump' but not 'link'.
Signed-off-by: Brian Norris
---
link.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/link.c b/link.c
index 0a323920ff2f..1ed7f631a121 100644
--- a/link.c
+++ b/link.c
@@ -121,6 +121,7 @@ static int print_link_sta(struct nl_msg *msg, void *arg)
[NL80211_STA_INFO_RX_PACKETS] = { .type = NLA_U32 },
[NL80211_STA_INFO_TX_PACKETS] = { .type = NLA_U32 },
[NL80211_STA_INFO_SIGNAL] = { .type = NLA_U8 },
+ [NL80211_STA_INFO_RX_BITRATE] = { .type = NLA_NESTED },
[NL80211_STA_INFO_TX_BITRATE] = { .type = NLA_NESTED },
[NL80211_STA_INFO_LLID] = { .type = NLA_U16 },
[NL80211_STA_INFO_PLID] = { .type = NLA_U16 },
@@ -160,6 +161,12 @@ static int print_link_sta(struct nl_msg *msg, void *arg)
printf("\tsignal: %d dBm\n",
(int8_t)nla_get_u8(sinfo[NL80211_STA_INFO_SIGNAL]));
+ if (sinfo[NL80211_STA_INFO_RX_BITRATE]) {
+ char buf[100];
+
+ parse_bitrate(sinfo[NL80211_STA_INFO_RX_BITRATE], buf,
sizeof(buf));
+ printf("\trx bitrate: %s\n", buf);
+ }
if (sinfo[NL80211_STA_INFO_TX_BITRATE]) {
char buf[100];
--
2.20.0.rc0.387.gc7a69e6b6c-goog
Re: [PATCH] qca-swiss-army-knife: ath10k-fwencoder: add new 'single-chan-info-per-channel'
On Fri, Nov 16, 2018 at 4:47 PM Brian Norris wrote: > This ancient Wifi still tells me I can send patches here, though I s/Wifi/Wiki/ Sorry, Brian > haven't seen that much :) > > https://github.com/mcgrof/qca-swiss-army-knife/wiki
[PATCH] qca-swiss-army-knife: ath10k-fwencoder: add new 'single-chan-info-per-channel'
Signed-off-by: Brian Norris
---
This ancient Wifi still tells me I can send patches here, though I
haven't seen that much :)
https://github.com/mcgrof/qca-swiss-army-knife/wiki
tools/scripts/ath10k/ath10k-fwencoder | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/scripts/ath10k/ath10k-fwencoder
b/tools/scripts/ath10k/ath10k-fwencoder
index be1e7844345d..848e92054ff5 100755
--- a/tools/scripts/ath10k/ath10k-fwencoder
+++ b/tools/scripts/ath10k/ath10k-fwencoder
@@ -63,7 +63,8 @@ ATH10K_FW_FEATURE_ALLOWS_MESH_BCAST = 16
ATH10K_FW_FEATURE_NO_PS = 17
ATH10K_FW_FEATURE_MGMT_TX_BY_REF = 18
ATH10K_FW_FEATURE_NON_BMI = 19
-ATH10K_FW_FEATURE_MAX = 20
+ATH10K_FW_FEATURE_SINGLE_CHAN_INFO_PER_CHANNEL = 20
+ATH10K_FW_FEATURE_MAX = 21
feature_map = {
'ext-wmi-mgmt-rx': ATH10K_FW_FEATURE_EXT_WMI_MGMT_RX,
@@ -87,6 +88,8 @@ feature_map = {
'no-ps': ATH10K_FW_FEATURE_NO_PS,
'mgmt-tx-by-ref': ATH10K_FW_FEATURE_MGMT_TX_BY_REF,
'non-bmi': ATH10K_FW_FEATURE_NON_BMI,
+'single-chan-info-per-channel':
+ATH10K_FW_FEATURE_SINGLE_CHAN_INFO_PER_CHANNEL,
}
# from enum ath10k_fw_wmi_op_version in ath10k/hw.h
--
2.19.1.1215.g8438c0b245-goog
Re: [PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"
On Fri, Nov 16, 2018 at 2:15 AM Kalle Valo wrote: > Thanks Brian for investigating this and providing the revert! I think Govind did most of the investigation, but I did at least do the hard work of submitting the revert ;)
Re: [PATCH] ath10k: support PCIe enter L1 state
Hi, On Thu, Nov 15, 2018 at 06:38:25AM +, Wen Gong wrote: > > -Original Message- > > From: ath10k On Behalf Of Brian Norris > > > > Is there some reason L1 was disabled in the first place? Was it known to be > > unreliable? > > Hi Brian, > It is a BUG for power, and it is not considered this BUG before. > So this change will fix the bug. I understand that the existing behavior is suboptimal for power, but on the other hand, code that goes out of its way to *clear* the L1 flag doesn't just pop up out of nowhere. Somebody clearly wrote that! If it just meant "we didn't verify L1 at first", then maybe it's fine to enable it unconditionally and see what happens, but if it meant "we tried L1 on some old chip and it caused problems", then it would be nice to know what those problems were. Or maybe that is hard to figure out, given there's no public git history tracking the original code, and we just need to try it out. Anyway, I'm giving it a try here, but I just wanted to ask :) Thanks, Brian
Re: [PATCH v4 2/3] dts: arm64/sdm845: Add WCN3990 WLAN module device node
Hi Govind,
On Mon, Nov 05, 2018 at 06:38:37PM +0530, Govind Singh wrote:
> Add device node for the ath10k SNOC platform driver probe
> and add resources required for WCN3990 on SDM845 soc.
>
> Signed-off-by: Govind Singh
> Reviewed-by: Brian Norris
> Tested-by: Brian Norris
> ---
> arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 8
> arch/arm64/boot/dts/qcom/sdm845.dtsi| 26 ++
> 2 files changed, 34 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index eedfaf8..c062c5c 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -440,3 +440,11 @@
> bias-pull-up;
> };
> };
> +
> +&wifi {
> + status = "okay";
> + vdd-0.8-cx-mx-supply = <&vreg_l5a_0p8>;
> + vdd-1.8-xo-supply = <&vreg_l7a_1p8>;
> + vdd-1.3-rfa-supply = <&vreg_l17a_1p3>;
> + vdd-3.3-ch0-supply = <&vreg_l25a_3p3>;
> +};
This node should be above the PINCTRL section.
Brian
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index b72bdb0..324be5b 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -87,6 +87,11 @@
> reg = <0 0x8620 0 0x2d0>;
> no-map;
> };
> +
> + wlan_msa_mem: memory@9670 {
> + reg = <0 0x9670 0 0x10>;
> + no-map;
> + };
> };
>
> cpus {
> @@ -1403,5 +1408,26 @@
> status = "disabled";
> };
> };
> +
> + wifi: wifi@1880 {
> + compatible = "qcom,wcn3990-wifi";
> + status = "disabled";
> + reg = <0x1880 0x80>;
> + reg-names = "membase";
> + memory-region = <&wlan_msa_mem>;
> + interrupts =
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ;
> + };
> };
> };
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
