[PATCH v2 net-next 0/2] net: dsa: lan9303: Add fdb/mdb methods

[PATCH v2 net-next 0/2] net: dsa: lan9303: Add fdb/mdb methods

[Egil Hjelmeland]

This series add support for accessing and managing the lan9303 ALR 
(Address Logic Resolution). 

The first patch add low level functions for accessing the ALR, along
with port_fast_age and port_fdb_dump methods.

The second patch add functions for managing ALR entires, along with
remaining fdb/mdb methods. 

Note that to complete STP support, a special ALR entry with the STP eth
address must be added too. This must be addressed later.

Comments welcome!

Changes v1 -> v2:
 - Patch 2: Removed question comment

Egil Hjelmeland (2):
  net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods
  net: dsa: lan9303: Add fdb/mdb manipulation

 drivers/net/dsa/lan9303-core.c | 331 +++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/lan9303.h      |  11 ++
 2 files changed, 342 insertions(+)

-- 
2.11.0

[PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods

[Egil Hjelmeland]

Add DSA method port_fast_age as a step to STP support.

Add low level functions for accessing the lan9303 ALR (Address Logic
Resolution).

Added DSA method port_fdb_dump

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 159 +++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/lan9303.h      |   2 +
 2 files changed, 161 insertions(+)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 09a748327fc6..ae904242b001 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -124,6 +124,21 @@
 #define LAN9303_MAC_RX_CFG_2 0x0c01
 #define LAN9303_MAC_TX_CFG_2 0x0c40
 #define LAN9303_SWE_ALR_CMD 0x1800
+# define ALR_CMD_MAKE_ENTRY    BIT(2)
+# define ALR_CMD_GET_FIRST     BIT(1)
+# define ALR_CMD_GET_NEXT      BIT(0)
+#define LAN9303_SWE_ALR_WR_DAT_0 0x1801
+#define LAN9303_SWE_ALR_WR_DAT_1 0x1802
+# define ALR_DAT1_VALID        BIT(26)
+# define ALR_DAT1_END_OF_TABL  BIT(25)
+# define ALR_DAT1_AGE_OVERRID  BIT(25)
+# define ALR_DAT1_STATIC       BIT(24)
+# define ALR_DAT1_PORT_BITOFFS  16
+# define ALR_DAT1_PORT_MASK    (7 << ALR_DAT1_PORT_BITOFFS)
+#define LAN9303_SWE_ALR_RD_DAT_0 0x1805
+#define LAN9303_SWE_ALR_RD_DAT_1 0x1806
+#define LAN9303_SWE_ALR_CMD_STS 0x1808
+# define ALR_STS_MAKE_PEND     BIT(0)
 #define LAN9303_SWE_VLAN_CMD 0x180b
 # define LAN9303_SWE_VLAN_CMD_RNW BIT(5)
 # define LAN9303_SWE_VLAN_CMD_PVIDNVLAN BIT(4)
@@ -478,6 +493,125 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
 	return 0;
 }
 
+/* ----------------- Address Logic Resolution (ALR)------------------*/
+
+/* Map ALR-port bits to port bitmap, and back*/
+static const int alrport_2_portmap[] = {1, 2, 4, 0, 3, 5, 6, 7 };
+static const int portmap_2_alrport[] = {3, 0, 1, 4, 2, 5, 6, 7 };
+
+/* ALR: Actual register access functions */
+
+/* This function will wait a while until mask & reg == value */
+/* Otherwise, return timeout */
+static int lan9303_csr_reg_wait(struct lan9303 *chip, int regno,
+				int mask, char value)
+{
+	int i;
+
+	for (i = 0; i < 0x1000; i++) {
+		u32 reg;
+
+		lan9303_read_switch_reg(chip, regno, &reg);
+		if ((reg & mask) == value)
+			return 0;
+		usleep_range(1000, 2000);
+	}
+	return -ETIMEDOUT;
+}
+
+static int lan9303_alr_make_entry_raw(struct lan9303 *chip, u32 dat0, u32 dat1)
+{
+	lan9303_write_switch_reg(
+		chip, LAN9303_SWE_ALR_WR_DAT_0, dat0);
+	lan9303_write_switch_reg(
+		chip, LAN9303_SWE_ALR_WR_DAT_1, dat1);
+	lan9303_write_switch_reg(
+		chip, LAN9303_SWE_ALR_CMD, ALR_CMD_MAKE_ENTRY);
+	lan9303_csr_reg_wait(
+		chip, LAN9303_SWE_ALR_CMD_STS, ALR_STS_MAKE_PEND, 0);
+	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
+	return 0;
+}
+
+typedef void alr_loop_cb_t(struct lan9303 *chip, u32 dat0, u32 dat1,
+			   int portmap, void *ctx);
+
+static void lan9303_alr_loop(struct lan9303 *chip, alr_loop_cb_t *cb, void *ctx)
+{
+	int i;
+
+	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_FIRST);
+	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
+
+	for (i = 1; i < LAN9303_NUM_ALR_RECORDS; i++) {
+		u32 dat0, dat1;
+		int alrport, portmap;
+
+		lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_0, &dat0);
+		lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_1, &dat1);
+		if (dat1 & ALR_DAT1_END_OF_TABL)
+			break;
+
+		alrport = (dat1 & ALR_DAT1_PORT_MASK) >> ALR_DAT1_PORT_BITOFFS;
+		portmap = alrport_2_portmap[alrport];
+
+		cb(chip, dat0, dat1, portmap, ctx);
+
+		lan9303_write_switch_reg(
+			chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_NEXT);
+		lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
+	}
+}
+
+/* ALR: lan9303_alr_loop callback functions */
+
+static void alr_reg_to_mac(u32 dat0, u32 dat1, u8 mac[6])
+{
+	mac[0] = (dat0 >>  0) & 0xff;
+	mac[1] = (dat0 >>  8) & 0xff;
+	mac[2] = (dat0 >> 16) & 0xff;
+	mac[3] = (dat0 >> 24) & 0xff;
+	mac[4] = (dat1 >>  0) & 0xff;
+	mac[5] = (dat1 >>  8) & 0xff;
+}
+
+/* Clear learned (non-static) entry on given port */
+static void alr_loop_cb_del_port_learned(struct lan9303 *chip, u32 dat0,
+					 u32 dat1, int portmap, void *ctx)
+{
+	int *port = ctx;
+
+	if (((BIT(*port) & portmap) == 0) || (dat1 & ALR_DAT1_STATIC))
+		return;
+
+	/* learned entries has only one port, we can just delete */
+	dat1 &= ~ALR_DAT1_VALID; /* delete entry */
+	lan9303_alr_make_entry_raw(chip, dat0, dat1);
+}
+
+struct port_fdb_dump_ctx {
+	int port;
+	void *data;
+	dsa_fdb_dump_cb_t *cb;
+};
+
+static void alr_loop_cb_fdb_port_dump(struct lan9303 *chip, u32 dat0,
+				      u32 dat1, int portmap, void *ctx)
+{
+	struct port_fdb_dump_ctx *dump_ctx = ctx;
+	u8 mac[ETH_ALEN];
+	bool is_static;
+
+	if ((BIT(dump_ctx->port) & portmap) == 0)
+		return;
+
+	alr_reg_to_mac(dat0, dat1, mac);
+	is_static = !!(dat1 & ALR_DAT1_STATIC);
+	dump_ctx->cb(mac, 0, is_static, dump_ctx->data);
+}
+
+/* --------------------- Various chip setup ----------------------*/
+
 static int lan9303_disable_processing_port(struct lan9303 *chip,
 					   unsigned int port)
 {
@@ -923,6 +1057,29 @@ static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
 	/* else: touching SWE_PORT_STATE would break port separation */
 }
 
