Re: [RFC 2/2] mac80211: Add new interface type for OCB mode

[Johannes Berg]

On Mon, 2014-05-19 at 16:49 +0200, Rostislav Lisovy wrote:

>  include/uapi/linux/nl80211.h |   3 ++
>  net/mac80211/Makefile        |   3 +-
>  net/mac80211/cfg.c           |   1 +
>  net/mac80211/chan.c          |   2 +
>  net/mac80211/driver-ops.h    |   3 +-
>  net/mac80211/ieee80211_i.h   |  38 ++++++++++++++
>  net/mac80211/iface.c         |  28 +++++++++++
>  net/mac80211/ocb.c           | 115 +++++++++++++++++++++++++++++++++++++++++++
>  net/mac80211/util.c          |   5 ++
>  net/wireless/util.c          |   1 +

please avoid patches that modify both cfg80211 and mac80211 unless
they're really small.

> +/**
> + * struct ieee80211_if_ocb - the specific struct for ocb-mode
> + *
> + * In this struct all ocb-specific information of an interface is stored.
> + *
> + * @timer: the timer used for all tasks in the ocb-code

Why do you need your own timer?

> + * @work: holds the workqueue

and work struct (not a workqueue - that's something entirely different)?
we have a general interface work struct.

> + * @skb_queue: holds all queued skb to be processed

Why do you need to queue SKBs?

> + * @wrkq_flags: bitmask telling what work is pending
> + * @timer_running: tells if the timer is running
> + * @bssid: holds the BSSID (IEEE802.11p defines this to be
> + *         ff:ff:ff:ff:ff:ff but this is more flexible)

Why would you ever need this flexibility? It seems pointless to prepare
for it - I'm guessing you're never going to implement it?

> @@ -622,6 +632,17 @@ int ieee80211_do_open(struct wireless_dev *wdev, bool coming_up)
>  
>  		if (sdata->vif.type != NL80211_IFTYPE_P2P_DEVICE)
>  			changed |= ieee80211_reset_erp_info(sdata);
> +
> +		if (sdata->vif.type == NL80211_IFTYPE_OCB) {
> +			/* Disable beacons */
> +			sdata->vif.bss_conf.enable_beacon = false;
> +			changed |= BSS_CHANGED_BEACON;
> +
> +			/* Receive all data frames */
> +			local->fif_other_bss++;
> +			ieee80211_configure_filter(local);
> +		}

NACK. Please make a proper "join OCB" operation etc. In the wireless
stack, "ifup" shouldn't be used to start real operation.

> +/**
> + * ieee80211_ocb_housekeeping - Housekeeping function (expires stations)
> + *
> + * @sdata:
> + * @ifocb:

yes?

> + * This function is used for all periodical clean up work.
> + * It expires all stations that have not shown up for a given period of time.
> + * After all cleanups have been done it schedules the next run.
> + */
> +static void ieee80211_ocb_housekeeping(struct ieee80211_sub_if_data *sdata,
> +			   struct ieee80211_if_ocb *ifocb)
> +{
> +	ieee80211_sta_expire(sdata, IEEE80211_OCB_PEER_INACTIVITY_LIMIT);

> +	mod_timer(&ifocb->timer,
> +		  round_jiffies(jiffies + IEEE80211_OCB_HOUSEKEEPING_INTERVAL));
> +}

use a delayed work struct?

I don't even see a call to turn on the carrier?

Anyway I think you should have some OCB join method in the cfg80211 API
even if there's nothing really to do... at minimum, you need to specify
a channel somehow!

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/