RE: [EXT] Re: [PATCH v2] mwifiex: don't call del_timer_sync() on uninitialized timer
Hi Tetsuo, > > "grep -FrB1 'del_timer' drivers/ | grep -FA1 '.function)'" says that > > currently there are 28 locations which call del_timer[_sync]() only if > > that timer's function field was initialized (because timer_setup() > > sets that timer's function field). Therefore, let's use same approach here. Thanks for the change, it look cleaner than my re-work; Acked-by: Ganapathi Bhat Regards, Ganapathi
RE: [EXT] [PATCH] mwifiex: Fix reporting 'operation not supported' error code
Hi Pali, > This patch fixes problem that mwifiex kernel driver sends to userspace > unsupported error codes like: "failed: -524 (No error information)". > After applying this patch userspace see: "failed: -95 (Not supported)". > OK, yes this was a mistake. Thank you for this change. Acked-by: Ganapathi Bhat Regards, Ganapathi
RE: [EXT] mwifiex: Firmware name for W8997 sdio wifi chip
Hi Pali, > > > > Hello Ganapathi! Seems that on both locations is older version of > > > > sdsd8997_combo_v4.bin firmware, not the latest one. On both > > > > location is available just version 16.68.1.p179. But we have newer > version 16.68.1.p197. Where did you find p197, kindly let me know so that I can try to get it's details and upstream. > > > > Could you please recheck it? > > > > > > p179 do have the fix but we will try to upstream p197 also soon. > > > > Thank you! > > Hello Ganapathi! Do you have any updates about upstreaming new firmware > version? Regards, Ganapathi
RE: [EXT] [PATCH 3/4] btmrvl: Fix firmware filename for sd8977 chipset
Acked-by: Ganapathi Bhat
RE: [EXT] [PATCH 1/4] mwifiex: Fix firmware filename for sd8977 chipset
Acked-by: Ganapathi Bhat
RE: [EXT] [PATCH 4/4] btmrvl: Fix firmware filename for sd8997 chipset
Acked-by: Ganapathi Bhat
RE: [EXT] [PATCH 2/4] mwifiex: Fix firmware filename for sd8997 chipset
Acked-by: Ganapathi Bhat
RE: [EXT] [PATCH 0/4] marvell: Fix firmware filenames for sd8977/sd8997 chipsets
Hi Pali, > This patch series fixes mwifiex and btmrvl drivers to load firmware for > sd8977 and sd8997 chipsets from correct filename. Thanks you for the changes, I will ack each patch; Regards, Ganapathi
RE: [EXT] mwifiex: Firmware name for W8997 sdio wifi chip
Hi Pali, > Hello Ganapathi! Seems that on both locations is older version of > sdsd8997_combo_v4.bin firmware, not the latest one. On both location is > available just version 16.68.1.p179. But we have newer version 16.68.1.p197. > Could you please recheck it? p179 do have the fix but we will try to upstream p197 also soon. Regards, Ganapathi
RE: [EXT] mwifiex: Firmware name for W8997 sdio wifi chip
Hi Pali, > Hello Ganapathi! Thank you for information. Can you point me to git tree or > location where are firmware files already updated? https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git or https://github.com/NXP/mwifiex-firmware Regards, Ganapathi
RE: [EXT] mwifiex: Firmware name for W8997 sdio wifi chip
Hi Pali, > According to publicly available information, firmware for these W8xxx > Marvell wifi chips is vulnerable to security issue CVE-2019-6496 [1]. > > Are you able to update firmwares to the last versions and give us some > information which (old) firmware versions are affected by that security issue? > > So Linux distribution would know if they are distributing older vulnerable > firmwares and should push security fixes with new firmware versions. > 88W8787, 88W8797, 88W8801, 88W8897, and 88W8997 models are all already updated, with one exception: usb8897_uapsta.bin, which we will upstream soon; Regards, Ganapathi
RE: [EXT] Re: [PATCH] mwifiex: Parse all API_VER_ID properties
Hi Pali, > Hello! Could you please look at this trivial patch? The change look good. Acked-by: Ganapathi Bhat
RE: [EXT] [PATCH 01/11] mmc: sdio: Fix macro name for Marvell device with ID 0x9134
HI Pali, > Kernel quirks for SDIO devices are > matched against device ID from SDIO Common CIS. Therefore device ID used > in quirk is correct, just has misleading name. OK, understood. Thanks for the change. Acked-by: Ganapathi Bhat
RE: [EXT] [PATCH 02/11] mmc: sdio: Change macro names for Marvell 8688 modules
Hi Pali, > Add underscore as separator in Marvell 8688 macro names for better > readability and consistency. > Thanks for the change; Acked-by: Ganapathi Bhat
RE: [EXT] [PATCH 03/11] mmc: sdio: Move SDIO IDs from mwifiex driver to common include file
Hi Pali, > Add _WLAN suffix to macro names for consistency with other Marvell macros. > These IDs represents wlan function of combo bt/wlan cards. Other functions > of these cards have different IDs. > OK, thanks for the cleanup change; Acked-by: Ganapathi Bhat
RE: [EXT] [PATCH 04/11] mmc: sdio: Move SDIO IDs from btmrvl driver to common include file
Hi Pali, > Define appropriate macro names for consistency with other Marvell macros. > Thanks for the change; Acked-by: Ganapathi Bhat
RE: [EXT] mwifiex: Firmware name for W8997 sdio wifi chip
Hi Pali, Thanks for this notice. We will try to push the new firmware and also, fix the naming problem. Regards, Ganapathi
RE: [EXT] [PATCH] mwifiex: Fix memory corruption in dump_station
Hi Pali, > The mwifiex_cfg80211_dump_station() uses static variable for iterating over > a linked list of all associated stations (when the driver is in UAP role). > This has > a race condition if .dump_station is called in parallel for multiple > interfaces. > This corruption can be triggered by registering multiple SSIDs and calling, in > parallel for multiple interfaces > iw dev station dump Thanks for this change. Acked-by: Ganapathi Bhat Regards, Ganapathi
RE: [EXT] [PATCH] mwifiex: pcie: Fix memory leak in mwifiex_pcie_alloc_cmdrsp_buf
Hi Navid,
> Fixes: fc3314609047 ("mwifiex: use pci_alloc/free_consistent APIs for PCIe")
Thanks for the change;
Acked-by: Ganapathi Bhat
Regards,
Ganapathi
RE: [EXT] [PATCH] mwifiex: pcie: Fix memory leak in mwifiex_pcie_init_evt_ring
Hi Navid,
> Fixes: 0732484b47b5 ("mwifiex: separate ring initialization and ring creation
> routines")
Thanks for the this change as well;
Acked-by: Ganapathi Bhat
Regards,
Ganapathi
RE: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
Hi Andy, > I was wondering if you've posted the updated version of the fix? Sorry for the delay; I have started addressing the comment from community; It should be done in couple of days; Regards, Ganapathi
RE: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
Hi Dmitry, We have a patch to fix this: https://patchwork.kernel.org/patch/10990275/ Regards, Ganapathi
RE: [EXT] [PATCH 1/2] mwifiex: dispatch/rotate from reorder table atomically
Hi Brian,
> (1) iterating / clearing the mwifiex reordering table
> (2) dispatching received packets to upper layers
>
> This makes it much harder to make lock recursion mistakes, as these two
> steps no longer need to hold the same locks.
Yes, this is clean;
>
> Testing: I've played with a variety of stress tests, including download stress
> tests on the same APs which caught regressions with commit
> 5188d5453bc9 ("mwifiex: restructure rx_reorder_tbl_lock usage"). I've
> primarily tested on Marvell 8997 / PCIe, although I've given 8897 / SDIO a
> quick spin as well.
>
Thanks a lot for this; We will also run the tests locally; But, I find the
change is good;
Acked-by: Ganapathi Bhat
Regards,
Ganapathi
RE: [EXT] Re: [4.20 PATCH] Revert "mwifiex: restructure rx_reorder_tbl_lock usage"
Hi Brian, > >netif_rx_ni+0xe8/0x120 > >mwifiex_recv_packet+0xfc/0x10c [mwifiex] > >mwifiex_process_rx_packet+0x1d4/0x238 [mwifiex] > >mwifiex_11n_dispatch_pkt+0x190/0x1ac [mwifiex] > >mwifiex_11n_rx_reorder_pkt+0x28c/0x354 [mwifiex] > > TL;DR: the problem was right here ^^^ > where you started running mwifiex_11n_dispatch_pkt() (via > mwifiex_11n_scan_and_dispatch()) while holding a spinlock. > > When you do that, you eventually call netif_rx_ni(), which specifically defers > to softirq contexts. Then, if you happen to have your flush timer expiring > just > before that, you end up in mwifiex_flush_data(), which also needs that > spinlock. Understood; Thanks for this detail; > > There are a few possible ways to handle this: > (a) prevent processing softirqs in that context; e.g., with > local_bh_disable(). This seems somewhat of a hack. > (Side note: I think most of the locks in this driver really could be > spin_lock_bh(), not spin_lock_irqsave() -- we don't really care > about hardirq context for 99% of these locks.) > (b) restructure so that packet processing (e.g., netif_rx_ni()) is done > outside of the spinlock. > > It's actually not that hard to do (b). You can just queue your skb's up in a > temporary sk_buff_head list and process them all at once after you've > finished processing the reorder table. I have a local patch to do this, and I > might send it your way if I can give it a bit more testing. OK; That will be good; We will run a complete test after the patch; (OR we can work on this, share for review); Regards, Ganapathi
RE: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
Hi Dmitry, > The "fixed" status relates to the similar past bug that was reported and fixed > more than a year ago: Oh OK; We understood the issue, working on a change to fix this; Thanks, Ganapathi
RE: [EXT] Re: [PATCH] mwifiex: check for null return from skb_copy
Hi Dan,
> > > > if (is_multicast_ether_addr(ra)) {
> > > > skb_uap = skb_copy(skb, GFP_ATOMIC);
> > > > + if (!skb_uap)
> > > > + return -ENOMEM;
> > >
> > > I think we would want to free dev_kfree_skb_any(skb) before returning.
> > I think if the pointer is NULL, no need to free it;
>
> You're misreading skb vs skb_uap. "skb_uap" is NULL but "skb" is non-NULL
> and I'm pretty sure we should free it.
Oh, right. I missed it; Yes you are correct.
Regards,
Ganapathi
RE: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
Hi syzbot, > > syzbot found the following crash on: > As per the link(https://syzkaller.appspot.com/bug?extid=dc4127f950da51639216), the issue is fixed; Is it OK? Let us know if we need to do something? Regards, Ganapathi
RE: [EXT] Re: [PATCH] mwifiex: check for null return from skb_copy
Hi Dan,
> > if (is_multicast_ether_addr(ra)) {
> > skb_uap = skb_copy(skb, GFP_ATOMIC);
> > + if (!skb_uap)
> > + return -ENOMEM;
>
> I think we would want to free dev_kfree_skb_any(skb) before returning.
I think if the pointer is NULL, no need to free it;
Regards,
Ganapathi
Re: [EXT] Re: [RFC PATCH v2 2/2] mwifiex: add NL80211_STA_INFO_RX_BITRATE support
Hi Kalle/Brian, >> RFC: I'd appreciate it if someone from Marvell could double check my work >> here. >BTW, if we don't hear anything from Marvell I'm going to apply these >anyway. This patch series looks good to us. Thanks, Ganapathi
RE: [EXT] Re: [PATCH] mwifiex: correct channel stat buffer overflows
Hi Brian,
> --
> On Thu, Jun 29, 2017 at 06:23:54PM -0700, Brian Norris wrote:
> > mwifiex records information about various channels as it receives
> scan
> > information. It does this by appending to a buffer that was sized to
> > the max number of supported channels on any band, but there are
> > numerous problems:
> >
> > (a) scans can return info from more than one band (e.g., both 2.4 and
> 5
> > GHz), so the determined "max" is not large enough
> > (b) some firmware appears to return multiple results for a given
> > channel, so the max *really* isn't large enough
> > (c) there is no bounds checking when stashing these stats, so
> problems
> > (a) and (b) can easily lead to buffer overflows
> >
> > Let's patch this by setting a slightly-more-correct max (that
> accounts
> > for a combination of both 2.4G and 5G bands) and adding a bounds
> check
> > when writing to our statistics buffer.
> >
> > Due to problem (b), we still might not properly report all known
> > survey information (e.g., with "iw survey dump"), since
> > duplicate results (or otherwise "larger than expected" results) will
> > cause some truncation. But that's a problem for a future bugfix.
> >
> > (And because of this known deficiency, only log the excess at the
> WARN
> > level, since that isn't visible by default in this driver and would
> > otherwise be a bit too noisy.)
> >
> > Fixes: bf35443314ac ("mwifiex: channel statistics support for
> > mwifiex")
> > Cc: <sta...@vger.kernel.org>
> > Cc: Avinash Patil <pat...@marvell.com>
> > Cc: Xinming Hu <h...@marvell.com>
> > Signed-off-by: Brian Norris <briannor...@chromium.org>
> > ---
> > I've got a ton of other patches still queued up locally, and I hope
> to
> > send them soon. But I realized this one is a nasty bug (with a
> trivial
> > fix), so it's probably best to get this out the door quickly.
>
> This does make sense to me.
>
> Reviewed-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
>
Changes look good.
Reviewed-by: Ganapathi Bhat <gb...@marvell.com>
> >
> > drivers/net/wireless/marvell/mwifiex/cfg80211.c | 2 +-
> > drivers/net/wireless/marvell/mwifiex/scan.c | 6 ++
> > 2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > index a850ec0054e2..82f4e796ed39 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > @@ -4219,7 +4219,7 @@ int mwifiex_init_channel_scan_gap(struct
> mwifiex_adapter *adapter)
> > if (adapter->config_bands & BAND_A)
> > n_channels_a = mwifiex_band_5ghz.n_channels;
> >
> > - adapter->num_in_chan_stats = max_t(u32, n_channels_bg,
> n_channels_a);
> > + adapter->num_in_chan_stats = n_channels_bg + n_channels_a;
> > adapter->chan_stats = vmalloc(sizeof(*adapter->chan_stats) *
> > adapter->num_in_chan_stats);
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c
> > b/drivers/net/wireless/marvell/mwifiex/scan.c
> > index ae9630b49342..9900855746ac 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> > @@ -2492,6 +2492,12 @@ mwifiex_update_chan_statistics(struct
> mwifiex_private *priv,
> > sizeof(struct mwifiex_chan_stats);
> >
> > for (i = 0 ; i < num_chan; i++) {
> > + if (adapter->survey_idx >= adapter->num_in_chan_stats) {
> > + mwifiex_dbg(adapter, WARN,
> > + "FW reported too many channel results (max
> %d)\n",
> > + adapter->num_in_chan_stats);
> > + return;
> > + }
> > chan_stats.chan_num = fw_chan_stats->chan_num;
> > chan_stats.bandcfg = fw_chan_stats->bandcfg;
> > chan_stats.flags = fw_chan_stats->flags;
> > --
> > 2.13.2.725.g09c95d1e9-goog
> >
>
> --
> Dmitry
Regards,
Ganapathi
RE: [EXT] Re: [PATCH] mwifiex: correct channel stat buffer overflows
Hi Brian,
> --
> On Thu, Jun 29, 2017 at 06:23:54PM -0700, Brian Norris wrote:
> > mwifiex records information about various channels as it receives
> scan
> > information. It does this by appending to a buffer that was sized to
> > the max number of supported channels on any band, but there are
> > numerous problems:
> >
> > (a) scans can return info from more than one band (e.g., both 2.4 and
> 5
> > GHz), so the determined "max" is not large enough
> > (b) some firmware appears to return multiple results for a given
> > channel, so the max *really* isn't large enough
> > (c) there is no bounds checking when stashing these stats, so
> problems
> > (a) and (b) can easily lead to buffer overflows
> >
> > Let's patch this by setting a slightly-more-correct max (that
> accounts
> > for a combination of both 2.4G and 5G bands) and adding a bounds
> check
> > when writing to our statistics buffer.
> >
> > Due to problem (b), we still might not properly report all known
> > survey information (e.g., with "iw survey dump"), since
> > duplicate results (or otherwise "larger than expected" results) will
> > cause some truncation. But that's a problem for a future bugfix.
> >
> > (And because of this known deficiency, only log the excess at the
> WARN
> > level, since that isn't visible by default in this driver and would
> > otherwise be a bit too noisy.)
> >
> > Fixes: bf35443314ac ("mwifiex: channel statistics support for
> > mwifiex")
> > Cc:
> > Cc: Avinash Patil
> > Cc: Xinming Hu
> > Signed-off-by: Brian Norris
> > ---
> > I've got a ton of other patches still queued up locally, and I hope
> to
> > send them soon. But I realized this one is a nasty bug (with a
> trivial
> > fix), so it's probably best to get this out the door quickly.
>
> This does make sense to me.
>
> Reviewed-by: Dmitry Torokhov
>
Changes look good.
Reviewed-by: Ganapathi Bhat
> >
> > drivers/net/wireless/marvell/mwifiex/cfg80211.c | 2 +-
> > drivers/net/wireless/marvell/mwifiex/scan.c | 6 ++
> > 2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > index a850ec0054e2..82f4e796ed39 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > @@ -4219,7 +4219,7 @@ int mwifiex_init_channel_scan_gap(struct
> mwifiex_adapter *adapter)
> > if (adapter->config_bands & BAND_A)
> > n_channels_a = mwifiex_band_5ghz.n_channels;
> >
> > - adapter->num_in_chan_stats = max_t(u32, n_channels_bg,
> n_channels_a);
> > + adapter->num_in_chan_stats = n_channels_bg + n_channels_a;
> > adapter->chan_stats = vmalloc(sizeof(*adapter->chan_stats) *
> > adapter->num_in_chan_stats);
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c
> > b/drivers/net/wireless/marvell/mwifiex/scan.c
> > index ae9630b49342..9900855746ac 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> > @@ -2492,6 +2492,12 @@ mwifiex_update_chan_statistics(struct
> mwifiex_private *priv,
> > sizeof(struct mwifiex_chan_stats);
> >
> > for (i = 0 ; i < num_chan; i++) {
> > + if (adapter->survey_idx >= adapter->num_in_chan_stats) {
> > + mwifiex_dbg(adapter, WARN,
> > + "FW reported too many channel results (max
> %d)\n",
> > + adapter->num_in_chan_stats);
> > + return;
> > + }
> > chan_stats.chan_num = fw_chan_stats->chan_num;
> > chan_stats.bandcfg = fw_chan_stats->bandcfg;
> > chan_stats.flags = fw_chan_stats->flags;
> > --
> > 2.13.2.725.g09c95d1e9-goog
> >
>
> --
> Dmitry
Regards,
Ganapathi
RE: [EXT] Re: [PATCH] MAINTAINERS: drop akarwar from mwifiex
Hi Kalle/Brian, > Brian Norris <briannor...@chromium.org> writes: > > > His email is bouncing, and I expect he's not doing this work any > more. > > > > Cc: Amitkumar Karwar <amitkar...@gmail.com> > > Cc: Nishant Sarmukadam <nisha...@marvell.com> > > Cc: Ganapathi Bhat <gb...@marvell.com> > > Cc: Xinming Hu <h...@marvell.com> > > Signed-off-by: Brian Norris <briannor...@chromium.org> > > --- > > Or alternatively, we can update his email address. I just don't want > > to keep seeing bounced emails :) > > Me neither :) So Amitkumar, which one do you prefer? > Yes. Amit's official mail id (akar...@marvell.com) can be removed. > -- > Kalle Valo Regards, Ganapathi
RE: [EXT] Re: [PATCH] MAINTAINERS: drop akarwar from mwifiex
Hi Kalle/Brian, > Brian Norris writes: > > > His email is bouncing, and I expect he's not doing this work any > more. > > > > Cc: Amitkumar Karwar > > Cc: Nishant Sarmukadam > > Cc: Ganapathi Bhat > > Cc: Xinming Hu > > Signed-off-by: Brian Norris > > --- > > Or alternatively, we can update his email address. I just don't want > > to keep seeing bounced emails :) > > Me neither :) So Amitkumar, which one do you prefer? > Yes. Amit's official mail id (akar...@marvell.com) can be removed. > -- > Kalle Valo Regards, Ganapathi
