diff mbox series

[net-next,4/5] net: mscc: ocelot: make entry_type a member of struct ocelot_multicast

Message ID 20201029022738.722794-5-vladimir.oltean@nxp.com
State Accepted
Delegated to: David Miller
Headers show
Series L2 multicast forwarding for Ocelot switch | expand

Checks

Context Check Description
jkicinski/cover_letter success Link
jkicinski/fixes_present success Link
jkicinski/patch_count success Link
jkicinski/tree_selection success Clearly marked for net-next
jkicinski/subject_prefix success Link
jkicinski/source_inline success Was 0 now: 0
jkicinski/verify_signedoff success Link
jkicinski/module_param success Was 0 now: 0
jkicinski/build_32bit fail Errors and warnings before: 4 this patch: 4
jkicinski/kdoc success Errors and warnings before: 0 this patch: 0
jkicinski/verify_fixes success Link
jkicinski/checkpatch fail Link
jkicinski/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
jkicinski/header_inline success Link
jkicinski/stable success Stable not CCed

Commit Message

Vladimir Oltean Oct. 29, 2020, 2:27 a.m. UTC 
This saves a re-classification of the MDB address on deletion.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 51 +++++++++++++++---------------
 drivers/net/ethernet/mscc/ocelot.h | 17 +++++-----
 2 files changed, 34 insertions(+), 34 deletions(-)

Comments

Florian Fainelli Oct. 29, 2020, 2:37 a.m. UTC | #1 
On 10/28/2020 7:27 PM, Vladimir Oltean wrote:
> This saves a re-classification of the MDB address on deletion.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

[snip]

