On Wednesday 16 May 2007 17:45, James Ketrenos wrote: > This patch adds the iwlwifi project directory and sources needed to > build the mac80211 based wireless drivers for the Intel PRO/Wireless > 3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN adapters. > Looks much better than when I last checked. A few comments: - The enter/leave stuff should be removed. - Many of the in_interrupt() checks are unnecessary. Jiri recently documented which callbacks need to be atomic - the ones that don't need to be atomic definitely won't be called in an interrupt. - Get rid of the stub functions for callbacks, like set_multicast_list and reset. mac80211 checks if those callbacks are defined before calling them. - CONFIG_IWLWIFI_QOS doesn't appear to be defined. - Read and set the MAC address before calling ieee80211_register_hw. - flush_scheduled_work in d_stop can lead to deadlocks since RTNL is held. It looks like everything is running on a dedicated workqueue so that doesn't really even do anything. priv->workqueue should be flushed instead if needed. - mac80211 (very recently) sets up a workqueue for the driver to use so you don't need to create your own anymore. - param_auto_create looks very suspicious. mac80211 already creates an adhoc station if it can't join one.. what's this for? - The suspend and resume callback setting in struct pci_driver iwl_driver needs to be put on two lines. - Why are probe requests being dropped in adhoc mode? (assuming hardware handles it.. but the message output for that doesn't sound like it) - The bitwise ORs shouldn't be getting a line to itself in iwl3945_rx_init and iwl3945_tx_reset. - Mixed case variable name in iwl_hw_txq_ctx_stop: queueId. - CONFIG_IWLWIFI_MONITOR and CONFIG_IEEE80211_RADIOTAP doesn't look right. - RX_FLAG_RADIOTAP should be set in the ieee80211_rx_status flag when a radiotap header is added to a frame. (otherwise, mac80211 will add its own radiotap header) - If you don't want to add a radiotap header all the time, you should check the IEEE80211_CONF_RADIOTAP flag to see if a radiotap header is desired by the stack. (not whether or not we're configured as IEEE80211_IF_TYPE_MNTR) - Splitting rx_start->phy_flags over two lines in iwl4965_rx_reply_rx doesn't look good. - The IEEE80211_FTYPE_MGMT case in the switch statement inside iwl4965_rx_reply_rx doesn't need that extra set of braces. Also, the && and || don't need their own lines. - param_channel looks suspicious too. - ieee80211chan2mhz is provided by ieee80211_radiotap.h. Haven't seen anything severe enough to be a blocker for wireless-dev, however. -Michael Wu
Attachment:
pgpKWklVCpAdR.pgp
Description: PGP signature
