From: Vivien Didelot <hidden> Date: 2015-08-06 05:44:17
This patchset refactors the DSA and mv88e6xxx code to use the switchdev FDB
objects.
The first two patches add minor but necessary changes to switchdev, the third
one implements the switchdev glue in DSA for FDB routines, and the remaining
ones refactor the FDB access functions in the mv88e6xxx code.
Below is an usage example (ports 0-2 belongs to br0, ports 3-4 belongs to br1):
# bridge fdb add 3c:97:0e:11:30:6e dev swp2
# bridge fdb add 3c:97:0e:11:40:78 dev swp3
# bridge fdb add 3c:97:0e:11:50:86 dev swp4
# bridge fdb del 3c:97:0e:11:40:78 dev swp3
# bridge fdb
01:00:5e:00:00:01 dev eth0 self permanent
01:00:5e:00:00:01 dev eth1 self permanent
00:50:d2:10:78:15 dev swp0 master br0 permanent
3c:97:0e:11:30:6e dev swp2 self static
00:50:d2:10:78:15 dev swp3 master br1 permanent
3c:97:0e:11:50:86 dev swp4 self static
# cat /sys/kernel/debug/dsa0/atu
# DB T/P Vec State Addr
# 001 Port 004 e 3c:97:0e:11:30:6e
# 004 Port 010 e 3c:97:0e:11:50:86
For the 88E6xxx switches, FIDs 1 to num_ports will be reserved for non-bridged
ports and bridge groups, and the remaining will be later used by VLANs.
This change is necessary to welcome the support for hardware VLANs (which will
follow soon).
Changes in v2:
- remove ndo_bridge_{get,set,del}link from switchdev/DSA glue code
- use ether_addr_copy instead of memcpy for MAC addresses
- constify MAC address in port_fdb_{add,del}
- split the mv88e6xxx code refactoring into several patches
Vivien Didelot (7):
net: switchdev: change fdb addr for a byte array
net: switchdev: support static FDB addresses
net: dsa: add support for switchdev FDB objects
net: dsa: mv88e6xxx: extend fid mask
net: dsa: mv88e6xxx: rename ATU MAC accessors
net: dsa: mv88e6xxx: rework FDB getnext operation
net: dsa: mv88e6xxx: rework FDB add/del operations
drivers/net/dsa/mv88e6171.c | 6 +-
drivers/net/dsa/mv88e6352.c | 6 +-
drivers/net/dsa/mv88e6xxx.c | 223 ++++++++++++++++++++++++-----------
drivers/net/dsa/mv88e6xxx.h | 31 +++--
drivers/net/ethernet/rocker/rocker.c | 2 +-
include/net/dsa.h | 16 ++-
include/net/switchdev.h | 3 +-
net/bridge/br_fdb.c | 2 +-
net/dsa/slave.c | 218 ++++++++++++++++++----------------
net/switchdev/switchdev.c | 7 +-
10 files changed, 317 insertions(+), 197 deletions(-)
--
2.4.6
From: Vivien Didelot <hidden> Date: 2015-08-06 05:44:22
This patch adds a is_static boolean to the switchdev_obj_fdb structure,
in order to set the ndm_state to either NUD_NOARP or NUD_REACHABLE.
Signed-off-by: Vivien Didelot <redacted>
---
include/net/switchdev.h | 1 +
net/switchdev/switchdev.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
From: Vivien Didelot <hidden> Date: 2015-08-06 05:44:25
Rename the __mv88e6xxx_{read,write}_addr functions to more explicit
_mv88e6xxx_atu_mac_{read,write} functions, which also respect the single
underscore convention used in the file (meaning SMI lock must be held).
In the meantime, define their MAC address parameters as an array of
ETH_ALEN bytes instead of a char pointer.
Signed-off-by: Vivien Didelot <redacted>
---
drivers/net/dsa/mv88e6xxx.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
From: Vivien Didelot <hidden> Date: 2015-08-06 05:44:28
This commit adds a low level _mv88e6xxx_atu_getnext function and helpers
to rewrite the mv88e6xxx_port_fdb_getnext operation.
A mv88e6xxx_atu_entry structure is added for convenient access to the
hardware, and GLOBAL_ATU_FID is defined instead of the raw 0x01 value.
The previous implementation did not handle the eventual trunk mapping.
If the related bit is set, then the ATU data register would contain the
trunk ID, and not the port vector.
Check this in the FDB getnext operation and do not handle it (yet).
Signed-off-by: Vivien Didelot <redacted>
---
drivers/net/dsa/mv88e6171.c | 1 +
drivers/net/dsa/mv88e6352.c | 1 +
drivers/net/dsa/mv88e6xxx.c | 97 +++++++++++++++++++++++++++++++++------------
drivers/net/dsa/mv88e6xxx.h | 15 ++++++-
4 files changed, 87 insertions(+), 27 deletions(-)
@@ -964,7 +964,7 @@ static int _mv88e6xxx_atu_cmd(struct dsa_switch *ds, int fid, u16 cmd){intret;-ret=_mv88e6xxx_reg_write(ds,REG_GLOBAL,0x01,fid);+ret=_mv88e6xxx_reg_write(ds,REG_GLOBAL,GLOBAL_ATU_FID,fid);if(ret<0)returnret;
@@ -1269,12 +1269,14 @@ int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,returnret;}-staticint__mv88e6xxx_port_getnext(structdsa_switch*ds,intport,-unsignedchar*addr,bool*is_static)+staticint_mv88e6xxx_atu_getnext(structdsa_switch*ds,u16fid,+constu8addr[ETH_ALEN],+structmv88e6xxx_atu_entry*entry){-structmv88e6xxx_priv_state*ps=ds_to_priv(ds);-u8fid=ps->fid[port];-intret,state;+structmv88e6xxx_atu_entrynext={0};+intret;++next.fid=fid;ret=_mv88e6xxx_atu_wait(ds);if(ret<0)
@@ -1284,39 +1286,84 @@ static int __mv88e6xxx_port_getnext(struct dsa_switch *ds, int port,if(ret<0)returnret;-do{-ret=_mv88e6xxx_atu_cmd(ds,fid,GLOBAL_ATU_OP_GET_NEXT_DB);-if(ret<0)-returnret;+ret=_mv88e6xxx_atu_cmd(ds,fid,GLOBAL_ATU_OP_GET_NEXT_DB);+if(ret<0)+returnret;-ret=_mv88e6xxx_reg_read(ds,REG_GLOBAL,GLOBAL_ATU_DATA);-if(ret<0)-returnret;-state=ret&GLOBAL_ATU_DATA_STATE_MASK;-if(state==GLOBAL_ATU_DATA_STATE_UNUSED)-return-ENOENT;-}while(!(((ret>>4)&0xff)&(1<<port)));+ret=_mv88e6xxx_atu_mac_read(ds,next.mac);+if(ret<0)+returnret;-ret=_mv88e6xxx_atu_mac_read(ds,addr);+ret=_mv88e6xxx_reg_read(ds,REG_GLOBAL,GLOBAL_ATU_DATA);if(ret<0)returnret;-*is_static=state==(is_multicast_ether_addr(addr)?-GLOBAL_ATU_DATA_STATE_MC_STATIC:-GLOBAL_ATU_DATA_STATE_UC_STATIC);+next.state=ret&GLOBAL_ATU_DATA_STATE_MASK;+if(next.state!=GLOBAL_ATU_DATA_STATE_UNUSED){+unsignedintmask,shift;++if(ret&GLOBAL_ATU_DATA_TRUNK){+next.trunk=true;+mask=GLOBAL_ATU_DATA_TRUNK_ID_MASK;+shift=GLOBAL_ATU_DATA_TRUNK_ID_SHIFT;+}else{+next.trunk=false;+mask=GLOBAL_ATU_DATA_PORT_VECTOR_MASK;+shift=GLOBAL_ATU_DATA_PORT_VECTOR_SHIFT;+}++next.portv_trunkid=(ret&mask)>>shift;+}+*entry=next;return0;}-/* get next entry for port */-intmv88e6xxx_port_fdb_getnext(structdsa_switch*ds,intport,-unsignedchar*addr,bool*is_static)+staticint_mv88e6xxx_port_vid_to_fid(structdsa_switch*ds,intport,u16vid){structmv88e6xxx_priv_state*ps=ds_to_priv(ds);++if(vid==0)+returnps->fid[port];++return-ENOENT;+}++intmv88e6xxx_port_fdb_getnext(structdsa_switch*ds,intport,u16*vid,+u8addr[ETH_ALEN],bool*is_static)+{+structmv88e6xxx_priv_state*ps=ds_to_priv(ds);+structmv88e6xxx_atu_entrynext;+u16fid;intret;mutex_lock(&ps->smi_mutex);-ret=__mv88e6xxx_port_getnext(ds,port,addr,is_static);++ret=_mv88e6xxx_port_vid_to_fid(ds,port,*vid);+if(ret<0)+gotounlock;+fid=ret;++do{+if(is_broadcast_ether_addr(addr)){+ret=-ENOENT;+gotounlock;+}++ret=_mv88e6xxx_atu_getnext(ds,fid,addr,&next);+if(ret<0)+gotounlock;++ether_addr_copy(addr,next.mac);++if(next.state==GLOBAL_ATU_DATA_STATE_UNUSED)+continue;+}while(next.trunk||(next.portv_trunkid&BIT(port))==0);++*is_static=next.state==(is_multicast_ether_addr(addr)?+GLOBAL_ATU_DATA_STATE_MC_STATIC:+GLOBAL_ATU_DATA_STATE_UC_STATIC);+unlock:mutex_unlock(&ps->smi_mutex);returnret;
@@ -312,6 +315,14 @@#define GLOBAL2_QOS_WEIGHT 0x1c#define GLOBAL2_MISC 0x1d+structmv88e6xxx_atu_entry{+u16fid;+u8state;+booltrunk;+u16portv_trunkid;+u8mac[ETH_ALEN];+};+structmv88e6xxx_priv_state{/* When using multi-chip addressing, this mutex protects*accesstotheindirectaccessregisters.(Insingle-chip
@@ -416,11 +427,11 @@ int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,constunsignedchar*addr,u16vid);intmv88e6xxx_port_fdb_del(structdsa_switch*ds,intport,constunsignedchar*addr,u16vid);-intmv88e6xxx_port_fdb_getnext(structdsa_switch*ds,intport,-unsignedchar*addr,bool*is_static);intmv88e6xxx_phy_page_read(structdsa_switch*ds,intport,intpage,intreg);intmv88e6xxx_phy_page_write(structdsa_switch*ds,intport,intpage,intreg,intval);+intmv88e6xxx_port_fdb_getnext(structdsa_switch*ds,intport,u16*vid,+u8addr[ETH_ALEN],bool*is_static);externstructdsa_switch_drivermv88e6131_switch_driver;externstructdsa_switch_drivermv88e6123_61_65_switch_driver;
@@ -423,13 +423,13 @@ int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,intmv88e6xxx_join_bridge(structdsa_switch*ds,intport,u32br_port_mask);intmv88e6xxx_leave_bridge(structdsa_switch*ds,intport,u32br_port_mask);intmv88e6xxx_port_stp_update(structdsa_switch*ds,intport,u8state);-intmv88e6xxx_port_fdb_add(structdsa_switch*ds,intport,-constunsignedchar*addr,u16vid);-intmv88e6xxx_port_fdb_del(structdsa_switch*ds,intport,-constunsignedchar*addr,u16vid);intmv88e6xxx_phy_page_read(structdsa_switch*ds,intport,intpage,intreg);intmv88e6xxx_phy_page_write(structdsa_switch*ds,intport,intpage,intreg,intval);+intmv88e6xxx_port_fdb_add(structdsa_switch*ds,intport,u16vid,+constu8addr[ETH_ALEN]);+intmv88e6xxx_port_fdb_del(structdsa_switch*ds,intport,u16vid,+constu8addr[ETH_ALEN]);intmv88e6xxx_port_fdb_getnext(structdsa_switch*ds,intport,u16*vid,u8addr[ETH_ALEN],bool*is_static);
From: Vivien Didelot <hidden> Date: 2015-08-06 05:45:46
Remove the fdb_{add,del,getnext} function pointer in favor of new
port_fdb_{add,del,getnext}.
Implement the switchdev_port_obj_{add,del,dump} functions in DSA to
support the SWITCHDEV_OBJ_PORT_FDB objects.
Signed-off-by: Vivien Didelot <redacted>
---
drivers/net/dsa/mv88e6171.c | 3 -
drivers/net/dsa/mv88e6352.c | 3 -
include/net/dsa.h | 16 ++--
net/dsa/slave.c | 218 +++++++++++++++++++++++---------------------
4 files changed, 126 insertions(+), 114 deletions(-)
@@ -200,105 +201,6 @@ out:return0;}-staticintdsa_slave_fdb_add(structndmsg*ndm,structnlattr*tb[],-structnet_device*dev,-constunsignedchar*addr,u16vid,u16nlm_flags)-{-structdsa_slave_priv*p=netdev_priv(dev);-structdsa_switch*ds=p->parent;-intret=-EOPNOTSUPP;--if(ds->drv->fdb_add)-ret=ds->drv->fdb_add(ds,p->port,addr,vid);--returnret;-}--staticintdsa_slave_fdb_del(structndmsg*ndm,structnlattr*tb[],-structnet_device*dev,-constunsignedchar*addr,u16vid)-{-structdsa_slave_priv*p=netdev_priv(dev);-structdsa_switch*ds=p->parent;-intret=-EOPNOTSUPP;--if(ds->drv->fdb_del)-ret=ds->drv->fdb_del(ds,p->port,addr,vid);--returnret;-}--staticintdsa_slave_fill_info(structnet_device*dev,structsk_buff*skb,-constunsignedchar*addr,u16vid,-boolis_static,-u32portid,u32seq,inttype,-unsignedintflags)-{-structnlmsghdr*nlh;-structndmsg*ndm;--nlh=nlmsg_put(skb,portid,seq,type,sizeof(*ndm),flags);-if(!nlh)-return-EMSGSIZE;--ndm=nlmsg_data(nlh);-ndm->ndm_family=AF_BRIDGE;-ndm->ndm_pad1=0;-ndm->ndm_pad2=0;-ndm->ndm_flags=NTF_EXT_LEARNED;-ndm->ndm_type=0;-ndm->ndm_ifindex=dev->ifindex;-ndm->ndm_state=is_static?NUD_NOARP:NUD_REACHABLE;--if(nla_put(skb,NDA_LLADDR,ETH_ALEN,addr))-gotonla_put_failure;--if(vid&&nla_put_u16(skb,NDA_VLAN,vid))-gotonla_put_failure;--nlmsg_end(skb,nlh);-return0;--nla_put_failure:-nlmsg_cancel(skb,nlh);-return-EMSGSIZE;-}--/* Dump information about entries, in response to GETNEIGH */-staticintdsa_slave_fdb_dump(structsk_buff*skb,structnetlink_callback*cb,-structnet_device*dev,-structnet_device*filter_dev,intidx)-{-structdsa_slave_priv*p=netdev_priv(dev);-structdsa_switch*ds=p->parent;-unsignedcharaddr[ETH_ALEN]={0};-intret;--if(!ds->drv->fdb_getnext)-return-EOPNOTSUPP;--for(;;idx++){-boolis_static;--ret=ds->drv->fdb_getnext(ds,p->port,addr,&is_static);-if(ret<0)-break;--if(idx<cb->args[0])-continue;--ret=dsa_slave_fill_info(dev,skb,addr,0,-is_static,-NETLINK_CB(cb->skb).portid,-cb->nlh->nlmsg_seq,-RTM_NEWNEIGH,NLM_F_MULTI);-if(ret<0)-break;-}--returnidx;-}-staticintdsa_slave_ioctl(structnet_device*dev,structifreq*ifr,intcmd){structdsa_slave_priv*p=netdev_priv(dev);
@@ -364,6 +266,115 @@ static int dsa_slave_port_attr_set(struct net_device *dev,returnret;}+staticintdsa_slave_port_fdb_add(structnet_device*dev,+structswitchdev_obj*obj)+{+structswitchdev_obj_fdb*fdb=&obj->u.fdb;+structdsa_slave_priv*p=netdev_priv(dev);+structdsa_switch*ds=p->parent;+interr;++if(obj->trans==SWITCHDEV_TRANS_PREPARE)+err=ds->drv->port_fdb_add?0:-EOPNOTSUPP;+elseif(obj->trans==SWITCHDEV_TRANS_COMMIT)+err=ds->drv->port_fdb_add(ds,p->port,fdb->vid,fdb->addr);+else+err=-EOPNOTSUPP;++returnerr;+}++staticintdsa_slave_port_fdb_del(structnet_device*dev,+structswitchdev_obj*obj)+{+structswitchdev_obj_fdb*fdb=&obj->u.fdb;+structdsa_slave_priv*p=netdev_priv(dev);+structdsa_switch*ds=p->parent;++if(!ds->drv->port_fdb_del)+return-EOPNOTSUPP;++returnds->drv->port_fdb_del(ds,p->port,fdb->vid,fdb->addr);+}++staticintdsa_slave_port_fdb_dump(structnet_device*dev,+structswitchdev_obj*obj)+{+structswitchdev_obj_fdb*fdb=&obj->u.fdb;+structdsa_slave_priv*p=netdev_priv(dev);+structdsa_switch*ds=p->parent;+interr;++if(!ds->drv->port_fdb_getnext)+return-EOPNOTSUPP;++memset(fdb,0,sizeof(*fdb));++for(;;){+err=ds->drv->port_fdb_getnext(ds,p->port,&fdb->vid,+fdb->addr,&fdb->is_static);+if(err)+break;++err=obj->cb(dev,obj);+if(err)+break;+}++returnerr==-ENOENT?0:err;+}++staticintdsa_slave_port_obj_add(structnet_device*dev,+structswitchdev_obj*obj)+{+interr;++switch(obj->id){+caseSWITCHDEV_OBJ_PORT_FDB:+err=dsa_slave_port_fdb_add(dev,obj);+break;+default:+err=-EOPNOTSUPP;+break;+}++returnerr;+}++staticintdsa_slave_port_obj_del(structnet_device*dev,+structswitchdev_obj*obj)+{+interr;++switch(obj->id){+caseSWITCHDEV_OBJ_PORT_FDB:+err=dsa_slave_port_fdb_del(dev,obj);+break;+default:+err=-EOPNOTSUPP;+break;+}++returnerr;+}++staticintdsa_slave_port_obj_dump(structnet_device*dev,+structswitchdev_obj*obj)+{+interr;++switch(obj->id){+caseSWITCHDEV_OBJ_PORT_FDB:+err=dsa_slave_port_fdb_dump(dev,obj);+break;+default:+err=-EOPNOTSUPP;+break;+}++returnerr;+}+staticintdsa_slave_bridge_port_join(structnet_device*dev,structnet_device*br){
From: Vivien Didelot <hidden> Date: 2015-08-06 05:46:36
The driver currently manages one FID per port (or bridge group), with a
mask of DSA_MAX_PORTS bits, where 0 means that the FID is in use.
The Marvell 88E6xxx switches support up to 4094 FIDs (from 1 to 0xfff;
FID 0 means that multiple address databases are not being used).
This patch changes the fid_mask for an fid_bitmap of 4096 bits.
From now on, FIDs 1 to num_ports are reserved for non-bridged ports and
bridge groups (a bridge group gets the FID of its first member). The
remaining bits will be reserved for VLAN entries.
Signed-off-by: Vivien Didelot <redacted>
---
drivers/net/dsa/mv88e6xxx.c | 20 +++++++++++++-------
drivers/net/dsa/mv88e6xxx.h | 8 +++++---
2 files changed, 18 insertions(+), 10 deletions(-)
@@ -1091,7 +1091,7 @@ int mv88e6xxx_join_bridge(struct dsa_switch *ds, int port, u32 br_port_mask)ps->bridge_mask[fid]=br_port_mask;if(fid!=ps->fid[port]){-ps->fid_mask|=1<<ps->fid[port];+clear_bit(ps->fid[port],ps->fid_bitmap);ps->fid[port]=fid;ret=_mv88e6xxx_update_bridge_config(ds,fid);}
@@ -1125,9 +1125,16 @@ int mv88e6xxx_leave_bridge(struct dsa_switch *ds, int port, u32 br_port_mask)mutex_lock(&ps->smi_mutex);-newfid=__ffs(ps->fid_mask);+newfid=find_next_zero_bit(ps->fid_bitmap,VLAN_N_VID,1);+if(unlikely(newfid>ps->num_ports)){+netdev_err(ds->ports[port],"all first %d FIDs are used\n",+ps->num_ports);+ret=-ENOSPC;+gotounlock;+}+ps->fid[port]=newfid;-ps->fid_mask&=~(1<<newfid);+set_bit(newfid,ps->fid_bitmap);ps->bridge_mask[fid]&=~(1<<port);ps->bridge_mask[newfid]=1<<port;
@@ -1135,6 +1142,7 @@ int mv88e6xxx_leave_bridge(struct dsa_switch *ds, int port, u32 br_port_mask)if(!ret)ret=_mv88e6xxx_update_bridge_config(ds,newfid);+unlock:mutex_unlock(&ps->smi_mutex);returnret;
@@ -1554,9 +1562,9 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)*ports,andalloweachofthe'real'portstoonlytalkto*theupstreamport.*/-fid=__ffs(ps->fid_mask);+fid=port+1;ps->fid[port]=fid;-ps->fid_mask&=~(1<<fid);+set_bit(fid,ps->fid_bitmap);if(!dsa_is_cpu_port(ds,port))ps->bridge_mask[fid]=1<<port;
@@ -1855,8 +1863,6 @@ int mv88e6xxx_setup_common(struct dsa_switch *ds)ps->id=REG_READ(REG_PORT(0),PORT_SWITCH_ID)&0xfff0;-ps->fid_mask=(1<<DSA_MAX_PORTS)-1;-INIT_WORK(&ps->bridge_work,mv88e6xxx_bridge_work);name=kasprintf(GFP_KERNEL,"dsa%d",ds->index);
@@ -348,9 +350,9 @@ struct mv88e6xxx_priv_state {/* hw bridging */-u32fid_mask;-u8fid[DSA_MAX_PORTS];-u16bridge_mask[DSA_MAX_PORTS];+DECLARE_BITMAP(fid_bitmap,VLAN_N_VID);/* FIDs 1 to 4095 available */+u16fid[DSA_MAX_PORTS];/* per (non-bridged) port FID */+u16bridge_mask[DSA_MAX_PORTS];/* br groups (indexed by FID) */unsignedlongport_state_update_mask;u8port_state[DSA_MAX_PORTS];
From: Vivien Didelot <hidden> Date: 2015-08-06 05:47:12
The address in the switchdev_obj_fdb structure is currently represented
as a pointer. Replacing it for a 6-byte array allows switchdev to carry
addresses directly read from hardware registers, not stored by the
switch chip driver (as in Rocker).
Signed-off-by: Vivien Didelot <redacted>
---
drivers/net/ethernet/rocker/rocker.c | 2 +-
include/net/switchdev.h | 2 +-
net/bridge/br_fdb.c | 2 +-
net/switchdev/switchdev.c | 5 +++--
4 files changed, 6 insertions(+), 5 deletions(-)
From: Scott Feldman <hidden> Date: 2015-08-06 06:28:37
On Wed, Aug 5, 2015 at 10:44 PM, Vivien Didelot
[off-list ref] wrote:
quoted hunk
This patch adds a is_static boolean to the switchdev_obj_fdb structure,
in order to set the ndm_state to either NUD_NOARP or NUD_REACHABLE.
Signed-off-by: Vivien Didelot <redacted>
---
include/net/switchdev.h | 1 +
net/switchdev/switchdev.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
What do you think about changing this to u16 ndm_state? That way, it
can be used on input (fdb add) and output (fdb dump), and the driver
can privately track the state, kind of like how the bridge keeps
is_static, is_local, etc.
From: Andrew Lunn <andrew@lunn.ch> Date: 2015-08-06 14:11:36
Hi Vivien
Thanks for splitting up the big patch. This it is much easier to
review now.
Is this patch git bisectable?
Clearly after this patch, but before all the other patches are in, we
will not be programming the hardware. The call into the driver is
removed here, but the replacement is added later. But is the EOPNOTSUP
enough that the system keeps working, by falling back to software?
The two driver APIs are very similar, the main difference being the
MAC address. Can you do the refactoring first, and then make the API
change. That means we can test each patch individually, and have
proper git bisectability.
Thanks
Andrew
From: Vivien Didelot <hidden> Date: 2015-08-06 14:19:40
On 15-08-05 23:28:15, Scott Feldman wrote:
On Wed, Aug 5, 2015 at 10:44 PM, Vivien Didelot
[off-list ref] wrote:
quoted
This patch adds a is_static boolean to the switchdev_obj_fdb structure,
in order to set the ndm_state to either NUD_NOARP or NUD_REACHABLE.
Signed-off-by: Vivien Didelot <redacted>
---
include/net/switchdev.h | 1 +
net/switchdev/switchdev.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
What do you think about changing this to u16 ndm_state? That way, it
can be used on input (fdb add) and output (fdb dump), and the driver
can privately track the state, kind of like how the bridge keeps
is_static, is_local, etc.
I'm OK with the change. Should we consider NUD_NONE (0) a valid value?
@@ -811,7 +811,7 @@ static int switchdev_port_fdb_dump_cb(struct net_device *dev,ndm->ndm_flags=NTF_SELF;ndm->ndm_type=0;ndm->ndm_ifindex=dev->ifindex;-ndm->ndm_state=NUD_REACHABLE;+ndm->ndm_state=obj->u.fdb.is_static?NUD_NOARP:NUD_REACHABLE;
In other word, do we prefer this:
ndm->ndm_state = obj->u.fdb.ndm_state == NUD_NONE ?
NUD_REACHABLE : obj->u.fdb.ndm_state;
Or this (meaning switchdev users cannot leave it blank and must at least
set NUD_REACHABLE themselves):
ndm->ndm_state = obj->u.fdb.ndm_state;
Thanks,
-v
From: Vivien Didelot <hidden> Date: 2015-08-06 14:52:31
On 15-08-06 16:04:32, Andrew Lunn wrote:
Hi Vivien
Thanks for splitting up the big patch. This it is much easier to
review now.
Is this patch git bisectable?
In terms of compilation, yes.
Clearly after this patch, but before all the other patches are in, we
will not be programming the hardware. The call into the driver is
removed here, but the replacement is added later. But is the EOPNOTSUP
enough that the system keeps working, by falling back to software?
You're right, it isn't. At this exact patch, issuing the example:
bridge fdb add 3c:97:0e:11:30:6e dev swp2
returns "RTNETLINK answers: Operation not supported".
The two driver APIs are very similar, the main difference being the
MAC address. Can you do the refactoring first, and then make the API
change. That means we can test each patch individually, and have
proper git bisectability.
That'd be better indeed. I'll integrate the migration from
.fdb_{add,del,getnext} to .port_fdb_{add,del,getnext} in this patch,
then add the next patches improving FID and ATU management on top of it.
Thanks,
-v
From: Scott Feldman <hidden> Date: 2015-08-06 16:37:35
On Thu, Aug 6, 2015 at 7:19 AM, Vivien Didelot
[off-list ref] wrote:
On 15-08-05 23:28:15, Scott Feldman wrote:
quoted
On Wed, Aug 5, 2015 at 10:44 PM, Vivien Didelot
[off-list ref] wrote:
quoted
This patch adds a is_static boolean to the switchdev_obj_fdb structure,
in order to set the ndm_state to either NUD_NOARP or NUD_REACHABLE.
Signed-off-by: Vivien Didelot <redacted>
---
include/net/switchdev.h | 1 +
net/switchdev/switchdev.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
What do you think about changing this to u16 ndm_state? That way, it
can be used on input (fdb add) and output (fdb dump), and the driver
can privately track the state, kind of like how the bridge keeps
is_static, is_local, etc.
I'm OK with the change. Should we consider NUD_NONE (0) a valid value?
@@ -811,7 +811,7 @@ static int switchdev_port_fdb_dump_cb(struct net_device *dev,ndm->ndm_flags=NTF_SELF;ndm->ndm_type=0;ndm->ndm_ifindex=dev->ifindex;-ndm->ndm_state=NUD_REACHABLE;+ndm->ndm_state=obj->u.fdb.is_static?NUD_NOARP:NUD_REACHABLE;
In other word, do we prefer this:
ndm->ndm_state = obj->u.fdb.ndm_state == NUD_NONE ?
NUD_REACHABLE : obj->u.fdb.ndm_state;
Or this (meaning switchdev users cannot leave it blank and must at least
set NUD_REACHABLE themselves):
ndm->ndm_state = obj->u.fdb.ndm_state;
From: Vivien Didelot <hidden> Date: 2015-08-10 13:39:33
Hi David,
On 15-08-09 22:48:22, David Miller wrote:
From: Vivien Didelot <redacted>
Date: Thu, 6 Aug 2015 01:44:01 -0400
quoted
This patchset refactors the DSA and mv88e6xxx code to use the switchdev FDB
objects.
Series applied, thanks.
I noticed you didn't push the serie yet. I've just sent the v3 which
includes the switchdev change (ndm_state) mentioned by Scott and the
reordering of commits to improve bisectability, as suggested by Andrew.
Please consider the v3 "[PATCH net-next v3 0/8] net: dsa: mv88e6xxx:
support switchdev FDB objects" instead.
Thanks,
-v
From: Vivien Didelot <hidden> Date: 2015-08-11 16:25:14
Hi David,
On Aug 10, 2015, at 9:39 AM, Vivien Didelot vivien.didelot@savoirfairelinux.com wrote:
Hi David,
On 15-08-09 22:48:22, David Miller wrote:
quoted
From: Vivien Didelot <redacted>
Date: Thu, 6 Aug 2015 01:44:01 -0400
quoted
This patchset refactors the DSA and mv88e6xxx code to use the switchdev FDB
objects.
Series applied, thanks.
I noticed you didn't push the serie yet. I've just sent the v3 which
includes the switchdev change (ndm_state) mentioned by Scott and the
reordering of commits to improve bisectability, as suggested by Andrew.
Please consider the v3 "[PATCH net-next v3 0/8] net: dsa: mv88e6xxx:
support switchdev FDB objects" instead.
Somehow this message was ignored or seen too late, and v2 got pushed in
the net-next tree.
v2 introduces an uneeded patch to convert switchdev fdb address (1/7);
an is_static member in switchdev that Scott didn't fully agree on (2/7);
and the calls into the driver are removed in 3/7 and added later in the
patchset, making it hard to bisect, as mentioned by Andrew.
v3 fixes all of that, Scott acked the switchdev change and Andrew
reviewed the whole patchset. It is indeed more readable and simpler:
v2: 10 files changed, 317 insertions(+), 197 deletions(-)
v3: 9 files changed, 260 insertions(+), 129 deletions(-)
I can work on fixup patches to restore v3 changes on top of v2, but this
won't fix the bisectability issue.
Instead of fixing individual portions, reverting the merge commit
f1d5ca4: "Merge branch 'mv88e6xxx-switchdev-fdb'" would undo all the v2
series at once, then v3 can be merged on top of it.
Can you consider this as an option?
Thanks,
-v
From: David Miller <davem@davemloft.net> Date: 2015-08-11 17:38:21
From: Vivien Didelot <redacted>
Date: Tue, 11 Aug 2015 12:25:06 -0400 (EDT)
I can work on fixup patches to restore v3 changes on top of v2, but this
won't fix the bisectability issue.
Instead of fixing individual portions, reverting the merge commit
f1d5ca4: "Merge branch 'mv88e6xxx-switchdev-fdb'" would undo all the v2
series at once, then v3 can be merged on top of it.
Can you consider this as an option?
Nothing will fix bisectability, so don't try.
Reverting an entire series when you have the fix available
already is excessive.
So as I have already asked you, send a relative fixup to clear
up this situation.
Thanks.
From: Vivien Didelot <redacted>
Date: Tue, 11 Aug 2015 12:25:06 -0400 (EDT)
quoted
I can work on fixup patches to restore v3 changes on top of v2, but this
won't fix the bisectability issue.
Instead of fixing individual portions, reverting the merge commit
f1d5ca4: "Merge branch 'mv88e6xxx-switchdev-fdb'" would undo all the v2
series at once, then v3 can be merged on top of it.
Can you consider this as an option?
Nothing will fix bisectability, so don't try.
Reverting an entire series when you have the fix available
already is excessive.
So as I have already asked you, send a relative fixup to clear
up this situation.
What if the fix is to actually not break bisectability? Put differently,
my question is how do you value not rewriting history vs. breaking
bisectability (by accident of course)?
--
Florian
Put differently, my question is how do you value not rewriting
history vs. breaking bisectability (by accident of course)?
I never will rewrite history, ever.
Too many people clone my tree and depend upon it.
Sorry, I still don't understand. What are the consequences of:
git revert -m 1 f1d5ca4
Then applying v3?
You already did that in the past:
https://github.com/torvalds/linux/commit/1f2cd84
Thanks,
-v