[PATCH net-next 0/3] net: remove dsa.h from netdevice.h

[PATCH net-next 0/3] net: remove dsa.h from netdevice.h

[Vivien Didelot]

In order to push switchdev objects down to DSA drivers, I need to include
switchdev.h in dsa.h. But compilation fails because of a circular dependency
issue, since dsa.h is also included in linux/netdevice.h.

dsa.h is included for two purposes: to have the definition of struct
dsa_switch_tree; and to have access to dsa_uses_tagged_protocol(), used by
netdev_uses_dsa().

This patchset forward declares struct dsa_switch_tree; adds a new uses_hw_tag
boolean to struct net_device, which is more explicit than the DSA helper, and
finally get rid of the dsa.h include.

With this patchset, switchdev.h can safely be included in dsa.h.

Vivien Didelot (3):
  net: dsa: add uses_hw_tag
  net: dsa: include dsa.h in dsa_priv.h
  net: remove dsa.h include from linux/netdevice.h

 include/linux/netdevice.h | 9 ++++++---
 include/net/dsa.h         | 5 -----
 net/dsa/dsa.c             | 1 +
 net/dsa/dsa_priv.h        | 1 +
 4 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.6.0

[PATCH net-next 1/3] net: dsa: add uses_hw_tag

[Vivien Didelot]

