[PATCH net-next 0/5] tun: debug messages cleanup

[PATCH net-next 0/5] tun: debug messages cleanup

[Michal Kubecek]

While testing ethtool output for "strange" devices, I noticed confusing and
obviously incorrect message level information for a tun device and sent
a quick fix. The result of the upstream discussion was that tun driver
would rather deserve a more complex cleanup of the way it handles debug
messages.

The main problem is that all debugging statements and setting of message
level are controlled by TUN_DEBUG macro which is only defined if one edits
the source and rebuilds the module, otherwise all DBG1() and tun_debug()
statements do nothing.

This series drops the TUN_DEBUG switch and replaces custom tun_debug()
macro with standard netif_info() so that message level (mask) set and
displayed using ethtool works as expected. Some debugging messages are
dropped as they only notify about entering a function which can be done
easily using ftrace or kprobe.

Patch 1 is a trivial fix for compilation warning with W=1.

Michal Kubecek (5):
  tun: fix misleading comment format
  tun: get rid of DBG1() macro
  tun: drop useless debugging statements
  tun: replace tun_debug() by netif_info()
  tun: drop TUN_DEBUG and tun_debug()

 drivers/net/tun.c | 105 ++++++++++++----------------------------------
 1 file changed, 27 insertions(+), 78 deletions(-)

-- 
2.25.1

[PATCH net-next 1/5] tun: fix misleading comment format

[Michal Kubecek]

The comment above tun_flow_save_rps_rxhash() starts with "/**" which
makes it look like kerneldoc comment and results in warnings when
building with W=1. Fix the format to make it look like a normal comment.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 drivers/net/tun.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 79f248cb282d..ea64c311a554 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -546,8 +546,7 @@ static void tun_flow_update(struct tun_struct *tun, u32 rxhash,
 	rcu_read_unlock();
 }
 
-/**
- * Save the hash received in the stack receive path and update the
+/* Save the hash received in the stack receive path and update the
  * flow_hash table accordingly.
  */
 static inline void tun_flow_save_rps_rxhash(struct tun_flow_entry *e, u32 hash)
-- 
2.25.1

[PATCH net-next 2/5] tun: get rid of DBG1() macro

[Michal Kubecek]

This macro is no-op unless TUN_DEBUG is defined (which requires editing and
recompiling the source) and only does something if variable debug is 2 but
that variable is zero initialized and never set to anything else. Moreover,
the only use of the macro informs about entering function tun_chr_open()
which can be easily achieved using ftrace or kprobe.

