Re: [RFC 3/3] orinoco: initiate cfg80211 conversion

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 4880 bytes --]

On Sat, 2009-05-16 at 13:22 +0100, David Kilroy wrote:

> +/* Supported bitrates. Must agree with hw.c
> + * TODO: are the flags correct? */
> +static const struct ieee80211_rate orinoco_rates[] = {
> +	{ .bitrate = 10 },
> +	{ .bitrate = 20, .flags = IEEE80211_RATE_SHORT_PREAMBLE },
> +	{ .bitrate = 55, .flags = IEEE80211_RATE_SHORT_PREAMBLE },
> +	{ .bitrate = 110, .flags = IEEE80211_RATE_SHORT_PREAMBLE },
> +};

Flags look fine from a protocol POV, I don't know whether your hw
supports short preamble. On the other hand, this is only reported to
userspace, we don't do anything with it in cfg80211.

> +/* Called after orinoco_private is allocated.
> + * Which must we do now? Which do we put off until we have firmware?
> + */

Generally, try to do things as early as possible.

> +void orinoco_wiphy_init(struct wiphy *wiphy, struct orinoco_private *priv)
> +{
> +	/* Setup so that we can recover orinoco_private from wiphy */
> +	ophy_priv(wiphy) = priv;
> +
> +	wiphy->privid = orinoco_wiphy_privid;
> +
> +	priv->wdev.wiphy = wiphy;
> +	priv->wdev.iftype = NL80211_IFTYPE_STATION;
> +
> +	set_wiphy_dev(wiphy, priv->dev);
> +}
> +
> +/* Called after firmware is initialised */
> +int orinoco_wiphy_register(struct wiphy *wiphy)
> +{
> +	struct orinoco_private *priv = ophy_priv(wiphy);
> +	int i, channels = 0;
> +
> +	memcpy(wiphy->perm_addr, priv->ndev->dev_addr, ETH_ALEN);
> +
> +	wiphy->max_scan_ssids = 1;
> +
> +	wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION);
> +
> +	/* TODO: should we set if we only have demo ad-hoc?
> +	 *       (priv->has_port3)
> +	 */
> +	if (priv->has_ibss)
> +		wiphy->interface_modes |= BIT(NL80211_IFTYPE_ADHOC);
> +
> +	memcpy(priv->rates, orinoco_rates, sizeof(orinoco_rates));
> +	priv->band.bitrates = priv->rates;
> +	priv->band.n_bitrates = ARRAY_SIZE(orinoco_rates);

Is there a need to copy the rates? Normally you should be able to just
do
	priv->band.bitrates = orinoco_rates;

> +	/* Only support channels allowed by the card EEPROM */
> +	for (i = 0; i < 14; i++) {
> +		if (priv->channel_mask & (1 << i)) {
> +			priv->channels[i].center_freq =
> +				ieee80211_dsss_chan_to_freq(i+1);
> +			channels++;
> +		}
> +	}
> +	priv->band.channels = priv->channels;
> +	priv->band.n_channels = channels;
> +
> +	wiphy->bands[IEEE80211_BAND_2GHZ] = &priv->band;
> +	wiphy->signal_type = CFG80211_SIGNAL_TYPE_MBM;
> +
> +	return wiphy_register(wiphy);
> +}

When do you load firmware? We try to only load it when the first
interface is brought up, but that's sometimes problematic because then
we don't have enough information.

> +const struct cfg80211_ops orinoco_cfg_ops = {
> +
> +};

Heh.

>  #include "orinoco.h"
> @@ -2502,6 +2504,11 @@ static int orinoco_init(struct net_device *dev)
>  	priv->wpa_ie_len = 0;
>  	priv->wpa_ie = NULL;
>  
> +	if (orinoco_wiphy_register(priv->wdev.wiphy)) {
> +		err = -ENODEV;
> +		goto out;
> +	}
> +
>  	/* Make the hardware available, as long as it hasn't been
>  	 * removed elsewhere (e.g. by PCMCIA hot unplug) */
>  	spin_lock_irq(&priv->lock);
> @@ -2533,12 +2540,26 @@ struct net_device
>  {
>  	struct net_device *dev;
>  	struct orinoco_private *priv;
> +	struct wiphy *wiphy;
> +
> +	/* allocate wiphy
> +	 * NOTE: We only support a single virtual interface, so wiphy
> +	 * and wireless_dev are somewhat synonymous for this device.
> +	 */
> +	wiphy = wiphy_new(&orinoco_cfg_ops,
> +			  sizeof(struct orinoco_private *));
> +	if (!wiphy)
> +		return NULL;

You could with little effort, support virtual monitor interfaces by just
passing all frames the firmware gives you to those. Probably not worth
bothering with though.

Other notes:
 * you could easily migrate over orinoco_ioctl_setmode/getmode
 * if we improve the cfg80211 iwrange implementation a little
   and you register which ciphers you support, you can migrate
   over orinoco_ioctl_getiwrange
 * iwencode(ext) should be fairly easy too
 * just get rid of iwnick
 * rts/frag/retry should be easy to migrate
 * what are ibss ports?
 * scan should be easy to port too
 * iwpriv isn't supported by cfg80211

Anyway looks good for a start, though I would maybe build the structures
a little different and not put the wdev into priv since priv is per
hardware.

Generally the layout I recommend would be like this:

+-------------+
|  wiphy      |
+-------------+
|  priv       |  == wiphy_priv()
+-------------+

and

+-------------+
|  netdev     |
+-------------+
|  wdev       |  == netdev_priv()
+-------------+

For you it doesn't really matter since you can only have one virtual
interface (well in monitor mode you could have multiple but that seems
pointless) -- it's still more in line with what other drivers are doing
though.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]