Instead of checking that the dsa_switch_tree rcv pointer is not NULL,
add a uses_hw_tag boolean to net_device to explicit whether it uses
hardware inserted tag or not.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 include/linux/netdevice.h | 6 ++++--
 include/net/dsa.h         | 5 -----
 net/dsa/dsa.c             | 1 +
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index b945078..1986a22 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1417,12 +1417,13 @@ enum netdev_priv_flags {
  *	@promiscuity:		Number of times, the NIC is told to work in
  *				Promiscuous mode, if it becomes 0 the NIC will
  *				exit from working in Promiscuous mode
  *	@allmulti:		Counter, enables or disables allmulticast mode
  *
  *	@vlan_info:	VLAN info
+ *	@uses_hw_tag:	Whether the device uses hardware inserted tag or not
  *	@dsa_ptr:	dsa specific data
  *	@tipc_ptr:	TIPC specific data
  *	@atalk_ptr:	AppleTalk link
  *	@ip_ptr:	IPv4 specific data
  *	@dn_ptr:	DECnet specific data
  *	@ip6_ptr:	IPv6 specific data
@@ -1635,12 +1636,13 @@ struct net_device {
 	/* Protocol specific pointers */
 
 #if IS_ENABLED(CONFIG_VLAN_8021Q)
 	struct vlan_info __rcu	*vlan_info;
 #endif
 #if IS_ENABLED(CONFIG_NET_DSA)
+	bool uses_hw_tag;
 	struct dsa_switch_tree	*dsa_ptr;
 #endif
 #if IS_ENABLED(CONFIG_TIPC)
 	struct tipc_bearer __rcu *tipc_ptr;
 #endif
 	void 			*atalk_ptr;
@@ -1886,14 +1888,14 @@ void dev_net_set(struct net_device *dev, struct net *net)
 	write_pnet(&dev->nd_net, net);
 }
 
 static inline bool netdev_uses_dsa(struct net_device *dev)
 {
 #if IS_ENABLED(CONFIG_NET_DSA)
-	if (dev->dsa_ptr != NULL)
-		return dsa_uses_tagged_protocol(dev->dsa_ptr);
+	if (dev->uses_hw_tag)
+		return true;
 #endif
 	return false;
 }
 
 /**
  *	netdev_priv - access network device private data
diff --git a/include/net/dsa.h b/include/net/dsa.h
index b34d812..3e9eb6c 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -330,12 +330,7 @@ void unregister_switch_driver(struct dsa_switch_driver *type);
 struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev);
 
 static inline void *ds_to_priv(struct dsa_switch *ds)
 {
 	return (void *)(ds + 1);
 }
-
-static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
-{
-	return dst->rcv != NULL;
-}
 #endif
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index c59fa5d..623065f 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -831,12 +831,13 @@ static void dsa_setup_dst(struct dsa_switch_tree *dst, struct net_device *dev,
 	 * If we use a tagging format that doesn't have an ethertype
 	 * field, make sure that all packets from this point on get
 	 * sent to the tag format's receive function.
 	 */
 	wmb();
 	dev->dsa_ptr = (void *)dst;
+	dev->uses_hw_tag = dst->tag_protocol != DSA_TAG_PROTO_NONE;
 
 	if (dst->link_poll_needed) {
 		INIT_WORK(&dst->link_poll_work, dsa_link_poll_work);
 		init_timer(&dst->link_poll_timer);
 		dst->link_poll_timer.data = (unsigned long)dst;
 		dst->link_poll_timer.function = dsa_link_poll_timer;
-- 
2.6.0

[PATCH net-next 2/3] net: dsa: include dsa.h in dsa_priv.h

[Vivien Didelot]

dsa_priv.h uses dsa specific structures, as well as the files using it,
so include dsa.h here.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/dsa_priv.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 311796c8..4522f47 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -11,12 +11,13 @@
 #ifndef __DSA_PRIV_H
 #define __DSA_PRIV_H
 
 #include <linux/phy.h>
 #include <linux/netdevice.h>
 #include <linux/netpoll.h>
+#include <net/dsa.h>
 
 struct dsa_device_ops {
 	struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev);
 	int (*rcv)(struct sk_buff *skb, struct net_device *dev,
 		   struct packet_type *pt, struct net_device *orig_dev);
 };
-- 
2.6.0

[PATCH net-next 3/3] net: remove dsa.h include from linux/netdevice.h

[Vivien Didelot]

Forward declare struct dsa_switch_tree in netdevice.h instead of
including the dsa.h header.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 include/linux/netdevice.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 1986a22..4618b1d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -38,13 +38,12 @@
 #include <linux/dmaengine.h>
 #include <linux/workqueue.h>
 #include <linux/dynamic_queue_limits.h>
 
 #include <linux/ethtool.h>
 #include <net/net_namespace.h>
-#include <net/dsa.h>
 #ifdef CONFIG_DCB
 #include <net/dcbnl.h>
 #endif
 #include <net/netprio_cgroup.h>
 
 #include <linux/netdev_features.h>
@@ -57,12 +56,14 @@ struct device;
 struct phy_device;
 /* 802.11 specific */
 struct wireless_dev;
 /* 802.15.4 specific */
 struct wpan_dev;
 struct mpls_dev;
+/* DSA specific */
+struct dsa_switch_tree;
 
 void netdev_set_default_ethtool_ops(struct net_device *dev,
 				    const struct ethtool_ops *ops);
 
 /* Backlog congestion levels */
 #define NET_RX_SUCCESS		0	/* keep 'em coming, baby */
-- 
2.6.0

Re: [PATCH net-next 0/3] net: remove dsa.h from netdevice.h

[Florian Fainelli]

On 06/10/15 14:54, Vivien Didelot wrote:
> In order to push switchdev objects down to DSA drivers, I need to include
> switchdev.h in dsa.h. But compilation fails because of a circular dependency
> issue, since dsa.h is also included in linux/netdevice.h.

Just for the record, what does this circular dependency looks like? Last
I tried something in that front, I ended-up forward declaring the
'switchdev_obj' structure, but that was weeks ago before the whole
restructuring, so could be pointless now.

> 
> dsa.h is included for two purposes: to have the definition of struct
> dsa_switch_tree; and to have access to dsa_uses_tagged_protocol(), used by
> netdev_uses_dsa().
> 
> This patchset forward declares struct dsa_switch_tree; adds a new uses_hw_tag
> boolean to struct net_device, which is more explicit than the DSA helper, and
> finally get rid of the dsa.h include.
> 
> With this patchset, switchdev.h can safely be included in dsa.h.
> 
> Vivien Didelot (3):
>   net: dsa: add uses_hw_tag
>   net: dsa: include dsa.h in dsa_priv.h
>   net: remove dsa.h include from linux/netdevice.h
> 
>  include/linux/netdevice.h | 9 ++++++---
>  include/net/dsa.h         | 5 -----
>  net/dsa/dsa.c             | 1 +
>  net/dsa/dsa_priv.h        | 1 +
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 


-- 
Florian

Re: [PATCH net-next 0/3] net: remove dsa.h from netdevice.h

[Vivien Didelot]

Hi Florian, All,

On Oct. Tuesday 06 (41) 05:42 PM, Florian Fainelli wrote:
> On 06/10/15 14:54, Vivien Didelot wrote:
> > In order to push switchdev objects down to DSA drivers, I need to include
> > switchdev.h in dsa.h. But compilation fails because of a circular dependency
> > issue, since dsa.h is also included in linux/netdevice.h.
> 
> Just for the record, what does this circular dependency looks like? Last
> I tried something in that front, I ended-up forward declaring the
> 'switchdev_obj' structure, but that was weeks ago before the whole
> restructuring, so could be pointless now.

My next work is to push the switchdev callback for FDB dump down to the
DSA drivers. It works if I forward declare all these in net/dsa.h:

 * struct switchdev_obj
 * struct switchdev_obj_port_fdb
 * typedef int switchdev_obj_dump_cb_t(struct switchdev_obj *obj);

Then will come the VLAN ops, etc. It doesn't feel like the way to go.

Below is how these dependency errors look like. There are two issues
this patchset fixes. First, here's what happens you just add:

    #include <net/switchdev.h>

in include/net/dsa.h:

[...]

In file included from include/net/dsa.h:22:0,
                 from include/linux/netdevice.h:44,
                 from include/linux/icmpv6.h:12,
                 from include/linux/ipv6.h:71,
                 from include/net/ipv6.h:16,
                 from include/linux/sunrpc/clnt.h:27,
                 from include/linux/nfs_fs.h:30,
                 from init/do_mounts.c:32:
include/net/switchdev.h:52:30: error: field ‘ppid’ has incomplete type
   struct netdev_phys_item_id ppid; /* PORT_PARENT_ID */
                              ^
include/net/switchdev.h:185:14: warning: ‘struct nlmsghdr’ declared inside parameter list [enabled by default]
       struct nlmsghdr *nlh, u16 flags);
              ^
