On Fri, Aug 6, 2010 at 11:05 AM, Simon Horman <span dir="ltr"><<a href="mailto:horms@verge.net.au" target="_blank">horms@verge.net.au</a>></span> wrote:<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+#ifndef HAVE_BR_PORT<br>
+#define vport_get_rcu(dev) \<br>
+ ((struct vport *) rcu_dereference(dev->rx_handler_data))<br></blockquote><div><br></div><div>I would just drop this into netdev_get_vport() and call that.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+#define vort_get(dev) ((struct vport *) dev->rx_handler_data)<br></blockquote><div><br></div><div>Is this actually used anywhere?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+/* XXX: Add IFF_OVS_DATAPATH to include/linux/if.h and use it<br>
+ * in place of IFF_VPORT in this file. */<br></blockquote><div><br></div><div>We could could do this with compatibility code: use the desired name in this file and then define it to the appropriate value in compat/include/linux/if.h. On pre-2.6.36 kernels it could be defined to 0, defined to IFF_BRIDGE_PORT for later kernels, and then we wouldn't need to define it all at once we make it upstream. This would remove a bunch of #ifdef's from this file.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- if (netdev_vport->dev->br_port) {<br>
+#ifdef HAVE_BR_PORT<br>
+ if (netdev_vport->dev->br_port)<br>
+#else<br>
+ if (netdev_vport->dev->priv_flags & IFF_BRIDGE_PORT)<br>
+#endif<br>
+ {<br>
err = -EBUSY;<br>
goto error_put;<br>
}<br></blockquote><div><br></div><div>I believe that we actually don't need this block of code here anymore. On older kernel versions it should really be moved to netdev_attach() and on newer versions the check is handled implicitly by netdev_rx_handler_register().</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+#ifdef HAVE_BR_PORT<br>
rcu_assign_pointer(netdev_vport->dev->br_port,<br>
(struct net_bridge_port *)vport);<br>
+ vport = NULL;<br>
+#endif<br>
#ifndef HAVE_BR_HANDLE_FRAME_HOOK<br>
err = netdev_rx_handler_register(netdev_vport->dev,<br>
- netdev_frame_hook, NULL);<br>
+ netdev_frame_hook, vport);<br>
+ if (err)<br>
+ return err;<br>
#endif<br>
- return err;<br>
+#ifndef HAVE_BR_PORT<br>
+ netdev_vport->dev->priv_flags |= IFF_BRIDGE_PORT;<br>
+#endif<br></blockquote><div><br></div><div>It looks like you are trying to handle the various possibilities for commit level changes. We'll just go crazy if we try to do that and it makes the code much harder to read. It's much cleaner if we do:</div>
<div><br></div><div>#if < 2.6.36</div><div>assign to dev->br_port</div><div>#else</div><div>use netdev_rx_register_handler()</div><div>#endif</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
struct vport *netdev_get_vport(struct net_device *dev)<br>
{<br>
+#ifdef HAVE_BR_PORT<br>
return (struct vport *)dev->br_port;<br></blockquote><div><br></div><div>If we make this a more general function we should probably add an rcu_dereference() here for consistency even if it isn't strictly required.</div>
<div><br></div><div>There's also a block of code at the bottom of the file that is a hack to enforce mutual exclusion with the bridge. Now that we can safely coexist with the bridge, it can be #if'd out on appropriate kernels.</div>
</div>