+static void lan9303_port_fast_age(struct dsa_switch *ds, int port)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(%d)\n", __func__, port);
+	lan9303_alr_loop(chip, alr_loop_cb_del_port_learned, &port);
+}
+
+static int lan9303_port_fdb_dump(struct dsa_switch *ds, int port,
+				 dsa_fdb_dump_cb_t *cb, void *data)
+{
+	struct lan9303 *chip = ds->priv;
+	struct port_fdb_dump_ctx dump_ctx = {
+		.port = port,
+		.data = data,
+		.cb   = cb,
+	};
+
+	dev_dbg(chip->dev, "%s(%d)\n", __func__, port);
+	lan9303_alr_loop(chip, alr_loop_cb_fdb_port_dump, &dump_ctx);
+	return 0;
+}
+
 static const struct dsa_switch_ops lan9303_switch_ops = {
 	.get_tag_protocol = lan9303_get_tag_protocol,
 	.setup = lan9303_setup,
@@ -937,6 +1094,8 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
 	.port_bridge_join       = lan9303_port_bridge_join,
 	.port_bridge_leave      = lan9303_port_bridge_leave,
 	.port_stp_state_set     = lan9303_port_stp_state_set,
+	.port_fast_age          = lan9303_port_fast_age,
+	.port_fdb_dump          = lan9303_port_fdb_dump,
 };
 
 static int lan9303_register_switch(struct lan9303 *chip)
diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
index 68ecd544b658..4db323d65741 100644
--- a/drivers/net/dsa/lan9303.h
+++ b/drivers/net/dsa/lan9303.h
@@ -11,6 +11,8 @@ struct lan9303_phy_ops {
 			     int regnum, u16 val);
 };
 
