Skip to contentSkip to conversation

various wext cleanups

PATCH In review Johannes Berg 8 participants 27 messages Apr 24, 2007 – Apr 27, 2007 9 messages not in the archive

Subsystems (3)
Networking Network drivers 802.11 (including cfg80211/NL80211)
lore
Share & tools
lore

Johannes Berg

author maintainer
#0 lore raw

cover various wext cleanups

This patch series contains various cleanups to the wireless extensions code.

johannes

#1 lore raw

PATCH 2/9 wext: clean up how wext is called

This patch cleans up the call paths from the core code into wext.

Signed-off-by Johannes Berg <[email protected]>
---
 include/net/iw_handler.h |   11 +----------
 include/net/wext.h       |   24 ++++++++++++++++++++++++
 net/core/dev.c           |   34 ++++------------------------------
 net/wireless/wext.c      |   28 ++++++++++++++++++++++++----
 4 files changed, 53 insertions(+), 44 deletions(-)
include/net/wext.h new file +24 -0
@@ -0,0 +1,24 @@
+#ifndef __NET_WEXT_H
+#define __NET_WEXT_H
+
+/*
+ * wireless extensions interface to the core code
+ */
+
+#ifdef CONFIG_WIRELESS_EXT
+extern int wext_proc_init(void);
+extern int wext_handle_ioctl(struct ifreq *ifr, unsigned int cmd,
+			     void __user *arg);
+#else
+static inline int wext_proc_init()
+{
+	return 0;
+}
+static inline int wext_handle_ioctl(struct ifreq *ifr, unsigned int cmd,
+				    void __user *arg)
+{
+	return -EINVAL;
+}
+#endif
+
+#endif /* __NET_WEXT_H */
net/core/dev.c +4 -30
@@ -109,7 +109,7 @@
 #include <linux/netpoll.h>
 #include <linux/rcupdate.h>
 #include <linux/delay.h>
-#include <linux/wireless.h>
+#include <net/wext.h>
 #include <net/iw_handler.h>
 #include <asm/current.h>
 #include <linux/audit.h>

@@ -2348,12 +2348,6 @@ static const struct file_operations ptyp
 };
 
 
-#ifdef CONFIG_WIRELESS_EXT
-extern int wireless_proc_init(void);
-#else
-#define wireless_proc_init() 0
-#endif
-
 static int __init dev_proc_init(void)
 {
 	int rc = -ENOMEM;

@@ -2365,7 +2359,7 @@ static int __init dev_proc_init(void)
 	if (!proc_net_fops_create("ptype", S_IRUGO, &ptype_seq_fops))
 		goto out_dev2;
 
-	if (wireless_proc_init())
+	if (wext_proc_init())
 		goto out_softnet;
 	rc = 0;
 out:

@@ -2923,29 +2917,9 @@ int dev_ioctl(unsigned int cmd, void __u
 					ret = -EFAULT;
 				return ret;
 			}
-#ifdef CONFIG_WIRELESS_EXT
 			/* Take care of Wireless Extensions */
-			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.c */
-				ret = wireless_process_ioctl(&ifr, cmd);
-				rtnl_unlock();
-				if (IW_IS_GET(cmd) &&
-				    copy_to_user(arg, &ifr,
-						 sizeof(struct ifreq)))
-					ret = -EFAULT;
-				return ret;
-			}
-#endif	/* CONFIG_WIRELESS_EXT */
+			if (cmd >= SIOCIWFIRST && cmd <= SIOCIWLAST)
+				return wext_handle_ioctl(&ifr, cmd, arg);
 			return -EINVAL;
 	}
 }
