On Fri, Jul 15, 2011 at 2:54 AM, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote: > On Wed, 2011-07-13 at 16:45 -0700, Thomas Pedersen wrote: > >> + if (ieee80211_vif_is_mesh(&sdata->vif)) >> + rcu_assign_pointer(sdata->u.mesh.beacon, new); > > I don't think this compiles if MESH isn't configured into mac80211. Not > sure adding lots of ifdefs is good, but it's probably the only choice > right now. The ifdef is already in ieee80211_vif_is_mesh() > >> @@ -461,6 +462,9 @@ void ieee80211_start_mesh(struct ieee80211_sub_if_data *sdata) >> { >> struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh; >> struct ieee80211_local *local = sdata->local; >> + struct ieee80211_mgmt *bcn_head; >> + struct beacon_parameters bcn_params; >> + u8 *pos; >> >> local->fif_other_bss++; >> /* mesh ifaces must set allmulti to forward mcast traffic */ >> @@ -473,7 +477,44 @@ void ieee80211_start_mesh(struct ieee80211_sub_if_data *sdata) >> set_bit(MESH_WORK_HOUSEKEEPING, &ifmsh->wrkq_flags); >> ieee80211_mesh_root_setup(ifmsh); >> ieee80211_queue_work(&local->hw, &sdata->work); >> + >> + /* Build fixed part of mesh beacon. */ >> + memset(&bcn_params, 0, sizeof(struct beacon_parameters)); >> + >> + /* header + fixed fields + null ssid */ >> + bcn_params.head_len = 24 + sizeof(bcn_head->u.beacon) + 2; >> + pos = kmalloc(bcn_params.head_len, GFP_KERNEL | __GFP_ZERO); > > Err, kzalloc, don't use __GFP_ZERO. > >> + if (mac80211_config_ops.add_beacon(sdata->wdev.wiphy, sdata->dev, >> + &bcn_params)) >> + printk(KERN_ERR "Unable to configure mesh beacon\n"); > > That's horrible. Don't do that. Just do whatever is necessary inline. If > you restructure, you only need like 10% of the code from > ieee80211_config_beacon() anyway. > >> @@ -498,6 +540,8 @@ void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata) >> * it no longer is. >> */ >> cancel_work_sync(&sdata->work); >> + if (sdata->u.mesh.beacon) >> + mac80211_config_ops.del_beacon(sdata->wdev.wiphy, sdata->dev); > > Ditto. > >> } else if (ieee80211_vif_is_mesh(&sdata->vif)) { >> - struct ieee80211_mgmt *mgmt; >> - u8 *pos; >> - >> + struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh; >> #ifdef CONFIG_MAC80211_MESH >> if (!sdata->u.mesh.mesh_id_len) >> goto out; >> #endif >> + beacon = rcu_dereference(ifmsh->beacon); > > not going to compile w/o CONFIG_MAC80211_MESH I'll look into this. > >> + if (beacon) { >> + /* headroom, head length, tail length, mesh_ies and >> + * maximum TIM length */ >> + skb = dev_alloc_skb(local->tx_headroom + >> + beacon->head_len + >> + beacon->tail_len + 400); > > what's + 400? You can't really think the TIM IE is that long? No, but you're right, that number is pretty arbitrary. Maximum length of mesh IEs currently (Mesh ID + Mesh Conf + Mesh Awake) is only 47. Since mesh IEs have to be added during beacon time, might it make sense to track the needed length in the if_mesh? > >> - /* headroom, head length, tail length and maximum TIM length */ >> - skb = dev_alloc_skb(local->tx_headroom + 400 + >> - sdata->u.mesh.ie_len); > > I guess it was even there before though ... huh? > > Couldn't more code here be shared between mesh and AP? I don't see any > difference here between mesh and AP. > > Also ... since you'll need PS support eventually anyway, maybe you > should refactor "struct ieee80211_if_ap" into something AP-specific > (currently only VLANs) and the rest, and then use "the rest" in mesh as > well to make all this code more unified. I like this approach much better. Are you suggesting just factoring out the PS data, or taking it further and creating a generic "if which beacons"? > johannes > > Thanks, Thomas -- 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