wl12xx: third submission

wl12xx: third submission

[Kalle Valo]

Hi John,

here is the third (and hopefully last) submission of wl12xx, a driver
for TI wl1251 chipset:

http://www.valot.fi/kalle/tmp/wl12xx/20090423/

Changes since second submission:

o remove mach includes to make it compile in wireless-testing

Changes since first submission from February 10:

o update to 2.6.29 (bob)
o sysfs interface removed (bob)
o netlink interface removed
o lots of bug fixes
o platform data for proper board file support

Please consider for including the driver to wireless-testing. Bob is
planning to add SDIO support to the driver and it's easier for us to
have the driver in wireless-testing. And if all goes well, it would be
nice to get the driver into 2.6.31. But that's up to you, of course.

The diffstat:

 drivers/net/wireless/Kconfig               |    1 +
 drivers/net/wireless/Makefile              |    2 +
 drivers/net/wireless/wl12xx/Kconfig        |   11 +
 drivers/net/wireless/wl12xx/Makefile       |    4 +
 drivers/net/wireless/wl12xx/acx.c          |  689 ++++++++++++++
 drivers/net/wireless/wl12xx/acx.h          | 1245 +++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/boot.c         |  295 ++++++
 drivers/net/wireless/wl12xx/boot.h         |   40 +
 drivers/net/wireless/wl12xx/cmd.c          |  353 +++++++
 drivers/net/wireless/wl12xx/cmd.h          |  265 ++++++
 drivers/net/wireless/wl12xx/debugfs.c      |  508 ++++++++++
 drivers/net/wireless/wl12xx/debugfs.h      |   33 +
 drivers/net/wireless/wl12xx/event.c        |  127 +++
 drivers/net/wireless/wl12xx/event.h        |  121 +++
 drivers/net/wireless/wl12xx/init.c         |  200 ++++
 drivers/net/wireless/wl12xx/init.h         |   40 +
 drivers/net/wireless/wl12xx/main.c         | 1375 ++++++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/ps.c           |  151 +++
 drivers/net/wireless/wl12xx/ps.h           |   36 +
 drivers/net/wireless/wl12xx/reg.h          |  745 +++++++++++++++
 drivers/net/wireless/wl12xx/rx.c           |  208 +++++
 drivers/net/wireless/wl12xx/rx.h           |  122 +++
 drivers/net/wireless/wl12xx/spi.c          |  358 ++++++++
 drivers/net/wireless/wl12xx/spi.h          |  109 +++
 drivers/net/wireless/wl12xx/tx.c           |  557 +++++++++++
 drivers/net/wireless/wl12xx/tx.h           |  215 +++++
 drivers/net/wireless/wl12xx/wl1251.c       |  709 ++++++++++++++
 drivers/net/wireless/wl12xx/wl1251.h       |  165 ++++
 drivers/net/wireless/wl12xx/wl12xx.h       |  409 +++++++++
 drivers/net/wireless/wl12xx/wl12xx_80211.h |  156 ++++
 include/linux/spi/wl12xx.h                 |   31 +
 31 files changed, 9280 insertions(+), 0 deletions(-)

-- 
Kalle Valo

Re: wl12xx: third submission

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2957 bytes --]

Hi,

A couple of comments.

It clashes with the patches that I just posted to remove
config_interface in favour of only using bss_info_changed.

This:
        if (memcmp(wl->mac_addr, conf->mac_addr, ETH_ALEN)) {
                memcpy(wl->mac_addr, conf->mac_addr, ETH_ALEN);
                SET_IEEE80211_PERM_ADDR(wl->hw, wl->mac_addr); 

is strange in wl12xx_op_add_interface for sure. Should at least be in
->start() if you can't read it before. Or is that trying to override the
address the user assigned??

wl12xx_op_remove_interface ought to clear the device's address to stop
acking frames.

	wl12xx_cmd_template_set(wl, CMD_PROBE_RESP, beacon->data,

hmm. mac80211 doesn't deal with probe response offload right now.

                /*
                 * We enter PSM only if we're already associated.
                 * If we're not, we'll enter it when joining an SSID,
                 * through the bss_info_changed() hook.
                 */

Umm, mac80211 only enables CONF_PS after associating. So psm_requested
is unnecessary.

wl12xx_build_basic_rates and wl12xx_build_extended_rates are unnecessary
-- use scan_req->ie and set the IE max length. Similarly
wl12xx_build_probe_req should be reduced a lot (to just putting in the
SSID). You could also support the channels passed in the scan request.

        wl->hw->flags = IEEE80211_HW_SIGNAL_DBM |
                IEEE80211_HW_NOISE_DBM;

No powersave bits?

MODULE_AUTHOR("Kalle Valo <Kalle.Valo@nokia.com>, "
                "Luciano Coelho <luciano.coelho@nokia.com>");

Use multiple MODULE_AUTHOR() macros.

        /*
         * The rx status timestamp is a 32 bits value while the TSF is a
         * 64 bits one.
         * For IBSS merging, TSF is mandatory, so we have to get it
         * somehow, so we ask for ACX_TSF_INFO.
         * That could be moved to the get_tsf() hook, but unfortunately,
         * this one must be atomic, while our SPI routines can sleep.   
         */
        if ((wl->bss_type == BSS_TYPE_IBSS) && beacon) {

If the chip supports a good filter flags set it could be useful for
monitoring to always do this if monitor is enabled. Not really necessary
though.

        status->flag |= RX_FLAG_TSFT;

You should set that only when it's actually completely filled I think.
p54 has a trick to keep track of the upper 32 bits in software, that
assumes getting a frame every 2**32 us though.

        skb = dev_alloc_skb(length);
        if (!skb) {
                wl12xx_error("Couldn't allocate RX frame");
                return;
        }

        rx_buffer = skb_put(skb, length);

how about IPv4 alignment? Could you read the packet header first,
memmove that and then read the rest, i.e. do two wl12xx_spi_mem_read
calls? Might or might not be more efficient...

You need to rm wl12xx_80211.h and use linux/ieee80211.h, extending where
necessary.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: wl12xx: third submission

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> Hi,

Moin,

> A couple of comments.

Thanks for the review, I appreciate this.

> It clashes with the patches that I just posted to remove
> config_interface in favour of only using bss_info_changed.

No problem, I can update wl12xx as soon as you finalise the interface.
Just let me know.

> This:
>         if (memcmp(wl->mac_addr, conf->mac_addr, ETH_ALEN)) {
>                 memcpy(wl->mac_addr, conf->mac_addr, ETH_ALEN);
>                 SET_IEEE80211_PERM_ADDR(wl->hw, wl->mac_addr); 
>
> is strange in wl12xx_op_add_interface for sure. Should at least be in
> ->start() if you can't read it before. Or is that trying to override the
> address the user assigned??

Frankly I don't even remember, that's old code :) I'll check it.

> wl12xx_op_remove_interface ought to clear the device's address to stop
> acking frames.

Will fix.

> 	wl12xx_cmd_template_set(wl, CMD_PROBE_RESP, beacon->data,
>
> hmm. mac80211 doesn't deal with probe response offload right now.

Yeah.

>
>                 /*
>                  * We enter PSM only if we're already associated.
>                  * If we're not, we'll enter it when joining an SSID,
>                  * through the bss_info_changed() hook.
>                  */
>
> Umm, mac80211 only enables CONF_PS after associating. So psm_requested
> is unnecessary.

True. Again old code.

> wl12xx_build_basic_rates and wl12xx_build_extended_rates are unnecessary
> -- use scan_req->ie and set the IE max length. Similarly
> wl12xx_build_probe_req should be reduced a lot (to just putting in the
> SSID). You could also support the channels passed in the scan request.

Ok.

>         wl->hw->flags = IEEE80211_HW_SIGNAL_DBM |
>                 IEEE80211_HW_NOISE_DBM;
>
> No powersave bits?

Again old code :) Will fix.

> MODULE_AUTHOR("Kalle Valo <Kalle.Valo@nokia.com>, "
>                 "Luciano Coelho <luciano.coelho@nokia.com>");
>
> Use multiple MODULE_AUTHOR() macros.

Ok.

>         /*
>          * The rx status timestamp is a 32 bits value while the TSF is a
>          * 64 bits one.
>          * For IBSS merging, TSF is mandatory, so we have to get it
>          * somehow, so we ask for ACX_TSF_INFO.
>          * That could be moved to the get_tsf() hook, but unfortunately,
>          * this one must be atomic, while our SPI routines can sleep.   
>          */
>         if ((wl->bss_type == BSS_TYPE_IBSS) && beacon) {
>
> If the chip supports a good filter flags set it could be useful for
> monitoring to always do this if monitor is enabled. Not really necessary
> though.

Actually I would like to get rid of that call. SPI access dead slow and
I don't want to have any extra commands in RX path. So this needs to be
reworked anyway.

>
>         status->flag |= RX_FLAG_TSFT;
>
> You should set that only when it's actually completely filled I think.
> p54 has a trick to keep track of the upper 32 bits in software, 

This would help.

> that assumes getting a frame every 2**32 us though.

But we can check that in driver and compensate, right?

>         skb = dev_alloc_skb(length);
>         if (!skb) {
>                 wl12xx_error("Couldn't allocate RX frame");
>                 return;
>         }
>
>         rx_buffer = skb_put(skb, length);
>
> how about IPv4 alignment? Could you read the packet header first,
> memmove that and then read the rest, i.e. do two wl12xx_spi_mem_read
> calls? Might or might not be more efficient...

SPI access add latency, it would be faster to read as much as possible
in one transaction.

> You need to rm wl12xx_80211.h and use linux/ieee80211.h, extending where
> necessary.

Agreed. I have been thinking about this but never took the challenge.

Is there anything which in your opinion prevents inclusion to
wireless-testing?

-- 
Kalle Valo

Re: wl12xx: third submission

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1593 bytes --]

On Thu, 2009-04-23 at 20:49 +0300, Kalle Valo wrote:

> No problem, I can update wl12xx as soon as you finalise the interface.
> Just let me know.

Well, I just posted it earlier as [PATCH] :)

> > 	wl12xx_cmd_template_set(wl, CMD_PROBE_RESP, beacon->data,
> >
> > hmm. mac80211 doesn't deal with probe response offload right now.
> 
> Yeah.

You're not setting the IBSS bit anyway though. ;) Might need to come up
with a patch to disable mac80211 probe responses, that would be much
more efficient...

> >         status->flag |= RX_FLAG_TSFT;
> >
> > You should set that only when it's actually completely filled I think.
> > p54 has a trick to keep track of the upper 32 bits in software, 
> 
> This would help.
> 
> > that assumes getting a frame every 2**32 us though.
> 
> But we can check that in driver and compensate, right?

Well, you can check against jiffies and then resync with the get_tsf
command, I guess.

> > how about IPv4 alignment? Could you read the packet header first,
> > memmove that and then read the rest, i.e. do two wl12xx_spi_mem_read
> > calls? Might or might not be more efficient...
> 
> SPI access add latency, it would be faster to read as much as possible
> in one transaction.

Question is how it measures against the CPU doing memmove() on the
entire frame when necessary.

> Is there anything which in your opinion prevents inclusion to
> wireless-testing?

Give that you and Bob are working on it, I don't think so. Except the
API issue, which means it won't compile when John merges my patch.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: wl12xx: third submission

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Thu, 2009-04-23 at 20:49 +0300, Kalle Valo wrote:
>
>> No problem, I can update wl12xx as soon as you finalise the interface.
>> Just let me know.
>
> Well, I just posted it earlier as [PATCH] :)

Thanks, I'll start working on it tomorrow.

>> > how about IPv4 alignment? Could you read the packet header first,
>> > memmove that and then read the rest, i.e. do two wl12xx_spi_mem_read
>> > calls? Might or might not be more efficient...
>> 
>> SPI access add latency, it would be faster to read as much as possible
>> in one transaction.
>
> Question is how it measures against the CPU doing memmove() on the
> entire frame when necessary.

I don't have any numbers, but for example omap2_mcspi is scheduling
processes etc. so I would say that memmove() is cheaper. Does anyone
have more knowledge about this?

>> Is there anything which in your opinion prevents inclusion to
>> wireless-testing?
>
> Give that you and Bob are working on it, I don't think so. Except the
> API issue, which means it won't compile when John merges my patch.

The compilation fix is easy to solve, I'll take care of it.

Thanks for your comments.

-- 
Kalle Valo