Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
On Mon, Oct 02, 2017 at 09:18:24PM -0700, Andy Lutomirski wrote: > > On Oct 2, 2017, at 7:25 PM, Jia-Ju Bai wrote: > > > > The SCTP program may sleep under a spinlock, and the function call path is: > > sctp_generate_t3_rtx_event (acquire the spinlock) > > sctp_do_sm > >sctp_side_effects > > sctp_cmd_interpreter > >sctp_make_init_ack > > sctp_pack_cookie > >crypto_shash_setkey > > shash_setkey_unaligned > >kmalloc(GFP_KERNEL) > > I'm going to go out on a limb here: why on Earth is out crypto API so > full of indirection that we allocate memory at all here? The crypto API operates on a one key per-tfm basis. So normally tfm allocation and key setting is done once only and not done on the data path. I have looked at the SCTP code and it appears to fit this paradigm. That is, we should be able to allocate the tfm and set the key when the key is actually generated via get_random_bytes, rather than every time the key is used which is not only a waste but as you see runs into API issues. Usually if you're invoking setkey from a non-sleeping code-path you're probably doing something wrong. As someone else noted recently, there is no single forum for reviewing code that uses the crypto API so buggy code like this is not surprising. > We're synchronously computing a hash of a small amount of data using > either HMAC-SHA1 or HMAC-SHA256 (determined at runtime) if I read it > right. There's a sane way to do this that doesn't need kmalloc, > alloca, or fancy indirection. And then there's crypto_shash_xyz(). There are some legitimate cases where you want to use a different key for every hashing operation. But so far these are uses have been very few so there has been no need to provide an API for them. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
> On Oct 2, 2017, at 7:25 PM, Jia-Ju Bai wrote: > > The SCTP program may sleep under a spinlock, and the function call path is: > sctp_generate_t3_rtx_event (acquire the spinlock) > sctp_do_sm >sctp_side_effects > sctp_cmd_interpreter >sctp_make_init_ack > sctp_pack_cookie >crypto_shash_setkey > shash_setkey_unaligned >kmalloc(GFP_KERNEL) > I'm going to go out on a limb here: why on Earth is out crypto API so full of indirection that we allocate memory at all here? We're synchronously computing a hash of a small amount of data using either HMAC-SHA1 or HMAC-SHA256 (determined at runtime) if I read it right. There's a sane way to do this that doesn't need kmalloc, alloca, or fancy indirection. And then there's crypto_shash_xyz(). --Andy, who is sick of seeing stupid bugs caused by the fact that it's basically impossible to use the crypto API in a sane way. P.S. gnulib has: int hmac_sha256 (const void *key, size_t keylen, const void *in, size_t inlen, void *resbuf); An init/update/final-style API would be nice, but something like this would be a phenomenal improvement over what we have.
[PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned
The SCTP program may sleep under a spinlock, and the function call path is: sctp_generate_t3_rtx_event (acquire the spinlock) sctp_do_sm sctp_side_effects sctp_cmd_interpreter sctp_make_init_ack sctp_pack_cookie crypto_shash_setkey shash_setkey_unaligned kmalloc(GFP_KERNEL) For the same reason, the orinoco driver may sleep in interrupt handler, and the function call path is: orinoco_rx_isr_tasklet orinoco_rx orinoco_mic crypto_shash_setkey shash_setkey_unaligned kmalloc(GFP_KERNEL) To fix it, GFP_KERNEL is replaced with GFP_ATOMIC. This bug is found by my static analysis tool and my code review. Signed-off-by: Jia-Ju Bai --- crypto/shash.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crypto/shash.c b/crypto/shash.c index 5e31c8d..8fcecc6 100644 --- a/crypto/shash.c +++ b/crypto/shash.c @@ -41,7 +41,7 @@ static int shash_setkey_unaligned(struct crypto_shash *tfm, const u8 *key, int err; absize = keylen + (alignmask & ~(crypto_tfm_ctx_alignment() - 1)); - buffer = kmalloc(absize, GFP_KERNEL); + buffer = kmalloc(absize, GFP_ATOMIC); if (!buffer) return -ENOMEM; -- 1.7.9.5
[PATCH] Fix a sleep-in-atomic bug in shash_setkey_unaligned
For the same reason, the orinoco driver may sleep in interrupt handler, and the function call path is: orinoco_rx_isr_tasklet orinoco_rx orinoco_mic crypto_shash_setkey shash_setkey_unaligned kmalloc(GFP_KERNEL) To fix it, GFP_KERNEL is replaced with GFP_ATOMIC. This bug is found by my static analysis tool and my code review. Signed-off-by: Jia-Ju Bai --- crypto/shash.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crypto/shash.c b/crypto/shash.c index 5e31c8d..8fcecc6 100644 --- a/crypto/shash.c +++ b/crypto/shash.c @@ -41,7 +41,7 @@ static int shash_setkey_unaligned(struct crypto_shash *tfm, const u8 *key, int err; absize = keylen + (alignmask & ~(crypto_tfm_ctx_alignment() - 1)); - buffer = kmalloc(absize, GFP_KERNEL); + buffer = kmalloc(absize, GFP_ATOMIC); if (!buffer) return -ENOMEM; -- 1.7.9.5
Re: [PATCH 05/18] net: use ARRAY_SIZE
On Mon, 02 Oct 2017 16:46:29 +0300 Kalle Valo wrote: > We have a tree for wireless so usually it's better to submit wireless > changes on their own but here I assume Dave will apply this to his tree. > If not, please resubmit the wireless part in a separate patch. Ok, I note that. I'll wait Dave's answer and I'll split this patch if needed. Thank you, Jérémy
Re: [PATCH 00/18] use ARRAY_SIZE macro
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). Thank you, Jérémy
Re: [PATCH 05/18] net: use ARRAY_SIZE
On Mon, 2 Oct 2017 16:07:36 +0300 Andy Shevchenko wrote: > > + {&gainctrl_lut_core0_rev0, ARRAY_SIZE(gainctrl_lut_core0_rev0), 26, > > 192, > > +32}, > > For all such cases I would rather put on one line disregard checkpatch > warning for better readability. I agree that it would be better. I didn't know that it was possible to not follow this rule for anything else than a string. I am waiting for more comments and I will send a v2. Thank you, Jérémy
Re: [PATCH] cfg80211/nl80211: add a port authorized event
On 29-09-17 14:21, Johannes Berg wrote: From: Avraham Stern Add an event that indicates that a connection is authorized (i.e. the 4 way handshake was performed by the driver). This event should be sent by the driver after sending a connect/roamed event. So is this event required for drivers supporting 4-way handshake offload. If so, the "should" above might need to be "shall" and I have some changes to do in brcmfmac ;-) Regards, Arend
Re: [PATCH 00/18] use ARRAY_SIZE macro
On Mon, Oct 02, 2017 at 07:35:54AM +0200, Greg KH wrote: > On Sun, Oct 01, 2017 at 08:52:20PM -0400, Jérémy Lefaure wrote: > > On Mon, 2 Oct 2017 09:01:31 +1100 > > "Tobin C. Harding" wrote: > > > > > > In order to reduce the size of the To: and Cc: lines, each patch of the > > > > series is sent only to the maintainers and lists concerned by the patch. > > > > This cover letter is sent to every list concerned by this series. > > > > > > Why don't you just send individual patches for each subsystem? I'm not a > > > maintainer but I don't see > > > how any one person is going to be able to apply this whole series, it is > > > making it hard for > > > maintainers if they have to pick patches out from among the series (if > > > indeed any will bother > > > doing that). > > Yeah, maybe it would have been better to send individual patches. > > > > From my point of view it's a series because the patches are related (I > > did a git format-patch from my local branch). But for the maintainers > > point of view, they are individual patches. > > And the maintainers view is what matters here, if you wish to get your > patches reviewed and accepted... 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 --b.
Re: [PATCH 00/18] use ARRAY_SIZE macro
Em Sun, 1 Oct 2017 20:52:20 -0400 Jérémy Lefaure escreveu: > Anyway, I can tell to each maintainer that they can apply the patches > they're concerned about and next time I may send individual patches. In the case of media, we'll handle it as if they were individual ones. Thanks, Mauro
Re: [PATCH] PCI MSI: allow alignment restrictions on vector allocation
On Mon, 2 Oct 2017, Thomas Gleixner wrote: > On Mon, 2 Oct 2017, Daniel Drake wrote: > 2) The affinity setting of straight MSI interrupts (w/o remapping) on x86 > requires to make the affinity change from the interrupt context of the > current active vector in order not to lose interrupts or worst case > getting into a stale state. > > That works for single vectors, but trying to move all vectors in one > go is more or less impossible, as there is no reliable way to > determine that none of the other vectors is on flight. > > There might be some 'workarounds' for that, but I rather avoid that > unless we get an official documented one from Intel/AMD. Thinking more about it. That might be actually a non issue for MSI, but we have that modus operandi in the current code and we need to address that first before even thinking about multi MSI support. Thanks, tglx
Re: [PATCH] nl80211: look for HT/VHT capabilities in beacon's tail
On Mon, 2017-10-02 at 18:34 +0300, Sergey Matyukevich wrote: > On Wed, Aug 30, 2017 at 01:52:25PM -0700, igor.mitsyanko.os@quantenna > .com wrote: > > > There are no HT/VHT capabilities in > > cfg80211_ap_settings::beacon_ies, > > these should be looked for in beacon's tail instead. > > > > Signed-off-by: Igor Mitsyanko > > --- > > > > This is true for hostapd (at least the one in mainline): it does > > not > > include HT/VHT caps and WLAN_EID_SUPP_RATES into beacon_ies. > > But worth noting that there is no clear documentation that I could > > find > > on what IEs could and could not be included into beacon_ies. > > > > net/wireless/nl80211.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > Hello Johannes, > > Any comments on this change ? It's already in linux.git ... johannes
Re: [PATCH] nl80211: look for HT/VHT capabilities in beacon's tail
On Wed, Aug 30, 2017 at 01:52:25PM -0700, igor.mitsyanko...@quantenna.com wrote: > There are no HT/VHT capabilities in cfg80211_ap_settings::beacon_ies, > these should be looked for in beacon's tail instead. > > Signed-off-by: Igor Mitsyanko > --- > > This is true for hostapd (at least the one in mainline): it does not > include HT/VHT caps and WLAN_EID_SUPP_RATES into beacon_ies. > But worth noting that there is no clear documentation that I could find > on what IEs could and could not be included into beacon_ies. > > net/wireless/nl80211.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Hello Johannes, Any comments on this change ? Regards, Sergey
Re: [PATCH] PCI MSI: allow alignment restrictions on vector allocation
Daniel, On Mon, 2 Oct 2017, Daniel Drake wrote: > On Wed, Sep 27, 2017 at 11:28 PM, Thomas Gleixner wrote: > On another system, I have multiple devices using IR-PCI-MSI according > to /proc/interrupts, and lspci shows that a MSI Message Data value 0 > is used for every single MSI-enabled device. > > I don't know if that's just luck, but its a value that would work > fine for ath9k. It's an implementation detail... > After checking out the new code and thinking this through a bit, I think > perhaps the only generic approach that would work is to make the > ath9k driver require a vector allocation that enables the entire block > of 4 MSI IRQs that the hardware supports (which is what Windows is doing). I wonder how Windows deals with the affinity problem for multi-MSI. Or does it just not allow to set it at all? > This way the alignment requirement is automatically met and ath9k is > free to stamp all over the lower 2 bits of the MSI Message Data. > > MSI_FLAG_MULTI_PCI_MSI is already supported by a couple of non-x86 drivers > and the interrupt remapping code, but it is not supported by the generic > pci_msi_controller, and the new vector allocator still rejects > X86_IRQ_ALLOC_CONTIGUOUS_VECTORS. Yes, and it does so because Multi-MSI on x86 requires single destination for all vectors, that means the affinity is the same for all vectors. This has two implications: 1) The generic interrupt code and its affinity management are not able to deal with that. Probably a solvable problem, but not trivial either. 2) The affinity setting of straight MSI interrupts (w/o remapping) on x86 requires to make the affinity change from the interrupt context of the current active vector in order not to lose interrupts or worst case getting into a stale state. That works for single vectors, but trying to move all vectors in one go is more or less impossible, as there is no reliable way to determine that none of the other vectors is on flight. There might be some 'workarounds' for that, but I rather avoid that unless we get an official documented one from Intel/AMD. With interrupt remapping this is a different story as the remapping unit removes all these problems and things just work. The switch to best effort reservation mode for vectors is another interesting problem. This cannot work with non remapped multi-MSI, unless we just give up for these type of interrupts and associate them straight away. I could be persuaded to do so, but the above problems (especially #2) wont magically go away. > I will try to take a look at enabling this for the generic allocator, > unless you have any other ideas. See above. > In the mean time, at least for reference, below is an updated version > of the previous patch based on the new allocator and incorporating > your feedback. (It's still rather ugly though) > > > I doubt that this can be made generic enough to make it work all over the > > place. Tell Acer/Foxconn to fix their stupid firmware. > > We're also working on this approach ever since the problematic models > first appeared months ago, but since these products are in mass production, I really wonder how they managed to screw that up. The spec is pretty strict about that: "The Multiple Message Enable field (bits 6-4 of the Message Control register) defines the number of low order message data bits the function is permitted to modify to generate its system software allocated vectors. For example, a Multiple Message Enable encoding of “010” indicates the function has been allocated four vectors and is permitted to modify message data bits 1 and 0 (a function modifies the lower message data bits to generate the allocated number of vectors). If the Multiple Message Enable field is “000”, the function is not permitted to modify the message data." Not permitted is the keyword here. > I think ultimately we are going to need a Linux workaround. What's wrong with just using the legacy INTx emulation if you cannot allocate 4 MSI vectors? Thanks, tglx
Re: [V3,1/3] brcmfmac: Avoid possible out-of-bounds read
Kevin Cernekee wrote: > In brcmf_p2p_notify_rx_mgmt_p2p_probereq(), chanspec is assigned before > the length of rxframe is validated. This could lead to uninitialized > data being accessed (but not printed). Since we already have a > perfectly good endian-swapped copy of rxframe->chanspec in ch.chspec, > and ch.chspec is not modified by decchspec(), avoid the extra > assignment and use ch.chspec in the debug print. > > Suggested-by: Mattias Nissler > Signed-off-by: Kevin Cernekee > Reviewed-by: Arend van Spriel 2 patches applied to wireless-drivers-next.git, thanks. 73f2c8e933b1 brcmfmac: Avoid possible out-of-bounds read a7c9acc452b2 brcmfmac: Delete redundant length check -- https://patchwork.kernel.org/patch/9954603/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [v4,2/9] brcmsmac: split up wlc_phy_workarounds_nphy
Arnd Bergmann wrote: > The stack consumption in this driver is still relatively high, with one > remaining warning if the warning level is lowered to 1536 bytes: > > drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:17135:1: error: > the frame size of 1880 bytes is larger than 1536 bytes > [-Werror=frame-larger-than=] > > The affected function is actually a collection of three separate > implementations, > and each of them is fairly large by itself. Splitting them up is done easily > and improves readability at the same time. > > I'm leaving the original indentation to make the review easier. > > Acked-by: Arend van Spriel > Signed-off-by: Arnd Bergmann I'll queue this for v4.15. Depends on: c503dd38f850 brcmsmac: make some local variables 'static const' to reduce stack size -- https://patchwork.kernel.org/patch/9967141/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [v4, 1/9] brcmsmac: make some local variables 'static const' to reduce stack size
Arnd Bergmann wrote: > With KASAN and a couple of other patches applied, this driver is one > of the few remaining ones that actually use more than 2048 bytes of > kernel stack: > > broadcom/brcm80211/brcmsmac/phy/phy_n.c: In function > 'wlc_phy_workarounds_nphy_gainctrl': > broadcom/brcm80211/brcmsmac/phy/phy_n.c:16065:1: warning: the frame size of > 3264 bytes is larger than 2048 bytes [-Wframe-larger-than=] > broadcom/brcm80211/brcmsmac/phy/phy_n.c: In function > 'wlc_phy_workarounds_nphy': > broadcom/brcm80211/brcmsmac/phy/phy_n.c:17138:1: warning: the frame size of > 2864 bytes is larger than 2048 bytes [-Wframe-larger-than=] > > Here, I'm reducing the stack size by marking as many local variables as > 'static const' as I can without changing the actual code. > > This is the first of three patches to improve the stack usage in this > driver. It would be good to have this backported to stabl kernels > to get all drivers in 'allmodconfig' below the 2048 byte limit so > we can turn on the frame warning again globally, but I realize that > the patch is larger than the normal limit for stable backports. > > The other two patches do not need to be backported. > > Cc: > Acked-by: Arend van Spriel > Signed-off-by: Arnd Bergmann Patch applied to wireless-drivers.git, thanks. c503dd38f850 brcmsmac: make some local variables 'static const' to reduce stack size -- https://patchwork.kernel.org/patch/9967145/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [V3,3/3] brcmfmac: Add check for short event packets
Kevin Cernekee wrote: > The length of the data in the received skb is currently passed into > brcmf_fweh_process_event() as packet_len, but this value is not checked. > event_packet should be followed by DATALEN bytes of additional event > data. Ensure that the received packet actually contains at least > DATALEN bytes of additional data, to avoid copying uninitialized memory > into event->data. > > Cc: # v3.8 > Suggested-by: Mattias Nissler > Signed-off-by: Kevin Cernekee Patch applied to wireless-drivers.git, thanks. dd2349121bb1 brcmfmac: Add check for short event packets -- https://patchwork.kernel.org/patch/9954607/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH 05/18] net: use ARRAY_SIZE
Jérémy Lefaure writes: > Using the ARRAY_SIZE macro improves the readability of the code. Also, > it is not always useful to use a variable to store this constant > calculated at compile time. > > Found with Coccinelle with the following semantic patch: > @r depends on (org || report)@ > type T; > T[] E; > position p; > @@ > ( > (sizeof(E)@p /sizeof(*E)) > | > (sizeof(E)@p /sizeof(E[...])) > | > (sizeof(E)@p /sizeof(T)) > ) > > Signed-off-by: Jérémy Lefaure > --- > drivers/net/ethernet/emulex/benet/be_cmds.c| 4 +- > drivers/net/ethernet/intel/i40e/i40e_adminq.h | 3 +- > drivers/net/ethernet/intel/i40evf/i40e_adminq.h| 3 +- > drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c | 3 +- > drivers/net/ethernet/intel/ixgbevf/vf.c| 17 +- > drivers/net/usb/kalmia.c | 9 +- > .../broadcom/brcm80211/brcmsmac/phy/phytbl_n.c | 473 > ++--- > .../net/wireless/realtek/rtlwifi/rtl8723be/hw.c| 9 +- > .../net/wireless/realtek/rtlwifi/rtl8723be/phy.c | 12 +- > .../net/wireless/realtek/rtlwifi/rtl8723be/table.c | 14 +- > .../net/wireless/realtek/rtlwifi/rtl8821ae/table.c | 34 +- > include/net/bond_3ad.h | 3 +- > net/ipv6/seg6_local.c | 6 +- > 13 files changed, 177 insertions(+), 413 deletions(-) We have a tree for wireless so usually it's better to submit wireless changes on their own but here I assume Dave will apply this to his tree. If not, please resubmit the wireless part in a separate patch. -- Kalle Valo
Re: [PATCH 05/18] net: use ARRAY_SIZE
On Sun, Oct 1, 2017 at 10:30 PM, Jérémy Lefaure wrote: > Using the ARRAY_SIZE macro improves the readability of the code. Also, > it is not always useful to use a variable to store this constant > calculated at compile time. > > + {&gainctrl_lut_core0_rev0, ARRAY_SIZE(gainctrl_lut_core0_rev0), 26, > 192, > +32}, For all such cases I would rather put on one line disregard checkpatch warning for better readability. -- With Best Regards, Andy Shevchenko
Re: rtlwifi: rtl8821ae: Fix connection lost problem
Larry Finger wrote: > In commit 40b368af4b75 ("rtlwifi: Fix alignment issues"), the read > of REG_DBI_READ was changed from 16 to 8 bits. For unknown reasonsi > this change results in reduced stability for the wireless connection. > This regression was located using bisection. > > Fixes: 40b368af4b75 ("rtlwifi: Fix alignment issues") > Reported-and-tested-by: James Cameron > Signed-off-by: Larry Finger > Cc: Stable # 4.11+ > Cc: Ping-Ke Shih Patch applied to wireless-drivers.git, thanks. b8b8b16352cd rtlwifi: rtl8821ae: Fix connection lost problem -- https://patchwork.kernel.org/patch/9962615/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [V3,3/3] brcmfmac: Add check for short event packets
Kevin Cernekee wrote: > The length of the data in the received skb is currently passed into > brcmf_fweh_process_event() as packet_len, but this value is not checked. > event_packet should be followed by DATALEN bytes of additional event > data. Ensure that the received packet actually contains at least > DATALEN bytes of additional data, to avoid copying uninitialized memory > into event->data. > > Suggested-by: Mattias Nissler > Signed-off-by: Kevin Cernekee I'll queue this for v4.14 and add: Cc: sta...@vger.kernel.org # v3.8 -- https://patchwork.kernel.org/patch/9954607/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [RFC] Add cfg80211/nl80211 support for AP mode 802.11 FT roaming
On Mon, 2017-09-25 at 15:30 +0300, Dedy Lansky wrote: > From: Dedy Lansky > > Drivers which have integrated AP SME can use this to communicate with > userspace (e.g. hostapd) for the purpose of FT roaming processing. > > cfg80211 API added for driver to indicate received Authentication and > Reassociation frames from the roaming STA. > NL80211_CMD_AUTHENTICATE and NL80211_CMD_ASSOCIATE are enhanced to be > used > in AP mode, for indication/response to/from userspace. That description could use some work :) It's not quite clear to me why this needs new API, and new semantics for the existing AUTH/ASSOC commands. In fact, the latter I dislike most, because those commands/events are only used in client mode right now, and having them in AP mode now seems confusing. Perhaps just mirroring the frames to userspace with CMD_FRAME, if needed with a new flag that they're already handled or so would be better? johannes
Re: [PATCH] mac80211: aead api to reduce redundancy
Please use "v2" tag or so in the subject line, having the same patch again is really not helpful. The next should be v3, obviously. > +++ b/net/mac80211/aead_api.c > @@ -1,7 +1,4 @@ > -/* > - * Copyright 2014-2015, Qualcomm Atheros, Inc. > - * > - * This program is free software; you can redistribute it and/or > modify > +/* This program is free software; you can redistribute it and/or > modify I see no reason to make this change, why remove copyright? > +++ b/net/mac80211/wpa.c > @@ -464,7 +464,8 @@ static int ccmp_encrypt_skb(struct > ieee80211_tx_data *tx, struct sk_buff *skb, > pos += IEEE80211_CCMP_HDR_LEN; > ccmp_special_blocks(skb, pn, b_0, aad); > return ieee80211_aes_ccm_encrypt(key->u.ccmp.tfm, b_0, aad, > pos, len, > - skb_put(skb, mic_len), > mic_len); > + skb_put(skb, > + key->u.ccmp.tfm- > >authsize)); > } I see no reason for the change from mic_len to authsize here? > @@ -540,10 +541,11 @@ ieee80211_crypto_ccmp_decrypt(struct > ieee80211_rx_data *rx, > ccmp_special_blocks(skb, pn, b_0, aad); > > if (ieee80211_aes_ccm_decrypt( > - key->u.ccmp.tfm, b_0, aad, > - skb->data + hdrlen + IEEE80211_CCMP_HDR_LEN, > - data_len, > - skb->data + skb->len - mic_len, mic_len)) > + key->u.ccmp.tfm, b_0, aad, > + skb->data + hdrlen + IEEE80211_CCMP_HDR_LEN, > + data_len, > + skb->data + skb->len - key->u.ccmp.tfm->authsize > + )) > return RX_DROP_UNUSABLE; That's a really really strange way of writing this ... Please reformat. johannes
Re: [PATCH] ath10k: Retry pci probe on failure.
Hi Ben, [auto build test ERROR on ath6kl/ath-next] [also build test ERROR on v4.14-rc3 next-20170929] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/greearb-candelatech-com/ath10k-Retry-pci-probe-on-failure/20170930-020608 base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git ath-next config: arm-allmodconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm All errors (new ones prefixed by >>): >> ERROR: "__bad_udelay" [drivers/net/wireless/ath/ath10k/ath10k_pci.ko] >> undefined! --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH 02/11] ath10k_sdio: wb396 reference card fix
Hi Alagu, On 2017-10-02 09:02, Alagu Sankar wrote: Hi Steve, On 2017-10-02 04:17, Steve deRosier wrote: Hi Alagu, On Sat, Sep 30, 2017 at 10:37 AM, wrote: From: Alagu Sankar The QCA9377-3 WB396 sdio reference card does not get initialized due to the conflict in uart gpio pins. This fix is not required for other QCA9377 sdio cards. Signed-off-by: Alagu Sankar --- drivers/net/wireless/ath/ath10k/core.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index b4f66cd..86247c8 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -1708,8 +1708,15 @@ static int ath10k_init_uart(struct ath10k *ar) return ret; } - if (!uart_print) + if (!uart_print) { + /* Hack: override dbg TX pin to avoid side effects of default + * GPIO_6 in QCA9377 WB396 reference card + */ + if (ar->hif.bus == ATH10K_BUS_SDIO) + ath10k_bmi_write32(ar, hi_dbg_uart_txpin, + ar->hw_params.uart_pin); If it is indeed a "hack", then I don't think the maintainer should accept this upstream. If you want it upstream you need a clean enough implementation that doesn't need to be labeled a "hack". It is a hack as per the qcacld reference driver. Your commit message states that this is only needed for a very specific card and not for other QCA9377 sdio cards. Yet, you're doing this for all ATH10K_BUS_SDIO devices. Not good. I think that it's a quirk and it's limited to a particular implementation of the device. My suggestion: if it can be automatically determined, then do so explicitly. If not, then it needs to be a DT setting or a module parameter or something like that so the platform maker can decide to do it. Having it affect all users of a SDIO QCA9377 when it doesn't apply doesn't seem like a good idea to me. - Steve Got it. The qcacld reference driver had it for all the QCA9377 sdio cards. But we found it to be a problem only for the WB396 reference card. Will have this checked again and release a v2 patch accordingly. While you are at it, you might as well change the commit comments to: "ath10k: sdio: " or perhaps just: "ath10k: " Best Regards, Alagu Sankar ___ ath10k mailing list ath...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 00/11] SDIO support for ath10k
Hi Alagu, It is great to see that we are finally about have fully working mainline support for QCA9377 SDIO chipsets! Great job! On 2017-09-30 19:37, silexcom...@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 with the board data from respective SDIO card vendors. 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 Can you share any scripts etc. (wrapping hostapd and wpa_supplicant stuff) or provide some more info about you test setup? I made a quick socat based test on an old laptop (I don't think it has SDIO 3.0 support) and I did unfortunately not get the same figures as you did :( 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. Ah, so that explains why I couldn't see any messages in the air. 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 removal and the surprise removal of teh card. Alagu Sankar (11): ath10k_sdio: sdio htt data transfer fixes ath10k_sdio: wb396 reference card fix ath10k_sdio: DMA bounce buffers for read write ath10k_sdio: reduce transmit msdu count ath10k_sdio: use clean packet headers ath10k_sdio: high latency fixes for beacon buffer ath10k_sdio: fix rssi indication ath10k_sdio: common read write ath10k_sdio: virtual scatter gather for receive ath10k_sdio: enable firmware crash dump ath10k_sdio: hif start once addition drivers/net/wireless/ath/ath10k/core.c| 35 ++- drivers/net/wireless/ath/ath10k/debug.c | 3 + drivers/net/wireless/ath/ath10k/htc.c | 4 +- drivers/net/wireless/ath/ath10k/htc.h | 1 + drivers/net/wireless/ath/ath10k/htt_rx.c | 19 +- drivers/net/wireless/ath/ath10k/htt_tx.c | 24 +- drivers/net/wireless/ath/ath10k/hw.c | 2 + drivers/net/wireless/ath/ath10k/hw.h | 1 + drivers/net/wireless/ath/ath10k/mac.c | 31 ++- drivers/net/wireless/ath/ath10k/sdio.c| 398 ++ drivers/net/wireless/ath/ath10k/sdio.h| 10 +- drivers/net/wireless/ath/ath10k/wmi-tlv.c | 2 +- 12 files changed, 403 insertions(+), 127 deletions(-)
Re: [PATCH] PCI MSI: allow alignment restrictions on vector allocation
Hi Thomas, Thanks a lot for looking at this. On Wed, Sep 27, 2017 at 11:28 PM, Thomas Gleixner wrote: > This code is gone in -next and replaced by a new vector allocator. > > git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/apic Nice, thanks for cleaning that up! > But the real question is how this is supposed to work with > > - interrupt remapping On another system, I have multiple devices using IR-PCI-MSI according to /proc/interrupts, and lspci shows that a MSI Message Data value 0 is used for every single MSI-enabled device. I don't know if that's just luck, but its a value that would work fine for ath9k. Unfortunately interrupt remapping is not available on the affected Acer products though. https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023717.html > - non X86 machines After checking out the new code and thinking this through a bit, I think perhaps the only generic approach that would work is to make the ath9k driver require a vector allocation that enables the entire block of 4 MSI IRQs that the hardware supports (which is what Windows is doing). This way the alignment requirement is automatically met and ath9k is free to stamp all over the lower 2 bits of the MSI Message Data. MSI_FLAG_MULTI_PCI_MSI is already supported by a couple of non-x86 drivers and the interrupt remapping code, but it is not supported by the generic pci_msi_controller, and the new vector allocator still rejects X86_IRQ_ALLOC_CONTIGUOUS_VECTORS. I will try to take a look at enabling this for the generic allocator, unless you have any other ideas. In the mean time, at least for reference, below is an updated version of the previous patch based on the new allocator and incorporating your feedback. (It's still rather ugly though) > I doubt that this can be made generic enough to make it work all over the > place. Tell Acer/Foxconn to fix their stupid firmware. We're also working on this approach ever since the problematic models first appeared months ago, but since these products are in mass production, I think ultimately we are going to need a Linux workaround. --- arch/x86/include/asm/hw_irq.h | 1 + arch/x86/kernel/apic/msi.c| 2 ++ arch/x86/kernel/apic/vector.c | 14 +++--- include/linux/irq.h | 6 +++--- include/linux/pci.h | 1 + kernel/irq/matrix.c | 22 ++ 6 files changed, 32 insertions(+), 14 deletions(-) diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h index 661540a93072..9e5e1bb62121 100644 --- a/arch/x86/include/asm/hw_irq.h +++ b/arch/x86/include/asm/hw_irq.h @@ -79,6 +79,7 @@ struct irq_alloc_info { struct { struct pci_dev *msi_dev; irq_hw_number_t msi_hwirq; + unsigned int vector_align; }; #endif #ifdef CONFIG_X86_IO_APIC diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c index 5b6dd1a85ec4..9b5277309c29 100644 --- a/arch/x86/kernel/apic/msi.c +++ b/arch/x86/kernel/apic/msi.c @@ -111,6 +111,8 @@ int pci_msi_prepare(struct irq_domain *domain, struct device *dev, int nvec, arg->flags |= X86_IRQ_ALLOC_CONTIGUOUS_VECTORS; } + arg->vector_align = pdev->align_msi_vector; + return 0; } EXPORT_SYMBOL_GPL(pci_msi_prepare); diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c index 6789e286def9..e85f19c09c34 100644 --- a/arch/x86/kernel/apic/vector.c +++ b/arch/x86/kernel/apic/vector.c @@ -31,6 +31,7 @@ struct apic_chip_data { unsigned intcpu; unsigned intprev_cpu; unsigned intirq; + unsigned intvector_align; struct hlist_node clist; unsigned intmove_in_progress: 1, is_managed : 1, @@ -171,7 +172,8 @@ static int reserve_managed_vector(struct irq_data *irqd) raw_spin_lock_irqsave(&vector_lock, flags); apicd->is_managed = true; - ret = irq_matrix_reserve_managed(vector_matrix, affmsk); + ret = irq_matrix_reserve_managed(vector_matrix, affmsk, +apicd->vector_align); raw_spin_unlock_irqrestore(&vector_lock, flags); trace_vector_reserve_managed(irqd->irq, ret); return ret; @@ -215,7 +217,8 @@ static int allocate_vector(struct irq_data *irqd, const struct cpumask *dest) if (vector && cpu_online(cpu) && cpumask_test_cpu(cpu, dest)) return 0; - vector = irq_matrix_alloc(vector_matrix, dest, resvd, &cpu); + vector = irq_matrix_alloc(vector_matrix, dest, resvd, &cpu, + apicd->vector_align); if (vector > 0) apic_update_vector(irqd, vector, cpu); trace_vector_alloc(irqd->irq, vector, resvd, vector); @@ -299,7 +302,8 @@ assign_managed_vector(struct irq_data *
Re: [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN
On Thu, Sep 28, 2017 at 4:30 PM, Arnd Bergmann wrote: > On Thu, Sep 28, 2017 at 6:09 AM, Andrey Ryabinin > wrote: >> On 09/27/2017 04:26 PM, Arnd Bergmann wrote: >>> On Tue, Sep 26, 2017 at 9:49 AM, Andrey Ryabinin >>> wrote: > >>> --- a/include/linux/string.h >>> +++ b/include/linux/string.h >>> @@ -227,7 +227,7 @@ static inline const char *kbasename(const char *path) >>> #define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline)) >>> #define __RENAME(x) __asm__(#x) >>> >>> -void fortify_panic(const char *name) __noreturn __cold; >>> +void fortify_panic(const char *name) __cold; >>> void __read_overflow(void) __compiletime_error("detected read beyond >>> size of object passed as 1st parameter"); >>> void __read_overflow2(void) __compiletime_error("detected read beyond >>> size of object passed as 2nd parameter"); >>> void __read_overflow3(void) __compiletime_error("detected read beyond >>> size of object passed as 3rd parameter"); >>> >>> I don't immediately see why the __noreturn changes the behavior here, any >>> idea? >>> >> >> >> At first I thought that this somehow might be related to >> __asan_handle_no_return(). GCC calls it >> before noreturn function. So I made patch to remove generation of these >> calls (we don't need them in the kernel anyway) >> but it didn't help. It must be something else than. > > I made a reduced test case yesterday (see http://paste.ubuntu.com/25628030/), > and it shows the same behavior with and without the sanitizer, it uses 128 > bytes without the noreturn attribute and 480 bytes when its added, the > sanitizer > adds a factor of 1.5x on top. It's possible that I did something wrong while > reducing, since the original driver file uses very little stack (a few hundred > bytes) without -fsanitize=kernel-address, but finding out what happens in > the reduced case may still help understand the other one. This is now GCC PR82365, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365 I've come up with a workaround, but I'm not sure if that is any better than the alternatives, will send the patch as a follow-up in a bit. Arnd
Re: [PATCH 02/11] ath10k_sdio: wb396 reference card fix
Hi Steve, On 2017-10-02 04:17, Steve deRosier wrote: Hi Alagu, On Sat, Sep 30, 2017 at 10:37 AM, wrote: From: Alagu Sankar The QCA9377-3 WB396 sdio reference card does not get initialized due to the conflict in uart gpio pins. This fix is not required for other QCA9377 sdio cards. Signed-off-by: Alagu Sankar --- drivers/net/wireless/ath/ath10k/core.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index b4f66cd..86247c8 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -1708,8 +1708,15 @@ static int ath10k_init_uart(struct ath10k *ar) return ret; } - if (!uart_print) + if (!uart_print) { + /* Hack: override dbg TX pin to avoid side effects of default +* GPIO_6 in QCA9377 WB396 reference card +*/ + if (ar->hif.bus == ATH10K_BUS_SDIO) + ath10k_bmi_write32(ar, hi_dbg_uart_txpin, + ar->hw_params.uart_pin); If it is indeed a "hack", then I don't think the maintainer should accept this upstream. If you want it upstream you need a clean enough implementation that doesn't need to be labeled a "hack". It is a hack as per the qcacld reference driver. Your commit message states that this is only needed for a very specific card and not for other QCA9377 sdio cards. Yet, you're doing this for all ATH10K_BUS_SDIO devices. Not good. I think that it's a quirk and it's limited to a particular implementation of the device. My suggestion: if it can be automatically determined, then do so explicitly. If not, then it needs to be a DT setting or a module parameter or something like that so the platform maker can decide to do it. Having it affect all users of a SDIO QCA9377 when it doesn't apply doesn't seem like a good idea to me. - Steve Got it. The qcacld reference driver had it for all the QCA9377 sdio cards. But we found it to be a problem only for the WB396 reference card. Will have this checked again and release a v2 patch accordingly. Best Regards, Alagu Sankar
Re: [PATCH 01/11] ath10k_sdio: sdio htt data transfer fixes
Hi Arend, On 2017-10-02 13:06, Arend van Spriel wrote: On 9/30/2017 7:37 PM, silexcom...@gmail.com wrote: From: Alagu Sankar [...] Signed-off-by: Alagu Sankar Not really have a specific remark for this patch, but for the entire series. These patches are sent using an anonymous email address, apart from 'silex' being in there, which does not show up in the certificate of origin. Just wondering if this is acceptable? Regards, Arend --- drivers/net/wireless/ath/ath10k/core.c | 20 +--- drivers/net/wireless/ath/ath10k/htt_rx.c | 6 -- drivers/net/wireless/ath/ath10k/htt_tx.c | 24 +++- 3 files changed, 40 insertions(+), 10 deletions(-) Could not use git send-email from the official ID due to mail server restrictions. If this is not acceptable, I will figure out a way to overcome this. Regards, Alagu Sankar
Re: [PATCH 01/11] ath10k_sdio: sdio htt data transfer fixes
On 9/30/2017 7:37 PM, silexcom...@gmail.com wrote: From: Alagu Sankar [...] Signed-off-by: Alagu Sankar Not really have a specific remark for this patch, but for the entire series. These patches are sent using an anonymous email address, apart from 'silex' being in there, which does not show up in the certificate of origin. Just wondering if this is acceptable? Regards, Arend --- drivers/net/wireless/ath/ath10k/core.c | 20 +--- drivers/net/wireless/ath/ath10k/htt_rx.c | 6 -- drivers/net/wireless/ath/ath10k/htt_tx.c | 24 +++- 3 files changed, 40 insertions(+), 10 deletions(-)