On 3/25/19 7:06 PM, Florian Fainelli wrote:
On 3/23/19 8:23 PM, Vladimir Oltean wrote:
This refactors the two-phase transaction from dsa_slave_vlan_rx_add_vid
and also makes that code available for other functions from within DSA.
The newly exposed function either adds or deletes the specified VLAN
entry based on a boolean argument.
Signed-off-by: Vladimir Oltean <olte...@gmail.com>
The name of the function does not make it particularly clear that
passing false results in deleting the VLAN. Can you just wrap this under
a different function name that is only doing the two-step adding of
VLANs and keep using dsa_port_vlan_del() explicitly when you want to
remove a VLAN?
The reason why I made it this way was mainly to make the
switchdev_obj_port_vlan struct an implementation detail. That being so I
wouldn't need to keep and continuously modify this struct during the 3
times I'm calling the function from within the 05/13 patch ("net: dsa:
Optional VLAN-based port separation for switches without tagging"). I
did try to make use of the .vid_begin/.vid_end feature and batch some of
the function calls, but doing so would restrict possible values that the
rx_vid and tx_vid functions may return - dsa_port_setup_8021q_tagging()
would need to ensure that they are contiguous prior to batching them
into a single vlan object. By the way, I don't think anybody is making
any good use of this feature, it's just creating useless boilerplate as
of now.
The other thing is that if I were to wrap around dsa_port_vlan_add() and
get rid of the switchdev_obj_port_vlan and just pass the vid as u16, I'd
have to do the same wrapping for the dsa_port_vlan_del() function too.
Plus I'd have to keep 3 'if' conditions just to decide whether to call
*_add or *_del.
diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 221299b264f5..1b11b245e2d6 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -120,8 +120,10 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds,
int port, bool enabled)
/* The Rx VID is a regular VLAN on all others */
flags = BRIDGE_VLAN_INFO_UNTAGGED;
- err = dsa_port_trans_vlan_apply(other_dp, rx_vid, flags,
- enabled);
+ if (enabled)
+ err = __dsa_port_vlan_add(other_dp, rx_vid, flags);
+ else
+ err = __dsa_port_vlan_del(other_dp, rx_vid);
if (err) {
dev_err(ds->dev, "Failed to apply Rx VID %d to port %d:
%d\n",
rx_vid, port, err);
@@ -129,14 +131,20 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds,
int port, bool enabled)
}
}
/* Finally apply the Tx VID on this port and on the CPU port */
- err = dsa_port_trans_vlan_apply(dp, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED,
- enabled);
+ if (enabled)
+ err = __dsa_port_vlan_add(dp, tx_vid,
+ BRIDGE_VLAN_INFO_UNTAGGED);
+ else
+ err = __dsa_port_vlan_del(dp, tx_vid);
if (err) {
dev_err(ds->dev, "Failed to apply Tx VID %d on port %d: %d\n",
tx_vid, port, err);
return err;
}
- err = dsa_port_trans_vlan_apply(upstream_dp, tx_vid, 0, enabled);
+ if (enabled)
+ err = __dsa_port_vlan_add(upstream_dp, tx_vid, 0);
+ else
+ err = __dsa_port_vlan_del(upstream_dp, tx_vid);
if (err) {
dev_err(ds->dev, "Failed to apply Tx VID %d on port %d: %d\n",
tx_vid, upstream, err);
How does something like the above look like?
Thanks,
-Vladimir
---
net/dsa/dsa_priv.h | 2 ++
net/dsa/port.c | 24 ++++++++++++++++++++++++
net/dsa/slave.c | 16 ++--------------
3 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 093b7d145eb1..8048ced3708f 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -164,6 +164,8 @@ int dsa_port_pre_bridge_flags(const struct dsa_port *dp,
unsigned long flags,
struct switchdev_trans *trans);
int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
struct switchdev_trans *trans);
+int dsa_port_trans_vlan_apply(struct dsa_port *dp, u16 vid, u16 flags,
+ bool enabled);
int dsa_port_vlan_add(struct dsa_port *dp,
const struct switchdev_obj_port_vlan *vlan,
struct switchdev_trans *trans);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index a86fe3be1261..9c7358f98004 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -326,6 +326,30 @@ int dsa_port_vlan_del(struct dsa_port *dp,
return 0;
}
+int dsa_port_trans_vlan_apply(struct dsa_port *dp, u16 vid, u16 flags,
+ bool enabled)
+{
+ struct switchdev_obj_port_vlan vlan = {
+ .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
+ .flags = flags,
+ .vid_begin = vid,
+ .vid_end = vid,
+ };
+ struct switchdev_trans trans;
+ int err;
+
+ if (!enabled)
+ return dsa_port_vlan_del(dp, &vlan);
+
+ trans.ph_prepare = true;
+ err = dsa_port_vlan_add(dp, &vlan, &trans);
+ if (err == -EOPNOTSUPP)
+ return 0;
+
+ trans.ph_prepare = false;
+ return dsa_port_vlan_add(dp, &vlan, &trans);
+}
+
static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp)
{
struct device_node *phy_dn;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 093eef6f2599..3191ef74f6a1 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -987,13 +987,6 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device
*dev, __be16 proto,
u16 vid)
{
struct dsa_port *dp = dsa_slave_to_port(dev);
- struct switchdev_obj_port_vlan vlan = {
- .vid_begin = vid,
- .vid_end = vid,
- /* This API only allows programming tagged, non-PVID VIDs */
- .flags = 0,
- };
- struct switchdev_trans trans;
struct bridge_vlan_info info;
int ret;
@@ -1010,13 +1003,8 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
return -EBUSY;
}
- trans.ph_prepare = true;
- ret = dsa_port_vlan_add(dp, &vlan, &trans);
- if (ret == -EOPNOTSUPP)
- return 0;
-
- trans.ph_prepare = false;
- return dsa_port_vlan_add(dp, &vlan, &trans);
+ /* This API only allows programming tagged, non-PVID VIDs */
+ return dsa_port_trans_vlan_apply(dp, vid, 0, true);
}
static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
