diff mbox series

[v2,net-next,4/4] Revert "net: dsa: Add more convenient functions for installing port VLANs"

Message ID 20200910164857.1221202-5-olteanv@gmail.com
State Accepted
Delegated to: David Miller
Headers show
Series DSA tag_8021q cleanup | expand

Commit Message

Vladimir Oltean Sept. 10, 2020, 4:48 p.m. UTC 
From: Vladimir Oltean <vladimir.oltean@nxp.com>

This reverts commit 314f76d7a68bab0516aa52877944e6aacfa0fc3f.

Citing that commit message, the call graph was:

    dsa_slave_vlan_rx_add_vid   dsa_port_setup_8021q_tagging
                |                        |
                |                        |
                |          +-------------+
                |          |
                v          v
               dsa_port_vid_add      dsa_slave_port_obj_add
                      |                         |
                      +-------+         +-------+
                              |         |
                              v         v
                           dsa_port_vlan_add

Now that tag_8021q has its own ops structure, it no longer relies on
dsa_port_vid_add, and therefore on the dsa_switch_ops to install its
VLANs.

So dsa_port_vid_add now only has one single caller. So we can simplify
the call graph to what it was before, aka:

        dsa_slave_vlan_rx_add_vid     dsa_slave_port_obj_add
                      |                         |
                      +-------+         +-------+
                              |         |
                              v         v
                           dsa_port_vlan_add

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa_priv.h |  2 --
 net/dsa/port.c     | 33 ---------------------------------
 net/dsa/slave.c    | 34 +++++++++++++++++++++++++++++++---
 3 files changed, 31 insertions(+), 38 deletions(-)

Comments

Florian Fainelli Sept. 10, 2020, 7:52 p.m. UTC | #1 
On 9/10/2020 9:48 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> This reverts commit 314f76d7a68bab0516aa52877944e6aacfa0fc3f.
> 
> Citing that commit message, the call graph was:
> 
>      dsa_slave_vlan_rx_add_vid   dsa_port_setup_8021q_tagging
>                  |                        |
>                  |                        |
>                  |          +-------------+
>                  |          |
>                  v          v
>                 dsa_port_vid_add      dsa_slave_port_obj_add
>                        |                         |
>                        +-------+         +-------+
>                                |         |
>                                v         v
>                             dsa_port_vlan_add
> 
> Now that tag_8021q has its own ops structure, it no longer relies on
> dsa_port_vid_add, and therefore on the dsa_switch_ops to install its
> VLANs.
> 
> So dsa_port_vid_add now only has one single caller. So we can simplify
> the call graph to what it was before, aka:
> 
>          dsa_slave_vlan_rx_add_vid     dsa_slave_port_obj_add
>                        |                         |
>                        +-------+         +-------+
>                                |         |
>                                v         v
>                             dsa_port_vlan_add
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

I would be keen on keeping this function just because it encapsulates 
the details of creating the switchdev object and it may be useful to add 
additional functionality later on (like the DSA master RX VLAN 
filtering?), but would not object to its removal if others disagree.
Vladimir Oltean Sept. 11, 2020, 5:30 p.m. UTC | #2 
On Thu, Sep 10, 2020 at 12:52:03PM -0700, Florian Fainelli wrote:
> On 9/10/2020 9:48 AM, Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > This reverts commit 314f76d7a68bab0516aa52877944e6aacfa0fc3f.
> >
> > Citing that commit message, the call graph was:
> >
> >      dsa_slave_vlan_rx_add_vid   dsa_port_setup_8021q_tagging
> >                  |                        |
> >                  |                        |
> >                  |          +-------------+
> >                  |          |
> >                  v          v
> >                 dsa_port_vid_add      dsa_slave_port_obj_add
> >                        |                         |
> >                        +-------+         +-------+
> >                                |         |
> >                                v         v
> >                             dsa_port_vlan_add
> >
> > Now that tag_8021q has its own ops structure, it no longer relies on
> > dsa_port_vid_add, and therefore on the dsa_switch_ops to install its
> > VLANs.
> >
> > So dsa_port_vid_add now only has one single caller. So we can simplify
> > the call graph to what it was before, aka:
> >
> >          dsa_slave_vlan_rx_add_vid     dsa_slave_port_obj_add
> >                        |                         |
> >                        +-------+         +-------+
> >                                |         |
> >                                v         v
> >                             dsa_port_vlan_add
> >
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> I would be keen on keeping this function just because it encapsulates the
> details of creating the switchdev object and it may be useful to add
> additional functionality later on (like the DSA master RX VLAN filtering?),
> but would not object to its removal if others disagree.
> --
> Florian

Hmm, I don't think there's a lot of value in having it, it's confusing
to have such a layered call stack, and it shouldn't be an exported
symbol any longer in any case.
Also, I already have a patch that calls vlan_vid_add(master) and having
this dsa_port_vid_add() helper doesn't save much at all.