Drop DBG1() macro, its only use and global variable debug.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 drivers/net/tun.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index ea64c311a554..59290ef07497 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -79,29 +79,17 @@ static void tun_default_link_ksettings(struct net_device *dev,
 /* #define TUN_DEBUG 1 */
 
 #ifdef TUN_DEBUG
-static int debug;
-
 #define tun_debug(level, tun, fmt, args...)			\
 do {								\
 	if (tun->debug)						\
 		netdev_printk(level, tun->dev, fmt, ##args);	\
 } while (0)
-#define DBG1(level, fmt, args...)				\
-do {								\
-	if (debug == 2)						\
-		printk(level fmt, ##args);			\
-} while (0)
 #else
 #define tun_debug(level, tun, fmt, args...)			\
 do {								\
 	if (0)							\
 		netdev_printk(level, tun->dev, fmt, ##args);	\
 } while (0)
-#define DBG1(level, fmt, args...)				\
-do {								\
-	if (0)							\
-		printk(level fmt, ##args);			\
-} while (0)
 #endif
 
 #define TUN_RX_PAD (NET_IP_ALIGN + NET_SKB_PAD)
@@ -3415,8 +3403,6 @@ static int tun_chr_open(struct inode *inode, struct file * file)
 	struct net *net = current->nsproxy->net_ns;
 	struct tun_file *tfile;
 
-	DBG1(KERN_INFO, "tunX: tun_chr_open\n");
-
 	tfile = (struct tun_file *)sk_alloc(net, AF_UNSPEC, GFP_KERNEL,
 					    &tun_proto, 0);
 	if (!tfile)
-- 
2.25.1

[PATCH net-next 3/5] tun: drop useless debugging statements

[Michal Kubecek]

Some of the tun_debug() statements only inform us about entering
a function which can be easily achieved with ftrace or kprobe. As
tun_debug() is no-op unless TUN_DEBUG is set which requires editing the
source and recompiling, setting up ftrace or kprobe is easier. Drop these
debug statements.

Also drop the tun_debug() statement informing about SIOCSIFHWADDR ioctl.
We can monitor these through rtnetlink and it makes little sense to log
address changes through ioctl but not changes through rtnetlink. Moreover,
this tun_debug() is called even if the actual address change fails which
makes it even less useful.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 drivers/net/tun.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 59290ef07497..15ae2050ab5b 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -473,8 +473,6 @@ static void tun_flow_cleanup(struct timer_list *t)
 	unsigned long count = 0;
 	int i;
 
-	tun_debug(KERN_INFO, tun, "tun_flow_cleanup\n");
-
 	spin_lock(&tun->lock);
 	for (i = 0; i < TUN_NUM_FLOW_ENTRIES; i++) {
 		struct tun_flow_entry *e;
@@ -1420,8 +1418,6 @@ static __poll_t tun_chr_poll(struct file *file, poll_table *wait)
 
 	sk = tfile->socket.sk;
 
-	tun_debug(KERN_INFO, tun, "tun_chr_poll\n");
-
 	poll_wait(file, sk_sleep(sk), wait);
 
 	if (!ptr_ring_empty(&tfile->tx_ring))
@@ -2192,8 +2188,6 @@ static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
 	ssize_t ret;
 	int err;
 
-	tun_debug(KERN_INFO, tun, "tun_do_read\n");
-
 	if (!iov_iter_count(to)) {
 		tun_ptr_free(ptr);
 		return 0;
@@ -2838,8 +2832,6 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 
 	netif_carrier_on(tun->dev);
 
-	tun_debug(KERN_INFO, tun, "tun_set_iff\n");
-
 	/* Make sure persistent devices do not get stuck in
 	 * xoff state.
 	 */
@@ -2870,8 +2862,6 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 
 static void tun_get_iff(struct tun_struct *tun, struct ifreq *ifr)
 {
-	tun_debug(KERN_INFO, tun, "tun_get_iff\n");
-
 	strcpy(ifr->ifr_name, tun->dev->name);
 
 	ifr->ifr_flags = tun_flags(tun);
@@ -3206,9 +3196,6 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 
 	case SIOCSIFHWADDR:
 		/* Set hw address */
-		tun_debug(KERN_DEBUG, tun, "set hw address: %pM\n",
-			  ifr.ifr_hwaddr.sa_data);
-
 		ret = dev_set_mac_address(tun->dev, &ifr.ifr_hwaddr, NULL);
 		break;
 
-- 
2.25.1

[PATCH net-next 4/5] tun: replace tun_debug() by netif_info()

[Michal Kubecek]

The tun driver uses custom macro tun_debug() which is only available if
TUN_DEBUG is set. Replace it by standard netif_ifinfo(). For that purpose,
rename tun_struct::debug to msg_enable and make it u32 and always present.
Finally, make tun_get_msglevel(), tun_set_msglevel() and TUNSETDEBUG ioctl
independent of TUN_DEBUG.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 drivers/net/tun.c | 60 +++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 33 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 15ae2050ab5b..42110aba0014 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -81,7 +81,7 @@ static void tun_default_link_ksettings(struct net_device *dev,
 #ifdef TUN_DEBUG
 #define tun_debug(level, tun, fmt, args...)			\
 do {								\
-	if (tun->debug)						\
+	if (tun->msg_enable)					\
 		netdev_printk(level, tun->dev, fmt, ##args);	\
 } while (0)
 #else
@@ -213,9 +213,7 @@ struct tun_struct {
 	struct sock_fprog	fprog;
 	/* protected by rtnl lock */
 	bool			filter_attached;
-#ifdef TUN_DEBUG
-	int debug;
-#endif
+	u32			msg_enable;
 	spinlock_t lock;
 	struct hlist_head flows[TUN_NUM_FLOW_ENTRIES];
 	struct timer_list flow_gc_timer;
@@ -411,8 +409,9 @@ static struct tun_flow_entry *tun_flow_create(struct tun_struct *tun,
 	struct tun_flow_entry *e = kmalloc(sizeof(*e), GFP_ATOMIC);
 
 	if (e) {
-		tun_debug(KERN_INFO, tun, "create flow: hash %u index %u\n",
-			  rxhash, queue_index);
+		netif_info(tun, tx_queued, tun->dev,
+			   "create flow: hash %u index %u\n",
+			   rxhash, queue_index);
 		e->updated = jiffies;
 		e->rxhash = rxhash;
 		e->rps_rxhash = 0;
@@ -426,8 +425,8 @@ static struct tun_flow_entry *tun_flow_create(struct tun_struct *tun,
 
 static void tun_flow_delete(struct tun_struct *tun, struct tun_flow_entry *e)
 {
-	tun_debug(KERN_INFO, tun, "delete flow: hash %u index %u\n",
-		  e->rxhash, e->queue_index);
+	netif_info(tun, tx_queued, tun->dev, "delete flow: hash %u index %u\n",
+		   e->rxhash, e->queue_index);
 	hlist_del_rcu(&e->hash_link);
 	kfree_rcu(e, rcu);
 	--tun->flow_count;
@@ -1061,7 +1060,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (!rcu_dereference(tun->steering_prog))
 		tun_automq_xmit(tun, skb);
 
-	tun_debug(KERN_INFO, tun, "tun_net_xmit %d\n", skb->len);
+	netif_info(tun, tx_queued, tun->dev, "%s %d\n", __func__, skb->len);
 
 	/* Drop if the filter does not like it.
 	 * This is a noop if the filter is disabled.
@@ -3085,7 +3084,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 	if (!tun)
 		goto unlock;
 
-	tun_debug(KERN_INFO, tun, "tun_chr_ioctl cmd %u\n", cmd);
+	netif_info(tun, drv, tun->dev, "tun_chr_ioctl cmd %u\n", cmd);
 
 	net = dev_net(tun->dev);
 	ret = 0;
@@ -3106,8 +3105,8 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 		/* Disable/Enable checksum */
 
 		/* [unimplemented] */
-		tun_debug(KERN_INFO, tun, "ignored: set checksum %s\n",
-			  arg ? "disabled" : "enabled");
+		netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n",
+			   arg ? "disabled" : "enabled");
 		break;
 
 	case TUNSETPERSIST:
@@ -3125,8 +3124,8 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 			do_notify = true;
 		}
 
-		tun_debug(KERN_INFO, tun, "persist %s\n",
-			  arg ? "enabled" : "disabled");
+		netif_info(tun, drv, tun->dev, "persist %s\n",
+			   arg ? "enabled" : "disabled");
 		break;
 
 	case TUNSETOWNER:
@@ -3138,8 +3137,8 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 		}
 		tun->owner = owner;
 		do_notify = true;
-		tun_debug(KERN_INFO, tun, "owner set to %u\n",
-			  from_kuid(&init_user_ns, tun->owner));
+		netif_info(tun, drv, tun->dev, "owner set to %u\n",
+			   from_kuid(&init_user_ns, tun->owner));
 		break;
 
 	case TUNSETGROUP:
@@ -3151,29 +3150,28 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 		}
 		tun->group = group;
 		do_notify = true;
-		tun_debug(KERN_INFO, tun, "group set to %u\n",
-			  from_kgid(&init_user_ns, tun->group));
+		netif_info(tun, drv, tun->dev, "group set to %u\n",
+			   from_kgid(&init_user_ns, tun->group));
 		break;
 
 	case TUNSETLINK:
 		/* Only allow setting the type when the interface is down */
 		if (tun->dev->flags & IFF_UP) {
-			tun_debug(KERN_INFO, tun,
-				  "Linktype set failed because interface is up\n");
+			netif_info(tun, drv, tun->dev,
+				   "Linktype set failed because interface is up\n");
 			ret = -EBUSY;
 		} else {
 			tun->dev->type = (int) arg;
-			tun_debug(KERN_INFO, tun, "linktype set to %d\n",
-				  tun->dev->type);
+			netif_info(tun, drv, tun->dev, "linktype set to %d\n",
+				   tun->dev->type);
 			ret = 0;
 		}
 		break;
 
-#ifdef TUN_DEBUG
 	case TUNSETDEBUG:
-		tun->debug = arg;
+		tun->msg_enable = (u32)arg;
 		break;
-#endif
+
 	case TUNSETOFFLOAD:
 		ret = set_offload(tun, arg);
 		break;
@@ -3529,20 +3527,16 @@ static void tun_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info
 
 static u32 tun_get_msglevel(struct net_device *dev)
 {
-#ifdef TUN_DEBUG
 	struct tun_struct *tun = netdev_priv(dev);
-	return tun->debug;
-#else
-	return -EOPNOTSUPP;
-#endif
+
+	return tun->msg_enable;
 }
 
 static void tun_set_msglevel(struct net_device *dev, u32 value)
 {
-#ifdef TUN_DEBUG
 	struct tun_struct *tun = netdev_priv(dev);
-	tun->debug = value;
-#endif
+
+	tun->msg_enable = value;
 }
 
 static int tun_get_coalesce(struct net_device *dev,
-- 
2.25.1

[PATCH net-next 5/5] tun: drop TUN_DEBUG and tun_debug()

[Michal Kubecek]

TUN_DEBUG and tun_debug() are no longer used anywhere, drop them.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 drivers/net/tun.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 42110aba0014..4689e4c62e21 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -75,23 +75,6 @@
 static void tun_default_link_ksettings(struct net_device *dev,
 				       struct ethtool_link_ksettings *cmd);
 
-/* Uncomment to enable debugging */
-/* #define TUN_DEBUG 1 */
-
-#ifdef TUN_DEBUG
-#define tun_debug(level, tun, fmt, args...)			\
-do {								\
-	if (tun->msg_enable)					\
-		netdev_printk(level, tun->dev, fmt, ##args);	\
-} while (0)
-#else
-#define tun_debug(level, tun, fmt, args...)			\
-do {								\
-	if (0)							\
-		netdev_printk(level, tun->dev, fmt, ##args);	\
-} while (0)
-#endif
-
 #define TUN_RX_PAD (NET_IP_ALIGN + NET_SKB_PAD)
 
 /* TUN device flags */
-- 
2.25.1

Re: [PATCH net-next 0/5] tun: debug messages cleanup

[David Miller]

From: Michal Kubecek <mkubecek@suse.cz>
Date: Wed,  4 Mar 2020 17:23:54 +0100 (CET)

> While testing ethtool output for "strange" devices, I noticed confusing and
> obviously incorrect message level information for a tun device and sent
> a quick fix. The result of the upstream discussion was that tun driver
> would rather deserve a more complex cleanup of the way it handles debug
> messages.
> 
> The main problem is that all debugging statements and setting of message
> level are controlled by TUN_DEBUG macro which is only defined if one edits
> the source and rebuilds the module, otherwise all DBG1() and tun_debug()
> statements do nothing.
> 
> This series drops the TUN_DEBUG switch and replaces custom tun_debug()
> macro with standard netif_info() so that message level (mask) set and
> displayed using ethtool works as expected. Some debugging messages are
> dropped as they only notify about entering a function which can be done
> easily using ftrace or kprobe.
> 
> Patch 1 is a trivial fix for compilation warning with W=1.

Series applied, thanks.