+#define LAN9303_NUM_ALR_RECORDS 512
+
 struct lan9303 {
 	struct device *dev;
 	struct regmap *regmap;
-- 
2.11.0

[PATCH v2 net-next 2/2] net: dsa: lan9303: Add fdb/mdb manipulation

[Egil Hjelmeland]

Add functions for managing the lan9303 ALR (Address Logic
Resolution).

Implement DSA methods: port_fdb_add, port_fdb_del, port_mdb_prepare,
port_mdb_add and port_mdb_del.

Since the lan9303 do not offer reading specific ALR entry, the driver
caches all static entries - in a flat table.

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 172 +++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/lan9303.h      |   9 +++
 2 files changed, 181 insertions(+)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index ae904242b001..71e91435e479 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -19,6 +19,7 @@
 #include <linux/mii.h>
 #include <linux/phy.h>
 #include <linux/if_bridge.h>
+#include <linux/etherdevice.h>
 
 #include "lan9303.h"
 
@@ -499,6 +500,37 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
 static const int alrport_2_portmap[] = {1, 2, 4, 0, 3, 5, 6, 7 };
 static const int portmap_2_alrport[] = {3, 0, 1, 4, 2, 5, 6, 7 };
 
+/* ALR: Cache static entries: mac address + port bitmap */
+
+/* Return pointer to first free ALR cache entry, return NULL if none */
+static struct lan9303_alr_cache_entry *
+lan9303_alr_cache_find_free(struct lan9303 *chip)
+{
+	int i;
+	struct lan9303_alr_cache_entry *entr = chip->alr_cache;
+
+	for (i = 0; i < LAN9303_NUM_ALR_RECORDS; i++, entr++)
+		if (entr->port_map == 0)
+			return entr;
+	return NULL;
+}
+
+/* Return pointer to ALR cache entry matching MAC address */
+static struct lan9303_alr_cache_entry *
+lan9303_alr_cache_find_mac(struct lan9303 *chip, const u8 *mac_addr)
+{
+	int i;
+	struct lan9303_alr_cache_entry *entr = chip->alr_cache;
+
+	BUILD_BUG_ON_MSG(sizeof(struct lan9303_alr_cache_entry) & 1,
+			 "ether_addr_equal require u16 alignment");
+
+	for (i = 0; i < LAN9303_NUM_ALR_RECORDS; i++, entr++)
+		if (ether_addr_equal(entr->mac_addr, mac_addr))
+			return entr;
+	return NULL;
+}
+
 /* ALR: Actual register access functions */
 
 /* This function will wait a while until mask & reg == value */
@@ -610,6 +642,73 @@ static void alr_loop_cb_fdb_port_dump(struct lan9303 *chip, u32 dat0,
 	dump_ctx->cb(mac, 0, is_static, dump_ctx->data);
 }
 
+/* ALR: Add/modify/delete ALR entries */
+
+/* Set a static ALR entry. Delete entry if port_map is zero */
+static void lan9303_alr_set_entry(struct lan9303 *chip, const u8 *mac,
+				  u8 port_map, bool stp_override)
+{
+	u32 dat0, dat1, alr_port;
+
+	dev_dbg(chip->dev, "%s(%pM, %d)\n", __func__, mac, port_map);
+	dat1 = ALR_DAT1_STATIC;
+	if (port_map)
+		dat1 |= ALR_DAT1_VALID; /* otherwise no ports: delete entry */
+	if (stp_override)
+		dat1 |= ALR_DAT1_AGE_OVERRID;
+
+	alr_port = portmap_2_alrport[port_map & 7];
+	dat1 &= ~ALR_DAT1_PORT_MASK;
+	dat1 |= alr_port << ALR_DAT1_PORT_BITOFFS;
+
+	dat0 = 0;
+	dat0 |= (mac[0] << 0);
+	dat0 |= (mac[1] << 8);
+	dat0 |= (mac[2] << 16);
+	dat0 |= (mac[3] << 24);
+
+	dat1 |= (mac[4] << 0);
+	dat1 |= (mac[5] << 8);
+
+	lan9303_alr_make_entry_raw(chip, dat0, dat1);
+}
+
+/* Add port to static ALR entry, create new static entry if needed */
+static int lan9303_alr_add_port(struct lan9303 *chip, const u8 *mac,
+				int port, bool stp_override)
+{
+	struct lan9303_alr_cache_entry *entr;
+
+	entr = lan9303_alr_cache_find_mac(chip, mac);
+	if (!entr) { /*New entry */
+		entr = lan9303_alr_cache_find_free(chip);
+		if (!entr)
+			return -ENOSPC;
+		ether_addr_copy(entr->mac_addr, mac);
+	}
+	entr->port_map |= BIT(port);
+	entr->stp_override = stp_override;
+	lan9303_alr_set_entry(chip, mac, entr->port_map, stp_override);
+	return 0;
+}
+
+/* Delete static port from ALR entry, delete entry if last port */
+static int lan9303_alr_del_port(struct lan9303 *chip, const u8 *mac,
+				int port)
+{
+	struct lan9303_alr_cache_entry *entr;
+
+	entr = lan9303_alr_cache_find_mac(chip, mac);
+	if (!entr)
+		return 0;  /* no static entry found */
+
+	entr->port_map &= ~BIT(port);
+	if (entr->port_map == 0) /* zero means its free again */
+		eth_zero_addr(&entr->port_map);
+	lan9303_alr_set_entry(chip, mac, entr->port_map, entr->stp_override);
+	return 0;
+}
+
 /* --------------------- Various chip setup ----------------------*/
 
 static int lan9303_disable_processing_port(struct lan9303 *chip,
@@ -1065,6 +1164,30 @@ static void lan9303_port_fast_age(struct dsa_switch *ds, int port)
 	lan9303_alr_loop(chip, alr_loop_cb_del_port_learned, &port);
 }
 
+static int lan9303_port_fdb_add(struct dsa_switch *ds, int port,
+				const unsigned char *addr, u16 vid)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(%d, %pM, %d)\n", __func__, port, addr, vid);
+	if (vid)
+		return -EOPNOTSUPP;
+	return lan9303_alr_add_port(chip, addr, port, false);
+}
+
+static int lan9303_port_fdb_del(struct dsa_switch *ds, int port,
+				const unsigned char *addr, u16 vid)
+
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(%d, %pM, %d)\n", __func__, port, addr, vid);
+	if (vid)
+		return -EOPNOTSUPP;
+	lan9303_alr_del_port(chip, addr, port);
+	return 0;
+}
+
 static int lan9303_port_fdb_dump(struct dsa_switch *ds, int port,
 				 dsa_fdb_dump_cb_t *cb, void *data)
 {
@@ -1080,6 +1203,50 @@ static int lan9303_port_fdb_dump(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static int lan9303_port_mdb_prepare(
+		struct dsa_switch *ds, int port,
+		const struct switchdev_obj_port_mdb *mdb,
+		struct switchdev_trans *trans)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(%d, %pM, %d)\n", __func__, port, mdb->addr,
+		mdb->vid);
+	if (mdb->vid)
+		return -EOPNOTSUPP;
+	if (lan9303_alr_cache_find_mac(chip, mdb->addr))
+		return 0;
+	if (!lan9303_alr_cache_find_free(chip))
+		return -ENOSPC;
+	return 0;
+}
+
+static void lan9303_port_mdb_add(
+		struct dsa_switch *ds, int port,
+		const struct switchdev_obj_port_mdb *mdb,
+		struct switchdev_trans *trans)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(%d, %pM, %d)\n", __func__, port, mdb->addr,
+		mdb->vid);
+	lan9303_alr_add_port(chip, mdb->addr, port, false);
+}
+
+static int lan9303_port_mdb_del(
+		struct dsa_switch *ds, int port,
+		const struct switchdev_obj_port_mdb *mdb)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(%d, %pM, %d)\n", __func__, port, mdb->addr,
+		mdb->vid);
+	if (mdb->vid)
+		return -EOPNOTSUPP;
+	lan9303_alr_del_port(chip, mdb->addr, port);
+	return 0;
+}
+
 static const struct dsa_switch_ops lan9303_switch_ops = {
 	.get_tag_protocol = lan9303_get_tag_protocol,
 	.setup = lan9303_setup,
@@ -1095,7 +1262,12 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
 	.port_bridge_leave      = lan9303_port_bridge_leave,
 	.port_stp_state_set     = lan9303_port_stp_state_set,
 	.port_fast_age          = lan9303_port_fast_age,
+	.port_fdb_add           = lan9303_port_fdb_add,
+	.port_fdb_del           = lan9303_port_fdb_del,
 	.port_fdb_dump          = lan9303_port_fdb_dump,
+	.port_mdb_prepare       = lan9303_port_mdb_prepare,
+	.port_mdb_add           = lan9303_port_mdb_add,
+	.port_mdb_del           = lan9303_port_mdb_del,
 };
 
 static int lan9303_register_switch(struct lan9303 *chip)
diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
index 4db323d65741..d807b1be35f2 100644
--- a/drivers/net/dsa/lan9303.h
+++ b/drivers/net/dsa/lan9303.h
@@ -12,6 +12,11 @@ struct lan9303_phy_ops {
 };
 
 #define LAN9303_NUM_ALR_RECORDS 512
