Re: [PATCH V3] Add iwlwifi wireless drivers

Johannes Berg
2007-09-04T16:33:37Z

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

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

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