[patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

[patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

[Jiri Pirko]

Some devices are not able to enable/disable rx/tw vlan accel separately.
they depend on ndo_vlan_rx_register to know if to enable of disable
hw accel. And since ndo_vlan_rx_register is going to die soon,
this must be resolved.

One solution might be to enable accel on device start every time, even
if there are no vlan up on. But this would change behaviour and might
lead to possible regression (on older devices).

Better is to preserve current behaviour and introduce a couple of ndos
to let device driver know when to enable/disable vlan accel. They are
alled at the same time as ndo_vlan_rx_register.

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 include/linux/netdevice.h |   14 ++++++++++++++
 net/8021q/vlan.c          |    4 ++++
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f84dfd2..8a69f33 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -775,6 +775,18 @@ struct netdev_tc_txq {
  *	when vlan groups for the device changes.  Note: grp is NULL
  *	if no vlan's groups are being used.
  *
+ * void (*ndo_vlan_enable)(struct net_device *dev);
+ *	This function is called when first vlan is registered on this device.
+ *	This is primary to be implemented for devices what can enable rx/tx
+ *	vlan accel only at one. Otherwise enabling of vlan accel should be
+ *	implemented using ndo_set_features.
+ *
+ * void (*ndo_vlan_disable)(struct net_device *dev);
+ *	This function is called when last vlan is unregistered on this device.
+ *	This is primary to be implemented for devices what can disable rx/tx
+ *	vlan accel only at one. Otherwise disabling of vlan accel should be
+ *	implemented using ndo_set_features.
+ *
  * void (*ndo_vlan_rx_add_vid)(struct net_device *dev, unsigned short vid);
  *	If device support VLAN filtering (dev->features & NETIF_F_HW_VLAN_FILTER)
  *	this function is called when a VLAN id is registered.
@@ -895,6 +907,8 @@ struct net_device_ops {
 
 	void			(*ndo_vlan_rx_register)(struct net_device *dev,
 						        struct vlan_group *grp);
+	void			(*ndo_vlan_enable)(struct net_device *dev);
+	void			(*ndo_vlan_disable)(struct net_device *dev);
 	void			(*ndo_vlan_rx_add_vid)(struct net_device *dev,
 						       unsigned short vid);
 	void			(*ndo_vlan_rx_kill_vid)(struct net_device *dev,
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index d24c464..051c3cd 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -136,6 +136,8 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
 		rcu_assign_pointer(real_dev->vlgrp, NULL);
 		if (ops->ndo_vlan_rx_register)
 			ops->ndo_vlan_rx_register(real_dev, NULL);
+		if (ops->ndo_vlan_disable)
+			ops->ndo_vlan_disable(real_dev);
 
 		/* Free the group, after all cpu's are done. */
 		call_rcu(&grp->rcu, vlan_rcu_free);
@@ -209,6 +211,8 @@ int register_vlan_dev(struct net_device *dev)
 	if (ngrp) {
 		if (ops->ndo_vlan_rx_register && (real_dev->features & NETIF_F_HW_VLAN_RX))
 			ops->ndo_vlan_rx_register(real_dev, ngrp);
+		if (ops->ndo_vlan_enable)
+			ops->ndo_vlan_enable(real_dev);
 		rcu_assign_pointer(real_dev->vlgrp, ngrp);
 	}
 	if (real_dev->features & NETIF_F_HW_VLAN_FILTER)
-- 
1.7.6

Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

[Michał Mirosław]

2011/7/16 Jiri Pirko <jpirko@redhat.com>:
> Some devices are not able to enable/disable rx/tw vlan accel separately.
> they depend on ndo_vlan_rx_register to know if to enable of disable
> hw accel. And since ndo_vlan_rx_register is going to die soon,
> this must be resolved.
>
> One solution might be to enable accel on device start every time, even
> if there are no vlan up on. But this would change behaviour and might
> lead to possible regression (on older devices).
[...]

Please describe the possible regression. As I see it, there won't be
any user visible change of behaviour - network code takes care of
reinserting VLAN tag when necessary. If you think that disabling tag
stripping is beneficial for cases where no VLANs are configured, it's
better to do that in netdev_fix_features() for devices which advertise
NETIF_F_HW_VLAN_RX in hw_features.

Best Regards,
Michał Mirosław

Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

[Jiri Pirko]

Sat, Jul 16, 2011 at 04:14:36PM CEST, mirqus@gmail.com wrote:
>2011/7/16 Jiri Pirko <jpirko@redhat.com>:
>> Some devices are not able to enable/disable rx/tw vlan accel separately.
>> they depend on ndo_vlan_rx_register to know if to enable of disable
>> hw accel. And since ndo_vlan_rx_register is going to die soon,
>> this must be resolved.
>>
>> One solution might be to enable accel on device start every time, even
>> if there are no vlan up on. But this would change behaviour and might
>> lead to possible regression (on older devices).
>[...]
>
>Please describe the possible regression. As I see it, there won't be
>any user visible change of behaviour - network code takes care of
>reinserting VLAN tag when necessary. If you think that disabling tag
>stripping is beneficial for cases where no VLANs are configured, it's
>better to do that in netdev_fix_features() for devices which advertise
>NETIF_F_HW_VLAN_RX in hw_features.

Well I just wanted to preserve current behaviour which is that in many
drivers vlan accel is enabled only if some vid is registered upon the
device and it's disabled again when no vid is registered. I can see
no way to do this with current code after removing ndo_vlan_rx_register.

I expect unexpected problems on old cards when vlan accel would be
enabled all the time, but maybe I'm wrong...

One idea is for device which do not support sepatate rx/tx vlan accel
enabling/disabling they can probably use ndo_fix_features force to 
enable/disable rx/tx pair together. That would resolve the situation as
well giving user possibility to turn off vlan accel in case of any issues.

>
>Best Regards,
>Michał Mirosław

Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

[Michał Mirosław]

W dniu 17 lipca 2011 09:30 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
> Sat, Jul 16, 2011 at 04:14:36PM CEST, mirqus@gmail.com wrote:
>>2011/7/16 Jiri Pirko <jpirko@redhat.com>:
>>> Some devices are not able to enable/disable rx/tw vlan accel separately.
>>> they depend on ndo_vlan_rx_register to know if to enable of disable
>>> hw accel. And since ndo_vlan_rx_register is going to die soon,
>>> this must be resolved.
>>>
>>> One solution might be to enable accel on device start every time, even
>>> if there are no vlan up on. But this would change behaviour and might
>>> lead to possible regression (on older devices).
>>[...]
>>
>>Please describe the possible regression. As I see it, there won't be
>>any user visible change of behaviour - network code takes care of
>>reinserting VLAN tag when necessary. If you think that disabling tag
>>stripping is beneficial for cases where no VLANs are configured, it's
>>better to do that in netdev_fix_features() for devices which advertise
>>NETIF_F_HW_VLAN_RX in hw_features.
>
> Well I just wanted to preserve current behaviour which is that in many
> drivers vlan accel is enabled only if some vid is registered upon the
> device and it's disabled again when no vid is registered. I can see
> no way to do this with current code after removing ndo_vlan_rx_register.
>
> I expect unexpected

:-D

> ... problems on old cards when vlan accel would be
> enabled all the time, but maybe I'm wrong...

Device has no way of knowing how the system uses VLAN tags, stripped
or not. Any problems would be driver problems and since you're making
it all use generic code, bugs will hit all drivers simultaneously or
(preferably) won't happen at all.

> One idea is for device which do not support sepatate rx/tx vlan accel
> enabling/disabling they can probably use ndo_fix_features force to
> enable/disable rx/tx pair together. That would resolve the situation as
> well giving user possibility to turn off vlan accel in case of any issues.

That is exactly the idea behind ndo_fix_features.

Best Regards,
Michał Mirosław

Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

[Jiri Pirko]

Sun, Jul 17, 2011 at 10:36:04AM CEST, mirqus@gmail.com wrote:
>W dniu 17 lipca 2011 09:30 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>> Sat, Jul 16, 2011 at 04:14:36PM CEST, mirqus@gmail.com wrote:
>>>2011/7/16 Jiri Pirko <jpirko@redhat.com>:
>>>> Some devices are not able to enable/disable rx/tw vlan accel separately.
>>>> they depend on ndo_vlan_rx_register to know if to enable of disable
>>>> hw accel. And since ndo_vlan_rx_register is going to die soon,
>>>> this must be resolved.
>>>>
>>>> One solution might be to enable accel on device start every time, even
>>>> if there are no vlan up on. But this would change behaviour and might
>>>> lead to possible regression (on older devices).
>>>[...]
>>>
>>>Please describe the possible regression. As I see it, there won't be
>>>any user visible change of behaviour - network code takes care of
>>>reinserting VLAN tag when necessary. If you think that disabling tag
>>>stripping is beneficial for cases where no VLANs are configured, it's
>>>better to do that in netdev_fix_features() for devices which advertise
>>>NETIF_F_HW_VLAN_RX in hw_features.
>>
>> Well I just wanted to preserve current behaviour which is that in many
>> drivers vlan accel is enabled only if some vid is registered upon the
>> device and it's disabled again when no vid is registered. I can see
>> no way to do this with current code after removing ndo_vlan_rx_register.
>>
>> I expect unexpected
>
>:-D
>
>> ... problems on old cards when vlan accel would be
>> enabled all the time, but maybe I'm wrong...
>
>Device has no way of knowing how the system uses VLAN tags, stripped
>or not. Any problems would be driver problems and since you're making
>it all use generic code, bugs will hit all drivers simultaneously or
>(preferably) won't happen at all.
>
>> One idea is for device which do not support sepatate rx/tx vlan accel
>> enabling/disabling they can probably use ndo_fix_features force to
>> enable/disable rx/tx pair together. That would resolve the situation as
>> well giving user possibility to turn off vlan accel in case of any issues.
>
>That is exactly the idea behind ndo_fix_features.

In netdev_fix_features add check if either one of NETIF_F_HW_VLAN_TX or
NETIF_F_HW_VLAN_RX is set and in that case set the other one. Of course
this would be done only for devices what do not support separate rx/tx
vlan on/off. But how to distinguish? NETIF_F_HW_VLAN_BOTH feature flag?

Thanks.

Jirka

>
>Best Regards,
>Michał Mirosław

Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

[Michał Mirosław]

W dniu 17 lipca 2011 21:44 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
> Sun, Jul 17, 2011 at 10:36:04AM CEST, mirqus@gmail.com wrote:
>>W dniu 17 lipca 2011 09:30 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>>> Sat, Jul 16, 2011 at 04:14:36PM CEST, mirqus@gmail.com wrote:
>>>>2011/7/16 Jiri Pirko <jpirko@redhat.com>:
>>>>> Some devices are not able to enable/disable rx/tw vlan accel separately.
>>>>> they depend on ndo_vlan_rx_register to know if to enable of disable
>>>>> hw accel. And since ndo_vlan_rx_register is going to die soon,
>>>>> this must be resolved.
>>>>>
>>>>> One solution might be to enable accel on device start every time, even
>>>>> if there are no vlan up on. But this would change behaviour and might
>>>>> lead to possible regression (on older devices).
>>>>[...]
>>>>
>>>>Please describe the possible regression. As I see it, there won't be
>>>>any user visible change of behaviour - network code takes care of
>>>>reinserting VLAN tag when necessary. If you think that disabling tag
>>>>stripping is beneficial for cases where no VLANs are configured, it's
>>>>better to do that in netdev_fix_features() for devices which advertise
>>>>NETIF_F_HW_VLAN_RX in hw_features.
>>>
>>> Well I just wanted to preserve current behaviour which is that in many
>>> drivers vlan accel is enabled only if some vid is registered upon the
>>> device and it's disabled again when no vid is registered. I can see
>>> no way to do this with current code after removing ndo_vlan_rx_register.
>>>
>>> I expect unexpected
>>
>>:-D
>>
>>> ... problems on old cards when vlan accel would be
>>> enabled all the time, but maybe I'm wrong...
>>
>>Device has no way of knowing how the system uses VLAN tags, stripped
>>or not. Any problems would be driver problems and since you're making
>>it all use generic code, bugs will hit all drivers simultaneously or
>>(preferably) won't happen at all.
>>
>>> One idea is for device which do not support sepatate rx/tx vlan accel
>>> enabling/disabling they can probably use ndo_fix_features force to
>>> enable/disable rx/tx pair together. That would resolve the situation as
>>> well giving user possibility to turn off vlan accel in case of any issues.
>>
>>That is exactly the idea behind ndo_fix_features.

> In netdev_fix_features add check if either one of NETIF_F_HW_VLAN_TX or
> NETIF_F_HW_VLAN_RX is set and in that case set the other one. Of course
> this would be done only for devices what do not support separate rx/tx
> vlan on/off. But how to distinguish? NETIF_F_HW_VLAN_BOTH feature flag?

Not in netdev_fix_features(). This case you describe should be handled
in driver-specific
ndo_fix_features callback. Because if NETIF_F_HW_VLAN_TX is disabled,
the network core will insert the tag anyway, this would cover the
restriction:

if (!(features & NEITF_F_HW_VLAN_RX))
  features &= ~NETIF_F_HW_VLAN_TX;

This works, because if HW_VLAN_TX is disabled, the driver never gets
packets which need tag insertion. Besides those already in the queue,
of course.

For netdev_fix_features() I thought that if no VLANs are configured, then it
could disable NETIF_F_HW_VLAN_RX (if set in hw_features). The
conditions for disabling it need some thought. For example, if the
device is not slave to some stacking device (bonding doesn't count, as
it passes VLAN registeration to its slaves) then packets with tags
would be either dropped or have tag reinserted (for network taps).

Best Regards,
Michał Mirosław

Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

[Jiri Pirko]

Sun, Jul 17, 2011 at 11:06:57PM CEST, mirqus@gmail.com wrote:
>W dniu 17 lipca 2011 21:44 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>> Sun, Jul 17, 2011 at 10:36:04AM CEST, mirqus@gmail.com wrote:
>>>W dniu 17 lipca 2011 09:30 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>>>> Sat, Jul 16, 2011 at 04:14:36PM CEST, mirqus@gmail.com wrote:
>>>>>2011/7/16 Jiri Pirko <jpirko@redhat.com>:
>>>>>> Some devices are not able to enable/disable rx/tw vlan accel separately.
>>>>>> they depend on ndo_vlan_rx_register to know if to enable of disable
>>>>>> hw accel. And since ndo_vlan_rx_register is going to die soon,
>>>>>> this must be resolved.
>>>>>>
>>>>>> One solution might be to enable accel on device start every time, even
>>>>>> if there are no vlan up on. But this would change behaviour and might
>>>>>> lead to possible regression (on older devices).
>>>>>[...]
>>>>>
>>>>>Please describe the possible regression. As I see it, there won't be
>>>>>any user visible change of behaviour - network code takes care of
>>>>>reinserting VLAN tag when necessary. If you think that disabling tag
>>>>>stripping is beneficial for cases where no VLANs are configured, it's
>>>>>better to do that in netdev_fix_features() for devices which advertise
>>>>>NETIF_F_HW_VLAN_RX in hw_features.
>>>>
>>>> Well I just wanted to preserve current behaviour which is that in many
>>>> drivers vlan accel is enabled only if some vid is registered upon the
>>>> device and it's disabled again when no vid is registered. I can see
>>>> no way to do this with current code after removing ndo_vlan_rx_register.
>>>>
>>>> I expect unexpected
>>>
>>>:-D
>>>
>>>> ... problems on old cards when vlan accel would be
>>>> enabled all the time, but maybe I'm wrong...
>>>
>>>Device has no way of knowing how the system uses VLAN tags, stripped
>>>or not. Any problems would be driver problems and since you're making
>>>it all use generic code, bugs will hit all drivers simultaneously or
>>>(preferably) won't happen at all.
>>>
>>>> One idea is for device which do not support sepatate rx/tx vlan accel
>>>> enabling/disabling they can probably use ndo_fix_features force to
>>>> enable/disable rx/tx pair together. That would resolve the situation as
>>>> well giving user possibility to turn off vlan accel in case of any issues.
>>>
>>>That is exactly the idea behind ndo_fix_features.
>
>> In netdev_fix_features add check if either one of NETIF_F_HW_VLAN_TX or
>> NETIF_F_HW_VLAN_RX is set and in that case set the other one. Of course
>> this would be done only for devices what do not support separate rx/tx
>> vlan on/off. But how to distinguish? NETIF_F_HW_VLAN_BOTH feature flag?
>
>Not in netdev_fix_features(). This case you describe should be handled
>in driver-specific

Well since the code would be the same in many drivers I was thinking
about putting it in general code...

Anyway, would you please look at following example patch and tell me if
it looks good to you?

diff --git a/drivers/net/atl1c/atl1c.h b/drivers/net/atl1c/atl1c.h
index 0f481b9..ca70e16 100644
--- a/drivers/net/atl1c/atl1c.h
+++ b/drivers/net/atl1c/atl1c.h
@@ -555,7 +555,6 @@ struct atl1c_smb {
 struct atl1c_adapter {
 	struct net_device   *netdev;
 	struct pci_dev      *pdev;
-	struct vlan_group   *vlgrp;
 	struct napi_struct  napi;
 	struct atl1c_hw        hw;
 	struct atl1c_hw_stats  hw_stats;
diff --git a/drivers/net/atl1c/atl1c_main.c b/drivers/net/atl1c/atl1c_main.c
index 1269ba5..ad49ad0 100644
--- a/drivers/net/atl1c/atl1c_main.c
+++ b/drivers/net/atl1c/atl1c_main.c
@@ -411,29 +411,30 @@ static void atl1c_set_multi(struct net_device *netdev)
 	}
 }
 
-static void atl1c_vlan_rx_register(struct net_device *netdev,
-				   struct vlan_group *grp)
+static void __atl1c_vlan_mode(u32 features, u32 *mac_ctrl_data)
+{
+	/* NETIF_F_HW_VLAN_RX is always in same state as NETIF_F_HW_VLAN_TX */
+	if (features & NETIF_F_HW_VLAN_TX) {
+		/* enable VLAN tag insert/strip */
+		*mac_ctrl_data |= MAC_CTRL_RMV_VLAN;
+	} else {
+		/* disable VLAN tag insert/strip */
+		*mac_ctrl_data &= ~MAC_CTRL_RMV_VLAN;
+	}
+}
+
+static void atl1c_vlan_mode(struct net_device *netdev, u32 features)
 {
 	struct atl1c_adapter *adapter = netdev_priv(netdev);
 	struct pci_dev *pdev = adapter->pdev;
 	u32 mac_ctrl_data = 0;
 
 	if (netif_msg_pktdata(adapter))
-		dev_dbg(&pdev->dev, "atl1c_vlan_rx_register\n");
+		dev_dbg(&pdev->dev, "atl1c_vlan_mode\n");
 
 	atl1c_irq_disable(adapter);
-
-	adapter->vlgrp = grp;
 	AT_READ_REG(&adapter->hw, REG_MAC_CTRL, &mac_ctrl_data);
-
-	if (grp) {
-		/* enable VLAN tag insert/strip */
-		mac_ctrl_data |= MAC_CTRL_RMV_VLAN;
-	} else {
-		/* disable VLAN tag insert/strip */
-		mac_ctrl_data &= ~MAC_CTRL_RMV_VLAN;
-	}
-
+	__atl1c_vlan_mode(features, &mac_ctrl_data);
 	AT_WRITE_REG(&adapter->hw, REG_MAC_CTRL, mac_ctrl_data);
 	atl1c_irq_enable(adapter);
 }
@@ -443,9 +444,10 @@ static void atl1c_restore_vlan(struct atl1c_adapter *adapter)
 	struct pci_dev *pdev = adapter->pdev;
 
 	if (netif_msg_pktdata(adapter))
-		dev_dbg(&pdev->dev, "atl1c_restore_vlan !");
-	atl1c_vlan_rx_register(adapter->netdev, adapter->vlgrp);
+		dev_dbg(&pdev->dev, "atl1c_restore_vlan\n");
+	atl1c_vlan_mode(adapter->netdev, adapter->netdev->features);
 }
+
 /*
  * atl1c_set_mac - Change the Ethernet Address of the NIC
  * @netdev: network interface device structure
@@ -483,12 +485,38 @@ static void atl1c_set_rxbufsize(struct atl1c_adapter *adapter,
 
 static u32 atl1c_fix_features(struct net_device *netdev, u32 features)
 {
+	u32 changed = netdev->features ^ features;
+
+	/*
+	 * Since there is no support for separate rx/tx vlan accel
+	 * enable/disable make sure these are always set in pair.
+	 */
+	if ((changed & NETIF_F_HW_VLAN_TX && features & NETIF_F_HW_VLAN_TX) ||
+	    (changed & NETIF_F_HW_VLAN_RX && features & NETIF_F_HW_VLAN_RX))
+		features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
+	else
+		features &= ~(NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
+
 	if (netdev->mtu > MAX_TSO_FRAME_SIZE)
 		features &= ~(NETIF_F_TSO | NETIF_F_TSO6);
 
 	return features;
 }
 
+static int atl1c_set_features(struct net_device *netdev, u32 features)
+{
+	u32 changed = netdev->features ^ features;
+
+	/*
+	 * Test for NETIF_F_HW_VLAN_TX as it's paired with NETIF_F_HW_VLAN_RX
+	 * by atl1c_fix_features.
+	 */
+	if (changed & NETIF_F_HW_VLAN_TX)
+		atl1c_vlan_mode(netdev, features);
+
+	return 0;
+}
+
 /*
  * atl1c_change_mtu - Change the Maximum Transfer Unit
  * @netdev: network interface device structure
@@ -1433,8 +1461,7 @@ static void atl1c_setup_mac_ctrl(struct atl1c_adapter *adapter)
 	mac_ctrl_data |= ((hw->preamble_len & MAC_CTRL_PRMLEN_MASK) <<
 			MAC_CTRL_PRMLEN_SHIFT);
 
-	if (adapter->vlgrp)
-		mac_ctrl_data |= MAC_CTRL_RMV_VLAN;
+	__atl1c_vlan_mode(netdev->features, &mac_ctrl_data);
 
 	mac_ctrl_data |= MAC_CTRL_BC_EN;
 	if (netdev->flags & IFF_PROMISC)
@@ -1878,14 +1905,14 @@ rrs_checked:
 		skb_put(skb, length - ETH_FCS_LEN);
 		skb->protocol = eth_type_trans(skb, netdev);
 		atl1c_rx_checksum(adapter, skb, rrs);
-		if (unlikely(adapter->vlgrp) && rrs->word3 & RRS_VLAN_INS) {
+		if (rrs->word3 & RRS_VLAN_INS) {
 			u16 vlan;
 
 			AT_TAG_TO_VLAN(rrs->vlan_tag, vlan);
 			vlan = le16_to_cpu(vlan);
-			vlan_hwaccel_receive_skb(skb, adapter->vlgrp, vlan);
-		} else
-			netif_receive_skb(skb);
+			__vlan_hwaccel_put_tag(skb, vlan);
+		}
+		netif_receive_skb(skb);
 
 		(*work_done)++;
 		count++;
@@ -2507,8 +2534,7 @@ static int atl1c_suspend(struct device *dev)
 		/* clear phy interrupt */
 		atl1c_read_phy_reg(hw, MII_ISR, &mii_intr_status_data);
 		/* Config MAC Ctrl register */
-		if (adapter->vlgrp)
-			mac_ctrl_data |= MAC_CTRL_RMV_VLAN;
+		__atl1c_vlan_mode(netdev->features, &mac_ctrl_data);
 
 		/* magic packet maybe Broadcast&multicast&Unicast frame */
 		if (wufc & AT_WUFC_MAG)
@@ -2581,14 +2607,14 @@ static const struct net_device_ops atl1c_netdev_ops = {
 	.ndo_stop		= atl1c_close,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_start_xmit		= atl1c_xmit_frame,
-	.ndo_set_mac_address 	= atl1c_set_mac_addr,
+	.ndo_set_mac_address	= atl1c_set_mac_addr,
 	.ndo_set_multicast_list = atl1c_set_multi,
 	.ndo_change_mtu		= atl1c_change_mtu,
 	.ndo_fix_features	= atl1c_fix_features,
+	.ndo_set_features	= atl1c_set_features,
 	.ndo_do_ioctl		= atl1c_ioctl,
 	.ndo_tx_timeout		= atl1c_tx_timeout,
 	.ndo_get_stats		= atl1c_get_stats,
-	.ndo_vlan_rx_register	= atl1c_vlan_rx_register,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= atl1c_netpoll,
 #endif
@@ -2608,10 +2634,10 @@ static int atl1c_init_netdev(struct net_device *netdev, struct pci_dev *pdev)
 	netdev->hw_features =	NETIF_F_SG	   |
 				NETIF_F_HW_CSUM	   |
 				NETIF_F_HW_VLAN_TX |
+				NETIF_F_HW_VLAN_RX |
 				NETIF_F_TSO	   |
 				NETIF_F_TSO6;
-	netdev->features =	netdev->hw_features |
-				NETIF_F_HW_VLAN_RX;
+	netdev->features =	netdev->hw_features;
 	return 0;
 }
 
-- 
1.7.6

Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

[Michał Mirosław]

W dniu 18 lipca 2011 09:13 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
> Sun, Jul 17, 2011 at 11:06:57PM CEST, mirqus@gmail.com wrote:
>>W dniu 17 lipca 2011 21:44 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>>> Sun, Jul 17, 2011 at 10:36:04AM CEST, mirqus@gmail.com wrote:
>>>>W dniu 17 lipca 2011 09:30 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>>>>> Sat, Jul 16, 2011 at 04:14:36PM CEST, mirqus@gmail.com wrote:
>>>>>>2011/7/16 Jiri Pirko <jpirko@redhat.com>:
>>>>>>> Some devices are not able to enable/disable rx/tw vlan accel separately.
>>>>>>> they depend on ndo_vlan_rx_register to know if to enable of disable
>>>>>>> hw accel. And since ndo_vlan_rx_register is going to die soon,
>>>>>>> this must be resolved.
>>>>>>>
>>>>>>> One solution might be to enable accel on device start every time, even
>>>>>>> if there are no vlan up on. But this would change behaviour and might
>>>>>>> lead to possible regression (on older devices).
>>>>>>[...]
>>>>>>
>>>>>>Please describe the possible regression. As I see it, there won't be
>>>>>>any user visible change of behaviour - network code takes care of
>>>>>>reinserting VLAN tag when necessary. If you think that disabling tag
>>>>>>stripping is beneficial for cases where no VLANs are configured, it's
>>>>>>better to do that in netdev_fix_features() for devices which advertise
>>>>>>NETIF_F_HW_VLAN_RX in hw_features.
>>>>>
>>>>> Well I just wanted to preserve current behaviour which is that in many
>>>>> drivers vlan accel is enabled only if some vid is registered upon the
>>>>> device and it's disabled again when no vid is registered. I can see
>>>>> no way to do this with current code after removing ndo_vlan_rx_register.
>>>>>
>>>>> I expect unexpected
>>>>
>>>>:-D
>>>>
>>>>> ... problems on old cards when vlan accel would be
>>>>> enabled all the time, but maybe I'm wrong...
>>>>
>>>>Device has no way of knowing how the system uses VLAN tags, stripped
>>>>or not. Any problems would be driver problems and since you're making
>>>>it all use generic code, bugs will hit all drivers simultaneously or
>>>>(preferably) won't happen at all.
>>>>
>>>>> One idea is for device which do not support sepatate rx/tx vlan accel
>>>>> enabling/disabling they can probably use ndo_fix_features force to
>>>>> enable/disable rx/tx pair together. That would resolve the situation as
>>>>> well giving user possibility to turn off vlan accel in case of any issues.
>>>>
>>>>That is exactly the idea behind ndo_fix_features.
>>
>>> In netdev_fix_features add check if either one of NETIF_F_HW_VLAN_TX or
>>> NETIF_F_HW_VLAN_RX is set and in that case set the other one. Of course
>>> this would be done only for devices what do not support separate rx/tx
>>> vlan on/off. But how to distinguish? NETIF_F_HW_VLAN_BOTH feature flag?
>>
>>Not in netdev_fix_features(). This case you describe should be handled
>>in driver-specific
>
> Well since the code would be the same in many drivers I was thinking
> about putting it in general code...
>
> Anyway, would you please look at following example patch and tell me if
> it looks good to you?

[...]
>  static u32 atl1c_fix_features(struct net_device *netdev, u32 features)
>  {
> +       u32 changed = netdev->features ^ features;
> +
> +       /*
> +        * Since there is no support for separate rx/tx vlan accel
> +        * enable/disable make sure these are always set in pair.
> +        */
> +       if ((changed & NETIF_F_HW_VLAN_TX && features & NETIF_F_HW_VLAN_TX) ||
> +           (changed & NETIF_F_HW_VLAN_RX && features & NETIF_F_HW_VLAN_RX))
> +               features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
> +       else
> +               features &= ~(NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
> +

You ignored my hint about combined TX/RX offload. Is that on purpose?
Your code will toggle VLAN acceleration on every
netdev_update_features call when user requests one offload on and one
off.

BTW, the register flag name (MAC_CTRL_RMV_VLAN) suggests that it
controls only tag stripping. Was it tested or documented that this
also links with tag insertion?

[...]
> +static int atl1c_set_features(struct net_device *netdev, u32 features)
> +{
> +       u32 changed = netdev->features ^ features;
> +
> +       /*
> +        * Test for NETIF_F_HW_VLAN_TX as it's paired with NETIF_F_HW_VLAN_RX
> +        * by atl1c_fix_features.
> +        */
> +       if (changed & NETIF_F_HW_VLAN_TX)
> +               atl1c_vlan_mode(netdev, features);
> +

Test for RX is better, as it will match the name of control bit
(MAC_CTRL_RMV_VLAN).

Best Regards,
Michał Mirosław

Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

[Jiri Pirko]

Tue, Jul 19, 2011 at 09:24:29AM CEST, mirqus@gmail.com wrote:
>W dniu 18 lipca 2011 09:13 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>> Sun, Jul 17, 2011 at 11:06:57PM CEST, mirqus@gmail.com wrote:
>>>W dniu 17 lipca 2011 21:44 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>>>> Sun, Jul 17, 2011 at 10:36:04AM CEST, mirqus@gmail.com wrote:
>>>>>W dniu 17 lipca 2011 09:30 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>>>>>> Sat, Jul 16, 2011 at 04:14:36PM CEST, mirqus@gmail.com wrote:
>>>>>>>2011/7/16 Jiri Pirko <jpirko@redhat.com>:
>>>>>>>> Some devices are not able to enable/disable rx/tw vlan accel separately.
>>>>>>>> they depend on ndo_vlan_rx_register to know if to enable of disable
>>>>>>>> hw accel. And since ndo_vlan_rx_register is going to die soon,
>>>>>>>> this must be resolved.
>>>>>>>>
>>>>>>>> One solution might be to enable accel on device start every time, even
>>>>>>>> if there are no vlan up on. But this would change behaviour and might
>>>>>>>> lead to possible regression (on older devices).
>>>>>>>[...]
>>>>>>>
>>>>>>>Please describe the possible regression. As I see it, there won't be
>>>>>>>any user visible change of behaviour - network code takes care of
>>>>>>>reinserting VLAN tag when necessary. If you think that disabling tag
>>>>>>>stripping is beneficial for cases where no VLANs are configured, it's
>>>>>>>better to do that in netdev_fix_features() for devices which advertise
>>>>>>>NETIF_F_HW_VLAN_RX in hw_features.
>>>>>>
>>>>>> Well I just wanted to preserve current behaviour which is that in many
>>>>>> drivers vlan accel is enabled only if some vid is registered upon the
>>>>>> device and it's disabled again when no vid is registered. I can see
>>>>>> no way to do this with current code after removing ndo_vlan_rx_register.
>>>>>>
>>>>>> I expect unexpected
>>>>>
>>>>>:-D
>>>>>
>>>>>> ... problems on old cards when vlan accel would be
>>>>>> enabled all the time, but maybe I'm wrong...
>>>>>
>>>>>Device has no way of knowing how the system uses VLAN tags, stripped
>>>>>or not. Any problems would be driver problems and since you're making
>>>>>it all use generic code, bugs will hit all drivers simultaneously or
>>>>>(preferably) won't happen at all.
>>>>>
>>>>>> One idea is for device which do not support sepatate rx/tx vlan accel
>>>>>> enabling/disabling they can probably use ndo_fix_features force to
>>>>>> enable/disable rx/tx pair together. That would resolve the situation as
>>>>>> well giving user possibility to turn off vlan accel in case of any issues.
>>>>>
>>>>>That is exactly the idea behind ndo_fix_features.
>>>
>>>> In netdev_fix_features add check if either one of NETIF_F_HW_VLAN_TX or
>>>> NETIF_F_HW_VLAN_RX is set and in that case set the other one. Of course
>>>> this would be done only for devices what do not support separate rx/tx
>>>> vlan on/off. But how to distinguish? NETIF_F_HW_VLAN_BOTH feature flag?
>>>
>>>Not in netdev_fix_features(). This case you describe should be handled
>>>in driver-specific
>>
>> Well since the code would be the same in many drivers I was thinking
>> about putting it in general code...
>>
>> Anyway, would you please look at following example patch and tell me if
>> it looks good to you?
>
>[...]
>>  static u32 atl1c_fix_features(struct net_device *netdev, u32 features)
>>  {
>> +       u32 changed = netdev->features ^ features;
>> +
>> +       /*
>> +        * Since there is no support for separate rx/tx vlan accel
>> +        * enable/disable make sure these are always set in pair.
>> +        */
>> +       if ((changed & NETIF_F_HW_VLAN_TX && features & NETIF_F_HW_VLAN_TX) ||
>> +           (changed & NETIF_F_HW_VLAN_RX && features & NETIF_F_HW_VLAN_RX))
>> +               features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
>> +       else
>> +               features &= ~(NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
>> +
>
>You ignored my hint about combined TX/RX offload. Is that on purpose?

Sorry but I'm probably missing what you mean.

>Your code will toggle VLAN acceleration on every
>netdev_update_features call when user requests one offload on and one
>off.

Well for hw which cannot no/off rx/tx accel separately I thought that if
user wants one accel on, the accel should be enabled. According to
following table (bits set when calling driver's fix_features):

 NETIF_F_HW_VLAN_TX  NETIF_F_HW_VLAN_RX -> enable
 NETIF_F_HW_VLAN_TX !NETIF_F_HW_VLAN_RX -> enable
!NETIF_F_HW_VLAN_TX  NETIF_F_HW_VLAN_RX -> enable
!NETIF_F_HW_VLAN_TX !NETIF_F_HW_VLAN_RX -> disable

This looks logical to me...

>
>BTW, the register flag name (MAC_CTRL_RMV_VLAN) suggests that it
>controls only tag stripping. Was it tested or documented that this
>also links with tag insertion?

comment says "/* enable VLAN tag insert/strip */" therefore it looks
like this register controls both.

>
>[...]
>> +static int atl1c_set_features(struct net_device *netdev, u32 features)
>> +{
>> +       u32 changed = netdev->features ^ features;
>> +
>> +       /*
>> +        * Test for NETIF_F_HW_VLAN_TX as it's paired with NETIF_F_HW_VLAN_RX
>> +        * by atl1c_fix_features.
>> +        */
>> +       if (changed & NETIF_F_HW_VLAN_TX)
>> +               atl1c_vlan_mode(netdev, features);
>> +
>
>Test for RX is better, as it will match the name of control bit
>(MAC_CTRL_RMV_VLAN).

Yeah, why not, I think this does not matter.

>
>Best Regards,
>Michał Mirosław

Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

[Michał Mirosław]

W dniu 19 lipca 2011 10:13 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
> Tue, Jul 19, 2011 at 09:24:29AM CEST, mirqus@gmail.com wrote:
>>W dniu 18 lipca 2011 09:13 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>>>  static u32 atl1c_fix_features(struct net_device *netdev, u32 features)
>>>  {
>>> +       u32 changed = netdev->features ^ features;
>>> +
>>> +       /*
>>> +        * Since there is no support for separate rx/tx vlan accel
>>> +        * enable/disable make sure these are always set in pair.
>>> +        */
>>> +       if ((changed & NETIF_F_HW_VLAN_TX && features & NETIF_F_HW_VLAN_TX) ||
>>> +           (changed & NETIF_F_HW_VLAN_RX && features & NETIF_F_HW_VLAN_RX))
>>> +               features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
>>> +       else
>>> +               features &= ~(NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
>>> +
>>
>>You ignored my hint about combined TX/RX offload. Is that on purpose?
> Sorry but I'm probably missing what you mean.

You could replace above code with:

if (!(features & NETIF_F_HW_VLAN_RX))
  features &= ~NETIF_F_HW_VLAN_TX;

>>Your code will toggle VLAN acceleration on every
>>netdev_update_features call when user requests one offload on and one
>>off.
>
> Well for hw which cannot no/off rx/tx accel separately I thought that if
> user wants one accel on, the accel should be enabled. According to
> following table (bits set when calling driver's fix_features):
>
>  NETIF_F_HW_VLAN_TX  NETIF_F_HW_VLAN_RX -> enable
>  NETIF_F_HW_VLAN_TX !NETIF_F_HW_VLAN_RX -> enable
> !NETIF_F_HW_VLAN_TX  NETIF_F_HW_VLAN_RX -> enable
> !NETIF_F_HW_VLAN_TX !NETIF_F_HW_VLAN_RX -> disable
>
> This looks logical to me...

If user requests eg. +TX -RX, you will have ndo_fix_features called
with +TX -RX, possibly multiple times. This generates this sequence:

current features: -TX,-RX
  -> changed: TX -> return +TX,+RX
  -> set_features +TX,+TX
current_features: +TX,+RX
  -> changed: RX -> return -TX,-RX
  -> set_features -TX,-RX
...

(And you cannot control when fix_features will be called.)

If you don't agree with the hint above (about forcing TX off when RX
is off, leaving the rest alone), then its better to remove TX from
hw_features and copy it's state from RX in ndo_fix_features. Like
this:

[init]
  netdev->hw_features |= NETIF_F_HW_VLAN_RX;
[fix_features]
  if (features & NETIF_F_HW_VLAN_RX)
    features |= NETIF_F_HW_VLAN_TX;
  else
    features &= ~NETIF_F_HW_VLAN_TX;

Best Regards,
Michał Mirosław

Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

[Michał Mirosław]

W dniu 19 lipca 2011 10:24 użytkownik Michał Mirosław
<mirqus@gmail.com> napisał:
> W dniu 19 lipca 2011 10:13 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>> Tue, Jul 19, 2011 at 09:24:29AM CEST, mirqus@gmail.com wrote:
>>>W dniu 18 lipca 2011 09:13 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>>>>  static u32 atl1c_fix_features(struct net_device *netdev, u32 features)
>>>>  {
>>>> +       u32 changed = netdev->features ^ features;
>>>> +
>>>> +       /*
>>>> +        * Since there is no support for separate rx/tx vlan accel
>>>> +        * enable/disable make sure these are always set in pair.
>>>> +        */
>>>> +       if ((changed & NETIF_F_HW_VLAN_TX && features & NETIF_F_HW_VLAN_TX) ||
>>>> +           (changed & NETIF_F_HW_VLAN_RX && features & NETIF_F_HW_VLAN_RX))
>>>> +               features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
>>>> +       else
>>>> +               features &= ~(NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
>>>> +
>>>
>>>You ignored my hint about combined TX/RX offload. Is that on purpose?
>> Sorry but I'm probably missing what you mean.
>
> You could replace above code with:
>
> if (!(features & NETIF_F_HW_VLAN_RX))
>  features &= ~NETIF_F_HW_VLAN_TX;

Or you could invert the test and effect:

  if (features & NETIF_F_HW_VLAN_TX)
    features |= NETIF_F_HW_VLAN_RX;

Best Regards,
Michał Mirosław

Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

[Jiri Pirko]

Tue, Jul 19, 2011 at 10:24:58AM CEST, mirqus@gmail.com wrote:
>W dniu 19 lipca 2011 10:13 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>> Tue, Jul 19, 2011 at 09:24:29AM CEST, mirqus@gmail.com wrote:
>>>W dniu 18 lipca 2011 09:13 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>>>>  static u32 atl1c_fix_features(struct net_device *netdev, u32 features)
>>>>  {
>>>> +       u32 changed = netdev->features ^ features;
>>>> +
>>>> +       /*
>>>> +        * Since there is no support for separate rx/tx vlan accel
>>>> +        * enable/disable make sure these are always set in pair.
>>>> +        */
>>>> +       if ((changed & NETIF_F_HW_VLAN_TX && features & NETIF_F_HW_VLAN_TX) ||
>>>> +           (changed & NETIF_F_HW_VLAN_RX && features & NETIF_F_HW_VLAN_RX))
>>>> +               features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
>>>> +       else
>>>> +               features &= ~(NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
>>>> +
>>>
>>>You ignored my hint about combined TX/RX offload. Is that on purpose?
>> Sorry but I'm probably missing what you mean.
>
>You could replace above code with:
>
>if (!(features & NETIF_F_HW_VLAN_RX))
>  features &= ~NETIF_F_HW_VLAN_TX;
>
>>>Your code will toggle VLAN acceleration on every
>>>netdev_update_features call when user requests one offload on and one
>>>off.
>>
>> Well for hw which cannot no/off rx/tx accel separately I thought that if
>> user wants one accel on, the accel should be enabled. According to
>> following table (bits set when calling driver's fix_features):
>>
>>  NETIF_F_HW_VLAN_TX  NETIF_F_HW_VLAN_RX -> enable
>>  NETIF_F_HW_VLAN_TX !NETIF_F_HW_VLAN_RX -> enable
>> !NETIF_F_HW_VLAN_TX  NETIF_F_HW_VLAN_RX -> enable
>> !NETIF_F_HW_VLAN_TX !NETIF_F_HW_VLAN_RX -> disable
>>
>> This looks logical to me...
>
>If user requests eg. +TX -RX, you will have ndo_fix_features called
>with +TX -RX, possibly multiple times. This generates this sequence:
>
>current features: -TX,-RX
>  -> changed: TX -> return +TX,+RX
>  -> set_features +TX,+TX
>current_features: +TX,+RX
>  -> changed: RX -> return -TX,-RX
>  -> set_features -TX,-RX
>...
>
>(And you cannot control when fix_features will be called.)
>
>If you don't agree with the hint above (about forcing TX off when RX
>is off, leaving the rest alone), then its better to remove TX from
>hw_features and copy it's state from RX in ndo_fix_features. Like
>this:
>
>[init]
>  netdev->hw_features |= NETIF_F_HW_VLAN_RX;
>[fix_features]
>  if (features & NETIF_F_HW_VLAN_RX)
>    features |= NETIF_F_HW_VLAN_TX;
>  else
>    features &= ~NETIF_F_HW_VLAN_TX;

This sounds good. Probably the best solution. I'll do it this way.

>
>Best Regards,
>Michał Mirosław