+struct lan9303_alr_cache_entry {
+	u8  mac_addr[ETH_ALEN];
+	u8  port_map;           /* Bitmap of ports. Zero if unused entry */
+	u8  stp_override;       /* non zero if set ALR_DAT1_AGE_OVERRID */
+};
 
 struct lan9303 {
 	struct device *dev;
@@ -25,6 +30,10 @@ struct lan9303 {
 	const struct lan9303_phy_ops *ops;
 	bool is_bridged; /* true if port 1 and 2 are bridged */
 	u32 swe_port_state; /* remember SWE_PORT_STATE while not bridged */
+	/* LAN9303 do not offer reading specific ALR entry. Cache all
+	 * static entries in a flat table
+	 **/
+	struct lan9303_alr_cache_entry alr_cache[LAN9303_NUM_ALR_RECORDS];
 };
 
 extern const struct regmap_access_table lan9303_register_set;
-- 
2.11.0

Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods

[Vivien Didelot]

Hi Egil,

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

> Add DSA method port_fast_age as a step to STP support.
>
> Add low level functions for accessing the lan9303 ALR (Address Logic
> Resolution).
>
> Added DSA method port_fdb_dump
>
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

The patch looks good overall, a few nitpicks though.

> ---
>  drivers/net/dsa/lan9303-core.c | 159 +++++++++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/lan9303.h      |   2 +
>  2 files changed, 161 insertions(+)
>
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 09a748327fc6..ae904242b001 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -124,6 +124,21 @@
>  #define LAN9303_MAC_RX_CFG_2 0x0c01
>  #define LAN9303_MAC_TX_CFG_2 0x0c40
>  #define LAN9303_SWE_ALR_CMD 0x1800
> +# define ALR_CMD_MAKE_ENTRY    BIT(2)
> +# define ALR_CMD_GET_FIRST     BIT(1)
> +# define ALR_CMD_GET_NEXT      BIT(0)
> +#define LAN9303_SWE_ALR_WR_DAT_0 0x1801
> +#define LAN9303_SWE_ALR_WR_DAT_1 0x1802
> +# define ALR_DAT1_VALID        BIT(26)
> +# define ALR_DAT1_END_OF_TABL  BIT(25)
> +# define ALR_DAT1_AGE_OVERRID  BIT(25)
> +# define ALR_DAT1_STATIC       BIT(24)
> +# define ALR_DAT1_PORT_BITOFFS  16
> +# define ALR_DAT1_PORT_MASK    (7 << ALR_DAT1_PORT_BITOFFS)
> +#define LAN9303_SWE_ALR_RD_DAT_0 0x1805
> +#define LAN9303_SWE_ALR_RD_DAT_1 0x1806
> +#define LAN9303_SWE_ALR_CMD_STS 0x1808
> +# define ALR_STS_MAKE_PEND     BIT(0)

Why is there different spacing and prefix with these defines?

>  #define LAN9303_SWE_VLAN_CMD 0x180b
>  # define LAN9303_SWE_VLAN_CMD_RNW BIT(5)
>  # define LAN9303_SWE_VLAN_CMD_PVIDNVLAN BIT(4)
> @@ -478,6 +493,125 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
>  	return 0;
>  }
>  
> +/* ----------------- Address Logic Resolution (ALR)------------------*/
> +
> +/* Map ALR-port bits to port bitmap, and back*/

The (leading and trailing) spacing in your comments is often
inconsistent. You can use simple inline or block comments, no need for
fancy rules. Please refer to Documentation/networking/netdev-FAQ.txt for
block comment style in netdev though, they are a bit different.

> +static const int alrport_2_portmap[] = {1, 2, 4, 0, 3, 5, 6, 7 };
> +static const int portmap_2_alrport[] = {3, 0, 1, 4, 2, 5, 6, 7 };
> +
> +/* ALR: Actual register access functions */
> +
> +/* This function will wait a while until mask & reg == value */
> +/* Otherwise, return timeout */

Same, a single block comment will do the job.

> +static int lan9303_csr_reg_wait(struct lan9303 *chip, int regno,
> +				int mask, char value)
> +{
> +	int i;
> +
> +	for (i = 0; i < 0x1000; i++) {
> +		u32 reg;
> +
> +		lan9303_read_switch_reg(chip, regno, &reg);
> +		if ((reg & mask) == value)
> +			return 0;
> +		usleep_range(1000, 2000);
> +	}
> +	return -ETIMEDOUT;
> +}
> +
> +static int lan9303_alr_make_entry_raw(struct lan9303 *chip, u32 dat0, u32 dat1)
> +{
> +	lan9303_write_switch_reg(
> +		chip, LAN9303_SWE_ALR_WR_DAT_0, dat0);
> +	lan9303_write_switch_reg(
> +		chip, LAN9303_SWE_ALR_WR_DAT_1, dat1);
> +	lan9303_write_switch_reg(
> +		chip, LAN9303_SWE_ALR_CMD, ALR_CMD_MAKE_ENTRY);
> +	lan9303_csr_reg_wait(
> +		chip, LAN9303_SWE_ALR_CMD_STS, ALR_STS_MAKE_PEND, 0);

As I said in a previous series, please don't do this. Function arguments
must be vertically aligned with the opening parenthesis.

> +	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
> +	return 0;

A newline before a return statement is appreciated.

> +}
> +
> +typedef void alr_loop_cb_t(struct lan9303 *chip, u32 dat0, u32 dat1,
> +			   int portmap, void *ctx);
> +
> +static void lan9303_alr_loop(struct lan9303 *chip, alr_loop_cb_t *cb, void *ctx)
> +{
> +	int i;
> +
> +	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_FIRST);
> +	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
> +
> +	for (i = 1; i < LAN9303_NUM_ALR_RECORDS; i++) {
> +		u32 dat0, dat1;
> +		int alrport, portmap;
> +
> +		lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_0, &dat0);
> +		lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_1, &dat1);
> +		if (dat1 & ALR_DAT1_END_OF_TABL)
> +			break;
> +
> +		alrport = (dat1 & ALR_DAT1_PORT_MASK) >> ALR_DAT1_PORT_BITOFFS;
> +		portmap = alrport_2_portmap[alrport];
> +
> +		cb(chip, dat0, dat1, portmap, ctx);
> +
> +		lan9303_write_switch_reg(
> +			chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_NEXT);

Please align arguments with the opening parenthesis.

> +		lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
> +	}
> +}
> +
> +/* ALR: lan9303_alr_loop callback functions */
> +

No need for an extra newline if your comment refers directly to the
function below. It will also be consistent with the rest of your patch.

> +static void alr_reg_to_mac(u32 dat0, u32 dat1, u8 mac[6])
> +{
> +	mac[0] = (dat0 >>  0) & 0xff;
> +	mac[1] = (dat0 >>  8) & 0xff;
> +	mac[2] = (dat0 >> 16) & 0xff;
> +	mac[3] = (dat0 >> 24) & 0xff;
> +	mac[4] = (dat1 >>  0) & 0xff;
> +	mac[5] = (dat1 >>  8) & 0xff;
> +}
> +
> +/* Clear learned (non-static) entry on given port */
> +static void alr_loop_cb_del_port_learned(struct lan9303 *chip, u32 dat0,
> +					 u32 dat1, int portmap, void *ctx)
> +{
> +	int *port = ctx;

You can get the value directly to make the line below more readable:

    int port = *(int *)ctx;

> +
> +	if (((BIT(*port) & portmap) == 0) || (dat1 & ALR_DAT1_STATIC))
> +		return;
> +
> +	/* learned entries has only one port, we can just delete */
> +	dat1 &= ~ALR_DAT1_VALID; /* delete entry */
> +	lan9303_alr_make_entry_raw(chip, dat0, dat1);
> +}
> +
> +struct port_fdb_dump_ctx {
> +	int port;
> +	void *data;
> +	dsa_fdb_dump_cb_t *cb;
> +};
> +
> +static void alr_loop_cb_fdb_port_dump(struct lan9303 *chip, u32 dat0,
> +				      u32 dat1, int portmap, void *ctx)
> +{
> +	struct port_fdb_dump_ctx *dump_ctx = ctx;
> +	u8 mac[ETH_ALEN];
> +	bool is_static;
> +
> +	if ((BIT(dump_ctx->port) & portmap) == 0)
> +		return;
> +
> +	alr_reg_to_mac(dat0, dat1, mac);
> +	is_static = !!(dat1 & ALR_DAT1_STATIC);
> +	dump_ctx->cb(mac, 0, is_static, dump_ctx->data);
> +}
> +
> +/* --------------------- Various chip setup ----------------------*/
> +