include/net/iw_handler.h +1 -10
@@ -431,16 +431,7 @@ struct iw_public_data {
  * Those may be called only within the kernel.
  */
 
-/* First : function strictly used inside the kernel */
-
-/* Handle /proc/net/wireless, called in net/code/dev.c */
-extern int dev_get_wireless_info(char * buffer, char **start, off_t offset,
-				 int length);
-
-/* Handle IOCTLs, called in net/core/dev.c */
-extern int wireless_process_ioctl(struct ifreq *ifr, unsigned int cmd);
-
-/* Second : functions that may be called by driver modules */
+/* functions that may be called by driver modules */
 
 /* Send a single event to user space */
 extern void wireless_send_event(struct net_device *	dev,
net/wireless/wext.c +24 -4
@@ -97,6 +97,7 @@
 #include <linux/wireless.h>		/* Pretty obvious */
 #include <net/iw_handler.h>		/* New driver API */
 #include <net/netlink.h>
+#include <net/wext.h>
 
 #include <asm/uaccess.h>		/* copy_to_user() */
 

@@ -696,7 +697,7 @@ static const struct file_operations wire
 	.release = seq_release,
 };
 
-int __init wireless_proc_init(void)
+int __init wext_proc_init(void)
 {
 	/* Create /proc/net/wireless entry */
 	if (!proc_net_fops_create("wireless", S_IRUGO, &wireless_seq_fops))

@@ -1075,11 +1076,10 @@ static inline int ioctl_private_call(str
 
 /* ---------------------------------------------------------------- */
 /*
- * Main IOCTl dispatcher. Called from the main networking code
- * (dev_ioctl() in net/core/dev.c).
+ * Main IOCTl dispatcher.
  * Check the type of IOCTL and call the appropriate wrapper...
  */
-int wireless_process_ioctl(struct ifreq *ifr, unsigned int cmd)
+static int wireless_process_ioctl(struct ifreq *ifr, unsigned int cmd)
 {
 	struct net_device *dev;
 	iw_handler	handler;

@@ -1143,6 +1143,26 @@ int wireless_process_ioctl(struct ifreq 
 	return -EINVAL;
 }
 
+/* entry point from dev ioctl */
+int wext_handle_ioctl(struct ifreq *ifr, unsigned int cmd,
+		      void __user *arg)
+{
+	int ret;
+
+	/* 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();
+	ret = wireless_process_ioctl(ifr, cmd);
+	rtnl_unlock();
+	if (IW_IS_GET(cmd) && copy_to_user(arg, ifr, sizeof(struct ifreq)))
+		return -EFAULT;
+	return ret;
+}
 
 /************************* EVENT PROCESSING *************************/
 /*

--

#2 lore raw

PATCH 4/9 wext: remove options

In reply to Johannes Berg

This patch kills the two options in wext that are required to be enabled anyway because they influence the userspace API.

Signed-off-by Johannes Berg <[email protected]>
---
 net/wireless/wext.c |   15 ---------------
 1 file changed, 15 deletions(-)
net/wireless/wext.c +0 -15
@@ -101,12 +101,6 @@
 
 #include <asm/uaccess.h>		/* copy_to_user() */
 
-/**************************** CONSTANTS ****************************/
-
-/* Options */
-#define WE_EVENT_RTNETLINK	/* Propagate events using RtNetlink */
-#define WE_SET_EVENT		/* Generate an event on some set commands */
-
 /************************* GLOBAL VARIABLES *************************/
 /*
  * You should not use global variables, because of re-entrancy.

@@ -741,12 +735,10 @@ static int ioctl_standard_call(struct ne
 		/* No extra arguments. Trivial to handle */
 		ret = handler(dev, &info, &(iwr->u), NULL);
 
-#ifdef WE_SET_EVENT
 		/* Generate an event to notify listeners of the change */
 		if ((descr->flags & IW_DESCR_FLAG_EVENT) &&
 		   ((ret == 0) || (ret == -EIWCOMMIT)))
 			wireless_send_event(dev, cmd, &(iwr->u), NULL);
-#endif	/* WE_SET_EVENT */
 	} else {
 		char *	extra;
 		int	extra_size;

@@ -859,7 +851,6 @@ static int ioctl_standard_call(struct ne
 				ret =  -EFAULT;
 		}
 
-#ifdef WE_SET_EVENT
 		/* Generate an event to notify listeners of the change */
 		if ((descr->flags & IW_DESCR_FLAG_EVENT) &&
 		   ((ret == 0) || (ret == -EIWCOMMIT))) {

@@ -871,7 +862,6 @@ static int ioctl_standard_call(struct ne
 				wireless_send_event(dev, cmd, &(iwr->u),
 						    extra);
 		}
-#endif	/* WE_SET_EVENT */
 
 		/* Cleanup - I told you it wasn't that long ;-) */
 		kfree(extra);

@@ -1121,7 +1111,6 @@ int wext_handle_ioctl(struct ifreq *ifr,
  * Most often, the event will be propagated through rtnetlink
  */
 
-#ifdef WE_EVENT_RTNETLINK
 /* ---------------------------------------------------------------- */
 /*
  * Locking...

@@ -1225,8 +1214,6 @@ static inline void rtmsg_iwinfo(struct n
 	tasklet_schedule(&wireless_nlevent_tasklet);
 }
 
-#endif	/* WE_EVENT_RTNETLINK */
-
 /* ---------------------------------------------------------------- */
 /*
  * Main event dispatcher. Called from other parts and drivers.

@@ -1305,10 +1292,8 @@ void wireless_send_event(struct net_devi
 	if (extra != NULL)
 		memcpy(((char *) event) + hdr_len, extra, extra_len);
 
-#ifdef WE_EVENT_RTNETLINK
 	/* Send via the RtNetlink event channel */
 	rtmsg_iwinfo(dev, (char *) event, event_len);
-#endif	/* WE_EVENT_RTNETLINK */
 
 	/* Cleanup */
 	kfree(event);

--

#3 lore raw

PATCH 5/9 wext: cleanup early ioctl call path

In reply to Johannes Berg

This patch makes the code in wireless_process_ioctl somewhat more readable.

Signed-off-by Johannes Berg <[email protected]>
---
 net/wireless/wext.c |   73 ++++++++++++++++++----------------------------------
 1 file changed, 26 insertions(+), 47 deletions(-)
net/wireless/wext.c +26 -47
@@ -1035,53 +1035,31 @@ static int wireless_process_ioctl(struct
 	/* A bunch of special cases, then the generic case...
 	 * Note that 'cmd' is already filtered in dev_ioctl() with
 	 * (cmd >= SIOCIWFIRST && cmd <= SIOCIWLAST) */
-	switch (cmd) {
-	case SIOCGIWSTATS:
-		/* Get Wireless Stats */
-		return ioctl_standard_call(dev,
-					   ifr,
-					   cmd,
+	if (cmd == SIOCGIWSTATS)
+		return ioctl_standard_call(dev, ifr, cmd,
 					   &iw_handler_get_iwstats);
 
-	case SIOCGIWPRIV:
-		/* Check if we have some wireless handlers defined */
-		if (dev->wireless_handlers != NULL) {
-			/* We export to user space the definition of
-			 * the private handler ourselves */
-			return ioctl_standard_call(dev,
-						   ifr,
-						   cmd,
-						   &iw_handler_get_private);
-		}
-		// ## Fall-through for old API ##
-	default:
-		/* Generic IOCTL */
-		/* Basic check */
-		if (!netif_device_present(dev))
-			return -ENODEV;
-		/* New driver API : try to find the handler */
-		handler = get_handler(dev, cmd);
-		if (handler != NULL) {
-			/* Standard and private are not the same */
-			if (cmd < SIOCIWFIRSTPRIV)
-				return ioctl_standard_call(dev,
-							   ifr,
-							   cmd,
-							   handler);
-			else
-				return ioctl_private_call(dev,
-							  ifr,
-							  cmd,
-							  handler);
-		}
-		/* Old driver API : call driver ioctl handler */
-		if (dev->do_ioctl) {
-			return dev->do_ioctl(dev, ifr, cmd);
-		}
-		return -EOPNOTSUPP;
+	if (cmd == SIOCGIWPRIV && dev->wireless_handlers)
+		return ioctl_standard_call(dev, ifr, cmd,
+					   &iw_handler_get_private);
+
+	/* Basic check */
+	if (!netif_device_present(dev))
+		return -ENODEV;
+
+	/* New driver API : try to find the handler */
+	handler = get_handler(dev, cmd);
+	if (handler) {
+		/* Standard and private are not the same */
+		if (cmd < SIOCIWFIRSTPRIV)
+			return ioctl_standard_call(dev, ifr, cmd, handler);
+		else
+			return ioctl_private_call(dev, ifr, cmd, handler);
 	}
-	/* Not reached */
-	return -EINVAL;
+	/* Old driver API : call driver ioctl handler */
+	if (dev->do_ioctl)
+		return dev->do_ioctl(dev, ifr, cmd);
+	return -EOPNOTSUPP;
 }
 
 /* entry point from dev ioctl */

@@ -1093,9 +1071,10 @@ int wext_handle_ioctl(struct ifreq *ifr,
 	/* 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;
+	if ((IW_IS_SET(cmd) || cmd == SIOCGIWENCODE || cmd == SIOCGIWENCODEEXT)
+	    && !capable(CAP_NET_ADMIN))
+		return -EPERM;
+
 	dev_load(ifr->ifr_name);
 	rtnl_lock();
 	ret = wireless_process_ioctl(ifr, cmd);

--

#4 lore raw

PATCH 6/9 wext: move EXPORT_SYMBOL statements where they belong

In reply to Johannes Berg

EXPORT_SYMBOL statements are supposed to go together with the symbol they're exporting. This patch moves them accordingly.

Signed-off-by Johannes Berg <[email protected]>
---
 net/wireless/wext.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
net/wireless/wext.c +5 -6
@@ -1279,6 +1279,7 @@ void wireless_send_event(struct net_devi
 
 	return;		/* Always success, I guess ;-) */
 }
+EXPORT_SYMBOL(wireless_send_event);
 
 /********************** ENHANCED IWSPY SUPPORT **********************/
 /*

@@ -1356,6 +1357,7 @@ int iw_handler_set_spy(struct net_device
 
 	return 0;
 }
+EXPORT_SYMBOL(iw_handler_set_spy);
 
 /*------------------------------------------------------------------*/
 /*

@@ -1391,6 +1393,7 @@ int iw_handler_get_spy(struct net_device
 		spydata->spy_stat[i].updated &= ~IW_QUAL_ALL_UPDATED;
 	return 0;
 }
+EXPORT_SYMBOL(iw_handler_get_spy);
 
 /*------------------------------------------------------------------*/
 /*

@@ -1417,6 +1420,7 @@ int iw_handler_set_thrspy(struct net_dev
 
 	return 0;
 }
+EXPORT_SYMBOL(iw_handler_set_thrspy);
 
 /*------------------------------------------------------------------*/
 /*

@@ -1440,6 +1444,7 @@ int iw_handler_get_thrspy(struct net_dev
 
 	return 0;
 }
+EXPORT_SYMBOL(iw_handler_get_thrspy);
 
 /*------------------------------------------------------------------*/
 /*

@@ -1516,10 +1521,4 @@ void wireless_spy_update(struct net_devi
 		}
 	}
 }
-
-EXPORT_SYMBOL(iw_handler_get_spy);
-EXPORT_SYMBOL(iw_handler_get_thrspy);
-EXPORT_SYMBOL(iw_handler_set_spy);
-EXPORT_SYMBOL(iw_handler_set_thrspy);
-EXPORT_SYMBOL(wireless_send_event);
 EXPORT_SYMBOL(wireless_spy_update);

--

#5 lore raw

PATCH 8/9 wext: misc code cleanups

In reply to Johannes Berg

Just a few things that didn't fit in with the other patches.

Signed-off-by Johannes Berg <[email protected]>
---
 net/wireless/wext.c |   28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)
net/wireless/wext.c +11 -17
@@ -476,7 +476,7 @@ static struct iw_statistics *get_wireles
 		return dev->wireless_handlers->get_wireless_stats(dev);
 
 	/* Not found */
-	return (struct iw_statistics *) NULL;
+	return NULL;
 }
 
 /* ---------------------------------------------------------------- */

@@ -501,11 +501,11 @@ static struct iw_statistics *get_wireles
 static int call_commit_handler(struct net_device *dev)
 {
 	if ((netif_running(dev)) &&
-	   (dev->wireless_handlers->standard[0] != NULL)) {
+	   (dev->wireless_handlers->standard[0] != NULL))
 		/* Call the commit handler on the driver */
 		return dev->wireless_handlers->standard[0](dev, NULL,
 							   NULL, NULL);
-	} else
+	else
 		return 0;		/* Command completed successfully */
 }
 

@@ -554,8 +554,7 @@ static int iw_handler_get_iwstats(struct
 	struct iw_statistics *stats;
 
 	stats = get_wireless_stats(dev);
-	if (stats != (struct iw_statistics *) NULL) {
-
+	if (stats) {
 		/* Copy statistics to extra */
 		memcpy(extra, stats, sizeof(struct iw_statistics));
 		wrqu->data.length = sizeof(struct iw_statistics);

@@ -814,9 +813,8 @@ static int ioctl_standard_call(struct ne
 		/* Create the kernel buffer */
 		/*    kzalloc ensures NULL-termination for essid_compat */
 		extra = kzalloc(extra_size, GFP_KERNEL);
-		if (extra == NULL) {
+		if (extra == NULL)
 			return -ENOMEM;
-		}
 
 		/* If it is a SET, get all the extra data in here */
 		if (IW_IS_SET(cmd) && (iwr->u.data.length != 0)) {

@@ -957,18 +955,14 @@ static int ioctl_private_call(struct net
 			if (iwr->u.data.length > (descr->set_args &
 						 IW_PRIV_SIZE_MASK))
 				return -E2BIG;
-		} else {
-			/* Check NULL pointer */
-			if (iwr->u.data.pointer == NULL)
-				return -EFAULT;
-		}
+		} else if (iwr->u.data.pointer == NULL)
+			return -EFAULT;
 
 		/* Always allocate for max space. Easier, and won't last
 		 * long... */
 		extra = kmalloc(extra_size, GFP_KERNEL);
-		if (extra == NULL) {
+		if (extra == NULL)
 			return -ENOMEM;
-		}
 
 		/* If it is a SET, get all the extra data in here */
 		if (IW_IS_SET(cmd) && (iwr->u.data.length != 0)) {

@@ -1259,7 +1253,7 @@ void wireless_send_event(struct net_devi
 	event->len = event_len;
 	event->cmd = cmd;
 	memcpy(&event->u, ((char *) wrqu) + wrqu_off, hdr_len - IW_EV_LCP_LEN);
-	if (extra != NULL)
+	if (extra)
 		memcpy(((char *) event) + hdr_len, extra, extra_len);
 
 	/* Send via the RtNetlink event channel */

@@ -1290,11 +1284,11 @@ EXPORT_SYMBOL(wireless_send_event);
  * Because this is called on the Rx path via wireless_spy_update(),
  * we want it to be efficient...
  */
-static inline struct iw_spy_data * get_spydata(struct net_device *dev)
+static inline struct iw_spy_data *get_spydata(struct net_device *dev)
 {
 	/* This is the new way */
 	if (dev->wireless_data)
-		return(dev->wireless_data->spy_data);
+		return dev->wireless_data->spy_data;
 	return NULL;
 }

--

#6 lore raw

PATCH 9/9 net_device: dont include wext bits if not required

In reply to Johannes Berg

This patch makes the wext bits in struct net_device depend on CONFIG_WIRELESS_EXT.

Signed-off-by Johannes Berg <[email protected]>
---
 include/linux/netdevice.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
include/linux/netdevice.h +2 -1
@@ -352,12 +352,13 @@ struct net_device
 	struct net_device_stats* (*get_stats)(struct net_device *dev);
 	struct net_device_stats	stats;
 
+#ifdef CONFIG_WIRELESS_EXT
 	/* List of functions to handle Wireless Extensions (instead of ioctl).
 	 * See <net/iw_handler.h> for details. Jean II */
 	const struct iw_handler_def *	wireless_handlers;
 	/* Instance data managed by the core of Wireless Extensions. */
 	struct iw_public_data *	wireless_data;
-
+#endif
 	const struct ethtool_ops *ethtool_ops;
 
 	/*

--

A message that is not in the archive (sent off-list or not yet received): [email protected]

#8 lore raw

Re: [PATCH 9/9] net_device: dont include wext bits if not required

Quoted 4 lines · Johannes Berg

This patch makes the wext bits in struct net_device depend on CONFIG_WIRELESS_EXT.

Signed-off-by Johannes Berg <[email protected]>

I personally would not do that. Having conditional fields in
"struct net_device" is very bad, it's a sure way to crash on modules in some cases. If I remember well, Jeff Garzik has been fighting those over the years.

Jean

author maintainer
#9 lore raw

Re: [PATCH 9/9] net_device: dont include wext bits if not required

Quoted 4 lines · Jean Tourrilhes

I personally would not do that. Having conditional fields in
"struct net_device" is very bad, it's a sure way to crash on modules in some cases. If I remember well, Jeff Garzik has been fighting those over the years.

I'm fine with posting a patch the other way around (i.e. removing the conditional from wireless-dev) just wanted to float this. It originally made much more sense anyway when I had wanted to do cfg80211/wext compatibility in a different way. Now it looks like it'll just depend on wext.

johannes

A message that is not in the archive (sent off-list or not yet received): 20070426165304.GC22576-yAE0UhLNZJawPNPzzlOzwdBPR1lH4CV8@public.gmane.org

#11 lore raw

Re: [PATCH 9/9] net_device: dont include wext bits if not required

Quoted 10 lines · Jean Tourrilhes
Quoted 4 lines · Johannes Berg

This patch makes the wext bits in struct net_device depend on CONFIG_WIRELESS_EXT.

Signed-off-by Johannes Berg <[email protected]>

I personally would not do that. Having conditional fields in
"struct net_device" is very bad, it's a sure way to crash on modules in some cases. If I remember well, Jeff Garzik has been fighting those over the years.

I'm not sure I understand the reasoning here. Without a doubt kernel modules need to be built against the same configuration as the kernels that are loading them. So, conditional component of net_device should not cause crashes.

FWIW, there are other conditionally compiled portions of net_device (NETPOLL, NET_POLL_CONTROLLER). I don't see the harm here.

John

Signature · 2 lines
John W. Linville
[email protected]
author maintainer
#12 lore raw

PATCH 1/9 wext: move to net/wireless

In reply to Johannes Berg

This patch moves dev/core/wireless.c to net/wireless/wext.c.

Signed-off-by Johannes Berg <[email protected]>
---
 net/core/Makefile     |    1 
 net/core/dev.c        |    2 
 net/core/wireless.c   | 1633 --------------------------------------------------
 net/wireless/Makefile |    1 
 net/wireless/wext.c   | 1633 ++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 1635 insertions(+), 1635 deletions(-)
net/wireless/Makefile +1 -0
@@ -1,3 +1,4 @@
+obj-$(CONFIG_WIRELESS_EXT) += wext.o
 obj-$(CONFIG_CFG80211) += cfg80211.o
 
 cfg80211-y += core.o sysfs.o
net/wireless/wext.c new file +1633 -0

1,633 lines not on this page · raw message

net/core/dev.c +1 -1
@@ -2936,7 +2936,7 @@ int dev_ioctl(unsigned int cmd, void __u
 				}
 				dev_load(ifr.ifr_name);
 				rtnl_lock();
-				/* Follow me in net/core/wireless.c */
+				/* Follow me in net/wireless/wext.c */
 				ret = wireless_process_ioctl(&ifr, cmd);
 				rtnl_unlock();
 				if (IW_IS_GET(cmd) &&
net/core/wireless.c deleted +0 -1633

1,355 lines not on this page · raw message

net/core/Makefile +0 -1

Lines of this file are not shown. View the raw message.

The rest of this diff is not shown. View the raw message.

--

Part of the diff is not shown here. View the raw message.

author maintainer
#13 lore raw

PATCH 3/9 wext: remove dead debug code

In reply to Johannes Berg

This patch kills a whole bunch of code that can only ever be used by defining some things in wext.c. Also, the things that are printed are mostly useless since the API is fairly well-tested.

Signed-off-by Johannes Berg <[email protected]>
---
 net/wireless/wext.c |   92 ----------------------------------------------------
 1 file changed, 92 deletions(-)
net/wireless/wext.c +0 -92
@@ -103,11 +103,6 @@
 
 /**************************** CONSTANTS ****************************/
 
-/* Debugging stuff */
-#undef WE_IOCTL_DEBUG		/* Debug IOCTL API */
-#undef WE_EVENT_DEBUG		/* Debug Event dispatcher */
-#undef WE_SPY_DEBUG		/* Debug enhanced spy support */
-
 /* Options */
 #define WE_EVENT_RTNETLINK	/* Propagate events using RtNetlink */
 #define WE_SET_EVENT		/* Generate an event on some set commands */

@@ -736,12 +731,6 @@ static int ioctl_standard_call(struct ne
 		return -EOPNOTSUPP;
 	descr = &(standard_ioctl[cmd - SIOCIWFIRST]);
 
-#ifdef WE_IOCTL_DEBUG
-	printk(KERN_DEBUG "%s (WE) : Found standard handler for 0x%04X\n",
-	       ifr->ifr_name, cmd);
-	printk(KERN_DEBUG "%s (WE) : Header type : %d, Token type : %d, size : %d, token : %d\n", dev->name, descr->header_type, descr->token_type, descr->token_size, descr->max_tokens);
-#endif	/* WE_IOCTL_DEBUG */
-
 	/* Prepare the call */
 	info.cmd = cmd;
 	info.flags = 0;

@@ -832,11 +821,6 @@ static int ioctl_standard_call(struct ne
 			}
 		}
 
-#ifdef WE_IOCTL_DEBUG
-		printk(KERN_DEBUG "%s (WE) : Malloc %d bytes\n",
-		       dev->name, extra_size);
-#endif	/* WE_IOCTL_DEBUG */
-
 		/* Create the kernel buffer */
 		/*    kzalloc ensures NULL-termination for essid_compat */
 		extra = kzalloc(extra_size, GFP_KERNEL);

@@ -853,11 +837,6 @@ static int ioctl_standard_call(struct ne
 				kfree(extra);
 				return -EFAULT;
 			}
-#ifdef WE_IOCTL_DEBUG
-			printk(KERN_DEBUG "%s (WE) : Got %d bytes\n",
-			       dev->name,
-			       iwr->u.data.length * descr->token_size);
-#endif	/* WE_IOCTL_DEBUG */
 		}
 
 		/* Call the handler */

@@ -878,11 +857,6 @@ static int ioctl_standard_call(struct ne
 					   descr->token_size);
 			if (err)
 				ret =  -EFAULT;
-#ifdef WE_IOCTL_DEBUG
-			printk(KERN_DEBUG "%s (WE) : Wrote %d bytes\n",
-			       dev->name,
-			       iwr->u.data.length * descr->token_size);
-#endif	/* WE_IOCTL_DEBUG */
 		}
 
 #ifdef WE_SET_EVENT

@@ -947,16 +921,6 @@ static inline int ioctl_private_call(str
 			break;
 		}
 
-#ifdef WE_IOCTL_DEBUG
-	printk(KERN_DEBUG "%s (WE) : Found private handler for 0x%04X\n",
-	       ifr->ifr_name, cmd);
-	if (descr) {
-		printk(KERN_DEBUG "%s (WE) : Name %s, set %X, get %X\n",
-		       dev->name, descr->name,
-		       descr->set_args, descr->get_args);
-	}
-#endif	/* WE_IOCTL_DEBUG */
-
 	/* Compute the size of the set/get arguments */
 	if (descr != NULL) {
 		if (IW_IS_SET(cmd)) {

@@ -1013,11 +977,6 @@ static inline int ioctl_private_call(str
 				return -EFAULT;
 		}
 
-#ifdef WE_IOCTL_DEBUG
-		printk(KERN_DEBUG "%s (WE) : Malloc %d bytes\n",
-		       dev->name, extra_size);
-#endif	/* WE_IOCTL_DEBUG */
-
 		/* Always allocate for max space. Easier, and won't last
 		 * long... */
 		extra = kmalloc(extra_size, GFP_KERNEL);

@@ -1033,10 +992,6 @@ static inline int ioctl_private_call(str
 				kfree(extra);
 				return -EFAULT;
 			}
-#ifdef WE_IOCTL_DEBUG
-			printk(KERN_DEBUG "%s (WE) : Got %d elem\n",
-			       dev->name, iwr->u.data.length);
-#endif	/* WE_IOCTL_DEBUG */
 		}
 
 		/* Call the handler */

@@ -1056,10 +1011,6 @@ static inline int ioctl_private_call(str
 					   extra_size);
 			if (err)
 				ret =  -EFAULT;
-#ifdef WE_IOCTL_DEBUG
-			printk(KERN_DEBUG "%s (WE) : Wrote %d elem\n",
-			       dev->name, iwr->u.data.length);
-#endif	/* WE_IOCTL_DEBUG */
 		}
 
 		/* Cleanup - I told you it wasn't that long ;-) */

@@ -1319,11 +1270,6 @@ void wireless_send_event(struct net_devi
 		       dev->name, cmd);
 		return;
 	}
-#ifdef WE_EVENT_DEBUG
-	printk(KERN_DEBUG "%s (WE) : Got event 0x%04X\n",
-	       dev->name, cmd);
-	printk(KERN_DEBUG "%s (WE) : Header type : %d, Token type : %d, size : %d, token : %d\n", dev->name, descr->header_type, descr->token_type, descr->token_size, descr->max_tokens);
-#endif	/* WE_EVENT_DEBUG */
 
 	/* Check extra parameters and set extra_len */
 	if (descr->header_type == IW_HEADER_TYPE_POINT) {

@@ -1341,19 +1287,12 @@ void wireless_send_event(struct net_devi
 			extra_len = wrqu->data.length * descr->token_size;
 		/* Always at an offset in wrqu */
 		wrqu_off = IW_EV_POINT_OFF;
-#ifdef WE_EVENT_DEBUG
-		printk(KERN_DEBUG "%s (WE) : Event 0x%04X, tokens %d, extra_len %d\n", dev->name, cmd, wrqu->data.length, extra_len);
-#endif	/* WE_EVENT_DEBUG */
 	}
 
 	/* Total length of the event */
 	hdr_len = event_type_size[descr->header_type];
 	event_len = hdr_len + extra_len;
 
-#ifdef WE_EVENT_DEBUG
-	printk(KERN_DEBUG "%s (WE) : Event 0x%04X, hdr_len %d, wrqu_off %d, event_len %d\n", dev->name, cmd, hdr_len, wrqu_off, event_len);
-#endif	/* WE_EVENT_DEBUG */
-
 	/* Create temporary buffer to hold the event */
 	event = kmalloc(event_len, GFP_ATOMIC);
 	if (event == NULL)

@@ -1443,19 +1382,6 @@ int iw_handler_set_spy(struct net_device
 		/* Reset stats */
 		memset(spydata->spy_stat, 0,
 		       sizeof(struct iw_quality) * IW_MAX_SPY);
-
-#ifdef WE_SPY_DEBUG
-		printk(KERN_DEBUG "iw_handler_set_spy() :  wireless_data %p, spydata %p, num %d\n", dev->wireless_data, spydata, wrqu->data.length);
-		for (i = 0; i < wrqu->data.length; i++)
-			printk(KERN_DEBUG
-			       "%02X:%02X:%02X:%02X:%02X:%02X \n",
-			       spydata->spy_address[i][0],
-			       spydata->spy_address[i][1],
-			       spydata->spy_address[i][2],
-			       spydata->spy_address[i][3],
-			       spydata->spy_address[i][4],
-			       spydata->spy_address[i][5]);
-#endif	/* WE_SPY_DEBUG */
 	}
 
 	/* Make sure above is updated before re-enabling */

@@ -1525,10 +1451,6 @@ int iw_handler_set_thrspy(struct net_dev
 	/* Clear flag */
 	memset(spydata->spy_thr_under, '\0', sizeof(spydata->spy_thr_under));
 
-#ifdef WE_SPY_DEBUG
-	printk(KERN_DEBUG "iw_handler_set_thrspy() :  low %d ; high %d\n", spydata->spy_thr_low.level, spydata->spy_thr_high.level);
-#endif	/* WE_SPY_DEBUG */
-
 	return 0;
 }
 

@@ -1579,16 +1501,6 @@ static void iw_send_thrspy_event(struct 
 	memcpy(&(threshold.low), &(spydata->spy_thr_low),
 	       2 * sizeof(struct iw_quality));
 
-#ifdef WE_SPY_DEBUG
-	printk(KERN_DEBUG "iw_send_thrspy_event() : address %02X:%02X:%02X:%02X:%02X:%02X, level %d, up = %d\n",
-	       threshold.addr.sa_data[0],
-	       threshold.addr.sa_data[1],
-	       threshold.addr.sa_data[2],
-	       threshold.addr.sa_data[3],
-	       threshold.addr.sa_data[4],
-	       threshold.addr.sa_data[5], threshold.qual.level);
-#endif	/* WE_SPY_DEBUG */
-
 	/* Send event to user space */
 	wireless_send_event(dev, SIOCGIWTHRSPY, &wrqu, (char *) &threshold);
 }

@@ -1612,10 +1524,6 @@ void wireless_spy_update(struct net_devi
 	if (!spydata)
 		return;
 
-#ifdef WE_SPY_DEBUG
-	printk(KERN_DEBUG "wireless_spy_update() :  wireless_data %p, spydata %p, address %02X:%02X:%02X:%02X:%02X:%02X\n", dev->wireless_data, spydata, address[0], address[1], address[2], address[3], address[4], address[5]);
-#endif	/* WE_SPY_DEBUG */
-
 	/* Update all records that match */
 	for (i = 0; i < spydata->spy_number; i++)
 		if (!compare_ether_addr(address, spydata->spy_address[i])) {

--

A message that is not in the archive (sent off-list or not yet received): [email protected]

#15 lore raw

Re: [PATCH 3/9] wext: remove dead debug code

Quoted 5 lines · Johannes Berg

This patch kills a whole bunch of code that can only ever be used by defining some things in wext.c. Also, the things that are printed are mostly useless since the API is fairly well-tested.

Signed-off-by Johannes Berg <[email protected]>

Hi,

Overall ok with the patches. Comments on a few of them... I personally would not remove this DEBUG code, because it
decreases the overall maitainability of the code.
First, it does not only help to debug the API, but it can be
used to debug userspace and drivers in some extreme cases.
Second, this debugging act as documentation. He points out
which are the important variables and how they are used at critical points of the code.
As this debug is localised, and compiled out, it does not
bother anybody and should stay.

Regards,

Jean

A message that is not in the archive (sent off-list or not yet received): 20070426164632.GA22576-yAE0UhLNZJawPNPzzlOzwdBPR1lH4CV8@public.gmane.org

#17 lore raw

Re: [PATCH 3/9] wext: remove dead debug code

Quoted 10 lines · Jean Tourrilhes

Overall ok with the patches. Comments on a few of them... I personally would not remove this DEBUG code, because it
decreases the overall maitainability of the code.
First, it does not only help to debug the API, but it can be
used to debug userspace and drivers in some extreme cases.
Second, this debugging act as documentation. He points out
which are the important variables and how they are used at critical points of the code.
As this debug is localised, and compiled out, it does not
bother anybody and should stay.

Jean,

I think the code looks cleaner with the patch applied. If the debugging is valuable to you, I recommend that you save this patch and reverse apply it before future work.

Thanks,

John

Signature · 2 lines
John W. Linville
[email protected]
author maintainer
#18 lore raw

PATCH 7/9 wext: reduce inline abuse

In reply to Johannes Berg

This patch removes a bunch of inline abuse from wext. Most functions that were marked inline are only used once so the compiler will inline them anyway, others are used multiple times but there's no requirement for them to be inline since they aren't in any fast paths.

Signed-off-by Johannes Berg <[email protected]>
---
 net/wireless/wext.c |   29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)
net/wireless/wext.c +10 -19
@@ -440,10 +440,8 @@ static const int event_type_pk_size[] = 
 /* ---------------------------------------------------------------- */
 /*
  * Return the driver handler associated with a specific Wireless Extension.
- * Called from various place, so make sure it remains efficient.
  */
-static inline iw_handler get_handler(struct net_device *dev,
-				     unsigned int cmd)
+static iw_handler get_handler(struct net_device *dev, unsigned int cmd)
 {
 	/* Don't "optimise" the following variable, it will crash */
 	unsigned int	index;		/* *MUST* be unsigned */

@@ -470,7 +468,7 @@ static inline iw_handler get_handler(str
 /*
  * Get statistics out of the driver
  */
-static inline struct iw_statistics *get_wireless_stats(struct net_device *dev)
+static struct iw_statistics *get_wireless_stats(struct net_device *dev)
 {
 	/* New location */
 	if ((dev->wireless_handlers != NULL) &&

@@ -500,7 +498,7 @@ static inline struct iw_statistics *get_
  * netif_running(dev) test. I'm open on that one...
  * Hopefully, the driver will remember to do a commit in "open()" ;-)
  */
-static inline int call_commit_handler(struct net_device *	dev)
+static int call_commit_handler(struct net_device *dev)
 {
 	if ((netif_running(dev)) &&
 	   (dev->wireless_handlers->standard[0] != NULL)) {

@@ -622,8 +620,8 @@ static int iw_handler_get_private(struct
 /*
  * Print one entry (line) of /proc/net/wireless
  */
-static __inline__ void wireless_seq_printf_stats(struct seq_file *seq,
-						 struct net_device *dev)
+static void wireless_seq_printf_stats(struct seq_file *seq,
+				      struct net_device *dev)
 {
 	/* Get stats from the driver */
 	struct iw_statistics *stats = get_wireless_stats(dev);

@@ -892,10 +890,8 @@ static int ioctl_standard_call(struct ne
  * a iw_handler but process it in your ioctl handler (i.e. use the
  * old driver API).
  */
-static inline int ioctl_private_call(struct net_device *	dev,
-				     struct ifreq *		ifr,
-				     unsigned int		cmd,
-				     iw_handler		handler)
+static int ioctl_private_call(struct net_device *dev, struct ifreq *ifr,
+			      unsigned int cmd, iw_handler handler)
 {
 	struct iwreq *			iwr = (struct iwreq *) ifr;
 	const struct iw_priv_args *	descr = NULL;

@@ -1134,11 +1130,8 @@ static DECLARE_TASKLET(wireless_nlevent_
  * current wireless config. Dumping the wireless config is far too
  * expensive (for each parameter, the driver need to query the hardware).
  */
-static inline int rtnetlink_fill_iwinfo(struct sk_buff *	skb,
-					struct net_device *	dev,
-					int			type,
-					char *			event,
-					int			event_len)
+static int rtnetlink_fill_iwinfo(struct sk_buff *skb, struct net_device *dev,
+				 int type, char *event, int event_len)
 {
 	struct ifinfomsg *r;
 	struct nlmsghdr  *nlh;

@@ -1172,9 +1165,7 @@ rtattr_failure:
  * Andrzej Krzysztofowicz mandated that I used a IFLA_XXX field
  * within a RTM_NEWLINK event.
  */
-static inline void rtmsg_iwinfo(struct net_device *	dev,
-				char *			event,
-				int			event_len)
+static void rtmsg_iwinfo(struct net_device *dev, char *event, int event_len)
 {
 	struct sk_buff *skb;
 	int size = NLMSG_GOODSIZE;

--

#19 lore raw

Re: [PATCH 7/9] wext: reduce inline abuse

Quoted 6 lines · Johannes Berg

This patch removes a bunch of inline abuse from wext. Most functions that were marked inline are only used once so the compiler will inline them anyway, others are used multiple times but there's no requirement for them to be inline since they aren't in any fast paths.

Signed-off-by Johannes Berg <[email protected]>

That's clearly not true of all compilers. All gcc versions
before 4.0 need serious help to inline functions used only once. Our current minimal requirement for the kernel is gcc 3.2, therefore this code is still useful.
Note that this is a legitimate use of inline (tell the
compiler to inline the function), not an abuse.

Jean

#20 lore raw

Re: [PATCH 7/9] wext: reduce inline abuse

Quoted 6 lines · On Thursday 26 April 2007 18:50:32 Jean Tourrilhes wrote:
Quoted 5 lines · Johannes Berg

This patch removes a bunch of inline abuse from wext. Most functions that were marked inline are only used once so the compiler will inline them anyway, others are used multiple times but there's no requirement for them to be inline since they aren't in any fast paths.

Quoted 8 lines
Quoted 1 line
Signed-off-by Johannes Berg <[email protected]>

That's clearly not true of all compilers. All gcc versions
before 4.0 need serious help to inline functions used only once. Our current minimal requirement for the kernel is gcc 3.2, therefore this code is still useful.
Note that this is a legitimate use of inline (tell the
compiler to inline the function), not an abuse.

By my personal definition _every_ use of inline is abuse, if it's not in an absolute fastpath and applied to a really tiny function. Sure, other people have different opinions on that, but I think with my approach we get smallest code with good speed.
In general I try to avoid inline whereever possible.
I think this patch is OK and should go in.

Often it's even desired to have out of line functions in fastpaths. See spinlocks.

Signature · 1 line
Greetings Michael.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#22 lore raw

Re: [PATCH 7/9] wext: reduce inline abuse

Quoted 3 lines · Michael Buesch

Sure, other people have different opinions on that, but I think with my approach we get smallest code with good speed.

Try with gcc-3.3 if you don't trust me. Your patch will
produce bigger and slower code. Thanks.

Jean

maintainer
#23 lore raw

Re: [PATCH 7/9] wext: reduce inline abuse

From: Jean Tourrilhes <[email protected]>
Date: Thu, 26 Apr 2007 10:15:17 -0700

Quoted 7 lines · Michael Buesch
Quoted 3 lines

Sure, other people have different opinions on that, but I think with my approach we get smallest code with good speed.

Try with gcc-3.3 if you don't trust me. Your patch will
produce bigger and slower code. Thanks.

A broken compiler is not an argument for keeping all of these bogus inlines around.

It is the compilers job to optimize things correctly, and current gcc's do.

We've always had this approach to dealing with compiler "issues".

A message that is not in the archive (sent off-list or not yet received): 20070426165032.GB22576-yAE0UhLNZJawPNPzzlOzwdBPR1lH4CV8@public.gmane.org

#25 lore raw

Re: [PATCH 7/9] wext: reduce inline abuse

Quoted 6 lines · Jean Tourrilhes

That's clearly not true of all compilers. All gcc versions
before 4.0 need serious help to inline functions used only once. Our current minimal requirement for the kernel is gcc 3.2, therefore this code is still useful.
Note that this is a legitimate use of inline (tell the
compiler to inline the function), not an abuse.

No, the abuse is using inline in the first place because it doesn't matter, none of this has an actual reason to be inlined.

johannes

A message that is not in the archive (sent off-list or not yet received): [email protected]

#27 lore raw
Quoted 1 line · Johannes Berg

This patch series contains various cleanups to the wireless extensions code.

Patch 1/9 got lost because it's too large, but it's only moving net/core/wireless.c to net/wireless/wext.c and doing some tiny Makefile updates.

johannes

maintainer
#28 lore raw

From: Johannes Berg <[email protected]>
Date: Thu, 26 Apr 2007 12:15:13 +0200

Quoted 6 lines · Johannes Berg
Quoted 1 line

This patch series contains various cleanups to the wireless extensions code.

Patch 1/9 got lost because it's too large, but it's only moving net/core/wireless.c to net/wireless/wext.c and doing some tiny Makefile updates.

I got the bounce so I have the changelog too :-)

A message that is not in the archive (sent off-list or not yet received): 20070426.031842.17863787.davem-fT/PcQaiUtIeIZ0/[email protected]

#30 lore raw
Quoted 1 line · David Miller

I got the bounce so I have the changelog too :-)

Sorry. I should've known and just posted git instructions. You should have gotten a copy directly to your inbox too.

johannes

A message that is not in the archive (sent off-list or not yet received): 20070426.031921.133286083.davem-fT/PcQaiUtIeIZ0/[email protected]

#33 lore raw
Quoted 6 lines · David Miller

From: Johannes Berg <[email protected]>
Date: Tue, 24 Apr 2007 20:07:32 +0200

Quoted 1 line

This patch series contains various cleanups to the wireless extensions code.

I'll wait for Linville to ACK this stuff, then I'll apply it.

I'm OK with the whole series.

Signed-off-by John W. Linville <[email protected]>

Good enough? Or would you prefer a git pull request?

John

Signature · 2 lines
John W. Linville
[email protected]