Re: [PATCH] orinoco: enable cfg80211 "set_channel" operation

Re: [PATCH] orinoco: enable cfg80211 "set_channel" operation

[Dave]

Holger Schurig wrote:
> Signed-off-by: Holger Schurig <hs4233@mail.mn-solutions.de>

Cool.

> +	if (orinoco_lock(priv, &flags) != 0)
> +		return -EBUSY;
> +
> +	priv->channel = channel;
> +	if (priv->iw_mode == NL80211_IFTYPE_MONITOR) {
> +		/* Fast channel change - no commit if successful */
> +		hermes_t *hw = &priv->hw;
> +		err = hermes_docmd_wait(hw, HERMES_CMD_TEST |
> +					    HERMES_TEST_SET_CHANNEL,
> +					channel, NULL);
> +	}
> +	orinoco_unlock(priv, &flags);
> +
> +	return err;
> +}

Looks right for monitor mode (as you tested), but for ad-hoc the channel
change would be delayed until the next SIOCSIWCOMMIT. Which for cfg80211
is currently only on change_vif.

Adding a call to orinoco_commit(priv) in the ad-hoc case should push the
change to the card. I think we'll end up with this call in each of the
orinoco cfg80211 functions. Otherwise we have to work out how to do the
changes incrementally across the different hw/fw.

You can also eliminate orinoco_ioctl_setfreq from wext.c with this change.


Thanks,

Dave.

Re: [PATCH] orinoco: enable cfg80211 "set_channel" operation

[Johannes Berg]

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

On Tue, 2009-07-28 at 17:27 +0100, Dave wrote:

> Looks right for monitor mode (as you tested), but for ad-hoc the channel
> change would be delayed until the next SIOCSIWCOMMIT. Which for cfg80211
> is currently only on change_vif.

You won't be getting this call for ad-hoc mode, because then the channel
is contained in the ibss_join() parameters.

johannes

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

Re: [PATCH] orinoco: enable cfg80211 "set_channel" operation

[Johannes Berg]

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

On Tue, 2009-07-28 at 18:29 +0200, Johannes Berg wrote:
> On Tue, 2009-07-28 at 17:27 +0100, Dave wrote:
> 
> > Looks right for monitor mode (as you tested), but for ad-hoc the channel
> > change would be delayed until the next SIOCSIWCOMMIT. Which for cfg80211
> > is currently only on change_vif.
> 
> You won't be getting this call for ad-hoc mode, because then the channel
> is contained in the ibss_join() parameters.


> > You can also eliminate orinoco_ioctl_setfreq from wext.c with this
> > change.

That also means you can't eliminate that, of course, since for connect()
the channels is also contained in the parameters, and the set_freq isn't
invoked.

johannes

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

Re: [PATCH] orinoco: enable cfg80211 "set_channel" operation

[Dave]

Johannes Berg wrote:
> On Tue, 2009-07-28 at 18:29 +0200, Johannes Berg wrote:
>> On Tue, 2009-07-28 at 17:27 +0100, Dave wrote:
>>
>>> Looks right for monitor mode (as you tested), but for ad-hoc the channel
>>> change would be delayed until the next SIOCSIWCOMMIT. Which for cfg80211
>>> is currently only on change_vif.
>> You won't be getting this call for ad-hoc mode, because then the channel
>> is contained in the ibss_join() parameters.
> 
> 
>>> You can also eliminate orinoco_ioctl_setfreq from wext.c with this
>>> change.
> 
> That also means you can't eliminate that, of course, since for connect()
> the channels is also contained in the parameters, and the set_freq isn't
> invoked.

Ah. Thanks for the corrections.


Dave.

Re: [PATCH] orinoco: enable cfg80211 "set_channel" operation

[Holger Schurig]

On Tuesday 28 July 2009 18:27:54 Dave wrote:
> You can also eliminate orinoco_ioctl_setfreq from wext.c with
> this change.

You mean this ?!?

--- linux-wl.orig/drivers/net/wireless/orinoco/wext.c
+++ linux-wl/drivers/net/wireless/orinoco/wext.c
@@ -372,55 +372,6 @@
 	return 0;
 }
 