>  	mc = ocelot_multicast_get(ocelot, mdb->addr, vid);
>  	if (!mc) {
>  		/* New entry */
> -		int pgid = ocelot_mdb_get_pgid(ocelot, entry_type);
> +		int pgid;
> +
> +		mc = devm_kzalloc(ocelot->dev, sizeof(*mc), GFP_KERNEL);

If the MDB object is programmed with SWITCHDEV_OBJ_ID_HOST_MDB then you
would need this gfp_t to be GFP_ATOMIC per
net/bridge/br_mdb.c::__br_mdb_notify, if this is a regular
SWITCHDEV_OBJ_ID_MDB then GFP_KERNEL appears to be fine.

Looks like this existed before, so that might have to be fixed separately.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Vladimir Oltean Oct. 29, 2020, 11:06 a.m. UTC | #2 
On Wed, Oct 28, 2020 at 07:37:09PM -0700, Florian Fainelli wrote:
> If the MDB object is programmed with SWITCHDEV_OBJ_ID_HOST_MDB then you
> would need this gfp_t to be GFP_ATOMIC per
> net/bridge/br_mdb.c::__br_mdb_notify, if this is a regular
> SWITCHDEV_OBJ_ID_MDB then GFP_KERNEL appears to be fine.

Good point, I think I would need to do something more radical anyway, as
programming anything into the MAC table calls:
ocelot_mact_learn
-> ocelot_mact_wait_for_completion
   -> readx_poll_timeout
I'm thinking to move all of this into a workqueue.
Jakub Kicinski Oct. 31, 2020, 1:16 a.m. UTC | #3 
On Thu, 29 Oct 2020 04:27:37 +0200 Vladimir Oltean wrote:
> +		mc = devm_kzalloc(ocelot->dev, sizeof(*mc), GFP_KERNEL);
> +		if (!mc)
> +			return -ENOMEM;
> +
> +		mc->entry_type = ocelot_classify_mdb(mdb->addr);
> +		ether_addr_copy(mc->addr, mdb->addr);
> +		mc->vid = vid;
> +
> +		pgid = ocelot_mdb_get_pgid(ocelot, mc);
>  
>  		if (pgid < 0) {
>  			dev_err(ocelot->dev,
> @@ -1038,24 +1044,19 @@ int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
>  			return -ENOSPC;
>  		}

Transitionally leaking mc here on pgid < 0
Vladimir Oltean Oct. 31, 2020, 9:07 a.m. UTC | #4 
On Fri, Oct 30, 2020 at 06:16:31PM -0700, Jakub Kicinski wrote:
> On Thu, 29 Oct 2020 04:27:37 +0200 Vladimir Oltean wrote:
> > +		mc = devm_kzalloc(ocelot->dev, sizeof(*mc), GFP_KERNEL);
> > +		if (!mc)
> > +			return -ENOMEM;
> > +
> > +		mc->entry_type = ocelot_classify_mdb(mdb->addr);
> > +		ether_addr_copy(mc->addr, mdb->addr);
> > +		mc->vid = vid;
> > +
> > +		pgid = ocelot_mdb_get_pgid(ocelot, mc);
> >  
> >  		if (pgid < 0) {
> >  			dev_err(ocelot->dev,
> > @@ -1038,24 +1044,19 @@ int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
> >  			return -ENOSPC;
> >  		}
> 
> Transitionally leaking mc here on pgid < 0

Is it a real leakage if it's allocated with devm though? At some point
it's still going to be freed. Nonetheless I agree there's still a lot of
work to do. Maybe I didn't choose the best moment to concentrate on a
new feature, should have focused on cleanup more beforehand, including a
change from devm to plain allocation/free of resources.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index ea49d715c9d0..713ab6ec8c8d 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -962,7 +962,7 @@  static enum macaccess_entry_type ocelot_classify_mdb(const unsigned char *addr)
 }
 
 static int ocelot_mdb_get_pgid(struct ocelot *ocelot,
-			       enum macaccess_entry_type entry_type)
+			       const struct ocelot_multicast *mc)
 {
 	int pgid;
 
@@ -971,8 +971,8 @@  static int ocelot_mdb_get_pgid(struct ocelot *ocelot,
 	 * destination mask table (PGID), the destination set is programmed as
 	 * part of the entry MAC address.", and the DEST_IDX is set to 0.
 	 */
-	if (entry_type == ENTRYTYPE_MACv4 ||
-	    entry_type == ENTRYTYPE_MACv6)
+	if (mc->entry_type == ENTRYTYPE_MACv4 ||
+	    mc->entry_type == ENTRYTYPE_MACv6)
 		return 0;
 
 	for_each_nonreserved_multicast_dest_pgid(ocelot, pgid) {
@@ -994,16 +994,15 @@  static int ocelot_mdb_get_pgid(struct ocelot *ocelot,
 }
 
 static void ocelot_encode_ports_to_mdb(unsigned char *addr,
-				       struct ocelot_multicast *mc,
-				       enum macaccess_entry_type entry_type)
+				       struct ocelot_multicast *mc)
 {
 	ether_addr_copy(addr, mc->addr);
 
-	if (entry_type == ENTRYTYPE_MACv4) {
+	if (mc->entry_type == ENTRYTYPE_MACv4) {
 		addr[0] = 0;
 		addr[1] = mc->ports >> 8;
 		addr[2] = mc->ports & 0xff;
-	} else if (entry_type == ENTRYTYPE_MACv6) {
+	} else if (mc->entry_type == ENTRYTYPE_MACv6) {
 		addr[0] = mc->ports >> 8;
 		addr[1] = mc->ports & 0xff;
 	}
@@ -1013,7 +1012,6 @@  int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
 			const struct switchdev_obj_port_mdb *mdb)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
-	enum macaccess_entry_type entry_type;
 	unsigned char addr[ETH_ALEN];
 	struct ocelot_multicast *mc;
 	u16 vid = mdb->vid;
@@ -1024,12 +1022,20 @@  int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
 	if (!vid)
 		vid = ocelot_port->pvid;
 
-	entry_type = ocelot_classify_mdb(mdb->addr);
-
 	mc = ocelot_multicast_get(ocelot, mdb->addr, vid);
 	if (!mc) {
 		/* New entry */
-		int pgid = ocelot_mdb_get_pgid(ocelot, entry_type);
+		int pgid;
+
+		mc = devm_kzalloc(ocelot->dev, sizeof(*mc), GFP_KERNEL);
+		if (!mc)
+			return -ENOMEM;
+
+		mc->entry_type = ocelot_classify_mdb(mdb->addr);
+		ether_addr_copy(mc->addr, mdb->addr);
+		mc->vid = vid;
+
+		pgid = ocelot_mdb_get_pgid(ocelot, mc);
 
 		if (pgid < 0) {
 			dev_err(ocelot->dev,
@@ -1038,24 +1044,19 @@  int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
 			return -ENOSPC;
 		}
 
-		mc = devm_kzalloc(ocelot->dev, sizeof(*mc), GFP_KERNEL);
-		if (!mc)
-			return -ENOMEM;
-
-		ether_addr_copy(mc->addr, mdb->addr);
-		mc->vid = vid;
 		mc->pgid = pgid;
 
 		list_add_tail(&mc->list, &ocelot->multicast);
 	} else {
-		ocelot_encode_ports_to_mdb(addr, mc, entry_type);
+		ocelot_encode_ports_to_mdb(addr, mc);
 		ocelot_mact_forget(ocelot, addr, vid);
 	}
 
 	mc->ports |= BIT(port);
-	ocelot_encode_ports_to_mdb(addr, mc, entry_type);
+	ocelot_encode_ports_to_mdb(addr, mc);
 
-	return ocelot_mact_learn(ocelot, mc->pgid, addr, vid, entry_type);
+	return ocelot_mact_learn(ocelot, mc->pgid, addr, vid,
+				 mc->entry_type);
 }
 EXPORT_SYMBOL(ocelot_port_mdb_add);
 
@@ -1063,7 +1064,6 @@  int ocelot_port_mdb_del(struct ocelot *ocelot, int port,
 			const struct switchdev_obj_port_mdb *mdb)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
-	enum macaccess_entry_type entry_type;
 	unsigned char addr[ETH_ALEN];
 	struct ocelot_multicast *mc;
 	u16 vid = mdb->vid;
@@ -1078,9 +1078,7 @@  int ocelot_port_mdb_del(struct ocelot *ocelot, int port,
 	if (!mc)
 		return -ENOENT;
 
-	entry_type = ocelot_classify_mdb(mdb->addr);
-
-	ocelot_encode_ports_to_mdb(addr, mc, entry_type);
+	ocelot_encode_ports_to_mdb(addr, mc);
 	ocelot_mact_forget(ocelot, addr, vid);
 
 	mc->ports &= ~BIT(port);
@@ -1090,9 +1088,10 @@  int ocelot_port_mdb_del(struct ocelot *ocelot, int port,
 		return 0;
 	}
 
-	ocelot_encode_ports_to_mdb(addr, mc, entry_type);
+	ocelot_encode_ports_to_mdb(addr, mc);
 
-	return ocelot_mact_learn(ocelot, mc->pgid, addr, vid, entry_type);
+	return ocelot_mact_learn(ocelot, mc->pgid, addr, vid,
+				 mc->entry_type);
 }
 EXPORT_SYMBOL(ocelot_port_mdb_del);
 
diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
index abb407dff93c..7f8b34c49971 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -41,14 +41,6 @@  struct frame_info {
 	u32 timestamp;	/* rew_val */
 };
 
-struct ocelot_multicast {
-	struct list_head list;
-	unsigned char addr[ETH_ALEN];
-	u16 vid;
-	u16 ports;
-	int pgid;
-};
-
 struct ocelot_port_tc {
 	bool block_shared;
 	unsigned long offload_cnt;
@@ -87,6 +79,15 @@  enum macaccess_entry_type {
 	ENTRYTYPE_MACv6,
 };
 
+struct ocelot_multicast {
+	struct list_head list;
+	enum macaccess_entry_type entry_type;
+	unsigned char addr[ETH_ALEN];
+	u16 vid;
+	u16 ports;
+	int pgid;
+};
+
 int ocelot_port_fdb_do_dump(const unsigned char *addr, u16 vid,
 			    bool is_static, void *data);
 int ocelot_mact_learn(struct ocelot *ocelot, int port,