| Message ID | 1357751882-8619-2-git-send-email-vyasevic@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 9 Jan 2013 12:17:48 -0500 Vlad Yasevich <vyasevic@redhat.com> wrote: > > /** > + * vlan_hw_buggy - Check to see if VLAN hw acceleration is supported. > + * @dev: netdevice of the lowerdev/hw nic > + * > + * Checks to see if HW and driver report VLAN acceleration correctly. > + */ > +static inline bool vlan_hw_buggy(const struct net_device *dev) > +{ > + const struct net_device_ops *ops = dev->netdev_ops; > + > + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && > + (!ops->ndo_vlan_rx_add_vid || !ops->ndo_vlan_rx_kill_vid)) > + return true; > + > + return false; > +} > + > +/** > + * vlan_vid_add_hw - Add the VLAN vid to the HW filter > + * @dev: netdevice of the lowerdev/hw nic > + * @vid: vlan id. > + * > + * Inserts the vid into the HW vlan filter table if hw supports it. > + */ > +static inline int vlan_vid_add_hw(struct net_device *dev, > + unsigned short vid) > +{ > + const struct net_device_ops *ops = dev->netdev_ops; > + int err = 0; > + > + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && > + ops->ndo_vlan_rx_add_vid) > + err = ops->ndo_vlan_rx_add_vid(dev, vid); > + > + return err; > +} > + > +/** > + * vlan_vid_del_hw - Delete the VLAN vid from the HW filter > + * @dev: netdevice of the lowerdev/hw nic > + * @vid: vlan id. > + * > + * Delete the vid from the HW vlan filter table if hw supports it. > + */ > +static inline int vlan_vid_del_hw(struct net_device *dev, > + unsigned short vid) > +{ > + const struct net_device_ops *ops = dev->netdev_ops; > + int err = 0; > + > + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && > + ops->ndo_vlan_rx_kill_vid) > + err = ops->ndo_vlan_rx_add_vid(dev, vid); > + > + return err; > +} > + I would rather not have all these inline's. This isn't performance critical. Also, the check for buggy devices should be done inside the vlan code, not repeated in the functions using the add/remove API. When device is registered the flag and add/kill should be checked, and if the device driver is buggy it should fail the register_netdevice. -- 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
On 01/10/2013 01:25 PM, Stephen Hemminger wrote: > On Wed, 9 Jan 2013 12:17:48 -0500 > Vlad Yasevich <vyasevic@redhat.com> wrote: > >> >> /** >> + * vlan_hw_buggy - Check to see if VLAN hw acceleration is supported. >> + * @dev: netdevice of the lowerdev/hw nic >> + * >> + * Checks to see if HW and driver report VLAN acceleration correctly. >> + */ >> +static inline bool vlan_hw_buggy(const struct net_device *dev) >> +{ >> + const struct net_device_ops *ops = dev->netdev_ops; >> + >> + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && >> + (!ops->ndo_vlan_rx_add_vid || !ops->ndo_vlan_rx_kill_vid)) >> + return true; >> + >> + return false; >> +} >> + >> +/** >> + * vlan_vid_add_hw - Add the VLAN vid to the HW filter >> + * @dev: netdevice of the lowerdev/hw nic >> + * @vid: vlan id. >> + * >> + * Inserts the vid into the HW vlan filter table if hw supports it. >> + */ >> +static inline int vlan_vid_add_hw(struct net_device *dev, >> + unsigned short vid) >> +{ >> + const struct net_device_ops *ops = dev->netdev_ops; >> + int err = 0; >> + >> + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && >> + ops->ndo_vlan_rx_add_vid) >> + err = ops->ndo_vlan_rx_add_vid(dev, vid); >> + >> + return err; >> +} >> + >> +/** >> + * vlan_vid_del_hw - Delete the VLAN vid from the HW filter >> + * @dev: netdevice of the lowerdev/hw nic >> + * @vid: vlan id. >> + * >> + * Delete the vid from the HW vlan filter table if hw supports it. >> + */ >> +static inline int vlan_vid_del_hw(struct net_device *dev, >> + unsigned short vid) >> +{ >> + const struct net_device_ops *ops = dev->netdev_ops; >> + int err = 0; >> + >> + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && >> + ops->ndo_vlan_rx_kill_vid) >> + err = ops->ndo_vlan_rx_add_vid(dev, vid); >> + >> + return err; >> +} >> + > > I would rather not have all these inline's. This isn't performance critical. I kind of need to keep them as inlines because of the VLAN support is built. Right now, none of the VLAN files are build if VLAN support is turned off. So all we get access to are inlines from if_vlan.h. I suppose I can change how VLANs get built, but not if that's the right thing. It looks like it is set up the way it is on purpose. > Also, the check for buggy devices should be done inside the vlan code, > not repeated in the functions using the add/remove API. When device is > registered the flag and add/kill should be checked, and if the device driver > is buggy it should fail the register_netdevice. > Not sure what you mean here. I don't check if it's buggy again. I check that the device supports filter and the pointer is set. I does exactly what the code used to do. I suppose that the checks for valid function pointers may be a little redundant since otherwise vlan_hw_buggy() would have triggered, but it's safer to have them since we can't guarantee that other users have checked for buggy implementations. -vlad -- 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
On Thu, 10 Jan 2013 13:41:58 -0500 Vlad Yasevich <vyasevic@redhat.com> wrote: > On 01/10/2013 01:25 PM, Stephen Hemminger wrote: > > On Wed, 9 Jan 2013 12:17:48 -0500 > > Vlad Yasevich <vyasevic@redhat.com> wrote: > > > >> > >> /** > >> + * vlan_hw_buggy - Check to see if VLAN hw acceleration is supported. > >> + * @dev: netdevice of the lowerdev/hw nic > >> + * > >> + * Checks to see if HW and driver report VLAN acceleration correctly. > >> + */ > >> +static inline bool vlan_hw_buggy(const struct net_device *dev) > >> +{ > >> + const struct net_device_ops *ops = dev->netdev_ops; > >> + > >> + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && > >> + (!ops->ndo_vlan_rx_add_vid || !ops->ndo_vlan_rx_kill_vid)) > >> + return true; > >> + > >> + return false; > >> +} > >> + > >> +/** > >> + * vlan_vid_add_hw - Add the VLAN vid to the HW filter > >> + * @dev: netdevice of the lowerdev/hw nic > >> + * @vid: vlan id. > >> + * > >> + * Inserts the vid into the HW vlan filter table if hw supports it. > >> + */ > >> +static inline int vlan_vid_add_hw(struct net_device *dev, > >> + unsigned short vid) > >> +{ > >> + const struct net_device_ops *ops = dev->netdev_ops; > >> + int err = 0; > >> + > >> + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && > >> + ops->ndo_vlan_rx_add_vid) > >> + err = ops->ndo_vlan_rx_add_vid(dev, vid); > >> + > >> + return err; > >> +} > >> + > >> +/** > >> + * vlan_vid_del_hw - Delete the VLAN vid from the HW filter > >> + * @dev: netdevice of the lowerdev/hw nic > >> + * @vid: vlan id. > >> + * > >> + * Delete the vid from the HW vlan filter table if hw supports it. > >> + */ > >> +static inline int vlan_vid_del_hw(struct net_device *dev, > >> + unsigned short vid) > >> +{ > >> + const struct net_device_ops *ops = dev->netdev_ops; > >> + int err = 0; > >> + > >> + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && > >> + ops->ndo_vlan_rx_kill_vid) > >> + err = ops->ndo_vlan_rx_add_vid(dev, vid); > >> + > >> + return err; > >> +} > >> + > > > > I would rather not have all these inline's. This isn't performance critical. > > I kind of need to keep them as inlines because of the VLAN support is > built. Right now, none of the VLAN files are build if VLAN support is > turned off. So all we get access to are inlines from if_vlan.h. > > I suppose I can change how VLANs get built, but not if that's the right > thing. It looks like it is set up the way it is on purpose. > > > Also, the check for buggy devices should be done inside the vlan code, > > not repeated in the functions using the add/remove API. When device is > > registered the flag and add/kill should be checked, and if the device driver > > is buggy it should fail the register_netdevice. > > > > Not sure what you mean here. I don't check if it's buggy again. I > check that the device supports filter and the pointer is set. I does > exactly what the code used to do. I suppose that the checks for valid > function pointers may be a little redundant since otherwise > vlan_hw_buggy() would have triggered, but it's safer to have them since > we can't guarantee that other users have checked for buggy implementations. > > -vlad The best way to handle this is to add stubs for the unconfigure case, and include real code if configured. I.e something like #if IS_ENABLED(CONFIG_VLAN_8012Q) extern int vlan_vid_add_hw(struct net_device *, unsigned short); extern int vlan_vid_del_hw(struct net_device *, unsigned short); #else #define vlan_vid_add_hw(dev,vid) (-ENOTSUPP) #define vlan_vid_del_hw(dev,vid) (-ENOTSUPP) #endif -- 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
On 01/10/2013 05:07 PM, Stephen Hemminger wrote: > On Thu, 10 Jan 2013 13:41:58 -0500 > Vlad Yasevich <vyasevic@redhat.com> wrote: > >> On 01/10/2013 01:25 PM, Stephen Hemminger wrote: >>> On Wed, 9 Jan 2013 12:17:48 -0500 >>> Vlad Yasevich <vyasevic@redhat.com> wrote: >>> >>>> >>>> /** >>>> + * vlan_hw_buggy - Check to see if VLAN hw acceleration is supported. >>>> + * @dev: netdevice of the lowerdev/hw nic >>>> + * >>>> + * Checks to see if HW and driver report VLAN acceleration correctly. >>>> + */ >>>> +static inline bool vlan_hw_buggy(const struct net_device *dev) >>>> +{ >>>> + const struct net_device_ops *ops = dev->netdev_ops; >>>> + >>>> + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && >>>> + (!ops->ndo_vlan_rx_add_vid || !ops->ndo_vlan_rx_kill_vid)) >>>> + return true; >>>> + >>>> + return false; >>>> +} >>>> + >>>> +/** >>>> + * vlan_vid_add_hw - Add the VLAN vid to the HW filter >>>> + * @dev: netdevice of the lowerdev/hw nic >>>> + * @vid: vlan id. >>>> + * >>>> + * Inserts the vid into the HW vlan filter table if hw supports it. >>>> + */ >>>> +static inline int vlan_vid_add_hw(struct net_device *dev, >>>> + unsigned short vid) >>>> +{ >>>> + const struct net_device_ops *ops = dev->netdev_ops; >>>> + int err = 0; >>>> + >>>> + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && >>>> + ops->ndo_vlan_rx_add_vid) >>>> + err = ops->ndo_vlan_rx_add_vid(dev, vid); >>>> + >>>> + return err; >>>> +} >>>> + >>>> +/** >>>> + * vlan_vid_del_hw - Delete the VLAN vid from the HW filter >>>> + * @dev: netdevice of the lowerdev/hw nic >>>> + * @vid: vlan id. >>>> + * >>>> + * Delete the vid from the HW vlan filter table if hw supports it. >>>> + */ >>>> +static inline int vlan_vid_del_hw(struct net_device *dev, >>>> + unsigned short vid) >>>> +{ >>>> + const struct net_device_ops *ops = dev->netdev_ops; >>>> + int err = 0; >>>> + >>>> + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && >>>> + ops->ndo_vlan_rx_kill_vid) >>>> + err = ops->ndo_vlan_rx_add_vid(dev, vid); >>>> + >>>> + return err; >>>> +} >>>> + >>> >>> I would rather not have all these inline's. This isn't performance critical. >> >> I kind of need to keep them as inlines because of the VLAN support is >> built. Right now, none of the VLAN files are build if VLAN support is >> turned off. So all we get access to are inlines from if_vlan.h. >> >> I suppose I can change how VLANs get built, but not if that's the right >> thing. It looks like it is set up the way it is on purpose. >> >>> Also, the check for buggy devices should be done inside the vlan code, >>> not repeated in the functions using the add/remove API. When device is >>> registered the flag and add/kill should be checked, and if the device driver >>> is buggy it should fail the register_netdevice. >>> >> >> Not sure what you mean here. I don't check if it's buggy again. I >> check that the device supports filter and the pointer is set. I does >> exactly what the code used to do. I suppose that the checks for valid >> function pointers may be a little redundant since otherwise >> vlan_hw_buggy() would have triggered, but it's safer to have them since >> we can't guarantee that other users have checked for buggy implementations. >> >> -vlad > > The best way to handle this is to add stubs for the unconfigure case, and > include real code if configured. > > I.e something like > > #if IS_ENABLED(CONFIG_VLAN_8012Q) > extern int vlan_vid_add_hw(struct net_device *, unsigned short); > extern int vlan_vid_del_hw(struct net_device *, unsigned short); > #else > #define vlan_vid_add_hw(dev,vid) (-ENOTSUPP) > #define vlan_vid_del_hw(dev,vid) (-ENOTSUPP) > #endif > > But that wouldn't be right as this would prevent HW filtering from working when VLAN support isn't built in which defeats the whole point of extracting this code out it its own functions so it can be re-used. Either we always build in some parts of VLAN support or we make keep these inlines. -vlad > -- 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
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h index d06cc5c..e1deb91 100644 --- a/include/linux/if_vlan.h +++ b/include/linux/if_vlan.h @@ -158,6 +158,63 @@ static inline bool vlan_uses_dev(const struct net_device *dev) #endif /** + * vlan_hw_buggy - Check to see if VLAN hw acceleration is supported. + * @dev: netdevice of the lowerdev/hw nic + * + * Checks to see if HW and driver report VLAN acceleration correctly. + */ +static inline bool vlan_hw_buggy(const struct net_device *dev) +{ + const struct net_device_ops *ops = dev->netdev_ops; + + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && + (!ops->ndo_vlan_rx_add_vid || !ops->ndo_vlan_rx_kill_vid)) + return true; + + return false; +} + +/** + * vlan_vid_add_hw - Add the VLAN vid to the HW filter + * @dev: netdevice of the lowerdev/hw nic + * @vid: vlan id. + * + * Inserts the vid into the HW vlan filter table if hw supports it. + */ +static inline int vlan_vid_add_hw(struct net_device *dev, + unsigned short vid) +{ + const struct net_device_ops *ops = dev->netdev_ops; + int err = 0; + + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && + ops->ndo_vlan_rx_add_vid) + err = ops->ndo_vlan_rx_add_vid(dev, vid); + + return err; +} + +/** + * vlan_vid_del_hw - Delete the VLAN vid from the HW filter + * @dev: netdevice of the lowerdev/hw nic + * @vid: vlan id. + * + * Delete the vid from the HW vlan filter table if hw supports it. + */ +static inline int vlan_vid_del_hw(struct net_device *dev, + unsigned short vid) +{ + const struct net_device_ops *ops = dev->netdev_ops; + int err = 0; + + if ((dev->features & NETIF_F_HW_VLAN_FILTER) && + ops->ndo_vlan_rx_kill_vid) + err = ops->ndo_vlan_rx_add_vid(dev, vid); + + return err; +} + +/** * vlan_insert_tag - regular VLAN tag inserting * @skb: skbuff to tag * @vlan_tci: VLAN TCI to insert diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c index babfde9..540d759 100644 --- a/net/8021q/vlan.c +++ b/net/8021q/vlan.c @@ -117,15 +117,13 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head) int vlan_check_real_dev(struct net_device *real_dev, u16 vlan_id) { const char *name = real_dev->name; - const struct net_device_ops *ops = real_dev->netdev_ops; if (real_dev->features & NETIF_F_VLAN_CHALLENGED) { pr_info("VLANs not supported on %s\n", name); return -EOPNOTSUPP; } - if ((real_dev->features & NETIF_F_HW_VLAN_FILTER) && - (!ops->ndo_vlan_rx_add_vid || !ops->ndo_vlan_rx_kill_vid)) { + if (vlan_hw_buggy(real_dev)) { pr_info("Device %s has buggy VLAN hw accel\n", name); return -EOPNOTSUPP; } diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c index 380440b..04a1f03 100644 --- a/net/8021q/vlan_core.c +++ b/net/8021q/vlan_core.c @@ -216,7 +216,6 @@ static int __vlan_vid_add(struct vlan_info *vlan_info, unsigned short vid, struct vlan_vid_info **pvid_info) { struct net_device *dev = vlan_info->real_dev; - const struct net_device_ops *ops = dev->netdev_ops; struct vlan_vid_info *vid_info; int err; @@ -224,13 +223,10 @@ static int __vlan_vid_add(struct vlan_info *vlan_info, unsigned short vid, if (!vid_info) return -ENOMEM; - if ((dev->features & NETIF_F_HW_VLAN_FILTER) && - ops->ndo_vlan_rx_add_vid) { - err = ops->ndo_vlan_rx_add_vid(dev, vid); - if (err) { - kfree(vid_info); - return err; - } + err = vlan_vid_add_hw(dev, vid); + if (err) { + kfree(vid_info); + return err; } list_add(&vid_info->list, &vlan_info->vid_list); vlan_info->nr_vids++; @@ -278,17 +274,13 @@ static void __vlan_vid_del(struct vlan_info *vlan_info, struct vlan_vid_info *vid_info) { struct net_device *dev = vlan_info->real_dev; - const struct net_device_ops *ops = dev->netdev_ops; unsigned short vid = vid_info->vid; int err; - if ((dev->features & NETIF_F_HW_VLAN_FILTER) && - ops->ndo_vlan_rx_kill_vid) { - err = ops->ndo_vlan_rx_kill_vid(dev, vid); - if (err) { - pr_warn("failed to kill vid %d for device %s\n", - vid, dev->name); - } + err = vlan_vid_del_hw(dev, vid); + if (err) { + pr_warn("failed to kill vid %d for device %s\n", + vid, dev->name); } list_del(&vid_info->list); kfree(vid_info);
Wrap VLAN hardware acceleration calls into separate functions. This way other code can re-use it. Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> --- include/linux/if_vlan.h | 57 +++++++++++++++++++++++++++++++++++++++++++++++ net/8021q/vlan.c | 4 +-- net/8021q/vlan_core.c | 24 ++++++------------- 3 files changed, 66 insertions(+), 19 deletions(-)