Re: [RFC 2/9] net: dsa: add basic support for VLAN operations
From: Vivien Didelot
Date: Tue Jun 02 2015 - 20:46:14 EST
Hi Guenter,
On Jun 2, 2015, at 10:42 AM, Guenter Roeck linux@xxxxxxxxxxxx wrote:
On 06/01/2015 06:27 PM, Vivien Didelot wrote:
>> This patch adds the glue between DSA and switchdev to add and delete
>> SWITCHDEV_OBJ_PORT_VLAN objects.
>>
>> This will allow the DSA switch drivers implementing the port_vlan_add
>> and port_vlan_del functions to access the switch VLAN database through
>> userspace commands such as "bridge vlan".
>>
>> Signed-off-by: Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx>
>> ---
>> include/net/dsa.h | 7 +++++++
>> net/dsa/slave.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 2 files changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index fbca63b..726357b 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -302,6 +302,13 @@ struct dsa_switch_driver {
>> const unsigned char *addr, u16 vid);
>> int (*fdb_getnext)(struct dsa_switch *ds, int port,
>> unsigned char *addr, bool *is_static);
>> +
>> + /*
>> + * VLAN support
>> + */
>> + int (*port_vlan_add)(struct dsa_switch *ds, int port, u16 vid,
>> + u16 bridge_flags);
>> + int (*port_vlan_del)(struct dsa_switch *ds, int port, u16 vid);
>> };
>>
>> void register_switch_driver(struct dsa_switch_driver *type);
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index cbda00a..52ba5a1 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -363,6 +363,25 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
>> return ret;
>> }
>>
>> +static int dsa_slave_port_vlans_add(struct net_device *dev,
>> + struct switchdev_obj_vlan *vlan)
>> +{
>> + struct dsa_slave_priv *p = netdev_priv(dev);
>> + struct dsa_switch *ds = p->parent;
>> + int vid, err = 0;
>> +
>> + if (!ds->drv->port_vlan_add)
>> + return -ENOTSUPP;
>> +
>> + for (vid = vlan->vid_start; vid <= vlan->vid_end; ++vid) {
>> + err = ds->drv->port_vlan_add(ds, p->port, vid, vlan->flags);
>> + if (err)
>> + break;
>> + }
>> +
>> + return err;
>> +}
>> +
>> static int dsa_slave_port_obj_add(struct net_device *dev,
>> struct switchdev_obj *obj)
>> {
>> @@ -378,6 +397,9 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>> return 0;
>>
>> switch (obj->id) {
>> + case SWITCHDEV_OBJ_PORT_VLAN:
>> + err = dsa_slave_port_vlans_add(dev, &obj->u.vlan);
>> + break;
>> default:
>> err = -ENOTSUPP;
>> break;
>> @@ -386,12 +408,34 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>> return err;
>> }
>>
>> +static int dsa_slave_port_vlans_del(struct net_device *dev,
>> + struct switchdev_obj_vlan *vlan)
>> +{
>> + struct dsa_slave_priv *p = netdev_priv(dev);
>> + struct dsa_switch *ds = p->parent;
>> + int vid, err = 0;
>> +
>> + if (!ds->drv->port_vlan_del)
>> + return -ENOTSUPP;
>> +
>> + for (vid = vlan->vid_start; vid <= vlan->vid_end; ++vid) {
>> + err = ds->drv->port_vlan_del(ds, p->port, vid);
>> + if (err)
>> + break;
>> + }
>> +
>> + return err;
>> +}
>> +
>> static int dsa_slave_port_obj_del(struct net_device *dev,
>> struct switchdev_obj *obj)
>> {
>> int err;
>>
>> switch (obj->id) {
>> + case SWITCHDEV_OBJ_PORT_VLAN:
>> + err = dsa_slave_port_vlans_del(dev, &obj->u.vlan);
>> + break;
>> default:
>> err = -EOPNOTSUPP;
>> break;
>> @@ -473,6 +517,15 @@ static netdev_tx_t dsa_slave_notag_xmit(struct sk_buff
>> *skb,
>> return NETDEV_TX_OK;
>> }
>>
>> +static int dsa_slave_vlan_noop(struct net_device *dev, __be16 proto, u16 vid)
>> +{
>> + /* NETIF_F_HW_VLAN_CTAG_FILTER requires ndo_vlan_rx_add_vid and
>> + * ndo_vlan_rx_kill_vid, otherwise the VLAN acceleration is considered
>> + * buggy (see net/core/dev.c).
>> + */
>> + return 0;
>> +}
>> +
>>
>> /* ethtool operations *******************************************************/
>> static int
>> @@ -734,6 +787,10 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
>> .ndo_fdb_dump = dsa_slave_fdb_dump,
>> .ndo_do_ioctl = dsa_slave_ioctl,
>> .ndo_get_iflink = dsa_slave_get_iflink,
>> + .ndo_vlan_rx_add_vid = dsa_slave_vlan_noop,
>> + .ndo_vlan_rx_kill_vid = dsa_slave_vlan_noop,
>> + .ndo_bridge_setlink = switchdev_port_bridge_setlink,
>> + .ndo_bridge_dellink = switchdev_port_bridge_dellink,
>> };
>>
>> static const struct switchdev_ops dsa_slave_switchdev_ops = {
>> @@ -924,7 +981,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device
>> *parent,
>> if (slave_dev == NULL)
>> return -ENOMEM;
>>
>> - slave_dev->features = master->vlan_features;
>> + slave_dev->features = master->vlan_features | NETIF_F_VLAN_FEATURES;
>
> Hi Vivien,
>
> NETIF_F_VLAN_FEATURES declares that the device supports receive and transmit
> tagging offload. We do this on transmit, by calling vlan_hwaccel_push_inside()
> with patch 9, but not on the receive side.
>
> I think you may need to add matching code on the receive side to remove
> the VLAN tag and move it into the skb with __vlan_hwaccel_put_tag().
> It may also be necessary to add the same code for the other tag handlers.
>
> Overall I wonder if it really makes sense to add those flags. Seems to me
> that would only make sense if the resulting code gets more efficient,
> which at least currently is not the case. Am I missing something ?
I initially added those flags because without them, bridge didn't call
my ndo_vlan_rx_add_vid. But with the switchdev abstraction, they seem
unrelevant.
I just undo this change (keeping slave_dev->{vlan_,}features to
master->vlan_features) and I am able to create VLANs through bridge.
TBH, I'm not familiar at all with these flags. Seems like I must revert
this feature changes.
Thanks,
-v
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/