Johannes Berg <johannes@xxxxxxxxxxxxxxxx> 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@xxxxxxxxx>, "
> "Luciano Coelho <luciano.coelho@xxxxxxxxx>");
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
