Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-30 Thread Arend Van Spriel
On 29-9-2016 23:57, Rafał Miłecki wrote:
> On 27 September 2016 at 11:24, Arend Van Spriel
>  wrote:
>> On 26-9-2016 14:38, Rafał Miłecki wrote:
>>> On 26 September 2016 at 14:13, Rafał Miłecki  wrote:
 On 26 September 2016 at 13:46, Arend Van Spriel
  wrote:
> On 26-9-2016 12:23, Rafał Miłecki wrote:
>> From: Rafał Miłecki 
>>
>> We need to track 802.1x packets to know if there are any pending ones
>> for transmission. This is required for performing key update in the
>> firmware.
>
> The problem we are trying to solve is a pretty old one. The problem is
> that wpa_supplicant uses two separate code paths: EAPOL messaging
> through data path and key configuration though nl80211.

 Can I find it described/reported somewhere?


>> Unfortunately our old tracking code wasn't very accurate. It was
>> treating skb as pending as soon as it was passed by the netif. Actual
>> handling packet to the firmware was happening later as brcmfmac
>> internally queues them and uses its own worker(s).
>
> That does not seem right. As soon as we get a 1x packet we need to wait
> with key configuration regardless whether it is still in the driver or
> handed over to firmware already.

 OK, thanks.