This isn't a very useful comment, at least use an inline or block
comment if you want to keep it.

>  static int lan9303_disable_processing_port(struct lan9303 *chip,
>  					   unsigned int port)
>  {
> @@ -923,6 +1057,29 @@ static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
>  	/* else: touching SWE_PORT_STATE would break port separation */
>  }
>  
> +static void lan9303_port_fast_age(struct dsa_switch *ds, int port)
> +{
> +	struct lan9303 *chip = ds->priv;
> +
> +	dev_dbg(chip->dev, "%s(%d)\n", __func__, port);
> +	lan9303_alr_loop(chip, alr_loop_cb_del_port_learned, &port);
> +}
> +
> +static int lan9303_port_fdb_dump(struct dsa_switch *ds, int port,
> +				 dsa_fdb_dump_cb_t *cb, void *data)
> +{
> +	struct lan9303 *chip = ds->priv;
> +	struct port_fdb_dump_ctx dump_ctx = {
> +		.port = port,
> +		.data = data,
> +		.cb   = cb,
> +	};
> +
> +	dev_dbg(chip->dev, "%s(%d)\n", __func__, port);
> +	lan9303_alr_loop(chip, alr_loop_cb_fdb_port_dump, &dump_ctx);
> +	return 0;

A newline before the return statement is welcome.

> +}
> +
>  static const struct dsa_switch_ops lan9303_switch_ops = {
>  	.get_tag_protocol = lan9303_get_tag_protocol,
>  	.setup = lan9303_setup,
> @@ -937,6 +1094,8 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
>  	.port_bridge_join       = lan9303_port_bridge_join,
>  	.port_bridge_leave      = lan9303_port_bridge_leave,
>  	.port_stp_state_set     = lan9303_port_stp_state_set,
> +	.port_fast_age          = lan9303_port_fast_age,
> +	.port_fdb_dump          = lan9303_port_fdb_dump,
>  };
>  
>  static int lan9303_register_switch(struct lan9303 *chip)
> diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
> index 68ecd544b658..4db323d65741 100644
> --- a/drivers/net/dsa/lan9303.h
> +++ b/drivers/net/dsa/lan9303.h
> @@ -11,6 +11,8 @@ struct lan9303_phy_ops {
>  			     int regnum, u16 val);
>  };
>  
> +#define LAN9303_NUM_ALR_RECORDS 512
> +
>  struct lan9303 {
>  	struct device *dev;
>  	struct regmap *regmap;
> -- 
> 2.11.0

Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods

[Andrew Lunn]

> > +/* Clear learned (non-static) entry on given port */
> > +static void alr_loop_cb_del_port_learned(struct lan9303 *chip, u32 dat0,
> > +					 u32 dat1, int portmap, void *ctx)
> > +{
> > +	int *port = ctx;
> 
> You can get the value directly to make the line below more readable:
> 
>     int port = *(int *)ctx;

You have to be a bit careful with this. You often see people
submitting patches taking away casts for void * pointers.
If they do that here, it should at least not compile...

So maybe do it in two steps?

   int * pport = ctx;
   int port = *pport;

???
	Andrew

Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods

[Egil Hjelmeland]

On 19. okt. 2017 15:58, Vivien Didelot wrote:
> Hi Egil,
> 
Hi Vivien

> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> 
>> Add DSA method port_fast_age as a step to STP support.
>>
>> Add low level functions for accessing the lan9303 ALR (Address Logic
>> Resolution).
>>
>> Added DSA method port_fdb_dump
>>
>> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> 
> The patch looks good overall, a few nitpicks though.
> 
>> ---
>>   drivers/net/dsa/lan9303-core.c | 159 +++++++++++++++++++++++++++++++++++++++++
>>   drivers/net/dsa/lan9303.h      |   2 +
>>   2 files changed, 161 insertions(+)
>>
>> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
>> index 09a748327fc6..ae904242b001 100644
>> --- a/drivers/net/dsa/lan9303-core.c
>> +++ b/drivers/net/dsa/lan9303-core.c
>> @@ -124,6 +124,21 @@
>>   #define LAN9303_MAC_RX_CFG_2 0x0c01
>>   #define LAN9303_MAC_TX_CFG_2 0x0c40
>>   #define LAN9303_SWE_ALR_CMD 0x1800
>> +# define ALR_CMD_MAKE_ENTRY    BIT(2)
>> +# define ALR_CMD_GET_FIRST     BIT(1)
>> +# define ALR_CMD_GET_NEXT      BIT(0)
>> +#define LAN9303_SWE_ALR_WR_DAT_0 0x1801
>> +#define LAN9303_SWE_ALR_WR_DAT_1 0x1802
>> +# define ALR_DAT1_VALID        BIT(26)
>> +# define ALR_DAT1_END_OF_TABL  BIT(25)
>> +# define ALR_DAT1_AGE_OVERRID  BIT(25)
>> +# define ALR_DAT1_STATIC       BIT(24)
>> +# define ALR_DAT1_PORT_BITOFFS  16
>> +# define ALR_DAT1_PORT_MASK    (7 << ALR_DAT1_PORT_BITOFFS)
>> +#define LAN9303_SWE_ALR_RD_DAT_0 0x1805
>> +#define LAN9303_SWE_ALR_RD_DAT_1 0x1806
>> +#define LAN9303_SWE_ALR_CMD_STS 0x1808
>> +# define ALR_STS_MAKE_PEND     BIT(0)
> 
> Why is there different spacing and prefix with these defines?

The extra space is to set bit definitions apart from register offsets,
a convention that is used in the file. However, agree that the
bit defs should be prefixed with LAN9303_ to be consistent with
rest of the file.

> 
>>   #define LAN9303_SWE_VLAN_CMD 0x180b
>>   # define LAN9303_SWE_VLAN_CMD_RNW BIT(5)
>>   # define LAN9303_SWE_VLAN_CMD_PVIDNVLAN BIT(4)
>> @@ -478,6 +493,125 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
>>   	return 0;
>>   }
>>   
>> +/* ----------------- Address Logic Resolution (ALR)------------------*/
>> +
>> +/* Map ALR-port bits to port bitmap, and back*/
> 
> The (leading and trailing) spacing in your comments is often
> inconsistent. You can use simple inline or block comments, no need for
> fancy rules. Please refer to Documentation/networking/netdev-FAQ.txt for
> block comment style in netdev though, they are a bit different.
> 

I can add an trailing space to the second comment.

The first comment is sort of "section" comment, so I wanted it to be 
separate. But perhaps I should drop these "section" comments, see 
further below.


>> +static const int alrport_2_portmap[] = {1, 2, 4, 0, 3, 5, 6, 7 };
>> +static const int portmap_2_alrport[] = {3, 0, 1, 4, 2, 5, 6, 7 };
>> +
>> +/* ALR: Actual register access functions */
>> +
>> +/* This function will wait a while until mask & reg == value */
>> +/* Otherwise, return timeout */
> 
> Same, a single block comment will do the job.
> 

See "section comment" comments...

>> +static int lan9303_csr_reg_wait(struct lan9303 *chip, int regno,
>> +				int mask, char value)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < 0x1000; i++) {
>> +		u32 reg;
>> +
>> +		lan9303_read_switch_reg(chip, regno, &reg);
>> +		if ((reg & mask) == value)
>> +			return 0;
>> +		usleep_range(1000, 2000);
>> +	}
>> +	return -ETIMEDOUT;
>> +}
>> +
>> +static int lan9303_alr_make_entry_raw(struct lan9303 *chip, u32 dat0, u32 dat1)
>> +{
>> +	lan9303_write_switch_reg(
>> +		chip, LAN9303_SWE_ALR_WR_DAT_0, dat0);
>> +	lan9303_write_switch_reg(
>> +		chip, LAN9303_SWE_ALR_WR_DAT_1, dat1);
>> +	lan9303_write_switch_reg(
>> +		chip, LAN9303_SWE_ALR_CMD, ALR_CMD_MAKE_ENTRY);
>> +	lan9303_csr_reg_wait(
>> +		chip, LAN9303_SWE_ALR_CMD_STS, ALR_STS_MAKE_PEND, 0);
> 
> As I said in a previous series, please don't do this. Function arguments
> must be vertically aligned with the opening parenthesis.
> 

Will fix

>> +	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
>> +	return 0;
> 
> A newline before a return statement is appreciated.
> 

OK

>> +}
>> +
>> +typedef void alr_loop_cb_t(struct lan9303 *chip, u32 dat0, u32 dat1,
>> +			   int portmap, void *ctx);
>> +
>> +static void lan9303_alr_loop(struct lan9303 *chip, alr_loop_cb_t *cb, void *ctx)
>> +{
>> +	int i;
>> +
>> +	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_FIRST);
>> +	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
>> +
>> +	for (i = 1; i < LAN9303_NUM_ALR_RECORDS; i++) {
>> +		u32 dat0, dat1;
>> +		int alrport, portmap;
>> +
>> +		lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_0, &dat0);
>> +		lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_1, &dat1);
>> +		if (dat1 & ALR_DAT1_END_OF_TABL)
>> +			break;
>> +
>> +		alrport = (dat1 & ALR_DAT1_PORT_MASK) >> ALR_DAT1_PORT_BITOFFS;
>> +		portmap = alrport_2_portmap[alrport];
>> +
>> +		cb(chip, dat0, dat1, portmap, ctx);
>> +
>> +		lan9303_write_switch_reg(
>> +			chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_NEXT);
> 
> Please align arguments with the opening parenthesis.
> 