include/net/switchdev.h:185:14: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default]
include/net/switchdev.h:187:14: warning: ‘struct nlmsghdr’ declared inside parameter list [enabled by default]
       struct nlmsghdr *nlh, u16 flags);
              ^
include/net/switchdev.h:195:7: warning: ‘struct nlattr’ declared inside parameter list [enabled by default]
       u16 vid, u16 nlm_flags);
       ^
include/net/switchdev.h:195:7: warning: ‘struct ndmsg’ declared inside parameter list [enabled by default]
include/net/switchdev.h:198:7: warning: ‘struct nlattr’ declared inside parameter list [enabled by default]
       u16 vid);
       ^
include/net/switchdev.h:198:7: warning: ‘struct ndmsg’ declared inside parameter list [enabled by default]
include/net/switchdev.h:201:15: warning: ‘struct netlink_callback’ declared inside parameter list [enabled by default]
        struct net_device *filter_dev, int idx);
               ^

[...]

Removing the dsa.h include from linux/netdevice.h gets rid of these
warnings, but then, the DSA code complains with the following:

[...]

net/dsa/slave.c: In function ‘dsa_slave_phy_read’:
net/dsa/slave.c:29:8: error: dereferencing pointer to incomplete type
  if (ds->phys_mii_mask & (1 << addr))
        ^

[...]

net/dsa/slave.c: In function ‘dsa_slave_set_mac_address’:
net/dsa/slave.c:178:39: error: dereferencing pointer to incomplete type
  struct net_device *master = p->parent->dst->master_netdev;
                                       ^
In file included from include/linux/list.h:8:0,
                 from net/dsa/slave.c:11:
net/dsa/slave.c: In function ‘dsa_bridge_check_vlan_range’:
net/dsa/slave.c:209:26: error: ‘DSA_MAX_PORTS’ undeclared (first use in this function)
  DECLARE_BITMAP(members, DSA_MAX_PORTS);
                          ^
include/linux/kernel.h:67:30: note: in definition of macro ‘DIV_ROUND_UP’
 #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
                              ^
