Hi,
A few more minor comments:
> +mac80211_hwsim is a Linux kernel module that can be used to simulate
> +arbitrary number of IEEE 802.11 radios for mac80211 on a single
> +device.
To me, that makes it sound as though you needed an underlying wireless
device. Maybe that "on a single device" should just be removed?
> +mac80211_hwsim kernel module has a parameter 'radios' that can be used
> +to select how many radios are simulates (default 2). This allows
^ typo, simulated
> +This example shows how to use mac80211_hwsim to simulate two radios:
> +one to act as an access point and the other as a station that
> +associates with the AP. hostapd and wpa_supplicant are used to take
> +care of WPA2-PSK authentication. In addition, hostapd is also
> +processing access point side of association.
> +
> +Please note that the current Linux kernel does not enable AP mode, so a
> +simple patch is needed to enable AP mode selection:
> +http://johannes.sipsolutions.net/patches/kernel/all/LATEST/006-allow-ap-vlan-modes.patch
Ok, once this code is in I'll add a hunk to my patch there that removes
this note.
> + hdr->hdr.it_present = __constant_cpu_to_le32(
> + (1 << IEEE80211_RADIOTAP_FLAGS) |
> + (1 << IEEE80211_RADIOTAP_RATE) |
> + (1 << IEEE80211_RADIOTAP_CHANNEL));
> + skb->protocol = __constant_htons(ETH_P_802_2);
No need to use the __constant_ prefix, the regular macros are smart
enough to optimise out in that case; the __constant versions are only
needed when the compiler expression has to be constant, e.g. for
switch (..) {
case __constant_cpu_to_le32(7): ...
}
> + if (memcmp(hdr->addr1, hwsim_radios[i]->wiphy->perm_addr,
> + ETH_ALEN) == 0)
> + ack = 1;
Since you never set "ack" anywhere else, you could just say
ack = memcmp(...) == 0;
and remove the =0 assignment earlier. Not that it matters, though.
> +static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac,
> + struct ieee80211_vif *vif)
> +{
> + struct ieee80211_hw *hw = arg;
> + struct mac80211_hwsim_data *data = hw->priv;
> + struct sk_buff *skb;
> + struct ieee80211_rx_status rx_status;
> + int i;
> + struct ieee80211_tx_info *info;
> +
> + if (vif->type != IEEE80211_IF_TYPE_AP)
> + return;
> +
> + skb = ieee80211_beacon_get(hw, vif);
> + if (skb == NULL)
> + return;
> + info = IEEE80211_SKB_CB(skb);
> +
> + mac80211_hwsim_monitor_rx(hw, skb);
> +
> + memset(&rx_status, 0, sizeof(rx_status));
> + /* TODO: set mactime */
> + rx_status.freq = data->channel->center_freq;
> + rx_status.band = data->channel->band;
> + rx_status.rate_idx = info->tx_rate_idx;
> + /* TODO: simulate signal strength (and optional packet drop) */
> +
> + /* Copy skb to all enabled radios that are on the current frequency */
> + for (i = 0; i < hwsim_radio_count; i++) {
> + struct mac80211_hwsim_data *data2;
> + struct sk_buff *nskb;
> +
> + if (hwsim_radios[i] == NULL || hwsim_radios[i] == hw)
> + continue;
> + data2 = hwsim_radios[i]->priv;
> + if (!data2->started || !data2->radio_enabled ||
> + data->channel->center_freq != data2->channel->center_freq)
> + continue;
> +
> + nskb = skb_copy(skb, GFP_ATOMIC);
> + if (nskb == NULL)
> + continue;
> +
> + ieee80211_rx_irqsafe(hwsim_radios[i], nskb, &rx_status);
Maybe there should be a helper function for all this? It seems mostly
duplicated.
> + ieee80211_iterate_active_interfaces(hw, mac80211_hwsim_beacon_tx, hw);
Hmm. You could use the _atomic version to avoid taking the RTNL, it uses
RCU instead.
None of this is really all that important, so
Acked-by: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
