Hi Jes,
> Many thanks to Johannes Berg for 802.11 insights and help and Larry
> Finger for help with the vendor driver.
:-)
Just a few comments since this is really the first time I'm looking
closely at the code.
> + h2c.ramask.cmd = H2C_SET_RATE_MASK;
> + put_unaligned_le16(ramask & 0xffff, &h2c.ramask.mask_lo);
> + put_unaligned_le16(ramask >> 16, &h2c.ramask.mask_hi);
you marked that __packed, so the put_unaligned_le16() isn't really
needed, you can use regular assignments (= cpu_to_le16(...))
> +static void
> +rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> + struct ieee80211_bss_conf *bss_conf, u32 changed)
> +{
> + struct rtl8xxxu_priv *priv = hw->priv;
> + struct device *dev = &priv->udev->dev;
> + struct ieee80211_sta *sta;
> + u32 val32;
> +#if 0
> + u16 val16;
> +#endif
> + u8 val8;
> +
> + if (changed & BSS_CHANGED_ASSOC) {
> + struct h2c_cmd h2c;
> +
> + memset(&h2c, 0, sizeof(struct h2c_cmd));
> + rtl8xxxu_set_linktype(priv, vif->type);
> +
> + if (bss_conf->assoc) {
> + rcu_read_lock();
> + sta = ieee80211_find_sta(vif, bss_conf->bssid);
> + if (!sta) {
> + dev_info(dev, "%s: ASSOC no sta found\n",
> + __func__);
> + rcu_read_unlock();
> + goto error;
> + }
> +
> + if (sta->ht_cap.ht_supported)
> + dev_info(dev, "%s: HT supported\n", __func__);
> + if (sta->vht_cap.vht_supported)
> + dev_info(dev, "%s: VHT supported\n", __func__);
> + rtl8xxxu_update_rate_table(priv, sta);
> + rcu_read_unlock();
You could perhaps consider doing this in the sta_state() callback, when
the station becomes associated, which is just before (or just after?)
this gets called. Then you don't need to do the find_sta() here. Not
that it's really a problem, but might look a bit nicer/cleaner.
> + if (changed & BSS_CHANGED_HT) {
> + u8 ampdu_factor, ampdu_density;
> + u8 sifs;
> +
> + rcu_read_lock();
> + sta = ieee80211_find_sta(vif, bss_conf->bssid);
> + if (!sta) {
> + dev_info(dev, "BSS_CHANGED_HT: No HT sta found!\n");
> + rcu_read_unlock();
> + goto error;
> + }
> + if (sta->ht_cap.ht_supported) {
> + ampdu_factor = sta->ht_cap.ampdu_factor;
> + ampdu_density = sta->ht_cap.ampdu_density;
> + sifs = 0x0e;
> + } else {
> + ampdu_factor = 0;
> + ampdu_density = 0;
> + sifs = 0x0a;
> + }
> + rcu_read_unlock();
This doesn't really make much sense - BSS_CHANGED_HT only says we
changed bss_conf.ht_operation_mode which you don't use here.
> + if (changed & BSS_CHANGED_BSSID) {
> + dev_info(dev, "Changed BSSID!\n");
> + rtl8xxxu_set_bssid(priv, bss_conf->bssid);
> +
> + rcu_read_lock();
> + sta = ieee80211_find_sta(vif, bss_conf->bssid);
> + if (!sta) {
> + dev_info(dev, "No bssid sta found!\n");
> + rcu_read_unlock();
> + goto error;
> + }
> + rcu_read_unlock();
> + }
You probably want to roll this up into CHANGED_ASSOC, since
CHANGED_BSSID always really comes together with that for station mode.
For IBSS things would be different but you don't seem to do that.
> + if (changed & BSS_CHANGED_BASIC_RATES) {
> + dev_info(dev, "Changed BASIC_RATES!\n");
> + rcu_read_lock();
> + sta = ieee80211_find_sta(vif, bss_conf->bssid);
> + if (sta)
> + rtl8xxxu_set_basic_rates(priv, sta);
> + else
> + dev_info(dev,
> + "BSS_CHANGED_BASIC_RATES: No sta found!\n");
> +
> + rcu_read_unlock();
> + }
Ditto, this ever change during the lifetime of a BSS either, i.e. it
cannot change while you're associated, and you don't care while you're
not. Again, IBSS could be different, not quite sure off the top of my
head.
> +static u32 rtl8xxxu_queue_select(struct ieee80211_hw *hw, struct sk_buff *skb)
> +{
> + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> + u32 queue;
> +
> + if (unlikely(ieee80211_is_beacon(hdr->frame_control)))
> + queue = TXDESC_QUEUE_BEACON;
> + if (ieee80211_is_mgmt(hdr->frame_control))
I think you mean else if?
Anyway beacons cannot happen here except perhaps for injection, in which
case you probably wouldn't want to use the beacon queue anyway. I'd
remove the beacon case here entirely.
> + queue = TXDESC_QUEUE_MGNT;
> + else
> + queue = rtl8xxxu_80211_to_rtl_queue(skb_get_queue_mapping(skb));
> +
> + return queue;
> +}
> +
> +static void rtl8xxxu_calc_tx_desc_csum(struct rtl8xxxu_tx_desc *tx_desc)
> +{
> + u16 *ptr = (u16 *)tx_desc;
> + u16 csum = 0;
> + int i;
> +
> + tx_desc->csum = cpu_to_le32(0);
> +
> + for (i = 0; i < (sizeof(struct rtl8xxxu_tx_desc) / sizeof(u16)); i++)
> + csum = csum ^ le16_to_cpu(ptr[i]);
> +
> + tx_desc->csum |= cpu_to_le32(csum);
This seems kinda pointless - you can remove the = 0 above and just use =
below?
> +static void rtl8xxxu_tx_complete(struct urb *urb)
> +{
> + struct sk_buff *skb = (struct sk_buff *)urb->context;
> + struct ieee80211_tx_info *tx_info;
> + struct ieee80211_hw *hw;
> +
> + tx_info = IEEE80211_SKB_CB(skb);
> + hw = tx_info->rate_driver_data[0];
Seems a bit odd to do one initialisation in the declarations, and the
other two not, but hey, that really doesn't matter :)
> + skb_pull(skb, sizeof(struct rtl8xxxu_tx_desc));
> +
> + ieee80211_tx_info_clear_status(tx_info);
> + tx_info->status.rates[0].idx = -1;
> + tx_info->status.rates[0].count = 0;
This is ... suboptimal. If you have any idea what the rates were it'd be
good to report at least the one that went through.
> + tx_info->flags |= IEEE80211_TX_STAT_ACK;
This shouldn't be done I think, if you can't report ACK status then I
think you shouldn't do it at all, you aren't setting the
IEEE80211_HW_REPORTS_TX_ACK_STATUS flag (though if you could have the
ack status it would certainly be better to report it properly)
[snip code]
Something about the TX is a bit fishy, you're using some software rate
control data but aren't reporting any data back for rate control which
seems to imply the device does it. You're also setting
IEEE80211_HW_HAS_RATE_CONTROL which means you can't be using rates etc.
here? I'll probably have to look into this more, it's an area that I'm
not intimately familiar with.
> + for (i = 0; i < (sizeof(struct rtl8xxxu_rx_desc) / sizeof(u32)); i++)
> + _rx_desc[i] = le32_to_cpu(_rx_desc_le[i]);
This is ... interesting. Wouldn't it make more sense to declare with
little endian and without bitfields instead? bitfields are a bit of a
minefield anyway as far as the C standard is concerned, so since you
need to rely here on exact memory layout to satisfy the firmware API,
I'd really avoid them.
> + ieee80211_rx_irqsafe(hw, skb);
Here you give up ownership of the skb
> + skb_size = sizeof(struct rtl8xxxu_rx_desc) +
> + RTL_RX_BUFFER_SIZE;
> + skb = dev_alloc_skb(skb_size);
And then you allocate a new one into the same variable. From a
maintenance POV that seems a bit ugly, perhaps you should avoid that.
> + if (!skb) {
> + dev_warn(dev, "%s: Unable to allocate skb\n", __func__);
> + goto cleanup;
> + }
> +
> + memset(skb->data, 0, sizeof(struct rtl8xxxu_rx_desc));
> + usb_fill_bulk_urb(&rx_urb->urb, priv->udev, priv->pipe_in,
> + skb->data, skb_size, rtl8xxxu_rx_complete,
This is also very strange, you're sending essentially random data, from
an skb pointer that's not even a proper skb len? I think you just want
to use kmalloc() here instead of having an skb? Or at least use
skb_put() to reserve that memory area - it doesn't make a big difference
to you but using the skb API in this way is likely to confuse people in
the future.
In fact you could even allocate a page instead, and then add it to the
skb as a frag like iwlwifi does for example, and potentially not have to
copy the data around later when it goes somewhere. Not sure that's worth
it for this device though :)
Or perhaps just refactor the resubmission code into a small new
function ... would also help with the error path here :)
Also, what happens if you cannot allocate an skb here a few times? do
your outstanding URBs get fewer every time? Or do you just have one?
> +static int rtl8xxxu_submit_rx_urb(struct ieee80211_hw *hw)
> +{
> + struct rtl8xxxu_priv *priv = hw->priv;
> + struct sk_buff *skb;
> + struct rtl8xxxu_rx_urb *rx_urb;
> + int skb_size;
> + int ret;
> +
> + skb_size = sizeof(struct rtl8xxxu_rx_desc) + RTL_RX_BUFFER_SIZE;
> + skb = dev_alloc_skb(skb_size);
> + if (!skb)
> + return -ENOMEM;
> +
> + memset(skb->data, 0, sizeof(struct rtl8xxxu_rx_desc));
> +
> + rx_urb = kmalloc(sizeof(struct rtl8xxxu_rx_urb), GFP_ATOMIC);
> + if (!rx_urb) {
> + dev_kfree_skb(skb);
> + return -ENOMEM;
> + }
> + usb_init_urb(&rx_urb->urb);
> + rx_urb->hw = hw;
> +
> + usb_fill_bulk_urb(&rx_urb->urb, priv->udev, priv->pipe_in, skb->data,
> + skb_size, rtl8xxxu_rx_complete, skb);
> + usb_anchor_urb(&rx_urb->urb, &priv->rx_anchor);
> + ret = usb_submit_urb(&rx_urb->urb, GFP_ATOMIC);
> + if (ret)
> + usb_unanchor_urb(&rx_urb->urb);
> + return ret;
> +}
Perhaps some part of this could also be refactored with the above
resubmission? It looks essentially the same, perhaps the above code
could even just call this.
> +static int rtl8xxxu_set_rts_threshold(struct ieee80211_hw *hw, u32 rts)
> +{
> + if (rts > 2347)
> + return -EINVAL;
> +
> + return 0;
> +}
You should either use the value here, or not implement this callback if
you want to rely on mac80211 setting the flags, IIRC.
> + switch (cmd) {
> + case SET_KEY:
> + /*
> + * This is a bit of a hack - the lower bits of the cipher
> + * suite selector happens to match the cipher index in the
> + * CAM
> + */
It's probably not really a hack - the cipher suite selectors are defined
in the spec, and we prefix them with the OUI to get the 32-bit
identifier. If you assume standard-only ciphers then the lower 8 bits
are just the number given to the suite in the spec.
> + if (priv->rf_paths > 1) {
> + sband->ht_cap.mcs.rx_mask[1] = 0xff;
> + sband->ht_cap.mcs.rx_highest = cpu_to_le16(300);
> + sband->ht_cap.cap |= IEEE80211_HT_CAP_SGI_40;
> + } else {
> + sband->ht_cap.mcs.rx_mask[1] = 0x00;
> + sband->ht_cap.mcs.rx_highest = cpu_to_le16(150);
> + }
You don't really need to set rx_highest, and you already cleared
rx_mask[1], so you don't need the else branch and can remove the
rx_highest from the if branch.
> + sband->ht_cap.mcs.tx_params = IEEE80211_HT_MCS_TX_DEFINED;
That doesn't seem true?
> + hw->wiphy->max_remain_on_channel_duration = 65535; /* ms */
That's a bit excessive, but you don't implement ROC in hw anyway? so I'd
remove this part I think.
> + hw->wiphy->cipher_suites = rtl8xxxu_cipher_suites;
> + hw->wiphy->n_cipher_suites = ARRAY_SIZE(rtl8xxxu_cipher_suites);
If you can deal with software crypto (which I believe you can) then you
should just remove this and get all the ciphers, just making the ones
you can do in software faster. You'll get all the new GCM ones, and
CCM-256 too, in SW.
johannes
--
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
