LinuxLists.cc - [PATCH V3] Add iwlwifi wireless drivers

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

I was just looking through the iwlwifi sources to find out whether it
can encrypt with a key even if the packet isn't unicast... Anybody know?
I myself removed the ability to upload multicast/broadcast keys because
it was so unclear, but it seems that maybe the firmware knows about some
sort of "virtual broadcast/multicast" station? How about multiple
broadcast keys for APs with VLANs?

In any case, I found this comment:

/* XXX: ACK flag must be set for CCMP even if it
* is a multicast/broadcast packet, because CCMP
* group communication encrypted by GTK is
* actually done by the AP. */
cmd->cmd.tx.tx_flags |= TX_CMD_FLG_ACK_MSK;

and it doesn't make any sense at all. Care to explain? If we're a
station associated to some other AP, then mac80211 will *obviously* set
the "expect ack" bit on a "multicast data" packet that's being sent
since it's actually unicast in 802.11 terms to the AP before it resends
it using the group key... And if we're an AP, this is plain wrong
(pending a positive answer to the question whether iwlwifi can offload
broadcast/multicast encryption...) I suggest you remove the comment and
the tx flags manipulation.

The same comment is present in the #if 0'ed TKIP code and still talks
about CCMP. Please!

And all the crypto code does:

cmd->cmd.tx.hdr[0].frame_control |=
cpu_to_le16(IEEE80211_FCTL_PROTECTED);

This is a no-op. Remove it. And while I'm at it, let me say this once
more: Do *NOT* work around the 802.11 implementation in mac80211 in the
driver. Fix bugs where there are any, and otherwise don't do work
mac80211 has done before.

And then I see things like
cmd->cmd.tx.sec_ctl = 1 | /* WEP */
(ctl->key_idx & 0x3) << 6;

Please use proper constants for the shifts and masks like everybody
else. Can't be too hard can it?

Generally, the driver needs *LOTS* more comments. While hacking on
mac80211 it is sometimes important to know what the hardware can handle,
and I'm unable to answer my initial question using the iwlwifi code. We
know a lot more about all the chipsets we've reverse engineered and are
able to judge their capabilities MUCH better than we'll ever be able to
by looking at the iwlwifi code :(

I can only guess this is intentional. While I applaud Intel's efforts in
wireless especially in the light of how some other vendors behave, this
is suboptimal and basically requires reverse engineering too to figure
out how things work.

Oh don't get me wrong. There are some comments explaining how things
work, for example the one about DMA queues. Except that it's not even
correct:

* The four transmit queues allow for performing quality of service (qos)
* transmissions as per the 802.11 protocol. Currently Linux does not
* provide a mechanism to the user for utilizing prioritized queues, so
* we only utilize the first data transmit queue (queue1).

No? Why do we have an 802.11 qdisc then and all that stuff?

Further comments below while I'm at it.

* mac80211 should also be examined to determine if sta_info is duplicating
* the functionality provided here

absolutely. It also needs a good description of how the firmware manages
stations. And is this for AP case or just for remote APs?

In fact, come to think of it, the only thing it does to the hardware is
iwl_send_add_station so what you really want is a notification from
mac80211 when a sta_info is added/removed. And for that you build a huge
amount of station management code into the driver? That all needs to go
and new features like HT must be folded into mac80211 instead.

* NOTE: This initializes the table for a single retry per data rate
* which is not optimal. Setting up an intelligent retry per rate
* requires feedback from transmission, which isn't exposed through
* rc80211_simple which is what this driver is currently using.

cute. No comment really, speaks for itself.

iwl_is_fat_tx_allowed and similar stuff needs to be in mac80211.

static struct ieee80211_ops iwl_hw_ops = {
.ht_tx_agg_start = iwl_mac_ht_tx_agg_start,
.ht_tx_agg_stop = iwl_mac_ht_tx_agg_stop,
.ht_rx_agg_start = iwl_mac_ht_rx_agg_start,
.ht_rx_agg_stop = iwl_mac_ht_rx_agg_stop,

That doesn't seem to be in my mac80211? Did you actually start
implementing HT features in mac80211? Why are they not available? Why
clutter the driver with #idef CONFIG_..._HT_AGG when that cannot
possibly be defined?

/* Hard coded to start at the highest retry fallback position
* until the 4965 specific rate control algorithm is tied in */
tx->initial_rate_index = LINK_QUAL_MAX_RETRY_NUM - 1;

/* Alternate between antenna A and B for successive frames */
if (priv->use_ant_b_for_management_frame) {
priv->use_ant_b_for_management_frame = 0;
rate_flags = RATE_MCS_ANT_B_MSK;
} else {
priv->use_ant_b_for_management_frame = 1;
rate_flags = RATE_MCS_ANT_A_MSK;
}

Again, you were telling us that you absolutely need the rate control
algorithm tied in with things etc., yet you're not doing it.

iwl4965_sign_extend can be implemented a lot better like such:

static inline s32 sign_extend(u32 value, u8 bits)
{
u8 shift = 32 - bits;

return (s32)(value << shift) >> shift;
}

Note the renamed variables.

struct ieee80211_rx_status stats = {
.mactime = le32_to_cpu(rx_start->beacon_time_stamp),

That beacon_time_start variable looks rather misnamed. Or it shouldn't
be used here.

Enough for now again. I suggest when you run into problems like the rate
control algorithm not working well or HT mode missing, you start by
describing the hardware functionality and the resulting software problem
to linux-wireless so we can give advice on how to best implement things,
instead of stuffing tons of code into the driver and then pushing it out
as "hey this works can it be merged?"

Thanks,
johannes

Attachments:

signature.asc (190.00 B)
This is a digitally signed message part

2007-09-05 18:41:47

by Inaky Perez-Gonzalez

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Tuesday 04 September 2007, Ivo van Doorn wrote:
> On Tuesday 04 September 2007, dragoran wrote:
> > On 9/4/07, Ivo van Doorn <[email protected]> wrote:
> > > On Tuesday 04 September 2007, dragoran wrote:
> > > > >+static ssize_t show_rf_kill(struct device *d,
> > >
> > > Yes. It currently is only missing users.
> >
> > ok thats great ;) is the (userspace) interface defined somewhere? or
> > should I read the code to understand how it works? (would like to add
> > support to hal)
>
> There isn't a documentation file for it, so best thing to do would be looking at the code.
> basically hal only needs to check the sysfs files:
>
> name -> Name of device/interface
> type -> wlan, bluetooth, irda

Can you add 'uwb' [for ultrawideband radios]? That way I could start to use it. A
Doc file would help enourmously also :)

-- Inaky

2007-09-06 18:02:41

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Thursday 06 September 2007, Inaky Perez-Gonzalez wrote:
> On Thursday 06 September 2007, Ivo van Doorn wrote:
> > >
> > > Can you add 'uwb' [for ultrawideband radios]? That way I could start to use it. A
> > > Doc file would help enourmously also :)
> >
> > I'll create a doc file this weekend which will explain the sysfs files
> > and some instructions for drivers that want to implement it.
>
> Thanks,
>
> > I'm not familiar with ultrawideband radios, don't they fall under the same
> > catagory as wireless lan?
>
> Nope, totally different beast; see linuxuwb.org. UWB is kind of like bluetooth
> on esteroids, for PAN applications. Wireless USB (for eaxmple) uses a UWB radio.

Ah ok.
Well if there will be users, I'll create a patch to add the UWB to the list.
Patch will, just like the doc file, follow this weekend.

Ivo

2007-09-07 02:35:14

by Zhu Yi

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Wed, 2007-09-05 at 13:28 +0200, Johannes Berg wrote:
> I've been saying in every second mail
> now: run this driver against the mac80211 in net-2.6.24. It *will*
> trigger a WARN_ON() in the key handling code.

Where is the WARN_ON? I ran the iwl4965 driver with net-2.6.24 commit-id
3259203776dc4c3f99b9fe3b303e3e76b13f934d, tried WEP, CCMP, no problem,
no WARN_ON.

Since the mac80211 in net-2.6.24 has some structure change, I modified
the iwlwifi a little bit. You can find the iwlwifi driver for davem's
net-2.6.24 from:
http://intellinuxwireless.org/iwlwifi/v6-iwlwifi-net-2.6.24.patch

Thanks,
-yi

2007-09-10 10:41:04

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Mon, 2007-09-10 at 10:09 +0800, Zhu Yi wrote:
> On Fri, 2007-09-07 at 15:40 +0200, Johannes Berg wrote:
> > But you need to have another place where this flag is set based on the
> > equivalent mac80211 flag, so this is not necessary.
>
> OK. Will use mac80211's indication if mac80211 already takes care about
> the "trick".

If not I'd consider it a bug. But I don't see a "trick" there anyhow.
What's non-standard in this behaviour? Almost every unicast packet is
expected to be acknowledged.

> > > BTW， the whole 802.11 qdisc hack will be removed with
> > > the multiqueue supported in .24.

> If nobody will work on that, we Intel will do. I remembered Tomas
> promised John in this OLS ;)

