| Message ID | 20171019121015.7395-2-privat@egil-hjelmeland.no |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: lan9303: Add fdb/mdb methods | expand |
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, ®); > + 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
> > +/* 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
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, ®); >> + 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
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
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
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
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
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
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
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
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, ®); + 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;
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(+)