include/linux/types.h:10:21: note: in expansion of macro ‘BITS_TO_LONGS’
  unsigned long name[BITS_TO_LONGS(bits)]
                     ^
net/dsa/slave.c:209:2: note: in expansion of macro ‘DECLARE_BITMAP’
  DECLARE_BITMAP(members, DSA_MAX_PORTS);
  ^
net/dsa/slave.c:209:26: note: each undeclared identifier is reported only once for each function it appears in
  DECLARE_BITMAP(members, DSA_MAX_PORTS);
                          ^
include/linux/kernel.h:67:30: note: in definition of macro ‘DIV_ROUND_UP’
 #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
                              ^
include/linux/types.h:10:21: note: in expansion of macro ‘BITS_TO_LONGS’
  unsigned long name[BITS_TO_LONGS(bits)]
                     ^
net/dsa/slave.c:209:2: note: in expansion of macro ‘DECLARE_BITMAP’
  DECLARE_BITMAP(members, DSA_MAX_PORTS);
  ^
net/dsa/slave.c:214:9: error: dereferencing pointer to incomplete type
  if (!ds->drv->vlan_getnext || !vid_begin)
         ^

[...]

net/dsa/slave.c:1190:7: error: ‘DSA_TAG_PROTO_EDSA’ undeclared (first use in this function)
  case DSA_TAG_PROTO_EDSA:
       ^
net/dsa/slave.c:1219:4: error: dereferencing pointer to incomplete type
  ds->ports[port] = slave_dev;
    ^
net/dsa/slave.c:1225:5: error: dereferencing pointer to incomplete type
   ds->ports[port] = NULL;
     ^
net/dsa/slave.c: In function ‘dsa_slave_get_iflink’:
net/dsa/slave.c:64:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^
cc1: some warnings being treated as errors
scripts/Makefile.build:258: recipe for target 'net/dsa/slave.o' failed
make[3]: *** [net/dsa/slave.o] Error 1
scripts/Makefile.build:403: recipe for target 'net/dsa' failed
make[2]: *** [net/dsa] Error 2

[...]

Finally, adding:

    #include <net/dsa.h>

in net/dsa/dsa_priv.h fixes these errors.

Does the patchset makes sense?

Best,
-v

Re: [PATCH net-next 0/3] net: remove dsa.h from netdevice.h

[Jiri Pirko]

Tue, Oct 06, 2015 at 11:54:24PM CEST, vivien.didelot@savoirfairelinux.com wrote:
>In order to push switchdev objects down to DSA drivers, I need to include
>switchdev.h in dsa.h. But compilation fails because of a circular dependency
>issue, since dsa.h is also included in linux/netdevice.h.
>
>dsa.h is included for two purposes: to have the definition of struct
>dsa_switch_tree; and to have access to dsa_uses_tagged_protocol(), used by
>netdev_uses_dsa().
>
>This patchset forward declares struct dsa_switch_tree; adds a new uses_hw_tag
>boolean to struct net_device, which is more explicit than the DSA helper, and
>finally get rid of the dsa.h include.
>
>With this patchset, switchdev.h can safely be included in dsa.h.

Looks fine to me. I think that you should push this along with your
other patchset which actually needs this.

Thanks!

Re: [PATCH net-next 0/3] net: remove dsa.h from netdevice.h

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Tue,  6 Oct 2015 17:54:24 -0400

> In order to push switchdev objects down to DSA drivers, I need to include
> switchdev.h in dsa.h. But compilation fails because of a circular dependency
> issue, since dsa.h is also included in linux/netdevice.h.
> 
> dsa.h is included for two purposes: to have the definition of struct
> dsa_switch_tree; and to have access to dsa_uses_tagged_protocol(), used by
> netdev_uses_dsa().
> 
> This patchset forward declares struct dsa_switch_tree; adds a new uses_hw_tag
> boolean to struct net_device, which is more explicit than the DSA helper, and
> finally get rid of the dsa.h include.
> 
> With this patchset, switchdev.h can safely be included in dsa.h.

The netdevice structure is already too big, and bloating it up even more
to satisfy some include ordering issue is rather unacceptable as far as I
am concerned.

Just forward declare the pointer type, you're not dereferencing it.