:)
Sounds good. Do you guys have contact to whoever wrote it? ISTR it came
from Intel in the first place.

> It should be 31, not 32 (shift = 31 - bits). Will use your suggestion.

Huh. /me double-checks. Am I doing this bit-calculation totally wrong? I
thought you had a "bits"-bit number that included the sign in the bit
"bits-1" (if you number 0 through "bits-1"). Then you have to shift up
by "32 - bits" so that the "bits" bits including the sign live in the
upper "bits" bits of the 32-bit number, and then shift down again by the
same amount to do the sign extension. I wonder if there's a good way to
ask gcc to do this :)

> OK. Let me remove the 11N stuff in iwlwifi for .24 and then we do
> another round of mac80211 11N patches submission/review later.

That would be much appreciated. We really also should have rate control
be able to influence that generically and lots more things that have to
be figured out but that are quite hard to work with when the 802.11n
drafts are not available to many.

> I believe
> you can do a better job than Jiri did.

Thanks. I'm not sure I can live up to that though; for example I'll have
to mostly disappear for the next 10 days or so to prepare for an exam.
I'll be around, but not patching quite as much ;)

> I thought John would take my .24 iwlwifi patch to wireless-dev GIT
> directly since he just rebased the tree (at that time) and all the
> commits for iwlwifi were merged into one big patch.

Ah ok.

> Anyway, I will
> repost it explicitly for wireless-dev next time: iwlwifi driver + HT
> features for the iwlwifi (or everything) branch; and iwlwifi driver
> without HT features for pending-upstream branch. We can work on iwlwifi
> driver to make it in .24 first and sort out HT things in .25.

Sounds good. I'll take another look then.

Thanks,
johannes

Attachments:

signature.asc (190.00 B)
This is a digitally signed message part

2007-09-10 02:16:14

by Zhu Yi

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Fri, 2007-09-07 at 15:40 +0200, Johannes Berg wrote:
> But you need to have another place where this flag is set based on th=
e
> equivalent mac80211 flag, so this is not necessary.

OK. Will use mac80211's indication if mac80211 already takes care about
the "trick".

> > BTW=A3=AC the whole 802.11 qdisc hack will be removed with
> > the multiqueue supported in .24. =20
>=20
> Yeah, is anybody working on that? I have to admit I didn't look into
> that yet.

If nobody will work on that, we Intel will do. I remembered Tomas
promised John in this OLS ;)

> > We have the HT AGG support in our mac80211. Will submit the patch f=
or
> > review when we are ready.
>=20
> Yeah I gathered that much from what Tomas said, but why do you keep
> pushing for including this driver when you know it's not the right th=
ing
> to do?
>=20
> > > iwl4965_sign_extend can be implemented a lot better like such:
> > >=20
> > > static inline s32 sign_extend(u32 value, u8 bits)
> > > {
> > > u8 shift =3D 32 - bits;
> > >=20
> > > return (s32)(value << shift) >> shift;
> > > }
> >=20
> > Really? Are you sure this work?
>=20
> Hmm? Of course, shift it up and then do a signed shift down.

It should be 31, not 32 (shift =3D 31 - bits). Will use your suggestion=
=2E

> You still haven't fixed the issues we pointed out in the 11N
> deaggregation code. And maybe we missed some things too. Also, I
> personally only recently started working on mac80211 and Jiri seems t=
o
> have vanished completely.

OK. Let me remove the 11N stuff in iwlwifi for .24 and then we do
another round of mac80211 11N patches submission/review later. I believ=
e
you can do a better job than Jiri did.

> In any case, let's get back to more productive things. I apologise fo=
r
> anything I might have said that offended you, I didn't mean to. All t=
he
> code that superficially looks like workarounds around mac80211 stuff
> made me not like the code and I suppose that showed.
>=20
> Can you post patches against the iwlwifi currently included in
> wireless-dev? Currently, it seems that I have to either dig in your
> repository or through your patches to figure out the current state of
> things I commented on, which is suboptimal because I don't even get t=
o
> see the fixes to things I pointed out. I'd like to have iwlwifi in
> wireless-dev be essentially the same as is merged into mainline, modu=
lo
> the bits that depend on things like HT deaggregation that are not in
> mainline yet.

I thought John would take my .24 iwlwifi patch to wireless-dev GIT
directly since he just rebased the tree (at that time) and all the
commits for iwlwifi were merged into one big patch. Anyway, I will
repost it explicitly for wireless-dev next time: iwlwifi driver + HT
features for the iwlwifi (or everything) branch; and iwlwifi driver
without HT features for pending-upstream branch. We can work on iwlwifi
driver to make it in .24 first and sort out HT things in .25.

> Incidentally, I currently have just about as much a hard time getting
> things changed in mac80211 as you do because nobody wants to review t=
hat
> code. Hence, it would even help if you could review the patches I pos=
ted
> and maybe look if that is easy/possible to support with iwlwifi and
> possibly even adapt iwlwifi and test so that John feels more confiden=
t
> in merging patches. Conversely, I promise I'll review and test any
> patches you make to mac80211 as time permits.

I totally agree with you.

Thanks,
-yi

2007-09-05 01:44:15

