| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 18 Jul 2011 09:13:02 +0200
From: Jiri Pirko <jpirko@...hat.com>
To: Michał Mirosław <mirqus@...il.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net,
shemminger@...ux-foundation.org, eric.dumazet@...il.com,
greearb@...delatech.com
Subject: Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]
Sun, Jul 17, 2011 at 11:06:57PM CEST, mirqus@...il.com wrote:
>W dniu 17 lipca 2011 21:44 użytkownik Jiri Pirko <jpirko@...hat.com> napisał:
>> Sun, Jul 17, 2011 at 10:36:04AM CEST, mirqus@...il.com wrote:
>>>W dniu 17 lipca 2011 09:30 użytkownik Jiri Pirko <jpirko@...hat.com> napisał:
>>>> Sat, Jul 16, 2011 at 04:14:36PM CEST, mirqus@...il.com wrote:
>>>>>2011/7/16 Jiri Pirko <jpirko@...hat.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists