Re: [PATCH V2] Fix a sleep-in-atomic bug in shash_setkey_unaligned

[Herbert Xu]

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

[Andy Lutomirski]

> 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

[Jia-Ju Bai]

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

[Jia-Ju Bai]

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

[Jérémy Lefaure]

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

[Jérémy Lefaure]

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

[Jérémy Lefaure]

On Mon, 2 Oct 2017 16:07:36 +0300
Andy Shevchenko  wrote:

> > +   {_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

[Arend van Spriel]

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

[J. Bruce Fields]

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

[Mauro Carvalho Chehab]

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

[Thomas Gleixner]

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

[Sergey Matyukevich]

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

[Thomas Gleixner]

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

[Kalle Valo]

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

[Kalle Valo]

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

[Kalle Valo]

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

[Kalle Valo]

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

[Kalle Valo]

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

[Andy Shevchenko]

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.
>

> +   {_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

[Kalle Valo]

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

[Kalle Valo]

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

[Johannes Berg]

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

[Johannes Berg]

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.

[kbuild test robot]

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

[Erik Stromdahl]

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

[Erik Stromdahl]

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 v4 4/9] em28xx: fix em28xx_dvb_init for KASAN

[Arnd Bergmann]

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

[Alagu Sankar]

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

[Alagu Sankar]

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

[Arend van Spriel]

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(-)