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