ok

>> +		lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
>> +	}
>> +}
>> +
>> +/* ALR: lan9303_alr_loop callback functions */
>> +
>  > No need for an extra newline if your comment refers directly to the
> function below. It will also be consistent with the rest of your patch.
> 

The comment refer to several functions...

>> +static void alr_reg_to_mac(u32 dat0, u32 dat1, u8 mac[6])
>> +{
>> +	mac[0] = (dat0 >>  0) & 0xff;
>> +	mac[1] = (dat0 >>  8) & 0xff;
>> +	mac[2] = (dat0 >> 16) & 0xff;
>> +	mac[3] = (dat0 >> 24) & 0xff;
>> +	mac[4] = (dat1 >>  0) & 0xff;
>> +	mac[5] = (dat1 >>  8) & 0xff;
>> +}
>> +
>> +/* Clear learned (non-static) entry on given port */
>> +static void alr_loop_cb_del_port_learned(struct lan9303 *chip, u32 dat0,
>> +					 u32 dat1, int portmap, void *ctx)
>> +{
>> +	int *port = ctx;
> 
> You can get the value directly to make the line below more readable:
> 
>      int port = *(int *)ctx;
> 

Agree

>> +
>> +	if (((BIT(*port) & portmap) == 0) || (dat1 & ALR_DAT1_STATIC))
>> +		return;
>> +
>> +	/* learned entries has only one port, we can just delete */
>> +	dat1 &= ~ALR_DAT1_VALID; /* delete entry */
>> +	lan9303_alr_make_entry_raw(chip, dat0, dat1);
>> +}
>> +
>> +struct port_fdb_dump_ctx {
>> +	int port;
>> +	void *data;
>> +	dsa_fdb_dump_cb_t *cb;
>> +};
>> +
>> +static void alr_loop_cb_fdb_port_dump(struct lan9303 *chip, u32 dat0,
>> +				      u32 dat1, int portmap, void *ctx)
>> +{
>> +	struct port_fdb_dump_ctx *dump_ctx = ctx;
>> +	u8 mac[ETH_ALEN];
>> +	bool is_static;
>> +
>> +	if ((BIT(dump_ctx->port) & portmap) == 0)
>> +		return;
>> +
>> +	alr_reg_to_mac(dat0, dat1, mac);
>> +	is_static = !!(dat1 & ALR_DAT1_STATIC);
>> +	dump_ctx->cb(mac, 0, is_static, dump_ctx->data);
>> +}
>> +
>> +/* --------------------- Various chip setup ----------------------*/
>> +
> 
> This isn't a very useful comment, at least use an inline or block
> comment if you want to keep it.
> 