by Zhu Yi

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Tue, Sep 04, 2007 at 04:04:11PM +0200, Johannes Berg wrote:
> On Tue, 2007-09-04 at 11:04 +0800, Zhu Yi wrote:
>
> > Note, the #include "../net/mac80211/ieee80211_rate.h" still existed in
> > this version of the driver. We are discussing how to abstract the
> > mac80211 rate scaling interface in another thread in this ML.
>
> You also haven't addressed any of the other points I raised wrt.
> encryption [please try running your driver on the latest
> mac80211/net-2.6.24 tree, it'll complain very loudly], looking into
> frames, using the configured MAC address instead of the programmed one,
> oopsing if pure monitor mode is requested,

Do you mean this?

> ian (1):
> avoid kernel oops in monitor mode with debug enabled

For your other comments, I agree they are valid and we will address
them. But I don't think any of them are a block issue for .24, right?
The driver works well for most users, I think we can push it into
mainline for better testing and fix them slowly (keep stability) by
.25 time.

Thanks,
-yi

2007-09-04 15:55:18

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Tue, Sep 04, 2007 at 11:04:17AM +0800, Zhu Yi wrote:
> In this version, we replace the "comiple iwl-base.c twice for two
> drivers" scheme with separating the two drivers into two code base. In
> the future, we will abstract a common layer for all the iwl based
> drivers and make it a separate module for better maintenance.

Nice. But please also get rid of the Makefile hacks added for this now
that they're not required anymore.

2007-09-04 14:02:57

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Tue, 2007-09-04 at 11:04 +0800, Zhu Yi wrote:

> Note, the #include "../net/mac80211/ieee80211_rate.h" still existed in
> this version of the driver. We are discussing how to abstract the
> mac80211 rate scaling interface in another thread in this ML.

You also haven't addressed any of the other points I raised wrt.
encryption [please try running your driver on the latest
mac80211/net-2.6.24 tree, it'll complain very loudly], looking into
frames, using the configured MAC address instead of the programmed one,
oopsing if pure monitor mode is requested, doing frame aggregation and
all the others.

I think this repost is mostly useless for review, I applaud that you're
working on things but don't see why we'd be interested in this step.

johannes

Attachments:

signature.asc (190.00 B)
This is a digitally signed message part

2007-09-07 06:38:07

by Zhu Yi

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Thu, 2007-09-06 at 13:00 +0200, Johannes Berg wrote:
> I was just looking through the iwlwifi sources to find out whether it
> can encrypt with a key even if the packet isn't unicast... Anybody kn=
ow?
> I myself removed the ability to upload multicast/broadcast keys becau=
se
> it was so unclear, but it seems that maybe the firmware knows about s=
ome
> sort of "virtual broadcast/multicast" station? How about multiple
> broadcast keys for APs with VLANs?
>=20
> In any case, I found this comment:
>=20
> /* XXX: ACK flag must be set for CCMP even if it
> * is a multicast/broadcast packet, because CCMP
> * group communication encrypted by GTK is
> * actually done by the AP. */
> cmd->cmd.tx.tx_flags |=3D TX_CMD_FLG_ACK_MSK;
>=20
> and it doesn't make any sense at all. Care to explain? If we're a
> station associated to some other AP, then mac80211 will *obviously* s=
et
> the "expect ack" bit on a "multicast data" packet that's being sent
> since it's actually unicast in 802.11 terms to the AP before it resen=
ds
> it using the group key...=20

This is what exactly the above comment tells you. The comment speaks fo=
r
the non-AP STA mode (the AP code was not there at that time). The
tx_flags is used by the firmware, so whatever mac80211 set its flag, it
has to be translated to the firmware's language -- because you are usin=
g
hardware crypto.

> And if we're an AP, this is plain wrong
> (pending a positive answer to the question whether iwlwifi can offloa=
d
> broadcast/multicast encryption...) I suggest you remove the comment a=
nd
> the tx flags manipulation.

The comment is not for AP mode.

> The same comment is present in the #if 0'ed TKIP code and still talks
> about CCMP. Please!

iwlwifi hardware/firmware only supports partial TKIP (ie. it can do [en=
|
de]crypt, but cannot build michael_mic), so we comment out the TKIP
hwcrypto code temporarily at this time. I agree the comment should run =
a
's/CCMP/TKIP/g' though.

> And all the crypto code does:
>=20
> cmd->cmd.tx.hdr[0].frame_control |=3D
> cpu_to_le16(IEEE80211_FCTL_PROTECTED);
>=20
> This is a no-op. Remove it. And while I'm at it, let me say this once
> more: Do *NOT* work around the 802.11 implementation in mac80211 in t=
he
> driver. Fix bugs where there are any, and otherwise don't do work
> mac80211 has done before.

It might be removed, but let me do some test (especially for EAP
packets) first.

> And then I see things like
> cmd->cmd.tx.sec_ctl =3D 1 | /* WEP */
> (ctl->key_idx & 0x3) << 6;
>=20
> Please use proper constants for the shifts and masks like everybody
> else. Can't be too hard can it?

OK.

> Generally, the driver needs *LOTS* more comments. While hacking on
> mac80211 it is sometimes important to know what the hardware can hand=
le,
> and I'm unable to answer my initial question using the iwlwifi code. =
We
> know a lot more about all the chipsets we've reverse engineered and a=
re
> able to judge their capabilities MUCH better than we'll ever be able =
to
> by looking at the iwlwifi code :(

It is obviously clear, both in the comment and the code: when you get a
GTK, you have already got a PTK anyway. In the non-AP STA mode, you
encypt your packets with PTK (whatever for unicast, broadcast), the AP
receives it and rencrypts them with GTK. Your GTK is used to decrypt fo=
r
multicast, broadcast packets.

> I can only guess this is intentional. While I applaud Intel's efforts=
in
> wireless especially in the light of how some other vendors behave, th=
is
> is suboptimal and basically requires reverse engineering too to figur=
e
> out how things work.

Intentional for what? When you are not able to follow a piece of code,
you think the author intentional mislead you, instead of blaming your
own capability? Arrogant!

> Oh don't get me wrong. There are some comments explaining how things
> work, for example the one about DMA queues. Except that it's not even
> correct:
>=20
> * The four transmit queues allow for performing quality of service (=
qos)
> * transmissions as per the 802.11 protocol. Currently Linux does no=
t
> * provide a mechanism to the user for utilizing prioritized queues, =
so
> * we only utilize the first data transmit queue (queue1).
>=20
> No? Why do we have an 802.11 qdisc then and all that stuff?

iwlwifi _does_ honor the ieee80211_tx_control->queue field, I'll delete
the old comment. BTW=A3=AC the whole 802.11 qdisc hack will be removed =
with
the multiqueue supported in .24. =20

> Further comments below while I'm at it.
>=20
> * mac80211 should also be examined to determine if sta_info is dupli=
cating
> * the functionality provided here
>=20
> absolutely. It also needs a good description of how the firmware mana=
ges
> stations. And is this for AP case or just for remote APs?
>=20
> In fact, come to think of it, the only thing it does to the hardware =
is
> iwl_send_add_station so what you really want is a notification from
> mac80211 when a sta_info is added/removed. And for that you build a h=
uge
> amount of station management code into the driver? That all needs to =
go
> and new features like HT must be folded into mac80211 instead.
>=20
> * NOTE: This initializes the table for a single retry per data rate
> * which is not optimal. Setting up an intelligent retry per rate =20
> * requires feedback from transmission, which isn't exposed through=20
> * rc80211_simple which is what this driver is currently using.
>=20
> cute. No comment really, speaks for itself.
>=20
> iwl_is_fat_tx_allowed and similar stuff needs to be in mac80211.
>=20
> static struct ieee80211_ops iwl_hw_ops =3D {
> .ht_tx_agg_start =3D iwl_mac_ht_tx_agg_start,
> .ht_tx_agg_stop =3D iwl_mac_ht_tx_agg_stop,
> .ht_rx_agg_start =3D iwl_mac_ht_rx_agg_start,
> .ht_rx_agg_stop =3D iwl_mac_ht_rx_agg_stop,
>=20
> That doesn't seem to be in my mac80211? Did you actually start
> implementing HT features in mac80211? Why are they not available? Why
> clutter the driver with #idef CONFIG_..._HT_AGG when that cannot
> possibly be defined?

We have the HT AGG support in our mac80211. Will submit the patch for
review when we are ready.

> /* Hard coded to start at the highest retry fallback position
> * until the 4965 specific rate control algorithm is tied in =
*/
> tx->initial_rate_index =3D LINK_QUAL_MAX_RETRY_NUM - 1;
>=20
> /* Alternate between antenna A and B for successive frames */
> if (priv->use_ant_b_for_management_frame) {
> priv->use_ant_b_for_management_frame =3D 0;
> rate_flags =3D RATE_MCS_ANT_B_MSK;
> } else {
> priv->use_ant_b_for_management_frame =3D 1;
> rate_flags =3D RATE_MCS_ANT_A_MSK;
> }
>=20
> Again, you were telling us that you absolutely need the rate control
> algorithm tied in with things etc., yet you're not doing it.

I have sent a proposal to reorganize the header files so that we can
export a rate scale interface to the drivers. It is not perfect but it
works. Then we don't like it here and there. If you have a better idea,
why don't you speak it out?

> iwl4965_sign_extend can be implemented a lot better like such:
>=20
> static inline s32 sign_extend(u32 value, u8 bits)
> {
> u8 shift =3D 32 - bits;
>=20
> return (s32)(value << shift) >> shift;
> }

Really? Are you sure this work?

> Note the renamed variables.
>=20
> struct ieee80211_rx_status stats =3D {
> .mactime =3D le32_to_cpu(rx_start->beacon_time_stamp)=
,
>=20
> That beacon_time_start variable looks rather misnamed. Or it shouldn'=
t
> be used here.

OK. I assuem you mentioned the beacon_time_stamp, it should be
->timestamp.

> Enough for now again.=20

Please don't. We have given you enough time for commenting the code.
Please do it in a batch like other people did. Don't squeeze a little i=
n
-rc2 and another little in -rc4, ... You are wasting people's time.

> I suggest when you run into problems like the rate
> control algorithm not working well or HT mode missing, you start by
> describing the hardware functionality and the resulting software prob=
lem
> to linux-wireless so we can give advice on how to best implement thin=
gs,
> instead of stuffing tons of code into the driver and then pushing it =
out

James had done this several months ago. We did everything the mac80211
people suggested, but the patch is still not merged. Do you think we'd
really like to workaound the mac80211 issues in the driver, or even hav=
e
our own mac80211 package? Absolutely not, if you mac80211 guys are easy
to work with. Without creating our own mac80211 package, today, the
users are still no better rate scale algorithm selection, no advanced
wireless QoS, no 11N support, not even mention about link aggregation.

> as "hey this works can it be merged?"

Something working is always better than something not working whatever
how beautiful the code looks like. And I believe the current iwlwifi
driver has reached the quality to be merged mainline (both it looks and
it works).

Thanks,
-yi

2007-09-11 17:37:41

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On 9/11/07, Johannes Berg <[email protected]> wrote:
> On Mon, 2007-09-10 at 17:20 +0300, Tomas Winkler wrote:
>
> > It is my best intention yet first we need native interface which make
> > in turn problems in eb tables. So it is a long shot.
>
> Ok. We've managed long enough without it so I guess we can wait :)
>
> > I wouldn't appreciate this at all. 11n is major feature of our NIC.
> > Major obstacle in finally pushing 11n is constant code base change of
> > iwlwifi. This is already 4th code base. The latest was because the
> > driver didn't look nice enough, what an engineering reason! In the
> > bottom line we are hunting our own tail for wrong reasons
>
> Well, I believe that there are bad layering violations in your current
> driver, namely looking at the packets mac80211 sends, doing 11N
> manipulations and everything in the driver and duplicating the sta_info
> stuff because mac80211 happens to be missing a few hooks. If you think
> those are "wrong reasons" that's fine with me.

There is no doubt that what you currently see is ugly, we reworked the
code and added these few hooks the problem is that they don't apply
anymore to recent code, since I'm stabilizing basic flows after each
code base in the middle of development.
I know I'm not 'release often' complained but I really test the code
that I'm publishing, it takes time.
I prefer to have stable driver and fix it step by step then releasing
30 patches that nobody have time to consume and review in addition it
just kills the functionality. Don't know maybe I'm too old for Linux
:)

>
> Personally, I'm just raising these points and marking them down as
> "against merging as-is". If others don't care about them, that's ok with
> me.
>
After all it is functional driver. It gets ~60Mps in TCP even more.

> johannes
>
>

2007-09-10 14:20:06

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On 9/10/07, Johannes Berg <[email protected]> wrote:
> On Mon, 2007-09-10 at 10:09 +0800, Zhu Yi wrote:
> > On Fri, 2007-09-07 at 15:40 +0200, Johannes Berg wrote:
> > > But you need to have another place where this flag is set based on the
> > > equivalent mac80211 flag, so this is not necessary.
> >
> > OK. Will use mac80211's indication if mac80211 already takes care about
> > the "trick".
>
> If not I'd consider it a bug. But I don't see a "trick" there anyhow.
> What's non-standard in this behaviour? Almost every unicast packet is
> expected to be acknowledged.
>
> > > > BTW$B!$(B the whole 802.11 qdisc hack will be removed with
> > > > the multiqueue supported in .24.
>
> > If nobody will work on that, we Intel will do. I remembered Tomas
> > promised John in this OLS ;)
>
> :)
> Sounds good. Do you guys have contact to whoever wrote it? ISTR it came
> from Intel in the first place.
>
It is my best intention yet first we need native interface which make
in turn problems in eb tables. So it is a long shot.

> > It should be 31, not 32 (shift = 31 - bits). Will use your suggestion.
>
> Huh. /me double-checks. Am I doing this bit-calculation totally wrong? I
> thought you had a "bits"-bit number that included the sign in the bit
> "bits-1" (if you number 0 through "bits-1"). Then you have to shift up
> by "32 - bits" so that the "bits" bits including the sign live in the
> upper "bits" bits of the 32-bit number, and then shift down again by the
> same amount to do the sign extension. I wonder if there's a good way to
> ask gcc to do this :)
>
> > OK. Let me remove the 11N stuff in iwlwifi for .24 and then we do
> > another round of mac80211 11N patches submission/review later.
>
> That would be much appreciated. We really also should have rate control
> be able to influence that generically and lots more things that have to
> be figured out but that are quite hard to work with when the 802.11n
> drafts are not available to many.

I wouldn't appreciate this at all. 11n is major feature of our NIC.
Major obstacle in finally pushing 11n is constant code base change of
iwlwifi. This is already 4th code base. The latest was because the
driver didn't look nice enough, what an engineering reason! In the
bottom line we are hunting our own tail for wrong reasons

> > I believe
> > you can do a better job than Jiri did.
>
> Thanks. I'm not sure I can live up to that though; for example I'll have
> to mostly disappear for the next 10 days or so to prepare for an exam.
> I'll be around, but not patching quite as much ;)
>
> > I thought John would take my .24 iwlwifi patch to wireless-dev GIT
> > directly since he just rebased the tree (at that time) and all the
> > commits for iwlwifi were merged into one big patch.
>
> Ah ok.
>
> > Anyway, I will
> > repost it explicitly for wireless-dev next time: iwlwifi driver + HT
> > features for the iwlwifi (or everything) branch; and iwlwifi driver
> > without HT features for pending-upstream branch. We can work on iwlwifi
> > driver to make it in .24 first and sort out HT things in .25.
>
> Sounds good. I'll take another look then.
>
> Thanks,
> johannes
>
>

2007-09-04 21:08:44

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Tuesday 04 September 2007, dragoran wrote:
> On 9/4/07, Ivo van Doorn <[email protected]> wrote:
> > On Tuesday 04 September 2007, dragoran wrote:
> > > On 9/4/07, Ivo van Doorn <[email protected]> wrote:
> > > > On Tuesday 04 September 2007, dragoran wrote:
> > > > > >+static ssize_t show_rf_kill(struct device *d,
> > > > > >+ struct device_attribute *attr, char *buf)
> > > > > >+{
> > > > > >+ /*
> > > > > >+ * 0 - RF kill not enabled
> > > > > >+ * 1 - SW based RF kill active (sysfs)
> > > > > >+ * 2 - HW based RF kill active
> > > > > >+ * 3 - Both HW and SW based RF kill active
> > > > > >
> > > > > >that as well, along with all the other sysfs bits. Also, how about using
> > > > > >the generic rfkill infrastructure Ivo did?
> > > > >
> > > > > is the generic rfkill interface already stable and merged into the linus tree?
> > > >
> > > > Yes. It currently is only missing users.
> > >
> > > ok thats great ;) is the (userspace) interface defined somewhere? or
> > > should I read the code to understand how it works? (would like to add
> > > support to hal)
> >
> > There isn't a documentation file for it, so best thing to do would be looking at the code.
> > basically hal only needs to check the sysfs files:
> >
> > name -> Name of device/interface
> > type -> wlan, bluetooth, irda
> > state -> Current device state. 0: Off, 1: On
> > claim -> 0: Kernel handles events, 1: Userspace handles events
> >
> > "name" and "type" are read-only
> > "claim" and "state" are read/writable
> >
> > Note that there is a bug in 2.6.22 which causes the "state" file to be read-only,
> > this has been fixed in 2.6.23-rc.
>
> ok that isn't complicated what is the claim used for?
> does It has to be set to userspace to be able to toggle the status via software?

By default the kernel will act when the rfkill button is pressed, it will loop
through all registered buttons of the same type and change the state.
Userspace can read the "state" sysfs file to see the current status,
but if a userspace tools wants to take control of taking action when the button is pressed,
or at least doesn't want the kernel to do anything, 1 should be written to "claim" to
tell the kernel that it should back off and ignore all events.

Ivo

2007-09-04 19:08:51

by drago01

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On 9/4/07, Ivo van Doorn <[email protected]> wrote:
> On Tuesday 04 September 2007, dragoran wrote:
> > On 9/4/07, Ivo van Doorn <[email protected]> wrote:
> > > On Tuesday 04 September 2007, dragoran wrote:
> > > > >+static ssize_t show_rf_kill(struct device *d,
> > > > >+ struct device_attribute *attr, char *buf)
> > > > >+{
> > > > >+ /*
> > > > >+ * 0 - RF kill not enabled
> > > > >+ * 1 - SW based RF kill active (sysfs)
> > > > >+ * 2 - HW based RF kill active
> > > > >+ * 3 - Both HW and SW based RF kill active
> > > > >
> > > > >that as well, along with all the other sysfs bits. Also, how about using
> > > > >the generic rfkill infrastructure Ivo did?
> > > >
> > > > is the generic rfkill interface already stable and merged into the linus tree?
> > >
> > > Yes. It currently is only missing users.
> >
> > ok thats great ;) is the (userspace) interface defined somewhere? or
> > should I read the code to understand how it works? (would like to add
> > support to hal)
>
> There isn't a documentation file for it, so best thing to do would be looking at the code.
> basically hal only needs to check the sysfs files:
>
> name -> Name of device/interface
> type -> wlan, bluetooth, irda
> state -> Current device state. 0: Off, 1: On
> claim -> 0: Kernel handles events, 1: Userspace handles events
>
> "name" and "type" are read-only
> "claim" and "state" are read/writable
>
> Note that there is a bug in 2.6.22 which causes the "state" file to be read-only,
> this has been fixed in 2.6.23-rc.

ok that isn't complicated what is the claim used for?
does It has to be set to userspace to be able to toggle the status via software?

> > shouldn't the new interface send events so that polling isn't required?
>
> Not really, rfkill was intended for a generic interface, which would make
> the rfkill buttons work with or without intervention of userspace. So polling
> is required unless the hardware generates interrupts when the button is called.

ok

2007-09-04 16:33:37

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

Some more things.

+ case IEEE80211_FTYPE_DATA:
+ if (unlikely(is_duplicate_packet(priv, header)))
+ IWL_DEBUG_DROP("Dropping (dup): " MAC_FMT ", "
+ MAC_FMT ", " MAC_FMT "\n",
+ MAC_ARG(header->addr1),
+ MAC_ARG(header->addr2),
+ MAC_ARG(header->addr3));

mac80211 does duplicate detection.

+ if (priv->iw_mode == IEEE80211_IF_TYPE_MNTR) {
+ if (iwl_param_hwcrypto)
+ iwl_set_decrypted_flag(priv, rxb->skb,
+ ampdu_status, stats);
+ iwl_handle_data_packet_monitor(priv, rxb, hdr, len, stats, 0);
+ return;
+ }

why handle monitor mode differently? mac80211 does all that for you.

How much does the hardware do when you're in AP mode and have stations
in PS mode? You seem to do some things but it's not clear why.

+ /* FIXME: not sure why this doesn't work in AP mode */
+ if (priv->iw_mode != IEEE80211_IF_TYPE_AP)
+ iwl_sequence_reset(priv);

Now, I'd understand that in a reverse engineered driver....

+static int iwl_mac_stop(struct ieee80211_hw *hw)
+{
+ struct iwl_priv *priv = hw->priv;
+
+ IWL_DEBUG_MAC80211("enter\n");
+ priv->is_open = 0;
+ /*netif_stop_queue(dev); */

eh? dead code in comments isn't nice especially if it's wrong.

+ if (priv->iw_mode == IEEE80211_IF_TYPE_MNTR) {
+ IWL_DEBUG_MAC80211("leave - monitor\n");
+ return -1;
+ }

Andy will hate you for that. And I see no point.

+ if (priv->interface_id) {
+ IWL_DEBUG_MAC80211("leave - interface_id != 0\n");
+ return 0;
+ }

I don't think you should return 0 for an error case, when userspace
tries to add multiple you just accept them?

+ * We ignore conf->flags & IEEE80211_CONF_SHORT_SLOT_TIME since it seems to
+ * be set inappropriately and the driver currently sets the hardware up to
+ * use it whenever needed.

would be nice to figure out if there is a bug and then fix it. I haven't
noticed one.

+ /* TODO: Figure out how to get ieee80211_local->sta_scanning w/ only
+ * what is exposed through include/ declrations */
+ if (unlikely(!iwl_param_disable_hw_scan &&
+ test_bit(STATUS_SCANNING, &priv->status))) {
+ IWL_DEBUG_MAC80211("leave - scanning\n");
+ mutex_unlock(&priv->mutex);
+ return 0;
+ }

Uh huh? Is that more workarounds for the hardware scanning?

+ iwl_set_rxon_hwcrypto(priv, 1);
+ iwl_commit_rxon(priv);
+ key->flags &= ~IEEE80211_KEY_FORCE_SW_ENCRYPT;
+ key->hw_key_idx = sta_id;
+ IWL_DEBUG_MAC80211("set_key success, using hwcrypto\n");

Since you're not going to be able to push the driver into .23 and .24
will include mac80211 updates, how about making it compile with those
updates? Hint: take patches from wireless-dev.

+ * The following adds a new attribute to the sysfs representation
+ * of this device driver (i.e. a new file in /sys/bus/pci/drivers/ipw/)
+ * used for controlling the debug level.
+ *
+ * See the level definitions in ipw for details.

That interface should likely live in debugfs.

+static ssize_t show_rf_kill(struct device *d,
+ struct device_attribute *attr, char *buf)
+{
+ /*
+ * 0 - RF kill not enabled
+ * 1 - SW based RF kill active (sysfs)
+ * 2 - HW based RF kill active
+ * 3 - Both HW and SW based RF kill active

that as well, along with all the other sysfs bits. Also, how about using
the generic rfkill infrastructure Ivo did?

+ /* The following it a temporary work around due to the
+ * suspend / resume not fully initializing the NIC correctly.

"temporary" until when?

+ if (unlikely(!network_packet))
+ IWL_DEBUG_DROP("Dropping (non network): "
+ MAC_FMT ", " MAC_FMT ", "
+ MAC_FMT "\n",
+ MAC_ARG(header->addr1),
+ MAC_ARG(header->addr2),
+ MAC_ARG(header->addr3));

Andy will hate that as well. I wonder if his stuff actually works?

Uh. Enough for now.

johannes

Attachments:

signature.asc (190.00 B)
This is a digitally signed message part

2007-09-11 10:22:14

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Mon, 2007-09-10 at 17:20 +0300, Tomas Winkler wrote:

> It is my best intention yet first we need native interface which make
> in turn problems in eb tables. So it is a long shot.

Ok. We've managed long enough without it so I guess we can wait :)

> I wouldn't appreciate this at all. 11n is major feature of our NIC.
> Major obstacle in finally pushing 11n is constant code base change of
> iwlwifi. This is already 4th code base. The latest was because the
> driver didn't look nice enough, what an engineering reason! In the
> bottom line we are hunting our own tail for wrong reasons

Well, I believe that there are bad layering violations in your current
driver, namely looking at the packets mac80211 sends, doing 11N
manipulations and everything in the driver and duplicating the sta_info
stuff because mac80211 happens to be missing a few hooks. If you think
those are "wrong reasons" that's fine with me.

Personally, I'm just raising these points and marking them down as
"against merging as-is". If others don't care about them, that's ok with
me.

johannes

Attachments:

signature.asc (190.00 B)
This is a digitally signed message part

2007-09-04 18:48:19

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Tuesday 04 September 2007, dragoran wrote:
> On 9/4/07, Ivo van Doorn <[email protected]> wrote:
> > On Tuesday 04 September 2007, dragoran wrote:
> > > >+static ssize_t show_rf_kill(struct device *d,
> > > >+ struct device_attribute *attr, char *buf)
> > > >+{
> > > >+ /*
> > > >+ * 0 - RF kill not enabled
> > > >+ * 1 - SW based RF kill active (sysfs)
> > > >+ * 2 - HW based RF kill active
> > > >+ * 3 - Both HW and SW based RF kill active
> > > >
> > > >that as well, along with all the other sysfs bits. Also, how about using
> > > >the generic rfkill infrastructure Ivo did?
> > >
> > > is the generic rfkill interface already stable and merged into the linus tree?
> >
> > Yes. It currently is only missing users.
>
> ok thats great ;) is the (userspace) interface defined somewhere? or
> should I read the code to understand how it works? (would like to add
> support to hal)

There isn't a documentation file for it, so best thing to do would be looking at the code.
basically hal only needs to check the sysfs files:

name -> Name of device/interface
type -> wlan, bluetooth, irda
state -> Current device state. 0: Off, 1: On
claim -> 0: Kernel handles events, 1: Userspace handles events

"name" and "type" are read-only
"claim" and "state" are read/writable

Note that there is a bug in 2.6.22 which causes the "state" file to be read-only,
this has been fixed in 2.6.23-rc.

> > > if not please leave this for now to not break userspace (hal).
> > > hal currently is using this on all ipw* and iwl* drivers to get and
> > > set the rfkill status. And NM uses this interface to set/get rfkill
> > > status.
> > >
> > > So please don't remove this yet until the proper interface is merged
> > > too (which should be better anyway because this one requires polling)
> >
> > The input-polldev interface could be used for polling if it is required.
>
> shouldn't the new interface send events so that polling isn't required?

Not really, rfkill was intended for a generic interface, which would make
the rfkill buttons work with or without intervention of userspace. So polling
is required unless the hardware generates interrupts when the button is called.

Ivo

2007-09-04 17:57:56

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

>

> Uh huh? Is that more workarounds for the hardware scanning?
>
> + iwl_set_rxon_hwcrypto(priv, 1);
> + iwl_commit_rxon(priv);
> + key->flags &= ~IEEE80211_KEY_FORCE_SW_ENCRYPT;
> + key->hw_key_idx = sta_id;
> + IWL_DEBUG_MAC80211("set_key success, using hwcrypto\n");
>
> Since you're not going to be able to push the driver into .23 and .24
> will include mac80211 updates, how about making it compile with those
> updates? Hint: take patches from wireless-dev.
>
> + * The following adds a new attribute to the sysfs representation
> + * of this device driver (i.e. a new file in /sys/bus/pci/drivers/ipw/)
> + * used for controlling the debug level.
> + *
> + * See the level definitions in ipw for details.
>

> That interface should likely live in debugfs.
>
On the way as well

>
> Uh. Enough for now.
>
> johannes
>
>

2007-09-11 20:55:11

by Michael Büsch

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Tuesday 11 September 2007 19:37:40 Tomas Winkler wrote:
> On 9/11/07, Johannes Berg <[email protected]> wrote:
> > On Mon, 2007-09-10 at 17:20 +0300, Tomas Winkler wrote:
> >
> > > It is my best intention yet first we need native interface which make
> > > in turn problems in eb tables. So it is a long shot.
> >
> > Ok. We've managed long enough without it so I guess we can wait :)
> >
> > > I wouldn't appreciate this at all. 11n is major feature of our NIC.
> > > Major obstacle in finally pushing 11n is constant code base change of
> > > iwlwifi. This is already 4th code base. The latest was because the
> > > driver didn't look nice enough, what an engineering reason! In the
> > > bottom line we are hunting our own tail for wrong reasons
> >
> > Well, I believe that there are bad layering violations in your current
> > driver, namely looking at the packets mac80211 sends, doing 11N
> > manipulations and everything in the driver and duplicating the sta_info
> > stuff because mac80211 happens to be missing a few hooks. If you think
> > those are "wrong reasons" that's fine with me.
>
> There is no doubt that what you currently see is ugly, we reworked the
> code and added these few hooks the problem is that they don't apply
> anymore to recent code, since I'm stabilizing basic flows after each
> code base in the middle of development.
> I know I'm not 'release often' complained but I really test the code
> that I'm publishing, it takes time.
> I prefer to have stable driver and fix it step by step then releasing
> 30 patches that nobody have time to consume and review in addition it
> just kills the functionality. Don't know maybe I'm too old for Linux
> :)

Well, if you post the patches to get included into wireless-dev,
people will start testing them. So they test them im parallel
with you (you will do your testing of course as well).
Of course you do some basic testing before releasing the patches,
such as "does it compile" or "does the card basically still work".
But with this you get _more_ review and testing. That's the whole
point of "release early and often".
Testing early-released patches is SMP, while testing alone is UP. ;)

> > Personally, I'm just raising these points and marking them down as
> > "against merging as-is". If others don't care about them, that's ok with
> > me.
> >
> After all it is functional driver. It gets ~60Mps in TCP even more.

Well, lots of other functional stuff was rejected, because it was
simply ugly and a maintanance hell. (I'm not saying that your driver
is such a thing. I didn't look at it, yet).

--
Greetings Michael.

2007-09-04 18:18:58

by drago01

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On 9/4/07, Ivo van Doorn <[email protected]> wrote:
> On Tuesday 04 September 2007, dragoran wrote:
> > >+static ssize_t show_rf_kill(struct device *d,
> > >+ struct device_attribute *attr, char *buf)
> > >+{
> > >+ /*
> > >+ * 0 - RF kill not enabled
> > >+ * 1 - SW based RF kill active (sysfs)
> > >+ * 2 - HW based RF kill active
> > >+ * 3 - Both HW and SW based RF kill active
> > >
> > >that as well, along with all the other sysfs bits. Also, how about using
> > >the generic rfkill infrastructure Ivo did?
> >
> > is the generic rfkill interface already stable and merged into the linus tree?
>
> Yes. It currently is only missing users.

ok thats great ;) is the (userspace) interface defined somewhere? or
should I read the code to understand how it works? (would like to add
support to hal)

> > if not please leave this for now to not break userspace (hal).
> > hal currently is using this on all ipw* and iwl* drivers to get and
> > set the rfkill status. And NM uses this interface to set/get rfkill
> > status.
> >
> > So please don't remove this yet until the proper interface is merged
> > too (which should be better anyway because this one requires polling)
>
> The input-polldev interface could be used for polling if it is required.

shouldn't the new interface send events so that polling isn't required?

2007-09-04 18:05:40

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Tuesday 04 September 2007, dragoran wrote:
> >+static ssize_t show_rf_kill(struct device *d,
> >+ struct device_attribute *attr, char *buf)
> >+{
> >+ /*
> >+ * 0 - RF kill not enabled
> >+ * 1 - SW based RF kill active (sysfs)
> >+ * 2 - HW based RF kill active
> >+ * 3 - Both HW and SW based RF kill active
> >
> >that as well, along with all the other sysfs bits. Also, how about using
> >the generic rfkill infrastructure Ivo did?
>
> is the generic rfkill interface already stable and merged into the linus tree?

Yes. It currently is only missing users.

> if not please leave this for now to not break userspace (hal).
> hal currently is using this on all ipw* and iwl* drivers to get and
> set the rfkill status. And NM uses this interface to set/get rfkill
> status.
>
> So please don't remove this yet until the proper interface is merged
> too (which should be better anyway because this one requires polling)

The input-polldev interface could be used for polling if it is required.

Ivo

2007-09-07 13:39:07

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Fri, 2007-09-07 at 14:31 +0800, Zhu Yi wrote:

> > /* XXX: ACK flag must be set for CCMP even if it
> > * is a multicast/broadcast packet, because CCMP
> > * group communication encrypted by GTK is
> > * actually done by the AP. */
> > cmd->cmd.tx.tx_flags |= TX_CMD_FLG_ACK_MSK;
> >
> > and it doesn't make any sense at all. Care to explain? If we're a
> > station associated to some other AP, then mac80211 will *obviously* set
> > the "expect ack" bit on a "multicast data" packet that's being sent
> > since it's actually unicast in 802.11 terms to the AP before it resends
> > it using the group key...
>
> This is what exactly the above comment tells you. The comment speaks for
> the non-AP STA mode (the AP code was not there at that time). The
> tx_flags is used by the firmware, so whatever mac80211 set its flag, it
> has to be translated to the firmware's language -- because you are using
> hardware crypto.

But you need to have another place where this flag is set based on the
equivalent mac80211 flag, so this is not necessary.

> BTW， the whole 802.11 qdisc hack will be removed with
> the multiqueue supported in .24.

Yeah, is anybody working on that? I have to admit I didn't look into
that yet.

> We have the HT AGG support in our mac80211. Will submit the patch for
> review when we are ready.

Yeah I gathered that much from what Tomas said, but why do you keep
pushing for including this driver when you know it's not the right thing
to do?

> > iwl4965_sign_extend can be implemented a lot better like such:
> >
> > static inline s32 sign_extend(u32 value, u8 bits)
> > {
> > u8 shift = 32 - bits;
> >
> > return (s32)(value << shift) >> shift;
> > }
>
> Really? Are you sure this work?

Hmm? Of course, shift it up and then do a signed shift down.

> > Enough for now again.
>
> Please don't. We have given you enough time for commenting the code.
> Please do it in a batch like other people did. Don't squeeze a little in
> -rc2 and another little in -rc4, ... You are wasting people's time.

I just don't have enough time/energy to read through all this at once.

> James had done this several months ago. We did everything the mac80211
> people suggested, but the patch is still not merged. Do you think we'd
> really like to workaound the mac80211 issues in the driver, or even have
> our own mac80211 package? Absolutely not, if you mac80211 guys are easy
> to work with. Without creating our own mac80211 package, today, the
> users are still no better rate scale algorithm selection, no advanced
> wireless QoS, no 11N support, not even mention about link aggregation.

You still haven't fixed the issues we pointed out in the 11N
deaggregation code. And maybe we missed some things too. Also, I
personally only recently started working on mac80211 and Jiri seems to
have vanished completely.

> Something working is always better than something not working whatever
> how beautiful the code looks like. And I believe the current iwlwifi
> driver has reached the quality to be merged mainline (both it looks and
> it works).

Well, I disagree. I see no incentive for you to help improving mac80211
when we have a driver with lots of stuff that works around mac80211.

In any case, let's get back to more productive things. I apologise for
anything I might have said that offended you, I didn't mean to. All the
code that superficially looks like workarounds around mac80211 stuff
made me not like the code and I suppose that showed.

Can you post patches against the iwlwifi currently included in
wireless-dev? Currently, it seems that I have to either dig in your
repository or through your patches to figure out the current state of
things I commented on, which is suboptimal because I don't even get to
see the fixes to things I pointed out. I'd like to have iwlwifi in
wireless-dev be essentially the same as is merged into mainline, modulo
the bits that depend on things like HT deaggregation that are not in
mainline yet.

Incidentally, I currently have just about as much a hard time getting
things changed in mac80211 as you do because nobody wants to review that
code. Hence, it would even help if you could review the patches I posted
and maybe look if that is easy/possible to support with iwlwifi and
possibly even adapt iwlwifi and test so that John feels more confident
in merging patches. Conversely, I promise I'll review and test any
patches you make to mac80211 as time permits.

johannes

Attachments:

signature.asc (190.00 B)
This is a digitally signed message part

2007-09-06 16:37:30

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Wednesday 05 September 2007, Inaky Perez-Gonzalez wrote:
> On Tuesday 04 September 2007, Ivo van Doorn wrote:
> > On Tuesday 04 September 2007, dragoran wrote:
> > > On 9/4/07, Ivo van Doorn <[email protected]> wrote:
> > > > On Tuesday 04 September 2007, dragoran wrote:
> > > > > >+static ssize_t show_rf_kill(struct device *d,
> > > >
> > > > Yes. It currently is only missing users.
> > >
> > > ok thats great ;) is the (userspace) interface defined somewhere? or
> > > should I read the code to understand how it works? (would like to add
> > > support to hal)
> >
> > There isn't a documentation file for it, so best thing to do would be looking at the code.
> > basically hal only needs to check the sysfs files:
> >
> > name -> Name of device/interface
> > type -> wlan, bluetooth, irda
>
> Can you add 'uwb' [for ultrawideband radios]? That way I could start to use it. A
> Doc file would help enourmously also :)

I'll create a doc file this weekend which will explain the sysfs files
and some instructions for drivers that want to implement it.

I'm not familiar with ultrawideband radios, don't they fall under the same
catagory as wireless lan?

Ivo

2007-09-07 13:42:10

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Fri, 2007-09-07 at 10:28 +0800, Zhu Yi wrote:

> Where is the WARN_ON? I ran the iwl4965 driver with net-2.6.24 commit-id
> 3259203776dc4c3f99b9fe3b303e3e76b13f934d, tried WEP, CCMP, no problem,
> no WARN_ON.

Huh, ok, I looked again, it seems I changed the WARN_ON later to be less
severe, but there's this code in key.c:
ret = key->local->ops->set_key(local_to_hw(key->local), DISABLE_KEY,
key->sdata->dev->dev_addr, addr,
&key->conf);

if (ret)
printk(KERN_ERR "mac80211-%s: failed to remove key "
"(%d, " MAC_FMT ") from hardware (%d)\n",
wiphy_name(key->local->hw.wiphy),
key->conf.keyidx, MAC_ARG(addr), ret);

which as far as I can tell will trigger with iwlwifi when a key is
removed from the hardware acceleration. Maybe my iwlwifi is not new
enough though.

> Since the mac80211 in net-2.6.24 has some structure change, I modified
> the iwlwifi a little bit. You can find the iwlwifi driver for davem's
> net-2.6.24 from:
> http://intellinuxwireless.org/iwlwifi/v6-iwlwifi-net-2.6.24.patch

Thanks. Since wireless-dev is mostly equivalent, can you post patches
against that? I know it's suboptimal to be essentially maintaining two
drivers..

johannes

Attachments:

signature.asc (190.00 B)
This is a digitally signed message part

2007-09-05 11:27:22

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Wed, 2007-09-05 at 09:38 +0800, Zhu Yi wrote:

> For your other comments, I agree they are valid and we will address
> them. But I don't think any of them are a block issue for .24, right?

Actually, at least the crypto thing is an issue for you: it'll be a
support nightmare. Please do what I've been saying in every second mail
now: run this driver against the mac80211 in net-2.6.24. It *will*
trigger a WARN_ON() in the key handling code.

johannes

Attachments:

signature.asc (190.00 B)
This is a digitally signed message part

2007-09-04 17:56:38

by drago01

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

>+static ssize_t show_rf_kill(struct device *d,
>+ struct device_attribute *attr, char *buf)
>+{
>+ /*
>+ * 0 - RF kill not enabled
>+ * 1 - SW based RF kill active (sysfs)
>+ * 2 - HW based RF kill active
>+ * 3 - Both HW and SW based RF kill active
>
>that as well, along with all the other sysfs bits. Also, how about using
>the generic rfkill infrastructure Ivo did?

is the generic rfkill interface already stable and merged into the linus tree?
if not please leave this for now to not break userspace (hal).
hal currently is using this on all ipw* and iwl* drivers to get and
set the rfkill status. And NM uses this interface to set/get rfkill
status.

So please don't remove this yet until the proper interface is merged
too (which should be better anyway because this one requires polling)

2007-09-06 17:54:19

by Inaky Perez-Gonzalez

[permalink] [raw]

Subject: Re: [PATCH V3] Add iwlwifi wireless drivers

On Thursday 06 September 2007, Ivo van Doorn wrote:
> >
> > Can you add 'uwb' [for ultrawideband radios]? That way I could start to use it. A
> > Doc file would help enourmously also :)
>
> I'll create a doc file this weekend which will explain the sysfs files
> and some instructions for drivers that want to implement it.

Thanks,

> I'm not familiar with ultrawideband radios, don't they fall under the same
> catagory as wireless lan?

Nope, totally different beast; see linuxuwb.org. UWB is kind of like bluetooth
on esteroids, for PAN applications. Wireless USB (for eaxmple) uses a UWB radio.

-- Inaky

2007-10-01 20:38:49

by John W. Linville

[permalink] [raw]

Subject: mac80211 multiqueue

On Mon, Sep 10, 2007 at 10:09:32AM +0800, Zhu Yi wrote:
> On Fri, 2007-09-07 at 15:40 +0200, Johannes Berg wrote:

> > > BTW=EF=BC=8C the whole 802.11 qdisc hack will be removed with
> > > the multiqueue supported in .24. =20
> >=20
> > Yeah, is anybody working on that? I have to admit I didn't look int=
o
> > that yet.
>=20
> If nobody will work on that, we Intel will do. I remembered Tomas
> promised John in this OLS ;)

Indeed he did...Tomas, any progress on this?

John
--=20
John W. Linville
[email protected]

2007-10-01 23:34:47