Re: pull request: mt76 2018-11-30
Lorenzo Bianconi writes: >> Felix Fietkau writes: >> >> > here's my first pull request for 4.21 >> > >> > - Felix >> > >> > The following changes since commit >> > b72c51a58e6d63ef673ac96b8ab5bc98799c5f7b: >> > >> > brcmfmac: Fix out of bounds memory access during fw load (2018-11-29 >> > 17:33:10 +0200) >> > >> > are available in the Git repository at: >> > >> > https://github.com/nbd168/wireless tags/mt76-for-kvalo-2018-11-30 >> > >> > for you to fetch changes up to e28487ea84a9c081c6d8d7da319427f7fcc32ff5: >> > >> > mt76: replace sta_add/remove ops with common sta_state function >> > (2018-11-30 12:30:37 +0100) >> > >> > >> > first batch of mt76 patches for 4.21 >> > >> > * use the same firmware for mt76x2e and mt76x2u >> > * mt76x2 fixes >> > * mt76x0 fixes >> > * mt76x0e survey support >> > * more unification between mt76x2 and mt76x0 >> > * mt76x0e AP mode support >> > * mt76x0e DFS support >> > * rework and fix tx status handling for mt76x0 and mt76x2 >> > >> > >> >> I fast forwarded w-d-next to net-next and now there's a conflict. I did >> a test merge in the pending branch, please double check: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?h=pending=e69caab09bf98d0d8b559d06887364cc0090097c >> > > ack Thanks for checking, I'll pull this soon. -- Kalle Valo
Re: pull request: mt76 2018-11-30
Felix Fietkau writes:
> here's my first pull request for 4.21
>
> - Felix
>
> The following changes since commit b72c51a58e6d63ef673ac96b8ab5bc98799c5f7b:
>
> brcmfmac: Fix out of bounds memory access during fw load (2018-11-29
> 17:33:10 +0200)
>
> are available in the Git repository at:
>
> https://github.com/nbd168/wireless tags/mt76-for-kvalo-2018-11-30
>
> for you to fetch changes up to e28487ea84a9c081c6d8d7da319427f7fcc32ff5:
>
> mt76: replace sta_add/remove ops with common sta_state function (2018-11-30
> 12:30:37 +0100)
>
>
> first batch of mt76 patches for 4.21
>
> * use the same firmware for mt76x2e and mt76x2u
> * mt76x2 fixes
> * mt76x0 fixes
> * mt76x0e survey support
> * more unification between mt76x2 and mt76x0
> * mt76x0e AP mode support
> * mt76x0e DFS support
> * rework and fix tx status handling for mt76x0 and mt76x2
>
>
I fast forwarded w-d-next to net-next and now there's a conflict. I did
a test merge in the pending branch, please double check:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?h=pending=e69caab09bf98d0d8b559d06887364cc0090097c
This was the conflict:
diff --cc drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c
index 3f001bd6806c,b54a32397486..
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c
@@@ -264,21 -173,6 +173,24 @@@ static int mt76x2_get_antenna(struct ie
return 0;
}
++<<<<<<< HEAD
+static int
+mt76x2_set_rts_threshold(struct ieee80211_hw *hw, u32 val)
+{
+ struct mt76x02_dev *dev = hw->priv;
+
+ if (val != ~0 && val > 0x)
+ return -EINVAL;
+
+ mutex_lock(>mt76.mutex);
+ mt76x2_mac_set_tx_protection(dev, val);
+ mutex_unlock(>mt76.mutex);
+
+ return 0;
+}
+
++===
++>>>>>>> 1b0adb0ab8649a1ed44f9840724878cffaaa6896
const struct ieee80211_ops mt76x2_ops = {
.tx = mt76x02_tx,
.start = mt76x2_start,
I solved it by removing mt76x2_set_rts_threshold(). After there was also
a compilation error which I fixed like this:
diff --cc drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c
index 3f001bd6806c,b54a32397486..
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
index 3a70e5bf7d42..38bd466cff16 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
@@ -462,9 +462,9 @@ int mt76x02_set_rts_threshold(struct ieee80211_hw *hw, u32
val)
if (val != ~0 && val > 0x)
return -EINVAL;
- mutex_lock(>mutex);
+ mutex_lock(>mt76.mutex);
mt76x02_mac_set_tx_protection(dev, val);
- mutex_unlock(>mutex);
+ mutex_unlock(>mt76.mutex);
return 0;
}
--
Kalle Valo
Re: [PATCH v3 1/3] brcmfmac: add credit numbers updating support
Wright Feng wrote: > The credit numbers are static and tunable per chip in firmware side. > However the credit number may be changed that is based on packet pool > length and will send BRCMF_E_FIFO_CREDIT_MAP event to notify host driver > updates the credit numbers during interface up. > The purpose of this patch is making host driver has ability of updating > the credit numbers when receiving the BRCMF_E_FIFO_CREDIT_MAP event. > > Signed-off-by: Wright Feng Arend, what should I do with this patchset? -- https://patchwork.kernel.org/patch/10667393/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] brcmfmac: Call brcmf_dmi_probe before brcmf_of_probe
Hans de Goede wrote:
> ARM systems with UEFI may have both devicetree (of) and DMI data in this
> case we end up setting brcmf_mp_device.board_type twice.
>
> In this case we should prefer the devicetree data, because:
> 1) The devicerree data is more reliable
> 2) Some ARM systems (e.g. the Raspberry Pi 3 models) support both UEFI and
>classic uboot booting, the devicetree data is always there, so using it
>makes sure we ask for the same nvram file independent of how we booted.
>
> This commit moves the brcmf_dmi_probe call to before the brcmf_of_probe
> call, so that the latter can override the value of the first if both are
> set.
>
> Fixes: bd1e82bb420a ("brcmfmac: Set board_type from DMI on x86 based ...")
> Cc: Peter Robinson
> Tested-and-reported-by: Peter Robinson
> Signed-off-by: Hans de Goede
Patch applied to wireless-drivers-next.git, thanks.
554da3868eb1 brcmfmac: Call brcmf_dmi_probe before brcmf_of_probe
--
https://patchwork.kernel.org/patch/10695255/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH v2] brcmfmac: support STA info struct v7
Dan Haab wrote: > The newest firmwares provide STA info using v7 of the struct. As v7 > isn't backward compatible, a union is needed. > > Even though brcmfmac does not use any of the new info it's important to > provide the proper struct buffer. Without this change new firmwares will > fallback to the very limited v3 instead of something in between such as > v4. > > Signed-off-by: Dan Haab > Reviewed-by: Rafał Miłecki > Reviewed-by: Arend van Spriel Patch applied to wireless-drivers-next.git, thanks. 4282ff17e557 brcmfmac: support STA info struct v7 -- https://patchwork.kernel.org/patch/10676321/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH 05/15] iwlwifi: don't define OTP_LOW_IMAGE_SIZE per family, but per size
Luca Coelho writes:
> From: Luca Coelho
>
> Using OTP_LOW_IMAGE_SIZE_FAMILY_8000/9000/22000 only obfuscates the
> actual values, since these 3 are the same. Redefine the values per
> size so it's easier to understand and compare the different
> configurations.
>
> Signed-off-by: Luca Coelho
[...]
> --- a/drivers/net/wireless/intel/iwlwifi/cfg/1000.c
> +++ b/drivers/net/wireless/intel/iwlwifi/cfg/1000.c
> @@ -48,7 +48,7 @@
> static const struct iwl_base_params iwl1000_base_params = {
> .num_of_queues = IWLAGN_NUM_QUEUES,
> .max_tfd_queue_size = 256,
> - .eeprom_size = OTP_LOW_IMAGE_SIZE,
> + /* .eeprom_size = OTP_LOW_IMAGE_SIZE_2K, */
Why this is commented out?
--
Kalle Valo
Re: pull-request: iwlwifi-next 2018-11-23
Luca Coelho writes: > Hi Kalle, > > This is the second batch of patches intended for v4.21. This includes > only the last patchset I sent. Usual development work, mostly in the > debugging infrastructure and small fixes and cleanups. More details > about the contents in the tag description. > > I have sent this out before and kbuildbot reported success. > > Please let me know if there are any issues. > > Cheers, > Luca. > > > The following changes since commit 12d56175c89c8a8cc41e478775155709274ff733: > > Merge tag 'iwlwifi-next-for-kalle-2018-11-11' of > git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-next > (2018-11-15 16:59:01 +0200) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-next.git > tags/iwlwifi-next-for-kalle-2018-11-23 > > for you to fetch changes up to 520229e4b02dc4f5241a251a569fe0691a2c2e2c: > > iwlwifi: mvm: set HW capability VHT_EXT_NSS_BW (2018-11-23 13:01:08 +0200) > > > second batch of iwlwifi patches intended for v4.21 > > * New FW debugging infrastructure; > * Some more work on 802.11ax; > * Improve support for multiple RF modules with 22000 devices; > * Remove an unused FW parameter; > * Other debugging improvements; > > Pulled, thanks. -- Kalle Valo
Re: [RFC 0/5] add XDP support to mt76x2e/mt76x0e drivers
Lorenzo Bianconi writes: > This series is intended as a playground to start experimenting/developing > with XDP/eBPF over WiFi and collect ideas/concerns about it. > Introduce XDP support to mt76x2e/mt76x0e drivers. Currently supported > actions are: > - XDP_PASS > - XDP_ABORTED > - XDP_DROP > Introduce ndo_bpf mac80211 callback in order to to load a bpf > program into low level driver XDP rx hook. > This series has been tested through a simple bpf program (available here: > https://github.com/LorenzoBianconi/bpf-workspace/tree/master/mt76_xdp_stats) > used to count frame types received by the device. > Possible eBPF use cases could be: > - implement new statistics through bpf maps > - implement fast packet filtering (e.g in monitor mode) > - ... This is most likely a stupid question, but why do this in the driver and not in mac80211 so that all drivers could benefit from it? I guess there are reasons for that, I just can't figure that out. -- Kalle Valo
Re: [PATCH] ell:: build: bump SO version of libell
Jan Engelhardt writes: > Commit 0.13-7-g4a386a1 changed the ABI of l_signal_create. This > warrants a bump in the SO version. > --- > Makefile.am | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Makefile.am b/Makefile.am > index d115360..5a44f40 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -8,8 +8,8 @@ ACLOCAL_AMFLAGS = -I build-aux > # Interfaces added: CURRENT++ REVISION=0 AGE++ > # Interfaces removed:CURRENT++ REVISION=0 AGE=0 > > -ELL_CURRENT = 0 > -ELL_REVISION = 2 > +ELL_CURRENT = 1 > +ELL_REVISION = 0 > ELL_AGE = 0 > > pkginclude_HEADERS = ell/ell.h \ For what project is this? I can't figure out why this was sent to linux-wireless, maybe by mistake? -- Kalle Valo
Re: [PATCH] brcmfmac: Call brcmf_dmi_probe before brcmf_of_probe
Hans de Goede writes:
> ARM systems with UEFI may have both devicetree (of) and DMI data in this
> case we end up setting brcmf_mp_device.board_type twice.
>
> In this case we should prefer the devicetree data, because:
> 1) The devicerree data is more reliable
> 2) Some ARM systems (e.g. the Raspberry Pi 3 models) support both UEFI and
>classic uboot booting, the devicetree data is always there, so using it
>makes sure we ask for the same nvram file independent of how we booted.
>
> This commit moves the brcmf_dmi_probe call to before the brcmf_of_probe
> call, so that the latter can override the value of the first if both are
> set.
>
> Fixes: bd1e82bb420a ("brcmfmac: Set board_type from DMI on x86 based ...")
> Cc: Peter Robinson
> Tested-and-reported-by: Peter Robinson
> Signed-off-by: Hans de Goede
Note to myself: commit bd1e82bb420a is on wireless-drivers-next so need
to queue for 4.20.
--
Kalle Valo
Re: [PATCH 0/8] brcmfmac: chip related changes
Chi-Hsien Lin writes: > This patch series includes various chip-related changes: > * 43012 support > * 4373 saverestore support > * SDIO bus settings > * 4354 raw chipid > > Changelog: > V3: > - Update comments for patch 3. > - Add defines for sr_control0 register bits in patch 7. > V2: > - Update commit message for patch 1. > - Update comments for patch 2. > - Remove CY_4373_F1_MESBUSYCTRL from patch 3. > - Collapse patch 6 (43102 sr support) in 4 (43012 support). Add helper > functions. > - Remove sr_eng_en variable from patch 8. > - Collapse patch 10 and 11 in 9 (sdio_aos disable). As you submitted v4 an hour later I assume this one was a mistake and I'll drop it. -- Kalle Valo
Re: [PATCH FIX] brcmfmac: fix reporting support for 160 MHz channels
Rafał Miłecki wrote: > From: Rafał Miłecki > > Driver can report IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ so it's > important to provide valid & complete info about supported bands for > each channel. By default no support for 160 MHz should be assumed unless > firmware reports it for a given channel later. > > This fixes info passed to the userspace. Without that change userspace > could try to use invalid channel and fail to start an interface. > > Signed-off-by: Rafał Miłecki > Cc: sta...@vger.kernel.org Patch applied to wireless-drivers.git, thanks. d1fe6ad6f6bd brcmfmac: fix reporting support for 160 MHz channels -- https://patchwork.kernel.org/patch/10674521/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: wiki: tree labels in patches
Johannes Berg writes: > On Fri, 2018-11-16 at 10:46 +0200, Kalle Valo wrote: > >> Yes, I do see your FIX tag in patchwork: >> >> [ 31] [FIX] brcmfmac: fix reporting support for 160 MHz channels >> 2018-11-08 >> >> But "FIX" is a bit ambigous as not all fixes not go to wireless-drivers, >> they can also go to wireless-drivers-next. So I prefer using the release >> number (or name of the tree) like this: >> >> [PATCH 4.20] brcmfmac: fix reporting support for 160 MHz channels > > FWIW, davem/networking just use > > [PATCH net] > [PATCH net-next] > > which puts a bit more effort on the submitter but is a bit easier on the > maintainer I suppose. Also, not really a problem here, but it would help > disambiguate different trees on the same mailing list. I don't really > mind either way. Actually I already added that to the wiki[1] but made it optional just bacause it's harder for patch submitters who are not so familiar with our tree structure. But yes, I also like using the full tree name as the label, even more so as it would help me to automatically assign patches to correct maintainers in patchwork. [1] https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#tree_labels -- Kalle Valo
wiki: tree labels in patches
(changing subject for better visibility and trimming Cc) Rafał Miłecki writes: > On 2018-11-09 15:05, Kalle Valo wrote: >> Rafał Miłecki writes: >> >>> From: Rafał Miłecki >>> >>> Driver can report IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ so it's >>> important to provide valid & complete info about supported bands for >>> each channel. By default no support for 160 MHz should be assumed >>> unless >>> firmware reports it for a given channel later. >>> >>> This fixes info passed to the userspace. Without that change userspace >>> could try to use invalid channel and fail to start an interface. >>> >>> Signed-off-by: Rafał Miłecki >>> Cc: sta...@vger.kernel.org >> >> Should this be queued to 4.20? > > That's my suggestion. > > I try to mark fixes (patches for currently developed release) with an > extra FIX tag in a subject. Do you have any other method in mind that > would be preferred by you? Yes, I do see your FIX tag in patchwork: [ 31] [FIX] brcmfmac: fix reporting support for 160 MHz channels 2018-11-08 But "FIX" is a bit ambigous as not all fixes not go to wireless-drivers, they can also go to wireless-drivers-next. So I prefer using the release number (or name of the tree) like this: [PATCH 4.20] brcmfmac: fix reporting support for 160 MHz channels After seeing your question I added something about this to the wiki which hopefully helps others: https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#tree_labels -- Kalle Valo
Re: [PATCH FIX] brcmfmac: fix reporting support for 160 MHz channels
Rafał Miłecki writes: > On 2018-11-09 15:05, Kalle Valo wrote: >> Rafał Miłecki writes: >> >>> From: Rafał Miłecki >>> >>> Driver can report IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ so it's >>> important to provide valid & complete info about supported bands for >>> each channel. By default no support for 160 MHz should be assumed >>> unless >>> firmware reports it for a given channel later. >>> >>> This fixes info passed to the userspace. Without that change userspace >>> could try to use invalid channel and fail to start an interface. >>> >>> Signed-off-by: Rafał Miłecki >>> Cc: sta...@vger.kernel.org >> >> Should this be queued to 4.20? > > That's my suggestion. Ok, I'll queue this for 4.20. -- Kalle Valo
Re: pull-request: iwlwifi 2018-11-15
Luca Coelho writes: > Hi Kalle, > > This is the first batch of fixes for v4.20. More details about the > contents in the tag description. > > I have sent this out before and kbuildbot reported success. > > Please let me know if there are any issues. > > Cheers, > Luca. > > > The following changes since commit b374e8686fc35ae124e62dc78725ea656ba1ef8a: > > mt76: fix building without CONFIG_LEDS_CLASS (2018-11-06 18:46:33 +0200) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-next.git > tags/iwlwifi-for-kalle-2018-11-15 > > for you to fetch changes up to 5d041c46ccb9b48acc110e214beff5e2789311df: > > iwlwifi: mvm: don't use SAR Geo if basic SAR is not used (2018-11-15 > 23:50:59 +0200) > > > First batch of iwlwifi fixes for 4.20 > > * New FW debugging infrastructure; > * Some more work on 802.11ax; > * Improve support for multiple RF modules with 22000 devices; > * Remove an unused FW parameter; > * Other debugging improvements; > > Pulled, thanks. -- Kalle Valo
Re: [RFC v4 08/13] rtw88: debug files
Joe Perches writes: > On Thu, 2018-11-15 at 16:15 +0200, Kalle Valo wrote: >> Joe Perches writes: >> >> > On Sat, 2018-10-13 at 13:28 -0700, Joe Perches wrote: >> > > On Sat, 2018-10-13 at 18:23 +0300, Kalle Valo wrote: >> > > > Joe Perches writes: >> > [] >> > > > > It's very unusual to have _all_ the logging under a >> > > > > CONFIG__DEBUG >> > > > > config guard flag. >> > > > >> > > > For wireless drivers that is actually quite typical. >> > > >> > > No, it isn't. >> > > >> > > > IIRC at least ath6kl, ath9k and ath10k do that, most likely also >> > > > others. >> > > >> > > No, they don't. Check again. > [] >> > Kalle, did I miss a reply here? >> >> I didn't bother to waste my time after reading comments like "check >> again" and "look harder". > > You are the maintainer here and you give advice that others are > likely to follow. > > I was a bit frustrated after your fairly assertive, but wrong > statement that the other drivers you maintain do the same thing. Yeah, I can understand that. But do note that I'm a non-native english speaker with an always full inbox, I don't have time nor skills to write in a way expected by a native speaker. So please don't assume that I'm assertive or other meanings from my style of text, just consider that my writings as "simple english". Also I do a lot of mistakes, and when I do please just clearly explain it to me. I'm always grateful about that, all constructive help is very welcome. -- Kalle Valo
Re: pull-request: iwlwifi-next 2018-11-11
Luca Coelho writes: > This is the first batch of patches intended for v4.21. This includes > only the last patchset I sent. Usual development work, new PCI IDs and > small fixes and cleanups. More details about the contents in the tag > description. > > I have sent this out before and kbuildbot reported success. > > Please let me know if there are any issues. > > Cheers, > Luca. > > > The following changes since commit bb38177cb6c6dc973ad8b88f219742b29f3002f1: > > Merge ath-next from > git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git (2018-11-09 > 17:15:25 +0200) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-next.git > tags/iwlwifi-next-for-kalle-2018-11-11 > > for you to fetch changes up to 56b657f7f9c07421a4f910b7e2b382184f1ddbc8: > > iwlwifi: fw: use helper to determine whether to dump paging (2018-11-11 > 11:06:23 +0200) > > > First set of iwlwifi patches for 4.21 > > * PCI IDs for some new 9000-series cards; > * Improve antenna usage on connection problems; > * Some improvements in the debugging code; > * Other clean-ups and small fixes; > > Pulled, thanks. -- Kalle Valo
Re: [PATCH 08/16] iwlwifi: trans: parse and store debug ini TLVs
Luca Coelho writes: > From: Sara Sharon > > The new debug ini TLVs can be either packed into firmware > binary or written in external file. Support loading them > from both. Store the data per apply point. Apply point is > a point during driver runtime, where the TLV becomes active. > For example, a trigger of hardware error may be configured > to collect a subset of data pre-alive, as a opposed to HW > error that occurs after alive. > > Signed-off-by: Sara Sharon > Signed-off-by: Luca Coelho [...] > @@ -1749,6 +1762,10 @@ MODULE_PARM_DESC(lar_disable, "disable LAR > functionality (default: N)"); > module_param_named(uapsd_disable, iwlwifi_mod_params.uapsd_disable, uint, > 0644); > MODULE_PARM_DESC(uapsd_disable, >"disable U-APSD functionality bitmap 1: BSS 2: P2P Client > (default: 3)"); > +module_param_named(enable_ini, iwlwifi_mod_params.enable_ini, > +bool, S_IRUGO | S_IWUSR); > +MODULE_PARM_DESC(enable_ini, > + "Enable debug INI TLV FW debug infrastructure (default: 0"); The commit log doesn't mention anything about the module parameter and how/when it's supposed to be used, that would be nice to have. -- Kalle Valo
Re: [PATCH 1/6] brcmfmac: Remove firmware-loading code duplication
Hans de Goede writes: > Hi, > > On 05-11-18 16:05, Kalle Valo wrote: >> Arend van Spriel writes: >> >>> On 10/9/2018 2:47 PM, Hans de Goede wrote: >>>> brcmf_fw_request_next_item and brcmf_fw_request_done both have identical >>>> code to complete the fw-request depending on the item-type. >>>> >>>> This commit adds a new brcmf_fw_complete_request helper removing this code >>>> duplication. >>> >>> Reviewed-by: Arend van Spriel >> >> For some reason you commented on v1 but there was v2 posted already: >> >> https://patchwork.kernel.org/patch/10634355/ >> >> I guess I can take v2 still? > > Yes the only thing different in v2 was dropping the SPDX license header > for the new drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c > file and replacing it with the full ISC license text as seen in other > brcmfmac files. Nothing else was changed, so the review of v1 is > valid for v2 too. > > Arend had one very minor comment on the name of a variable in the fifth > patch, but that is not important so if you want to pick up v2 as is > go for it. > > Note the ISC license text is now in Torvald's tree as: > LICENSES/other/ISC > > So could even go with v1, but I guess you prefer to move all files of > a driver over to the SPDX headers in one go ... Correct, I really would prefer move to SPDX headers in one go. -- Kalle Valo
Re: [RFC v3 01/12] rtw88: main files
Tony Chuang writes:
>> -Original Message-
>> From: Kalle Valo [mailto:kv...@codeaurora.org]
>> Sent: Sunday, October 14, 2018 1:48 AM
>> To: Tony Chuang
>> Cc: Johannes Berg; larry.fin...@lwfinger.net; Pkshih; Andy Huang;
>> sgrus...@redhat.com; linux-wireless@vger.kernel.org
>> Subject: Re: [RFC v3 01/12] rtw88: main files
>>
>> Tony Chuang writes:
>>
>> >> > +static void rtw_watch_dog_work(struct work_struct *work)
>> >> > +{
>> >> > + struct rtw_dev *rtwdev = container_of(work, struct rtw_dev,
>> >> > + watch_dog_work.work);
>> >> > + struct rtw_vif *rtwvif;
>> >> > +
>> >> > + if (!rtw_flag_check(rtwdev, RTW_FLAG_RUNNING))
>> >> > + return;
>> >> > +
>> >> > + ieee80211_queue_delayed_work(rtwdev->hw,
>> >> >watch_dog_work,
>> >> > +RTW_WATCH_DOG_DELAY_TIME);
>> >>
>> >> You're aware of the power cost of waking up every 2 seconds? That's a
>> >> really bad idea, in general, at the very least you should use a more
>> >> power efficient scheduling here to combine with other wakeups
>> >> (round_jiffies_relative, or so).
>> >
>> > Yeah I knew it, but so far we can only work like this...
>> > Will use round_jiffies_relative to combine the CPU wakeups.
>>
>> Can you elaborate more why this horrible timer is needed? And it
>> definitely needs a comment in the code explaining the reason.
>>
>
>
> The watchdog timer is required for our devices to enhance the performance.
> It does a lot of tx/rx statistics processing for the hardware.
> Those information process routines help the devices to adapt to the
> environment.
>
> However, status polling every two seconds is not a good solution.
> But it makes drive simpler to be implemented.
>
> We will try to change it to interrupt mode.
> But it will take a lot of time to work on it.
> So, before it's done, I think we can leave the timer here.
Yeah, interrupt mode sounds like a much better idea. But if you have to
keep two second polling at least add a proper comment to the code
explaining what you said above.
--
Kalle Valo
Re: [RFC v4 08/13] rtw88: debug files
Joe Perches writes: > On Sat, 2018-10-13 at 13:28 -0700, Joe Perches wrote: >> On Sat, 2018-10-13 at 18:23 +0300, Kalle Valo wrote: >> > Joe Perches writes: > [] >> > > It's very unusual to have _all_ the logging under a CONFIG__DEBUG >> > > config guard flag. >> > >> > For wireless drivers that is actually quite typical. >> >> No, it isn't. >> >> > IIRC at least ath6kl, ath9k and ath10k do that, most likely also others. >> >> No, they don't. Check again. >> >> > > Typical debugging would dynamic debugging on a per-line instance andl >> > > this uses a single dev_dbg for all debugging. >> > >> > I don't recall seeing anyone using per-line dynamic debugging with >> > wireless drivers. The drivers are so complex that enabling one message >> > at a time doesn't really get you anywhere, that's why we mostly group >> > messages into similar groups (or levels) to make it easier to enable >> > certain debug messages. >> >> You should look harder. >> >> > > This seems unnecessarily complexity for a negative gain. >> > >> > I haven't reviewed the driver yet but from a quick look I don't see this >> > as a problem. >> >> It is unnecessarily complex. >> This saves one dereference per call, but is it really worth it? > > Kalle, did I miss a reply here? I didn't bother to waste my time after reading comments like "check again" and "look harder". -- Kalle Valo
Re: [RFC 00/19] wilc: added driver for wilc module
writes: > On 10/08/2018 12:38 AM, Kalle Valo wrote: >> Ajay Singh writes: >> >>> On Sat, 6 Oct 2018 15:45:41 +0300 >>> Kalle Valo wrote: >>> >>>> Ajay Singh writes: >>>> >>>>> This patch set contains the driver files from >>>>> 'driver/staging/wilc1000'. Renamed the driver from 'wilc1000' to >>>>> 'wilc' to have generic name, as the same driver will be used by >>>>> other wilc family members. >>>> >>>> I'm worried that the name 'wilc' is just too generic, I liked the >>>> original name wilc1000 much more. Quite often when we have a new >>>> generation of wireless devices there's also a new driver, so in the >>>> long run I'm worried that a generic name like 'wilc' could be a >>>> source of confusion. I think it's much smaller problem if >>>> 'wilc1000' (the driver) also supports wilc3000 (the device), people >>>> are already used to that. > >> If I'm understanding correctly you are worried that 'wilc1000-spi.ko' >> also supports wilc3000 devices, but I don't see that as a problem. I >> think it's very common (not just in wireless) that the marketing names >> don't always match with driver names. >> > > It's highly unlikely that microchip will have a new generation of wilc > devices other than wilc1000 and wilc3000, since a new family is > already in development. > And in case a new generation was developed, it will be best to support > it in the current driver because of the similarities between wilc > devices. > > I'm afraid that it might be more confusing for users to use wilc1000 > naming while they are using wilc3000 hardware. It's not only that the > name that is different from the marketing name, but it also refers to > another existing product. Well, I see it very differently. For example, if I google 'wilc1000' I get directly to the product page but with 'wilc' I get nothing useful. And I have been dealing with marketing for so long that "never say never" about what they will decide ;) So I'm still not convinced that renaming is a good idea. But actually my opinion doesn't matter here, as the rename should happen in the staging tree (when the driver is moved from staging to drivers/net/wireless it should be a simple rename, no other changes). So I'll leave this for Greg to decide if the rename is worthwhile or not. My vote would be a clear "no" :) -- Kalle Valo
Re: [EXTERNAL] [PATCH] ath9k_htc: add fw 1.4.1
(fixing quotation, PLEASE don't top post) Tom Psyborg writes: >> On 6 September 2018 at 17:32, Kalle Valo wrote: >> >> Tomislav Požega writes: >> >> > Add firmware v1.4.1 that supports up to 8 virtual APs and up >> > to 16 (current limit, at least on AR9271) client interfaces. >> > (1 AP + 15 STA, 4 AP + 12 STA etc) >> > >> > Signed-off-by: Tomislav Požega >> > --- >> > ath9k_htc/htc_7010-1.4.1.fw | Bin 0 -> 72812 bytes >> > ath9k_htc/htc_9271-1.4.1.fw | Bin 0 -> 51008 bytes >> > 2 files changed, 0 insertions(+), 0 deletions(-) >> > create mode 100644 ath9k_htc/htc_7010-1.4.1.fw >> > create mode 100644 ath9k_htc/htc_9271-1.4.1.fw >> >> Is this is an official release from the open-ath9k-htc-firmware >> project >> or just some custom build of yours? I'm asking because I don't see >> anything in the master branch since January: >> >> https://github.com/qca/open-ath9k-htc-firmware/commits/master >> >> IMHO Adrian Chadd (CCed) should be the one pushing the ath9k_htc >> firmware image to linux-firmware, unless the maintainer has >> changed of >> course. > > I was not sure if the fw images from open-ath9k-htc-firmware project > are still being pulled into linux-firmware tree, so I created this > build myself to meet the requirements for one of the patches I sent to > linux-wireless list. As ath9k_htc firmere is open it's easy to create backdoors etc and this is why I think that linux-firmware should not take ath9k_htc firmware binaries just from anyone, only from maintainers of open-ath9k-htc-firmware project. > Adrian, can you push the new fw version via official channel? Adrian, can you help here? -- Kalle Valo
Re: [PATCH] mac80211_hwsim: Timer should be initialized before device registered
(fixing top posting) Vasyl Vavrychuk writes: > On Thu, Oct 18, 2018 at 1:02 AM Vasyl Vavrychuk > wrote: >> >> From: Vasyl Vavrychuk >> >> Otherwise if network manager starts configuring Wi-Fi interface >> immidiatelly after getting notification of its creation, we will get >> NULL pointer dereference: >> >> BUG: unable to handle kernel NULL pointer dereference at (null) >> IP: [] hrtimer_active+0x28/0x50 >> ... >> Call Trace: >>[] ? hrtimer_try_to_cancel+0x27/0x110 >>[] ? hrtimer_cancel+0x15/0x20 >>[] ? mac80211_hwsim_config+0x140/0x1c0 [mac80211_hwsim] >> >> Signed-off-by: Vasyl Vavrychuk > > Does anyone have some feedback about this? Johannes has applied it six days ago: https://patchwork.kernel.org/patch/10646105/ https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git/commit/?id=a1881c9b8a1edef0a5ae1d5c1b61406fe3402114 To help the maintainers you can check yourself the status from patchwork: https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#checking_state_of_patches_from_patchwork -- Kalle Valo
Re: [PATCH FIX] brcmfmac: fix reporting support for 160 MHz channels
Rafał Miłecki writes: > From: Rafał Miłecki > > Driver can report IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ so it's > important to provide valid & complete info about supported bands for > each channel. By default no support for 160 MHz should be assumed unless > firmware reports it for a given channel later. > > This fixes info passed to the userspace. Without that change userspace > could try to use invalid channel and fail to start an interface. > > Signed-off-by: Rafał Miłecki > Cc: sta...@vger.kernel.org Should this be queued to 4.20? -- Kalle Valo
Re: [PATCH] brcmfmac: support STA info struct v7
Rafał Miłecki writes: > On Thu, 11 Oct 2018 at 22:21, Dan Haab wrote: >> The newest firmwares provide STA info using v7 of the struct. As v7 >> isn't backward compatible, a union is needed. >> >> Even though brcmfmac does not use any of the new info it's important to >> provide the proper struct buffer. Without this change new firmwares will >> fallback to the very limited v3 instead of something in between such as >> v4. >> >> Signed-off-by: Dan Haab > > It's too bad Broadcom's existing struct has been changed instead of > just being extended. > > The patch looks good to me though. I just wanted to share my opinion / > ping due to patch being marked as "Deferred". > > Reviewed-by: Rafał Miłecki Good that you brought this up, I wasn't sure what to do with it so I marked as Deferred. Arend, please let me know what I should do. -- Kalle Valo
Re: [PATCH] rtlwifi: rtl8192de: Fix misleading REG_MCUFWDL information
Shaokun Zhang wrote: > RT_TRACE shows REG_MCUFWDL value as a decimal value with a '0x' > prefix, which is somewhat misleading. > > Fix it to print hexadecimal, as was intended. > > Cc: Ping-Ke Shih > Cc: Kalle Valo > Signed-off-by: Shaokun Zhang > Acked-by: Ping-Ke Shih Patch applied to wireless-drivers-next.git, thanks. 7d129adff3af rtlwifi: rtl8192de: Fix misleading REG_MCUFWDL information -- https://patchwork.kernel.org/patch/10667863/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] brcmutil: print invalid chanspec when WARN-ing
Rafał Miłecki wrote: > From: Rafał Miłecki > > On one of my devices I got WARNINGs when brcmfmac tried to decode > chanspec. I couldn't tell if it was some unsupported format or just a > malformed value passed by a firmware. > > Print chanspec value so it's possible to debug a possible problem. > > Signed-off-by: Rafał Miłecki Patch applied to wireless-drivers-next.git, thanks. ae5848cb4511 brcmutil: print invalid chanspec when WARN-ing -- https://patchwork.kernel.org/patch/10657255/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()
Dan Carpenter wrote:
> It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
> of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
> (precision) so if the ssid doesn't have a NUL terminator this could lead
> to an overflow.
>
> Static analysis. Not tested.
>
> Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
> Signed-off-by: Dan Carpenter
Patch applied to wireless-drivers-next.git, thanks.
3d39e1bb1c88 wireless: airo: potential buffer overflow in sprintf()
--
https://patchwork.kernel.org/patch/10654389/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] brcmsmac: never log "tid x is not agg'able" by default
Ali MJ Al-Nasrawy wrote: > This message greatly spams the log under heavy Tx of frames with BK access > class which is especially true when operating as AP. It is also not > informative > as the "agg'ablity" of TIDs are set once and never change. > Fix this by logging only in debug mode. > > Signed-off-by: Ali MJ Al-Nasrawy Patch applied to wireless-drivers-next.git, thanks. 96fca788e578 brcmsmac: never log "tid x is not agg'able" by default -- https://patchwork.kernel.org/patch/10653373/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] qtnfmac: fix error handling in control path
Sergey Matyukevich wrote: > This patch fixes the following warnings: > > - smatch > drivers/net/wireless/quantenna/qtnfmac/commands.c:132 > qtnf_cmd_send_with_reply() warn: variable dereferenced before check 'resp' > (see line 117) > drivers/net/wireless/quantenna/qtnfmac/commands.c:716 > qtnf_cmd_get_sta_info() error: uninitialized symbol 'var_resp_len'. > drivers/net/wireless/quantenna/qtnfmac/commands.c:1668 > qtnf_cmd_get_mac_info() error: uninitialized symbol 'var_data_len'. > drivers/net/wireless/quantenna/qtnfmac/commands.c:1697 qtnf_cmd_get_hw_info() > error: uninitialized symbol 'info_len'. > drivers/net/wireless/quantenna/qtnfmac/commands.c:1753 > qtnf_cmd_band_info_get() error: uninitialized symbol 'info_len'. > drivers/net/wireless/quantenna/qtnfmac/commands.c:1782 > qtnf_cmd_send_get_phy_params() error: uninitialized symbol 'response_size'. > drivers/net/wireless/quantenna/qtnfmac/commands.c:2438 > qtnf_cmd_get_chan_stats() error: uninitialized symbol 'var_data_len'. > > - gcc-8.2.1 > drivers/net/wireless/quantenna/qtnfmac/commands.c: In function > 'qtnf_cmd_send_with_reply': > drivers/net/wireless/quantenna/qtnfmac/commands.c:133:54: error: 'resp' may > be used uninitialized in this function [-Werror=maybe-uninitialized] > > Reported-by: Dan Carpenter > Reported-by: Arnd Bergmann > Signed-off-by: Sergey Matyukevich Patch applied to wireless-drivers-next.git, thanks. 1066bd193d68 qtnfmac: fix error handling in control path -- https://patchwork.kernel.org/patch/10645681/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH v2 1/2] qtnfmac_pcie: use single PCIe driver for all platforms
Sergey Matyukevich wrote: > From: Igor Mitsyanko > > Single PCIe driver can identify hardware type by reading CHIP ID at > probe time and invoking a correct initialization sequence. > > Signed-off-by: Igor Mitsyanko 2 patches applied to wireless-drivers-next.git, thanks. b7da53cd6cd1 qtnfmac_pcie: use single PCIe driver for all platforms e401fa25cfa2 qtnfmac: add support for Topaz chipsets -- https://patchwork.kernel.org/patch/10643355/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH v4 1/2] brcmfmac: Add support for getting nvram contents from EFI variables
Hans de Goede wrote: > Various X86 laptops with a SDIO attached brcmfmac wifi chip, store the > nvram contents in a special EFI variable. This commit adds support for > getting nvram directly from this EFI variable, without the user needing > to manually copy it. > > This makes Wifi / Bluetooth work out of the box on these devices instead of > requiring manual setup. > > This has been tested on the following models: Acer Iconia Tab8 w1-810, > Acer One 10, Asus T100CHI, Asus T100HA, Asus T100TA, Asus T200TA and a > Lenovo Mixx 2 8. > > Tested-by: Hans de Goede > Signed-off-by: Hans de Goede 2 patches applied to wireless-drivers-next.git, thanks. ce2e6db554fa brcmfmac: Add support for getting nvram contents from EFI variables 29ec3394f0bd brcmfmac: Fix ccode from EFI nvram when necessary -- https://patchwork.kernel.org/patch/10636371/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH v2 1/6] brcmfmac: Remove firmware-loading code duplication
Hans de Goede wrote: > brcmf_fw_request_next_item and brcmf_fw_request_done both have identical > code to complete the fw-request depending on the item-type. > > This commit adds a new brcmf_fw_complete_request helper removing this code > duplication. > > Signed-off-by: Hans de Goede 6 patches applied to wireless-drivers-next.git, thanks. a1a3b7621638 brcmfmac: Remove firmware-loading code duplication 5b587496dc63 brcmfmac: Remove recursion from firmware load error handling eae8e50669e1 brcmfmac: Add support for first trying to get a board specific nvram file 0ad4b55b2f29 brcmfmac: Set board_type used for nvram file selection to machine-compatible bd1e82bb420a brcmfmac: Set board_type from DMI on x86 based machines 55e491edbf14 brcmfmac: Cleanup brcmf_fw_request_done() -- https://patchwork.kernel.org/patch/10634355/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH v2] mt76: fix building without CONFIG_LEDS_CLASS
Lorenzo Bianconi wrote:
> From: Arnd Bergmann
>
> When CONFIG_LEDS_CLASS is disabled, or it is a loadable module while
> mt76 is built-in, we run into a link error:
>
> drivers/net/wireless/mediatek/mt76/mac80211.o: In function
> `mt76_register_device':
> mac80211.c:(.text+0xb78): relocation truncated to fit: R_AARCH64_CALL26
> against undefined symbol `devm_of_led_classdev_register'
>
> We don't really need a hard dependency here as the driver can presumably
> work just fine without LEDs, so this follows the iwlwifi example and
> adds a separate Kconfig option for the LED support, this will be available
> whenever it will link, and otherwise the respective code gets left out from
> the driver object.
>
> Fixes: 17f1de56df05 ("mt76: add common code shared between multiple chipsets")
> Signed-off-by: Arnd Bergmann
> Signed-off-by: Lorenzo Bianconi
Patch applied to wireless-drivers.git, thanks.
b374e8686fc3 mt76: fix building without CONFIG_LEDS_CLASS
--
https://patchwork.kernel.org/patch/10668565/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH 4.20 FIX] brcmutil: really fix decoding channel info for 160 MHz bandwidth
Rafał Miłecki wrote:
> From: Rafał Miłecki
>
> Previous commit /adding/ support for 160 MHz chanspecs was incomplete.
> It didn't set bandwidth info and didn't extract control channel info. As
> the result it was also using uninitialized "sb" var.
>
> This change has been tested for two chanspecs found to be reported by
> some devices/firmwares:
> 1) 60/160 (0xee32)
>Before: chnum:50 control_ch_num:36
> After: chnum:50 control_ch_num:60
> 2) 120/160 (0xed72)
>Before: chnum:114 control_ch_num:100
> After: chnum:114 control_ch_num:120
>
> Fixes: 330994e8e8ec ("brcmfmac: fix for proper support of 160MHz bandwidth")
> Signed-off-by: Rafał Miłecki
Patch applied to wireless-drivers.git, thanks.
3401d42c7ea2 brcmutil: really fix decoding channel info for 160 MHz bandwidth
--
https://patchwork.kernel.org/patch/10657205/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] ath9k: Fix a locking bug in ath9k_add_interface()
Dan Carpenter wrote:
> We tried to revert commit d9c52fd17cb4 ("ath9k: fix tx99 with monitor
> mode interface") but accidentally missed part of the locking change.
>
> The lock has to be held earlier so that we're holding it when we do
> "sc->tx99_vif = vif;" and also there in the current code there is a
> stray unlock before we have taken the lock.
>
> Fixes: 6df0580be8bc ("ath9k: add back support for using active monitor
> interfaces for tx99")
> Signed-off-by: Dan Carpenter
Patch applied to wireless-drivers.git, thanks.
461cf0360574 ath9k: Fix a locking bug in ath9k_add_interface()
--
https://patchwork.kernel.org/patch/10650123/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH 1/5] ath9k: dynack: use authentication messages for 'late' ack
Lorenzo Bianconi wrote: > In order to properly support dynack in ad-hoc mode running > wpa_supplicant, take into account authentication frames for > 'late ack' detection. This patch has been tested on devices > mounted on offshore high-voltage stations connected through > ~24Km link > > Reported-by: Koen Vandeputte > Tested-by: Koen Vandeputte > Signed-off-by: Lorenzo Bianconi > Signed-off-by: Kalle Valo 5 patches applied to ath-next branch of ath.git, thanks. 3831a2a0010c ath9k: dynack: use authentication messages for 'late' ack 5e3d4718b157 ath9k: dynack: move debug log after buffer increments 9d3d65a91f02 ath9k: dynack: check da->enabled first in sampling routines 0c60c490830a ath9k: dynack: make ewma estimation faster 55bb78d265c6 ath9k: dynack: remove 'experimental' tag -- https://patchwork.kernel.org/patch/10666101/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH 01/16] wil6210: remove fake support for RXHASH
Maya Erez wrote: > Setting the same fake hash to all skbs prevents > distributing different flows to different CPU cores. > > Signed-off-by: Hamad Kadmany > Signed-off-by: Lior David > Signed-off-by: Maya Erez > Signed-off-by: Kalle Valo 15 patches applied to ath-next branch of ath.git, thanks. a078c4cf0197 wil6210: remove fake support for RXHASH d083b2e2b7db wil6210: fix reset flow for Talyn-mb cbebe277beb1 wil6210: increase RX rings and RX buff array size 61e5ec044748 wil6210: make sure Rx ring sizes are correlated e41ab937d47b wil6210: add recovery for FW error while in AP mode 664497400c89 wil6210: fix memory leak in wil_find_tx_bcast_2 e1b43407c034 wil6210: refactor disconnect flow b571e71bcb98 wil6210: notify cqm packet loss on disable_ap_sme ac0e541ab2f2 wil6210: add general initialization/size checks 84ec040d0fb2 wil6210: fix debugfs memory access alignment 04de15010aa4 wil6210: fix L2 RX status handling 7c69709f8ed2 wil6210: fix RGF_CAF_ICR address for Talyn-MB a834df7497b4 wil6210: remove unnecessary alignment code from rx flow 6470f31927b4 wil6210: fix freeing of rx buffers in EDMA mode dc57731dbd53 wil6210: fix locking in wmi_call -- https://patchwork.kernel.org/patch/10662235/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH 07/16] wil6210: refactor disconnect flow
me...@codeaurora.org writes:
> On 2018-11-06 14:28, Kalle Valo wrote:
>> me...@codeaurora.org writes:
>>
>>> On 2018-11-06 12:30, Kalle Valo wrote:
>>>> Maya Erez writes:
>>>>
>>>>> From: Ahmad Masri
>>>>>
>>>>> Separate sending command to the fw from the event handling
>>>>> function to
>>>>> simplify the disconnect flow and track the from_event flag
>>>>> correctly.
>>>>>
>>>>> Signed-off-by: Ahmad Masri
>>>>> Signed-off-by: Maya Erez
>>>>
>>>> [...]
>>>>
>>>>> +static int wil_disconnect_cid(struct wil6210_vif *vif, int cid,
>>>>> + u16 reason_code)
>>>>> +__acquires(>tid_rx_lock) __releases(>tid_rx_lock)
>>>>> +{
>>>>> + struct wil6210_priv *wil = vif_to_wil(vif);
>>>>> + struct wireless_dev *wdev = vif_to_wdev(vif);
>>>>> + struct wil_sta_info *sta = >sta[cid];
>>>>> + bool del_sta = false;
>>>>> +
>>>>> + might_sleep();
>>>>> + wil_dbg_misc(wil, "disconnect_cid: CID %d, MID %d, status %d\n",
>>>>> + cid, sta->mid, sta->status);
>>>>> +
>>>>> + if (sta->status == wil_sta_unused)
>>>>> + return 0;
>>>>> +
>>>>> + if (vif->mid != sta->mid) {
>>>>> + wil_err(wil, "STA MID mismatch with VIF MID(%d)\n", vif->mid);
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + /* inform lower layers */
>>>>> + if (wdev->iftype == NL80211_IFTYPE_AP && disable_ap_sme)
>>>>> + del_sta = true;
>>>>> +
>>>>> + /* disconnect by sending command disconnect/del_sta and wait
>>>>> + * synchronously for WMI_DISCONNECT_EVENTID event.
>>>>> + */
>>>>> + return wmi_disconnect_sta(vif, sta->addr, reason_code, del_sta);
>>>>> +}
>>>>
>>>> I don't get use of __acquires() and __releases() in this function. I
>>>> see
>>>> similar pattern already in wil6210 but care to explain why this is
>>>> needed? I don't see the function even accessing tid_rx_lock so I'm
>>>> very
>>>> confused.
>>>
>>> I assume it is a copy / paste leftover that we missed in the code
>>> review. We will remove it.
>>
>> Actually I already removed the annotations from the pending branch and
>> no need to resend, it's faster that way. Please double check if you
>> can,
>> unfortunately I cannot provide a direct link cgit doesn't show the new
>> commit yet.
>
> In such a case you can go ahead and apply the patches without
> "wil6210: ignore HALP ICR if already handled". I'll upstream its fixed
> version
> in the next set of wil6210 patches.
Ok, I dropped "wil6210: ignore HALP ICR if already handled" and I'm
planning to apply the rest.
--
Kalle Valo
Re: [PATCH 07/16] wil6210: refactor disconnect flow
me...@codeaurora.org writes:
> On 2018-11-06 12:30, Kalle Valo wrote:
>> Maya Erez writes:
>>
>>> From: Ahmad Masri
>>>
>>> Separate sending command to the fw from the event handling function to
>>> simplify the disconnect flow and track the from_event flag correctly.
>>>
>>> Signed-off-by: Ahmad Masri
>>> Signed-off-by: Maya Erez
>>
>> [...]
>>
>>> +static int wil_disconnect_cid(struct wil6210_vif *vif, int cid,
>>> + u16 reason_code)
>>> +__acquires(>tid_rx_lock) __releases(>tid_rx_lock)
>>> +{
>>> + struct wil6210_priv *wil = vif_to_wil(vif);
>>> + struct wireless_dev *wdev = vif_to_wdev(vif);
>>> + struct wil_sta_info *sta = >sta[cid];
>>> + bool del_sta = false;
>>> +
>>> + might_sleep();
>>> + wil_dbg_misc(wil, "disconnect_cid: CID %d, MID %d, status %d\n",
>>> +cid, sta->mid, sta->status);
>>> +
>>> + if (sta->status == wil_sta_unused)
>>> + return 0;
>>> +
>>> + if (vif->mid != sta->mid) {
>>> + wil_err(wil, "STA MID mismatch with VIF MID(%d)\n", vif->mid);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* inform lower layers */
>>> + if (wdev->iftype == NL80211_IFTYPE_AP && disable_ap_sme)
>>> + del_sta = true;
>>> +
>>> + /* disconnect by sending command disconnect/del_sta and wait
>>> +* synchronously for WMI_DISCONNECT_EVENTID event.
>>> +*/
>>> + return wmi_disconnect_sta(vif, sta->addr, reason_code, del_sta);
>>> +}
>>
>> I don't get use of __acquires() and __releases() in this function. I
>> see
>> similar pattern already in wil6210 but care to explain why this is
>> needed? I don't see the function even accessing tid_rx_lock so I'm very
>> confused.
>
> I assume it is a copy / paste leftover that we missed in the code
> review. We will remove it.
Actually I already removed the annotations from the pending branch and
no need to resend, it's faster that way. Please double check if you can,
unfortunately I cannot provide a direct link cgit doesn't show the new
commit yet.
--
Kalle Valo
Re: [PATCH 07/16] wil6210: refactor disconnect flow
Maya Erez writes:
> From: Ahmad Masri
>
> Separate sending command to the fw from the event handling function to
> simplify the disconnect flow and track the from_event flag correctly.
>
> Signed-off-by: Ahmad Masri
> Signed-off-by: Maya Erez
[...]
> +static int wil_disconnect_cid(struct wil6210_vif *vif, int cid,
> + u16 reason_code)
> +__acquires(>tid_rx_lock) __releases(>tid_rx_lock)
> +{
> + struct wil6210_priv *wil = vif_to_wil(vif);
> + struct wireless_dev *wdev = vif_to_wdev(vif);
> + struct wil_sta_info *sta = >sta[cid];
> + bool del_sta = false;
> +
> + might_sleep();
> + wil_dbg_misc(wil, "disconnect_cid: CID %d, MID %d, status %d\n",
> + cid, sta->mid, sta->status);
> +
> + if (sta->status == wil_sta_unused)
> + return 0;
> +
> + if (vif->mid != sta->mid) {
> + wil_err(wil, "STA MID mismatch with VIF MID(%d)\n", vif->mid);
> + return -EINVAL;
> + }
> +
> + /* inform lower layers */
> + if (wdev->iftype == NL80211_IFTYPE_AP && disable_ap_sme)
> + del_sta = true;
> +
> + /* disconnect by sending command disconnect/del_sta and wait
> + * synchronously for WMI_DISCONNECT_EVENTID event.
> + */
> + return wmi_disconnect_sta(vif, sta->addr, reason_code, del_sta);
> +}
I don't get use of __acquires() and __releases() in this function. I see
similar pattern already in wil6210 but care to explain why this is
needed? I don't see the function even accessing tid_rx_lock so I'm very
confused.
--
Kalle Valo
Re: [PATCH 13/16] wil6210: ignore HALP ICR if already handled
Maya Erez writes:
> HALP ICR is set as long as the FW should stay awake.
> To prevent its multiple handling the driver masks this IRQ bit.
> However, if there is a different MISC ICR before the driver clears
> this bit, there is a risk of race condition between HALP mask and
> unmask. This race leads to HALP timeout, in case it is mistakenly
> masked.
> Add an atomic flag to indicate if HALP ICR should be handled.
>
> Signed-off-by: Maya Erez
[...]
> --- a/drivers/net/wireless/ath/wil6210/interrupt.c
> +++ b/drivers/net/wireless/ath/wil6210/interrupt.c
> @@ -575,10 +575,14 @@ static irqreturn_t wil6210_irq_misc(int irq, void
> *cookie)
> }
>
> if (isr & BIT_DMA_EP_MISC_ICR_HALP) {
> - wil_dbg_irq(wil, "irq_misc: HALP IRQ invoked\n");
> - wil6210_mask_halp(wil);
> isr &= ~BIT_DMA_EP_MISC_ICR_HALP;
> - complete(>halp.comp);
> + if (atomic_read(>halp.handle_icr)) {
> + /* no need to handle HALP ICRs until next vote */
> + atomic_set(>halp.handle_icr, 0);
atomic_read() followed by atomic_set() is IMHO not really atomic :) I
would assume there's a function to reset the variable really in atomic
way.
--
Kalle Valo
Re: [PATCH 1/6] brcmfmac: Remove firmware-loading code duplication
Arend van Spriel writes: > On 10/9/2018 2:47 PM, Hans de Goede wrote: >> brcmf_fw_request_next_item and brcmf_fw_request_done both have identical >> code to complete the fw-request depending on the item-type. >> >> This commit adds a new brcmf_fw_complete_request helper removing this code >> duplication. > > Reviewed-by: Arend van Spriel For some reason you commented on v1 but there was v2 posted already: https://patchwork.kernel.org/patch/10634355/ I guess I can take v2 still? -- Kalle Valo
Re: [PATCH] mt76x0: use band parameter for LC calibration
Stanislaw Gruszka writes:
> On Thu, Oct 25, 2018 at 06:18:33PM +0200, Stanislaw Gruszka wrote:
>> We use always 1 as band parameter for MCU_CAL_LC, this break 2GHz,
>> we should use 0 for this band instead.
>>
>> Patch fixes problems happened sometimes when try to associate with 2GHz
>> AP and manifest by errors like below:
>>
>> [14680.920823] wlan0: authenticate with 18:31:bf:c0:51:b0
>> [14681.109506] wlan0: send auth to 18:31:bf:c0:51:b0 (try 1/3)
>> [14681.310454] wlan0: send auth to 18:31:bf:c0:51:b0 (try 2/3)
>> [14681.518469] wlan0: send auth to 18:31:bf:c0:51:b0 (try 3/3)
>> [14681.726499] wlan0: authentication with 18:31:bf:c0:51:b0 timed out
>>
>> Fixes: 9aec146d0f6b ("mt76x0: pci: introduce mt76x0_phy_calirate routine")
>> Signed-off-by: Stanislaw Gruszka
>> ---
>> This is for 4.20.
>
> Actually it is not needed for 4.20, bacause the new calibrate code is
> not use for USB in 4.20. It start to be used since:
>
> commit e868a944c55b1f42303ab2941dc1aaada9a3570c
> Author: Lorenzo Bianconi
> Date: Mon Oct 15 14:18:05 2018 +0200
>
> mt76x0: phy: unify calibration between mt76x0u and mt76x0e
So what should happen to this patch? Will Felix take it?
--
Kalle Valo
Re: [PATCH] mt76: fix building without CONFIG_LEDS_CLASS
Lorenzo Bianconi writes:
>> Lorenzo Bianconi writes:
>>
>> > From: Arnd Bergmann
>> >
>> > When CONFIG_LEDS_CLASS is disabled, or it is a loadable module while
>> > mt76 is built-in, we run into a link error:
>> >
>> > drivers/net/wireless/mediatek/mt76/mac80211.o: In function
>> > `mt76_register_device':
>> > mac80211.c:(.text+0xb78): relocation truncated to fit:
>> > R_AARCH64_CALL26 against undefined symbol
>> > `devm_of_led_classdev_register'
>> >
>> > We don't really need a hard dependency here as the driver can presumably
>> > work just fine without LEDs, so this follows the iwlwifi example and
>> > adds a separate Kconfig option for the LED support, this will be available
>> > whenever it will link, and otherwise the respective code gets left out from
>> > the driver object.
>> >
>> > Fixes: 17f1de56df05 ("mt76: add common code shared between multiple
>> > chipsets")
>> > Signed-off-by: Arnd Bergmann
>> > Signed-off-by: Lorenzo Bianconi
>>
>> Should this go to 4.20? A linker error is pretty bad, even though I
>> think this is few months old issue already.
>
> I guess so. The patch is based on top of Felix's repository so it apply with
> a
> 'fuzz' on net-next/wireless-drivers-next but the patch is ok. Do I need to
> resend or it is ok?
Doesn't seem to apply to wireless-drivers (which I fast forwarded to
v4.20-rc1 today):
Failed to apply the patch: ['git', 'am', '-s', '-3'] failed: 128
fatal: sha1 information is lacking or useless
(drivers/net/wireless/mediatek/mt76/mt76x2/pci_init.c).
error: could not build fake ancestor
So please rebase and resend.
--
Kalle Valo
Re: [PATCH] mt76: fix building without CONFIG_LEDS_CLASS
Lorenzo Bianconi writes:
> From: Arnd Bergmann
>
> When CONFIG_LEDS_CLASS is disabled, or it is a loadable module while
> mt76 is built-in, we run into a link error:
>
> drivers/net/wireless/mediatek/mt76/mac80211.o: In function
> `mt76_register_device':
> mac80211.c:(.text+0xb78): relocation truncated to fit: R_AARCH64_CALL26
> against undefined symbol `devm_of_led_classdev_register'
>
> We don't really need a hard dependency here as the driver can presumably
> work just fine without LEDs, so this follows the iwlwifi example and
> adds a separate Kconfig option for the LED support, this will be available
> whenever it will link, and otherwise the respective code gets left out from
> the driver object.
>
> Fixes: 17f1de56df05 ("mt76: add common code shared between multiple chipsets")
> Signed-off-by: Arnd Bergmann
> Signed-off-by: Lorenzo Bianconi
Should this go to 4.20? A linker error is pretty bad, even though I
think this is few months old issue already.
And are conflicts with -next version of mt76 likely?
--
Kalle Valo
Re: [PATCH] mt76x0: run calibration after scanning
Stanislaw Gruszka writes:
> On Mon, Oct 29, 2018 at 04:25:31PM +0200, Kalle Valo wrote:
>> Stanislaw Gruszka writes:
>>
>> > If we are associated and scanning is performed , sw_scan_complete callback
>> > is done after we get back to operating channel, so we do not perform
>> > phy calibration and queue cal work. Fix this by run calibration from
>> > sw_scan_complete().
>> >
>> > Fixes: bbd10586f0df ("mt76x0: phy: do not run calibration during channel
>> > switch")
>> > Signed-off-by: Stanislaw Gruszka
>> > ---
>> > This is for 4.20
>>
>> So what are the symptoms from user's point of view? Is this a
>> regression?
>
> We do not perform gain calibration any longer if somebody will
> request scan after association. This is formally a regression,
> but calibration code changed a lot, so perhaps this can be dropped
> for 4.20 and eventually go through -stable if I can confirm
> it fixes performance problems.
To me that would be a much better option, less conflicts that way.
--
Kalle Valo
Re: [RFC v3] cfg80211: add peer measurement with FTM API
Johannes Berg writes:
> On Sat, 2018-10-13 at 12:55 +0300, Kalle Valo wrote:
>>
>> > struct cfg80211_ops {
>> >int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
>>
>> It would be nice to document if these ops can sleep or not.
>
> I don't think we've documented that *anywhere* in cfg80211, but
> everything can sleep there ...
>
> At the mac80211 level things are different, so there we do document a
> case-by-case basis.
Makes sense, sorry for the noise.
--
Kalle Valo
Re: [PATCH 5/5] qtnfmac: add support for Topaz chipsets
Sergey Matyukevich writes: >> > +config QTNFMAC_TOPAZ_PCIE >> > + tristate "Quantenna QSR1000/QSR2000 PCIe support" >> > + default n >> > + depends on PCI && CFG80211 >> > + select QTNFMAC >> > + select FW_LOADER >> > + select CRC32 >> > + help >> > + This option adds support for wireless adapters based on Quantenna >> > + 802.11ac QSR1000/QSR2000 (aka Topaz) FullMAC chipset >> > + running over PCIe. >> > + >> > + If you choose to build it as a module, two modules will be built: >> > + qtnfmac.ko and qtnfmac_pcie.ko. > >> I'm not really fond of adding a Kconfig option for every supported >> hardware version unless there are very good reasons (memory savings >> etc). So is this really needed? > > Yes, the idea was to save some memory building pcie backend for > the requested chip family only. > >> A much better approach would be to have a generic QTNFMAC_PCIE option >> which can be used to include or exclude all PCI code. > > Ok, sounds reasonable. Three Kconfig options for the single qtnfmac_pcie > module is way too much. We will drop chipset specific Kconfig knobs in > v2, keeping single Kconfig option for qtnfmac_pcie backend as a whole. Great, thanks. -- Kalle Valo
Re: [PATCH 03/19] wilc: add host_interface.h
Ajay Singh writes: > On 10/9/2018 4:06 PM, Johannes Berg wrote: >> On Tue, 2018-10-09 at 16:04 +0530, Ajay Singh wrote: >> >>>>> +typedef void (*wilc_remain_on_chan_expired)(void *, u32); >>>>> +typedef void (*wilc_remain_on_chan_ready)(void *); >>> I think as per coding style the typedef for function pointer are allowed. >> True, I guess, but why do you need them? > > Actually these function pointer are used in multiple places i.e inside > the struct and also for passing as the argument for the function. So i > think its better to keep them as typedef to simplify and avoid any 'line > over 80 chars' checkpatch issue. But anyway if you suggest we can modify > to remove these typedefs . Please remove them, they just make the code harder to read. -- Kalle Valo
Re: [PATCH] mt76x0: use band parameter for LC calibration
Stanislaw Gruszka writes:
> We use always 1 as band parameter for MCU_CAL_LC, this break 2GHz,
> we should use 0 for this band instead.
>
> Patch fixes problems happened sometimes when try to associate with 2GHz
> AP and manifest by errors like below:
>
> [14680.920823] wlan0: authenticate with 18:31:bf:c0:51:b0
> [14681.109506] wlan0: send auth to 18:31:bf:c0:51:b0 (try 1/3)
> [14681.310454] wlan0: send auth to 18:31:bf:c0:51:b0 (try 2/3)
> [14681.518469] wlan0: send auth to 18:31:bf:c0:51:b0 (try 3/3)
> [14681.726499] wlan0: authentication with 18:31:bf:c0:51:b0 timed out
>
> Fixes: 9aec146d0f6b ("mt76x0: pci: introduce mt76x0_phy_calirate routine")
> Signed-off-by: Stanislaw Gruszka
> ---
> This is for 4.20.
This describes the symptoms better than the other 4.20 patch, thanks for
that. And at least to me looks ok for 4.20.
--
Kalle Valo
Re: [PATCH] mt76x0: run calibration after scanning
Stanislaw Gruszka writes:
> If we are associated and scanning is performed , sw_scan_complete callback
> is done after we get back to operating channel, so we do not perform
> phy calibration and queue cal work. Fix this by run calibration from
> sw_scan_complete().
>
> Fixes: bbd10586f0df ("mt76x0: phy: do not run calibration during channel
> switch")
> Signed-off-by: Stanislaw Gruszka
> ---
> This is for 4.20
So what are the symptoms from user's point of view? Is this a
regression?
I'm just trying to understand if this really is needed for 4.20. There
are so many mt76 patches flowing to -next that the bar for mt76 -rc
patches is even higher than normally.
--
Kalle Valo
Re: [PATCH 4.20 FIX] brcmutil: really fix decoding channel info for 160 MHz bandwidth
Rafał Miłecki writes:
> From: Rafał Miłecki
>
> Previous commit /adding/ support for 160 MHz chanspecs was incomplete.
> It didn't set bandwidth info and didn't extract control channel info. As
> the result it was also using uninitialized "sb" var.
>
> This change has been tested for two chanspecs found to be reported by
> some devices/firmwares:
> 1) 60/160 (0xee32)
>Before: chnum:50 control_ch_num:36
> After: chnum:50 control_ch_num:60
> 2) 120/160 (0xed72)
>Before: chnum:114 control_ch_num:100
> After: chnum:114 control_ch_num:120
>
> Fixes: 330994e8e8ec ("brcmfmac: fix for proper support of 160MHz bandwidth")
> Signed-off-by: Rafał Miłecki
I'll queue this for 4.20.
--
Kalle Valo
Re: [PATCH] ath9k: Fix a locking bug in ath9k_add_interface()
Dan Carpenter writes:
> On Wed, Oct 24, 2018 at 08:50:52AM +0300, Kalle Valo wrote:
>> Dan Carpenter writes:
>>
>> > We tried to revert commit d9c52fd17cb4 ("ath9k: fix tx99 with monitor
>> > mode interface") but accidentally missed part of the locking change.
>> >
>> > The lock has to be held earlier so that we're holding it when we do
>> > "sc->tx99_vif = vif;" and also there in the current code there is a
>> > stray unlock before we have taken the lock.
>> >
>> > Fixes: 6df0580be8bc ("ath9k: add back support for using active
>> > monitor interfaces for tx99")
>> > Signed-off-by: Dan Carpenter
>>
>> commit 6df0580be8bc is on it's way to v4.20 so should I also queue this
>> to v4.20?
>
> Yeah. Obviously this is a static checker thing and I haven't tested it.
>
> I don't know if add_interface() is ever called in parallel, but I can
> imagine that it might be. In that case the race condition is something
> that would affect real life.
>
> Anyway, it's a small obvious fix.
Ok, I'll then queue this to v4.20. But I would appreciate if others
could test or review this.
--
Kalle Valo
Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()
Dan Carpenter writes:
> On Wed, Oct 24, 2018 at 11:56:53AM +0300, Kalle Valo wrote:
>> Dan Carpenter writes:
>>
>> > It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
>> > of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
>> > (precision) so if the ssid doesn't have a NUL terminator this could lead
>> > to an overflow.
>> >
>> > Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
>> > Signed-off-by: Dan Carpenter
>> > ---
>> > Static analsysis. Not tested.
>>
>> IMHO this part (after "---" line) is important information and should be
>> part of commit log. I can fix that.
>>
>
> In my experience most maintainers disagree (with varying degrees of
> intensity).
Heh, why would adding four words explaining the background of the patch
to a commit log would be a bad thing? :) Well, I guess I just view
things differently.
--
Kalle Valo
Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()
Dan Carpenter writes:
> It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
> of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
> (precision) so if the ssid doesn't have a NUL terminator this could lead
> to an overflow.
>
> Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
> Signed-off-by: Dan Carpenter
> ---
> Static analsysis. Not tested.
IMHO this part (after "---" line) is important information and should be
part of commit log. I can fix that.
I'll also remove "wireless:" from the title.
--
Kalle Valo
Re: [PATCH] ath9k: Fix a locking bug in ath9k_add_interface()
Dan Carpenter writes:
> We tried to revert commit d9c52fd17cb4 ("ath9k: fix tx99 with monitor
> mode interface") but accidentally missed part of the locking change.
>
> The lock has to be held earlier so that we're holding it when we do
> "sc->tx99_vif = vif;" and also there in the current code there is a
> stray unlock before we have taken the lock.
>
> Fixes: 6df0580be8bc ("ath9k: add back support for using active monitor
> interfaces for tx99")
> Signed-off-by: Dan Carpenter
commit 6df0580be8bc is on it's way to v4.20 so should I also queue this
to v4.20?
--
Kalle Valo
Re: 4.18.16: memory leak in sta_set_sinfo
Arend van Spriel writes: > On 10/22/2018 10:37 AM, Johannes Berg wrote: >> On Mon, 2018-10-22 at 10:35 +0200, Johannes Stezenbach wrote: >> >>>> commit 848e616e66d4592fe9afc40743d3504deb7632b4 >>>> Author: Stefan Seyfried >>>> Date: Sun Sep 30 12:53:00 2018 +0200 >>>> >>>> cfg80211: fix wext-compat memory leak >>>> >>>> Hopefully that'll eventually propagate to stable. >>> >>> Good to know it's fixed, but "hopefully" makes me wonder, >>> I'd love to hear the confirmation that it's been queued. >> >> Well, normally it works automatically when you have a Fixes tag, so I >> don't usually try to queue anything manually. Maybe Greg has had his >> hands full with the 4.19 release :-) > > Missed the memo. Does that mean Cc: to stable is no longer needed? It > is still documented as such in > Documentation/process/stable-kernel-rules.rst. I haven't seen any memos either but in lot of cases having just Fixes line seems to be enough to get the patch to stable releases. If someone has any documentation about this, please do share. -- Kalle Valo
Re: pull request: mt76 2018-10-13 v2
Felix Fietkau writes: > Here's another large batch of mt76 code cleanup / deduplication / fixes > v2: add missing S-o-b. > > - Felix > > The following changes since commit c894696188d5c2af1e636e458190e80c53fb893d: > > rtlwifi: rtl8821ae: replace _rtl8821ae_mrate_idx_to_arfr_id with generic > version (2018-10-13 15:00:37 +0300) > > are available in the Git repository at: > > https://github.com/nbd168/wireless tags/mt76-for-kvalo-2018-10-13 > > for you to fetch changes up to f27a058199ee6da50d724302d42fff94a00851c7: > > mt76x0: phy: do not run calibration during channel switch (2018-10-13 > 16:35:16 +0200) > > > mt76 patches for 4.20 > > * mt76x0 fixes > * mt76x0e improvements (should be usable now) > * usb support improvements > * more mt76x0/mt76x2 unification work > * minor fix for aggregation + powersave clients > > ---- Pulled, thanks. -- Kalle Valo
Re: [RFC v3 01/12] rtw88: main files
Tony Chuang writes:
>> > +static void rtw_watch_dog_work(struct work_struct *work)
>> > +{
>> > + struct rtw_dev *rtwdev = container_of(work, struct rtw_dev,
>> > +watch_dog_work.work);
>> > + struct rtw_vif *rtwvif;
>> > +
>> > + if (!rtw_flag_check(rtwdev, RTW_FLAG_RUNNING))
>> > + return;
>> > +
>> > + ieee80211_queue_delayed_work(rtwdev->hw,
>> >watch_dog_work,
>> > + RTW_WATCH_DOG_DELAY_TIME);
>>
>> You're aware of the power cost of waking up every 2 seconds? That's a
>> really bad idea, in general, at the very least you should use a more
>> power efficient scheduling here to combine with other wakeups
>> (round_jiffies_relative, or so).
>
> Yeah I knew it, but so far we can only work like this...
> Will use round_jiffies_relative to combine the CPU wakeups.
Can you elaborate more why this horrible timer is needed? And it
definitely needs a comment in the code explaining the reason.
--
Kalle Valo
Re: [PATCH 1/5] qtnfmac: use 'help' in Kconfig
Sergey Matyukevich wrote: > Fix checkpatch warning: use preferred 'help' option in Kconfig. > > Signed-off-by: Sergey Matyukevich 3 patches applied to wireless-drivers-next.git, thanks. db62abe51853 qtnfmac: use 'help' in Kconfig b458a033ca2f qtnfmac: use SPDX identifier for pcie bus layer files 4cb5054957b2 qtnfmac_pcie: cleanup Pearl platform headers -- https://patchwork.kernel.org/patch/10630323/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH v4] brcmsmac: AP mode: update beacon when TIM changes
Ali MJ Al-Nasrawy wrote: > Beacons are not updated to reflect TIM changes. This is not compliant with > power-saving client stations as the beacons do not have valid TIM and can > cause the network to stall at random occasions and to have highly variable > latencies. > Fix it by updating beacon templates on mac80211 set_tim callback. > > Addresses an issue described in: > https://marc.info/?i=20180911163534.21312d08%20()%20manjaro > > Signed-off-by: Ali MJ Al-Nasrawy Patch applied to wireless-drivers-next.git, thanks. 2258ee58baa5 brcmsmac: AP mode: update beacon when TIM changes -- https://patchwork.kernel.org/patch/10625067/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: pull request: mt76 2018-10-13
Felix Fietkau writes: > Hi Kalle, > > Here's another batch of mt76 code cleanup / deduplication / fixes > > - Felix > > The following changes since commit c894696188d5c2af1e636e458190e80c53fb893d: > > rtlwifi: rtl8821ae: replace _rtl8821ae_mrate_idx_to_arfr_id with generic > version (2018-10-13 15:00:37 +0300) > > are available in the Git repository at: > > https://github.com/nbd168/wireless tags/mt76-for-kvalo-2018-10-13 > > for you to fetch changes up to f27a058199ee6da50d724302d42fff94a00851c7: > > mt76x0: phy: do not run calibration during channel switch (2018-10-13 > 16:35:16 +0200) > > > mt76 patches for 4.20 > > * mt76x0 fixes > * mt76x0e improvements (should be usable now) > * usb support improvements > * more mt76x0/mt76x2 unification work > * minor fix for aggregation + powersave clients > > ---- Your s-o-b is missing from some of the patches, I can't pull this. -- Kalle Valo
Re: [RFC v4 08/13] rtw88: debug files
Joe Perches writes:
> On Sat, 2018-10-13 at 17:00 +0800, yhchu...@realtek.com wrote:
>> From: Yan-Hsuan Chuang
> []
>> diff --git a/drivers/net/wireless/realtek/rtw88/debug.c
>> b/drivers/net/wireless/realtek/rtw88/debug.c
> []
>> +#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 =
>> +
>> +if (net_ratelimit())
>> +dev_dbg(rtwdev->dev, "%pV", );
>> +
>> +va_end(args);
>> +}
>> +EXPORT_SYMBOL(__rtw_dbg);
>> +
>> +#define __rtw_fn(fn)
>> \
>> +void __rtw_ ##fn(struct rtw_dev *rtwdev, const char *fmt, ...)
>> \
>> +{ \
>> +struct va_format vaf = {\
>> +.fmt = fmt, \
>> +}; \
>> +va_list args; \
>> +\
>> +va_start(args, fmt);\
>> +vaf.va = \
>> +dev_ ##fn(rtwdev->dev, "%pV", );\
>> +va_end(args); \
>> +} \
>> +EXPORT_SYMBOL(__rtw_ ##fn);
>> +
>> +__rtw_fn(info)
>> +__rtw_fn(warn)
>> +__rtw_fn(err)
>
> It's very unusual to have _all_ the logging under a CONFIG__DEBUG
> config guard flag.
For wireless drivers that is actually quite typical. IIRC at least
ath6kl, ath9k and ath10k do that, most likely also others.
> Typical debugging would dynamic debugging on a per-line instance and
> this uses a single dev_dbg for all debugging.
I don't recall seeing anyone using per-line dynamic debugging with
wireless drivers. The drivers are so complex that enabling one message
at a time doesn't really get you anywhere, that's why we mostly group
messages into similar groups (or levels) to make it easier to enable
certain debug messages.
> This seems unnecessarily complexity for a negative gain.
I haven't reviewed the driver yet but from a quick look I don't see this
as a problem.
> Also, many callers of the rtw_ logging function do not include
> a terminating newline.
>
> Please consistently use a newline for each format.
But with this I do agree.
--
Kalle Valo
Re: [PATCH 5/5] qtnfmac: add support for Topaz chipsets
Sergey Matyukevich writes: > This patch adds support for QSR1000/QSR2000 family of chipsets > to qtnfmac_pcie platform driver. > > QSR1000/QSR2000 (aka Topaz) is a family of 80MHz, 11ac Wave2, > 4x4/2x4/2x2 chips, including single and dual band devices. > Depending on specific chip model and firmware in use, either > STA or both STA and AP modes are supported. > > Patch adds Topaz support to qtnfmac_pcie driver. It is possible > to enable both Topaz and Pearl support in kernel configuration. > In that case proper platform bus will be selected on probing > based on chip ID. > > Signed-off-by: Igor Mitsyanko > Signed-off-by: Sergey Matyukevich > Signed-off-by: Andrey Shevchenko [...] > +config QTNFMAC_TOPAZ_PCIE > + tristate "Quantenna QSR1000/QSR2000 PCIe support" > + default n > + depends on PCI && CFG80211 > + select QTNFMAC > + select FW_LOADER > + select CRC32 > + help > + This option adds support for wireless adapters based on Quantenna > + 802.11ac QSR1000/QSR2000 (aka Topaz) FullMAC chipset > + running over PCIe. > + > + If you choose to build it as a module, two modules will be built: > + qtnfmac.ko and qtnfmac_pcie.ko. I'm not really fond of adding a Kconfig option for every supported hardware version unless there are very good reasons (memory savings etc). So is this really needed? A much better approach would be to have a generic QTNFMAC_PCIE option which can be used to include or exclude all PCI code. -- Kalle Valo
Re: [PATCH] iwlwifi: mvm: check return value of rs_rate_from_ucode_rate()
Luca Coelho wrote: > From: Luca Coelho > > The rs_rate_from_ucode_rate() function may return -EINVAL if the rate > is invalid, but none of the callsites check for the error, potentially > making us access arrays with index IWL_RATE_INVALID, which is larger > than the arrays, causing an out-of-bounds access. This will trigger > KASAN warnings, such as the one reported in the bugzilla issue > mentioned below. > > This fixes https://bugzilla.kernel.org/show_bug.cgi?id=200659 > > Cc: sta...@vger.kernel.org > Signed-off-by: Luca Coelho Patch applied to wireless-drivers-next.git, thanks. 3d71c3f1f50c iwlwifi: mvm: check return value of rs_rate_from_ucode_rate() -- https://patchwork.kernel.org/patch/10639957/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: pull-request: iwlwifi-next 2018-10-12
Luca Coelho writes: > Hi Kalle, > > This is the fourth batch of patches intended for v4.20. This includes > only the last patchset I sent. Usual development work, mostly queue > handling cleanups. More details about the contents in the tag > description. > > I have sent this out before and kbuildbot reported success. > > Please let me know if there are any issues. > > Cheers, > Luca. > > > The following changes since commit abf1a08ff3237a27188ff8cc2904f2cea893af55: > > net: vhost: remove bad code line (2018-10-07 21:31:32 -0700) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-next.git > tags/iwlwifi-next-for-kalle-2018-10-12 > > for you to fetch changes up to 724fe7710ac5f4289886b90060ed67e3a4bdd584: > > iwlwifi: mvm: kill INACTIVE queue state (2018-10-08 10:49:22 +0300) > > > Fourth set of iwlwifi patches intended for 4.20 > > * Support for a new scan type; > * Clean-up in the queue handling code; > * A few bug fixes; > > Pulled, thanks. -- Kalle Valo
Re: [RFC v3] cfg80211: add peer measurement with FTM API
Johannes Berg writes:
> From: Johannes Berg
>
> Add a new "peer measurement" API, that can be used to measure
> certain things related to a peer. Right now, only implement
> FTM (flight time measurement) over it, but the idea is that
> it'll be extensible to also support measuring the necessary
> things to calculate e.g. angle-of-arrival for WiGig.
>
> The API is structured to have a generic list of peers and
> channels to measure with/on, and then for each of those a
> set of measurements (again, only FTM right now) to perform.
>
> Results are sent to the requesting socket, including a final
> complete message.
>
> Closing the controlling netlink socket will abort a running
> measurement.
>
> Signed-off-by: Johannes Berg
> ---
> v3:
> - add a bit to report "final" for partial results
> - remove list keeping etc. and just unicast out the results
>to the requester (big code reduction ...)
> - also send complete message unicast, and as a result
>remove the multicast group
> - separate out struct cfg80211_pmsr_ftm_request_peer
>from struct cfg80211_pmsr_request_peer
> - document timeout == 0 if no timeout
> - disallow setting timeout nl80211 attribute to 0,
>must not include attribute for no timeout
> - make MAC address randomization optional
> - change num bursts exponent default to 0 (1 burst, rather
>rather than the old default of 15==don't care)
[...]
> @@ -3176,6 +3361,8 @@ struct cfg80211_ftm_responder_stats {
> *
> * @get_ftm_responder_stats: Retrieve FTM responder statistics, if available.
> * Statistics should be cumulative, currently no way to reset is provided.
> + * @start_pmsr: start peer measurement (e.g. FTM)
> + * @abort_pmsr: abort peer measurement
> */
> struct cfg80211_ops {
> int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
It would be nice to document if these ops can sleep or not.
--
Kalle Valo
Re: [PATCH v4 4/8] rt2800: fix registers init for MT7620
Stanislaw Gruszka writes: > There is duplicated 'if (rt2x00_rt(rt2x00dev, RT6352))' entry that > causes we do not perform register initialization for RT6352 (MT7620 > SOCs) in correct branch. Fix this and disable registers initialization > that is specific to particular MT7620 revision. > > Signed-off-by: Stanislaw Gruszka [...] > @@ -5466,6 +5465,10 @@ static int rt2800_init_registers(struct rt2x00_dev > *rt2x00dev) > rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x0401); > rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x000C); > rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x000C0408); > + /* TODO add chip version support and init registers > + * according to the version. > + */ > +#if 0 > rt2800_register_write(rt2x00dev, MIMO_PS_CFG, 0x0002); > rt2800_register_write(rt2x00dev, TX_PIN_CFG, 0x00150F0F); > rt2800_register_write(rt2x00dev, TX_ALC_VGA3, 0x06060606); > @@ -5480,6 +5483,7 @@ static int rt2800_init_registers(struct rt2x00_dev > *rt2x00dev) > reg = rt2800_register_read(rt2x00dev, TX_ALC_CFG_1); > rt2x00_set_field32(, TX_ALC_CFG_1_ROS_BUSY_EN, 0); > rt2800_register_write(rt2x00dev, TX_ALC_CFG_1, reg); > +#endif No '#if 0', please. If the code is not needed you can remove it, it's available from git history anyway if it's needed later. -- Kalle Valo
Re: [PATCH][next] ath10k: fix out of bound read on array ath10k_rates
Colin King wrote:
> From: Colin Ian King
>
> An out-of-bounds read on array ath10k_rates is occurring because
> the maximum number of elements is currently based on the size of
> the array and not the number of elements in the array. Fix this
> by using ARRAY_SIZE instead of sizeof.
>
> Detected by CoverityScan, CID#1473918 ("Out-of-bounds read")
>
> Fixes: f279294e9ee2 ("ath10k: add support for configuring management packet
> rate")
> Signed-off-by: Colin Ian King
This is already fixed in ath-next.
error: patch failed: drivers/net/wireless/ath/ath10k/mac.c:164
error: drivers/net/wireless/ath/ath10k/mac.c: patch does not apply
stg import: Diff does not apply cleanly
Patch set to Rejected.
--
https://patchwork.kernel.org/patch/10628815/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] qtnfmac: avoid uninitialized variable access
Sergey Matyukevich writes:
>> > When qtnf_trans_send_cmd_with_resp() fails, we have not yet initialized
>> > 'resp', as pointed out by a valid gcc warning:
>> >
>> > drivers/net/wireless/quantenna/qtnfmac/commands.c: In function
>> > 'qtnf_cmd_send_with_reply':
>> > drivers/net/wireless/quantenna/qtnfmac/commands.c:133:54: error:
>> > 'resp' may be used uninitialized in this function
>> > [-Werror=maybe-uninitialized]
>> >
>> > Since 'resp_skb' is also not set here, we can skip all further
>> > processing and just print the warning and return the failure code.
>> >
>> > Fixes: c6ed298ffe09 ("qtnfmac: cleanup and unify command error handling")
>> > Signed-off-by: Arnd Bergmann
>>
>> Thanks for the patch! And for reminding me that I forgot to enable
>> gcc warnings in CI builds in addition to sparse checks.
>>
>> Reviewed-by: Sergey Matyukevich
>
> Hi Kalle,
>
> Could you please hold back applying this patch for now. We have got
> another report for the same function, this time static analysis tool
> warning. It looks like the patch from Arnd does not cover both cases.
> So we will take a closer look and send a combined fix later.
Ok, I'll drop this from my queue. Please resend whatever patch I need to
apply.
--
Kalle Valo
Re: [PATCH 5/6] brcmfmac: Set board_type from DMI on x86 based machines
Arend van Spriel writes: > On 10/10/2018 9:28 AM, Hans de Goede wrote: >> So how do you want to proceed with this, do you want me to just >> put the full ISC text in the header for now as the rest of brcmfmac >> does? > > This is not entirely true as far as I know. I assume you are referring > to this: > > /* > * Copyright (c) 2010 Broadcom Corporation > * > * Permission to use, copy, modify, and/or distribute this software for any > * purpose with or without fee is hereby granted, provided that the above > * copyright notice and this permission notice appear in all copies. > * > * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES > * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF > * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE > FOR ANY > * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES > * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN > AN ACTION > * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN > * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > */ > > As far as I recall we opted for BSD license and ISC is equivalent. Oh, sorry for missing that. > However, The BSD license are already in place so why not use that. I > would say BSD-2-Clause should cover it. As this is a new file I guess > it is up to you although I would prefer to stick with a permissive > license. >From maintainer's point of view I very much prefer that a driver uses the same license in all files. It's very confusing if the license changes within different files. -- Kalle Valo
Re: [PATCH 5/6] brcmfmac: Set board_type from DMI on x86 based machines
Hans de Goede writes: > Hi, > > On 10-10-18 09:09, Kalle Valo wrote: >> Hans de Goede writes: >> >>> For x86 based machines, set the board_type used for nvram file selection >>> based on the DMI sys-vendor and product-name strings. >>> >>> Since on some models these strings are too generic, this commit also adds >>> a quirk table overriding the strings for models listed in that table. >>> >>> The board_type setting is used to load the board-specific nvram file with >>> a board-specific name so that we can ship files for each supported board >>> in linux-firmware. >>> >>> Signed-off-by: Hans de Goede >> >> [...] >> >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c >>> @@ -0,0 +1,104 @@ >>> +// SPDX-License-Identifier: ISC >> >> I don't see the ISC file in LICENSES directory[1] and I don't feel >> comfortable taking SPDX tags which which don't have a license file. > > Ok. I need to do a patch for the LICENSES directory anyways, so I will > also submit the ISC license upstream while at it, I will hopefully > get around to doing that today. That would be awesome, thanks! Then I could do the SPDX conversion also on ath10k. > So how do you want to proceed with this, do you want me to just > put the full ISC text in the header for now as the rest of brcmfmac > does? Yeah, I think this is the fastest way. > Then later someone (me if I get around to it) can replace all of > the headers with // SPDX-License-Identifier: ISC once it has been > added under LICENSES. Sounds good to me. -- Kalle Valo
Re: [PATCH 5/6] brcmfmac: Set board_type from DMI on x86 based machines
Hans de Goede writes: > For x86 based machines, set the board_type used for nvram file selection > based on the DMI sys-vendor and product-name strings. > > Since on some models these strings are too generic, this commit also adds > a quirk table overriding the strings for models listed in that table. > > The board_type setting is used to load the board-specific nvram file with > a board-specific name so that we can ship files for each supported board > in linux-firmware. > > Signed-off-by: Hans de Goede [...] > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c > @@ -0,0 +1,104 @@ > +// SPDX-License-Identifier: ISC I don't see the ISC file in LICENSES directory[1] and I don't feel comfortable taking SPDX tags which which don't have a license file. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/LICENSES -- Kalle Valo
Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes
James Prestwood writes: > The current driver hard codes various features, ext features and supported > interface types when a new radio is created. This patch adds several new > hwsim attributes so user space can specify specific features the radio should > have: > > - HWSIM_ATTR_FEATURE_SUPPORT > Bit field of nl80211_feature_flags flags > - HWSIM_ATTR_EXT_FEATURE_SUPPORT > Variable length bit field where bits map to nl80211_ext_feature_index > values. > - HWSIM_ATTR_IFTYPE_SUPPORT > Nested attribute of flags containing all supported interface types. > Each flag attribute sets support for an interface type. > > Each attribute data format matches a corresponding NL80211 attribute: > > HWSIM_ATTR_FEATURE_SUPPORT --> NL80211_ATTR_FEATURE_FLAGS > HWSIM_ATTR_EXT_FEATURE_SUPPORT --> NL80211_ATTR_EXT_FEATURES > HWSIM_ATTR_IFTYPE_SUPPORT --> NL80211_ATTR_SUPPORTED_IFTYPES Signed-off-by is missing. https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#signed-off-by_missing -- Kalle Valo
Re: [PATCH] libertas: call into generic suspend code before turning off power
Ulf Hansson writes:
> On 8 October 2018 at 22:03, Daniel Mack wrote:
>> When powering down a SDIO connected card during suspend, make sure to call
>> into the generic lbs_suspend() function before pulling the plug. This will
>> make sure the card is successfully deregistered from the system to avoid
>> communication to the card starving out.
>>
>> Fixes: 7444a8092906 ("libertas: fix suspend and resume for SDIO connected
>> cards")
>> Signed-off-by: Daniel Mack
>
> Reviewed-by: Ulf Hansson
>
> BTW, if you need this to reach 4.19 I have already queued some other
> mmc fixes so I can take this as well, if it helps. I need and ack of
> course.
I'm not planning to send anything to 4.19 anymore and this is so simple
that hopefully it cause any conflicts with -next patches, so feel free
to do that:
Acked-by: Kalle Valo
--
Kalle Valo
Re: [PATCH 5/5] qtnfmac: add support for Topaz chipsets
Sergey Matyukevich writes: > This patch adds support for QSR1000/QSR2000 family of chipsets > to qtnfmac_pcie platform driver. > > QSR1000/QSR2000 (aka Topaz) is a family of 80MHz, 11ac Wave2, > 4x4/2x4/2x2 chips, including single and dual band devices. > Depending on specific chip model and firmware in use, either > STA or both STA and AP modes are supported. > > Patch adds Topaz support to qtnfmac_pcie driver. It is possible > to enable both Topaz and Pearl support in kernel configuration. > In that case proper platform bus will be selected on probing > based on chip ID. > > Signed-off-by: Igor Mitsyanko > Signed-off-by: Sergey Matyukevich > Signed-off-by: Andrey Shevchenko [...] > --- a/drivers/net/wireless/quantenna/qtnfmac/qtn_hw_ids.h > +++ b/drivers/net/wireless/quantenna/qtnfmac/qtn_hw_ids.h > @@ -24,6 +24,7 @@ > /* PCIE Device IDs */ > > #define PCIE_DEVICE_ID_QTN_PEARL(0x0008) > +#define PCIE_DEVICE_ID_QTN_TOPAZ (0x0008) Same ids for both, is this really correct? -- Kalle Valo
Re: [RFC 00/19] wilc: added driver for wilc module
Ajay Singh writes:
> On Sat, 6 Oct 2018 15:45:41 +0300
> Kalle Valo wrote:
>
>> Ajay Singh writes:
>>
>> > This patch set contains the driver files from
>> > 'driver/staging/wilc1000'. Renamed the driver from 'wilc1000' to
>> > 'wilc' to have generic name, as the same driver will be used by
>> > other wilc family members.
>>
>> I'm worried that the name 'wilc' is just too generic, I liked the
>> original name wilc1000 much more. Quite often when we have a new
>> generation of wireless devices there's also a new driver, so in the
>> long run I'm worried that a generic name like 'wilc' could be a
>> source of confusion. I think it's much smaller problem if
>> 'wilc1000' (the driver) also supports wilc3000 (the device), people
>> are already used to that.
>
> I also thought about retaining the same wilc1000 name, which has be
> used for quite some time now. But as we are moving this driver
> to '/driver/net/wireless/' freshly so thought it’s better to change in
> the first commit.
>
> As you know, 'wilc1000' name is used for folder name as well as module
> name (i.e wilc1000.ko and wilc1000-spi.ko/wilc1000-sdio.ko). WILC3000 is
> going to use the same modules(.ko).
So just to be clear, this sounds good to me.
> Do you think its good approach to have 'wilc1000' folder but only remove
> ‘wilc1000’ prefix from .ko modules names in Makefile (i.e wilc.ko and
> wilc-spio.ko/wilc-sdio.ko). So these .ko becomes generic and give clear
> name to be used with both wilc1000 and wilc3000 modules.
But this I think is confusing. The driver should use the same name
everywhere, both in folder name ('wilc1000/') and in the module name
('wilc1000-*.ko').
If I'm understanding correctly you are worried that 'wilc1000-spi.ko'
also supports wilc3000 devices, but I don't see that as a problem. I
think it's very common (not just in wireless) that the marketing names
don't always match with driver names.
> Also should I submit the v2 version by retaining old name or wait for
> few more comments to include in that.
I haven't had a chance to review the driver and apparently nobody else
either. So I recommend waiting for review comments before submitting v2.
Also I suggest that you don't make any changes from the staging version,
like you did the rename. I think it's simplest to submit the code as it
is now in staging. And once you get review comments, first submit the
changes to staging and then send v2 for new review.
--
Kalle Valo
Re: [RFC 00/19] wilc: added driver for wilc module
Ajay Singh writes: > This patch set contains the driver files from 'driver/staging/wilc1000'. > Renamed the driver from 'wilc1000' to 'wilc' to have generic name, as > the same driver will be used by other wilc family members. I'm worried that the name 'wilc' is just too generic, I liked the original name wilc1000 much more. Quite often when we have a new generation of wireless devices there's also a new driver, so in the long run I'm worried that a generic name like 'wilc' could be a source of confusion. I think it's much smaller problem if 'wilc1000' (the driver) also supports wilc3000 (the device), people are already used to that. And if there's ever a need for a new driver, you can call it 'wilc4000' or whatever. For example, I think good driver names are like 'mt76' or 'rtw88' (currently under review). -- Kalle Valo
Re: [RFC v2 06/12] rtw88: fw and efuse files
Stanislaw Gruszka writes:
> On Wed, Oct 03, 2018 at 04:02:22PM +0800, yhchu...@realtek.com wrote:
>
>> +void rtw_add_rsvd_page(struct rtw_dev *rtwdev, enum rtw_rsvd_packet_type
>> type,
>> + bool txdesc)
>> +{
>> +struct rtw_rsvd_page *rsvd_pkt;
>> +
>> +list_for_each_entry(rsvd_pkt, >rsvd_page_list, list) {
>> +if (rsvd_pkt->type == type)
>> +return;
>> +}
>> +
>> +rsvd_pkt = kmalloc(sizeof(*rsvd_pkt),
>> + in_interrupt() ? GFP_ATOMIC : GFP_KERNEL);
>
> It's never called from interrupt. You have to check kmalloc output value.
My recommendation is to always avoid in_interrupt().
--
Kalle Valo
Re: [RFC v2 04/12] rtw88: trx files
Tony Chuang writes: >> > + pkt_info->bmc = bmc; >> > + pkt_info->sec_type = sec_type; >> > + pkt_info->tx_pkt_size = skb->len; >> > + pkt_info->offset = chip->tx_pkt_desc_sz; >> > + pkt_info->qsel = skb->priority; >> >> Shouldn't be qsel somehow mapped from skb->priority ? > > Firmware handles it. What if meaning of skb->priority changes in the future? I don't think it's safe to provide kernel internal values to firmware, you should do some kind of mapping in the driver. -- Kalle Valo
Re: [RFC v2 03/12] rtw88: hci files
Tony Chuang writes: >> > + } >> > + >> > + info = IEEE80211_SKB_CB(skb); >> > + ieee80211_tx_info_clear_status(info); >> > + info->flags |= IEEE80211_TX_STAT_ACK; >> > + ieee80211_tx_status_irqsafe(hw, skb); >> >> Always report ACK ? > > The ACK report is for mac80211 stack, it looks abnormal at the first glance. > But we can only do this for every data frame because there is no ack report > unless the driver ask the firmware to give one. > Ask for every data frame is resource consuming. I don't remember how mac80211 wants to handle the cases when the hardware doesn't provide ack status, but lying that to mac80211 sounds like a bad idea to me. Anyone have any suggestions how this should be handled? At the minimum add a proper comment explaining why you are lying to mac80211. -- Kalle Valo
Re: [PATCH 01/12] rtwlan: main files
Larry Finger writes: > On 10/4/18 8:42 AM, Stanislaw Gruszka wrote: >> On Thu, Oct 04, 2018 at 03:39:55PM +0300, Kalle Valo wrote: >>>>> Can we put the configuration file in the firmware directory? >>>>> Should we package them into binary files? Or just put the raw data. >>>>> >>>>> We can test the performance for it. After we got the result, we >>>>> will make a decision >>>>> about it. And if we decide to put them in the firmware directory, >>>>> will send a patch. >>>>> For now, I think we can just leave them in the .c. >>>> >>>> Yes, you could put the configuration files in the firmware directory. >>>> I would put them in binary form, not as text files. That way the size >>>> would be smaller, and it would not be possible to alter them, >>>> particularly if the binary file is checksummed. >>>> >>>> It would likely be OK if only the agc table was stored in this way. >>>> That would take away about half of the lines in the 8822b table file. >>> >>> So what's the worry here? The lines of source code, binary size or what? >>> >>> .../net/wireless/realtek/rtw88/rtw8822b_table.c| 20783 >>> +++ >>> >>> Looking at the diffstat rtw8822b_table.c seems to be 20 kLOC, IMHO it's >>> not that bad as it's just data. But of course I might be missing >>> something as I haven't checked patches yet. >> >> My concern was it's plenty of redundant data, for example: >> >> 0x81C, 0xFF03, >> 0x81C, 0xFE03, >> 0x81C, 0xFD020003, >> 0x81C, 0xFC040003, >> 0x81C, 0xFB060003, >> 0x81C, 0xFA080003, >> 0x81C, 0xF90A0003, >> 0x81C, 0xF80C0003, >> 0x81C, 0xF70E0003, >> 0x81C, 0xF613, >> >> Approx 1 lines like this, braked by lines like this >> >> 0x9012, 0x, 0x4000, 0x, >> >> in more or less regular way. >> >> Not big deal, but perhaps this could be coded in much more compact way. > > What should be the tradeoff between large tables of redundant data and > complicated generation and interpretation? I think this table should > be converted to binary in its present form and added to the > "firmware", the way that is done for b43. That way the source is > smaller, and the loading will be only a bit more time consuming. But separating the data from the driver creates another set of problems. For example, what if the data is dependent on the driver changes? Then we need to think about backwards compatibility etc, which creates more work us. I prefer simple solutions, less work and less problems :) -- Kalle Valo
Re: [PATCH 01/12] rtwlan: main files
Stanislaw Gruszka writes: > On Thu, Oct 04, 2018 at 03:39:55PM +0300, Kalle Valo wrote: >> >> Can we put the configuration file in the firmware directory? >> >> Should we package them into binary files? Or just put the raw data. >> >> >> >> We can test the performance for it. After we got the result, we >> >> will make a decision >> >> about it. And if we decide to put them in the firmware directory, >> >> will send a patch. >> >> For now, I think we can just leave them in the .c. >> > >> > Yes, you could put the configuration files in the firmware directory. >> > I would put them in binary form, not as text files. That way the size >> > would be smaller, and it would not be possible to alter them, >> > particularly if the binary file is checksummed. >> > >> > It would likely be OK if only the agc table was stored in this way. >> > That would take away about half of the lines in the 8822b table file. >> >> So what's the worry here? The lines of source code, binary size or what? >> >> .../net/wireless/realtek/rtw88/rtw8822b_table.c| 20783 >> +++ >> >> Looking at the diffstat rtw8822b_table.c seems to be 20 kLOC, IMHO it's >> not that bad as it's just data. But of course I might be missing >> something as I haven't checked patches yet. > > My concern was it's plenty of redundant data, for example: > > 0x81C, 0xFF03, > 0x81C, 0xFE03, > 0x81C, 0xFD020003, > 0x81C, 0xFC040003, > 0x81C, 0xFB060003, > 0x81C, 0xFA080003, > 0x81C, 0xF90A0003, > 0x81C, 0xF80C0003, > 0x81C, 0xF70E0003, > 0x81C, 0xF613, > > Approx 1 lines like this, braked by lines like this > > 0x9012, 0x, 0x4000, 0x, > > in more or less regular way. > > Not big deal, but perhaps this could be coded in much more compact way. Sure, making it more compact makes sense to me. But I don't see that as a strict requirement, that could be done also after the driver is accepted. Do note that I haven't reviewed the driver yet so I reserve the right to change my opinion :) -- Kalle Valo
Re: [RFC 00/12] rtwlan: mac80211 driver for Realtek 802.11ac wireless network chips
Tony Chuang writes: >> -Original Message- >> From: Kalle Valo [mailto:kv...@codeaurora.org] >> Sent: Monday, September 24, 2018 7:05 PM >> To: Stanislaw Gruszka >> Cc: Tony Chuang; larry.fin...@lwfinger.net; linux-wireless@vger.kernel.org; >> Pkshih; Andy Huang >> Subject: Re: [RFC 00/12] rtwlan: mac80211 driver for Realtek 802.11ac >> wireless network chips >> >> Stanislaw Gruszka writes: >> >> > On Fri, Sep 21, 2018 at 02:03:55PM +0800, yhchu...@realtek.com wrote: >> >> From: Yan-Hsuan Chuang >> >> >> >> This is a new mac80211 driver for Realtek 802.11ac wireless network chips. >> >> rtwlan supports 8822BE and 8822CE chips, and will be able to support >> >> multi-vif combinations in run-time. >> >> >> >> For now, only PCI bus is supported, but rtwlan was originally designed >> >> to optionally support three buses includes USB & SDIO. USB & SDIO >> modules >> >> will soon be supported by rtwlan, with configurable core module to fit >> >> with different bus modules in the same time. >> >> >> >> For example, if we choose 8822BE and 8822CU, only PCI & USB modules >> will >> >> be selected, built, loaded into kernel. This is one of the major >> >> difference from rtlwifi, which can only support specific combinations. >> >> >> >> Another difference from rtlwifi is that rtwlan is designed to support >> >> the latest Realtek 802.11ac wireless network chips like 8822B and >> >> 8822C series. Compared to the earlier chips supported by rtlwifi like >> >> the 802.11n 8192EE chipset or 802.11ac 8821AE/8812AE chips, newer ICs >> >> have different MAC & PHY settings, such as new multi-port feature for the >> >> MAC layer design and Jaguar2/Jaguar3 PHY layer IPs. >> >> >> >> Multi-Port feature is also supported under rtwlan's software architecture. >> >> rtlwifi can only support one vif in the same time, most because of the >> >> hardware limitations for early chips, hence the original design of it >> >> also restricts the usage of multi-vif support, so latest chipset seems not >> >> take advantages from its new MAC engine. >> >> >> >> However, rtwlan can run multiple vifs concurrently by holding them on >> >> hardware ports provided by MAC engine, so we can easily start different >> >> roles on a single device. >> >> >> >> Based on the reasons mentioned before, we implemented rtwlan. It had >> many >> >> authors, they are listed here alphabetically: >> >> >> >> Ping-Ke Shih >> >> Tzu-En Huang >> >> Yan-Hsuan Chuang >> > >> > I didn't do detailed review, but my general impression is very very >> > positive. New driver looks great! >> >> I also did a quick 10 min look at the driver and indeed in general it >> looks good. >> >> > Just 2 generic remarks: >> > - please add MAINTAINERS file entry >> > - please post a patch or request to remove staging/rtlwifi driver >> > since this one is replace for it (8822BE PCI-ID is the same) >> >> Something I noticed: >> >> o Magic numbers (BIT(1) etc) in quite a few places. >> >> o Personally not really fond of "#ifdef LITTLE_ENDIAN" usage, but I >> guess we can live with that? >> >> o To me the name "rtwlan" sounds confusing when one compares it with >> "rtlwifi". And how would the possible next generation 11ax driver be >> then called? As a good example I really like the driver name mt76, >> could this one have something similar to make it more descriptive? >> >> I also pushed this to the pending branch on wireless-drivers-next so >> that kbuild bot can extensively test it. > > Yes, we will add MAINTAINER files entry after this driver. > got accepted into the kernel, or should we include the entry in this patch? Please update the MAINTAINERS file in this patchset, just in a separate patch. I can them apply it at the same time as the driver (once it's ready). -- Kalle Valo
Re: pull request: mt76 2018-10-05 v2
Felix Fietkau writes: > here's the fixed version of the previous pull request. I've dropped > the broken patch from the previous round. > > - Felix > > The following changes since commit e1c02eb16a9c742178874a7d1a08d300981715fb: > > qtnfmac: implement dump_station support for STA mode (2018-10-05 14:01:44 > +0300) > > are available in the Git repository at: > > https://github.com/nbd168/wireless tags/mt76-for-kvalo-2018-10-05 > > for you to fetch changes up to 9b43960b899c71c758209a58c7e8d7d6e481e272: > > mt76: move irq handler in mt76x02-lib moudle (2018-10-05 20:05:46 +0200) > > > mt76 patches for 4.20 > > * unify code between mt76x0, mt76x2 > * mt76x0 fixes > * another fix for rx buffer allocation regression on usb > * move mt76x2 source files to mt76x2 folder > * more work on mt76x0e support > > ---- This compiled without problems, thanks for the quick update. Pulled. -- Kalle Valo
Re: pull-request: iwlwifi-next 2018-10-06
Luca Coelho writes: > This is the third batch of patches intended for v4.20. This includes > the last 2 patchsets I sent. Usual development work, including some > improvements in the HE radiotap stuff, a new FW version support and > other small improvements and fixes. More details about the contents in > the tag description. > > I have sent this out before and kbuildbot reported success. > > Please let me know if there are any issues. > > Cheers, > Luca. > > > The following changes since commit e1c02eb16a9c742178874a7d1a08d300981715fb: > > qtnfmac: implement dump_station support for STA mode (2018-10-05 14:01:44 > +0300) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-next.git > tags/iwlwifi-next-for-kalle-2018-10-06 > > for you to fetch changes up to ea7cb8293874f8d9cf36c9e5387e2247f15373dc: > > iwlwifi: dbg: make trigger functions type agnostic (2018-10-06 10:25:55 > +0300) > > > Third set of iwlwifi patches for 4.20 > > * Fix for a race condition that caused the FW to crash; > * HE radiotap cleanup and improvements; > * Reorder channel optimization for scans; > * Bumped the FW API version supported after the last API change for > this release; > * Debugging improvements; > * A few bug fixes; > * Some cleanups in preparation for a new implementation; > * Other small improvements, cleanups and fixes. > > Pulled, thanks. -- Kalle Valo
Re: pull request: mt76 2018-10-05
Felix Fietkau writes: > Here's another large batch of mt76 code cleanup / deduplication / fixes > > - Felix > > The following changes since commit e1c02eb16a9c742178874a7d1a08d300981715fb: > > qtnfmac: implement dump_station support for STA mode (2018-10-05 14:01:44 > +0300) > > are available in the Git repository at: > > https://github.com/nbd168/wireless tags/mt76-for-kvalo-2018-10-05 > > for you to fetch changes up to 06ac97c2e58c7b32bf950ac53976c4260687d386: > > mt76x0: pci: report firmware version using ethtool (2018-10-05 20:05:46 > +0200) > > > mt76 patches for 4.20 > > * unify code between mt76x0, mt76x2 > * mt76x0 fixes > * another fix for rx buffer allocation regression on usb > * move mt76x2 source files to mt76x2 folder > * more work on mt76x0e support > > I have to drop this as it doesn't build for me: drivers/net/wireless/mediatek/mt76/mt76x0/pci_mcu.c: In function 'mt76x0e_load_firmware': drivers/net/wireless/mediatek/mt76/mt76x0/pci_mcu.c:119:28: error: passing argument 1 of 'mt76x02_set_ethtool_fwver' from incompatible pointer type [-Werror=incompatible-pointer-types] mt76x02_set_ethtool_fwver(dev, hdr); ^~~ In file included from drivers/net/wireless/mediatek/mt76/mt76x0/mcu.h:18:0, from drivers/net/wireless/mediatek/mt76/mt76x0/pci_mcu.c:20: drivers/net/wireless/mediatek/mt76/mt76x0/../mt76x02_mcu.h:108:6: note: expected 'struct mt76_dev *' but argument is of type 'struct mt76x02_dev *' void mt76x02_set_ethtool_fwver(struct mt76_dev *dev, ^ cc1: some warnings being treated as errors scripts/Makefile.build:305: recipe for target 'drivers/net/wireless/mediatek/mt76/mt76x0/pci_mcu.o' failed -- Kalle Valo
Re: [PATCH v2 01/13] qtnfmac: do not track STA states in driver
Sergey Matyukevich wrote: > Remove STA connection states tracking from driver. > Leave it wireless core on host and to firmware. > > Signed-off-by: Sergey Matyukevich 13 patches applied to wireless-drivers-next.git, thanks. 263ee96b77a7 qtnfmac: do not track STA states in driver d5f693bc4bb9 qtnfmac: generate local disconnect event in disconnect callback 92246b126ebf qtnfmac: request userspace to do OBSS scanning if FW can not 75001bbc0765 qtnfmac: do not initialize per-MAC data multiple times c6ed298ffe09 qtnfmac: cleanup and unify command error handling aaa981406f4f qtnfmac: do not cancel scan in disconnect callback d5657b709e2a qtnfmac: pass sgi rate info flag to wireless core ab1c64a1d349 qtnfmac: inform wireless core about supported extended capabilities 35da3fe63b86 qtnfmac: drop error reports for out-of-bounds key indexes 6d85930f2653 qtnfmac: add support for scan flush 2525f188f7fd qtnfmac: add support for scan dwell time configuration 8804ea9e15a3 qtnfmac: drop redundant data copy in control path e1c02eb16a9c qtnfmac: implement dump_station support for STA mode -- https://patchwork.kernel.org/patch/10627791/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] wlcore: Add support for optional wakeirq
Tony Lindgren wrote: > Now with wlcore using PM runtime, we can also add support for Linux > generic wakeirq handling for it if configured in the dts file. > > The wakeirq can be configured as the second interrupt in the dts file > with interrupts-extended property where it is the padconf irq of the OOB > GPIO pin used for wlcore interrupt. > > Note that eventually we should also allow configuring wlcore to use the > SDIO dat1 IRQ for wake-up, and in that case the the wakeirq should be > configured to be the padconf interrupt of the dat1 pin and not the > padconf interrupt of the OOB GPIO pin. > > Cc: Eyal Reizer > Signed-off-by: Tony Lindgren Patch applied to wireless-drivers-next.git, thanks. 3c83dd577c7f wlcore: Add support for optional wakeirq -- https://patchwork.kernel.org/patch/10622795/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] wlcore: Fix BUG with clear completion on timeout
Tony Lindgren wrote: > We do not currently clear wl->elp_compl on ELP timeout and we have bogus > lingering pointer that wlcore_irq then will try to access after recovery > is done: > > BUG: spinlock bad magic on CPU#1, irq/255-wl12xx/580 > ... > (spin_dump) from [] (do_raw_spin_lock+0xc8/0x124) > (do_raw_spin_lock) from [] (_raw_spin_lock_irqsave+0x68/0x74) > (_raw_spin_lock_irqsave) from [] (complete+0x24/0x58) > (complete) from [] (wlcore_irq+0x48/0x17c [wlcore]) > (wlcore_irq [wlcore]) from [] (irq_thread_fn+0x2c/0x64) > (irq_thread_fn) from [] (irq_thread+0x148/0x290) > (irq_thread) from [] (kthread+0x160/0x17c) > (kthread) from [] (ret_from_fork+0x14/0x20) > ... > > After that the system will hang. Let's fix this by adding a flag for > recovery and moving the recovery work call to to the error handling > section. > > And we want to set WL1271_FLAG_INTENDED_FW_RECOVERY and actually clear > it too in wl1271_recovery_work() and just downgrade the error to a > warning to prevent overly verbose output. > > Cc: Eyal Reizer > Signed-off-by: Tony Lindgren Patch applied to wireless-drivers-next.git, thanks. 4e651bad8489 wlcore: Fix BUG with clear completion on timeout -- https://patchwork.kernel.org/patch/10622767/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH v2] rtlwifi: Removed unused define and code efuse_re_pg* from wifi.h
wrote: > From: Ping-Ke Shih > > The following: > bool efuse_re_pg_sec1flag; > u8 efuse_re_pg_data[8]; > are not referenced anywhere in the rtlwifi code. > > This patch is originally created by Rick Veens , > and Joe Perches reminded to apply it to rtlwifi. > > Signed-off-by: Ping-Ke Shih Patch applied to wireless-drivers-next.git, thanks. 9c22211e1d71 rtlwifi: Removed unused define and code efuse_re_pg* from wifi.h -- https://patchwork.kernel.org/patch/10621717/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH v2 1/2] brcmfmac: reduce timeout for action frame scan
Chi-Hsien Lin wrote: > From: Chung-Hsien Hsu > > Finding a common channel to send an action frame out is required for > some action types. Since a loop with several scan retry is used to find > the channel, a short wait time could be considered for each attempt. > This patch reduces the wait time from 1500 to 450 msec for each action > frame scan. > > This patch fixes the WFA p2p certification 5.1.20 failure caused by the > long action frame send time. > > Signed-off-by: Chung-Hsien Hsu > Signed-off-by: Chi-Hsien Lin 2 patches applied to wireless-drivers-next.git, thanks. edb6d6885bef brcmfmac: reduce timeout for action frame scan fbf07000960d brcmfmac: fix full timeout waiting for action frame on-channel tx -- https://patchwork.kernel.org/patch/10618131/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH 00/13] qtnfmac: fixes and cleanups for STA mode
Sergey Matyukevich OS writes: > Hello Kalle and all, > > Here is the next patch set with fixes and cleanups for qtnfmac driver. > This set mostly consists of multiple fixes and improvements for STA > mode. The major changes include the following items: > - fixes for 'iw' output: rates for enabled SGI, 'dump station' > - expose more scan features to host: scan flush and dwell time > - inform host wireless core when OBSS is not supported by firmware > - command processing cleanup: unify and get rid of 'unlikely' macros The string "OS" is again in the from field. Can you fix that and resend, please? -- Kalle Valo
Re: feedback on mainlining wilc1000 staging driver
Ajay Singh writes: >> > I have submitted a patch series for wilc1000 driver, single file >> > per patch and its based on wireless-drivers-next. >> > I hope its done correctly, please provide inputs so we can >> > address and make this driver ready for mainline. >> >> Thanks, I see it in patchwork: >> >> https://patchwork.kernel.org/project/linux-wireless/list/?series=23251=*=date >> >> Do note that we have also another new driver (rtwlan/rtw88) under >> review so review from the wireless folks might take some time. >> > > Thanks for the update. > Btw I could see our driver was added to the pending branch. Just > curious, could you please share details about how the pending branch is > used ? I use pending branch mainly for testing bigger patches with kbuild bot, though I think the bot is offline this week. (Or was it next week? Can't remember) -- Kalle Valo
Re: [PATCH 01/12] rtwlan: main files
Larry Finger writes:
> On 10/2/18 9:57 PM, Tony Chuang wrote:
>>
>>
>>> -Original Message-
>>> From: Larry Finger [mailto:larry.fin...@gmail.com] On Behalf Of Larry Finger
>>> Sent: Tuesday, October 02, 2018 11:24 PM
>>> To: Stanislaw Gruszka; Tony Chuang
>>> Cc: kv...@codeaurora.org; linux-wireless@vger.kernel.org; Pkshih; Andy
>>> Huang
>>> Subject: Re: [PATCH 01/12] rtwlan: main files
>>>
>>> On 10/2/18 5:29 AM, Stanislaw Gruszka wrote:
>>>> On Fri, Sep 28, 2018 at 11:32:41AM +, Tony Chuang wrote:
>>>>>> if (rtw_hci_tx(rtwdev, _info, skb))
>>>>>> dev_kfree_skb_any(skb)
>>>>>>
>>>>>> just to remove 'return;' and out label.
>>>>>
>>>>>
>>>>> OK, but why not use ieee80211_free_txskb, should it be better for
>>> mac80211?
>>>>
>>>> Yes, it is better as it also do some extra thing for dropped frame.
>>>>
>>>>>>> OK, but I think this is needed, our tables have different forms
>>>>>>
>>>>>> Not sure if that is better solution, but could the tables be pre-prarsed
>>>>>> by user-space program and then embed in the driver in ready to send
>>>>>> to the hardware from ?
>>>>>>
>>>>>> Also there are lot of redundancy in those tables, for example:
>>>>>>
>>>>>> +0x81C, 0xFF03,
>>>>>> +0x81C, 0xF503,
>>>>>> +0x81C, 0xF4020003,
>>>>>> +0x81C, 0xF3040003,
>>>>>> +0x81C, 0xF2060003,
>>>>>> +0x81C, 0xF1080003,
>>>>>> +0x81C, 0xF00A0003,
>>>>>> +0x81C, 0xEF0C0003,
>>>>>> +0x81C, 0xEE0E0003,
>>>>>> +0x81C, 0xED13,
>>>>>> +0x81C, 0xEC120003,
>>>>>> +0x81C, 0xEB140003,
>>>>>> +0x81C, 0xEA160003,
>>>>>> +0x81C, 0xE9180003,
>>>>>> +0x81C, 0xE81A0003,
>>>>>> +0x81C, 0xE71C0003,
>>>>>> +0x81C, 0xE61E0003,
>>>>>> +0x81C, 0xE523,
>>>>>>
>>>>>> 0x81C and 0003 repeats in many lines.
>>>>>>
>>>>>> This seems to be parse data, not that we have to write 0x81C
>>>>>> register many times. Would be possible to remove the redundancy?
>>>>>
>>>>>
>>>>> No, they cannot be removed, the sequence matters.
>>>>> And it is really writing to 0x81C ...
>>>>> It is really magic, I cannot believe to this, too.
>>>>
>>>> This is contradiction for what I asked you before, i.e. doing parsing
>>>> in user space, but since we have this parsing mechanism in the driver
>>>> perhaps the tables can be coded in some more compact way, for example:
>>>>
>>>> { prefix, suffix, len, {data} }
>>>>
>>>> { 0x81C, 0x0003, N ,
>>>> { 0xFF02 , 0xF500 , , 0xE520 } }
>>>>
>>>> The rtw8822b_table.c file is quite big.
>>>
>>> You might also consider having these tables as a configuration file read
>>> from
>>> the firmware directory.
>>>
>>
>> Hi Larry,
>>
>> Can we put the configuration file in the firmware directory?
>> Should we package them into binary files? Or just put the raw data.
>>
>> We can test the performance for it. After we got the result, we will make a
>> decision
>> about it. And if we decide to put them in the firmware directory, will send
>> a patch.
>> For now, I think we can just leave them in the .c.
>
> Yes, you could put the configuration files in the firmware directory.
> I would put them in binary form, not as text files. That way the size
> would be smaller, and it would not be possible to alter them,
> particularly if the binary file is checksummed.
>
> It would likely be OK if only the agc table was stored in this way.
> That would take away about half of the lines in the 8822b table file.
So what's the worry here? The lines of source code, binary size or what?
.../net/wireless/realtek/rtw88/rtw8822b_table.c| 20783 +++
Looking at the diffstat rtw8822b_table.c seems to be 20 kLOC, IMHO it's
not that bad as it's just data. But of course I might be missing
something as I haven't checked patches yet.
--
Kalle Valo
Re: [PATCH 01/12] rtwlan: main files
Larry Finger writes:
> On 10/2/18 5:29 AM, Stanislaw Gruszka wrote:
>> On Fri, Sep 28, 2018 at 11:32:41AM +, Tony Chuang wrote:
>>>>if (rtw_hci_tx(rtwdev, _info, skb))
>>>>dev_kfree_skb_any(skb)
>>>>
>>>> just to remove 'return;' and out label.
>>>
>>>
>>> OK, but why not use ieee80211_free_txskb, should it be better for mac80211?
>>
>> Yes, it is better as it also do some extra thing for dropped frame.
>>
>>>>> OK, but I think this is needed, our tables have different forms
>>>>
>>>> Not sure if that is better solution, but could the tables be pre-prarsed
>>>> by user-space program and then embed in the driver in ready to send
>>>> to the hardware from ?
>>>>
>>>> Also there are lot of redundancy in those tables, for example:
>>>>
>>>> + 0x81C, 0xFF03,
>>>> + 0x81C, 0xF503,
>>>> + 0x81C, 0xF4020003,
>>>> + 0x81C, 0xF3040003,
>>>> + 0x81C, 0xF2060003,
>>>> + 0x81C, 0xF1080003,
>>>> + 0x81C, 0xF00A0003,
>>>> + 0x81C, 0xEF0C0003,
>>>> + 0x81C, 0xEE0E0003,
>>>> + 0x81C, 0xED13,
>>>> + 0x81C, 0xEC120003,
>>>> + 0x81C, 0xEB140003,
>>>> + 0x81C, 0xEA160003,
>>>> + 0x81C, 0xE9180003,
>>>> + 0x81C, 0xE81A0003,
>>>> + 0x81C, 0xE71C0003,
>>>> + 0x81C, 0xE61E0003,
>>>> + 0x81C, 0xE523,
>>>>
>>>> 0x81C and 0003 repeats in many lines.
>>>>
>>>> This seems to be parse data, not that we have to write 0x81C
>>>> register many times. Would be possible to remove the redundancy?
>>>
>>>
>>> No, they cannot be removed, the sequence matters.
>>> And it is really writing to 0x81C ...
>>> It is really magic, I cannot believe to this, too.
>>
>> This is contradiction for what I asked you before, i.e. doing parsing
>> in user space, but since we have this parsing mechanism in the driver
>> perhaps the tables can be coded in some more compact way, for example:
>>
>> { prefix, suffix, len, {data} }
>>
>> { 0x81C, 0x0003, N ,
>>{ 0xFF02 , 0xF500 , , 0xE520 } }
>>
>> The rtw8822b_table.c file is quite big.
>
> You might also consider having these tables as a configuration file
> read from the firmware directory.
Configuration files for upstream drivers is something I would like to
avoid until we have a generic solution. That seems to come up now and
then but there does not seem to be any consensus how it should be
implemented.
--
Kalle Valo
Re: [PATCH 01/12] rtwlan: main files
Stanislaw Gruszka writes:
> On Thu, Sep 27, 2018 at 10:40:44AM -0500, Larry Finger wrote:
>> On 9/27/18 8:50 AM, Stanislaw Gruszka wrote:
>> --snip
>>
>> >
>> > > +#define BIT_LEN_MASK_32(__bitlen) (0x >> (32 - (__bitlen)))
>> > > +#define BIT_OFFSET_LEN_MASK_32(__bitoffset, __bitlen)
>> > >\
>> > > +(BIT_LEN_MASK_32(__bitlen) << (__bitoffset))
>> > > +#define LE_P4BYTE_TO_HOST_4BYTE(__start) (le32_to_cpu(*((__le32
>> > > *)(__start
>> > > +#define LE_BITS_CLEARED_TO_4BYTE(__start, __bitoffset, __bitlen)
>> > >\
>> > > +(LE_P4BYTE_TO_HOST_4BYTE(__start) &
>> > >\
>> > > + (~BIT_OFFSET_LEN_MASK_32(__bitoffset, __bitlen)))
>> > > +#define LE_BITS_TO_4BYTE(__start, __bitoffset, __bitlen)
>> > >\
>> > > +((LE_P4BYTE_TO_HOST_4BYTE(__start) >> (__bitoffset)) &
>> > >\
>> > > + BIT_LEN_MASK_32(__bitlen))
>> > > +#define SET_BITS_TO_LE_4BYTE(__start, __bitoffset, __bitlen, __value)
>> > >\
>> > > +do {
>> > >\
>> > > +*((__le32 *)(__start)) = \
>> > > +cpu_to_le32( \
>> > > +LE_BITS_CLEARED_TO_4BYTE(__start, __bitoffset,
>> > > __bitlen) | \
>> > > +u32)__value) & BIT_LEN_MASK_32(__bitlen)) <<
>> > > (__bitoffset))\
>> > > +);
>> > >\
>> > > +} while (0)
This is horrible.
>> Stanislaw,
>>
>> I have never loved these macros, and it took a lot of time to get them to be
>> endian correct. Could you point me to a method that would overwrite a
>> portion of a 32-bit little-endian word that would be correct for both
>> little- and big-endian machines? Keep in mind that Kalle hates the use of
>> compile tests on __LITTLE_ENDIAN.
>
> Maybe something like this (not tested)
>
> #define SET_LE32(reg, off, len, val) \
> ((reg & cpu_to_le32(~GENMASK(off + len - 1, off))) | cpu_to_le32(val <<
> off))
>
> ?
>
> There are plenty of bitops and endian primitives in kernel, it's
> very very unlikly you need custom macros for handle that.
Indeed, try avoiding reinventing wheel as much as possible.
--
Kalle Valo
