* Re: ROAM/CONNECT event with PORT_AUTHORIZED
From: Ben Greear @ 2017-09-14 19:38 UTC (permalink / raw)
To: Denis Kenzior, Johannes Berg, Arend van Spriel, Arend van Spriel,
Jouni Malinen
Cc: Avraham Stern, linux-wireless
In-Reply-To: <9219316a-5556-6acf-30de-e9aa65a05706@gmail.com>
On 09/14/2017 12:34 PM, Denis Kenzior wrote:
> Hi Johannes,
>
> On 09/14/2017 02:17 PM, Johannes Berg wrote:
>> On Thu, 2017-09-14 at 13:37 -0500, Denis Kenzior wrote:
>>
>>> The question is whether all APs are actually sane after a
>>> roam. E.g. can the STA assume that the same IP address, DHCP lease,
>>> etc is still valid? I heard from various people that this might not
>>> be the case, but we haven't had a chance to verify those claims...
>>
>> I think you pretty much have to assume that, otherwise there's no point
>> in roaming at all - you want your connections to stay, possibly voice
>> calls to continue, etc.
>
> I'm all for using this assumption. I just wonder if real world disagrees? :)
There are different meanings for 'roam'. Are you just talking about
fast-transition roaming?
I would think that the decision to restart DHCP (at least ipv4) should
be in user-space. I'm less sure about how IPv6 should deal with this.
I have tested roaming using FT and normal-ish wpa_supplicant without
doing DHCP, and it works fine. Of course, it also works if you
choose to re-do DHCP.
Thanks,
Ben
>
>> However, I'm not really convinced (any more) that this is actually
>> correct. If I'm reading the supplicant code correctly, then it sets
>> IF_OPER_UP only once the connection is *completed*, so it's already
>> doing what I thought it should be doing and couldn't.
>>
>
> Yes. For a new connection it does something like:
>
> New Key
> Set Key
> New Key (group)
> Set Station (Authorized) (which fails on some drivers)
> Set OperState to UP
>
> Regards,
> -Denis
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: ROAM/CONNECT event with PORT_AUTHORIZED
From: Denis Kenzior @ 2017-09-14 19:37 UTC (permalink / raw)
To: Johannes Berg, Arend van Spriel, Jouni Malinen
Cc: Avraham Stern, linux-wireless
In-Reply-To: <1505416964.31630.17.camel@sipsolutions.net>
Hi Johannes,
>> My earlier point is that software roams need to have the exact same
>> behavior as well. And my understanding is that when we try to
>> Fast-Transition (e.g. issue a CMD_ASSOCIATE), operstate is no longer
>> UP.
>
> I'm not sure - I don't know what the state machine in wpa_s goes
> through here. Probably easier to test than try to reason about the
> code...
I'm talking about the kernel behavior, not wpa_s. My guys discovered
that the kernel seems to twiddle operstate automagically? E.g. see
description of
https://git.kernel.org/pub/scm/network/wireless/iwd.git/commit/?id=4d20db05d7c8806749db574e9231bd5d1c476c7f
Regards,
-Denis
^ permalink raw reply
* Re: ROAM/CONNECT event with PORT_AUTHORIZED
From: Denis Kenzior @ 2017-09-14 19:34 UTC (permalink / raw)
To: Johannes Berg, Arend van Spriel, Arend van Spriel, Jouni Malinen
Cc: Avraham Stern, linux-wireless
In-Reply-To: <1505416658.31630.15.camel@sipsolutions.net>
Hi Johannes,
On 09/14/2017 02:17 PM, Johannes Berg wrote:
> On Thu, 2017-09-14 at 13:37 -0500, Denis Kenzior wrote:
>
>> The question is whether all APs are actually sane after a
>> roam. E.g. can the STA assume that the same IP address, DHCP lease,
>> etc is still valid? I heard from various people that this might not
>> be the case, but we haven't had a chance to verify those claims...
>
> I think you pretty much have to assume that, otherwise there's no point
> in roaming at all - you want your connections to stay, possibly voice
> calls to continue, etc.
I'm all for using this assumption. I just wonder if real world
disagrees? :)
> However, I'm not really convinced (any more) that this is actually
> correct. If I'm reading the supplicant code correctly, then it sets
> IF_OPER_UP only once the connection is *completed*, so it's already
> doing what I thought it should be doing and couldn't.
>
Yes. For a new connection it does something like:
New Key
Set Key
New Key (group)
Set Station (Authorized) (which fails on some drivers)
Set OperState to UP
Regards,
-Denis
^ permalink raw reply
* Re: ROAM/CONNECT event with PORT_AUTHORIZED
From: Johannes Berg @ 2017-09-14 19:22 UTC (permalink / raw)
To: Denis Kenzior, Arend van Spriel, Jouni Malinen
Cc: Avraham Stern, linux-wireless
In-Reply-To: <6f177c6d-ff79-bc9b-6ed6-e91a1ad96899@gmail.com>
Hi,
> Yep, but I seem to recall there was some vague language that said the
> AP would delete the PMKSA if the client disconnected.
Ok, not sure about that. But even if the AP does, we could try to send
it and it just can't use it :)
> operstates.txt states that for new connections, operstate should be
> dormant until 802.1x is complete & successful. So the !eapol-over-
> nl condition would violate that, no?
As I just wrote in my other email, I think I'm totally confused
regarding this, and the supplicant already does it correctly - and you
can ignore the whole "!eapol-over-nl" conditions, and just read it like
what I thought we could only do in the eapol-over-nl case.
No idea how I ended up with the idea that you could only send data
frames when the netdev was IF_OPER_UP - that doesn't seem to have any
basis in reality.
> > > > - initialize 1X state machines/timeouts
> > > > - 1X handshake
> > > > - send PMK to device for 4-way-HS
> > > > - AUTHORIZED event
> > > > - [if eapol-over-nl: toggle oper state up]
> > > >
>
> Given your explanation above, should this be [if !eapol-over-nl ..?
> So I agree that OPERSTATE_UP should not change on a roam. I think
> we're both in agreement here.
Great.
> My earlier point is that software roams need to have the exact same
> behavior as well. And my understanding is that when we try to
> Fast-Transition (e.g. issue a CMD_ASSOCIATE), operstate is no longer
> UP.
I'm not sure - I don't know what the state machine in wpa_s goes
through here. Probably easier to test than try to reason about the
code...
> At the very least there's lots of confusion with what is supposed to
> happen with operstate and when. So if we can work out & document a
> consistent behavior, I'm all for it.
:-)
johannes
^ permalink raw reply
* Re: ROAM/CONNECT event with PORT_AUTHORIZED
From: Johannes Berg @ 2017-09-14 19:17 UTC (permalink / raw)
To: Denis Kenzior, Arend van Spriel, Arend van Spriel, Jouni Malinen
Cc: Avraham Stern, linux-wireless
In-Reply-To: <e418234f-00a0-dd44-7352-d279f49f83e5@gmail.com>
On Thu, 2017-09-14 at 13:37 -0500, Denis Kenzior wrote:
> The question is whether all APs are actually sane after a
> roam. E.g. can the STA assume that the same IP address, DHCP lease,
> etc is still valid? I heard from various people that this might not
> be the case, but we haven't had a chance to verify those claims...
I think you pretty much have to assume that, otherwise there's no point
in roaming at all - you want your connections to stay, possibly voice
calls to continue, etc.
> I think it does make sense to tie one into the other. However, do
> we have a race condition here? E.g. AUTHORIZED is sent on one
> socket, then OPER_STATE is signaled on rtnl. Which one do
> applications rely on?
Regular applications wouldn't really look at nl80211.
> > Note that we *can't* do this right now, otherwise we can't transfer
> > the EAPOL frames; but once we do that over nl80211 we'd be able to.
However, I'm not really convinced (any more) that this is actually
correct. If I'm reading the supplicant code correctly, then it sets
IF_OPER_UP only once the connection is *completed*, so it's already
doing what I thought it should be doing and couldn't.
> *wakes up*
>
> Ah I now seem to remember that I volunteered to look into this before
> my sabbatical :) I think this was in early June? I'm certainly
> still interested in doing so. Let me dust off that portion of my
> brain and come up with a proposal. Unless you already have a clear
> idea of how things should work?
Not really. I guess a new command/event with the frame, and some flags,
I know that at least we want a "don't encrypt" flag, for example.
johannes
^ permalink raw reply
* Re: ROAM/CONNECT event with PORT_AUTHORIZED
From: Denis Kenzior @ 2017-09-14 19:08 UTC (permalink / raw)
To: Johannes Berg, Arend van Spriel, Jouni Malinen
Cc: Avraham Stern, linux-wireless
In-Reply-To: <1505414172.31630.13.camel@sipsolutions.net>
Hi Johannes,
On 09/14/2017 01:36 PM, Johannes Berg wrote:
> Hi Denis,
>
> I'd actually only Cc'ed you to see if you were actually working on
> EAPOL-over-nl80211 :-)
>
>> Curious, isn't sending a PMKID for a non-roaming case not strictly
>> per spec? Is this just to avoid running 802.1x?
>
> I don't think so, what makes you? In -2016 9.4.2.25.5 for example, the
> spec says:
>
> The PMKID Count and List fields are used only in the RSNE in the
> (Re)Association Request frame to an AP and in FT authentication
> sequence frames.
>
> Why should it only be valid in roaming? You might turn off and on wifi
> and still have a PMKSA.
Yep, but I seem to recall there was some vague language that said the AP
would delete the PMKSA if the client disconnected. Sorry, getting back
on track:
>
>> How does this mesh with operstates outlined in
>> Documentation/networking/operstates.txt? Are you treating the
>> 'roaming' case as 802.1x 'reauthentication'?
>
> I think we have to, because otherwise DHCP/IPv6 could happen again
> after roaming - we really can't set the oper state to dormant during
> the roaming.
>
Okay, I can buy this interpretation...
>>> Assuming this assessment is correct, this opens up another
>>> possibility:
>>> tracking the PORT_AUTHORIZED state with a separate event:
>>>
>>> * new connection case - no PMKSA (usage)
>>> - CONNECT event
>>> - [if !eapol-over-nl: toggle oper state up]
>>
>> Doesn't this go against operstates.txt, Section 4?
>
> Why?
operstates.txt states that for new connections, operstate should be
dormant until 802.1x is complete & successful. So the !eapol-over-nl
condition would violate that, no?
>
>> Also, I must be dense, but why is the behavior different between
>> eapol-over-nl and not?
>
> Because for EAPOL-over-data, we actually have to have IF_OPER_UP to be
> able to TX/RX the EAPOL frames.
>
Okay. But then wouldn't userspace now have to deal with two different
behaviors now?
>>> - initialize 1X state machines/timeouts
>>> - 1X handshake
>>> - send PMK to device for 4-way-HS
>>> - AUTHORIZED event
>>> - [if eapol-over-nl: toggle oper state up]
>>>
Given your explanation above, should this be [if !eapol-over-nl ..?
>>> * new connection case - PMKSA usage
>>> - CONNECT event
>>> - [if !eapol-over-nl: toggle oper state up]
>>> - initialize 1X state machines/timeouts
>>> - AUTHORIZED event
>>> - stop 1X state machine/timeouts
>>> - [if eapol-over-nl: toggle oper state up]
>>>
>>> * roaming case - no PMKSA (usage)
>>> - ROAM event
>>> - [no touching oper state]
>>
>> My understanding is that oper state goes back to dormant in case we
>> re-associate. At least we needed to set it back to UP after a fast
>> transition?
>
> Like I said above, I really don't think we should/can do this, I think
> we'd lose at least IPv6 connectivity in the meantime?
>
So I agree that OPERSTATE_UP should not change on a roam. I think we're
both in agreement here.
My earlier point is that software roams need to have the exact same
behavior as well. And my understanding is that when we try to
Fast-Transition (e.g. issue a CMD_ASSOCIATE), operstate is no longer UP.
At the very least there's lots of confusion with what is supposed to
happen with operstate and when. So if we can work out & document a
consistent behavior, I'm all for it.
Regards,
-Denis
^ permalink raw reply
* Re: ROAM/CONNECT event with PORT_AUTHORIZED
From: Denis Kenzior @ 2017-09-14 18:37 UTC (permalink / raw)
To: Johannes Berg, Arend van Spriel, Arend van Spriel, Jouni Malinen
Cc: Avraham Stern, linux-wireless
In-Reply-To: <1505389462.31630.6.camel@sipsolutions.net>
Hi Johannes,
On 09/14/2017 06:44 AM, Johannes Berg wrote:
> On Thu, 2017-09-14 at 13:21 +0200, Arend van Spriel wrote:
>
>> Yep. Toggling the OPER_STATE seems to go against what roaming is
>> about.
>
> Agree.
The question is whether all APs are actually sane after a roam. E.g.
can the STA assume that the same IP address, DHCP lease, etc is still
valid? I heard from various people that this might not be the case, but
we haven't had a chance to verify those claims...
>
>> Come to think of it, is it a good idea to tightly couple
>> PORT_AUTHORIZED to OPER_STATE. Aren't these separate concepts in
>> different layers of the network stack.
>
> Well, I think that coupling would make the most sense, since once you
> have oper state UP you'll try to get IPv6 etc., no? And before being
> authorized there's no point.
>
I think it does make sense to tie one into the other. However, do we
have a race condition here? E.g. AUTHORIZED is sent on one socket, then
OPER_STATE is signaled on rtnl. Which one do applications rely on?
> Note that we *can't* do this right now, otherwise we can't transfer the
> EAPOL frames; but once we do that over nl80211 we'd be able to.
>
>> In earlier discussions the proposal for a separate event was made by
>> Jithu (colleague). In brcmfmac it would become a bit less
>> complicated with a separate event so it has my vote as well. So the
>> AUTHORIZED event will have no attributes, right? So if the event
>> occurs it is AUTHORIZED.
>
> I think so, yes. I pondered having the attribute in there so you could
> explicitly have a "not authorized" event, but do we really need that?
> If you get disconnected that's pretty much implied, so ... I don't
> think we need it.
>
>
>>> (*) is anyone working on that? I'll throw it on my list if not.
>
> ["that" being EAPOL-over-nl80211]
>
>> The last I saw on this was Denis Kenzior volunteering for it, but
>> that was about it.
>
> Oh, thanks for the reminder, I'd forgotten entirely...
> Denis?
*wakes up*
Ah I now seem to remember that I volunteered to look into this before my
sabbatical :) I think this was in early June? I'm certainly still
interested in doing so. Let me dust off that portion of my brain and
come up with a proposal. Unless you already have a clear idea of how
things should work?
Regards,
-Denis
^ permalink raw reply
* Re: ROAM/CONNECT event with PORT_AUTHORIZED
From: Johannes Berg @ 2017-09-14 18:36 UTC (permalink / raw)
To: Denis Kenzior, Arend van Spriel, Jouni Malinen
Cc: Avraham Stern, linux-wireless
In-Reply-To: <14eb89c4-680b-a1b9-c430-9f92a72bb86c@gmail.com>
Hi Denis,
I'd actually only Cc'ed you to see if you were actually working on
EAPOL-over-nl80211 :-)
> Curious, isn't sending a PMKID for a non-roaming case not strictly
> per spec? Is this just to avoid running 802.1x?
I don't think so, what makes you? In -2016 9.4.2.25.5 for example, the
spec says:
The PMKID Count and List fields are used only in the RSNE in the
(Re)Association Request frame to an AP and in FT authentication
sequence frames.
Why should it only be valid in roaming? You might turn off and on wifi
and still have a PMKSA.
> How does this mesh with operstates outlined in
> Documentation/networking/operstates.txt? Are you treating the
> 'roaming' case as 802.1x 'reauthentication'?
I think we have to, because otherwise DHCP/IPv6 could happen again
after roaming - we really can't set the oper state to dormant during
the roaming.
> > Assuming this assessment is correct, this opens up another
> > possibility:
> > tracking the PORT_AUTHORIZED state with a separate event:
> >
> > * new connection case - no PMKSA (usage)
> > - CONNECT event
> > - [if !eapol-over-nl: toggle oper state up]
>
> Doesn't this go against operstates.txt, Section 4?
Why?
> Also, I must be dense, but why is the behavior different between
> eapol-over-nl and not?
Because for EAPOL-over-data, we actually have to have IF_OPER_UP to be
able to TX/RX the EAPOL frames.
> > - initialize 1X state machines/timeouts
> > - 1X handshake
> > - send PMK to device for 4-way-HS
> > - AUTHORIZED event
> > - [if eapol-over-nl: toggle oper state up]
> >
> > * new connection case - PMKSA usage
> > - CONNECT event
> > - [if !eapol-over-nl: toggle oper state up]
> > - initialize 1X state machines/timeouts
> > - AUTHORIZED event
> > - stop 1X state machine/timeouts
> > - [if eapol-over-nl: toggle oper state up]
> >
> > * roaming case - no PMKSA (usage)
> > - ROAM event
> > - [no touching oper state]
>
> My understanding is that oper state goes back to dormant in case we
> re-associate. At least we needed to set it back to UP after a fast
> transition?
Like I said above, I really don't think we should/can do this, I think
we'd lose at least IPv6 connectivity in the meantime?
johannes
^ permalink raw reply
* Re: ROAM/CONNECT event with PORT_AUTHORIZED
From: Denis Kenzior @ 2017-09-14 18:27 UTC (permalink / raw)
To: Johannes Berg, Arend van Spriel, Jouni Malinen
Cc: Avraham Stern, linux-wireless
In-Reply-To: <1505378361.31630.2.camel@sipsolutions.net>
Hi Johannes,
On 09/14/2017 03:39 AM, Johannes Berg wrote:
> Hi Arend, Jouni, all,
>
> Avi and I just discussed another use case for the PORT_AUTHORIZED flag,
> which is PMKSA caching.
>
> Assume you have 4-way-HS offloaded, then if you send a PMKID in the new
> connection (doesn't matter if it's roaming or not), the AP may chose to
> use it and start the 4-way-HS, or it may not be able to and start the
> 802.1X handshake.
Curious, isn't sending a PMKID for a non-roaming case not strictly per
spec? Is this just to avoid running 802.1x?
>
> In the supplicant, if you are waiting for the 1X handshake, then in the
> case the PMKSA gets used the supplicant would wait forever, and
> eventually terminate the connection because the handshake isn't
> successful.
>
> Thus, *both* cases need to know about the PORT_AUTHORIZED flag.
>
> I previously didn't apply the patch for the flag in CONNECT
> notification, but nobody is using it in ROAM notification so far, so
> that we still have a chance of revisiting this.
>
> Throwing a potential EAPOL-over-nl80211 (*) into the mix complicates
> the picture further. For example, in that case the OPER_STATE might not
> be set to UP until the port is authorized. In the case of roaming, this
> might - on first sight - lead to toggling DOWN/UP if a new 1X handshake
> needs to be done and the PORT_AUTHORIZED flag doesn't come with the
> ROAM event. However, I think this is actually wrong as it would
> probably lose IPv6 address assignment etc. - so I think we don't want
> to do this even if we do have EAPOL-over-nl80211 and don't need the
> oper_state to be up in order to do the 1X handshake. Do you agree with
> this assessment?
How does this mesh with operstates outlined in
Documentation/networking/operstates.txt? Are you treating the 'roaming'
case as 802.1x 'reauthentication'?
>
> Assuming this assessment is correct, this opens up another possibility:
> tracking the PORT_AUTHORIZED state with a separate event:
>
> * new connection case - no PMKSA (usage)
> - CONNECT event
> - [if !eapol-over-nl: toggle oper state up]
Doesn't this go against operstates.txt, Section 4?
Also, I must be dense, but why is the behavior different between
eapol-over-nl and not?
> - initialize 1X state machines/timeouts
> - 1X handshake
> - send PMK to device for 4-way-HS
> - AUTHORIZED event
> - [if eapol-over-nl: toggle oper state up]
>
> * new connection case - PMKSA usage
> - CONNECT event
> - [if !eapol-over-nl: toggle oper state up]
> - initialize 1X state machines/timeouts
> - AUTHORIZED event
> - stop 1X state machine/timeouts
> - [if eapol-over-nl: toggle oper state up]
>
> * roaming case - no PMKSA (usage)
> - ROAM event
> - [no touching oper state]
My understanding is that oper state goes back to dormant in case we
re-associate. At least we needed to set it back to UP after a fast
transition?
> - initialize 1X state machines/timeouts
> - 1X handshake
> - AUTHORIZED event
> - [no touching oper state]
>
> * roaming case - PMKSA usage
> - ROAM event
> - [no touching oper state]
> - initialize 1X state machines/timeouts
> - AUTHORIZED event
> - stop 1X state machine/timeouts
> - [no touching oper state]
>
>
> Does this seem reasonable? If possible, I prefer this to just having
> the attribute, because with the attribute the roaming case will be
> difficult, the natural code flow in the driver would probably make it
> end up looking like this:
>
> * roaming case - no PMKSA (usage)
> - ROAM event
> - [no touching
> oper state]
> - initialize 1X state machines/timeouts
> - 1X
> handshake
> - CONNECT event w/ PORT_AUTHORIZED
> - [no touching oper
> state]
>
> which seems awkward.
>
> Any other thoughts?
A separate authorized event seems fine to me...
Regards,
-Denis
^ permalink raw reply
* [PATCH] rtlwifi: rtl8192ee: Fix memory leak when loading firmware
From: Larry Finger @ 2017-09-14 18:17 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Larry Finger, Stable
In routine rtl92ee_set_fw_rsvdpagepkt(), the driver allocates an skb, but
never calls rtl_cmd_send_packet(), which will free the buffer. All other
rtlwifi drivers perform this operation correctly.
This problem has been in the driver since it was included in the kernel.
Fortunately, each firmware load only leaks 4 buffers, which likely
explains why it has not previously been detected.
Cc: Stable <stable@vger.kernel.org> # 3.18+
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
drivers/net/wireless/realtek/rtlwifi/rtl8192ee/fw.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/fw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/fw.c
index 7eae27f8e173..f9563ae301ad 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/fw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/fw.c
@@ -682,7 +682,7 @@ void rtl92ee_set_fw_rsvdpagepkt(struct ieee80211_hw *hw, bool b_dl_finished)
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rtl_mac *mac = rtl_mac(rtl_priv(hw));
struct sk_buff *skb = NULL;
-
+ bool rtstatus;
u32 totalpacketlen;
u8 u1rsvdpageloc[5] = { 0 };
bool b_dlok = false;
@@ -768,7 +768,9 @@ void rtl92ee_set_fw_rsvdpagepkt(struct ieee80211_hw *hw, bool b_dl_finished)
skb = dev_alloc_skb(totalpacketlen);
skb_put_data(skb, &reserved_page_packet, totalpacketlen);
- b_dlok = true;
+ rtstatus = rtl_cmd_send_packet(hw, skb);
+ if (rtstatus)
+ b_dlok = true;
if (b_dlok) {
RT_TRACE(rtlpriv, COMP_POWER, DBG_LOUD ,
--
2.12.3
^ permalink raw reply related
* Re: RTL8192EE PCIe Wireless Network Adapter crashed with linux-4.13
From: Larry Finger @ 2017-09-14 18:05 UTC (permalink / raw)
To: Zwindl, linux-wireless@vger.kernel.org
Cc: chaoming_li@realsil.com.cn, kvalo@codeaurora.org,
pkshih@realtek.com, johannes.berg@intel.com,
gregkh@linuxfoundation.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <2WhCEttl4IJtrTB2AkJYmhbsu-kiR_5fU0A_Z7eWW05rbVE5tB86A_ZS8ek_FQFdgaZw1bfx0wFbxOa0Ydjv1T6BzBkIHtaWTbSptcpB_kg=@protonmail.com>
On 09/14/2017 08:30 AM, Zwindl wrote:
> Dear developers:
> I'm using Arch Linux with testing enabled, the current kernel version and
> details are
> `Linux zwindl 4.13.2-1-ARCH #1 SMP PREEMPT Thu Sep 14 02:57:34 UTC 2017 x86_64
> GNU/Linux`.
> The wireless card can't work properly from the kernel 4.13. Here's the log(in
> attachment) when NetworkManager trying to connect my wifi which is named as
> 'TP', my mac addr hided as xx:xx:xx:xx:xx.
> What should I provide to help to debug?
> ZWindL.
The BUG-ON arises in __intel_map_single() due to dir (for direction of DMA)
equal to DMA_NONE (3). When rtl8192ee calls pci_map_single(), it uses
PCI_DMA_TODEVICE (1). I followed the calling sequence through the entire chain,
and none of the routines made any changes to 'dir', other that changing the type
from int to enum dma_data_direction. That would not have changed a 1 to a 3.
I built a 4.13.2 system. The problem does not happen here. At this point, the
system has been up for about two hours. I did discover a small memory leak
associated with firmware loading, but that should not have caused the problem.
Nonetheless, I will be sending a patch to fix that problem.
I will continue testing, although I doubt that the problem will happen here.
How long had your system been up when the problem occurred? Your dmesg fragment
did not show any times. What kernels have you tried besides 4.13.2?
Larry
^ permalink raw reply
* Re: iwlwifi firmware load broken in current -git
From: Johannes Berg @ 2017-09-14 17:50 UTC (permalink / raw)
To: Srinath Mannam, Jens Axboe
Cc: Bjorn Helgaas, Luca Coelho, Grumbach, Emmanuel, linuxwifi,
linux-wireless@vger.kernel.org, linux-pci@vger.kernel.org
In-Reply-To: <CABe79T5fzTthk=c9891iT-_pQrNGaf+THzyi-B7cdqghX15Vug@mail.gmail.com>
On Thu, 2017-09-14 at 23:14 +0530, Srinath Mannam wrote:
>
> atomic_inc_return(&dev->enable_cnt) in function
> pci_enable_device_flags enables passed pcie device.
> !pci_is_enabled(bridge) check in "if (bridge &&
> !pci_is_enabled(bridge))" checks for bridge device of previous pcie
> device.
> So it will not stop bus_master of bridge device.
It also doesn't *enable* it though, does it?
johannes
^ permalink raw reply
* Re: iwlwifi firmware load broken in current -git
From: Jens Axboe @ 2017-09-14 17:45 UTC (permalink / raw)
To: Srinath Mannam
Cc: Bjorn Helgaas, Johannes Berg, Luca Coelho, Grumbach, Emmanuel,
linuxwifi, linux-wireless@vger.kernel.org,
linux-pci@vger.kernel.org
In-Reply-To: <CABe79T5fzTthk=c9891iT-_pQrNGaf+THzyi-B7cdqghX15Vug@mail.gmail.com>
On 09/14/2017 11:44 AM, Srinath Mannam wrote:
> Hi Jens Axboe,
>
> On Thu, Sep 14, 2017 at 11:05 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 09/14/2017 11:28 AM, Srinath Mannam wrote:
>>> Hi Bjorn,
>>>
>>> On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
>>>>> [+cc linux-pci]
>>>>>
>>>>> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>>>>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>>>>>
>>>>>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>>>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>>>>>> functional changes.
>>>>>>>
>>>>>>> and pci_enable_bridge() already checks if it's already enabled, but
>>>>>>> still enables mastering in that case if it isn't:
>>>>>>>
>>>>>>> static void pci_enable_bridge(struct pci_dev *dev)
>>>>>>> {
>>>>>>> [...]
>>>>>>> if (pci_is_enabled(dev)) {
>>>>>>> if (!dev->is_busmaster)
>>>>>>> pci_set_master(dev);
>>>>>>> return;
>>>>>>> }
>>>>>>>
>>>>>>> so I guess due to the new check we end up with mastering disabled, and
>>>>>>> thus the firmware can't load since that's a DMA thing?
>>>>>>
>>>>>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>>>>>> from working on a pretty standard laptop. It'd suck to have this be in
>>>>>> -rc1. Seems like the trivial fix would be:
>>>>>>
>>>>>>
>>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>>> index b0002daa50f3..ffbe11dbdd61 100644
>>>>>> --- a/drivers/pci/pci.c
>>>>>> +++ b/drivers/pci/pci.c
>>>>>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>>>>> return 0; /* already enabled */
>>>>>>
>>>>>> bridge = pci_upstream_bridge(dev);
>>>>>> - if (bridge && !pci_is_enabled(bridge))
>>>>>> + if (bridge)
>>> With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
>>> pci_enable_bridge functoin will causes a nexted lock.
>>
>> Took a look, and looks like you are right. That code looks like a mess,
>> fwiw.
>>
>> I'd strongly suggest that the bad commit is reverted until a proper
>> solution is found, since the simple one-liner could potentially
>> introduce a deadlock with your patch applied.
> atomic_inc_return(&dev->enable_cnt) in function
> pci_enable_device_flags enables passed pcie device.
> !pci_is_enabled(bridge) check in "if (bridge &&
> !pci_is_enabled(bridge))" checks for bridge device of previous pcie
> device.
> So it will not stop bus_master of bridge device.
> In your case how many bridges in between endpoint and host bridge?
> Please provide the details about your use case.
It's a bog standard Lenovo X1 Carbon, gen4.
# lspci
00:00.0 Host bridge: Intel Corporation Sky Lake Host Bridge/DRAM Registers (rev 08)
00:02.0 VGA compatible controller: Intel Corporation Sky Lake Integrated Graphics (rev 07)
00:08.0 System peripheral: Intel Corporation Sky Lake Gaussian Mixture Model
00:13.0 Non-VGA unclassified device: Intel Corporation Device 9d35 (rev 21)
00:14.0 USB controller: Intel Corporation Sunrise Point-LP USB 3.0 xHCI Controller (rev 21)
00:14.2 Signal processing controller: Intel Corporation Sunrise Point-LP Thermal subsystem (rev 21)
00:16.0 Communication controller: Intel Corporation Sunrise Point-LP CSME HECI (rev 21)
00:1c.0 PCI bridge: Intel Corporation Device 9d10 (rev f1)
00:1c.2 PCI bridge: Intel Corporation Device 9d12 (rev f1)
00:1c.4 PCI bridge: Intel Corporation Sunrise Point-LP PCI Express Root Port (rev f1)
00:1f.0 ISA bridge: Intel Corporation Sunrise Point-LP LPC Controller (rev 21)
00:1f.2 Memory controller: Intel Corporation Sunrise Point-LP PMC (rev 21)
00:1f.3 Audio device: Intel Corporation Sunrise Point-LP HD Audio (rev 21)
00:1f.4 SMBus: Intel Corporation Sunrise Point-LP SMBus (rev 21)
00:1f.6 Ethernet controller: Intel Corporation Ethernet Connection I219-LM (rev 21)
04:00.0 Network controller: Intel Corporation Wireless 8260 (rev 3a)
05:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller (rev 01)
# lspci -t
-[0000:00]-+-00.0
+-02.0
+-08.0
+-13.0
+-14.0
+-14.2
+-16.0
+-1c.0-[02]--
+-1c.2-[04]----00.0
+-1c.4-[05]----00.0
+-1f.0
+-1f.2
+-1f.3
+-1f.4
\-1f.6
--
Jens Axboe
^ permalink raw reply
* Re: iwlwifi firmware load broken in current -git
From: Jens Axboe @ 2017-09-14 17:44 UTC (permalink / raw)
To: Srinath Mannam
Cc: Bjorn Helgaas, Johannes Berg, Luca Coelho, Grumbach, Emmanuel,
linuxwifi, linux-wireless@vger.kernel.org,
linux-pci@vger.kernel.org
In-Reply-To: <8de8d1e2-1673-e1ff-d0ac-6d3bb03e9e99@kernel.dk>
On 09/14/2017 11:35 AM, Jens Axboe wrote:
> On 09/14/2017 11:28 AM, Srinath Mannam wrote:
>> Hi Bjorn,
>>
>> On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
>>>> [+cc linux-pci]
>>>>
>>>> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>>>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>>>>
>>>>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>>>>> functional changes.
>>>>>>
>>>>>> and pci_enable_bridge() already checks if it's already enabled, but
>>>>>> still enables mastering in that case if it isn't:
>>>>>>
>>>>>> static void pci_enable_bridge(struct pci_dev *dev)
>>>>>> {
>>>>>> [...]
>>>>>> if (pci_is_enabled(dev)) {
>>>>>> if (!dev->is_busmaster)
>>>>>> pci_set_master(dev);
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> so I guess due to the new check we end up with mastering disabled, and
>>>>>> thus the firmware can't load since that's a DMA thing?
>>>>>
>>>>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>>>>> from working on a pretty standard laptop. It'd suck to have this be in
>>>>> -rc1. Seems like the trivial fix would be:
>>>>>
>>>>>
>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>> index b0002daa50f3..ffbe11dbdd61 100644
>>>>> --- a/drivers/pci/pci.c
>>>>> +++ b/drivers/pci/pci.c
>>>>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>>>> return 0; /* already enabled */
>>>>>
>>>>> bridge = pci_upstream_bridge(dev);
>>>>> - if (bridge && !pci_is_enabled(bridge))
>>>>> + if (bridge)
>> With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
>> pci_enable_bridge functoin will causes a nexted lock.
>
> Took a look, and looks like you are right. That code looks like a mess,
> fwiw.
>
> I'd strongly suggest that the bad commit is reverted until a proper
> solution is found, since the simple one-liner could potentially
> introduce a deadlock with your patch applied.
BTW, your patch looks pretty bad too, introducing a random mutex
deep on code that can be recursive. Why isn't this check in
pci_enable_device_flags() enough to prevent double-enable of
an upstream bridge?
if (atomic_inc_return(&dev->enable_cnt) > 1)
return 0; /* already enabled */
--
Jens Axboe
^ permalink raw reply
* Re: iwlwifi firmware load broken in current -git
From: Srinath Mannam @ 2017-09-14 17:44 UTC (permalink / raw)
To: Jens Axboe
Cc: Bjorn Helgaas, Johannes Berg, Luca Coelho, Grumbach, Emmanuel,
linuxwifi, linux-wireless@vger.kernel.org,
linux-pci@vger.kernel.org
In-Reply-To: <8de8d1e2-1673-e1ff-d0ac-6d3bb03e9e99@kernel.dk>
Hi Jens Axboe,
On Thu, Sep 14, 2017 at 11:05 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 09/14/2017 11:28 AM, Srinath Mannam wrote:
>> Hi Bjorn,
>>
>> On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
>>>> [+cc linux-pci]
>>>>
>>>> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>>>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>>>>
>>>>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>>>>> functional changes.
>>>>>>
>>>>>> and pci_enable_bridge() already checks if it's already enabled, but
>>>>>> still enables mastering in that case if it isn't:
>>>>>>
>>>>>> static void pci_enable_bridge(struct pci_dev *dev)
>>>>>> {
>>>>>> [...]
>>>>>> if (pci_is_enabled(dev)) {
>>>>>> if (!dev->is_busmaster)
>>>>>> pci_set_master(dev);
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> so I guess due to the new check we end up with mastering disabled, and
>>>>>> thus the firmware can't load since that's a DMA thing?
>>>>>
>>>>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>>>>> from working on a pretty standard laptop. It'd suck to have this be in
>>>>> -rc1. Seems like the trivial fix would be:
>>>>>
>>>>>
>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>> index b0002daa50f3..ffbe11dbdd61 100644
>>>>> --- a/drivers/pci/pci.c
>>>>> +++ b/drivers/pci/pci.c
>>>>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>>>> return 0; /* already enabled */
>>>>>
>>>>> bridge = pci_upstream_bridge(dev);
>>>>> - if (bridge && !pci_is_enabled(bridge))
>>>>> + if (bridge)
>> With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
>> pci_enable_bridge functoin will causes a nexted lock.
>
> Took a look, and looks like you are right. That code looks like a mess,
> fwiw.
>
> I'd strongly suggest that the bad commit is reverted until a proper
> solution is found, since the simple one-liner could potentially
> introduce a deadlock with your patch applied.
atomic_inc_return(&dev->enable_cnt) in function
pci_enable_device_flags enables passed pcie device.
!pci_is_enabled(bridge) check in "if (bridge &&
!pci_is_enabled(bridge))" checks for bridge device of previous pcie
device.
So it will not stop bus_master of bridge device.
In your case how many bridges in between endpoint and host bridge?
Please provide the details about your use case.
Thank you.
>
> --
> Jens Axboe
>
Regards,
Srinath.
^ permalink raw reply
* Re: iwlwifi firmware load broken in current -git
From: Jens Axboe @ 2017-09-14 17:35 UTC (permalink / raw)
To: Srinath Mannam
Cc: Bjorn Helgaas, Johannes Berg, Luca Coelho, Grumbach, Emmanuel,
linuxwifi, linux-wireless@vger.kernel.org,
linux-pci@vger.kernel.org
In-Reply-To: <CABe79T7WObM+LOGdPFfYC7hf0dKrhm5NwMgQ3DpcWst3eP6kNg@mail.gmail.com>
On 09/14/2017 11:28 AM, Srinath Mannam wrote:
> Hi Bjorn,
>
> On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
>>> [+cc linux-pci]
>>>
>>> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>>>
>>>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>>>> functional changes.
>>>>>
>>>>> and pci_enable_bridge() already checks if it's already enabled, but
>>>>> still enables mastering in that case if it isn't:
>>>>>
>>>>> static void pci_enable_bridge(struct pci_dev *dev)
>>>>> {
>>>>> [...]
>>>>> if (pci_is_enabled(dev)) {
>>>>> if (!dev->is_busmaster)
>>>>> pci_set_master(dev);
>>>>> return;
>>>>> }
>>>>>
>>>>> so I guess due to the new check we end up with mastering disabled, and
>>>>> thus the firmware can't load since that's a DMA thing?
>>>>
>>>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>>>> from working on a pretty standard laptop. It'd suck to have this be in
>>>> -rc1. Seems like the trivial fix would be:
>>>>
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index b0002daa50f3..ffbe11dbdd61 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>>> return 0; /* already enabled */
>>>>
>>>> bridge = pci_upstream_bridge(dev);
>>>> - if (bridge && !pci_is_enabled(bridge))
>>>> + if (bridge)
> With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
> pci_enable_bridge functoin will causes a nexted lock.
Took a look, and looks like you are right. That code looks like a mess,
fwiw.
I'd strongly suggest that the bad commit is reverted until a proper
solution is found, since the simple one-liner could potentially
introduce a deadlock with your patch applied.
--
Jens Axboe
^ permalink raw reply
* Re: iwlwifi firmware load broken in current -git
From: Srinath Mannam @ 2017-09-14 17:28 UTC (permalink / raw)
To: Jens Axboe
Cc: Bjorn Helgaas, Johannes Berg, Luca Coelho, Grumbach, Emmanuel,
linuxwifi, linux-wireless@vger.kernel.org,
linux-pci@vger.kernel.org
In-Reply-To: <e7af2f85-0ab3-e524-a544-c2ccdf4f8bd6@kernel.dk>
Hi Bjorn,
On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>
> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
> > [+cc linux-pci]
> >
> > On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
> >> On 09/12/2017 02:04 PM, Johannes Berg wrote:
> >>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
> >>>
> >>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
> >>>> pci_is_enabled() check, since the rest of the patch shouldn't have
> >>>> functional changes.
> >>>
> >>> and pci_enable_bridge() already checks if it's already enabled, but
> >>> still enables mastering in that case if it isn't:
> >>>
> >>> static void pci_enable_bridge(struct pci_dev *dev)
> >>> {
> >>> [...]
> >>> if (pci_is_enabled(dev)) {
> >>> if (!dev->is_busmaster)
> >>> pci_set_master(dev);
> >>> return;
> >>> }
> >>>
> >>> so I guess due to the new check we end up with mastering disabled, and
> >>> thus the firmware can't load since that's a DMA thing?
> >>
> >> Bjorn/Srinath, any input here? This is a regression that prevents wifi
> >> from working on a pretty standard laptop. It'd suck to have this be in
> >> -rc1. Seems like the trivial fix would be:
> >>
> >>
> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> index b0002daa50f3..ffbe11dbdd61 100644
> >> --- a/drivers/pci/pci.c
> >> +++ b/drivers/pci/pci.c
> >> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
> >> return 0; /* already enabled */
> >>
> >> bridge = pci_upstream_bridge(dev);
> >> - if (bridge && !pci_is_enabled(bridge))
> >> + if (bridge)
With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
pci_enable_bridge functoin will causes a nexted lock.
> >> pci_enable_bridge(bridge);
> >>
> >> /* only skip sriov related */
> >>
> >>
> >
> > Looks like a reasonable fix. I assume it works for you? I don't have
> > a way to test it, so if you can verify that it works and supply a
> > Signed-off-by, I can merge it.
>
> Booting it right now, I'll send out a proper version in a few minutes.
>
> --
> Jens Axboe
>
^ permalink raw reply
* Re: iwlwifi firmware load broken in current -git
From: Jens Axboe @ 2017-09-14 17:22 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Johannes Berg, Luca Coelho, Grumbach, Emmanuel, linuxwifi,
linux-wireless@vger.kernel.org, srinath.mannam,
linux-pci@vger.kernel.org
In-Reply-To: <CAErSpo777jq9FVRtmoHXVbzE-ZOc7mnT+eRJ_PhP_Dd=MbKv4g@mail.gmail.com>
On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
> [+cc linux-pci]
>
> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>
>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>> functional changes.
>>>
>>> and pci_enable_bridge() already checks if it's already enabled, but
>>> still enables mastering in that case if it isn't:
>>>
>>> static void pci_enable_bridge(struct pci_dev *dev)
>>> {
>>> [...]
>>> if (pci_is_enabled(dev)) {
>>> if (!dev->is_busmaster)
>>> pci_set_master(dev);
>>> return;
>>> }
>>>
>>> so I guess due to the new check we end up with mastering disabled, and
>>> thus the firmware can't load since that's a DMA thing?
>>
>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>> from working on a pretty standard laptop. It'd suck to have this be in
>> -rc1. Seems like the trivial fix would be:
>>
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index b0002daa50f3..ffbe11dbdd61 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>> return 0; /* already enabled */
>>
>> bridge = pci_upstream_bridge(dev);
>> - if (bridge && !pci_is_enabled(bridge))
>> + if (bridge)
>> pci_enable_bridge(bridge);
>>
>> /* only skip sriov related */
>>
>>
>
> Looks like a reasonable fix. I assume it works for you? I don't have
> a way to test it, so if you can verify that it works and supply a
> Signed-off-by, I can merge it.
Booting it right now, I'll send out a proper version in a few minutes.
--
Jens Axboe
^ permalink raw reply
* Re: iwlwifi firmware load broken in current -git
From: Bjorn Helgaas @ 2017-09-14 17:11 UTC (permalink / raw)
To: Jens Axboe
Cc: Johannes Berg, Luca Coelho, Grumbach, Emmanuel, linuxwifi,
linux-wireless@vger.kernel.org, srinath.mannam,
linux-pci@vger.kernel.org
In-Reply-To: <feabbc25-eb85-ffa2-0fd6-254c07e3574b@kernel.dk>
[+cc linux-pci]
On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>
>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>> functional changes.
>>
>> and pci_enable_bridge() already checks if it's already enabled, but
>> still enables mastering in that case if it isn't:
>>
>> static void pci_enable_bridge(struct pci_dev *dev)
>> {
>> [...]
>> if (pci_is_enabled(dev)) {
>> if (!dev->is_busmaster)
>> pci_set_master(dev);
>> return;
>> }
>>
>> so I guess due to the new check we end up with mastering disabled, and
>> thus the firmware can't load since that's a DMA thing?
>
> Bjorn/Srinath, any input here? This is a regression that prevents wifi
> from working on a pretty standard laptop. It'd suck to have this be in
> -rc1. Seems like the trivial fix would be:
>
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b0002daa50f3..ffbe11dbdd61 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
> return 0; /* already enabled */
>
> bridge = pci_upstream_bridge(dev);
> - if (bridge && !pci_is_enabled(bridge))
> + if (bridge)
> pci_enable_bridge(bridge);
>
> /* only skip sriov related */
>
>
Looks like a reasonable fix. I assume it works for you? I don't have
a way to test it, so if you can verify that it works and supply a
Signed-off-by, I can merge it.
Bjorn
^ permalink raw reply
* Re: iwlwifi firmware load broken in current -git
From: Jens Axboe @ 2017-09-14 17:00 UTC (permalink / raw)
To: Johannes Berg, Luca Coelho, Grumbach, Emmanuel
Cc: linuxwifi, linux-wireless@vger.kernel.org, srinath.mannam,
Bjorn Helgaas
In-Reply-To: <1505246657.1974.11.camel@sipsolutions.net>
On 09/12/2017 02:04 PM, Johannes Berg wrote:
> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>
>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>> pci_is_enabled() check, since the rest of the patch shouldn't have
>> functional changes.
>
> and pci_enable_bridge() already checks if it's already enabled, but
> still enables mastering in that case if it isn't:
>
> static void pci_enable_bridge(struct pci_dev *dev)
> {
> [...]
> if (pci_is_enabled(dev)) {
> if (!dev->is_busmaster)
> pci_set_master(dev);
> return;
> }
>
> so I guess due to the new check we end up with mastering disabled, and
> thus the firmware can't load since that's a DMA thing?
Bjorn/Srinath, any input here? This is a regression that prevents wifi
from working on a pretty standard laptop. It'd suck to have this be in
-rc1. Seems like the trivial fix would be:
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b0002daa50f3..ffbe11dbdd61 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
return 0; /* already enabled */
bridge = pci_upstream_bridge(dev);
- if (bridge && !pci_is_enabled(bridge))
+ if (bridge)
pci_enable_bridge(bridge);
/* only skip sriov related */
--
Jens Axboe
^ permalink raw reply related
* Re: Kernel developer to work on wifi injection in mainline
From: Steve deRosier @ 2017-09-14 15:13 UTC (permalink / raw)
To: Raphael Hertzog; +Cc: linux-wireless, devel, Ben Hutchings
In-Reply-To: <20170914143134.5ybcusoqu74wf6d7@home.ouaza.com>
Hi Raphael,
On Thu, Sep 14, 2017 at 7:31 AM, Raphael Hertzog <buxy@kali.org> wrote:
> (Please keep-me in CC and use reply-to all)
>
...
>
> Thus we are looking for a kernel developer that we could work with on a
> contractor basis to:
> - make wifi injection work on the current Kali kernel on various drivers
> that we want to support
> - upstream as much as possible of changes needed for wifi injection to
> work
> - ensure continued support of the feature in new Linux releases
>
I do this sort of consulting work. While I kept this reply on the list
due to your instructions, I'll send you information directly from my
consulting address.
> I wonder whether there are non-technical hurdles that would prevent
> merging of wifi injection patches. Are there legal challenges to this for
> instance?
>
I don't think there would be a legal challenge provided the code is
yours to sign-off on and isn't patent encumbered.
Personally, I think the biggest issue is interest of the Linux
community as a whole. In other words, if you can convince maintainers
that the change is:
1. valuable to the Linux community members
2. not harmful to the community as a whole
3. worth maintaining (and in part will be responsible for maintaining)
4. not purely based on closed business reasons
then I think it'll be possible to get it upstream.
I'm not that familiar with the patch(es) in question but from my quick
look I don't see an obvious blocking issue with the concepts here.
- Steve
^ permalink raw reply
* Kernel developer to work on wifi injection in mainline
From: Raphael Hertzog @ 2017-09-14 14:31 UTC (permalink / raw)
To: linux-wireless; +Cc: devel, Ben Hutchings
(Please keep-me in CC and use reply-to all)
Hello everybody,
I'm working for Kali Linux/Offensive Security. Pentesters rely on working
wifi injection to do their work but support of this feature is sub-optimal
in mainline Linux. This is why we carry a patch:
http://git.kali.org/gitweb/?p=packages/linux.git;a=blob;f=debian/patches/features/all/kali-wifi-injection.patch;h=0b872f83f3ca09c967d9c0c520e11cf7d75dfd91;hb=refs/heads/kali/master
This feature also tends to break on various drivers from time to time
as is currently the case with carl9170 and rt2800usb:
https://bugs.kali.org/view.php?id=4220
Thus we are looking for a kernel developer that we could work with on a
contractor basis to:
- make wifi injection work on the current Kali kernel on various drivers
that we want to support
- upstream as much as possible of changes needed for wifi injection to
work
- ensure continued support of the feature in new Linux releases
I wonder whether there are non-technical hurdles that would prevent
merging of wifi injection patches. Are there legal challenges to this for
instance?
Cheers,
PS: We track new kernel release out of Debian Testing so we tend to
use all releases for 2 months.
--
Raphaël Hertzog ◈ Kali Linux Developer ◈ Debian Developer
Get the Debian Administrator's Handbook:
→ https://debian-handbook.info/get/
^ permalink raw reply
* RE: [EXT] Re: [PATCH 2/2] mwifiex: print URB submit failure error after threshold attemtps
From: Ganapathi Bhat @ 2017-09-14 14:14 UTC (permalink / raw)
To: Joe Perches
Cc: Cathy Luo, Xinming Hu, Zhiyuan Yang, James Cao, Mangesh Malusare,
linux-wireless@vger.kernel.org
In-Reply-To: <1504238727.2361.1.camel@perches.com>
Hi Joe,
> On Thu, 2017-08-31 at 01:21 +0530, Ganapathi Bhat wrote:
> > Current driver prints dev_alloc_skb failures everytime while
> > submitting RX URBs. This failure might be frequent in some low
> > resource platforms. So, wait for a threshold failure count before
> > start priting the error. This change is a follow up for the 'commit
> > 7b368e3d15c3
> > ("mwifiex: resubmit failed to submit RX URBs in main thread")'
>
> []
>
> > diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c
> > b/drivers/net/wireless/marvell/mwifiex/usb.c
> []
> > @@ -300,9 +300,16 @@ static int mwifiex_usb_submit_rx_urb(struct
> urb_context *ctx, int size)
> > if (card->rx_cmd_ep != ctx->ep) {
> > ctx->skb = dev_alloc_skb(size);
> > if (!ctx->skb) {
> > - mwifiex_dbg(adapter, ERROR,
> > - "%s: dev_alloc_skb failed\n", __func__);
> > + if (++card->rx_urb_failure_count >
> > + MWIFIEX_RX_URB_FAILURE_THRESHOLD) {
> > + mwifiex_dbg(adapter, ERROR,
> > + "%s: dev_alloc_skb failed, failure
> count = %u\n",
> > + __func__,
> > + card->rx_urb_failure_count);
> > + }
> > return -ENOMEM;
>
> Why not use a ratelimit?
Since this is for receive, the packets are from AP side and we cannot lower the rate from AP. On some low performance systems this change will be helpful.
Regards,
Ganapathi
^ permalink raw reply
* RE: [EXT] Re: [PATCH] mwifiex: check for mfg_mode in add_virtual_intf
From: Ganapathi Bhat @ 2017-09-14 14:11 UTC (permalink / raw)
To: Kalle Valo
Cc: linux-wireless@vger.kernel.org, Cathy Luo, Xinming Hu,
Zhiyuan Yang, James Cao, Mangesh Malusare, Brian Norris
In-Reply-To: <87poawmp1t.fsf@purkki.adurom.net>
Hi Kalle,
> -----Original Message-----
> From: Kalle Valo [mailto:kvalo@codeaurora.org]
> Sent: Tuesday, September 12, 2017 11:33 AM
> To: Brian Norris
> Cc: Ganapathi Bhat; linux-wireless@vger.kernel.org; Cathy Luo; Xinming
> Hu; Zhiyuan Yang; James Cao; Mangesh Malusare
> Subject: [EXT] Re: [PATCH] mwifiex: check for mfg_mode in
> add_virtual_intf
>
> External Email
>
> ----------------------------------------------------------------------
> Brian Norris <briannorris@chromium.org> writes:
>
> > Could have used a 'v2' in the subject, but hopefully that doesn't
> > bother Kalle too much.
>
> It does create more work for me when sorting patches so please always
> try include the version in the subject. But no need to resend.
Sure. I will keep this in mind for future submits.
>
> --
> Kalle Valo
Thanks,
Ganapathi
^ permalink raw reply
* Re: ROAM/CONNECT event with PORT_AUTHORIZED
From: Johannes Berg @ 2017-09-14 11:44 UTC (permalink / raw)
To: Arend van Spriel, Arend van Spriel, Jouni Malinen
Cc: Avraham Stern, linux-wireless, Denis Kenzior
In-Reply-To: <e4a3ab99-f49a-0849-56d7-71ce19b1f323@broadcom.com>
On Thu, 2017-09-14 at 13:21 +0200, Arend van Spriel wrote:
> Yep. Toggling the OPER_STATE seems to go against what roaming is
> about.
Agree.
> Come to think of it, is it a good idea to tightly couple
> PORT_AUTHORIZED to OPER_STATE. Aren't these separate concepts in
> different layers of the network stack.
Well, I think that coupling would make the most sense, since once you
have oper state UP you'll try to get IPv6 etc., no? And before being
authorized there's no point.
Note that we *can't* do this right now, otherwise we can't transfer the
EAPOL frames; but once we do that over nl80211 we'd be able to.
> In earlier discussions the proposal for a separate event was made by
> Jithu (colleague). In brcmfmac it would become a bit less
> complicated with a separate event so it has my vote as well. So the
> AUTHORIZED event will have no attributes, right? So if the event
> occurs it is AUTHORIZED.
I think so, yes. I pondered having the attribute in there so you could
explicitly have a "not authorized" event, but do we really need that?
If you get disconnected that's pretty much implied, so ... I don't
think we need it.
> > (*) is anyone working on that? I'll throw it on my list if not.
["that" being EAPOL-over-nl80211]
> The last I saw on this was Denis Kenzior volunteering for it, but
> that was about it.
Oh, thanks for the reminder, I'd forgotten entirely...
Denis?
> By the way, I still have wpa_supplicant patches for 4way-hs
> offloading. I need to dust it off a little and obviously did not
> foresee the change above ;-) Let me know if you (Intel) plan to
> submit patches for it.
We also have some patches, we can go either way.
Thanks!
johannes
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox