RE: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv
> -Original Message- > From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com] > Sent: 2016年11月4日 2:49 > To: Brian Norris > Cc: Xinming Hu; Amitkumar Karwar; linux-wireless@vger.kernel.org; Cathy Luo; > Nishant Sarmukadam; raja...@google.com > Subject: Re: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' in > shutdown_drv > > On Thu, Nov 3, 2016 at 12:27 PM, Brian Norris > wrote: > > On Thu, Nov 03, 2016 at 09:15:04AM -0700, Dmitry Torokhov wrote: > >> On Thu, Nov 03, 2016 at 08:34:06AM +, Xinming Hu wrote: > >> > > -Original Message- > >> > > From: linux-wireless-ow...@vger.kernel.org > >> > > [mailto:linux-wireless-ow...@vger.kernel.org] On Behalf Of Dmitry > >> > > Torokhov > >> > > > >> > > Instead please remove call to mwifiex_shutdown_drv() in the main > >> > > routine and "if (adapter->mwifiex_processing)" check here. > >> > > > >> > > >> > mwifiex_main_process will be used from interrupt or workqueue. > >> > Now we have disabled interrupt and flush workqueue, so > >> > mwifiex_main_process won't be scheduled in the future. > >> > But mwifiex_main_process might just running in context of last > >> > interrupt, so we need wait current main_process complete in > >> > mwifiex_shutdown_drv. > >> > >> synchronize_irq() is your friend then. > > > > Hmm, that sounds right, but IIUC, the "interrupt context" is actually > > only used for SDIO, and for SDIO, the driver doesn't actually have > > access to the IRQ number. The MMC/SDIO layer has some extra > > abstraction around the IRQ. So this may be more difficult than it appears. > > > > Do we need a sdio_synchronize_irq() API? > > Actually the ->disable_irq() method should be responsible for making sure it > does not complete while interrupt handler is running. As far as I can see, for > SDIO case, we end up calling sdio_card_irq_put() which stops kernel thread > and won't return while the thread is running. For other interfaces we need to > check. IIRC USB lacks > ->disable_irq() altogether and this is something that shoudl be fixed > (by doing usb_kill_urb() at the minimum). > Thanks for the explain, will keep this cleanup work. > Thanks. > > -- > Dmitry Regards, Xinming
Re: Avery's notes from LPC2016 wireless track (Santa Fe)
On Thu, Nov 3, 2016 at 7:55 PM, Kathy Giori wrote: > On Thu, Nov 3, 2016 at 1:43 PM, Avery Pennarun wrote: >> We talked about many topics at the Linux Plumbers' Conference in Santa >> Fe on Tuesday. I took fairly detailed notes, which you can find at >> the links below. > > Great notes Avery. Did you happen to take note of how many > participants there were? Or was the attendee list posted on the wiki > beforehand fairly accurate? > https://wireless.wiki.kernel.org/en/developers/summits/santa-fe-2016 There were quite a few people not on the list. Google sent an extra-large contingent last year. People estimated about 50% of the wireless meeting were Googlers, which comes to maybe 15 out of around 30-ish. Google is doing a lot of (sometimes redundant :)) wireless work lately. > Was there any talk about having more frequent "live" discussions of > these topics (via video conf or conf call or scheduled IRC), to help > overall collaboration without having to wait for the next f2f summit? > Mailing list interaction doesn't seem to elicit the same energy as > occurs over live dialog. Quarterly? It didn't come up. Honestly, it feels to me like 6 months is the right cadence. A lot of work does happen in the background, such as the fq_codel work (yay!) which was definitely launched by one or two of these, but proceeded well afterwards. Hope you're doing well at whatever you're doing now! Have fun, Avery
Re: Avery's notes from LPC2016 wireless track (Santa Fe)
On Thu, Nov 3, 2016 at 5:55 PM, Barry Day wrote: > Thanks for that. Can I take this as meaning there won't be any videos? > I would like to have seen Jes Sorensen's talk on rtl8xxxu As far as I know, no talks at this LPC were recorded.
Re: Avery's notes from LPC2016 wireless track (Santa Fe)
On Thu, Nov 3, 2016 at 1:43 PM, Avery Pennarun wrote: > Hi all, > > We talked about many topics at the Linux Plumbers' Conference in Santa > Fe on Tuesday. I took fairly detailed notes, which you can find at > the links below. Great notes Avery. Did you happen to take note of how many participants there were? Or was the attendee list posted on the wiki beforehand fairly accurate? https://wireless.wiki.kernel.org/en/developers/summits/santa-fe-2016 Was there any talk about having more frequent "live" discussions of these topics (via video conf or conf call or scheduled IRC), to help overall collaboration without having to wait for the next f2f summit? Mailing list interaction doesn't seem to elicit the same energy as occurs over live dialog. Quarterly? kathy
Re: Avery's notes from LPC2016 wireless track (Santa Fe)
Thanks for that. Can I take this as meaning there won't be any videos? I would like to have seen Jes Sorensen's talk on rtl8xxxu Barry
Avery's notes from LPC2016 wireless track (Santa Fe)
Hi all, We talked about many topics at the Linux Plumbers' Conference in Santa Fe on Tuesday. I took fairly detailed notes, which you can find at the links below. Fancy html commentable version: https://docs.google.com/document/d/1Cr2bEf23wLkhiXXtyuBJtvvpb9xvCw7zsU-m1q1g4tA/edit Less fancy version that (I think) will not ask for a Google account (let me know if it gives you trouble): https://docs.google.com/document/u/1/d/e/2PACX-1vQXbVQ-3zQt3Bcr3OWfwzbw_C49tTvf0ed8Hmf7b20E6tXc3a40tWZmPku49iGDE-OhgxNmO_lkkHEn/pub Thanks, everyone, for the great discussion! Have fun, Avery
Re: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv
On Thu, Nov 3, 2016 at 12:27 PM, Brian Norris wrote: > On Thu, Nov 03, 2016 at 09:15:04AM -0700, Dmitry Torokhov wrote: >> On Thu, Nov 03, 2016 at 08:34:06AM +, Xinming Hu wrote: >> > > -Original Message- >> > > From: linux-wireless-ow...@vger.kernel.org >> > > [mailto:linux-wireless-ow...@vger.kernel.org] On Behalf Of Dmitry >> > > Torokhov >> > > >> > > Instead please remove call to mwifiex_shutdown_drv() in the main routine >> > > and "if (adapter->mwifiex_processing)" check here. >> > > >> > >> > mwifiex_main_process will be used from interrupt or workqueue. >> > Now we have disabled interrupt and flush workqueue, so >> > mwifiex_main_process won't be scheduled in the future. >> > But mwifiex_main_process might just running in context of last >> > interrupt, so we need wait current main_process complete in >> > mwifiex_shutdown_drv. >> >> synchronize_irq() is your friend then. > > Hmm, that sounds right, but IIUC, the "interrupt context" is actually > only used for SDIO, and for SDIO, the driver doesn't actually have > access to the IRQ number. The MMC/SDIO layer has some extra abstraction > around the IRQ. So this may be more difficult than it appears. > > Do we need a sdio_synchronize_irq() API? Actually the ->disable_irq() method should be responsible for making sure it does not complete while interrupt handler is running. As far as I can see, for SDIO case, we end up calling sdio_card_irq_put() which stops kernel thread and won't return while the thread is running. For other interfaces we need to check. IIRC USB lacks ->disable_irq() altogether and this is something that shoudl be fixed (by doing usb_kill_urb() at the minimum). Thanks. -- Dmitry
Re: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv
On Thu, Nov 03, 2016 at 09:15:04AM -0700, Dmitry Torokhov wrote: > On Thu, Nov 03, 2016 at 08:34:06AM +, Xinming Hu wrote: > > > -Original Message- > > > From: linux-wireless-ow...@vger.kernel.org > > > [mailto:linux-wireless-ow...@vger.kernel.org] On Behalf Of Dmitry Torokhov > > > > > > Instead please remove call to mwifiex_shutdown_drv() in the main routine > > > and "if (adapter->mwifiex_processing)" check here. > > > > > > > mwifiex_main_process will be used from interrupt or workqueue. > > Now we have disabled interrupt and flush workqueue, so > > mwifiex_main_process won't be scheduled in the future. > > But mwifiex_main_process might just running in context of last > > interrupt, so we need wait current main_process complete in > > mwifiex_shutdown_drv. > > synchronize_irq() is your friend then. Hmm, that sounds right, but IIUC, the "interrupt context" is actually only used for SDIO, and for SDIO, the driver doesn't actually have access to the IRQ number. The MMC/SDIO layer has some extra abstraction around the IRQ. So this may be more difficult than it appears. Do we need a sdio_synchronize_irq() API? Brian
Re: [PATCH v2] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC
John Heenan writes: > The rtl8723bu wireless IC shows evidence of a more agressive approach to > power saving, powering down its RF side when there is no wireless > interfacing but leaving USB interfacing intact. This makes the wireless > IC more suitable for use in devices which need to keep their power use > as low as practical, such as tablets and Surface Pro type devices. > > In effect this means that a full initialisation must be performed > whenever a wireless interface is brought up. It also means that > interpretations of power status from general wireless registers should > not be relied on to influence an init sequence. > > The patch works by forcing a fuller initialisation and forcing it to > occur more often in code paths (such as occurs during a low level > authentication that initiates wireless interfacing). > > The initialisation sequence is now more consistent with code based > directly on vendor code. For example while the vendor derived code > interprets a register as indcating a particular powered state, it does > not use this information to influence its init sequence. > > Only devices that use the rtl8723bu driver are affected by this patch. > > With this patch wpa_supplicant reliably and consistently connects with > an AP. Before a workaround such as executing rmmod and modprobe before > each call to wpa_supplicant worked with some distributions. > > Signed-off-by: John Heenan > --- > drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 16 > 1 file changed, 12 insertions(+), 4 deletions(-) > I am at Linux Plumbers this week, so my response time is slow. Next week I am on PTO, so I will not respond. First of all, why do you keep CC'ing multiple mailing lists that do not matter? This discussion belongs on linux-wireless not on netdev or lkml. CC'ing Kalle directly is not going to get him to apply this broken patch for you. > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > index 04141e5..ab2f2ef 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > @@ -3900,7 +3900,7 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw) >* Fix 92DU-VC S3 hang with the reason is that secondary mac is not >* initialized. First MAC returns 0xea, second MAC returns 0x00 >*/ > - if (val8 == 0xea) > + if (val8 == 0xea || priv->fops == &rtl8723bu_fops) > macpower = false; > else > macpower = true; Why oh why do you insist on not using the *standard* way of coping with this? 'priv-rtl_chip' is used everywhere else, but you just have to do something awful like this? > @@ -5779,6 +5779,12 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw) > > ret = 0; > > + if(priv->fops == &rtl8723bu_fops) { > + ret = rtl8xxxu_init_device(hw); > + if (ret) > + goto error_out; > + } > + > init_usb_anchor(&priv->rx_anchor); > init_usb_anchor(&priv->tx_anchor); > init_usb_anchor(&priv->int_anchor); Read Documentation/CodingStyle - as others already pointed you at. > @@ -6080,9 +6086,11 @@ static int rtl8xxxu_probe(struct usb_interface > *interface, > goto exit; > } > > - ret = rtl8xxxu_init_device(hw); > - if (ret) > - goto exit; > + if(priv->fops != &rtl8723bu_fops) { > + ret = rtl8xxxu_init_device(hw); > + if (ret) > + goto exit; > + } > > hw->wiphy->max_scan_ssids = 1; > hw->wiphy->max_scan_ie_len = IEEE80211_MAX_DATA_LEN; Again coding style violation. Second, I am NOT going to accept any patches that fundamentally changes the init sequence of the code for just one device. I already told you I want to find out *why* this matters, and what part of rtl8xxxu_init_device() is the culprit. I want to understand the actual problem, not just blindly move stuff around. Jes
Re: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv
On Thu, Nov 03, 2016 at 08:34:06AM +, Xinming Hu wrote: > Hi Dmitry, > > > -Original Message- > > From: linux-wireless-ow...@vger.kernel.org > > [mailto:linux-wireless-ow...@vger.kernel.org] On Behalf Of Dmitry Torokhov > > Sent: 2016年10月28日 1:44 > > To: Amitkumar Karwar > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam; > > raja...@google.com; briannor...@google.com > > Subject: Re: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' > > in > > shutdown_drv > > > > Hi Amit, > > > > On Thu, Oct 27, 2016 at 02:42:40PM +0530, Amitkumar Karwar wrote: > > > This variable is guarded by spinlock at all other places. This patch > > > takes care of missing spinlock usage in mwifiex_shutdown_drv(). > > > > Since in the previous discussion you stated that we inhibit interrupts and > > flush > > the workqueue so that mwifiex_shutdown_drv() can't run simultaneously with > > the main processing routine, why do we need this? > > > > Instead please remove call to mwifiex_shutdown_drv() in the main routine > > and "if (adapter->mwifiex_processing)" check here. > > > > mwifiex_main_process will be used from interrupt or workqueue. > Now we have disabled interrupt and flush workqueue, so mwifiex_main_process > won't be scheduled in the future. > But mwifiex_main_process might just running in context of last interrupt, so > we need wait current main_process complete in mwifiex_shutdown_drv. synchronize_irq() is your friend then. Thanks. -- Dmitry
Re: [PATCH v2 01/12] mwifiex: check tx_hw_pending before downloading sleep confirm
+ Wei-Ning On Tue, Nov 01, 2016 at 08:08:17PM +0800, Xinming Hu wrote: > From: Shengzhen Li > > We may get SLEEP event from firmware even if TXDone interrupt > for last Tx packet is still pending. In this case, we may > end up accessing PCIe memory for handling TXDone after power > save handshake is completed. This causes kernel crash with > external abort. > > This patch will only allow downloading sleep confirm > when no tx done interrupt is pending in the hardware. > > --- > v2: address format issues(Brain) > --- Nit: typically, it's best if the changelog is placed after the Sign-offs, so that the first "---" line denotes the beginning of text that can be discarded. (Typically the changelog doesn't go into the final git commit, and it also happens that git-am discards everything between the first "---" line and the actual patch content.) > Signed-off-by: Cathy Luo > Signed-off-by: Shengzhen Li > Signed-off-by: Xinming Hu > Signed-off-by: Amitkumar Karwar > --- > drivers/net/wireless/marvell/mwifiex/cmdevt.c | 5 +++-- > drivers/net/wireless/marvell/mwifiex/init.c | 1 + > drivers/net/wireless/marvell/mwifiex/main.h | 1 + > drivers/net/wireless/marvell/mwifiex/pcie.c | 5 + > 4 files changed, 10 insertions(+), 2 deletions(-) Overall, I think this change is good, and it tests out fine for me so far. Do we have the same problem for other interfaces too? e.g., SDIO? It seems to me that the root problem is generic (i.e., don't try to SLEEP while processing TX traffic) but the symptom just happened to be fatal on a particular PCIe controller. Brian > diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c > b/drivers/net/wireless/marvell/mwifiex/cmdevt.c > index 5347728..25a7475 100644 > --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c > +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c > @@ -1118,13 +1118,14 @@ mwifiex_cancel_pending_ioctl(struct mwifiex_adapter > *adapter) > void > mwifiex_check_ps_cond(struct mwifiex_adapter *adapter) > { > - if (!adapter->cmd_sent && > + if (!adapter->cmd_sent && !atomic_read(&adapter->tx_hw_pending) && > !adapter->curr_cmd && !IS_CARD_RX_RCVD(adapter)) > mwifiex_dnld_sleep_confirm_cmd(adapter); > else > mwifiex_dbg(adapter, CMD, > - "cmd: Delay Sleep Confirm (%s%s%s)\n", > + "cmd: Delay Sleep Confirm (%s%s%s%s)\n", > (adapter->cmd_sent) ? "D" : "", > + atomic_read(&adapter->tx_hw_pending) ? "T" : "", > (adapter->curr_cmd) ? "C" : "", > (IS_CARD_RX_RCVD(adapter)) ? "R" : ""); > } > diff --git a/drivers/net/wireless/marvell/mwifiex/init.c > b/drivers/net/wireless/marvell/mwifiex/init.c > index 82839d9..b36cb3f 100644 > --- a/drivers/net/wireless/marvell/mwifiex/init.c > +++ b/drivers/net/wireless/marvell/mwifiex/init.c > @@ -270,6 +270,7 @@ static void mwifiex_init_adapter(struct mwifiex_adapter > *adapter) > adapter->adhoc_11n_enabled = false; > > mwifiex_wmm_init(adapter); > + atomic_set(&adapter->tx_hw_pending, 0); > > sleep_cfm_buf = (struct mwifiex_opt_sleep_confirm *) > adapter->sleep_cfm->data; > diff --git a/drivers/net/wireless/marvell/mwifiex/main.h > b/drivers/net/wireless/marvell/mwifiex/main.h > index d61fe3a..7f67f23 100644 > --- a/drivers/net/wireless/marvell/mwifiex/main.h > +++ b/drivers/net/wireless/marvell/mwifiex/main.h > @@ -857,6 +857,7 @@ struct mwifiex_adapter { > atomic_t rx_pending; > atomic_t tx_pending; > atomic_t cmd_pending; > + atomic_t tx_hw_pending; > struct workqueue_struct *workqueue; > struct work_struct main_work; > struct workqueue_struct *rx_workqueue; > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c > b/drivers/net/wireless/marvell/mwifiex/pcie.c > index 063c707..4aa5d91 100644 > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c > @@ -516,6 +516,7 @@ static int mwifiex_pcie_enable_host_int(struct > mwifiex_adapter *adapter) > } > } > > + atomic_set(&adapter->tx_hw_pending, 0); > return 0; > } > > @@ -689,6 +690,7 @@ static void mwifiex_cleanup_txq_ring(struct > mwifiex_adapter *adapter) > card->tx_buf_list[i] = NULL; > } > > + atomic_set(&adapter->tx_hw_pending, 0); > return; > } > > @@ -1126,6 +1128,7 @@ static int mwifiex_pcie_send_data_complete(struct > mwifiex_adapter *adapter) > -1); > else > mwifiex_write_data_complete(adapter, skb, 0, 0); > + atomic_dec(&adapter->tx_hw_pending); > } > > card->tx_buf_list[wrdoneidx] = NULL; > @@ -1218,6 +1221,7 @@ mwifiex_pcie_send_data(struct mwifiex_adapter *a
Re: [PATCH 2/2] rtl8xxxu: Fix for bogus data used to determine macpower
On 11/03/2016 03:41 AM, Joe Perches wrote: On Sun, 2016-10-30 at 19:02 -0400, Jes Sorensen wrote: Code is 80 characters wide, and comments are /* */ never the ugly C++ crap. You might look at the recent Linus Torvalds authored commit 5e467652ffef (?printk: re-organize log_output() to be more legible") which does both of those: c99 // comments and > 80 columns. Absolutes are for zealots. Of course, but who is going to criticize Linus? I have gently chided him when an untested patch of his was inserted just before the final release, and broke my laptop. At least the bisection was pretty quick. :) Larry
Re: [PATCH 1/2] rtl8xxxu: Fix for authentication failure
On 11/03/2016 02:10 AM, John Heenan wrote: On 3 November 2016 at 11:00, Larry Finger wrote: On 10/30/2016 05:20 AM, John Heenan wrote: This fix enables the same sequence of init behaviour as the alternative working driver for the wireless rtl8723bu IC at https://github.com/lwfinger/rtl8723bu For exampe rtl8xxxu_init_device is now called each time userspace wpa_supplicant is executed instead of just once when modprobe is executed. After all the trouble you have had with your patches, I would expect you to use more care when composing the commit message. Note the typo in the paragraph above. OK, the nasty games continue and the message is not getting through. An appropriate response by a maintainer would have been to request I revise the code according to the way it has currently and elegantly revised in. I am NOT a maintainer for this driver. I do have an interest in it, but not in any official capacity. If you cannot accept constructive criticism, you are in the wrong activity. Please grow up! Larry
[PATCH] Revert "mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE"
This reverts commit c68df2e7be0c1238ea3c281fd744a204ef3b15a0. __sta_info_recalc_tim turns into a no-op if local->ops->set_tim is not set. This prevents the beacon TIM bit from being set for all drivers that do not implement this op (almost all of them), thus thoroughly essential AP mode powersave functionality. Cc: Emmanuel Grumbach Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE") Signed-off-by: Felix Fietkau --- net/mac80211/sta_info.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index 236d47e..1711bae 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -688,7 +688,7 @@ static void __sta_info_recalc_tim(struct sta_info *sta, bool ignore_pending) } /* No need to do anything if the driver does all */ - if (!local->ops->set_tim) + if (ieee80211_hw_check(&local->hw, AP_LINK_PS)) return; if (sta->dead) -- 2.10.1
Re: [PATCH] mac80211: fix broken AP mode handling of powersave clients
On Thu, Nov 3, 2016 at 1:08 PM, Felix Fietkau wrote: > On 2016-11-03 11:51, Emmanuel Grumbach wrote: >> On Thu, Nov 3, 2016 at 12:51 PM, Emmanuel Grumbach >> wrote: >>> On Thu, Nov 3, 2016 at 11:59 AM, Felix Fietkau wrote: Commit c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE") introduced a logic error, where __sta_info_recalc_tim turns into a no-op if local->ops->set_tim is not set. This prevents the beacon TIM bit from being set for all drivers that do not implement this op (almost all of them), thus thoroughly essential AP mode powersave functionality. Cc: Emmanuel Grumbach Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE") Signed-off-by: Felix Fietkau --- net/mac80211/sta_info.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index 236d47e..621734e 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -688,7 +688,7 @@ static void __sta_info_recalc_tim(struct sta_info *sta, bool ignore_pending) } /* No need to do anything if the driver does all */ - if (!local->ops->set_tim) + if (local->ops->set_tim) return; >>> >>> but ... then, you'll need call to drv_set_tim below... >> >> s/need/never/ >> >>> Apparently, we can't rely on the ops pointer to enter or not enter this >>> flow. > Are you going to submit a fix, or should I just send a revert of your > commit? > Go ahead and send a revert. I won't be fast enough :)
Re: [PATCH] mac80211: fix broken AP mode handling of powersave clients
On 2016-11-03 11:51, Emmanuel Grumbach wrote: > On Thu, Nov 3, 2016 at 12:51 PM, Emmanuel Grumbach > wrote: >> On Thu, Nov 3, 2016 at 11:59 AM, Felix Fietkau wrote: >>> Commit c68df2e7be0c ("mac80211: allow using AP_LINK_PS with >>> mac80211-generated TIM IE") introduced a logic error, where >>> __sta_info_recalc_tim turns into a no-op if local->ops->set_tim is not >>> set. This prevents the beacon TIM bit from being set for all drivers >>> that do not implement this op (almost all of them), thus thoroughly >>> essential AP mode powersave functionality. >>> >>> Cc: Emmanuel Grumbach >>> Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with >>> mac80211-generated TIM IE") >>> Signed-off-by: Felix Fietkau >>> --- >>> net/mac80211/sta_info.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c >>> index 236d47e..621734e 100644 >>> --- a/net/mac80211/sta_info.c >>> +++ b/net/mac80211/sta_info.c >>> @@ -688,7 +688,7 @@ static void __sta_info_recalc_tim(struct sta_info *sta, >>> bool ignore_pending) >>> } >>> >>> /* No need to do anything if the driver does all */ >>> - if (!local->ops->set_tim) >>> + if (local->ops->set_tim) >>> return; >> >> but ... then, you'll need call to drv_set_tim below... > > s/need/never/ > >> Apparently, we can't rely on the ops pointer to enter or not enter this flow. Are you going to submit a fix, or should I just send a revert of your commit? - Felix
Re: [PATCH] mac80211: fix broken AP mode handling of powersave clients
On Thu, Nov 3, 2016 at 12:51 PM, Emmanuel Grumbach wrote: > On Thu, Nov 3, 2016 at 11:59 AM, Felix Fietkau wrote: >> Commit c68df2e7be0c ("mac80211: allow using AP_LINK_PS with >> mac80211-generated TIM IE") introduced a logic error, where >> __sta_info_recalc_tim turns into a no-op if local->ops->set_tim is not >> set. This prevents the beacon TIM bit from being set for all drivers >> that do not implement this op (almost all of them), thus thoroughly >> essential AP mode powersave functionality. >> >> Cc: Emmanuel Grumbach >> Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with >> mac80211-generated TIM IE") >> Signed-off-by: Felix Fietkau >> --- >> net/mac80211/sta_info.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c >> index 236d47e..621734e 100644 >> --- a/net/mac80211/sta_info.c >> +++ b/net/mac80211/sta_info.c >> @@ -688,7 +688,7 @@ static void __sta_info_recalc_tim(struct sta_info *sta, >> bool ignore_pending) >> } >> >> /* No need to do anything if the driver does all */ >> - if (!local->ops->set_tim) >> + if (local->ops->set_tim) >> return; > > but ... then, you'll need call to drv_set_tim below... s/need/never/ > Apparently, we can't rely on the ops pointer to enter or not enter this flow.
Re: [PATCH] mac80211: fix broken AP mode handling of powersave clients
On Thu, Nov 3, 2016 at 11:59 AM, Felix Fietkau wrote: > Commit c68df2e7be0c ("mac80211: allow using AP_LINK_PS with > mac80211-generated TIM IE") introduced a logic error, where > __sta_info_recalc_tim turns into a no-op if local->ops->set_tim is not > set. This prevents the beacon TIM bit from being set for all drivers > that do not implement this op (almost all of them), thus thoroughly > essential AP mode powersave functionality. > > Cc: Emmanuel Grumbach > Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with > mac80211-generated TIM IE") > Signed-off-by: Felix Fietkau > --- > net/mac80211/sta_info.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c > index 236d47e..621734e 100644 > --- a/net/mac80211/sta_info.c > +++ b/net/mac80211/sta_info.c > @@ -688,7 +688,7 @@ static void __sta_info_recalc_tim(struct sta_info *sta, > bool ignore_pending) > } > > /* No need to do anything if the driver does all */ > - if (!local->ops->set_tim) > + if (local->ops->set_tim) > return; but ... then, you'll need call to drv_set_tim below... Apparently, we can't rely on the ops pointer to enter or not enter this flow.
[PATCH] mac80211: fix broken AP mode handling of powersave clients
Commit c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE") introduced a logic error, where __sta_info_recalc_tim turns into a no-op if local->ops->set_tim is not set. This prevents the beacon TIM bit from being set for all drivers that do not implement this op (almost all of them), thus thoroughly essential AP mode powersave functionality. Cc: Emmanuel Grumbach Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE") Signed-off-by: Felix Fietkau --- net/mac80211/sta_info.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index 236d47e..621734e 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -688,7 +688,7 @@ static void __sta_info_recalc_tim(struct sta_info *sta, bool ignore_pending) } /* No need to do anything if the driver does all */ - if (!local->ops->set_tim) + if (local->ops->set_tim) return; if (sta->dead) -- 2.10.1
Re: [PATCH 2/2] rtl8xxxu: Fix for bogus data used to determine macpower
On Sun, 2016-10-30 at 19:02 -0400, Jes Sorensen wrote: > Code is 80 characters wide, and comments are /* */ never the ugly C++ > crap. You might look at the recent Linus Torvalds authored commit 5e467652ffef (?printk: re-organize log_output() to be more legible") which does both of those: c99 // comments and > 80 columns. Absolutes are for zealots.
RE: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv
Hi Dmitry, > -Original Message- > From: linux-wireless-ow...@vger.kernel.org > [mailto:linux-wireless-ow...@vger.kernel.org] On Behalf Of Dmitry Torokhov > Sent: 2016年10月28日 1:44 > To: Amitkumar Karwar > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam; > raja...@google.com; briannor...@google.com > Subject: Re: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' in > shutdown_drv > > Hi Amit, > > On Thu, Oct 27, 2016 at 02:42:40PM +0530, Amitkumar Karwar wrote: > > This variable is guarded by spinlock at all other places. This patch > > takes care of missing spinlock usage in mwifiex_shutdown_drv(). > > Since in the previous discussion you stated that we inhibit interrupts and > flush > the workqueue so that mwifiex_shutdown_drv() can't run simultaneously with > the main processing routine, why do we need this? > > Instead please remove call to mwifiex_shutdown_drv() in the main routine > and "if (adapter->mwifiex_processing)" check here. > mwifiex_main_process will be used from interrupt or workqueue. Now we have disabled interrupt and flush workqueue, so mwifiex_main_process won't be scheduled in the future. But mwifiex_main_process might just running in context of last interrupt, so we need wait current main_process complete in mwifiex_shutdown_drv. > Thanks. > > > > > Signed-off-by: Amitkumar Karwar > > --- > > v2: Same as v1 > > --- > > drivers/net/wireless/marvell/mwifiex/init.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/init.c > > b/drivers/net/wireless/marvell/mwifiex/init.c > > index 82839d9..8e5e424 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/init.c > > +++ b/drivers/net/wireless/marvell/mwifiex/init.c > > @@ -670,11 +670,14 @@ mwifiex_shutdown_drv(struct mwifiex_adapter > > *adapter) > > > > adapter->hw_status = MWIFIEX_HW_STATUS_CLOSING; > > /* wait for mwifiex_process to complete */ > > + spin_lock_irqsave(&adapter->main_proc_lock, flags); > > if (adapter->mwifiex_processing) { > > + spin_unlock_irqrestore(&adapter->main_proc_lock, flags); > > mwifiex_dbg(adapter, WARN, > > "main process is still running\n"); > > return ret; > > } > > + spin_unlock_irqrestore(&adapter->main_proc_lock, flags); > > > > /* cancel current command */ > > if (adapter->curr_cmd) { > > -- > > 1.9.1 > > > > -- > Dmitry
RE: [PATCH v2 1/5] mwifiex: remove redundant condition in main process
Hi, We have include below change in latest submit https://patchwork.kernel.org/patch/9407283/ Please drop this patch. > -Original Message- > From: linux-wireless-ow...@vger.kernel.org > [mailto:linux-wireless-ow...@vger.kernel.org] On Behalf Of Brian Norris > Sent: 2016年10月28日 2:36 > To: Amitkumar Karwar > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam; > raja...@google.com; briannor...@google.com; dmitry.torok...@gmail.com > Subject: Re: [PATCH v2 1/5] mwifiex: remove redundant condition in main > process > > On Thu, Oct 27, 2016 at 02:42:39PM +0530, Amitkumar Karwar wrote: > > This condition while calling mwifiex_check_ps_cond() is redundant. > > The function internally already takes care of it. > > > > Signed-off-by: Amitkumar Karwar > > --- > > v2: Same as v1 > > You've omitted the Reviewed-by tags from v1: > > Reviewed-by: Brian Norris
Re: [PATCH 1/2] rtl8xxxu: Fix for authentication failure
On 3 November 2016 at 11:00, Larry Finger wrote: > On 10/30/2016 05:20 AM, John Heenan wrote: >> >> This fix enables the same sequence of init behaviour as the alternative >> working driver for the wireless rtl8723bu IC at >> https://github.com/lwfinger/rtl8723bu >> >> For exampe rtl8xxxu_init_device is now called each time >> userspace wpa_supplicant is executed instead of just once when >> modprobe is executed. > > > After all the trouble you have had with your patches, I would expect you to > use more care when composing the commit message. Note the typo in the > paragraph above. > OK, the nasty games continue and the message is not getting through. An appropriate response by a maintainer would have been to request I revise the code according to the way it has currently and elegantly revised in. [PATCH v2] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC where I use a simple pointer comparison of priv->fops with &rtl8723bu_fops. As far as I can see there is nothing more to be done on my part code wise. The supposed issue with matching functions, as far as I can see, is a non issue. If you want to comment on comments in patch messages please do so on the above current proposed patch instead of dragging up stale and irreelvant patch proposals to make a petty point. Jes has now moved the goal posts, indicating all drivers for rtl8xxxu need to be tested fro similar issues. If there are problems with other rtl8xxxu drives then it is not my problem and has nothing to do with me. My issue is only with the rtl8723bu driver. Such doubts also makes it look as if there was never any proper testing and there is no real interest in proper testing. After all that would involve cooperation and team work. Want concrete evidence? I actually did report problems to Jes in private emails, AS REQUESTED, in dmesg before I started tests and posting patches. In the emalI stated I was willing to test drivers with printk messages. I did not even get the courtesy of a reply. Want even more concrete evidence? Jes has some very strange comments in his tree for his current last commit f958b1e0806c045830d78c4287fbcddf9e5fd9d0 " This uncovered a huge bug where the old code would use struct ieee80211_rate.flags to test for rate parameters, which is always zero, instead of the flags value from struct ieee80211_tx_rate. "Time to find a brown paper bag :(" John Heenan >> Along with 'Fix for bogus data used to determine macpower', >> wpa_supplicant now reliably and successfully authenticates. > > > I'm not sure this paragraph belongs in the permanent commit record. > >> For rtl8xxxu-devel branch of >> git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git > > > I know this line does not belong. If you want to include information like > this, include it after a line containing "---". Those lines will be > available to reviewers and maintainers, but will be stripped before it gets > included in the code base. > > >> Signed-off-by: John Heenan >> --- >> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 9 + >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >> index 04141e5..f25b4df 100644 >> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >> @@ -5779,6 +5779,11 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw) >> >> ret = 0; >> >> + ret = rtl8xxxu_init_device(hw); >> + if (ret) >> + goto error_out; >> + >> + >> init_usb_anchor(&priv->rx_anchor); >> init_usb_anchor(&priv->tx_anchor); >> init_usb_anchor(&priv->int_anchor); >> @@ -6080,10 +6085,6 @@ static int rtl8xxxu_probe(struct usb_interface >> *interface, >> goto exit; >> } >> >> - ret = rtl8xxxu_init_device(hw); >> - if (ret) >> - goto exit; >> - >> hw->wiphy->max_scan_ssids = 1; >> hw->wiphy->max_scan_ie_len = IEEE80211_MAX_DATA_LEN; >> hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION); >> > > I will let Jes comment on any side effects of this code change. > > Larry > > -- > If I was stranded on an island and the only way to get off > the island was to make a pretty UI, I’d die there. > > Linus Torvalds