Message ID | 20090107141343.3e256119@extreme |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
Stephen Hemminger <shemminger@vyatta.com> writes: > Fix build problem with hdlc driver. hdlc_null_ops was being referenced > before it was defined! :-) Unfortunately I can't see it, which tree is it? In David's and Linus' trees it appears twice, it seem to be entirely correct and btw you are the author :-) ... and it compiled here 100 times or so :-) static const struct header_ops hdlc_null_ops; ^^^^^^^^^^^^^ static void hdlc_setup_dev(struct net_device *dev) { /* Re-init all variables changed by HDLC protocol drivers, * including ether_setup() called from hdlc_raw_eth.c. */ dev->get_stats = hdlc_get_stats; dev->flags = IFF_POINTOPOINT | IFF_NOARP; dev->mtu = HDLC_MAX_MTU; dev->type = ARPHRD_RAWHDLC; dev->hard_header_len = 16; dev->addr_len = 0; dev->header_ops = &hdlc_null_ops; ^^^^^^^^^^^^^^^^ dev->change_mtu = hdlc_change_mtu; }
On Thu, 08 Jan 2009 00:21:03 +0100 Krzysztof Halasa <khc@pm.waw.pl> wrote: > Stephen Hemminger <shemminger@vyatta.com> writes: > > > Fix build problem with hdlc driver. hdlc_null_ops was being referenced > > before it was defined! > > :-) > > Unfortunately I can't see it, which tree is it? > > > In David's and Linus' trees it appears twice, it seem to be entirely > correct and btw you are the author :-) > ... and it compiled here 100 times or so :-) > > static const struct header_ops hdlc_null_ops; > ^^^^^^^^^^^^^ > static void hdlc_setup_dev(struct net_device *dev) > { > /* Re-init all variables changed by HDLC protocol drivers, > * including ether_setup() called from hdlc_raw_eth.c. > */ > dev->get_stats = hdlc_get_stats; > dev->flags = IFF_POINTOPOINT | IFF_NOARP; > dev->mtu = HDLC_MAX_MTU; > dev->type = ARPHRD_RAWHDLC; > dev->hard_header_len = 16; > dev->addr_len = 0; > dev->header_ops = &hdlc_null_ops; > ^^^^^^^^^^^^^^^^ > dev->change_mtu = hdlc_change_mtu; > } It was introduced (by me) in previous patch. Since that isn't applied yet, I'll go back and fix. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephen Hemminger <shemminger@vyatta.com> writes: > It was introduced (by me) in previous patch. Since that isn't applied yet, > I'll go back and fix. I can see it now, I must have missed it because of the "synclink" subject. @@ -106,7 +98,7 @@ static int hdlc_device_event(struct noti if (dev_net(dev) != &init_net) return NOTIFY_DONE; - if (dev->get_stats != hdlc_get_stats) + if (dev->header_ops != &hdlc_null_ops) return NOTIFY_DONE; /* not an HDLC device */ This won't work because hdlc_null_ops may be substituted by cisco_header_ops or ppp_header_ops. Current test works for now but is wrong, too, at least for wanxl driver (it tries to use its get_stats()). Perhaps some + if (dev->header_ops->something != &something) would do? Alternatively we can return to hdlc_carrier_on|off() and hdlc_device_event() won't be needed. Essentially, it's a bit hard to check if the device is one of ours if we are the middle layer. There are two layers here: - hardware drivers (10+) - protocol drivers (5 - hdlc* files) Protocol drivers want to define things like: - dev->header_ops (no problem) - dev->change_mtu (perhaps) - dev->hard_start_xmit Hardware drivers need to define the rest of NDO. Hmm, not much, only hard_start_xmit() is really problematic. I think we need a trampoline: hw driver will define the NDO and set hard_start_xmit and possibly change_mtu to (exported by hdlc.c) hdlc_xmit (hdlc_change_mtu), and hdlc_start_xmit will just call protocol's xmit(). We could also use it for the test in hdlc_device_event: if (dev->netdev_ops->start_hard_xmit != hdlc_xmit) If we could then get rid of this special xmit() and do all header and TX preparations in hard_header(), it would be even better (cleaner) though IIRC it requires some changes to Ethernet(?) code. The changes have to be applied to the 4 synclink drivers and to most .c files in drivers/net/wan simultaneously, perhaps it's more practical if I do it?
--- a/drivers/net/wan/hdlc.c 2009-01-07 14:08:43.510247589 -0800 +++ b/drivers/net/wan/hdlc.c 2009-01-07 14:09:39.093495837 -0800 @@ -40,6 +40,8 @@ static const char* version = "HDLC support module revision 1.22"; +static const struct header_ops hdlc_null_ops; + #undef DEBUG_LINK static struct hdlc_proto *first_proto; @@ -218,8 +220,6 @@ int hdlc_ioctl(struct net_device *dev, s return -EINVAL; } -static const struct header_ops hdlc_null_ops; - static void hdlc_setup_dev(struct net_device *dev) { /* Re-init all variables changed by HDLC protocol drivers,
Fix build problem with hdlc driver. hdlc_null_ops was being referenced before it was defined! Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html