Re: [RFC PATCH 3/3] cfg80211: add wext-compatible client

[Johannes Berg <johannes@sipsolutions.net>]

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

On Wed, 2007-02-07 at 07:54 +0000, Christoph Hellwig wrote:

> Do we really have to add this directly to struct net_device and bloat it
> for all users?  I'd put this behind  ieee80211_ptr instead unless there's
> a good reason against that.

Well, the reason currently is that cfg80211 treats ieee80211_ptr as an
opaque cookie, it doesn't need to know what it points to.

Of course, I intend to change that too...

> > +config CFG80211_WEXTNL_COMPAT
> > +	bool "cfg80211 WE-netlink compatibility"
> > +	depends CFG80211 && CFG80211_WEXT_COMPAT
> > +	---help---
> > +	This option allows using devices whose drivers have been
> > +	converted to use the new cfg80211 with wireless extensions
> > +	over rtnetlink, providing WE-20 compatibility. Note that
> > +	cfg80211's "native" interface is nl80211 using generic netlink.
> > +	The wireless extensions are being deprecated and the netlink
> > +	based API for WE was never configured by default, nor do any
> > +	userspace tools use this feature.
> > +
> > +	This option exists only to make Jean happy. Say N.
> 
> Should we really put this code in?  It seems like unessecary bloat to me.

Yeah. Jean disappeared anyway, I'll kill it.

> > @@ -2798,6 +2799,39 @@ int dev_ioctl(unsigned int cmd, void __user *arg)
> >  					ret = -EFAULT;
> >  				return ret;
> >  			}
> > +#ifdef CONFIG_CFG80211_WEXT_COMPAT
> > +			/* Take care of cfg80211 WE compatibility */
> > +			if (cmd >= SIOCIWFIRST && cmd <= SIOCIWLAST) {
> > +				/* If command is `set a parameter', or
> > +				 * `get the encoding parameters', check if
> > +				 * the user has the right to do it */
> > +				if (IW_IS_SET(cmd) || cmd == SIOCGIWENCODE
> > +				    || cmd == SIOCGIWENCODEEXT) {
> > +					if (!capable(CAP_NET_ADMIN))
> > +						return -EPERM;
> > +				}
> > +				dev_load(ifr.ifr_name);
> > +				rtnl_lock();
> > +				/* Follow me in net/wireless/wext-compat.c */
> > +				ret = cfg80211_wext_ioctl(&ifr, cmd);
> > +				rtnl_unlock();
> > +				if (ret == 0 && IW_IS_GET(cmd) &&
> > +				    copy_to_user(arg, &ifr,
> > +						 sizeof(struct ifreq)))
> > +					ret = -EFAULT;
> > +				/* haha, I cheat here by allowing a driver or
> > +				 * stack to have both WE or CFG80211-WE for
> > +				 * a little while during conversion... hope that
> > +				 * ENOSYS is only used to indicate not implemented
> > +				 *
> > +				 * if wireless extensions are not configured
> > +				 * then this is the last thing here so that
> > +				 * if we fall through we return -EINVAL
> > +				 */
> > +				if (ret != -ENOSYS)
> > +					return ret;
> > +			}
> 
> Can we split this into a nice little helper function in wext-compat.c
> or a dummy macro if it's not configured?

I guess I can do that. Maybe I'll do the same with wext too.

> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -56,6 +56,9 @@
> >  #include <linux/wireless.h>
> >  #include <net/iw_handler.h>
> >  #endif	/* CONFIG_NET_WIRELESS_RTNETLINK */
> > +#ifdef CONFIG_CFG80211_WEXTNL_COMPAT
> > +#include <net/cfg80211.h>
> > +#endif
> 
> There's not point in including headers conditionally.

It helps ccache when you change that header, but yeah, let's just
include it.

> This looks odd. Kbuild filters out duplicate objects, so something like
> the snipplet below should work aswell:
> 
> obj-$(CONFIG_WIRELESS_EXT)		+= wext-common.o wext-old.o
> obj-$(CONFIG_CFG80211_WEXT_COMPAT)	+= wext-common.o wext-compat.o	

Hah, good point, I never even thought of something that would just
duplicate it.

> > +#ifdef CONFIG_CFG80211_WEXT_COMPAT
> > +/* wext compatibility must be compiled in...
> > + * this extern is in wext-compat.c */
> 
> Is there really a point in making cfg80211 a loadable module?
> This whle operation vector stuff looks like a lot of unnessecary
> complexity to me.

I think there is point in doing it as it allows us to replace it with an
out-of-tree newer version for getting it into the hands of users who
might not want to replace their whole kernel.

> > new file mode 100644
> > index 0000000..c1de1d7
> > --- /dev/null
> > +++ b/net/wireless/wext-common.c
> > @@ -0,0 +1,610 @@
> > +/* common wext support routines, proc interface and events */
> 
> This is mostly Jean's old code, right?  Probably wants at least some
> attribution.

Yeah, good point, it's mostly code that moved from that file to here.

> Btw, if we submit cfg80211 for inclusion there should be at least one patch
> in the series actually converts a driver to use it.  Both to make this
> code not dead and give people and example on how to use the cfg80211
> infrastructure.

I guess I could make softmac use it.

johannes

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