I put it to signify the end of the ALR section. But could not think of a
better text... Perhaps "End of ALR functions" would be better?
Or perhaps I should just drop the "section comments"? After all the
functions in question has "alr" in their names.

>>   static int lan9303_disable_processing_port(struct lan9303 *chip,
>>   					   unsigned int port)
>>   {
>> @@ -923,6 +1057,29 @@ static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
>>   	/* else: touching SWE_PORT_STATE would break port separation */
>>   }
>>   
>> +static void lan9303_port_fast_age(struct dsa_switch *ds, int port)
>> +{
>> +	struct lan9303 *chip = ds->priv;
>> +
>> +	dev_dbg(chip->dev, "%s(%d)\n", __func__, port);
>> +	lan9303_alr_loop(chip, alr_loop_cb_del_port_learned, &port);
>> +}section
>> +
>> +static int lan9303_port_fdb_dump(struct dsa_switch *ds, int port,
>> +				 dsa_fdb_dump_cb_t *cb, void *data)
>> +{
>> +	struct lan9303 *chip = ds->priv;
>> +	struct port_fdb_dump_ctx dump_ctx = {
>> +		.port = port,
>> +		.data = data,
>> +		.cb   = cb,
>> +	};
>> +
>> +	dev_dbg(chip->dev, "%s(%d)\n", __func__, port);
>> +	lan9303_alr_loop(chip, alr_loop_cb_fdb_port_dump, &dump_ctx);
>> +	return 0;
> 
> A newline before the return statement is welcome.
> 

ok

>> +}
>> +
>>   static const struct dsa_switch_ops lan9303_switch_ops = {
>>   	.get_tag_protocol = lan9303_get_tag_protocol,
>>   	.setup = lan9303_setup,
>> @@ -937,6 +1094,8 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
>>   	.port_bridge_join       = lan9303_port_bridge_join,
>>   	.port_bridge_leave      = lan9303_port_bridge_leave,
>>   	.port_stp_state_set     = lan9303_port_stp_state_set,
>> +	.port_fast_age          = lan9303_port_fast_age,
>> +	.port_fdb_dump          = lan9303_port_fdb_dump,
>>   };
>>   
>>   static int lan9303_register_switch(struct lan9303 *chip)
>> diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
>> index 68ecd544b658..4db323d65741 100644
>> --- a/drivers/net/dsa/lan9303.h
>> +++ b/drivers/net/dsa/lan9303.h
>> @@ -11,6 +11,8 @@ struct lan9303_phy_ops {
>>   			     int regnum, u16 val);
>>   };
>>   
>> +#define LAN9303_NUM_ALR_RECORDS 512
>> +
>>   struct lan9303 {
>>   	struct device *dev;
>>   	struct regmap *regmap;
>> -- 
>> 2.11.0
> 

Thanks
Egil

Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods

[Egil Hjelmeland]

On 19. okt. 2017 16:15, Andrew Lunn wrote:
>>> +/* Clear learned (non-static) entry on given port */
>>> +static void alr_loop_cb_del_port_learned(struct lan9303 *chip, u32 dat0,
>>> +					 u32 dat1, int portmap, void *ctx)
>>> +{
>>> +	int *port = ctx;
>>
>> You can get the value directly to make the line below more readable:
>>
>>      int port = *(int *)ctx;
> 
> You have to be a bit careful with this. You often see people
> submitting patches taking away casts for void * pointers.
> If they do that here, it should at least not compile...
> 
> So maybe do it in two steps?
> 
>     int * pport = ctx;
>     int port = *pport;
> 
> ???
> 	Andrew
> 

Without cast

    int port = *ctx;

... I can not compile. Neither arm-v5te-linux-gnueabi-gcc 4.7.2 or 
native 64bit gcc 6.3.0.

So I feel it is safe to do as Vivien suggest.

Egil

Re: [PATCH v2 net-next 2/2] net: dsa: lan9303: Add fdb/mdb manipulation

[Vivien Didelot]

Hi Egil,

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

> Add functions for managing the lan9303 ALR (Address Logic
> Resolution).
>
> Implement DSA methods: port_fdb_add, port_fdb_del, port_mdb_prepare,
> port_mdb_add and port_mdb_del.
>
> Since the lan9303 do not offer reading specific ALR entry, the driver
> caches all static entries - in a flat table.
>
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

The comments I made in patch 1/2 apply here as well, please consider
them. Other than that, the patch looks good to me:

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods

[Vivien Didelot]

Hi Egil,

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

>>>   #define LAN9303_MAC_RX_CFG_2 0x0c01
>>>   #define LAN9303_MAC_TX_CFG_2 0x0c40
>>>   #define LAN9303_SWE_ALR_CMD 0x1800
>>> +# define ALR_CMD_MAKE_ENTRY    BIT(2)
>>> +# define ALR_CMD_GET_FIRST     BIT(1)
>>> +# define ALR_CMD_GET_NEXT      BIT(0)
>>> +#define LAN9303_SWE_ALR_WR_DAT_0 0x1801
>>> +#define LAN9303_SWE_ALR_WR_DAT_1 0x1802
>>> +# define ALR_DAT1_VALID        BIT(26)
>>> +# define ALR_DAT1_END_OF_TABL  BIT(25)
>>> +# define ALR_DAT1_AGE_OVERRID  BIT(25)
>>> +# define ALR_DAT1_STATIC       BIT(24)
>>> +# define ALR_DAT1_PORT_BITOFFS  16
>>> +# define ALR_DAT1_PORT_MASK    (7 << ALR_DAT1_PORT_BITOFFS)
>>> +#define LAN9303_SWE_ALR_RD_DAT_0 0x1805
>>> +#define LAN9303_SWE_ALR_RD_DAT_1 0x1806
>>> +#define LAN9303_SWE_ALR_CMD_STS 0x1808
>>> +# define ALR_STS_MAKE_PEND     BIT(0)
>> 
>> Why is there different spacing and prefix with these defines?
>
> The extra space is to set bit definitions apart from register offsets,
> a convention that is used in the file. However, agree that the
> bit defs should be prefixed with LAN9303_ to be consistent with
> rest of the file.

OK, I'm fine with this spacing then. The prefix would be nice though,
thanks!

>>> +/* --------------------- Various chip setup ----------------------*/
>>> +
>> 
>> This isn't a very useful comment, at least use an inline or block
>> comment if you want to keep it.
>> 
>
> I put it to signify the end of the ALR section. But could not think of a
> better text... Perhaps "End of ALR functions" would be better?
> Or perhaps I should just drop the "section comments"? After all the
> functions in question has "alr" in their names.

If you cannot think about a comment text which brings value, it
certainly means it isn't necessary. As you said the implicit "alr"
namespace already helps here. I'd personally drop all section comments
;-)

A general thought against comments is that namespaced and readable code
must act as its own documentation. Explicit comments are only necessary
and appreciated for complex portion of code, for example when
performance is chosen over readability.


Thank you,

      Vivien

RE: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods

[David Laight]

From: Andrew Lunn
> Sent: 19 October 2017 15:15
> > > +/* Clear learned (non-static) entry on given port */
> > > +static void alr_loop_cb_del_port_learned(struct lan9303 *chip, u32 dat0,
> > > +					 u32 dat1, int portmap, void *ctx)
> > > +{
> > > +	int *port = ctx;
> >
> > You can get the value directly to make the line below more readable:
> >
> >     int port = *(int *)ctx;
> 
> You have to be a bit careful with this. You often see people
> submitting patches taking away casts for void * pointers.
> If they do that here, it should at least not compile...
> 
> So maybe do it in two steps?
> 
>    int * pport = ctx;
>    int port = *pport;

IMHO it is best to define a struct for the 'ctx and then do:
	..., void *v_ctx)
{
	foo_ctx *ctx = v_ctx;
	int port = ctx->port;

That stops anyone having to double-check that the *(int *)
is operating on a pointer to an integer of the correct size.

One of the syntax checkers probably ought to generate a warning
for *(integer_type *)foo since it is often a bug.

	David

Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods

[Egil Hjelmeland]

On 19. okt. 2017 17:12, Vivien Didelot wrote:
> Hi Egil,
> 
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> 
>>> Why is there different spacing and prefix with these defines?
>>
>> The extra space is to set bit definitions apart from register offsets,
>> a convention that is used in the file. However, agree that the
>> bit defs should be prefixed with LAN9303_ to be consistent with
>> rest of the file.
> 
> OK, I'm fine with this spacing then. The prefix would be nice though,
> thanks!
> 
Prefix already done in my working version.

> 
> If you cannot think about a comment text which brings value, it
> certainly means it isn't necessary. As you said the implicit "alr"
> namespace already helps here. I'd personally drop all section comments
> ;-)
> 

Then I will just drop the section comments.

> 
> Thank you,
> 
>        Vivien
> 

Egil

Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods

[Egil Hjelmeland]

On 19. okt. 2017 17:15, David Laight wrote:
> From: Andrew Lunn
>> Sent: 19 October 2017 15:15
>>>> +/* Clear learned (non-static) entry on given port */
>>>> +static void alr_loop_cb_del_port_learned(struct lan9303 *chip, u32 dat0,
>>>> +					 u32 dat1, int portmap, void *ctx)
>>>> +{
>>>> +	int *port = ctx;
>>>
>>> You can get the value directly to make the line below more readable:
>>>
>>>      int port = *(int *)ctx;
>>
>> You have to be a bit careful with this. You often see people
>> submitting patches taking away casts for void * pointers.
>> If they do that here, it should at least not compile...
>>
>> So maybe do it in two steps?
>>
>>     int * pport = ctx;
>>     int port = *pport;
> 
> IMHO it is best to define a struct for the 'ctx and then do:
> 	..., void *v_ctx)
> {
> 	foo_ctx *ctx = v_ctx;
> 	int port = ctx->port;
> 
> That stops anyone having to double-check that the *(int *)
> is operating on a pointer to an integer of the correct size.
> 

Does casting to a struct pointer require less manual double-check than
to a int-pointer? In neither cases the compiler can protect us, IFAIK.
But on the other hand, a the text "foo_ctx" can searched in the editor.
So in that respect it can somewhat aid to the double-checking.

So I can do that.


> One of the syntax checkers probably ought to generate a warning
> for *(integer_type *)foo since it is often a bug.
> 
> 	David
> 
> 

Egil

Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods

[Egil Hjelmeland]

Den 19. okt. 2017 17:42, skrev Egil Hjelmeland:
> On 19. okt. 2017 17:15, David Laight wrote:
>> From: Andrew Lunn
>>> Sent: 19 October 2017 15:15
>>>>> +/* Clear learned (non-static) entry on given port */
>>>>> +static void alr_loop_cb_del_port_learned(struct lan9303 *chip, u32 
>>>>> dat0,
>>>>> +                     u32 dat1, int portmap, void *ctx)
>>>>> +{
>>>>> +    int *port = ctx;
>>>>
>>>> You can get the value directly to make the line below more readable:
>>>>
>>>>      int port = *(int *)ctx;
>>>
>>> You have to be a bit careful with this. You often see people
>>> submitting patches taking away casts for void * pointers.
>>> If they do that here, it should at least not compile...
>>>
>>> So maybe do it in two steps?
>>>
>>>     int * pport = ctx;
>>>     int port = *pport;
>>
>> IMHO it is best to define a struct for the 'ctx and then do:
>>     ..., void *v_ctx)
>> {
>>     foo_ctx *ctx = v_ctx;
>>     int port = ctx->port;
>>
>> That stops anyone having to double-check that the *(int *)
>> is operating on a pointer to an integer of the correct size.
>>
> 
> Does casting to a struct pointer require less manual double-check than
> to a int-pointer? In neither cases the compiler can protect us, IFAIK.
> But on the other hand, a the text "foo_ctx" can searched in the editor.
> So in that respect it can somewhat aid to the double-checking.
> 
> So I can do that.
> 
> 

I understand now that the caller side (lan9303_port_fast_age) is
vulnerable. Say somebody has the idea to change the "port" param
of .port_fast_age from int to u8, then my code is a trap.

Thanks for the education.

Egil

RE: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods

[David Laight]

From: Egil Hjelmeland
> Sent: 19 October 2017 17:53
...
> >> IMHO it is best to define a struct for the 'ctx and then do:
> >>     ..., void *v_ctx)
> >> {
> >>     foo_ctx *ctx = v_ctx;
> >>     int port = ctx->port;
> >>
> >> That stops anyone having to double-check that the *(int *)
> >> is operating on a pointer to an integer of the correct size.
> >>
> >
> > Does casting to a struct pointer require less manual double-check than
> > to a int-pointer? In neither cases the compiler can protect us, IFAIK.
> > But on the other hand, a the text "foo_ctx" can searched in the editor.
> > So in that respect it can somewhat aid to the double-checking.
> >
> > So I can do that.
> >
> >
> 
> I understand now that the caller side (lan9303_port_fast_age) is
> vulnerable. Say somebody has the idea to change the "port" param
> of .port_fast_age from int to u8, then my code is a trap.

Worse, change it to a long and it will work on everything except
64bit big-endian systems.

	David