-static int orinoco_ioctl_setfreq(struct net_device *dev,
-				 struct iw_request_info *info,
-				 struct iw_freq *frq,
-				 char *extra)
-{
-	struct orinoco_private *priv = ndev_priv(dev);
-	int chan = -1;
-	unsigned long flags;
-	int err = -EINPROGRESS;		/* Call commit handler */
-
-	/* In infrastructure mode the AP sets the channel */
-	if (priv->iw_mode == NL80211_IFTYPE_STATION)
-		return -EBUSY;
-
-	if ((frq->e == 0) && (frq->m <= 1000)) {
-		/* Setting by channel number */
-		chan = frq->m;
-	} else {
-		/* Setting by frequency */
-		int denom = 1;
-		int i;
-
-		/* Calculate denominator to rescale to MHz */
-		for (i = 0; i < (6 - frq->e); i++)
-			denom *= 10;
-
-		chan = ieee80211_freq_to_dsss_chan(frq->m / denom);
-	}
-
-	if ((chan < 1) || (chan > NUM_CHANNELS) ||
-	     !(priv->channel_mask & (1 << (chan-1))))
-		return -EINVAL;
-
-	if (orinoco_lock(priv, &flags) != 0)
-		return -EBUSY;
-
-	priv->channel = chan;
-	if (priv->iw_mode == NL80211_IFTYPE_MONITOR) {
-		/* Fast channel change - no commit if successful */
-		hermes_t *hw = &priv->hw;
-		err = hermes_docmd_wait(hw, HERMES_CMD_TEST |
-					    HERMES_TEST_SET_CHANNEL,
-					chan, NULL);
-	}
-	orinoco_unlock(priv, &flags);
-
-	return err;
-}
-
 static int orinoco_ioctl_getfreq(struct net_device *dev,
 				 struct iw_request_info *info,
 				 struct iw_freq *frq,
@@ -1481,7 +1432,6 @@
 static const iw_handler	orinoco_handler[] = {
 	STD_IW_HANDLER(SIOCSIWCOMMIT,	orinoco_ioctl_commit),
 	STD_IW_HANDLER(SIOCGIWNAME,	cfg80211_wext_giwname),
-	STD_IW_HANDLER(SIOCSIWFREQ,	orinoco_ioctl_setfreq),
 	STD_IW_HANDLER(SIOCGIWFREQ,	orinoco_ioctl_getfreq),
 	STD_IW_HANDLER(SIOCSIWMODE,	cfg80211_wext_siwmode),
 	STD_IW_HANDLER(SIOCGIWMODE,	cfg80211_wext_giwmode),

I tried, but with then an "iwconfig eth1 channel 1" gave me an 
error. I did not investigate this further.

-- 
http://www.holgerschurig.de

Re: [PATCH] orinoco: enable cfg80211 "set_channel" operation

[Dave]

Holger Schurig wrote:
> On Tuesday 28 July 2009 18:27:54 Dave wrote:
>> You can also eliminate orinoco_ioctl_setfreq from wext.c with
>> this change.
> 
> You mean this ?!?

Yes, except:

> @@ -1481,7 +1432,6 @@
>  static const iw_handler	orinoco_handler[] = {
>  	STD_IW_HANDLER(SIOCSIWCOMMIT,	orinoco_ioctl_commit),
>  	STD_IW_HANDLER(SIOCGIWNAME,	cfg80211_wext_giwname),
> -	STD_IW_HANDLER(SIOCSIWFREQ,	orinoco_ioctl_setfreq),
+	STD_IW_HANDLER(SIOCSIWFREQ,	cfg80211_wext_setfreq ish),

>  	STD_IW_HANDLER(SIOCGIWFREQ,	orinoco_ioctl_getfreq),
>  	STD_IW_HANDLER(SIOCSIWMODE,	cfg80211_wext_siwmode),
>  	STD_IW_HANDLER(SIOCGIWMODE,	cfg80211_wext_giwmode),
> 
> I tried, but with then an "iwconfig eth1 channel 1" gave me an 
> error. I did not investigate this further.

And as Johannes pointed out, we need to have the non-cfg80211 version so
adhoc connections can still be made via wext.

So the original patch is fine as is. Ack from me.


Dave.