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

[Vivien Didelot <vivien.didelot@savoirfairelinux.com>]

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