Thanks,
-Vladimir
Florian Fainelli Sept. 11, 2020, 5:31 p.m. UTC | #3 
On 9/11/2020 10:30 AM, Vladimir Oltean wrote:
> On Thu, Sep 10, 2020 at 12:52:03PM -0700, Florian Fainelli wrote:
>> On 9/10/2020 9:48 AM, Vladimir Oltean wrote:
>>> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>>>
>>> This reverts commit 314f76d7a68bab0516aa52877944e6aacfa0fc3f.
>>>
>>> Citing that commit message, the call graph was:
>>>
>>>       dsa_slave_vlan_rx_add_vid   dsa_port_setup_8021q_tagging
>>>                   |                        |
>>>                   |                        |
>>>                   |          +-------------+
>>>                   |          |
>>>                   v          v
>>>                  dsa_port_vid_add      dsa_slave_port_obj_add
>>>                         |                         |
>>>                         +-------+         +-------+
>>>                                 |         |
>>>                                 v         v
>>>                              dsa_port_vlan_add
>>>
>>> Now that tag_8021q has its own ops structure, it no longer relies on
>>> dsa_port_vid_add, and therefore on the dsa_switch_ops to install its
>>> VLANs.
>>>
>>> So dsa_port_vid_add now only has one single caller. So we can simplify
>>> the call graph to what it was before, aka:
>>>
>>>           dsa_slave_vlan_rx_add_vid     dsa_slave_port_obj_add
>>>                         |                         |
>>>                         +-------+         +-------+
>>>                                 |         |
>>>                                 v         v
>>>                              dsa_port_vlan_add
>>>
>>> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>>
>> I would be keen on keeping this function just because it encapsulates the
>> details of creating the switchdev object and it may be useful to add
>> additional functionality later on (like the DSA master RX VLAN filtering?),
>> but would not object to its removal if others disagree.
>> --
>> Florian
> 
> Hmm, I don't think there's a lot of value in having it, it's confusing
> to have such a layered call stack, and it shouldn't be an exported
> symbol any longer in any case.
> Also, I already have a patch that calls vlan_vid_add(master) and having
> this dsa_port_vid_add() helper doesn't save much at all.

OK, I am convinced:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
diff mbox series

Patch

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 1653e3377cb3..2da656d984ef 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -164,8 +164,6 @@  int dsa_port_vlan_add(struct dsa_port *dp,
 		      struct switchdev_trans *trans);
 int dsa_port_vlan_del(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan);
-int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags);
-int dsa_port_vid_del(struct dsa_port *dp, u16 vid);
 int dsa_port_link_register_of(struct dsa_port *dp);
 void dsa_port_link_unregister_of(struct dsa_port *dp);
 extern const struct phylink_mac_ops dsa_port_phylink_mac_ops;
diff --git a/net/dsa/port.c b/net/dsa/port.c
index e23ece229c7e..46c9bf709683 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -433,39 +433,6 @@  int dsa_port_vlan_del(struct dsa_port *dp,
 	return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 }
 
-int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags)
-{
-	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;
-
-	trans.ph_prepare = true;
-	err = dsa_port_vlan_add(dp, &vlan, &trans);
-	if (err)
-		return err;
-
-	trans.ph_prepare = false;
-	return dsa_port_vlan_add(dp, &vlan, &trans);
-}
-EXPORT_SYMBOL(dsa_port_vid_add);
-
-int dsa_port_vid_del(struct dsa_port *dp, u16 vid)
-{
-	struct switchdev_obj_port_vlan vlan = {
-		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-		.vid_begin = vid,
-		.vid_end = vid,
-	};
-
-	return dsa_port_vlan_del(dp, &vlan);
-}
-EXPORT_SYMBOL(dsa_port_vid_del);
-
 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 4987f94a8f52..66a5268398a5 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1233,7 +1233,15 @@  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 = {
+		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
+		.vid_begin = vid,
+		.vid_end = vid,
+		/* This API only allows programming tagged, non-PVID VIDs */
+		.flags = 0,
+	};
 	struct bridge_vlan_info info;
+	struct switchdev_trans trans;
 	int ret;
 
 	/* Check for a possible bridge VLAN entry now since there is no
@@ -1252,11 +1260,25 @@  static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
 			return -EBUSY;
 	}
 
-	ret = dsa_port_vid_add(dp, vid, 0);
+	/* User port... */
+	trans.ph_prepare = true;
+	ret = dsa_port_vlan_add(dp, &vlan, &trans);
+	if (ret)
+		return ret;
+
+	trans.ph_prepare = false;
+	ret = dsa_port_vlan_add(dp, &vlan, &trans);
 	if (ret)
 		return ret;
 
-	ret = dsa_port_vid_add(dp->cpu_dp, vid, 0);
+	/* And CPU port... */
+	trans.ph_prepare = true;
+	ret = dsa_port_vlan_add(dp->cpu_dp, &vlan, &trans);
+	if (ret)
+		return ret;
+
+	trans.ph_prepare = false;
+	ret = dsa_port_vlan_add(dp->cpu_dp, &vlan, &trans);
 	if (ret)
 		return ret;
 
@@ -1267,6 +1289,12 @@  static int dsa_slave_vlan_rx_kill_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 bridge_vlan_info info;
 	int ret;
 
@@ -1289,7 +1317,7 @@  static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
 	/* Do not deprogram the CPU port as it may be shared with other user
 	 * ports which can be members of this VLAN as well.
 	 */
-	return dsa_port_vid_del(dp, vid);
+	return dsa_port_vlan_del(dp, &vlan);
 }
 
 struct dsa_hw_port {