>>>
>>> Actually, it's not OK. I was trying to report/describe/discuss this
>>> problem for over a week. I couldn't get much of answer from you.
>>>
>>> I had to come with a patch I worked on for quite some time. Only then
>>> you decided to react and reply with a reason for a nack. I see this
>>> patch may be wrong (but it's still hard to know what's going wrong
>>> without a proper hostapd bug report). I'd expect you to somehow work &
>>> communicate with open source community.
>>
>> We do or at least make an honest attempt, but there is more on our plate
>> so responses may be delayed. It also does not help when you get anal and
>> preachy when we do respond. Also not OK. In this case the delay is
>> caused because I had to pick up the thread(s) as Hante is on vacation
>> (he needed a break :-p ). However, you started sending patches so I
>> decided to look at and respond to those. Sorry if you felt like we left
>> you hanging to dry.
> 
> I believe I get easily irritated due to my communication experience I
> got so far :(
> 
> 
> Over a year ago I reported brcmfmac can't recover from failed
> register_netdev(ice). This bug remains unfixed.
> 
> In 2014 I reported problem with 80 MHz support. I didn't have hardware
> to fix & test it on my own (you weren't able/allowed to send me one of
> your PCIe cards). In remained broken until I fixed it year later.
> 
> You missed my crash bug report about caused by missing eth_type_trans
> and came with patch on your own a month later.
> 
> Earlier this year I reported you problem with BCM4366 and multiple
> interfaces. I didn't get much help. 3 months later I came with patch
> to workaround the problem but you said there's a better way to do
> this. It took me 2 weeks to figure out a new wlioctl API for that
> while all I needed was a simple hint on "interface_remove".
> 
> Right now I'm waiting to get any answer from you about 4366c0
> firmware. It's still less than 2 weeks since I asked for it, but a
> simple ETA would be nice. I'm actually not sure if I should report
> more problems to you to don't distract you from pending things.

This is a difficult question. All upstream firmware releases for router
chips are put on hold until further notice. Some decisions have been
made, but I have not seen a detailed plan to give an ETA.

> Problems with brcmf_netdev_wait_pend8021x were reported multiples
> times for last few months. When I finally got time for that it took me
> a week to debug them.

For the pend8021x you were sending a number of messages showing debug
progress so not sure whether you wanted our feedback on that. If so a
ping might have done it.

> As you can see, it takes me months to get help on some things. And in
> few cases I never got much help at all. Yes, I was hoping to have you
> more involved into brcmfmac development and problems solving. I guess
> things didn't meet my expectations and I got grumpy & preachy.

Thanks for listing all our failures. Somehow we are very good at getting
each other grumpy. When we provide a patch and you break it up and
submit that to Kalle, we get grumpy and it all piles up to the point
where we have this kind of conversation. As long as it helps to get
things of our chest I can live with that. Hope you can too. We strive to
give support to the community, but the priority is low as it is not
full-time activity.

Regards,
Arend


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-30 Thread Arend Van Spriel
On 29-9-2016 23:57, Rafał Miłecki wrote:
> On 27 September 2016 at 11:24, Arend Van Spriel
>  wrote:
>> On 26-9-2016 14:38, Rafał Miłecki wrote:
>>> On 26 September 2016 at 14:13, Rafał Miłecki  wrote:
 On 26 September 2016 at 13:46, Arend Van Spriel
  wrote:
> On 26-9-2016 12:23, Rafał Miłecki wrote:
>> From: Rafał Miłecki 
>>
>> We need to track 802.1x packets to know if there are any pending ones
>> for transmission. This is required for performing key update in the
>> firmware.
>
> The problem we are trying to solve is a pretty old one. The problem is
> that wpa_supplicant uses two separate code paths: EAPOL messaging
> through data path and key configuration though nl80211.

 Can I find it described/reported somewhere?


>> Unfortunately our old tracking code wasn't very accurate. It was
>> treating skb as pending as soon as it was passed by the netif. Actual
>> handling packet to the firmware was happening later as brcmfmac
>> internally queues them and uses its own worker(s).
>
> That does not seem right. As soon as we get a 1x packet we need to wait
> with key configuration regardless whether it is still in the driver or
> handed over to firmware already.

 OK, thanks.
>>>
>>> Actually, it's not OK. I was trying to report/describe/discuss this
>>> problem for over a week. I couldn't get much of answer from you.
>>>
>>> I had to come with a patch I worked on for quite some time. Only then
>>> you decided to react and reply with a reason for a nack. I see this
>>> patch may be wrong (but it's still hard to know what's going wrong
>>> without a proper hostapd bug report). I'd expect you to somehow work &
>>> communicate with open source community.
>>
>> We do or at least make an honest attempt, but there is more on our plate
>> so responses may be delayed. It also does not help when you get anal and
>> preachy when we do respond. Also not OK. In this case the delay is
>> caused because I had to pick up the thread(s) as Hante is on vacation
>> (he needed a break :-p ). However, you started sending patches so I
>> decided to look at and respond to those. Sorry if you felt like we left
>> you hanging to dry.
> 
> I believe I get easily irritated due to my communication experience I
> got so far :(
> 
> 
> Over a year ago I reported brcmfmac can't recover from failed
> register_netdev(ice). This bug remains unfixed.
> 
> In 2014 I reported problem with 80 MHz support. I didn't have hardware
> to fix & test it on my own (you weren't able/allowed to send me one of
> your PCIe cards). In remained broken until I fixed it year later.
> 
> You missed my crash bug report about caused by missing eth_type_trans
> and came with patch on your own a month later.
> 
> Earlier this year I reported you problem with BCM4366 and multiple
> interfaces. I didn't get much help. 3 months later I came with patch
> to workaround the problem but you said there's a better way to do
> this. It took me 2 weeks to figure out a new wlioctl API for that
> while all I needed was a simple hint on "interface_remove".
> 
> Right now I'm waiting to get any answer from you about 4366c0
> firmware. It's still less than 2 weeks since I asked for it, but a
> simple ETA would be nice. I'm actually not sure if I should report
> more problems to you to don't distract you from pending things.

This is a difficult question. All upstream firmware releases for router
chips are put on hold until further notice. Some decisions have been
made, but I have not seen a detailed plan to give an ETA.

> Problems with brcmf_netdev_wait_pend8021x were reported multiples
> times for last few months. When I finally got time for that it took me
> a week to debug them.

For the pend8021x you were sending a number of messages showing debug
progress so not sure whether you wanted our feedback on that. If so a
ping might have done it.

> As you can see, it takes me months to get help on some things. And in
> few cases I never got much help at all. Yes, I was hoping to have you
> more involved into brcmfmac development and problems solving. I guess
> things didn't meet my expectations and I got grumpy & preachy.

Thanks for listing all our failures. Somehow we are very good at getting
each other grumpy. When we provide a patch and you break it up and
submit that to Kalle, we get grumpy and it all piles up to the point
where we have this kind of conversation. As long as it helps to get
things of our chest I can live with that. Hope you can too. We strive to
give support to the community, but the priority is low as it is not
full-time activity.

Regards,
Arend


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-29 Thread Rafał Miłecki
On 27 September 2016 at 11:24, Arend Van Spriel
 wrote:
> On 26-9-2016 14:38, Rafał Miłecki wrote:
>> On 26 September 2016 at 14:13, Rafał Miłecki  wrote:
>>> On 26 September 2016 at 13:46, Arend Van Spriel
>>>  wrote:
 On 26-9-2016 12:23, Rafał Miłecki wrote:
> From: Rafał Miłecki 
>
> We need to track 802.1x packets to know if there are any pending ones
> for transmission. This is required for performing key update in the
> firmware.

 The problem we are trying to solve is a pretty old one. The problem is
 that wpa_supplicant uses two separate code paths: EAPOL messaging
 through data path and key configuration though nl80211.
>>>
>>> Can I find it described/reported somewhere?
>>>
>>>
> Unfortunately our old tracking code wasn't very accurate. It was
> treating skb as pending as soon as it was passed by the netif. Actual
> handling packet to the firmware was happening later as brcmfmac
> internally queues them and uses its own worker(s).

 That does not seem right. As soon as we get a 1x packet we need to wait
 with key configuration regardless whether it is still in the driver or
 handed over to firmware already.
>>>
>>> OK, thanks.
>>
>> Actually, it's not OK. I was trying to report/describe/discuss this
>> problem for over a week. I couldn't get much of answer from you.
>>
>> I had to come with a patch I worked on for quite some time. Only then
>> you decided to react and reply with a reason for a nack. I see this
>> patch may be wrong (but it's still hard to know what's going wrong
>> without a proper hostapd bug report). I'd expect you to somehow work &
>> communicate with open source community.
>
> We do or at least make an honest attempt, but there is more on our plate
> so responses may be delayed. It also does not help when you get anal and
> preachy when we do respond. Also not OK. In this case the delay is
> caused because I had to pick up the thread(s) as Hante is on vacation
> (he needed a break :-p ). However, you started sending patches so I
> decided to look at and respond to those. Sorry if you felt like we left
> you hanging to dry.

I believe I get easily irritated due to my communication experience I
got so far :(


Over a year ago I reported brcmfmac can't recover from failed
register_netdev(ice). This bug remains unfixed.

In 2014 I reported problem with 80 MHz support. I didn't have hardware
to fix & test it on my own (you weren't able/allowed to send me one of
your PCIe cards). In remained broken until I fixed it year later.

You missed my crash bug report about caused by missing eth_type_trans
and came with patch on your own a month later.

Earlier this year I reported you problem with BCM4366 and multiple
interfaces. I didn't get much help. 3 months later I came with patch
to workaround the problem but you said there's a better way to do
this. It took me 2 weeks to figure out a new wlioctl API for that
while all I needed was a simple hint on "interface_remove".

Right now I'm waiting to get any answer from you about 4366c0
firmware. It's still less than 2 weeks since I asked for it, but a
simple ETA would be nice. I'm actually not sure if I should report
more problems to you to don't distract you from pending things.

Problems with brcmf_netdev_wait_pend8021x were reported multiples
times for last few months. When I finally got time for that it took me
a week to debug them.


As you can see, it takes me months to get help on some things. And in
few cases I never got much help at all. Yes, I was hoping to have you
more involved into brcmfmac development and problems solving. I guess
things didn't meet my expectations and I got grumpy & preachy.

-- 
Rafał


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-29 Thread Rafał Miłecki
On 27 September 2016 at 11:24, Arend Van Spriel
 wrote:
> On 26-9-2016 14:38, Rafał Miłecki wrote:
>> On 26 September 2016 at 14:13, Rafał Miłecki  wrote:
>>> On 26 September 2016 at 13:46, Arend Van Spriel
>>>  wrote:
 On 26-9-2016 12:23, Rafał Miłecki wrote:
> From: Rafał Miłecki 
>
> We need to track 802.1x packets to know if there are any pending ones
> for transmission. This is required for performing key update in the
> firmware.

 The problem we are trying to solve is a pretty old one. The problem is
 that wpa_supplicant uses two separate code paths: EAPOL messaging
 through data path and key configuration though nl80211.
>>>
>>> Can I find it described/reported somewhere?
>>>
>>>
> Unfortunately our old tracking code wasn't very accurate. It was
> treating skb as pending as soon as it was passed by the netif. Actual
> handling packet to the firmware was happening later as brcmfmac
> internally queues them and uses its own worker(s).

 That does not seem right. As soon as we get a 1x packet we need to wait
 with key configuration regardless whether it is still in the driver or
 handed over to firmware already.
>>>
>>> OK, thanks.
>>
>> Actually, it's not OK. I was trying to report/describe/discuss this
>> problem for over a week. I couldn't get much of answer from you.
>>
>> I had to come with a patch I worked on for quite some time. Only then
>> you decided to react and reply with a reason for a nack. I see this
>> patch may be wrong (but it's still hard to know what's going wrong
>> without a proper hostapd bug report). I'd expect you to somehow work &
>> communicate with open source community.
>
> We do or at least make an honest attempt, but there is more on our plate
> so responses may be delayed. It also does not help when you get anal and
> preachy when we do respond. Also not OK. In this case the delay is
> caused because I had to pick up the thread(s) as Hante is on vacation
> (he needed a break :-p ). However, you started sending patches so I
> decided to look at and respond to those. Sorry if you felt like we left
> you hanging to dry.

I believe I get easily irritated due to my communication experience I
got so far :(


Over a year ago I reported brcmfmac can't recover from failed
register_netdev(ice). This bug remains unfixed.

In 2014 I reported problem with 80 MHz support. I didn't have hardware
to fix & test it on my own (you weren't able/allowed to send me one of
your PCIe cards). In remained broken until I fixed it year later.

You missed my crash bug report about caused by missing eth_type_trans
and came with patch on your own a month later.

Earlier this year I reported you problem with BCM4366 and multiple
interfaces. I didn't get much help. 3 months later I came with patch
to workaround the problem but you said there's a better way to do
this. It took me 2 weeks to figure out a new wlioctl API for that
while all I needed was a simple hint on "interface_remove".

Right now I'm waiting to get any answer from you about 4366c0
firmware. It's still less than 2 weeks since I asked for it, but a
simple ETA would be nice. I'm actually not sure if I should report
more problems to you to don't distract you from pending things.

Problems with brcmf_netdev_wait_pend8021x were reported multiples
times for last few months. When I finally got time for that it took me
a week to debug them.


As you can see, it takes me months to get help on some things. And in
few cases I never got much help at all. Yes, I was hoping to have you
more involved into brcmfmac development and problems solving. I guess
things didn't meet my expectations and I got grumpy & preachy.

-- 
Rafał


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-27 Thread Arend Van Spriel
On 26-9-2016 14:38, Rafał Miłecki wrote:
> On 26 September 2016 at 14:13, Rafał Miłecki  wrote:
>> On 26 September 2016 at 13:46, Arend Van Spriel
>>  wrote:
>>> On 26-9-2016 12:23, Rafał Miłecki wrote:
 From: Rafał Miłecki 

 We need to track 802.1x packets to know if there are any pending ones
 for transmission. This is required for performing key update in the
 firmware.
>>>
>>> The problem we are trying to solve is a pretty old one. The problem is
>>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>>> through data path and key configuration though nl80211.
>>
>> Can I find it described/reported somewhere?
>>
>>
 Unfortunately our old tracking code wasn't very accurate. It was
 treating skb as pending as soon as it was passed by the netif. Actual
 handling packet to the firmware was happening later as brcmfmac
 internally queues them and uses its own worker(s).
>>>
>>> That does not seem right. As soon as we get a 1x packet we need to wait
>>> with key configuration regardless whether it is still in the driver or
>>> handed over to firmware already.
>>
>> OK, thanks.
> 
> Actually, it's not OK. I was trying to report/describe/discuss this
> problem for over a week. I couldn't get much of answer from you.
> 
> I had to come with a patch I worked on for quite some time. Only then
> you decided to react and reply with a reason for a nack. I see this
> patch may be wrong (but it's still hard to know what's going wrong
> without a proper hostapd bug report). I'd expect you to somehow work &
> communicate with open source community.

We do or at least make an honest attempt, but there is more on our plate
so responses may be delayed. It also does not help when you get anal and
preachy when we do respond. Also not OK. In this case the delay is
caused because I had to pick up the thread(s) as Hante is on vacation
(he needed a break :-p ). However, you started sending patches so I
decided to look at and respond to those. Sorry if you felt like we left
you hanging to dry.

Regards,
Arend


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-27 Thread Arend Van Spriel
On 26-9-2016 14:38, Rafał Miłecki wrote:
> On 26 September 2016 at 14:13, Rafał Miłecki  wrote:
>> On 26 September 2016 at 13:46, Arend Van Spriel
>>  wrote:
>>> On 26-9-2016 12:23, Rafał Miłecki wrote:
 From: Rafał Miłecki 

 We need to track 802.1x packets to know if there are any pending ones
 for transmission. This is required for performing key update in the
 firmware.
>>>
>>> The problem we are trying to solve is a pretty old one. The problem is
>>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>>> through data path and key configuration though nl80211.
>>
>> Can I find it described/reported somewhere?
>>
>>
 Unfortunately our old tracking code wasn't very accurate. It was
 treating skb as pending as soon as it was passed by the netif. Actual
 handling packet to the firmware was happening later as brcmfmac
 internally queues them and uses its own worker(s).
>>>
>>> That does not seem right. As soon as we get a 1x packet we need to wait
>>> with key configuration regardless whether it is still in the driver or
>>> handed over to firmware already.
>>
>> OK, thanks.
> 
> Actually, it's not OK. I was trying to report/describe/discuss this
> problem for over a week. I couldn't get much of answer from you.
> 
> I had to come with a patch I worked on for quite some time. Only then
> you decided to react and reply with a reason for a nack. I see this
> patch may be wrong (but it's still hard to know what's going wrong
> without a proper hostapd bug report). I'd expect you to somehow work &
> communicate with open source community.

We do or at least make an honest attempt, but there is more on our plate
so responses may be delayed. It also does not help when you get anal and
preachy when we do respond. Also not OK. In this case the delay is
caused because I had to pick up the thread(s) as Hante is on vacation
(he needed a break :-p ). However, you started sending patches so I
decided to look at and respond to those. Sorry if you felt like we left
you hanging to dry.

Regards,
Arend


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-27 Thread Arend Van Spriel
On 26-9-2016 16:59, Dan Williams wrote:
> On Mon, 2016-09-26 at 14:13 +0200, Rafał Miłecki wrote:
>> On 26 September 2016 at 13:46, Arend Van Spriel
>>  wrote:
>>>
>>> On 26-9-2016 12:23, Rafał Miłecki wrote:

 From: Rafał Miłecki 

 We need to track 802.1x packets to know if there are any pending
 ones
 for transmission. This is required for performing key update in
 the
 firmware.
>>>
>>> The problem we are trying to solve is a pretty old one. The problem
>>> is
>>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>>> through data path and key configuration though nl80211.
>>
>> Can I find it described/reported somewhere?
> 
> If I understand the issue correctly, you can find all this in the
> supplicant code.  Once the supplicant has done whatever it wants to do
> with the data frames that just happen to be EAPOL it then sends the
> keys down to the driver with nl80211.

Indeed. EAPOL packets are simply data packets as far as the 802.11 stack
is concerned. The arrival of those in the driver is not predictable
hence we hold off the key configuration until those have been passed
over to firmware.

> But it sounds like, instead of sniffing EAPOL frames in the driver skb
> tracking and sniffing ETH_P_PAE, you should probably implement support
> for NL80211_CMD_CRIT_PROTOCOL_START/NL80211_CMD_CRIT_PROTOCOL_STOP and
> key off the passed-in NL80211_CRIT_PROTO_EAPOL.  At least at the
> beginning of connection setup only EAPOL packets will be allowed
> anyway.
> 
> It doesn't seem like the supplicant uses NL80211_CRIT_PROTO_EAPOL yet,
> but that should also be fixed in the supplicant itself.  You should
> probably get some comments from Jouni on how he'd like to see all this
> work.  But generally the less specific sniffing of frames in drivers,
> likely the better.

Indeed. That was the main motivation to introduce the CRIT_PROTO api. If
I recall correctly it was considered the task of the network manager to
issue the START/STOP. Recently noticed the use of CRIT_PROTO_DHCP on
some target system, which we already support in brcmfmac. From your
response I guess you consider CRIT_PROTO_EAPOL to be issued by the
supplicant.

Regards,
Arend

> Dan
> 
>>
>>>

 Unfortunately our old tracking code wasn't very accurate. It was
 treating skb as pending as soon as it was passed by the netif.
 Actual
 handling packet to the firmware was happening later as brcmfmac
 internally queues them and uses its own worker(s).
>>>
>>> That does not seem right. As soon as we get a 1x packet we need to
>>> wait
>>> with key configuration regardless whether it is still in the driver
>>> or
>>> handed over to firmware already.
>>
>> OK, thanks.


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-27 Thread Arend Van Spriel
On 26-9-2016 16:59, Dan Williams wrote:
> On Mon, 2016-09-26 at 14:13 +0200, Rafał Miłecki wrote:
>> On 26 September 2016 at 13:46, Arend Van Spriel
>>  wrote:
>>>
>>> On 26-9-2016 12:23, Rafał Miłecki wrote:

 From: Rafał Miłecki 

 We need to track 802.1x packets to know if there are any pending
 ones
 for transmission. This is required for performing key update in
 the
 firmware.
>>>
>>> The problem we are trying to solve is a pretty old one. The problem
>>> is
>>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>>> through data path and key configuration though nl80211.
>>
>> Can I find it described/reported somewhere?
> 
> If I understand the issue correctly, you can find all this in the
> supplicant code.  Once the supplicant has done whatever it wants to do
> with the data frames that just happen to be EAPOL it then sends the
> keys down to the driver with nl80211.

Indeed. EAPOL packets are simply data packets as far as the 802.11 stack
is concerned. The arrival of those in the driver is not predictable
hence we hold off the key configuration until those have been passed
over to firmware.

> But it sounds like, instead of sniffing EAPOL frames in the driver skb
> tracking and sniffing ETH_P_PAE, you should probably implement support
> for NL80211_CMD_CRIT_PROTOCOL_START/NL80211_CMD_CRIT_PROTOCOL_STOP and
> key off the passed-in NL80211_CRIT_PROTO_EAPOL.  At least at the
> beginning of connection setup only EAPOL packets will be allowed
> anyway.
> 
> It doesn't seem like the supplicant uses NL80211_CRIT_PROTO_EAPOL yet,
> but that should also be fixed in the supplicant itself.  You should
> probably get some comments from Jouni on how he'd like to see all this
> work.  But generally the less specific sniffing of frames in drivers,
> likely the better.

Indeed. That was the main motivation to introduce the CRIT_PROTO api. If
I recall correctly it was considered the task of the network manager to
issue the START/STOP. Recently noticed the use of CRIT_PROTO_DHCP on
some target system, which we already support in brcmfmac. From your
response I guess you consider CRIT_PROTO_EAPOL to be issued by the
supplicant.

Regards,
Arend

> Dan
> 
>>
>>>

 Unfortunately our old tracking code wasn't very accurate. It was
 treating skb as pending as soon as it was passed by the netif.
 Actual
 handling packet to the firmware was happening later as brcmfmac
 internally queues them and uses its own worker(s).
>>>
>>> That does not seem right. As soon as we get a 1x packet we need to
>>> wait
>>> with key configuration regardless whether it is still in the driver
>>> or
>>> handed over to firmware already.
>>
>> OK, thanks.


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-26 Thread Dan Williams
On Mon, 2016-09-26 at 14:13 +0200, Rafał Miłecki wrote:
> On 26 September 2016 at 13:46, Arend Van Spriel
>  wrote:
> > 
> > On 26-9-2016 12:23, Rafał Miłecki wrote:
> > > 
> > > From: Rafał Miłecki 
> > > 
> > > We need to track 802.1x packets to know if there are any pending
> > > ones
> > > for transmission. This is required for performing key update in
> > > the
> > > firmware.
> > 
> > The problem we are trying to solve is a pretty old one. The problem
> > is
> > that wpa_supplicant uses two separate code paths: EAPOL messaging
> > through data path and key configuration though nl80211.
> 
> Can I find it described/reported somewhere?

If I understand the issue correctly, you can find all this in the
supplicant code.  Once the supplicant has done whatever it wants to do
with the data frames that just happen to be EAPOL it then sends the
keys down to the driver with nl80211.

But it sounds like, instead of sniffing EAPOL frames in the driver skb
tracking and sniffing ETH_P_PAE, you should probably implement support
for NL80211_CMD_CRIT_PROTOCOL_START/NL80211_CMD_CRIT_PROTOCOL_STOP and
key off the passed-in NL80211_CRIT_PROTO_EAPOL.  At least at the
beginning of connection setup only EAPOL packets will be allowed
anyway.

It doesn't seem like the supplicant uses NL80211_CRIT_PROTO_EAPOL yet,
but that should also be fixed in the supplicant itself.  You should
probably get some comments from Jouni on how he'd like to see all this
work.  But generally the less specific sniffing of frames in drivers,
likely the better.

Dan

> 
> > 
> > > 
> > > Unfortunately our old tracking code wasn't very accurate. It was
> > > treating skb as pending as soon as it was passed by the netif.
> > > Actual
> > > handling packet to the firmware was happening later as brcmfmac
> > > internally queues them and uses its own worker(s).
> > 
> > That does not seem right. As soon as we get a 1x packet we need to
> > wait
> > with key configuration regardless whether it is still in the driver
> > or
> > handed over to firmware already.
> 
> OK, thanks.


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-26 Thread Dan Williams
On Mon, 2016-09-26 at 14:13 +0200, Rafał Miłecki wrote:
> On 26 September 2016 at 13:46, Arend Van Spriel
>  wrote:
> > 
> > On 26-9-2016 12:23, Rafał Miłecki wrote:
> > > 
> > > From: Rafał Miłecki 
> > > 
> > > We need to track 802.1x packets to know if there are any pending
> > > ones
> > > for transmission. This is required for performing key update in
> > > the
> > > firmware.
> > 
> > The problem we are trying to solve is a pretty old one. The problem
> > is
> > that wpa_supplicant uses two separate code paths: EAPOL messaging
> > through data path and key configuration though nl80211.
> 
> Can I find it described/reported somewhere?

If I understand the issue correctly, you can find all this in the
supplicant code.  Once the supplicant has done whatever it wants to do
with the data frames that just happen to be EAPOL it then sends the
keys down to the driver with nl80211.

But it sounds like, instead of sniffing EAPOL frames in the driver skb
tracking and sniffing ETH_P_PAE, you should probably implement support
for NL80211_CMD_CRIT_PROTOCOL_START/NL80211_CMD_CRIT_PROTOCOL_STOP and
key off the passed-in NL80211_CRIT_PROTO_EAPOL.  At least at the
beginning of connection setup only EAPOL packets will be allowed
anyway.

It doesn't seem like the supplicant uses NL80211_CRIT_PROTO_EAPOL yet,
but that should also be fixed in the supplicant itself.  You should
probably get some comments from Jouni on how he'd like to see all this
work.  But generally the less specific sniffing of frames in drivers,
likely the better.

Dan

> 
> > 
> > > 
> > > Unfortunately our old tracking code wasn't very accurate. It was
> > > treating skb as pending as soon as it was passed by the netif.
> > > Actual
> > > handling packet to the firmware was happening later as brcmfmac
> > > internally queues them and uses its own worker(s).
> > 
> > That does not seem right. As soon as we get a 1x packet we need to
> > wait
> > with key configuration regardless whether it is still in the driver
> > or
> > handed over to firmware already.
> 
> OK, thanks.


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-26 Thread Rafał Miłecki
On 26 September 2016 at 14:13, Rafał Miłecki  wrote:
> On 26 September 2016 at 13:46, Arend Van Spriel
>  wrote:
>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>> From: Rafał Miłecki 
>>>
>>> We need to track 802.1x packets to know if there are any pending ones
>>> for transmission. This is required for performing key update in the
>>> firmware.
>>
>> The problem we are trying to solve is a pretty old one. The problem is
>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>> through data path and key configuration though nl80211.
>
> Can I find it described/reported somewhere?
>
>
>>> Unfortunately our old tracking code wasn't very accurate. It was
>>> treating skb as pending as soon as it was passed by the netif. Actual
>>> handling packet to the firmware was happening later as brcmfmac
>>> internally queues them and uses its own worker(s).
>>
>> That does not seem right. As soon as we get a 1x packet we need to wait
>> with key configuration regardless whether it is still in the driver or
>> handed over to firmware already.
>
> OK, thanks.

Actually, it's not OK. I was trying to report/describe/discuss this
problem for over a week. I couldn't get much of answer from you.

I had to come with a patch I worked on for quite some time. Only then
you decided to react and reply with a reason for a nack. I see this
patch may be wrong (but it's still hard to know what's going wrong
without a proper hostapd bug report). I'd expect you to somehow work &
communicate with open source community.

-- 
Rafał


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-26 Thread Rafał Miłecki
On 26 September 2016 at 14:13, Rafał Miłecki  wrote:
> On 26 September 2016 at 13:46, Arend Van Spriel
>  wrote:
>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>> From: Rafał Miłecki 
>>>
>>> We need to track 802.1x packets to know if there are any pending ones
>>> for transmission. This is required for performing key update in the
>>> firmware.
>>
>> The problem we are trying to solve is a pretty old one. The problem is
>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>> through data path and key configuration though nl80211.
>
> Can I find it described/reported somewhere?
>
>
>>> Unfortunately our old tracking code wasn't very accurate. It was
>>> treating skb as pending as soon as it was passed by the netif. Actual
>>> handling packet to the firmware was happening later as brcmfmac
>>> internally queues them and uses its own worker(s).
>>
>> That does not seem right. As soon as we get a 1x packet we need to wait
>> with key configuration regardless whether it is still in the driver or
>> handed over to firmware already.
>
> OK, thanks.

Actually, it's not OK. I was trying to report/describe/discuss this
problem for over a week. I couldn't get much of answer from you.

I had to come with a patch I worked on for quite some time. Only then
you decided to react and reply with a reason for a nack. I see this
patch may be wrong (but it's still hard to know what's going wrong
without a proper hostapd bug report). I'd expect you to somehow work &
communicate with open source community.

-- 
Rafał


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-26 Thread Arend Van Spriel


On 26-9-2016 14:13, Rafał Miłecki wrote:
> On 26 September 2016 at 13:46, Arend Van Spriel
>  wrote:
>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>> From: Rafał Miłecki 
>>>
>>> We need to track 802.1x packets to know if there are any pending ones
>>> for transmission. This is required for performing key update in the
>>> firmware.
>>
>> The problem we are trying to solve is a pretty old one. The problem is
>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>> through data path and key configuration though nl80211.
> 
> Can I find it described/reported somewhere?

Not sure. It is something that I recall from working at Intersil so back
in the prism days.

Regards,
Arend

>>> Unfortunately our old tracking code wasn't very accurate. It was
>>> treating skb as pending as soon as it was passed by the netif. Actual
>>> handling packet to the firmware was happening later as brcmfmac
>>> internally queues them and uses its own worker(s).
>>
>> That does not seem right. As soon as we get a 1x packet we need to wait
>> with key configuration regardless whether it is still in the driver or
>> handed over to firmware already.
> 
> OK, thanks.
> 


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-26 Thread Arend Van Spriel


On 26-9-2016 14:13, Rafał Miłecki wrote:
> On 26 September 2016 at 13:46, Arend Van Spriel
>  wrote:
>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>> From: Rafał Miłecki 
>>>
>>> We need to track 802.1x packets to know if there are any pending ones
>>> for transmission. This is required for performing key update in the
>>> firmware.
>>
>> The problem we are trying to solve is a pretty old one. The problem is
>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>> through data path and key configuration though nl80211.
> 
> Can I find it described/reported somewhere?

Not sure. It is something that I recall from working at Intersil so back
in the prism days.

Regards,
Arend

>>> Unfortunately our old tracking code wasn't very accurate. It was
>>> treating skb as pending as soon as it was passed by the netif. Actual
>>> handling packet to the firmware was happening later as brcmfmac
>>> internally queues them and uses its own worker(s).
>>
>> That does not seem right. As soon as we get a 1x packet we need to wait
>> with key configuration regardless whether it is still in the driver or
>> handed over to firmware already.
> 
> OK, thanks.
> 


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-26 Thread Rafał Miłecki
On 26 September 2016 at 13:46, Arend Van Spriel
 wrote:
> On 26-9-2016 12:23, Rafał Miłecki wrote:
>> From: Rafał Miłecki 
>>
>> We need to track 802.1x packets to know if there are any pending ones
>> for transmission. This is required for performing key update in the
>> firmware.
>
> The problem we are trying to solve is a pretty old one. The problem is
> that wpa_supplicant uses two separate code paths: EAPOL messaging
> through data path and key configuration though nl80211.

Can I find it described/reported somewhere?


>> Unfortunately our old tracking code wasn't very accurate. It was
>> treating skb as pending as soon as it was passed by the netif. Actual
>> handling packet to the firmware was happening later as brcmfmac
>> internally queues them and uses its own worker(s).
>
> That does not seem right. As soon as we get a 1x packet we need to wait
> with key configuration regardless whether it is still in the driver or
> handed over to firmware already.

OK, thanks.


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-26 Thread Rafał Miłecki
On 26 September 2016 at 13:46, Arend Van Spriel
 wrote:
> On 26-9-2016 12:23, Rafał Miłecki wrote:
>> From: Rafał Miłecki 
>>
>> We need to track 802.1x packets to know if there are any pending ones
>> for transmission. This is required for performing key update in the
>> firmware.
>
> The problem we are trying to solve is a pretty old one. The problem is
> that wpa_supplicant uses two separate code paths: EAPOL messaging
> through data path and key configuration though nl80211.

Can I find it described/reported somewhere?


>> Unfortunately our old tracking code wasn't very accurate. It was
>> treating skb as pending as soon as it was passed by the netif. Actual
>> handling packet to the firmware was happening later as brcmfmac
>> internally queues them and uses its own worker(s).
>
> That does not seem right. As soon as we get a 1x packet we need to wait
> with key configuration regardless whether it is still in the driver or
> handed over to firmware already.

OK, thanks.


Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-26 Thread Arend Van Spriel
On 26-9-2016 12:23, Rafał Miłecki wrote:
> From: Rafał Miłecki 
> 
> We need to track 802.1x packets to know if there are any pending ones
> for transmission. This is required for performing key update in the
> firmware.

The problem we are trying to solve is a pretty old one. The problem is
that wpa_supplicant uses two separate code paths: EAPOL messaging
through data path and key configuration though nl80211.

> Unfortunately our old tracking code wasn't very accurate. It was
> treating skb as pending as soon as it was passed by the netif. Actual
> handling packet to the firmware was happening later as brcmfmac
> internally queues them and uses its own worker(s).

That does not seem right. As soon as we get a 1x packet we need to wait
with key configuration regardless whether it is still in the driver or
handed over to firmware already.

Regards,
Arend

> Other than that it was hard to handle freeing packets. Everytime we had
> to determine (in more generic funcions) if packet was counted as pending
> 802.1x one or not. It was causing some problems, e.g. it wasn't clear if
> brcmf_flowring_delete should free skb directly or not.
> 
> This patch introduces 2 separated functions for tracking skbs. This
> simplifies logic, fixes brcmf_flowring_delete (maybe other hidden bugs
> as well) and allows further simplifications. Thanks to better accuracy
> is also increases time window for key update (and lowers timeout risk).
> 
> Signed-off-by: Rafał Miłecki 
> ---
> This was successfully tested with 4366b1. Can someone give it a try with
> some USB/SDIO device, please?
> ---
>  .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c| 11 +++
>  .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c  | 12 +++-
>  .../wireless/broadcom/brcm80211/brcmfmac/core.c| 36 
> --
>  .../wireless/broadcom/brcm80211/brcmfmac/core.h|  2 ++
>  .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c  | 14 +++--
>  .../wireless/broadcom/brcm80211/brcmfmac/proto.h   | 11 +++
>  .../wireless/broadcom/brcm80211/brcmfmac/sdio.c|  8 +
>  .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 10 ++
>  8 files changed, 91 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> index d1bc51f..3e40244 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> @@ -326,6 +326,16 @@ brcmf_proto_bcdc_hdrpull(struct brcmf_pub *drvr, bool 
> do_fws,
>   return 0;
>  }
>  
> +static int brcmf_proto_bcdc_hdr_get_ifidx(struct brcmf_pub *drvr,
> +   struct sk_buff *skb)
> +{
> + struct brcmf_proto_bcdc_header *h;
> +
> + h = (struct brcmf_proto_bcdc_header *)(skb->data);
> +
> + return BCDC_GET_IF_IDX(h);
> +}
> +
>  static int
>  brcmf_proto_bcdc_txdata(struct brcmf_pub *drvr, int ifidx, u8 offset,
>   struct sk_buff *pktbuf)
> @@ -373,6 +383,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
>   }
>  
>   drvr->proto->hdrpull = brcmf_proto_bcdc_hdrpull;
> + drvr->proto->hdr_get_ifidx = brcmf_proto_bcdc_hdr_get_ifidx;
>   drvr->proto->query_dcmd = brcmf_proto_bcdc_query_dcmd;
>   drvr->proto->set_dcmd = brcmf_proto_bcdc_set_dcmd;
>   drvr->proto->txdata = brcmf_proto_bcdc_txdata;
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index 03404cb..fef9d02 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -43,6 +43,7 @@
>  #include "chip.h"
>  #include "bus.h"
>  #include "debug.h"
> +#include "proto.h"
>  #include "sdio.h"
>  #include "core.h"
>  #include "common.h"
> @@ -772,6 +773,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, 
> u8 *buf, uint nbytes)
>  int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
>struct sk_buff_head *pktq)
>  {
> + struct brcmf_pub *pub = sdiodev->bus_if->drvr;
>   struct sk_buff *skb;
>   u32 addr = sdiodev->sbwad;
>   int err;
> @@ -784,10 +786,18 @@ int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
>  
>   if (pktq->qlen == 1 || !sdiodev->sg_support)
>   skb_queue_walk(pktq, skb) {
> + struct brcmf_if *ifp;
> + int ifidx;
> +
> + ifidx = brcmf_proto_hdr_get_ifidx(pub, skb);
> + ifp = brcmf_get_ifp(pub, ifidx);
> + brcmf_tx_passing_skb(ifp, skb);
>   err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, true,
>addr, skb);
> - if (err)
> + if (err) {
> + 

Re: [PATCH] brcmfmac: implement more accurate skb tracking

2016-09-26 Thread Arend Van Spriel
On 26-9-2016 12:23, Rafał Miłecki wrote:
> From: Rafał Miłecki 
> 
> We need to track 802.1x packets to know if there are any pending ones
> for transmission. This is required for performing key update in the
> firmware.

The problem we are trying to solve is a pretty old one. The problem is
that wpa_supplicant uses two separate code paths: EAPOL messaging
through data path and key configuration though nl80211.

> Unfortunately our old tracking code wasn't very accurate. It was
> treating skb as pending as soon as it was passed by the netif. Actual
> handling packet to the firmware was happening later as brcmfmac
> internally queues them and uses its own worker(s).

That does not seem right. As soon as we get a 1x packet we need to wait
with key configuration regardless whether it is still in the driver or
handed over to firmware already.

Regards,
Arend

> Other than that it was hard to handle freeing packets. Everytime we had
> to determine (in more generic funcions) if packet was counted as pending
> 802.1x one or not. It was causing some problems, e.g. it wasn't clear if
> brcmf_flowring_delete should free skb directly or not.
> 
> This patch introduces 2 separated functions for tracking skbs. This
> simplifies logic, fixes brcmf_flowring_delete (maybe other hidden bugs
> as well) and allows further simplifications. Thanks to better accuracy
> is also increases time window for key update (and lowers timeout risk).
> 
> Signed-off-by: Rafał Miłecki 
> ---
> This was successfully tested with 4366b1. Can someone give it a try with
> some USB/SDIO device, please?
> ---
>  .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c| 11 +++
>  .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c  | 12 +++-
>  .../wireless/broadcom/brcm80211/brcmfmac/core.c| 36 
> --
>  .../wireless/broadcom/brcm80211/brcmfmac/core.h|  2 ++
>  .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c  | 14 +++--
>  .../wireless/broadcom/brcm80211/brcmfmac/proto.h   | 11 +++
>  .../wireless/broadcom/brcm80211/brcmfmac/sdio.c|  8 +
>  .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 10 ++
>  8 files changed, 91 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> index d1bc51f..3e40244 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> @@ -326,6 +326,16 @@ brcmf_proto_bcdc_hdrpull(struct brcmf_pub *drvr, bool 
> do_fws,
>   return 0;
>  }
>  
> +static int brcmf_proto_bcdc_hdr_get_ifidx(struct brcmf_pub *drvr,
> +   struct sk_buff *skb)
> +{
> + struct brcmf_proto_bcdc_header *h;
> +
> + h = (struct brcmf_proto_bcdc_header *)(skb->data);
> +
> + return BCDC_GET_IF_IDX(h);
> +}
> +
>  static int
>  brcmf_proto_bcdc_txdata(struct brcmf_pub *drvr, int ifidx, u8 offset,
>   struct sk_buff *pktbuf)
> @@ -373,6 +383,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
>   }
>  
>   drvr->proto->hdrpull = brcmf_proto_bcdc_hdrpull;
> + drvr->proto->hdr_get_ifidx = brcmf_proto_bcdc_hdr_get_ifidx;
>   drvr->proto->query_dcmd = brcmf_proto_bcdc_query_dcmd;
>   drvr->proto->set_dcmd = brcmf_proto_bcdc_set_dcmd;
>   drvr->proto->txdata = brcmf_proto_bcdc_txdata;
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index 03404cb..fef9d02 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -43,6 +43,7 @@
>  #include "chip.h"
>  #include "bus.h"
>  #include "debug.h"
> +#include "proto.h"
>  #include "sdio.h"
>  #include "core.h"
>  #include "common.h"
> @@ -772,6 +773,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, 
> u8 *buf, uint nbytes)
>  int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
>struct sk_buff_head *pktq)
>  {
> + struct brcmf_pub *pub = sdiodev->bus_if->drvr;
>   struct sk_buff *skb;
>   u32 addr = sdiodev->sbwad;
>   int err;
> @@ -784,10 +786,18 @@ int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
>  
>   if (pktq->qlen == 1 || !sdiodev->sg_support)
>   skb_queue_walk(pktq, skb) {
> + struct brcmf_if *ifp;
> + int ifidx;
> +
> + ifidx = brcmf_proto_hdr_get_ifidx(pub, skb);
> + ifp = brcmf_get_ifp(pub, ifidx);
> + brcmf_tx_passing_skb(ifp, skb);
>   err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, true,
>addr, skb);
> - if (err)
> + if (err) {
> + brcmf_tx_regained_skb(ifp, skb);
>