Re: [PATCH] mwifiex: Use put_unaligned_le32
On 10/05/2017 12:07 PM, Himanshu Jha wrote: In this case, the key is CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS. It seems that asm-generic/unaligned.h is set up to include different headers, based on the expected architecture behavior. Yes, asm-generic/unaligned.h looks more appopriate and is most generic implementation of unaligned accesses and arc specific. Probably the idea is that each ARCH knows exactly what is the best way to do unaligned access, so common code (like drivers) should include ARCH-specific asm/unaligned.h only. It will then decide how to do that and will include asm-generic/unaligned.h itself, if required.
Re: converting mac80211 to TXQs entirely
Johannes Berg writes: > On Thu, 2017-10-05 at 18:28 +0200, Toke Høiland-Jørgensen wrote: > >> I'm been thinking about how to move the airtime fairness scheduler >> out of ath9k and into mac80211 (so more drivers can take advantage of >> it). This will require some changes to the TXQ API that drivers speak >> to, so wanted to add my thoughts here to make sure it's compatible >> with your thinking. >> >> I think the easiest way to have mac80211 handle airtime fairness is >> to add a way for ieee80211_tx_dequeue() to return some sort of DEFER >> signal which semantically means "there is no packet for this queue >> right now, but please keep scheduling it" (which would be the result >> of a TXQ belonging to a station that has used its airtime quota but >> still has packets pending). This is different from the current >> meaning of NULL, which will make the driver stop scheduling that TXQ >> until it gets a new wake signal. > > I think that's reasonable. I'm not really sure it's *necessary* > though? Couldn't mac80211 return NULL, and then simply call > wake_tx_queue again when the TXQ becomes eligible for scheduling > again? Otherwise the driver might end up doing a lot of polling for it > to become eligible again? Theoretically, but then there would have to be some kind of callback or another mechanism to figure out when the queue is ready again. The neat thing about DRR scheduling is that we just use the fact that there was an attempt to schedule the queue as an opportunity to increase that queue's deficit by one quantum. This does give a bit of "polling overhead" as you say, but it has been hidden in the measurement noise in all my tests. The obvious alternative is to do a token-based airtime scheduler, where the airtime used by one station is immediately divided out to all active stations. But that requires walking over all active stations on every TX completion, and there's a lot of housekeeping to make sure we don't accidentally starve everyone. So I'd prefer to stick with the DRR scheduler :) >> And I'm wondering if this "is queue stopped" API could be the same >> one used for the airtime DEFER case? > > I think these are two completely different cases. > > The "is queue stopped" I was thinking of would be basically checking > the variable local->queue_stop_reasons, so that the driver could still > use the stop_queue API(s). Yeah, this would be very roundabout, but as > a conversion step I think that'd not be a bad thing. Ah, I see. Yeah, these are different, and the existing packet/NULL return is probably sufficient, then :) >> Oh, and BTW: I see this is on the list of topics for the wireless >> summit in Seoul. How do I go about signing up for that? I'll be at >> netdev talking about some of this stuff anyway[1] :) > > Just show up there, or you can add yourself to the list on the wiki > page :) Cool, I did. See you in Seoul :) -Toke
Re: [PATCH] mwifiex: Use put_unaligned_le32
On Thu, Oct 05, 2017 at 11:02:50AM -0700, Brian Norris wrote: > On Thu, Oct 05, 2017 at 08:52:33PM +0530, Himanshu Jha wrote: > > There are various instances where a function used in file say for eg > > int func_align (void* a) > > is used and it is defined in align.h > > But many files don't *directly* include align.h and rather include > > any other header which includes align.h > > I believe the general rule is that you should included headers for all > symbols you use, and not rely on implicit includes. > > The modification to the general rule is that not all headers are > intended to be included directly, and in such cases there's likely a > parent header that is the more appropriate target. > > In this case, the key is CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS. It > seems that asm-generic/unaligned.h is set up to include different > headers, based on the expected architecture behavior. > Yes, asm-generic/unaligned.h looks more appopriate and is most generic implementation of unaligned accesses and arc specific. Let's see what Kalle Valo recommends! And then I will send v2 of the patch. Thanks for the information! Himanshu Jha > I wonder if include/linux/unaligned/access_ok.h should have a safety > check (e.g., raise an #error if > !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS?). > > > Is compiling the file the only way to check if apppropriate header is > > included or is there some other way to check for it. > > I believe it's mostly manual. Implicit includes have been a problem for > anyone who refactors header files. > > Brian
Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
On Thu, Oct 05, 2017 at 09:16:31PM +0800, Herbert Xu wrote: > On Thu, Oct 05, 2017 at 06:16:20PM +0800, Herbert Xu wrote: > > > > That was my point. Functions like sctp_pack_cookie shouldn't be > > setting the key in the first place. The setkey should happen at > > the point when the key is generated. That's sctp_endpoint_init > > which AFAICS only gets called in GFP_KERNEL context. > > > > Or is there a code-path where sctp_endpoint_init is called in > > softirq context? > > OK, there are indeed code paths where the key is derived in softirq > context. Notably sctp_auth_calculate_hmac. > > So I think this patch is the correct fix and I will push it upstream > as well as back to stable. Okay, thanks. Marcelo
Re: [PATCH] mwifiex: Use put_unaligned_le32
On Thu, Oct 05, 2017 at 08:52:33PM +0530, Himanshu Jha wrote: > There are various instances where a function used in file say for eg > int func_align (void* a) > is used and it is defined in align.h > But many files don't *directly* include align.h and rather include > any other header which includes align.h I believe the general rule is that you should included headers for all symbols you use, and not rely on implicit includes. The modification to the general rule is that not all headers are intended to be included directly, and in such cases there's likely a parent header that is the more appropriate target. In this case, the key is CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS. It seems that asm-generic/unaligned.h is set up to include different headers, based on the expected architecture behavior. I wonder if include/linux/unaligned/access_ok.h should have a safety check (e.g., raise an #error if !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS?). > Is compiling the file the only way to check if apppropriate header is > included or is there some other way to check for it. I believe it's mostly manual. Implicit includes have been a problem for anyone who refactors header files. Brian
Re: [PATCH 00/18] use ARRAY_SIZE macro
On Mon, Oct 02, 2017 at 09:33:12PM -0400, Jérémy Lefaure wrote: > On Mon, 2 Oct 2017 15:22:24 -0400 > bfie...@fieldses.org (J. Bruce Fields) wrote: > > > Mainly I'd just like to know which you're asking for. Do you want me to > > apply this, or to ACK it so someone else can? If it's sent as a series > > I tend to assume the latter. > > > > But in this case I'm assuming it's the former, so I'll pick up the nfsd > > one > Could you to apply the NFSD patch ("nfsd: use ARRAY_SIZE") with the > Reviewed-by: Jeff Layton ) tag please ? > > This patch is an individual patch and it should not have been sent in a > series (sorry about that). Applying for 4.15, thanks.--b.
[PATCH v2] net/mac80211/mesh_plink: Convert timers to use timer_setup()
In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly. This requires adding a pointer back to the sta_info since container_of() can't resolve the sta_info. Cc: Johannes Berg Cc: "David S. Miller" Cc: linux-wireless@vger.kernel.org Cc: net...@vger.kernel.org Cc: Thomas Gleixner Signed-off-by: Kees Cook --- This requires commit 686fef928bba ("timer: Prepare to change timer callback argument type") in v4.14-rc3, but should be otherwise stand-alone. v2: - make mesh_plink_timer non-static and use it in timer_setup() call directly. --- net/mac80211/mesh.h | 1 + net/mac80211/mesh_plink.c | 10 -- net/mac80211/sta_info.c | 4 +++- net/mac80211/sta_info.h | 2 ++ 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/net/mac80211/mesh.h b/net/mac80211/mesh.h index 7e5f271e3c30..465b7853edc0 100644 --- a/net/mac80211/mesh.h +++ b/net/mac80211/mesh.h @@ -275,6 +275,7 @@ void mesh_neighbour_update(struct ieee80211_sub_if_data *sdata, u8 *hw_addr, struct ieee802_11_elems *ie); bool mesh_peer_accepts_plinks(struct ieee802_11_elems *ie); u32 mesh_accept_plinks_update(struct ieee80211_sub_if_data *sdata); +void mesh_plink_timer(struct timer_list *t); void mesh_plink_broken(struct sta_info *sta); u32 mesh_plink_deactivate(struct sta_info *sta); u32 mesh_plink_open(struct sta_info *sta); diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c index f69c6c38ca43..e79adb4164f3 100644 --- a/net/mac80211/mesh_plink.c +++ b/net/mac80211/mesh_plink.c @@ -604,8 +604,9 @@ void mesh_neighbour_update(struct ieee80211_sub_if_data *sdata, ieee80211_mbss_info_change_notify(sdata, changed); } -static void mesh_plink_timer(unsigned long data) +void mesh_plink_timer(struct timer_list *t) { + struct mesh_sta *mesh = from_timer(mesh, t, plink_timer); struct sta_info *sta; u16 reason = 0; struct ieee80211_sub_if_data *sdata; @@ -617,7 +618,7 @@ static void mesh_plink_timer(unsigned long data) * del_timer_sync() this timer after having made sure * it cannot be readded (by deleting the plink.) */ - sta = (struct sta_info *) data; + sta = mesh->plink_sta; if (sta->sdata->local->quiescing) return; @@ -697,11 +698,8 @@ static void mesh_plink_timer(unsigned long data) static inline void mesh_plink_timer_set(struct sta_info *sta, u32 timeout) { - sta->mesh->plink_timer.expires = jiffies + msecs_to_jiffies(timeout); - sta->mesh->plink_timer.data = (unsigned long) sta; - sta->mesh->plink_timer.function = mesh_plink_timer; sta->mesh->plink_timeout = timeout; - add_timer(&sta->mesh->plink_timer); + mod_timer(&sta->mesh->plink_timer, jiffies + msecs_to_jiffies(timeout)); } static bool llid_in_use(struct ieee80211_sub_if_data *sdata, diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index 69615016d5bf..6c254a9d5f11 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -329,10 +329,12 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata, sta->mesh = kzalloc(sizeof(*sta->mesh), gfp); if (!sta->mesh) goto free; + sta->mesh->plink_sta = sta; spin_lock_init(&sta->mesh->plink_lock); if (ieee80211_vif_is_mesh(&sdata->vif) && !sdata->u.mesh.user_mpm) - init_timer(&sta->mesh->plink_timer); + timer_setup(&sta->mesh->plink_timer, mesh_plink_timer, + 0); sta->mesh->nonpeer_pm = NL80211_MESH_POWER_ACTIVE; } #endif diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h index 3acbdfa9f649..21d9760ce5c3 100644 --- a/net/mac80211/sta_info.h +++ b/net/mac80211/sta_info.h @@ -344,6 +344,7 @@ DECLARE_EWMA(mesh_fail_avg, 20, 8) * @plink_state: peer link state * @plink_timeout: timeout of peer link * @plink_timer: peer link watch timer + * @plink_sta: peer link watch timer's sta_info * @t_offset: timing offset relative to this host * @t_offset_setpoint: reference timing offset of this sta to be used when * calculating clockdrift @@ -356,6 +357,7 @@ DECLARE_EWMA(mesh_fail_avg, 20, 8) */ struct mesh_sta { struct timer_list plink_timer; + struct sta_info *plink_sta; s64 t_offset; s64 t_offset_setpoint; -- 2.7.4 -- Kees Cook Pixel Security
Re: [08/11] ath10k_sdio: common read write
Hi Gary, On 2017-10-05 15:39, Gary Bisson wrote: Hi Alagu, On Sat, Sep 30, 2017 at 11:07:45PM +0530, silexcom...@gmail.com wrote: From: Alagu Sankar convert different read write functions in sdio hif to bring it under a single read-write path. This helps in having a common dma bounce buffer implementation. Also helps in address modification that is required specific to change in certain mbox addresses of sdio_write. Signed-off-by: Alagu Sankar --- drivers/net/wireless/ath/ath10k/sdio.c | 131 - 1 file changed, 64 insertions(+), 67 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c index 77d4fa4..bb6fa67 100644 --- a/drivers/net/wireless/ath/ath10k/sdio.c +++ b/drivers/net/wireless/ath/ath10k/sdio.c @@ -36,6 +36,11 @@ #define ATH10K_SDIO_DMA_BUF_SIZE (32 * 1024) +static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf, + u32 len, bool incr); +static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf, +u32 len, bool incr); + As mentioned by Kalle, u32 needs to be size_t. Yes, the compiler I used is probably a step older and did not catch this. /* inlined helper functions */ /* Macro to check if DMA buffer is WORD-aligned and DMA-able. @@ -152,6 +157,7 @@ static int ath10k_sdio_config(struct ath10k *ar) struct sdio_func *func = ar_sdio->func; unsigned char byte, asyncintdelay = 2; int ret; + u32 addr; ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio configuration\n"); @@ -180,9 +186,8 @@ static int ath10k_sdio_config(struct ath10k *ar) CCCR_SDIO_DRIVER_STRENGTH_ENABLE_C | CCCR_SDIO_DRIVER_STRENGTH_ENABLE_D); - ret = ath10k_sdio_func0_cmd52_wr_byte(func->card, - CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR, - byte); + addr = CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR, + ret = ath10k_sdio_func0_cmd52_wr_byte(func->card, addr, byte); Not sure this part is needed. This is to overcome checkpatch warning. Too big a names for the function and macro not helping in there. Will have to move it as a separate patch. if (ret) { ath10k_warn(ar, "failed to enable driver strength: %d\n", ret); goto out; @@ -233,13 +238,16 @@ static int ath10k_sdio_config(struct ath10k *ar) static int ath10k_sdio_write32(struct ath10k *ar, u32 addr, u32 val) { - struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar); - struct sdio_func *func = ar_sdio->func; + __le32 *buf; int ret; - sdio_claim_host(func); + buf = kzalloc(sizeof(*buf), GFP_KERNEL); + if (!buf) + return -ENOMEM; - sdio_writel(func, val, addr, &ret); + *buf = cpu_to_le32(val); + + ret = ath10k_sdio_write(ar, addr, &val, sizeof(val), true); Shouldn't we use buf instead of val? buf seems pretty useless otherwise. Yes, thanks for pointing this out. will be corrected in v2. Regards, Gary ___ ath10k mailing list ath...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH] net/mac80211/mesh_plink: Convert timers to use
On Wed, Oct 4, 2017 at 11:47 PM, Johannes Berg wrote: > On Wed, 2017-10-04 at 17:49 -0700, Kees Cook wrote: >> In preparation for unconditionally passing the struct timer_list >> pointer to all timer callbacks, switch to using the new timer_setup() >> and from_timer() to pass the timer pointer explicitly. This requires >> adding a pointer back to the sta_info since container_of() can't >> resolve the sta_info. > > The subject seems to be lacking something ... :-) Oh wonderful, all the subjects are cut off. *sigh* I wonder which piece of my workflow broke that... >> This requires commit 686fef928bba ("timer: Prepare to change timer >> callback argument type") in v4.14-rc3, but should be otherwise >> stand-alone. > > I still can't apply that because that's not in net-next right now. Okay, I'll see if Dave brings that into net-next and resend after that. >> static inline void mesh_plink_timer_set(struct sta_info *sta, u32 >> timeout) >> { >> sta->mesh->plink_timer.expires = jiffies + >> msecs_to_jiffies(timeout); >> - sta->mesh->plink_timer.data = (unsigned long) sta; >> - sta->mesh->plink_timer.function = mesh_plink_timer; >> + sta->mesh->plink_sta = sta; >> + sta->mesh->plink_timer.function = >> (TIMER_FUNC_TYPE)mesh_plink_timer; >> sta->mesh->plink_timeout = timeout; >> add_timer(&sta->mesh->plink_timer); > > Wouldn't it be better to convert this to timer_setup() now? The problem is that plink_timer is used in several places, and it's originally initialized in net/mac80211/sta_info.c. The call to mesh_plink_timer_set() does an update of the function field, so it didn't look like it could get merged with the timer_setup(), but in looking again, it seems that this is the _only_ update to plink_timer.function, so it could probably get collapsed into the timer_setup() call. > That add_timer() should probably also be mod_timer() anyway? Agreed. I'd avoided making those changes in most places, but I can do it here. >> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c >> index 69615016d5bf..5e5de9455e4e 100644 >> --- a/net/mac80211/sta_info.c >> +++ b/net/mac80211/sta_info.c >> @@ -332,7 +332,7 @@ struct sta_info *sta_info_alloc(struct >> ieee80211_sub_if_data *sdata, >> spin_lock_init(&sta->mesh->plink_lock); >> if (ieee80211_vif_is_mesh(&sdata->vif) && >> !sdata->u.mesh.user_mpm) >> - init_timer(&sta->mesh->plink_timer); >> + timer_setup(&sta->mesh->plink_timer, NULL, >> 0); >> sta->mesh->nonpeer_pm = NL80211_MESH_POWER_ACTIVE; >> } > > You just have to make mesh_plink_timer() non-static, put a prototype > into mesh.h and then you can use the proper timer_setup() here with the > function? > > Also, the sta->mesh->plink_sta assignment should be here I'd say, no > point rewriting it all the time. Sounds good. I'll get it cleaned up. -Kees -- Kees Cook Pixel Security
Re: [PATCH 00/11] SDIO support for ath10k
Hi Gary, On 05-10-2017 20:42, Gary Bisson wrote: Hi Alagu, First of all, thank you for sharing your patches, this will be a very nice improvement to have SDIO QCA9377 working with ath10k. I've tried your series with Nitrogen7 [1] platform which is supported in mainline already. It uses BD-SDMAC [2] which uses the same module as the SX-SDMAC. Below are some questions/remarks I have after the testing. On Sat, Sep 30, 2017 at 11:07:37PM +0530, silexcommon at gmail.com wrote: From: Alagu Sankar This patchset, generated against master-pending branch, enables a fully functional SDIO interface driver for ath10k. Patches have been verified on QCA9377-3 WB396 and Silex's SX-SDCAC reference cards with Station, Access Point and P2P modes. The driver is verified with the firmware WLAN.TF.1.1.1-00061-QCATFSWPZ-1 Quick question on the firmware, is it the one from Kalle's repository?[3] If so, where does this firmware comes from? Is 00061 the firmware version? So far I've only seen up to v0.0.0.60, see qcacld-2.0 output: Host SW:4.5.20.037, FW:0.0.0.60, HW:QCA9377_REV1_1 Yes, it is from https://github.com/kvalo/ath10k-firmware/tree/master/QCA9377/hw1.0/untested. I have also used custom firmware from QCA/Silex as used in qcacld-2.0 driver without any issue. You need to use the firmware merger tool from https://github.com/erstrom/linux-ath/wiki/Firmware to combine the qwlan30.bin and otp30.bin to generate the firmware-sdio.bin. with the board data from respective SDIO card vendors. About the board-sdio.bin, is it just a copy of your bdwlan30.bin? Yes board-sdio.bin is a copy of bdwlan30.bin Receive performance matches the QCA reference driver when used with SDIO3.0 enabled platforms. iperf tests indicate a downlink UDP of 275Mbit/s and TCP of 150Mbit/s Nice performances. Unfortunately I don't get better than 30Mbits/s in TX and 50Mbits/s in RX: # iperf -c 192.168.1.1 Client connecting to 192.168.1.1, TCP port 5001 TCP window size: 43.8 KByte (default) [ 3] local 192.168.1.115 port 41354 connected with 192.168.1.1 port 5001 [ ID] Interval Transfer Bandwidth [ 3] 0.0-10.0 sec 34.9 MBytes 29.2 Mbits/sec # iperf -s Server listening on TCP port 5001 TCP window size: 85.3 KByte (default) [ 4] local 192.168.1.115 port 5001 connected with 192.168.1.1 port 50646 [ ID] Interval Transfer Bandwidth [ 4] 0.0-10.0 sec 63.1 MBytes 52.7 Mbits/sec Do you have any idea why? Note that qcacld-2.0 driver on that same platform (same OS) gives the performances you advertize (150Mbits/s). For some reason, if I use the imx_v6_v7_defconfig as is, performance is very poor. In fact, the firmware download itself will take about 6 seconds. This can also be seen when you up/down the wlan0 interface. For the i.MX6 SoloX board, I used the configuration of 4.1.15 as provided by the BSP from NXP/Freescale. This improved the performance quite a bit. I haven't had a chance to spend time on this to figure out the difference and reason. Another difference is that the default device tree for SoloX did not have the correct settings to support SDIO3.x. Had to modify them, but did not include the device tree patches here as it is not meant for this group. This patchset differs from the previous high latency patches, specific to SDIO. HI_ACS_FLAGS_SDIO_REDUCE_TX_COMPL_SET is enabled for HI_ACS. This instructs the firmware to use HTT_T2H_MSG_TYPE_TX_COMPL_IND for outgoing packets. Without this flag, the management frames are not sent out by the firmware. Possibility of management frames being sent via WMI and data frames through the reduced Tx completion needs to be probed further. Further improvements can be done on the transmit path by implementing packet bundle. Scatter Gather is another area of improvement for both Transmit and Receive, but may not work on all platforms Known issues: Surprise removal of the card, when the device is in connected state, delays sdio function remove due to delayed WMI command failures. Existing ath10k framework can not differentiate between a kernel module removae and the surprise removal of teh card. Here are some questions: - Is it normal to see a warning about board-2.bin, shouldn't it look for board-sdio.bin only? [ 14.696704] ath10k_sdio mmc1:0001:1: Direct firmware load for ath10k/QCA9377/hw1.0/board-2.bin failed with error -2 This was only noticed in the latest update. Like the different firmware versions, the driver also looks for the board-2.bin now. I have only tested with the board-sdio.bin - Did you have pre-cal and/or cal binaries for your testing? [ 14.067159] ath10k_sdio mmc1:0001:1: Direct firmware load for ath10k/pre-cal-sdio-mmc1:0001:1.bin failed with error -2 [ 14.149257] ath10k_sdio mmc1:0001:
Re: converting mac80211 to TXQs entirely
On Thu, 2017-10-05 at 18:28 +0200, Toke Høiland-Jørgensen wrote: > I'm been thinking about how to move the airtime fairness scheduler > out of ath9k and into mac80211 (so more drivers can take advantage of > it). This will require some changes to the TXQ API that drivers speak > to, so wanted to add my thoughts here to make sure it's compatible > with your thinking. > > I think the easiest way to have mac80211 handle airtime fairness is > to add a way for ieee80211_tx_dequeue() to return some sort of DEFER > signal which semantically means "there is no packet for this queue > right now, but please keep scheduling it" (which would be the result > of a TXQ belonging to a station that has used its airtime quota but > still has packets pending). This is different from the current > meaning of NULL, which will make the driver stop scheduling that TXQ > until it gets a new wake signal. I think that's reasonable. I'm not really sure it's *necessary* though? Couldn't mac80211 return NULL, and then simply call wake_tx_queue again when the TXQ becomes eligible for scheduling again? Otherwise the driver might end up doing a lot of polling for it to become eligible again? I've mostly glossed over a mac80211 scheduler, which is obviously needed as part of a complete conversion, and it'd just have to integrate with this new return value. > The alternative would be to change the API so the driver asks > mac80211 which TXQ it should pull from next, instead of doing its own > scheduling as it does now. But I'm not sure that's doable with a sane > API. > > Now, I believe this is related to this point of yours: > > > 5) handle non-data frames for vifs > > > > Similarly, we need a vif->nondata_txq where we can put probe > > responses and the (few) other frames that we send before we have a > > station entry. > > > > According to my earlier analysis (previous email) after these steps > > we have a TXQ for all frames. All of these steps will require > > certain adjustments in the drivers currently using TXQs (ath9k & > > ath10k) since they'll be relying on some amount of buffering and > > queue stop/wake in mac80211 for these frames. We might just have to > > add some API to ask "is queue stopped" to make the transition here > > easy. > > And I'm wondering if this "is queue stopped" API could be the same > one used for the airtime DEFER case? I think these are two completely different cases. The "is queue stopped" I was thinking of would be basically checking the variable local->queue_stop_reasons, so that the driver could still use the stop_queue API(s). Yeah, this would be very roundabout, but as a conversion step I think that'd not be a bad thing. A more complete conversion likely wouldn't need this, but would instead have the driver record its own stop reasons and just stop scheduling those TXQs that belong to a stopped "class" (it's no longer really a queue). (and for mac80211 stop reasons, it would just return NULL and re- schedule the TXQ when it becomes eligible again) > Oh, and BTW: I see this is on the list of topics for the wireless > summit in Seoul. How do I go about signing up for that? I'll be at > netdev talking about some of this stuff anyway[1] :) Just show up there, or you can add yourself to the list on the wiki page :) johannes
Re: converting mac80211 to TXQs entirely
Johannes Berg writes: > Part 2 - where can we go from here > > > Of course - as mentioned in the subject - my goal is to just convert > over to TXQs entirely in mac80211. That would get rid of a LOT of > special case code, like queueing for aggregation setup, powersave > buffering (both unicast and multicast), etc. At first glance this looks like a decent way forward. I'll think about it some more and comment again later, but for now I just wanted to add this: I'm been thinking about how to move the airtime fairness scheduler out of ath9k and into mac80211 (so more drivers can take advantage of it). This will require some changes to the TXQ API that drivers speak to, so wanted to add my thoughts here to make sure it's compatible with your thinking. I think the easiest way to have mac80211 handle airtime fairness is to add a way for ieee80211_tx_dequeue() to return some sort of DEFER signal which semantically means "there is no packet for this queue right now, but please keep scheduling it" (which would be the result of a TXQ belonging to a station that has used its airtime quota but still has packets pending). This is different from the current meaning of NULL, which will make the driver stop scheduling that TXQ until it gets a new wake signal. The alternative would be to change the API so the driver asks mac80211 which TXQ it should pull from next, instead of doing its own scheduling as it does now. But I'm not sure that's doable with a sane API. Now, I believe this is related to this point of yours: > 5) handle non-data frames for vifs > > Similarly, we need a vif->nondata_txq where we can put probe responses > and the (few) other frames that we send before we have a station entry. > > According to my earlier analysis (previous email) after these steps we > have a TXQ for all frames. All of these steps will require certain > adjustments in the drivers currently using TXQs (ath9k & ath10k) since > they'll be relying on some amount of buffering and queue stop/wake in > mac80211 for these frames. We might just have to add some API to ask > "is queue stopped" to make the transition here easy. And I'm wondering if this "is queue stopped" API could be the same one used for the airtime DEFER case? Oh, and BTW: I see this is on the list of topics for the wireless summit in Seoul. How do I go about signing up for that? I'll be at netdev talking about some of this stuff anyway[1] :) -Toke [1] https://www.netdevconf.org/2.2/session.html?jorgensen-wifistack-talk
Re: [PATCH] rsi: fix integer overflow warning
On Thu, 2017-10-05 at 15:12 +, David Laight wrote: > From: Joe Perches > > Sent: 05 October 2017 13:19 > > On Thu, 2017-10-05 at 14:05 +0200, Arnd Bergmann wrote: > > > gcc produces a harmless warning about a recently introduced > > > signed integer overflow: > > > > > > drivers/net/wireless/rsi/rsi_91x_hal.c: In function > > > 'rsi_prepare_mgmt_desc': > > > include/uapi/linux/swab.h:13:15: error: integer overflow in expression > > > [-Werror=overflow] > > > (((__u16)(x) & (__u16)0x00ffU) << 8) | \ > > >^ > > > include/uapi/linux/swab.h:104:2: note: in expansion of macro > > > '___constant_swab16' > > > ___constant_swab16(x) : \ > > > ^~ > > > include/uapi/linux/byteorder/big_endian.h:34:43: note: in expansion of > > > macro '__swab16' > > > #define __cpu_to_le16(x) ((__force __le16)__swab16((x))) > > > > [] > > > > > The problem is that the 'mask' value is a signed integer that gets > > > turned into a negative number when truncated to 16 bits. Making it > > > an unsigned constant avoids this. > > > > I would expect there are more of these. > > > > Perhaps this define in include/uapi/linux/swab.h: > > > > #define __swab16(x) \ > > (__builtin_constant_p((__u16)(x)) ? \ > > ___constant_swab16(x) : \ > > __fswab16(x)) > > > > should be > > > > #define __swab16(x) \ > > (__builtin_constant_p((__u16)(x)) ? \ > > ___constant_swab16((__u16)(x)) :\ > > __fswab16((__u16)(x))) > > You probably don't want the cast in the call to __fswab16() since > that is likely to generate an explicit and with 0x. > You will likely also get one if the argument is _u16 (not unsigned int). It would just an explicit vs implicit cast as __fswab16 is a static inline with a __u16 argument
Re: converting mac80211 to TXQs entirely
Part 2 - where can we go from here Of course - as mentioned in the subject - my goal is to just convert over to TXQs entirely in mac80211. That would get rid of a LOT of special case code, like queueing for aggregation setup, powersave buffering (both unicast and multicast), etc. I think the following would be appropriate steps to take 1) convert multicast PS buffering This is a bit strange today - when multicast PS buffering *isn't* needed, frames go to TXQ drivers via the vif->txq, but when it *is* done, then frames are tagged with IEEE80211_TX_CTL_SEND_AFTER_DTIM and are buffered on ps->bc_buf (if HOST_BROADCAST_PS_BUFFERING is set) and then handed to the driver through the legacy ->tx() path. This should be reasonably simple to change - get rid of ps->bc_buf, and keep the frames on the TXQ, making ieee80211_get_buffered_bc() retrieve them from there instead. We'd have to tell the driver (it needs to know in the wake callback) that it has sleeping clients and needs to buffer, so it can decide whether or not to retrieve immediately (basically, for TXQ drivers, implementing HOST_BROADCAST_PS_BUFFERING in driver logic: retrieve immediately if it wants to buffer itself, or keep them there for a later ieee80211_get_buffered_bc() call if it doesn't). 2) use TXQ for offchannel frames I'm a bit unsure about this - we never really want to queue offchannel packets, and if they don't go out immediately then we can basically drop them. Nevertheless, having everything deal with the TXQ API will be simpler, and so I think this also should. Perhaps for this we should have a TXQ but only ever use txqi->frags, so that we never really have to deal with the FQ stuff for these frames. Then the wake would basically just pull down the packets and send them immediately. 3) handle monitor mode I thought this was more complicated, but I think the easiest way to solve this is to actually just use the local->monitor_sdata->vif.txq. This requires that mac80211 be changed to always allocate local- >monitor_sdata, even if WANT_MONITOR_VIF isn't set, and ath9k/ath10k do something special for this TXQ (and perhaps ath9k should set WANT_MONITOR_VIF), but that seems reasonable. 4) handle non-data frames for stations This is probably the trickiest part. I have a patch to add a non-data TID to the STA TXQs, and that's perhaps the right thing to do - though I guess those frames should again always go onto txqi->frags so they don't compete with data frames for resources. For powersave reasons we'll discuss later, this should probably only apply to bufferable MMPDUs, with others going to the vif->nondata_txq (next item). 5) handle non-data frames for vifs Similarly, we need a vif->nondata_txq where we can put probe responses and the (few) other frames that we send before we have a station entry. According to my earlier analysis (previous email) after these steps we have a TXQ for all frames. All of these steps will require certain adjustments in the drivers currently using TXQs (ath9k & ath10k) since they'll be relying on some amount of buffering and queue stop/wake in mac80211 for these frames. We might just have to add some API to ask "is queue stopped" to make the transition here easy. After this, we can start thinking about doing internal cleanups in mac80211. 6) First thing to do is probably to introduce a compat layer, so that all drivers go through the TXQs, regardless of whether they handle that. I have the beginnings of a patch that does that, it basically requires drivers to set the wake_tx_queue method to a new mac80211 function ieee80211_wake_tx_queue() [so the ops can remain const] when they don't actually want to deal with TXQs themselves. This method can then check the queue stop reasons etc. and only service TXQs that don't have their corresponding queue stopped. We'd also hook into when the queues get re-enabled, and call the servicing function from there for such drivers. My current prototype just calls the existing __ieee80211_tx() but I no longer think that's a good idea - the idea is that this would eventually allow us to get rid of tx_pending. So it'd be better to have ieee80211_wake_tx_queue() just check all the required things, and once a frame is pulled from a TXQ it's committed to be given to the driver. For off-channel, a special case will be needed - dropping the frame when there's no way to transmit it at the time. 7) Remove all the now-dead code A lot of code is now dead - we'll never have multiple netdev queues, all IFF_NOQUEUE etc - remove all the code associated with that 8) convert more infrastructure to TXQs It gets more vague (starting from 6), but eventually I want to * get rid of tx_pending (why do we even use both TXQ and tx_pending for aggregation?) * do all powersave buffering on TXQs [this may be tricky for filtered frames, perhaps disallow filtered for TXQ drivers like ath9k/ath10k, and have a separate per-TXQI list in the private txq data for ieee80211_
Re: [PATCH] mwifiex: Use put_unaligned_le32
On Thu, Oct 05, 2017 at 11:41:38AM +0300, Kalle Valo wrote: > Himanshu Jha writes: > > >> > --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c > >> > +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c > >> > @@ -17,6 +17,7 @@ > >> > * this warranty disclaimer. > >> > */ > >> > > >> > +#include > >> > >> I don't think this is correct. Should it be asm/unaligned.h? > > > > Would mind explainig me as to why it is incorrect! Also, it defined in > > both the header files but, why is asm/unaligned.h preferred ? > > asm/unaligned.h seems to be the toplevel header file which includes > header files based on arch configuration. Also grepping the sources > support that, nobody from drivers/ include access_ok.h directly. > Yes, you are correct! I will send v2 patch with asm/unaligned.h > But I can't say that I fully understand how the header files work so > please do correct me if I have mistaken. > It is confusing to me as well. There are various instances where a function used in file say for eg int func_align (void* a) is used and it is defined in align.h But many files don't *directly* include align.h and rather include any other header which includes align.h Is compiling the file the only way to check if apppropriate header is included or is there some other way to check for it. Thanks
RE: [PATCH] rsi: fix integer overflow warning
From: Joe Perches > Sent: 05 October 2017 13:19 > On Thu, 2017-10-05 at 14:05 +0200, Arnd Bergmann wrote: > > gcc produces a harmless warning about a recently introduced > > signed integer overflow: > > > > drivers/net/wireless/rsi/rsi_91x_hal.c: In function 'rsi_prepare_mgmt_desc': > > include/uapi/linux/swab.h:13:15: error: integer overflow in expression > > [-Werror=overflow] > > (((__u16)(x) & (__u16)0x00ffU) << 8) | \ > >^ > > include/uapi/linux/swab.h:104:2: note: in expansion of macro > > '___constant_swab16' > > ___constant_swab16(x) : \ > > ^~ > > include/uapi/linux/byteorder/big_endian.h:34:43: note: in expansion of > > macro '__swab16' > > #define __cpu_to_le16(x) ((__force __le16)__swab16((x))) > > [] > > > The problem is that the 'mask' value is a signed integer that gets > > turned into a negative number when truncated to 16 bits. Making it > > an unsigned constant avoids this. > > I would expect there are more of these. > > Perhaps this define in include/uapi/linux/swab.h: > > #define __swab16(x) \ > (__builtin_constant_p((__u16)(x)) ? \ > ___constant_swab16(x) : \ > __fswab16(x)) > > should be > > #define __swab16(x) \ > (__builtin_constant_p((__u16)(x)) ? \ > ___constant_swab16((__u16)(x)) :\ > __fswab16((__u16)(x))) You probably don't want the cast in the call to __fswab16() since that is likely to generate an explicit and with 0x. You will likely also get one if the argument is _u16 (not unsigned int). David
Re: [PATCH 00/11] SDIO support for ath10k
Hi Alagu, First of all, thank you for sharing your patches, this will be a very nice improvement to have SDIO QCA9377 working with ath10k. I've tried your series with Nitrogen7 [1] platform which is supported in mainline already. It uses BD-SDMAC [2] which uses the same module as the SX-SDMAC. Below are some questions/remarks I have after the testing. On Sat, Sep 30, 2017 at 11:07:37PM +0530, silexcommon at gmail.com wrote: > From: Alagu Sankar > > This patchset, generated against master-pending branch, enables a fully > functional SDIO interface driver for ath10k. Patches have been verified on > QCA9377-3 WB396 and Silex's SX-SDCAC reference cards with Station, Access > Point > and P2P modes. > > The driver is verified with the firmware WLAN.TF.1.1.1-00061-QCATFSWPZ-1 Quick question on the firmware, is it the one from Kalle's repository?[3] If so, where does this firmware comes from? Is 00061 the firmware version? So far I've only seen up to v0.0.0.60, see qcacld-2.0 output: Host SW:4.5.20.037, FW:0.0.0.60, HW:QCA9377_REV1_1 > with the board data from respective SDIO card vendors. About the board-sdio.bin, is it just a copy of your bdwlan30.bin? > Receive performance > matches the QCA reference driver when used with SDIO3.0 enabled platforms. > iperf tests indicate a downlink UDP of 275Mbit/s and TCP of 150Mbit/s Nice performances. Unfortunately I don't get better than 30Mbits/s in TX and 50Mbits/s in RX: # iperf -c 192.168.1.1 Client connecting to 192.168.1.1, TCP port 5001 TCP window size: 43.8 KByte (default) [ 3] local 192.168.1.115 port 41354 connected with 192.168.1.1 port 5001 [ ID] Interval Transfer Bandwidth [ 3] 0.0-10.0 sec 34.9 MBytes 29.2 Mbits/sec # iperf -s Server listening on TCP port 5001 TCP window size: 85.3 KByte (default) [ 4] local 192.168.1.115 port 5001 connected with 192.168.1.1 port 50646 [ ID] Interval Transfer Bandwidth [ 4] 0.0-10.0 sec 63.1 MBytes 52.7 Mbits/sec Do you have any idea why? Note that qcacld-2.0 driver on that same platform (same OS) gives the performances you advertize (150Mbits/s). > This patchset differs from the previous high latency patches, specific to > SDIO. > HI_ACS_FLAGS_SDIO_REDUCE_TX_COMPL_SET is enabled for HI_ACS. This instructs > the > firmware to use HTT_T2H_MSG_TYPE_TX_COMPL_IND for outgoing packets. Without > this flag, the management frames are not sent out by the firmware. Possibility > of management frames being sent via WMI and data frames through the reduced Tx > completion needs to be probed further. > > Further improvements can be done on the transmit path by implementing packet > bundle. Scatter Gather is another area of improvement for both Transmit and > Receive, but may not work on all platforms > > Known issues: Surprise removal of the card, when the device is in connected > state, delays sdio function remove due to delayed WMI command failures. > Existing ath10k framework can not differentiate between a kernel module > removae and the surprise removal of teh card. Here are some questions: - Is it normal to see a warning about board-2.bin, shouldn't it look for board-sdio.bin only? [ 14.696704] ath10k_sdio mmc1:0001:1: Direct firmware load for ath10k/QCA9377/hw1.0/board-2.bin failed with error -2 - Did you have pre-cal and/or cal binaries for your testing? [ 14.067159] ath10k_sdio mmc1:0001:1: Direct firmware load for ath10k/pre-cal-sdio-mmc1:0001:1.bin failed with error -2 [ 14.149257] ath10k_sdio mmc1:0001:1: Direct firmware load for ath10k/cal-sdio-mmc1:0001:1.bin failed with error -2 Also noticed that the "tx bitrate" output of 'iw link' is wrong in my case: # iw wlan0 link Connected to 00:00:00:00:00:b0 (on wlan0) SSID: TPLINK_AC_5G freq: 5180 RX: 72072365 bytes (67934 packets) TX: 79084128 bytes (73649 packets) signal: -35 dBm tx bitrate: 6.0 MBit/s bss flags: short-slot-time dtim period:2 beacon int: 100 When connecting using qcacld driver it shows 433MBit/s as expected. Is it working properly in your case? As a FYI, I've built Erik's tree[4] for this testing, should I use another tree instead? Let me know your thoughts. Regards, Gary [1] https://boundarydevices.com/product/nitrogen7/ [2] https://boundarydevices.com/product/bd_sdmac_wifi/ [3] https://github.com/kvalo/ath10k-firmware/tree/master/QCA9377/hw1.0/untested [4] https://github.com/erstrom/linux-ath
Re: [PATCH] NFC: fdp: make struct nci_ops static
On Thu, 5 Oct 2017 10:47:12 +0100 Colin King wrote: > From: Colin Ian King > > The structure nci_ops is local to the source and does not need to > be in global scope, so make it static. > > Cleans up sparse warning: > symbol 'nci_ops' was not declared. Should it be static? > > Signed-off-by: Colin Ian King > --- > drivers/nfc/fdp/fdp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/nfc/fdp/fdp.c b/drivers/nfc/fdp/fdp.c > index ec50027b0d8b..d5784a47fc13 100644 > --- a/drivers/nfc/fdp/fdp.c > +++ b/drivers/nfc/fdp/fdp.c > @@ -726,7 +726,7 @@ static struct nci_driver_ops fdp_prop_ops[] = { > }, > }; > > -struct nci_ops nci_ops = { > +static struct nci_ops nci_ops = { > .open = fdp_nci_open, > .close = fdp_nci_close, > .send = fdp_nci_send, Why not static const? Yes this goes deeper. NFC needs to make all nfc ops const.
Re: [RFC v2 1/2] fq: support filtering a given tin
Johannes Berg writes: > From: Johannes Berg > > Add to the FQ API a way to filter a given tin, in order to > remove frames that fulfil certain criteria according to a > filter function. > > This will be used by mac80211 to remove frames belonging to > an AP VLAN interface that's being removed. > > Signed-off-by: Johannes Berg Yup, this version looks reasonable to me :) -Toke
[RFC v2 1/2] fq: support filtering a given tin
From: Johannes Berg Add to the FQ API a way to filter a given tin, in order to remove frames that fulfil certain criteria according to a filter function. This will be used by mac80211 to remove frames belonging to an AP VLAN interface that's being removed. Signed-off-by: Johannes Berg --- include/net/fq.h | 7 + include/net/fq_impl.h | 72 --- 2 files changed, 69 insertions(+), 10 deletions(-) diff --git a/include/net/fq.h b/include/net/fq.h index 6d8521a30c5c..ac944a686840 100644 --- a/include/net/fq.h +++ b/include/net/fq.h @@ -90,6 +90,13 @@ typedef void fq_skb_free_t(struct fq *, struct fq_flow *, struct sk_buff *); +/* Return %true to filter (drop) the frame. */ +typedef bool fq_skb_filter_t(struct fq *, +struct fq_tin *, +struct fq_flow *, +struct sk_buff *, +void *); + typedef struct fq_flow *fq_flow_get_default_t(struct fq *, struct fq_tin *, int idx, diff --git a/include/net/fq_impl.h b/include/net/fq_impl.h index 4e6131cd3f43..8b237e4afee6 100644 --- a/include/net/fq_impl.h +++ b/include/net/fq_impl.h @@ -12,24 +12,22 @@ /* functions that are embedded into includer */ -static struct sk_buff *fq_flow_dequeue(struct fq *fq, - struct fq_flow *flow) +static void fq_adjust_removal(struct fq *fq, + struct fq_flow *flow, + struct sk_buff *skb) { struct fq_tin *tin = flow->tin; - struct fq_flow *i; - struct sk_buff *skb; - - lockdep_assert_held(&fq->lock); - - skb = __skb_dequeue(&flow->queue); - if (!skb) - return NULL; tin->backlog_bytes -= skb->len; tin->backlog_packets--; flow->backlog -= skb->len; fq->backlog--; fq->memory_usage -= skb->truesize; +} + +static void fq_rejigger_backlog(struct fq *fq, struct fq_flow *flow) +{ + struct fq_flow *i; if (flow->backlog == 0) { list_del_init(&flow->backlogchain); @@ -43,6 +41,21 @@ static struct sk_buff *fq_flow_dequeue(struct fq *fq, list_move_tail(&flow->backlogchain, &i->backlogchain); } +} + +static struct sk_buff *fq_flow_dequeue(struct fq *fq, + struct fq_flow *flow) +{ + struct sk_buff *skb; + + lockdep_assert_held(&fq->lock); + + skb = __skb_dequeue(&flow->queue); + if (!skb) + return NULL; + + fq_adjust_removal(fq, flow, skb); + fq_rejigger_backlog(fq, flow); return skb; } @@ -188,6 +201,45 @@ static void fq_tin_enqueue(struct fq *fq, } } +static void fq_flow_filter(struct fq *fq, + struct fq_flow *flow, + fq_skb_filter_t filter_func, + void *filter_data, + fq_skb_free_t free_func) +{ + struct fq_tin *tin = flow->tin; + struct sk_buff *skb, *tmp; + + lockdep_assert_held(&fq->lock); + + skb_queue_walk_safe(&flow->queue, skb, tmp) { + if (!filter_func(fq, tin, flow, skb, filter_data)) + continue; + + __skb_unlink(skb, &flow->queue); + fq_adjust_removal(fq, flow, skb); + free_func(fq, tin, flow, skb); + } + + fq_rejigger_backlog(fq, flow); +} + +static void fq_tin_filter(struct fq *fq, + struct fq_tin *tin, + fq_skb_filter_t filter_func, + void *filter_data, + fq_skb_free_t free_func) +{ + struct fq_flow *flow; + + lockdep_assert_held(&fq->lock); + + list_for_each_entry(flow, &tin->new_flows, flowchain) + fq_flow_filter(fq, flow, filter_func, filter_data, free_func); + list_for_each_entry(flow, &tin->old_flows, flowchain) + fq_flow_filter(fq, flow, filter_func, filter_data, free_func); +} + static void fq_flow_reset(struct fq *fq, struct fq_flow *flow, fq_skb_free_t free_func) -- 2.14.2
[RFC v2 2/2] mac80211: only remove AP VLAN frames from TXQ
From: Johannes Berg When removing an AP VLAN interface, mac80211 currently purges the entire TXQ for the AP interface. Fix this by using the FQ API introduced in the previous patch to filter frames. Signed-off-by: Johannes Berg --- net/mac80211/ieee80211_i.h | 2 ++ net/mac80211/iface.c | 25 +++-- net/mac80211/tx.c | 34 ++ 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 9675814f64db..68f874e73561 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -2009,6 +2009,8 @@ void ieee80211_txq_init(struct ieee80211_sub_if_data *sdata, struct txq_info *txq, int tid); void ieee80211_txq_purge(struct ieee80211_local *local, struct txq_info *txqi); +void ieee80211_txq_remove_vlan(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata); void ieee80211_send_auth(struct ieee80211_sub_if_data *sdata, u16 transaction, u16 auth_alg, u16 status, const u8 *extra, size_t extra_len, const u8 *bssid, diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 2619daa29961..13b16f90e1cf 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -793,9 +793,7 @@ static int ieee80211_open(struct net_device *dev) static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, bool going_down) { - struct ieee80211_sub_if_data *txq_sdata = sdata; struct ieee80211_local *local = sdata->local; - struct fq *fq = &local->fq; unsigned long flags; struct sk_buff *skb, *tmp; u32 hw_reconf_flags = 0; @@ -939,9 +937,6 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, switch (sdata->vif.type) { case NL80211_IFTYPE_AP_VLAN: - txq_sdata = container_of(sdata->bss, -struct ieee80211_sub_if_data, u.ap); - mutex_lock(&local->mtx); list_del(&sdata->u.vlan.list); mutex_unlock(&local->mtx); @@ -998,8 +993,6 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, skb_queue_purge(&sdata->skb_queue); } - sdata->bss = NULL; - spin_lock_irqsave(&local->queue_stop_reason_lock, flags); for (i = 0; i < IEEE80211_MAX_QUEUES; i++) { skb_queue_walk_safe(&local->pending[i], skb, tmp) { @@ -1012,22 +1005,10 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, } spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags); - if (txq_sdata->vif.txq) { - struct txq_info *txqi = to_txq_info(txq_sdata->vif.txq); + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) + ieee80211_txq_remove_vlan(local, sdata); - /* -* FIXME FIXME -* -* We really shouldn't purge the *entire* txqi since that -* contains frames for the other AP_VLANs (and possibly -* the AP itself) as well, but there's no API in FQ now -* to be able to filter. -*/ - - spin_lock_bh(&fq->lock); - ieee80211_txq_purge(local, txqi); - spin_unlock_bh(&fq->lock); - } + sdata->bss = NULL; if (local->open_count == 0) ieee80211_clear_tx_pending(local); diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 94826680cf2b..7b8154474b9e 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -1396,6 +1396,40 @@ static void ieee80211_txq_enqueue(struct ieee80211_local *local, fq_flow_get_default_func); } +static bool fq_vlan_filter_func(struct fq *fq, struct fq_tin *tin, + struct fq_flow *flow, struct sk_buff *skb, + void *data) +{ + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb); + + return info->control.vif == data; +} + +void ieee80211_txq_remove_vlan(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata) +{ + struct fq *fq = &local->fq; + struct txq_info *txqi; + struct fq_tin *tin; + struct ieee80211_sub_if_data *ap; + + if (WARN_ON(sdata->vif.type != NL80211_IFTYPE_AP_VLAN)) + return; + + ap = container_of(sdata->bss, struct ieee80211_sub_if_data, u.ap); + + if (!ap->vif.txq) + return; + + txqi = to_txq_info(ap->vif.txq); + tin = &txqi->tin; + + spin_lock_bh(&fq->lock); + fq_tin_filter(fq, tin, fq_vlan_filter_func, &sdata->vif, + fq_skb_free_func); + spin_unlock_bh(&fq->lock); +} + void ieee80211_txq_init(struct ieee80211_sub_if_data *sdata,
Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
On Thu, Oct 05, 2017 at 06:16:20PM +0800, Herbert Xu wrote: > > That was my point. Functions like sctp_pack_cookie shouldn't be > setting the key in the first place. The setkey should happen at > the point when the key is generated. That's sctp_endpoint_init > which AFAICS only gets called in GFP_KERNEL context. > > Or is there a code-path where sctp_endpoint_init is called in > softirq context? OK, there are indeed code paths where the key is derived in softirq context. Notably sctp_auth_calculate_hmac. So I think this patch is the correct fix and I will push it upstream as well as back to stable. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [RFC 1/2] fq: support filtering a given tin
On Thu, 2017-10-05 at 14:24 +0200, Toke Høiland-Jørgensen wrote: > > + for (;;) { > > + head = &tin->new_flows; > > + if (list_empty(head)) { > > + head = &tin->old_flows; > > + if (list_empty(head)) > > + break; > > + } > > + > > + flow = list_first_entry(head, struct fq_flow, flowchain); > > + fq_flow_filter(fq, flow, filter_func, filter_data, free_func); > > + } > > Isn't this going to loop forever? Good question, I'll admit that I copied this without understanding it from fq_tin_reset(), and didn't think about it much. I think you're right though - I guess this needs to iterate the new_flows and old_flows. johannes
Re: [RFC 1/2] fq: support filtering a given tin
Johannes Berg writes: > +static void fq_tin_filter(struct fq *fq, > + struct fq_tin *tin, > + fq_skb_filter_t filter_func, > + void *filter_data, > + fq_skb_free_t free_func) > +{ > + struct list_head *head; > + struct fq_flow *flow; > + > + lockdep_assert_held(&fq->lock); > + > + for (;;) { > + head = &tin->new_flows; > + if (list_empty(head)) { > + head = &tin->old_flows; > + if (list_empty(head)) > + break; > + } > + > + flow = list_first_entry(head, struct fq_flow, flowchain); > + fq_flow_filter(fq, flow, filter_func, filter_data, free_func); > + } Isn't this going to loop forever? -Toke
Re: [PATCH] rsi: fix integer overflow warning
On Thu, 2017-10-05 at 14:05 +0200, Arnd Bergmann wrote: > gcc produces a harmless warning about a recently introduced > signed integer overflow: > > drivers/net/wireless/rsi/rsi_91x_hal.c: In function 'rsi_prepare_mgmt_desc': > include/uapi/linux/swab.h:13:15: error: integer overflow in expression > [-Werror=overflow] > (((__u16)(x) & (__u16)0x00ffU) << 8) | \ >^ > include/uapi/linux/swab.h:104:2: note: in expansion of macro > '___constant_swab16' > ___constant_swab16(x) : \ > ^~ > include/uapi/linux/byteorder/big_endian.h:34:43: note: in expansion of macro > '__swab16' > #define __cpu_to_le16(x) ((__force __le16)__swab16((x))) [] > The problem is that the 'mask' value is a signed integer that gets > turned into a negative number when truncated to 16 bits. Making it > an unsigned constant avoids this. I would expect there are more of these. Perhaps this define in include/uapi/linux/swab.h: #define __swab16(x) \ (__builtin_constant_p((__u16)(x)) ? \ ___constant_swab16(x) : \ __fswab16(x)) should be #define __swab16(x) \ (__builtin_c onstant_p((__u16)(x)) ? \ ___constant_swab16((__u16)(x)) : \ __fswab16((__u16)(x)))
converting mac80211 to TXQs entirely
Hi, Part 1 is just a dump of my notes analysing the current TX scheme. driver setup non-QUEUE_CONTROL drivers * have >= 4 queues - per-AC queues [0-3] * have < 4 queues - all goes to queue 0 QUEUE_CONTROL drivers * hwsim (doesn't handle CAB correctly) - each vif: 0...3 - each cab: 0 - offchannel: 4 * TI (doesn't set HOST_BROADCAST_PS_BUFFERING) - each vif: 4 queues (separate) - each cab: separate queue - offchannel: separate from all others * iwldvm/iwlmvm (doesn't set HOST_BROADCAST_PS_BUFFERING) - each vif: 4 queues (separate) - each cab: separate queue - offchannel: separate from all others (AUX) * ath9k (uses TXQ, sets HOST_BROADCAST_PS_BUFFERING) - each vif: 4 queues (shared based on chanctx) - each cab: all the same (# queues - 2) - offchannel: separate from all others * ath10k (may use TXQ, doesn't set HOST_BROADCAST_PS_BUFFERING) - each vif: 1 queue for all ACs - each cab: same queue as for ACs - offchannel: separate from all others current TX scheme = HOST_BROADCAST_PS_BUFFERING && IEEE80211_TX_CTL_SEND_AFTER_DTIM --> queue for ieee80211_get_buffered_bc() !AP_LINK_PS && sta sleeping --> queue on sta->ps_tx_buf[ac] for wakeup/poll --> send on poll with IEEE80211_TX_CTRL_PS_RESPONSE --> send on wakeup as normal frame (with or without TXQ) [NB: with TXQs, this is buggy due to waking old TXQ before tx_filtered] finally --> send directly (with or without TXQ) if filtered TX status --> append to tx_filtered[ac] and use that before ps_tx_buf[ac] TXQ scheme (where used) === MONITOR || IEEE80211_TX_CTL_SEND_AFTER_DTIM || IEEE80211_TX_CTRL_PS_RESPONSE || non-data --> send directly (to TX queue number as given above) have STA --> per-STA/TID TXQ otherwise --> per-VIF TXQ (for VLAN use AP instead)
[PATCH] rsi: fix integer overflow warning
gcc produces a harmless warning about a recently introduced signed integer overflow: drivers/net/wireless/rsi/rsi_91x_hal.c: In function 'rsi_prepare_mgmt_desc': include/uapi/linux/swab.h:13:15: error: integer overflow in expression [-Werror=overflow] (((__u16)(x) & (__u16)0x00ffU) << 8) | \ ^ include/uapi/linux/swab.h:104:2: note: in expansion of macro '___constant_swab16' ___constant_swab16(x) : \ ^~ include/uapi/linux/byteorder/big_endian.h:34:43: note: in expansion of macro '__swab16' #define __cpu_to_le16(x) ((__force __le16)__swab16((x))) ^~~~ include/linux/byteorder/generic.h:89:21: note: in expansion of macro '__cpu_to_le16' #define cpu_to_le16 __cpu_to_le16 ^ drivers/net/wireless/rsi/rsi_91x_hal.c:136:3: note: in expansion of macro 'cpu_to_le16' cpu_to_le16((tx_params->vap_id << RSI_DESC_VAP_ID_OFST) & ^~~ The problem is that the 'mask' value is a signed integer that gets turned into a negative number when truncated to 16 bits. Making it an unsigned constant avoids this. Fixes: eac4eed3224b ("rsi: tx and rx path enhancements for p2p mode") Signed-off-by: Arnd Bergmann --- drivers/net/wireless/rsi/rsi_mgmt.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/rsi/rsi_mgmt.h b/drivers/net/wireless/rsi/rsi_mgmt.h index b9d0802c1b0f..e21723013f8d 100644 --- a/drivers/net/wireless/rsi/rsi_mgmt.h +++ b/drivers/net/wireless/rsi/rsi_mgmt.h @@ -189,7 +189,7 @@ IEEE80211_WMM_IE_STA_QOSINFO_AC_BE | \ IEEE80211_WMM_IE_STA_QOSINFO_AC_BK) -#define RSI_DESC_VAP_ID_MASK 0xC000 +#define RSI_DESC_VAP_ID_MASK 0xC000u #define RSI_DESC_VAP_ID_OFST 14 #define RSI_DATA_DESC_MAC_BBP_INFO BIT(0) #define RSI_DATA_DESC_NO_ACK_IND BIT(9) -- 2.9.0
[RFC 2/2] mac80211: only remove AP VLAN frames from TXQ
From: Johannes Berg When removing an AP VLAN interface, mac80211 currently purges the entire TXQ for the AP interface. Fix this by using the FQ API introduced in the previous patch to filter frames. Signed-off-by: Johannes Berg --- net/mac80211/ieee80211_i.h | 2 ++ net/mac80211/iface.c | 25 +++-- net/mac80211/tx.c | 34 ++ 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 9675814f64db..68f874e73561 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -2009,6 +2009,8 @@ void ieee80211_txq_init(struct ieee80211_sub_if_data *sdata, struct txq_info *txq, int tid); void ieee80211_txq_purge(struct ieee80211_local *local, struct txq_info *txqi); +void ieee80211_txq_remove_vlan(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata); void ieee80211_send_auth(struct ieee80211_sub_if_data *sdata, u16 transaction, u16 auth_alg, u16 status, const u8 *extra, size_t extra_len, const u8 *bssid, diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 2619daa29961..13b16f90e1cf 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -793,9 +793,7 @@ static int ieee80211_open(struct net_device *dev) static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, bool going_down) { - struct ieee80211_sub_if_data *txq_sdata = sdata; struct ieee80211_local *local = sdata->local; - struct fq *fq = &local->fq; unsigned long flags; struct sk_buff *skb, *tmp; u32 hw_reconf_flags = 0; @@ -939,9 +937,6 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, switch (sdata->vif.type) { case NL80211_IFTYPE_AP_VLAN: - txq_sdata = container_of(sdata->bss, -struct ieee80211_sub_if_data, u.ap); - mutex_lock(&local->mtx); list_del(&sdata->u.vlan.list); mutex_unlock(&local->mtx); @@ -998,8 +993,6 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, skb_queue_purge(&sdata->skb_queue); } - sdata->bss = NULL; - spin_lock_irqsave(&local->queue_stop_reason_lock, flags); for (i = 0; i < IEEE80211_MAX_QUEUES; i++) { skb_queue_walk_safe(&local->pending[i], skb, tmp) { @@ -1012,22 +1005,10 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, } spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags); - if (txq_sdata->vif.txq) { - struct txq_info *txqi = to_txq_info(txq_sdata->vif.txq); + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) + ieee80211_txq_remove_vlan(local, sdata); - /* -* FIXME FIXME -* -* We really shouldn't purge the *entire* txqi since that -* contains frames for the other AP_VLANs (and possibly -* the AP itself) as well, but there's no API in FQ now -* to be able to filter. -*/ - - spin_lock_bh(&fq->lock); - ieee80211_txq_purge(local, txqi); - spin_unlock_bh(&fq->lock); - } + sdata->bss = NULL; if (local->open_count == 0) ieee80211_clear_tx_pending(local); diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 94826680cf2b..7b8154474b9e 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -1396,6 +1396,40 @@ static void ieee80211_txq_enqueue(struct ieee80211_local *local, fq_flow_get_default_func); } +static bool fq_vlan_filter_func(struct fq *fq, struct fq_tin *tin, + struct fq_flow *flow, struct sk_buff *skb, + void *data) +{ + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb); + + return info->control.vif == data; +} + +void ieee80211_txq_remove_vlan(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata) +{ + struct fq *fq = &local->fq; + struct txq_info *txqi; + struct fq_tin *tin; + struct ieee80211_sub_if_data *ap; + + if (WARN_ON(sdata->vif.type != NL80211_IFTYPE_AP_VLAN)) + return; + + ap = container_of(sdata->bss, struct ieee80211_sub_if_data, u.ap); + + if (!ap->vif.txq) + return; + + txqi = to_txq_info(ap->vif.txq); + tin = &txqi->tin; + + spin_lock_bh(&fq->lock); + fq_tin_filter(fq, tin, fq_vlan_filter_func, &sdata->vif, + fq_skb_free_func); + spin_unlock_bh(&fq->lock); +} + void ieee80211_txq_init(struct ieee80211_sub_if_data *sdata,
[RFC 1/2] fq: support filtering a given tin
From: Johannes Berg Add to the FQ API a way to filter a given tin, in order to remove frames that fulfil certain criteria according to a filter function. This will be used by mac80211 to remove frames belonging to an AP VLAN interface that's being removed. Signed-off-by: Johannes Berg --- include/net/fq.h | 7 include/net/fq_impl.h | 92 ++- 2 files changed, 83 insertions(+), 16 deletions(-) diff --git a/include/net/fq.h b/include/net/fq.h index 6d8521a30c5c..ac944a686840 100644 --- a/include/net/fq.h +++ b/include/net/fq.h @@ -90,6 +90,13 @@ typedef void fq_skb_free_t(struct fq *, struct fq_flow *, struct sk_buff *); +/* Return %true to filter (drop) the frame. */ +typedef bool fq_skb_filter_t(struct fq *, +struct fq_tin *, +struct fq_flow *, +struct sk_buff *, +void *); + typedef struct fq_flow *fq_flow_get_default_t(struct fq *, struct fq_tin *, int idx, diff --git a/include/net/fq_impl.h b/include/net/fq_impl.h index 4e6131cd3f43..b27f13d22a90 100644 --- a/include/net/fq_impl.h +++ b/include/net/fq_impl.h @@ -12,24 +12,9 @@ /* functions that are embedded into includer */ -static struct sk_buff *fq_flow_dequeue(struct fq *fq, - struct fq_flow *flow) +static void fq_rejigger_backlog(struct fq *fq, struct fq_flow *flow) { - struct fq_tin *tin = flow->tin; struct fq_flow *i; - struct sk_buff *skb; - - lockdep_assert_held(&fq->lock); - - skb = __skb_dequeue(&flow->queue); - if (!skb) - return NULL; - - tin->backlog_bytes -= skb->len; - tin->backlog_packets--; - flow->backlog -= skb->len; - fq->backlog--; - fq->memory_usage -= skb->truesize; if (flow->backlog == 0) { list_del_init(&flow->backlogchain); @@ -43,6 +28,34 @@ static struct sk_buff *fq_flow_dequeue(struct fq *fq, list_move_tail(&flow->backlogchain, &i->backlogchain); } +} + +static void fq_adjust_removal(struct fq *fq, + struct fq_flow *flow, + struct sk_buff *skb) +{ + struct fq_tin *tin = flow->tin; + + tin->backlog_bytes -= skb->len; + tin->backlog_packets--; + flow->backlog -= skb->len; + fq->backlog--; + fq->memory_usage -= skb->truesize; +} + +static struct sk_buff *fq_flow_dequeue(struct fq *fq, + struct fq_flow *flow) +{ + struct sk_buff *skb; + + lockdep_assert_held(&fq->lock); + + skb = __skb_dequeue(&flow->queue); + if (!skb) + return NULL; + + fq_adjust_removal(fq, flow, skb); + fq_rejigger_backlog(fq, flow); return skb; } @@ -188,6 +201,53 @@ static void fq_tin_enqueue(struct fq *fq, } } +static void fq_flow_filter(struct fq *fq, + struct fq_flow *flow, + fq_skb_filter_t filter_func, + void *filter_data, + fq_skb_free_t free_func) +{ + struct fq_tin *tin = flow->tin; + struct sk_buff *skb, *tmp; + + lockdep_assert_held(&fq->lock); + + skb_queue_walk_safe(&flow->queue, skb, tmp) { + if (!filter_func(fq, tin, flow, skb, filter_data)) + continue; + + __skb_unlink(skb, &flow->queue); + fq_adjust_removal(fq, flow, skb); + free_func(fq, tin, flow, skb); + } + + fq_rejigger_backlog(fq, flow); +} + +static void fq_tin_filter(struct fq *fq, + struct fq_tin *tin, + fq_skb_filter_t filter_func, + void *filter_data, + fq_skb_free_t free_func) +{ + struct list_head *head; + struct fq_flow *flow; + + lockdep_assert_held(&fq->lock); + + for (;;) { + head = &tin->new_flows; + if (list_empty(head)) { + head = &tin->old_flows; + if (list_empty(head)) + break; + } + + flow = list_first_entry(head, struct fq_flow, flowchain); + fq_flow_filter(fq, flow, filter_func, filter_data, free_func); + } +} + static void fq_flow_reset(struct fq *fq, struct fq_flow *flow, fq_skb_free_t free_func) -- 2.14.2
Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
On Wed, Oct 04, 2017 at 09:37:58PM -0700, David Miller wrote: > > > I'm not talking about the code-path in question. I'm talking > > about the function which generates the secret key in the first > > place. AFAICS that's only called in GFP_KERNEL context. What > > am I missing? > > The setkey happens in functions like sctp_pack_cookie() and > sctp_unpack_cookie(), which seems to run from software interrupts. That was my point. Functions like sctp_pack_cookie shouldn't be setting the key in the first place. The setkey should happen at the point when the key is generated. That's sctp_endpoint_init which AFAICS only gets called in GFP_KERNEL context. Or is there a code-path where sctp_endpoint_init is called in softirq context? Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] PCI MSI: allow alignment restrictions on vector allocation
On Thu, 5 Oct 2017, Daniel Drake wrote: > On Mon, Oct 2, 2017 at 10:38 PM, Thomas Gleixner wrote: > > What's wrong with just using the legacy INTx emulation if you cannot > > allocate 4 MSI vectors? > > The Legacy interrupt simply doesn't work for the wifi on at least 8 new Acer > laptop products based on Intel Apollo Lake. > Plus 4 Dell systems included in the patches in this thread: > https://lkml.org/lkml/2017/9/26/55 > (the 2 which I can find specs for are also Apollo Lake) > > We have tried taking the mini-PCIe wifi module out of one of the affected > Acer products and moved it to another computer, where it is working fine > with legacy interrupts. So this suggests that the wifi module itself is OK, > but we are facing a hardware limitation or BIOS limitation on the affected > products. In the Dell thread it says "Some platform(BIOS) blocks legacy > interrupts (INTx)". > > If you have any suggestions for how we might solve this without getting into > the MSI mess then that would be much appreciated. If the BIOS blocks the > interrupts, can Linux unblock them? I'm pretty sure we can. Cc'ed Rafael and Andy. They might know, if not they certainly know whom to ask @Intel. > Just for reference I'm attaching my latest attempt at enabling MULTI_PCI_MSI. > It would definitely need further work if we proceed here - so far I've > ignored the affinity considerations that you explained, and it's not > particularly clean. Yeah, I know how that looks. When I rewrote the allocator I initialy had that multi-vector mode implemented and then ditched it due to the affinity setting mess and because its hard vs. the global best effort reservation mode, which is way more important to have than multi MSI. > int irq_matrix_alloc(struct irq_matrix *m, const struct cpumask *msk, > - bool reserved, unsigned int *mapped_cpu); > + bool reserved, unsigned int *mapped_cpu, unsigned int num, > + unsigned int align_mask); That's not needed. We can assume that multivector allocations must be aligned in the following way: count = __roundup_pow_of_two(count); mask = ilog2(count); That's a sane assumption in general. Thanks, tglx
Re: [08/11] ath10k_sdio: common read write
Hi Alagu, On Sat, Sep 30, 2017 at 11:07:45PM +0530, silexcom...@gmail.com wrote: > From: Alagu Sankar > > convert different read write functions in sdio hif to bring it under a > single read-write path. This helps in having a common dma bounce buffer > implementation. Also helps in address modification that is required > specific to change in certain mbox addresses of sdio_write. > > Signed-off-by: Alagu Sankar > --- > drivers/net/wireless/ath/ath10k/sdio.c | 131 > - > 1 file changed, 64 insertions(+), 67 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/sdio.c > b/drivers/net/wireless/ath/ath10k/sdio.c > index 77d4fa4..bb6fa67 100644 > --- a/drivers/net/wireless/ath/ath10k/sdio.c > +++ b/drivers/net/wireless/ath/ath10k/sdio.c > @@ -36,6 +36,11 @@ > > #define ATH10K_SDIO_DMA_BUF_SIZE (32 * 1024) > > +static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf, > + u32 len, bool incr); > +static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf, > + u32 len, bool incr); > + As mentioned by Kalle, u32 needs to be size_t. > /* inlined helper functions */ > > /* Macro to check if DMA buffer is WORD-aligned and DMA-able. > @@ -152,6 +157,7 @@ static int ath10k_sdio_config(struct ath10k *ar) > struct sdio_func *func = ar_sdio->func; > unsigned char byte, asyncintdelay = 2; > int ret; > + u32 addr; > > ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio configuration\n"); > > @@ -180,9 +186,8 @@ static int ath10k_sdio_config(struct ath10k *ar) >CCCR_SDIO_DRIVER_STRENGTH_ENABLE_C | >CCCR_SDIO_DRIVER_STRENGTH_ENABLE_D); > > - ret = ath10k_sdio_func0_cmd52_wr_byte(func->card, > - > CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR, > - byte); > + addr = CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR, > + ret = ath10k_sdio_func0_cmd52_wr_byte(func->card, addr, byte); Not sure this part is needed. > if (ret) { > ath10k_warn(ar, "failed to enable driver strength: %d\n", ret); > goto out; > @@ -233,13 +238,16 @@ static int ath10k_sdio_config(struct ath10k *ar) > > static int ath10k_sdio_write32(struct ath10k *ar, u32 addr, u32 val) > { > - struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar); > - struct sdio_func *func = ar_sdio->func; > + __le32 *buf; > int ret; > > - sdio_claim_host(func); > + buf = kzalloc(sizeof(*buf), GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > > - sdio_writel(func, val, addr, &ret); > + *buf = cpu_to_le32(val); > + > + ret = ath10k_sdio_write(ar, addr, &val, sizeof(val), true); Shouldn't we use buf instead of val? buf seems pretty useless otherwise. Regards, Gary
[PATCH] NFC: fdp: make struct nci_ops static
From: Colin Ian King The structure nci_ops is local to the source and does not need to be in global scope, so make it static. Cleans up sparse warning: symbol 'nci_ops' was not declared. Should it be static? Signed-off-by: Colin Ian King --- drivers/nfc/fdp/fdp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/nfc/fdp/fdp.c b/drivers/nfc/fdp/fdp.c index ec50027b0d8b..d5784a47fc13 100644 --- a/drivers/nfc/fdp/fdp.c +++ b/drivers/nfc/fdp/fdp.c @@ -726,7 +726,7 @@ static struct nci_driver_ops fdp_prop_ops[] = { }, }; -struct nci_ops nci_ops = { +static struct nci_ops nci_ops = { .open = fdp_nci_open, .close = fdp_nci_close, .send = fdp_nci_send, -- 2.14.1
Re: [PATCH] mwifiex: Use put_unaligned_le32
Himanshu Jha writes: >> > --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c >> > +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c >> > @@ -17,6 +17,7 @@ >> > * this warranty disclaimer. >> > */ >> > >> > +#include >> >> I don't think this is correct. Should it be asm/unaligned.h? > > Would mind explainig me as to why it is incorrect! Also, it defined in > both the header files but, why is asm/unaligned.h preferred ? asm/unaligned.h seems to be the toplevel header file which includes header files based on arch configuration. Also grepping the sources support that, nobody from drivers/ include access_ok.h directly. But I can't say that I fully understand how the header files work so please do correct me if I have mistaken. -- Kalle Valo
Re: [PATCH] mwifiex: Use put_unaligned_le32
On Thu, Oct 05, 2017 at 10:23:37AM +0300, Kalle Valo wrote: > Himanshu Jha writes: > > > Use put_unaligned_le32 rather than using byte ordering function and > > memcpy which makes code clear. > > Also, add the header file where it is declared. > > > > Done using Coccinelle and semantic patch used is : > > > > @ rule1 @ > > identifier tmp; expression ptr,x; type T; > > @@ > > > > - tmp = cpu_to_le32(x); > > > > <+... when != tmp > > - memcpy(ptr, (T)&tmp, ...); > > + put_unaligned_le32(x,ptr); > > ...+> > > > > @ depends on rule1 @ > > type j; identifier tmp; > > @@ > > > > - j tmp; > > ...when != tmp > > > > Signed-off-by: Himanshu Jha > > --- > > drivers/net/wireless/marvell/mwifiex/cmdevt.c | 10 -- > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c > > b/drivers/net/wireless/marvell/mwifiex/cmdevt.c > > index 0edc5d6..e28e119 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c > > +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c > > @@ -17,6 +17,7 @@ > > * this warranty disclaimer. > > */ > > > > +#include > > I don't think this is correct. Should it be asm/unaligned.h? Would mind explainig me as to why it is incorrect! Also, it defined in both the header files but, why is asm/unaligned.h preferred ? Thanks > -- > Kalle Valo
[PATCH v6] brcmfmac: add CLM download support
From: Chung-Hsien Hsu The firmware for brcmfmac devices includes information regarding regulatory constraints. For certain devices this information is kept separately in a binary form that needs to be downloaded to the device. This patch adds support to download this so-called CLM blob file. It uses the same naming scheme as the other firmware files with extension of .clm_blob. The CLM blob file is optional. If the file does not exist, the download process will be bypassed. It will not affect the driver loading. Signed-off-by: Chung-Hsien Hsu --- v2: Revise commit message to describe in more detail v3: Add error handling in brcmf_c_get_clm_name function v4: Correct the length of dload_buf in brcmf_c_download function v5: Remove unnecessary cast and alignment v6: Add debug log for the case of no CLM file present --- .../net/wireless/broadcom/brcm80211/brcmfmac/bus.h | 10 ++ .../wireless/broadcom/brcm80211/brcmfmac/common.c | 162 + .../wireless/broadcom/brcm80211/brcmfmac/core.c| 2 + .../wireless/broadcom/brcm80211/brcmfmac/core.h| 2 + .../broadcom/brcm80211/brcmfmac/fwil_types.h | 31 .../wireless/broadcom/brcm80211/brcmfmac/pcie.c| 19 +++ .../wireless/broadcom/brcm80211/brcmfmac/sdio.c| 19 +++ .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 18 +++ 8 files changed, 263 insertions(+) diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h index b55c329..df42e09 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h @@ -71,6 +71,7 @@ struct brcmf_bus_dcmd { * @wowl_config: specify if dongle is configured for wowl when going to suspend * @get_ramsize: obtain size of device memory. * @get_memdump: obtain device memory dump in provided buffer. + * @get_fwname: obtain firmware name. * * This structure provides an abstract interface towards the * bus specific driver. For control messages to common driver @@ -87,6 +88,8 @@ struct brcmf_bus_ops { void (*wowl_config)(struct device *dev, bool enabled); size_t (*get_ramsize)(struct device *dev); int (*get_memdump)(struct device *dev, void *data, size_t len); + int (*get_fwname)(struct device *dev, uint chip, uint chiprev, + unsigned char *fw_name); }; @@ -214,6 +217,13 @@ int brcmf_bus_get_memdump(struct brcmf_bus *bus, void *data, size_t len) return bus->ops->get_memdump(bus->dev, data, len); } +static inline +int brcmf_bus_get_fwname(struct brcmf_bus *bus, uint chip, uint chiprev, +unsigned char *fw_name) +{ + return bus->ops->get_fwname(bus->dev, chip, chiprev, fw_name); +} + /* * interface functions from common layer */ diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c index 7a2b495..5397727 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include "core.h" @@ -28,6 +29,7 @@ #include "tracepoint.h" #include "common.h" #include "of.h" +#include "firmware.h" MODULE_AUTHOR("Broadcom Corporation"); MODULE_DESCRIPTION("Broadcom 802.11 wireless LAN fullmac driver."); @@ -104,12 +106,140 @@ void brcmf_c_set_joinpref_default(struct brcmf_if *ifp) brcmf_err("Set join_pref error (%d)\n", err); } +static int brcmf_c_download(struct brcmf_if *ifp, u16 flag, + struct brcmf_dload_data_le *dload_buf, + u32 len) +{ + s32 err; + + flag |= (DLOAD_HANDLER_VER << DLOAD_FLAG_VER_SHIFT); + dload_buf->flag = cpu_to_le16(flag); + dload_buf->dload_type = cpu_to_le16(DL_TYPE_CLM); + dload_buf->len = cpu_to_le32(len); + dload_buf->crc = cpu_to_le32(0); + len = sizeof(*dload_buf) + len - 1; + + err = brcmf_fil_iovar_data_set(ifp, "clmload", dload_buf, len); + + return err; +} + +static int brcmf_c_get_clm_name(struct brcmf_if *ifp, u8 *clm_name) +{ + struct brcmf_bus *bus = ifp->drvr->bus_if; + struct brcmf_rev_info *ri = &ifp->drvr->revinfo; + u8 fw_name[BRCMF_FW_NAME_LEN]; + u8 *ptr; + size_t len; + s32 err; + + memset(fw_name, 0, BRCMF_FW_NAME_LEN); + err = brcmf_bus_get_fwname(bus, ri->chipnum, ri->chiprev, fw_name); + if (err) { + brcmf_err("get firmware name failed (%d)\n", err); + goto done; + } + + /* generate CLM blob file name */ + ptr = strrchr(fw_name, '.'); + if (!ptr) { + err = -ENOENT; + goto done; + } + + len = ptr - fw_name + 1; + if (len + strlen(".clm_blob") > BRCMF_FW_NAME_LEN) { + err = -E2BIG; +
Re: [PATCH] mwifiex: Use put_unaligned_le32
Himanshu Jha writes: > Use put_unaligned_le32 rather than using byte ordering function and > memcpy which makes code clear. > Also, add the header file where it is declared. > > Done using Coccinelle and semantic patch used is : > > @ rule1 @ > identifier tmp; expression ptr,x; type T; > @@ > > - tmp = cpu_to_le32(x); > > <+... when != tmp > - memcpy(ptr, (T)&tmp, ...); > + put_unaligned_le32(x,ptr); > ...+> > > @ depends on rule1 @ > type j; identifier tmp; > @@ > > - j tmp; > ...when != tmp > > Signed-off-by: Himanshu Jha > --- > drivers/net/wireless/marvell/mwifiex/cmdevt.c | 10 -- > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c > b/drivers/net/wireless/marvell/mwifiex/cmdevt.c > index 0edc5d6..e28e119 100644 > --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c > +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c > @@ -17,6 +17,7 @@ > * this warranty disclaimer. > */ > > +#include I don't think this is correct. Should it be asm/unaligned.h? -- Kalle Valo