Re: [PATCH 0/2] net: dsa: lan9303: check error value from devm_gpiod_get_optional()

[Egil Hjelmeland]

Den 13. nov. 2017 09:07, skrev Phil Reid:

Replaces Pan Bian  patch
"net: dsa: lan9303: correctly check return value of devm_gpiod_get_optional"

Errors need to be prograted back from probe.

Note: I have only compile tested the code as I don't have the hardware.

Phil Reid (2):
   net: dsa: lan9303: make lan9303_handle_reset() a void function
   net: dsa: lan9303: check error value from devm_gpiod_get_optional()

  drivers/net/dsa/lan9303-core.c | 21 -
  1 file changed, 12 insertions(+), 9 deletions(-)



Evidently this one did not make it through. Do you care to rebase and 
try again?


Thanks
Egil

Re: [PATCH net-next] net: dsa: lan9303: Fix error return code in lan9303_check_device()

[Egil Hjelmeland]

Den 04. jan. 2018 08:30, skrev Wei Yongjun:

Fix to return error code -ENODEV from the chip not found error handling
case instead of 0(ret have been overwritten to 0 by lan9303_read()), as
done elsewhere in this function.

Signed-off-by: Wei Yongjun <weiyongj...@huawei.com>
---


Reviewed-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>

Egil

Re: net-next lan9303 and CONFIG_NET_DSA_LEGACY

[Egil Hjelmeland]

On 15. des. 2017 21:06, Florian Fainelli wrote:

On December 15, 2017 6:51:45 AM PST, Egil Hjelmeland 
<pri...@egil-hjelmeland.no> wrote:

Hi
I found that our lan9303 board is unable to receive network data unless

CONFIG_NET_DSA_LEGACY is set. Is that a problem with the driver, or our

Any advise would be appreciated.


Your DTS appears sane and using the new binding. Is the switch driver 
successfully probing and it is just packet transmission/reception that is non 
functional?
Hi Egil,



Hi Florian

I found that the problem is that !CONFIG_NET_DSA_LEGACY version of 
dsa_legacy_register() return -ENODEV;


That makes dsa_init_module() drop out before 
"dev_add_pack(_pack_type)", and we don't get any handler for ETH_P_XDSA.


Changed dsa_legacy_register to return 0, and it works.

Egil

[PATCH net-next 1/2] net: dsa: lan9303: phy_addr_sel_strap rename and retype

[Egil Hjelmeland]

chip->phy_addr_sel_strap is declared as a bool, but is also used as an
integer address base.

Rename 'phy_addr_sel_strap' to 'phy_addr_base', and change type to int.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 20 ++--
 include/linux/dsa/lan9303.h|  2 +-
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 944901f03f8b..3088cdc5d205 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -479,7 +479,8 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
 {
int reg;
 
-   /* depending on the 'phy_addr_sel_strap' setting, the three phys are
+   /* Calculate chip->phy_addr_base:
+* Depending on the 'phy_addr_sel_strap' setting, the three phys are
 * using IDs 0-1-2 or IDs 1-2-3. We cannot read back the
 * 'phy_addr_sel_strap' setting directly, so we need a test, which
 * configuration is active:
@@ -495,12 +496,12 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
}
 
if ((reg != 0) && (reg != 0x))
-   chip->phy_addr_sel_strap = 1;
+   chip->phy_addr_base = 1;
else
-   chip->phy_addr_sel_strap = 0;
+   chip->phy_addr_base = 0;
 
dev_dbg(chip->dev, "Phy setup '%s' detected\n",
-   chip->phy_addr_sel_strap ? "1-2-3" : "0-1-2");
+   chip->phy_addr_base ? "1-2-3" : "0-1-2");
 
return 0;
 }
@@ -1019,7 +1020,7 @@ static int lan9303_get_sset_count(struct dsa_switch *ds)
 static int lan9303_phy_read(struct dsa_switch *ds, int phy, int regnum)
 {
struct lan9303 *chip = ds->priv;
-   int phy_base = chip->phy_addr_sel_strap;
+   int phy_base = chip->phy_addr_base;
 
if (phy == phy_base)
return lan9303_virt_phy_reg_read(chip, regnum);
@@ -1033,7 +1034,7 @@ static int lan9303_phy_write(struct dsa_switch *ds, int 
phy, int regnum,
 u16 val)
 {
struct lan9303 *chip = ds->priv;
-   int phy_base = chip->phy_addr_sel_strap;
+   int phy_base = chip->phy_addr_base;
 
if (phy == phy_base)
return lan9303_virt_phy_reg_write(chip, regnum, val);
@@ -1070,7 +1071,7 @@ static void lan9303_adjust_link(struct dsa_switch *ds, 
int port,
 
res =  lan9303_phy_write(ds, port, MII_BMCR, ctl);
 
-   if (port == chip->phy_addr_sel_strap) {
+   if (port == chip->phy_addr_base) {
/* Virtual Phy: Remove Turbo 200Mbit mode */
lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, );
 
@@ -1094,8 +1095,7 @@ static void lan9303_port_disable(struct dsa_switch *ds, 
int port,
struct lan9303 *chip = ds->priv;
 
lan9303_disable_processing_port(chip, port);
-   lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
- MII_BMCR, BMCR_PDOWN);
+   lan9303_phy_write(ds, chip->phy_addr_base + port, MII_BMCR, BMCR_PDOWN);
 }
 
 static int lan9303_port_bridge_join(struct dsa_switch *ds, int port,
@@ -1289,7 +1289,7 @@ static int lan9303_register_switch(struct lan9303 *chip)
 
chip->ds->priv = chip;
chip->ds->ops = _switch_ops;
-   chip->ds->phys_mii_mask = chip->phy_addr_sel_strap ? 0xe : 0x7;
+   chip->ds->phys_mii_mask = chip->phy_addr_base ? 0xe : 0x7;
 
return dsa_register_switch(chip->ds);
 }
diff --git a/include/linux/dsa/lan9303.h b/include/linux/dsa/lan9303.h
index b6514c29563f..b4f22112ba75 100644
--- a/include/linux/dsa/lan9303.h
+++ b/include/linux/dsa/lan9303.h
@@ -23,7 +23,7 @@ struct lan9303 {
struct regmap_irq_chip_data *irq_data;
struct gpio_desc *reset_gpio;
u32 reset_duration; /* in [ms] */
-   bool phy_addr_sel_strap;
+   int phy_addr_base;
struct dsa_switch *ds;
struct mutex indirect_mutex; /* protect indexed register access */
struct mutex alr_mutex; /* protect ALR access */
-- 
2.14.1

[PATCH net-next 2/2] net: dsa: lan9303: Adjust phy_addr_base expressions

[Egil Hjelmeland]

Simplify calculation of chip->phy_addr_base in lan9303_detect_phy_setup().

Use GENMASK to calculate phys_mii_mask from LAN9303_NUM_PORTS and
phy_addr_base.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 3088cdc5d205..4efb772dbc7e 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -495,10 +495,7 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
return reg;
}
 
-   if ((reg != 0) && (reg != 0x))
-   chip->phy_addr_base = 1;
-   else
-   chip->phy_addr_base = 0;
+   chip->phy_addr_base = reg != 0 && reg != 0x;
 
dev_dbg(chip->dev, "Phy setup '%s' detected\n",
chip->phy_addr_base ? "1-2-3" : "0-1-2");
@@ -1283,13 +1280,16 @@ static const struct dsa_switch_ops lan9303_switch_ops = 
{
 
 static int lan9303_register_switch(struct lan9303 *chip)
 {
+   int base;
+
chip->ds = dsa_switch_alloc(chip->dev, LAN9303_NUM_PORTS);
if (!chip->ds)
return -ENOMEM;
 
chip->ds->priv = chip;
chip->ds->ops = _switch_ops;
-   chip->ds->phys_mii_mask = chip->phy_addr_base ? 0xe : 0x7;
+   base = chip->phy_addr_base;
+   chip->ds->phys_mii_mask = GENMASK(LAN9303_NUM_PORTS - 1 + base, base);
 
return dsa_register_switch(chip->ds);
 }
-- 
2.14.1

[PATCH net-next 0/2] net: dsa: lan9303: phy_addr_sel_strap rename and retype

[Egil Hjelmeland]

Non functional cleanups involving chip->phy_addr_sel_strap.
As promised in https://lkml.org/lkml/2017/11/6/273 

Egil Hjelmeland (2):
  net: dsa: lan9303: phy_addr_sel_strap rename and retype
  net: dsa: lan9303: Adjust phy_addr_base expressions

 drivers/net/dsa/lan9303-core.c | 24 
 include/linux/dsa/lan9303.h|  2 +-
 2 files changed, 13 insertions(+), 13 deletions(-)

-- 
2.14.1

[PATCH v2 net-next] net: dsa: lan9303: lan9303_csr_reg_wait cleanups

[Egil Hjelmeland]

Non-functional cleanups in lan9303_csr_reg_wait():
 - Change type of param 'mask' from int to u32.
 - Remove param 'value' (will probably never be used)
 - Reduced retries from 1000 to 25, consistent with lan9303_read_wait.
 - Removed comments

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>

Changes v1 -> v2:
 - Removed comments
---
 drivers/net/dsa/lan9303-core.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index f412aad58253..944901f03f8b 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -249,7 +249,6 @@ static int lan9303_read(struct regmap *regmap, unsigned int 
offset, u32 *reg)
return -EIO;
 }
 
-/* Wait a while until mask & reg == value. Otherwise return timeout. */
 static int lan9303_read_wait(struct lan9303 *chip, int offset, u32 mask)
 {
int i;
@@ -541,20 +540,19 @@ lan9303_alr_cache_find_mac(struct lan9303 *chip, const u8 
*mac_addr)
return NULL;
 }
 
-/* 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)
+static int lan9303_csr_reg_wait(struct lan9303 *chip, int regno, u32 mask)
 {
int i;
 
-   for (i = 0; i < 0x1000; i++) {
+   for (i = 0; i < 25; i++) {
u32 reg;
 
lan9303_read_switch_reg(chip, regno, );
-   if ((reg & mask) == value)
+   if (!(reg & mask))
return 0;
usleep_range(1000, 2000);
}
+
return -ETIMEDOUT;
 }
 
@@ -564,8 +562,7 @@ static int lan9303_alr_make_entry_raw(struct lan9303 *chip, 
u32 dat0, u32 dat1)
lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_WR_DAT_1, dat1);
lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD,
 LAN9303_ALR_CMD_MAKE_ENTRY);
-   lan9303_csr_reg_wait(chip, LAN9303_SWE_ALR_CMD_STS, ALR_STS_MAKE_PEND,
-0);
+   lan9303_csr_reg_wait(chip, LAN9303_SWE_ALR_CMD_STS, ALR_STS_MAKE_PEND);
lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
 
return 0;
-- 
2.14.1

Re: net-next lan9303 and CONFIG_NET_DSA_LEGACY

[Egil Hjelmeland]

Den 15. des. 2017 21:06, skrev Florian Fainelli:

On December 15, 2017 6:51:45 AM PST, Egil Hjelmeland 
<pri...@egil-hjelmeland.no> wrote:

Hi
I found that our lan9303 board is unable to receive network data unless

CONFIG_NET_DSA_LEGACY is set. Is that a problem with the driver, or our

DTS?  (imx28)



Your DTS appears sane and using the new binding. Is the switch driver 
successfully probing and it is just packet transmission/reception that is non 
functional?


Hi Florian

Yes, it seems it probes ok. The netdevs are made, and the userports goes 
to "forwarding" in the log. But no packet transmission/reception. 
"ifconfig" says that all packets on the CPU side port (FEC) are 
discarded. Otherwise I did not do any debugging. I decided to start by 
having the DTS checked. I guess I will do some more debugging next week.


Thanks a lot
Egil

Re: [RFT/RFC net-next 0/2] net: dsa: Plug in PHYLINK support

[Egil Hjelmeland]

Hi


Den 15. des. 2017 01:28, skrev Florian Fainelli:

Hi all,

This patch series replaces the existing PHYLIB integration with PHYLINK which
is a superior solution since we need to support a collection of fixed links,
integrated PHYs, SFP, non-pluggable SFPs etc.

I am expecting quite a lot of breakage, for a number of reasons:

- PHYLINK does not create a PHY device for fixed links (MLO_AN_FIXED), which
   means that user-facing port (not DSA, not CPU) need to be explicitly handled
   with phylink_mac_ops now



FWIW:  We (zenitel) does not use fixed-link on user-port. I gave the 
patches a spin on our board with lan9303. As expected: no breakage.


Egil

net-next lan9303 and CONFIG_NET_DSA_LEGACY

[Egil Hjelmeland]

Hi
I found that our lan9303 board is unable to receive network data unless 
CONFIG_NET_DSA_LEGACY is set. Is that a problem with the driver, or our 
DTS?  (imx28)


   ahb@8008 {
  mac0: ethernet@800f {
 clocks = < 57>, < 57>, < 64>;
 clock-names = "ipg", "ahb", "enet_out";
 phy-mode = "rmii";
 pinctrl-names = "default";
 pinctrl-0 = <_pins_a>;
 status = "okay";

 fixed-link {
speed = <100>;
full-duplex;
 };

 mdio {
#address-cells = <1>;
#size-cells = <0>;

switch: switch-phy@0 {
   compatible = "smsc,lan9303-mdio";
   reg = <0>;
   reset-gpios = < 7 GPIO_ACTIVE_LOW >;
   reset-duration = <10>;
   ports {
  #address-cells = <1>;
  #size-cells = <0>;

  port@0 {
 reg = <0>;
 label = "cpu";
 ethernet = <>;
 phy-mode = "rmii";
 fixed-link {
speed = <100>;
full-duplex;
 };
  };

  port@1 { /* external port 1 */
 reg = <1>;
 label = "sw1";
 phy-mode = "rmii";
  };

  port@2 { /* external port 2 */
 reg = <2>;
 label = "sw2";
 phy-mode = "rmii";
  };
   };
};
 };
  };
   };

Any advise would be appreciated.

Egil

[PATCH net-next] net: dsa: lan9303: lan9303_csr_reg_wait cleanups

[Egil Hjelmeland]

Non-functional cleanups in lan9303_csr_reg_wait():
 - Change type of param 'mask' from int to u32.
 - Remove param 'value' (will probably never be used)
 - Reduced retries from 1000 to 25, consistent with lan9303_read_wait.
 - Corrected comments

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index f412aad58253..209882075e3b 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -249,7 +249,7 @@ static int lan9303_read(struct regmap *regmap, unsigned int 
offset, u32 *reg)
return -EIO;
 }
 
-/* Wait a while until mask & reg == value. Otherwise return timeout. */
+/* Wait a while until mask & reg == 0. Otherwise return timeout. */
 static int lan9303_read_wait(struct lan9303 *chip, int offset, u32 mask)
 {
int i;
@@ -541,20 +541,20 @@ lan9303_alr_cache_find_mac(struct lan9303 *chip, const u8 
*mac_addr)
return NULL;
 }
 
-/* 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)
+/* Wait a while until mask & reg == 0. Otherwise return timeout. */
+static int lan9303_csr_reg_wait(struct lan9303 *chip, int regno, u32 mask)
 {
int i;
 
-   for (i = 0; i < 0x1000; i++) {
+   for (i = 0; i < 25; i++) {
u32 reg;
 
lan9303_read_switch_reg(chip, regno, );
-   if ((reg & mask) == value)
+   if (!(reg & mask))
return 0;
usleep_range(1000, 2000);
}
+
return -ETIMEDOUT;
 }
 
@@ -564,8 +564,7 @@ static int lan9303_alr_make_entry_raw(struct lan9303 *chip, 
u32 dat0, u32 dat1)
lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_WR_DAT_1, dat1);
lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD,
 LAN9303_ALR_CMD_MAKE_ENTRY);
-   lan9303_csr_reg_wait(chip, LAN9303_SWE_ALR_CMD_STS, ALR_STS_MAKE_PEND,
-0);
+   lan9303_csr_reg_wait(chip, LAN9303_SWE_ALR_CMD_STS, ALR_STS_MAKE_PEND);
lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
 
return 0;
-- 
2.14.1

[PATCH v2 net-next] net: dsa: lan9303: Introduce lan9303_read_wait

[Egil Hjelmeland]

Simplify lan9303_indirect_phy_wait_for_completion()
and lan9303_switch_wait_for_completion() by using a new function
lan9303_read_wait()

Changes v1 -> v2:
 - param 'mask' type u32
 - removed param 'value' (will probably never be used)
 - add newline before return

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 59 +++---
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index c1b004fa64d9..f412aad58253 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -249,6 +249,29 @@ static int lan9303_read(struct regmap *regmap, unsigned 
int offset, u32 *reg)
return -EIO;
 }
 
+/* Wait a while until mask & reg == value. Otherwise return timeout. */
+static int lan9303_read_wait(struct lan9303 *chip, int offset, u32 mask)
+{
+   int i;
+
+   for (i = 0; i < 25; i++) {
+   u32 reg;
+   int ret;
+
+   ret = lan9303_read(chip->regmap, offset, );
+   if (ret) {
+   dev_err(chip->dev, "%s failed to read offset %d: %d\n",
+   __func__, offset, ret);
+   return ret;
+   }
+   if (!(reg & mask))
+   return 0;
+   usleep_range(1000, 2000);
+   }
+
+   return -ETIMEDOUT;
+}
+
 static int lan9303_virt_phy_reg_read(struct lan9303 *chip, int regnum)
 {
int ret;
@@ -274,22 +297,8 @@ static int lan9303_virt_phy_reg_write(struct lan9303 
*chip, int regnum, u16 val)
 
 static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)
 {
-   int ret, i;
-   u32 reg;
-
-   for (i = 0; i < 25; i++) {
-   ret = lan9303_read(chip->regmap, LAN9303_PMI_ACCESS, );
-   if (ret) {
-   dev_err(chip->dev,
-   "Failed to read pmi access status: %d\n", ret);
-   return ret;
-   }
-   if (!(reg & LAN9303_PMI_ACCESS_MII_BUSY))
-   return 0;
-   usleep_range(1000, 2000);
-   }
-
-   return -EIO;
+   return lan9303_read_wait(chip, LAN9303_PMI_ACCESS,
+LAN9303_PMI_ACCESS_MII_BUSY);
 }
 
 static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int 
regnum)
@@ -366,22 +375,8 @@ EXPORT_SYMBOL_GPL(lan9303_indirect_phy_ops);
 
 static int lan9303_switch_wait_for_completion(struct lan9303 *chip)
 {
-   int ret, i;
-   u32 reg;
-
-   for (i = 0; i < 25; i++) {
-   ret = lan9303_read(chip->regmap, LAN9303_SWITCH_CSR_CMD, );
-   if (ret) {
-   dev_err(chip->dev,
-   "Failed to read csr command status: %d\n", ret);
-   return ret;
-   }
-   if (!(reg & LAN9303_SWITCH_CSR_CMD_BUSY))
-   return 0;
-   usleep_range(1000, 2000);
-   }
-
-   return -EIO;
+   return lan9303_read_wait(chip, LAN9303_SWITCH_CSR_CMD,
+LAN9303_SWITCH_CSR_CMD_BUSY);
 }
 
 static int lan9303_write_switch_reg(struct lan9303 *chip, u16 regnum, u32 val)
-- 
2.14.1

Re: [PATCH net-next] net: dsa: lan9303: Introduce lan9303_read_wait

[Egil Hjelmeland]

Hi Vivien.

Den 12. des. 2017 19:08, skrev Vivien Didelot:

Hi Egil,

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


Simplify lan9303_indirect_phy_wait_for_completion()
and lan9303_switch_wait_for_completion() by using a new function
lan9303_read_wait()

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
  drivers/net/dsa/lan9303-core.c | 59 +++---
  1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index c1b004fa64d9..96ccce0939d3 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -249,6 +249,29 @@ static int lan9303_read(struct regmap *regmap, unsigned 
int offset, u32 *reg)
return -EIO;
  }
  
+/* Wait a while until mask & reg == value. Otherwise return timeout. */

+static int lan9303_read_wait(struct lan9303 *chip, int offset, int mask,
+char value)
+{
+   int i;
+
+   for (i = 0; i < 25; i++) {
+   u32 reg;
+   int ret;
+
+   ret = lan9303_read(chip->regmap, offset, );
+   if (ret) {
+   dev_err(chip->dev, "%s failed to read offset %d: %d\n",
+   __func__, offset, ret);
+   return ret;
+   }
+   if ((reg & mask) == value)
+   return 0;


That is weird to mix int, u32 and char for mask checking. I suggest you
to use the u32 type as well for both mask and value.



Good catch. Will fix that. Same with lan9303_csr_reg_wait() then.



Looking at how lan9303_read_wait is called, the value argument doesn't
seem necessary. You can directly return 0 if (!(reg & mask)).



The idea was to make in more general usable, in case one need to wait 
for a bit to be set. But I don't have any example from the datasheet 
that needs it, so I could take "value" away.



+   usleep_range(1000, 2000);
+   }
+   return -ETIMEDOUT;


A newline before the return statment would be appreciated.


Ok.


+}
+
  static int lan9303_virt_phy_reg_read(struct lan9303 *chip, int regnum)
  {
int ret;
@@ -274,22 +297,8 @@ static int lan9303_virt_phy_reg_write(struct lan9303 
*chip, int regnum, u16 val)
  
  static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)

  {
-   int ret, i;
-   u32 reg;
-
-   for (i = 0; i < 25; i++) {
-   ret = lan9303_read(chip->regmap, LAN9303_PMI_ACCESS, );
-   if (ret) {
-   dev_err(chip->dev,
-   "Failed to read pmi access status: %d\n", ret);
-   return ret;
-   }
-   if (!(reg & LAN9303_PMI_ACCESS_MII_BUSY))
-   return 0;
-   usleep_range(1000, 2000);
-   }
-
-   return -EIO;
+   return lan9303_read_wait(chip, LAN9303_PMI_ACCESS,
+LAN9303_PMI_ACCESS_MII_BUSY, 0);
  }
  
  static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int regnum)

@@ -366,22 +375,8 @@ EXPORT_SYMBOL_GPL(lan9303_indirect_phy_ops);
  
  static int lan9303_switch_wait_for_completion(struct lan9303 *chip)

  {
-   int ret, i;
-   u32 reg;
-
-   for (i = 0; i < 25; i++) {
-   ret = lan9303_read(chip->regmap, LAN9303_SWITCH_CSR_CMD, );
-   if (ret) {
-   dev_err(chip->dev,
-   "Failed to read csr command status: %d\n", ret);
-   return ret;
-   }
-   if (!(reg & LAN9303_SWITCH_CSR_CMD_BUSY))
-   return 0;
-   usleep_range(1000, 2000);
-   }
-
-   return -EIO;
+   return lan9303_read_wait(chip, LAN9303_SWITCH_CSR_CMD,
+LAN9303_SWITCH_CSR_CMD_BUSY, 0);
  }
  
  static int lan9303_write_switch_reg(struct lan9303 *chip, u16 regnum, u32 val)



Thanks,

 Vivien



Thanks, Egil

[PATCH net-next] net: dsa: lan9303: Introduce lan9303_read_wait

[Egil Hjelmeland]

Simplify lan9303_indirect_phy_wait_for_completion()
and lan9303_switch_wait_for_completion() by using a new function
lan9303_read_wait()

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 59 +++---
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index c1b004fa64d9..96ccce0939d3 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -249,6 +249,29 @@ static int lan9303_read(struct regmap *regmap, unsigned 
int offset, u32 *reg)
return -EIO;
 }
 
+/* Wait a while until mask & reg == value. Otherwise return timeout. */
+static int lan9303_read_wait(struct lan9303 *chip, int offset, int mask,
+char value)
+{
+   int i;
+
+   for (i = 0; i < 25; i++) {
+   u32 reg;
+   int ret;
+
+   ret = lan9303_read(chip->regmap, offset, );
+   if (ret) {
+   dev_err(chip->dev, "%s failed to read offset %d: %d\n",
+   __func__, offset, ret);
+   return ret;
+   }
+   if ((reg & mask) == value)
+   return 0;
+   usleep_range(1000, 2000);
+   }
+   return -ETIMEDOUT;
+}
+
 static int lan9303_virt_phy_reg_read(struct lan9303 *chip, int regnum)
 {
int ret;
@@ -274,22 +297,8 @@ static int lan9303_virt_phy_reg_write(struct lan9303 
*chip, int regnum, u16 val)
 
 static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)
 {
-   int ret, i;
-   u32 reg;
-
-   for (i = 0; i < 25; i++) {
-   ret = lan9303_read(chip->regmap, LAN9303_PMI_ACCESS, );
-   if (ret) {
-   dev_err(chip->dev,
-   "Failed to read pmi access status: %d\n", ret);
-   return ret;
-   }
-   if (!(reg & LAN9303_PMI_ACCESS_MII_BUSY))
-   return 0;
-   usleep_range(1000, 2000);
-   }
-
-   return -EIO;
+   return lan9303_read_wait(chip, LAN9303_PMI_ACCESS,
+LAN9303_PMI_ACCESS_MII_BUSY, 0);
 }
 
 static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int 
regnum)
@@ -366,22 +375,8 @@ EXPORT_SYMBOL_GPL(lan9303_indirect_phy_ops);
 
 static int lan9303_switch_wait_for_completion(struct lan9303 *chip)
 {
-   int ret, i;
-   u32 reg;
-
-   for (i = 0; i < 25; i++) {
-   ret = lan9303_read(chip->regmap, LAN9303_SWITCH_CSR_CMD, );
-   if (ret) {
-   dev_err(chip->dev,
-   "Failed to read csr command status: %d\n", ret);
-   return ret;
-   }
-   if (!(reg & LAN9303_SWITCH_CSR_CMD_BUSY))
-   return 0;
-   usleep_range(1000, 2000);
-   }
-
-   return -EIO;
+   return lan9303_read_wait(chip, LAN9303_SWITCH_CSR_CMD,
+LAN9303_SWITCH_CSR_CMD_BUSY, 0);
 }
 
 static int lan9303_write_switch_reg(struct lan9303 *chip, u16 regnum, u32 val)
-- 
2.14.1

[PATCH net-next] net: dsa: lan9303: Protect ALR operations with mutex

[Egil Hjelmeland]

ALR table operations are a sequence of related register operations which
should be protected from concurrent access. The alr_cache should also be
protected. Add alr_mutex doing that.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 14 --
 include/linux/dsa/lan9303.h|  1 +
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index ea59dadefb33..c1b004fa64d9 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -583,6 +583,7 @@ static void lan9303_alr_loop(struct lan9303 *chip, 
alr_loop_cb_t *cb, void *ctx)
 {
int i;
 
+   mutex_lock(>alr_mutex);
lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD,
 LAN9303_ALR_CMD_GET_FIRST);
lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
@@ -606,6 +607,7 @@ static void lan9303_alr_loop(struct lan9303 *chip, 
alr_loop_cb_t *cb, void *ctx)
 LAN9303_ALR_CMD_GET_NEXT);
lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
}
+   mutex_unlock(>alr_mutex);
 }
 
 static void alr_reg_to_mac(u32 dat0, u32 dat1, u8 mac[6])
@@ -694,16 +696,20 @@ static int lan9303_alr_add_port(struct lan9303 *chip, 
const u8 *mac, int port,
 {
struct lan9303_alr_cache_entry *entr;
 
+   mutex_lock(>alr_mutex);
entr = lan9303_alr_cache_find_mac(chip, mac);
if (!entr) { /*New entry */
entr = lan9303_alr_cache_find_free(chip);
-   if (!entr)
+   if (!entr) {
+   mutex_unlock(>alr_mutex);
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);
+   mutex_unlock(>alr_mutex);
 
return 0;
 }
@@ -713,15 +719,18 @@ static int lan9303_alr_del_port(struct lan9303 *chip, 
const u8 *mac, int port)
 {
struct lan9303_alr_cache_entry *entr;
 
+   mutex_lock(>alr_mutex);
entr = lan9303_alr_cache_find_mac(chip, mac);
if (!entr)
-   return 0;  /* no static entry found */
+   goto out;  /* no static entry found */
 
entr->port_map &= ~BIT(port);
if (entr->port_map == 0) /* zero means its free again */
eth_zero_addr(entr->mac_addr);
lan9303_alr_set_entry(chip, mac, entr->port_map, entr->stp_override);
 
+out:
+   mutex_unlock(>alr_mutex);
return 0;
 }
 
@@ -1323,6 +1332,7 @@ int lan9303_probe(struct lan9303 *chip, struct 
device_node *np)
int ret;
 
mutex_init(>indirect_mutex);
+   mutex_init(>alr_mutex);
 
lan9303_probe_reset_gpio(chip, np);
 
diff --git a/include/linux/dsa/lan9303.h b/include/linux/dsa/lan9303.h
index f48a85c377de..b6514c29563f 100644
--- a/include/linux/dsa/lan9303.h
+++ b/include/linux/dsa/lan9303.h
@@ -26,6 +26,7 @@ struct lan9303 {
bool phy_addr_sel_strap;
struct dsa_switch *ds;
struct mutex indirect_mutex; /* protect indexed register access */
+   struct mutex alr_mutex; /* protect ALR access */
const struct lan9303_phy_ops *ops;
bool is_bridged; /* true if port 1 and 2 are bridged */
 
-- 
2.14.1

Re: question lan9303: DSA concurrency and locking,

[Egil Hjelmeland]

On 15. nov. 2017 15:08, Andrew Lunn wrote:

On Wed, Nov 15, 2017 at 01:08:22PM +0100, Egil Hjelmeland wrote:

Hi experts



Hi, thanks for response, both Andrew and Vivien!


I am hoping for some guidance.

Does DSA offer any protection against concurrent calls of dsa_switch_ops?


Hi Egil

DSA itself does not.

There are various upper locks, which protect some calls, in some ways.
e.g. phy ops are protected by the mdio lock. stats calls are protected
by the rtnl lock, as well as some other calls. And other locks protect
other things.

But nothing gives you protection across them all.

For the mv88e6xxx driver, we took the simple approach. We generally
take a lock at the beginning of each of the dsa_swtich_ops functions,
and release it at the end. Since all accesses to the chip go through
two read/write functions, we also have code in them to detect when
they are called without holding the lock.

Some driver writers worry about performance in some situations, and
want finer grain locking. So they have multiple locks. When reviewing
drivers i will look for obvious locking issues, but don't look too
deeply. Without knowing the chip, it has hard for me to know if
something is safe or not. So i would not be surprised if there are
locking issues in some drivers.


The most "interesting" part of the lan9303 driver that has no locking is the
ALR (=fdb/mdb). ALR access is a sequence of register operations. Anyway it
is very unlikely that mdb related calls are reentered. But if it can happen,
it would mean that IGMP snooping can go wrong. (Which is actually very bad
in our applications.)

Is this something to worry about?


I would suggest looking a bit higher in the stack. fdb/mdb operations
come via switchdev, and have a notification mechanism between slave.c
and port.c. Check if that notification mechanism enforces
serialisation. Also, check that everything actually does go though
this notification mechanism. Maybe the dump operations do not?



OK, for my education I took a look in upper layers. Bridge layer specify 
SWITCHDEV_F_DEFER option to switchdev operations. Which means switchdev 
hand the work over to a workqueue. Which is executed by a kworker kernel 
thread. In DSA, operations go through raw_notifier_call_chain. 
raw_notifier_call_chain has no locking, and I assume it executes in same 
context. A dump_stack() in the driver confirm my theory.


So the (most?) dsa operations execute in switchdev_deferred_process_work 
queue. If a operation sleep, other dsa operations will run in the mean 
time. So there is no serialization. Just as indicated by Vivien.


So if I still have time at hands when net-next opens again, I will do 
something about it for lan9303.




And then check the lower levels of the driver. If say statistics
operations are performed at the same time as fdb/mdb, can the register
accesses get interleaved? If they can, is that actually a problem for
the hardware?



I have not seen anything in the datasheet about simultaneous access to 
different registers. Until proven otherwise, I assume protecting 
functions that require a sequence of related read/write operations will do.


At the moment I have changed my mind, I think it is better to add a new 
alr_mutex to protect the ALR (fdb/mdb) operations. And not touch the 
existing mutex. alr_mutex need to be locked in lan9303_alr_add_port, 
lan9303_alr_del_port and lan9303_alr_loop, all of them simple functions.



 Andrew



Egil

Re: [PATCH] net: bridge: add max_fdb_count

[Egil Hjelmeland]

Den 15. nov. 2017 20:27, skrev Sarah Newman:

Current memory and CPU usage for managing bridge fdb entries is unbounded.
Add a parameter max_fdb_count, controlled from sysfs, which places an upper
limit on the number of entries. Defaults to 1024.

When max_fdb_count is met or exceeded, whether traffic is sent out a
given port should depend on its flooding behavior.

This may instead be mitigated by filtering mac address entries in the
PREROUTING chain of the ebtables nat table, but this is only practical
when mac addresses are known in advance.

Signed-off-by: Sarah Newman 
---
  net/bridge/br_device.c   |  2 ++
  net/bridge/br_fdb.c  | 25 -
  net/bridge/br_private.h  |  3 +++
  net/bridge/br_sysfs_br.c | 24 
  4 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 723f25e..18fabdf 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -335,6 +335,28 @@ static ssize_t flush_store(struct device *d,
  }
  static DEVICE_ATTR_WO(flush);
  
+static ssize_t max_fdb_count_show(struct device *d, struct device_attribute *attr,

+char *buf)
+{
+   struct net_bridge *br = to_bridge(d);
+   return sprintf(buf, "%lu\n", br->max_fdb_count);
+}
+
+static ssize_t max_fdb_count_store(struct device *d, struct device_attribute 
*attr,
+ const char *buf, size_t len)
+{
+   return store_bridge_parm(d, buf, len, br_set_max_fdb_count);
+}
+static DEVICE_ATTR_RW(max_fdb_count);
+
+static ssize_t fdb_count_show(struct device *d, struct device_attribute *attr,
+   char *buf)
+{
+   struct net_bridge *br = to_bridge(d);
+   return sprintf(buf, "%lu\n", br->fdb_count);
+}
+static DEVICE_ATTR_RO(fdb_count);
+
  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
  static ssize_t multicast_router_show(struct device *d,
 struct device_attribute *attr, char *buf)
@@ -830,6 +852,8 @@ static ssize_t vlan_stats_enabled_store(struct device *d,
_attr_gc_timer.attr,
_attr_group_addr.attr,
_attr_flush.attr,
+   _attr_max_fdb_count.attr,
+   _attr_fdb_count.attr,
  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
_attr_multicast_router.attr,
_attr_multicast_snooping.attr,





Documentation/filesystems/sysfs.txt:

All new sysfs attributes must be documented in Documentation/ABI. See 
also Documentation/ABI/README for more information.



Egil

question lan9303: DSA concurrency and locking,

[Egil Hjelmeland]

Hi experts

I am hoping for some guidance.

Does DSA offer any protection against concurrent calls of dsa_switch_ops?

I did not include any locking in the code I contributed to lan9303. 
First I felt bad locking is worse than no locking. Second I assumed that 
reviewers would point out if locking is needed.


The most "interesting" part of the lan9303 driver that has no locking is 
the ALR (=fdb/mdb). ALR access is a sequence of register operations. 
Anyway it is very unlikely that mdb related calls are reentered. But if 
it can happen, it would mean that IGMP snooping can go wrong. (Which is 
actually very bad in our applications.)


Is this something to worry about?

If yes, what is the best strategy to deal with it?

Looking in drivers/net/dsa/lan9303-core.c :

lan9303_indirect_phy_read/lan9303_indirect_phy_write + 
lan9303_write_switch_reg/lan9303_read_switch_reg are already protected 
by chip->indirect_mutex


ALR access uses lan9303_write_switch_reg/lan9303_read_switch_reg.

Adding a chip->alr_mutex on top of chip->indirect_mutex does not feel 
right to me. Would it be better to move the locking up to the relevant 
dsa_switch_ops functions?


Thanks

Egil Hjelmeland

[PATCH net-next] net: dsa: lan9303: calculate offload_fwd_mark from tag

[Egil Hjelmeland]

The lan9303 set bits in the host CPU tag indicating if a ingress frame
is a trapped IGMP or STP frame. Use these bits to calculate
skb->offload_fwd_mark more efficiently.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 net/dsa/tag_lan9303.c | 24 
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index b8c5e52b2eff..548c00254c07 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -42,6 +42,10 @@
 #define LAN9303_TAG_LEN 4
 # define LAN9303_TAG_TX_USE_ALR BIT(3)
 # define LAN9303_TAG_TX_STP_OVERRIDE BIT(4)
+# define LAN9303_TAG_RX_IGMP BIT(3)
+# define LAN9303_TAG_RX_STP BIT(4)
+# define LAN9303_TAG_RX_TRAPPED_TO_CPU (LAN9303_TAG_RX_IGMP | \
+   LAN9303_TAG_RX_STP)
 
 /* Decide whether to transmit using ALR lookup, or transmit directly to
  * port using tag. ALR learning is performed only when using ALR lookup.
@@ -91,9 +95,8 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, 
struct net_device *dev,
   struct packet_type *pt)
 {
u16 *lan9303_tag;
+   u16 lan9303_tag1;
unsigned int source_port;
-   u16 ether_type_nw;
-   u8 ip_protocol;
 
if (unlikely(!pskb_may_pull(skb, LAN9303_TAG_LEN))) {
dev_warn_ratelimited(>dev,
@@ -114,7 +117,8 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, 
struct net_device *dev,
return NULL;
}
 
-   source_port = ntohs(lan9303_tag[1]) & 0x3;
+   lan9303_tag1 = ntohs(lan9303_tag[1]);
+   source_port = lan9303_tag1 & 0x3;
 
skb->dev = dsa_master_find_slave(dev, 0, source_port);
if (!skb->dev) {
@@ -128,19 +132,7 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, 
struct net_device *dev,
skb_pull_rcsum(skb, 2 + 2);
memmove(skb->data - ETH_HLEN, skb->data - (ETH_HLEN + LAN9303_TAG_LEN),
2 * ETH_ALEN);
-   skb->offload_fwd_mark = !ether_addr_equal(skb->data - ETH_HLEN,
- eth_stp_addr);
-
-   /* We also need IGMP packets to have skb->offload_fwd_mark = 0.
-* Solving this for all conceivable situations would add more cost to
-* every packet. Instead we handle just the common case:
-* No VLAN tag + Ethernet II framing.
-* Test least probable term first.
-*/
-   ether_type_nw = lan9303_tag[2];
-   ip_protocol = *(skb->data + 9);
-   if (ip_protocol == IPPROTO_IGMP && ether_type_nw == htons(ETH_P_IP))
-   skb->offload_fwd_mark = 0;
+   skb->offload_fwd_mark = !(lan9303_tag1 & LAN9303_TAG_RX_TRAPPED_TO_CPU);
 
return skb;
 }
-- 
2.11.0

Re: [PATCH net-next 2/2] net: dsa: lan9303: Clear offload_fwd_mark for IGMP

[Egil Hjelmeland]

On 13. nov. 2017 14:02, Andrew Lunn wrote:

RTFM, my bad. The lan9303 has both STP and IGMP bits in the receive tag. It
is as simple as:

u16 lan9303_tag1 = ntohs(lan9303_tag[1]);
skb->offload_fwd_mark = !(lan9303_tag1 & 0x18);


Hi Egil

That is much nicer. But please add a couple of #defines for the 0x18
bits.


Of course!


Andrew



Egil

Re: [PATCH 0/2] net: dsa: lan9303: check error value from devm_gpiod_get_optional()

[Egil Hjelmeland]

On 13. nov. 2017 09:07, Phil Reid wrote:

Replaces Pan Bian  patch
"net: dsa: lan9303: correctly check return value of devm_gpiod_get_optional"

Errors need to be prograted back from probe.

Note: I have only compile tested the code as I don't have the hardware.

Phil Reid (2):
   net: dsa: lan9303: make lan9303_handle_reset() a void function
   net: dsa: lan9303: check error value from devm_gpiod_get_optional()

  drivers/net/dsa/lan9303-core.c | 21 -
  1 file changed, 12 insertions(+), 9 deletions(-)



I tried this on our HW lan9303, it did not break anything.

Egil

Re: [PATCH net-next 2/2] net: dsa: lan9303: Clear offload_fwd_mark for IGMP

[Egil Hjelmeland]

On 10. nov. 2017 12:54, Egil Hjelmeland wrote:

Now that IGMP packets no longer is flooded in HW, we want the SW bridge to
forward packets based on bridge configuration. To make that happen,
IGMP packets must have skb->offload_fwd_mark = 0.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
  net/dsa/tag_lan9303.c | 13 +
  1 file changed, 13 insertions(+)

diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 5ba01fc3c6ba..b8c5e52b2eff 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -92,6 +92,8 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, 
struct net_device *dev,
  {
u16 *lan9303_tag;
unsigned int source_port;
+   u16 ether_type_nw;
+   u8 ip_protocol;
  
  	if (unlikely(!pskb_may_pull(skb, LAN9303_TAG_LEN))) {

dev_warn_ratelimited(>dev,
@@ -129,6 +131,17 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, 
struct net_device *dev,
skb->offload_fwd_mark = !ether_addr_equal(skb->data - ETH_HLEN,
  eth_stp_addr);
  
+	/* We also need IGMP packets to have skb->offload_fwd_mark = 0.

+* Solving this for all conceivable situations would add more cost to
+* every packet. Instead we handle just the common case:
+* No VLAN tag + Ethernet II framing.
+* Test least probable term first.
+*/
+   ether_type_nw = lan9303_tag[2];
+   ip_protocol = *(skb->data + 9);
+   if (ip_protocol == IPPROTO_IGMP && ether_type_nw == htons(ETH_P_IP))
+   skb->offload_fwd_mark = 0;
+
return skb;
  }
  



RTFM, my bad. The lan9303 has both STP and IGMP bits in the receive tag. 
It is as simple as:


u16 lan9303_tag1 = ntohs(lan9303_tag[1]);
skb->offload_fwd_mark = !(lan9303_tag1 & 0x18);

Egil

Re: [PATCH net-next 0/2] net: dsa: lan9303: IGMP handling

[Egil Hjelmeland]

Den 10. nov. 2017 15:07, skrev Andrew Lunn:

skb->offload_fwd_mark calculation is a candidate for consolidation into the
DSA core. The calculation can probably be more polished when done at a point
where DSA has updated skb.


Hi Egil

Yes, at some point we should do this. But at the moment it is too
early. We don't have enough experience with what the different
switches need. I would like to see 4 or more devices doing it before
we consolidate the code into the core.



Absolutely. My point was rather I prefer not spend time polishing this 
snippet right now, because it will probably be refactored later anyway.


One steppingstone could be to make reusable helper-functions in 
net/dsa/dsa_priv.h. But I leave that to device #2 that implement this ;-)



Andrew



Egil

Re: [PATCH 4/4] RFC: net: dsa: realtek-smi: Add Realtek SMI driver

[Egil Hjelmeland]

On 09. nov. 2017 14:24, Andrew Lunn wrote:

What should also be considered is who is pushing swithdev and DSA
forward, and for what market.

Pure switchdev drivers is mostly being pushed forward for Top or Rack
switches. Big switches, 10G, 40G, 100G ports and lots of them. L3
routing, etc.

switchdev/DSA is mostly being pushed forward by industrial
applications. Switches in big vehicles, trains, planes. Industrial
plant control.



Out of curiosity: Are there fundamental reasons why ToR switches don't 
use the DSA model, or is it more historical?




 Andrew



Egil

[PATCH net-next 1/2] net: dsa: lan9303: Set up trapping of IGMP to CPU port

[Egil Hjelmeland]

IGMP packets should be trapped to the CPU port. The SW bridge knows
whether to forward to other ports.

With "IGMP snooping for local traffic" merged, IGMP trapping is also
required for stable IGMPv2 operation.

LAN9303 does not trap IGMP packets by default.

Enable IGMP trapping in lan9303_setup.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 320651a57c6f..6d7dee67d822 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -153,6 +153,8 @@
 # define LAN9303_SWE_VLAN_UNTAG_PORT0 BIT(12)
 #define LAN9303_SWE_VLAN_CMD_STS 0x1810
 #define LAN9303_SWE_GLB_INGRESS_CFG 0x1840
+# define LAN9303_SWE_GLB_INGR_IGMP_TRAP BIT(7)
+# define LAN9303_SWE_GLB_INGR_IGMP_PORT(p) BIT(10 + p)
 #define LAN9303_SWE_PORT_STATE 0x1843
 # define LAN9303_SWE_PORT_STATE_FORWARDING_PORT2 (0)
 # define LAN9303_SWE_PORT_STATE_LEARNING_PORT2 BIT(5)
@@ -450,6 +452,21 @@ static int lan9303_read_switch_reg(struct lan9303 *chip, 
u16 regnum, u32 *val)
return ret;
 }
 
+static int lan9303_write_switch_reg_mask(struct lan9303 *chip, u16 regnum,
+u32 val, u32 mask)
+{
+   int ret;
+   u32 reg;
+
+   ret = lan9303_read_switch_reg(chip, regnum, );
+   if (ret)
+   return ret;
+
+   reg = (reg & ~mask) | val;
+
+   return lan9303_write_switch_reg(chip, regnum, reg);
+}
+
 static int lan9303_write_switch_port(struct lan9303 *chip, int port,
 u16 regnum, u32 val)
 {
@@ -905,6 +922,15 @@ static int lan9303_setup(struct dsa_switch *ds)
if (ret)
dev_err(chip->dev, "failed to re-enable switching %d\n", ret);
 
+   /* Trap IGMP to port 0 */
+   ret = lan9303_write_switch_reg_mask(chip, LAN9303_SWE_GLB_INGRESS_CFG,
+   LAN9303_SWE_GLB_INGR_IGMP_TRAP |
+   LAN9303_SWE_GLB_INGR_IGMP_PORT(0),
+   LAN9303_SWE_GLB_INGR_IGMP_PORT(1) |
+   LAN9303_SWE_GLB_INGR_IGMP_PORT(2));
+   if (ret)
+   dev_err(chip->dev, "failed to setup IGMP trap %d\n", ret);
+
return 0;
 }
 
-- 
2.11.0

[PATCH net-next 2/2] net: dsa: lan9303: Clear offload_fwd_mark for IGMP

[Egil Hjelmeland]

Now that IGMP packets no longer is flooded in HW, we want the SW bridge to
forward packets based on bridge configuration. To make that happen,
IGMP packets must have skb->offload_fwd_mark = 0.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 net/dsa/tag_lan9303.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 5ba01fc3c6ba..b8c5e52b2eff 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -92,6 +92,8 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, 
struct net_device *dev,
 {
u16 *lan9303_tag;
unsigned int source_port;
+   u16 ether_type_nw;
+   u8 ip_protocol;
 
if (unlikely(!pskb_may_pull(skb, LAN9303_TAG_LEN))) {
dev_warn_ratelimited(>dev,
@@ -129,6 +131,17 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, 
struct net_device *dev,
skb->offload_fwd_mark = !ether_addr_equal(skb->data - ETH_HLEN,
  eth_stp_addr);
 
+   /* We also need IGMP packets to have skb->offload_fwd_mark = 0.
+* Solving this for all conceivable situations would add more cost to
+* every packet. Instead we handle just the common case:
+* No VLAN tag + Ethernet II framing.
+* Test least probable term first.
+*/
+   ether_type_nw = lan9303_tag[2];
+   ip_protocol = *(skb->data + 9);
+   if (ip_protocol == IPPROTO_IGMP && ether_type_nw == htons(ETH_P_IP))
+   skb->offload_fwd_mark = 0;
+
return skb;
 }
 
-- 
2.11.0

[PATCH net-next 0/2] net: dsa: lan9303: IGMP handling

[Egil Hjelmeland]

Set up the HW switch to trap IGMP packets to CPU port. 
And make sure skb->offload_fwd_mark is cleared for incoming IGMP packets.

skb->offload_fwd_mark calculation is a candidate for consolidation into the
DSA core. The calculation can probably be more polished when done at a point
where DSA has updated skb.  


Egil Hjelmeland (2):
  net: dsa: lan9303: Set up trapping of IGMP to CPU port
  net: dsa: lan9303: Clear offload_fwd_mark for IGMP

 drivers/net/dsa/lan9303-core.c | 26 ++
 net/dsa/tag_lan9303.c  | 13 +
 2 files changed, 39 insertions(+)

-- 
2.11.0

Re: [PATCH net-next 1/6] net: dsa: remove trans argument from mdb ops

[Egil Hjelmeland]

On 08. nov. 2017 18:19, Vivien Didelot wrote:

The DSA switch MDB ops pass the switchdev_trans structure down to the
drivers, but no one is using them and they aren't supposed to anyway.

Remove the trans argument from MDB prepare and add operations.

-   int (*port_mdb_prepare)(struct dsa_switch *ds, int port,
-   const struct switchdev_obj_port_mdb *mdb,
-   struct switchdev_trans *trans);
-   void(*port_mdb_add)(struct dsa_switch *ds, int port,
-   const struct switchdev_obj_port_mdb *mdb,
-   struct switchdev_trans *trans);
+   int (*port_mdb_prepare)(struct dsa_switch *ds, int port,
+   const struct switchdev_obj_port_mdb *mdb);
+   void (*port_mdb_add)(struct dsa_switch *ds, int port,
+const struct switchdev_obj_port_mdb *mdb);
int (*port_mdb_del)(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_mdb *mdb);


Hi Vivien
Nice to get rid of "trans". I recall I was confused by this parameter. 
"Am I supposed to do something with this parameter?".


But when at it. What about getting rid of switchdev_obj_port_mdb, making 
similar signatures as the new .port_fdb_xxx functions? Would that make 
sense?


Egil

Re: [PATCH net-next 2/6] net: dsa: return after mdb prepare phase

[Egil Hjelmeland]

On 08. nov. 2017 18:19, Vivien Didelot wrote:

The current code does not return after successfully preparing the MDB
addition on every ports member of a multicast group. Fix this.

Fixes: a1a6b7ea7f2d ("net: dsa: add cross-chip multicast support")
Reported-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
  net/dsa/switch.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 04e8ad2c3d5d..0041aba5c339 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -133,6 +133,8 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
if (err)
return err;
}
+
+   return 0;
}
  
  	for_each_set_bit(port, group, ds->num_ports)




Hi Vivien!

Will this cause a merge-conflict with the "net" patch you sent, when 
"net" is merged to "net-next"? Perhaps more polite to hold on to this 
(and following patches) until "net" patch has trickled through the system?


Regards
Egil

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Egil Hjelmeland]

On 07. nov. 2017 18:58, Andrew Lunn wrote:

Hi Andrew!

When local application join multicast; the driver get 2 X .port_mdb_prepare
+ 4 x .port_mdb_add for the address.


Humm, i would expect equal numbers of those.



To be precise: it is (1 .port_mdb_prepare + 2 x .port_mdb_add), two times.

I see that the outer repeat is due to netdev_for_each_lower_dev() in 
br_mdb_switchdev_host(). So that means we get one notification for each 
dsa-port which is joined to the bridge.



But _all_ .port_mdb_add are repeated twice as well. This is more 
interesting. I suspect that there is a missing "return 0;" in 
dsa_switch_mdb_add(), at the end of the "if 
(switchdev_trans_ph_prepare(trans)) {". Dating back to

a1a6b7ea7f2de270a51360cc48e7c49f7a283dda.




While I had an application joined on a multicast address; I restarted the
networking: meaning deleting the bridge and setting it up again. No
.port_mdb_del on the multicast address. Stopped the application. Still no
.port_mdb_del on the multicast address. So the multicast address stays in
the HW fdb, until I started and stopped the application again. Not really a
problem, just an observation.


The bridge is working by snooping joins/leaves. When the application
joined the group, the kernel would of sent an IGMP report. This
triggers the snooping code to request the hardware to start forwarding
frames to the hardware. When the application quits, it might send a
leave. But it is not required. The bridge however has a timer. The
IGMP querier in your network should cause all active groups to send a
report every so often. This keeps the timer alive. If the timer
expires, the bridge should then delete the group from the hardware.

So try stopping your application, and waiting a while.



I waited 10 minutes after stopping the application, still no cleanup. 
That was the case when restarted the networking while the application 
was running.


But when stopping the application having not restarted the networking, 
the .port_mdb_del happen within few seconds.


Cheers
Egil

[PATCH net-next] net: dsa: lan9303: Documentation: Add missing word "Mbps"

[Egil Hjelmeland]

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 Documentation/networking/dsa/lan9303.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/dsa/lan9303.txt 
b/Documentation/networking/dsa/lan9303.txt
index ec28683d107d..144b02b95207 100644
--- a/Documentation/networking/dsa/lan9303.txt
+++ b/Documentation/networking/dsa/lan9303.txt
@@ -1,9 +1,9 @@
 LAN9303 Ethernet switch driver
 ==
 
-The LAN9303 is a three port 10/100 ethernet switch with integrated phys for the
-two external ethernet ports. The third port is an RMII/MII interface to a host
-master network interface (e.g. fixed link).
+The LAN9303 is a three port 10/100 Mbps ethernet switch with integrated phys 
for
+the two external ethernet ports. The third port is an RMII/MII interface to a
+host master network interface (e.g. fixed link).
 
 
 Driver details
-- 
2.11.0

[PATCH net-next] net: dsa: lan9303: Fix lan9303_alr_del_port()

[Egil Hjelmeland]

Fix embarrassing bug in lan9303_alr_del_port(): Instead of zeroing
entr->mac_addr, I destroyed the next cache entry. Affected .port_fdb_del and
.port_mdb_del.

Fixes: 0620427ea0d6 ("net: dsa: lan9303: Add fdb/mdb manipulation")
Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 320651a57c6f..c0910aebc037 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -702,7 +702,7 @@ static int lan9303_alr_del_port(struct lan9303 *chip, const 
u8 *mac, int port)
 
entr->port_map &= ~BIT(port);
if (entr->port_map == 0) /* zero means its free again */
-   eth_zero_addr(>port_map);
+   eth_zero_addr(entr->mac_addr);
lan9303_alr_set_entry(chip, mac, entr->port_map, entr->stp_override);
 
return 0;
-- 
2.11.0

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Egil Hjelmeland]

On 07. nov. 2017 00:26, Andrew Lunn wrote:

The linux bridge supports IGMP snooping. It will listen to IGMP
reports on bridge ports and keep track of which groups have been
joined on an interface. It will then forward multicast based on this
group membership.

When the bridge adds or removed groups from an interface, it uses
switchdev to request the hardware add an mdb to a port, so the
hardware can perform the selective forwarding between ports.

What is not covered by the current bridge code, is IGMP joins/leaves
from the host on the brX interface. These are not reported via
switchdev so that hardware knows the local host is interested in the
multicast frames.

Luckily, the bridge does track joins/leaves on the brX interface. The
code is obfusticated, which is why i missed it with my first attempt.
So the first patch tries to remove this obfustication. Currently,
there is no notifications sent when the bridge interface joins a
group. The second patch adds them. bridge monitor then shows
joins/leaves in the same way as for other ports of the bridge.

Then starts the work passing down to the hardware that the host has
joined/left a group. The existing switchdev mdb object cannot be used,
since the semantics are different. The existing
SWITCHDEV_OBJ_ID_PORT_MDB is used to indicate a specific multicast
group should be forwarded out that port of the switch. However here we
require the exact opposite. We want multicast frames for the group
received on the port to the forwarded to the host. Hence add a new
object SWITCHDEV_OBJ_ID_HOST_MDB, a multicast database entry to
forward to the host. This new object is then propagated through the
DSA layers. No DSA driver changes should be needed, this should just
work...

Andrew Lunn (5):
   net: bridge: Rename mglist to host_joined
   net: bridge: Send notification when host join/leaves a group
   net: bridge: Add/del switchdev object on host join/leave
   net: dsa: slave: Handle switchdev host mdb add/del
   net: dsa: switch: Don't add CPU port to an mdb by default

  include/net/switchdev.h   |  1 +
  net/bridge/br_input.c |  2 +-
  net/bridge/br_mdb.c   | 50 +++
  net/bridge/br_multicast.c | 18 ++---
  net/bridge/br_private.h   |  2 +-
  net/dsa/slave.c   | 13 
  net/dsa/switch.c  |  3 ++-
  net/switchdev/switchdev.c |  2 ++
  8 files changed, 77 insertions(+), 14 deletions(-)


Hi Andrew!

I tested this series today on my board with lan9303, and it does seem to 
work. But no extensive testing though.


I have not looked at the contents of the patches.

Some observations, not meant to delay the series:

When local application join multicast; the driver get 2 X 
.port_mdb_prepare + 4 x .port_mdb_add for the address.


"bridge mdb" does not list local multicast memberships, is that a "feature"?

While I had an application joined on a multicast address; I restarted 
the networking: meaning deleting the bridge and setting it up again. No 
.port_mdb_del on the multicast address. Stopped the application. Still 
no .port_mdb_del on the multicast address. So the multicast address 
stays in the HW fdb, until I started and stopped the application again. 
Not really a problem, just an observation.



Then finally, there is a more serious issue related to IGMPv2. As I 
suspected, with "IGMP snooping for local traffic", I have to activate 
"IGMP trapping" in the lan9303 HW in order to get it to work with a 
IGMPv2 querier. (Remember, unlike IGMPv3, IGMPv2 reports are "inband" in 
the group). The HW allows me to select which ports receive IGMP packets. 
The best is to only send to CPU, so the SW bridge can do the right thing 
in all situations. However, then we must make sure skb->offload_fwd_mark 
is zero for IGMP packets. Which was easy to do in my proof-of-concept 
test, but I suspect a proper test on ip->protocol is more complicated?


Acked-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>


Cheers
Egil

Re: [PATCH net-next] net: dsa: lan9303: Drop port range check

[Egil Hjelmeland]

On 06. nov. 2017 15:19, Egil Hjelmeland wrote:

Now that ds->num_ports is 3, there is no need to check range of "port"
parameter.

+   lan9303_disable_processing_port(chip, port);
+   lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
+ MII_BMCR, BMCR_PDOWN);
  }


I see now that line breaking could be adjusted here. But I am going to 
touch that line in a upcomming patch about phy_addr_sel_strap. I

promise fix it then, making it to a one-liner.

  
  static int lan9303_port_bridge_join(struct dsa_switch *ds, int port,




Egil

[PATCH net-next] net: dsa: lan9303: Drop port range check

[Egil Hjelmeland]

Now that ds->num_ports is 3, there is no need to check range of "port"
parameter.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 27 ---
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 70ecd18a5e7d..320651a57c6f 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1057,17 +1057,7 @@ static int lan9303_port_enable(struct dsa_switch *ds, 
int port,
 {
struct lan9303 *chip = ds->priv;
 
-   /* enable internal packet processing */
-   switch (port) {
-   case 1:
-   case 2:
-   return lan9303_enable_processing_port(chip, port);
-   default:
-   dev_dbg(chip->dev,
-   "Error: request to power up invalid port %d\n", port);
-   }
-
-   return -ENODEV;
+   return lan9303_enable_processing_port(chip, port);
 }
 
 static void lan9303_port_disable(struct dsa_switch *ds, int port,
@@ -1075,18 +1065,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, 
int port,
 {
struct lan9303 *chip = ds->priv;
 
-   /* disable internal packet processing */
-   switch (port) {
-   case 1:
-   case 2:
-   lan9303_disable_processing_port(chip, port);
-   lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
- MII_BMCR, BMCR_PDOWN);
-   break;
-   default:
-   dev_dbg(chip->dev,
-   "Error: request to power down invalid port %d\n", port);
-   }
+   lan9303_disable_processing_port(chip, port);
+   lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
+ MII_BMCR, BMCR_PDOWN);
 }
 
 static int lan9303_port_bridge_join(struct dsa_switch *ds, int port,
-- 
2.11.0

Re: [PATCH net-next 4/5] net: dsa: lan9303: Remove unnecessary parentheses

[Egil Hjelmeland]

On 03. nov. 2017 15:54, Vivien Didelot wrote:

Hi Egil,

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

If you send a v2, you may want to address the other parenthesis
alignment issues found when running ./scripts/checkpatch -f on the
lan9303* files.



There is just one remaining alignment issue. Removing that would
require introducing an extra variable just for that purpose. I
don't think that makes the code more readable. So I will not do it.
If anybody else want to do it, fine, I will just watch in silence.


Applying this gives you a few more: https://patchwork.kernel.org/patch/10014913/

(you can also add my Reviewed-by tag on patches you didn't touch.)


Thanks,

 Vivien



Egil

[PATCH v2 net-next 4/4] net: dsa: lan9303: Adjust indenting

[Egil Hjelmeland]

Remove scripts/checkpatch.pl CHECKs by adjusting indenting.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
Reviewed-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 drivers/net/dsa/lan9303_i2c.c  | 2 +-
 drivers/net/dsa/lan9303_mdio.c | 2 +-
 net/dsa/tag_lan9303.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/lan9303_i2c.c b/drivers/net/dsa/lan9303_i2c.c
index 24ec20f7f444..909a7e864246 100644
--- a/drivers/net/dsa/lan9303_i2c.c
+++ b/drivers/net/dsa/lan9303_i2c.c
@@ -50,7 +50,7 @@ static int lan9303_i2c_probe(struct i2c_client *client,
return -ENOMEM;
 
sw_dev->chip.regmap = devm_regmap_init_i2c(client,
-   _i2c_regmap_config);
+  _i2c_regmap_config);
if (IS_ERR(sw_dev->chip.regmap)) {
ret = PTR_ERR(sw_dev->chip.regmap);
dev_err(>dev, "Failed to allocate register map: %d\n",
diff --git a/drivers/net/dsa/lan9303_mdio.c b/drivers/net/dsa/lan9303_mdio.c
index 0bc56b9900f9..cc9c2ea1c4fe 100644
--- a/drivers/net/dsa/lan9303_mdio.c
+++ b/drivers/net/dsa/lan9303_mdio.c
@@ -116,7 +116,7 @@ static int lan9303_mdio_probe(struct mdio_device *mdiodev)
return -ENOMEM;
 
sw_dev->chip.regmap = devm_regmap_init(>dev, NULL, sw_dev,
-   _mdio_regmap_config);
+  _mdio_regmap_config);
if (IS_ERR(sw_dev->chip.regmap)) {
ret = PTR_ERR(sw_dev->chip.regmap);
dev_err(>dev, "regmap init failed: %d\n", ret);
diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index e526c8967b98..5ba01fc3c6ba 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -88,7 +88,7 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, 
struct net_device *dev)
 }
 
 static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
-   struct packet_type *pt)
+  struct packet_type *pt)
 {
u16 *lan9303_tag;
unsigned int source_port;
-- 
2.11.0

[PATCH v2 net-next 1/4] net: dsa: lan9303: Correct register names in comments

[Egil Hjelmeland]

Two comments refer to registers, but lack the LAN9303_ prefix.
Fix that.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
Reviewed-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 include/linux/dsa/lan9303.h | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/dsa/lan9303.h b/include/linux/dsa/lan9303.h
index 05d8d136baab..f48a85c377de 100644
--- a/include/linux/dsa/lan9303.h
+++ b/include/linux/dsa/lan9303.h
@@ -13,8 +13,8 @@ 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 */
+   u8  port_map; /* Bitmap of ports. Zero if unused entry */
+   u8  stp_override; /* non zero if set LAN9303_ALR_DAT1_AGE_OVERRID */
 };
 
 struct lan9303 {
@@ -28,7 +28,9 @@ struct lan9303 {
struct mutex indirect_mutex; /* protect indexed register access */
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 */
+
+   /* remember LAN9303_SWE_PORT_STATE while not bridged */
+   u32 swe_port_state;
/* LAN9303 do not offer reading specific ALR entry. Cache all
 * static entries in a flat table
 **/
-- 
2.11.0

[PATCH v2 net-next 2/4] net: dsa: lan9303: Fix syntax errors in device tree examples

[Egil Hjelmeland]

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
Reviewed-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 Documentation/devicetree/bindings/net/dsa/lan9303.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/lan9303.txt 
b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
index 4448d063ddf6..464d6bf87605 100644
--- a/Documentation/devicetree/bindings/net/dsa/lan9303.txt
+++ b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
@@ -52,7 +52,7 @@ I2C managed mode:
 
port@1 { /* external port 1 */
reg = <1>;
-   label = "lan1;
+   label = "lan1";
};
 
port@2 { /* external port 2 */
@@ -89,7 +89,7 @@ MDIO managed mode:
 
port@1 { /* external port 1 */
reg = <1>;
-   label = "lan1;
+   label = "lan1";
};
 
port@2 { /* external port 2 */
-- 
2.11.0

[PATCH v2 net-next 0/4] net: dsa: lan9303: Linting

[Egil Hjelmeland]

This series is non-functional. 
 - Correct some errors in comments and documentation.
Remove scripts/checkpatch.pl WARNINGs and most CHECKs:
 - Replace msleep(1) with usleep_range()
 - Adjust indenting
 

Changes v1 -> v2:
 - Removed patch 4 "Remove unnecessary parentheses", to be addressed later

 
Egil Hjelmeland (4):
  net: dsa: lan9303: Correct register names in comments
  net: dsa: lan9303: Fix syntax errors in device tree examples
  net: dsa: lan9303: Replace msleep(1) with usleep_range()
  net: dsa: lan9303: Adjust indenting

 Documentation/devicetree/bindings/net/dsa/lan9303.txt | 4 ++--
 drivers/net/dsa/lan9303-core.c| 4 ++--
 drivers/net/dsa/lan9303_i2c.c | 2 +-
 drivers/net/dsa/lan9303_mdio.c| 2 +-
 include/linux/dsa/lan9303.h   | 8 +---
 net/dsa/tag_lan9303.c | 2 +-
 6 files changed, 12 insertions(+), 10 deletions(-)

-- 
2.11.0

[PATCH v2 net-next 3/4] net: dsa: lan9303: Replace msleep(1) with usleep_range()

[Egil Hjelmeland]

Remove scripts/checkpatch.pl WARNING by replacing msleep(1) with usleep_range()

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
Reviewed-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 drivers/net/dsa/lan9303-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index c4afc8f1a66d..70ecd18a5e7d 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -284,7 +284,7 @@ static int lan9303_indirect_phy_wait_for_completion(struct 
lan9303 *chip)
}
if (!(reg & LAN9303_PMI_ACCESS_MII_BUSY))
return 0;
-   msleep(1);
+   usleep_range(1000, 2000);
}
 
return -EIO;
@@ -376,7 +376,7 @@ static int lan9303_switch_wait_for_completion(struct 
lan9303 *chip)
}
if (!(reg & LAN9303_SWITCH_CSR_CMD_BUSY))
return 0;
-   msleep(1);
+   usleep_range(1000, 2000);
}
 
return -EIO;
-- 
2.11.0

Re: [PATCH net-next 4/5] net: dsa: lan9303: Remove unnecessary parentheses

[Egil Hjelmeland]

On 03. nov. 2017 15:35, Egil Hjelmeland wrote:

On 03. nov. 2017 15:11, Joe Perches wrote:

On Fri, 2017-11-03 at 11:55 +0100, Egil Hjelmeland wrote:

Remove scripts/checkpatch.pl CHECKs by remove unnecessary parentheses

[]
diff --git a/drivers/net/dsa/lan9303-core.c 
b/drivers/net/dsa/lan9303-core.c

[]
@@ -483,7 +483,7 @@ static int lan9303_detect_phy_setup(struct 
lan9303 *chip)

  return reg;
  }
-    if ((reg != 0) && (reg != 0x))
+    if (reg != 0 && reg != 0x)
  chip->phy_addr_sel_strap = 1;
  else
  chip->phy_addr_sel_strap = 0;


phy_addr_sel_strap is currently bool.

If this is to be changed, it should be set
true or false.

My preference would be:

chip->phy_addr_sel_strap = (reg != 0 && reg != 0x);

But perhaps its bool type should be converted
to int as this phy_addr_sel_strap is used as
int several times.



Hi Joe

I had not noticed that phy_addr_sel_strap is bool. I agree that is
misleading. Your suggestion is perhaps a bit "magic", but on the other
hand the magic is explained well in the comment above.

If there are no disagreements, I can do a v2 with that.

And thanks for teaching me about "git grep"!

Cheers
Egil


Hi
I changed my mind slightly. I will just remove patch 4 in v2. In stead
deal with phy_addr_sel_strap in a separate post.

Because I think I want to rename phy_addr_sel_strap as well, plus some
other simplification. So starting to creep out of the "linting" scope.

Egil

Re: [PATCH net-next 4/5] net: dsa: lan9303: Remove unnecessary parentheses

[Egil Hjelmeland]

On 03. nov. 2017 15:11, Joe Perches wrote:

On Fri, 2017-11-03 at 11:55 +0100, Egil Hjelmeland wrote:

Remove scripts/checkpatch.pl CHECKs by remove unnecessary parentheses

[]

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c

[]

@@ -483,7 +483,7 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
return reg;
}
  
-	if ((reg != 0) && (reg != 0x))

+   if (reg != 0 && reg != 0x)
chip->phy_addr_sel_strap = 1;
else
chip->phy_addr_sel_strap = 0;


phy_addr_sel_strap is currently bool.

If this is to be changed, it should be set
true or false.

My preference would be:

chip->phy_addr_sel_strap = (reg != 0 && reg != 0x);

But perhaps its bool type should be converted
to int as this phy_addr_sel_strap is used as
int several times.



Hi Joe

I had not noticed that phy_addr_sel_strap is bool. I agree that is
misleading. Your suggestion is perhaps a bit "magic", but on the other
hand the magic is explained well in the comment above.

If there are no disagreements, I can do a v2 with that.

And thanks for teaching me about "git grep"!

Cheers
Egil

[PATCH net-next 4/5] net: dsa: lan9303: Remove unnecessary parentheses

[Egil Hjelmeland]

Remove scripts/checkpatch.pl CHECKs by remove unnecessary parentheses

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 70ecd18a5e7d..b9a95f542f65 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -483,7 +483,7 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
return reg;
}
 
-   if ((reg != 0) && (reg != 0x))
+   if (reg != 0 && reg != 0x)
chip->phy_addr_sel_strap = 1;
else
chip->phy_addr_sel_strap = 0;
-- 
2.11.0

[PATCH net-next 5/5] net: dsa: lan9303: Adjust indenting

[Egil Hjelmeland]

Remove scripts/checkpatch.pl CHECKs by adjusting indenting.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303_i2c.c  | 2 +-
 drivers/net/dsa/lan9303_mdio.c | 2 +-
 net/dsa/tag_lan9303.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/lan9303_i2c.c b/drivers/net/dsa/lan9303_i2c.c
index 24ec20f7f444..909a7e864246 100644
--- a/drivers/net/dsa/lan9303_i2c.c
+++ b/drivers/net/dsa/lan9303_i2c.c
@@ -50,7 +50,7 @@ static int lan9303_i2c_probe(struct i2c_client *client,
return -ENOMEM;
 
sw_dev->chip.regmap = devm_regmap_init_i2c(client,
-   _i2c_regmap_config);
+  _i2c_regmap_config);
if (IS_ERR(sw_dev->chip.regmap)) {
ret = PTR_ERR(sw_dev->chip.regmap);
dev_err(>dev, "Failed to allocate register map: %d\n",
diff --git a/drivers/net/dsa/lan9303_mdio.c b/drivers/net/dsa/lan9303_mdio.c
index 0bc56b9900f9..cc9c2ea1c4fe 100644
--- a/drivers/net/dsa/lan9303_mdio.c
+++ b/drivers/net/dsa/lan9303_mdio.c
@@ -116,7 +116,7 @@ static int lan9303_mdio_probe(struct mdio_device *mdiodev)
return -ENOMEM;
 
sw_dev->chip.regmap = devm_regmap_init(>dev, NULL, sw_dev,
-   _mdio_regmap_config);
+  _mdio_regmap_config);
if (IS_ERR(sw_dev->chip.regmap)) {
ret = PTR_ERR(sw_dev->chip.regmap);
dev_err(>dev, "regmap init failed: %d\n", ret);
diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index e526c8967b98..5ba01fc3c6ba 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -88,7 +88,7 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, 
struct net_device *dev)
 }
 
 static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
-   struct packet_type *pt)
+  struct packet_type *pt)
 {
u16 *lan9303_tag;
unsigned int source_port;
-- 
2.11.0

[PATCH net-next 2/5] net: dsa: lan9303: Fix syntax errors in device tree examples

[Egil Hjelmeland]

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 Documentation/devicetree/bindings/net/dsa/lan9303.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/lan9303.txt 
b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
index 4448d063ddf6..464d6bf87605 100644
--- a/Documentation/devicetree/bindings/net/dsa/lan9303.txt
+++ b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
@@ -52,7 +52,7 @@ I2C managed mode:
 
port@1 { /* external port 1 */
reg = <1>;
-   label = "lan1;
+   label = "lan1";
};
 
port@2 { /* external port 2 */
@@ -89,7 +89,7 @@ MDIO managed mode:
 
port@1 { /* external port 1 */
reg = <1>;
-   label = "lan1;
+   label = "lan1";
};
 
port@2 { /* external port 2 */
-- 
2.11.0

[PATCH net-next 0/5] net: dsa: lan9303: Linting

[Egil Hjelmeland]

This series is non-functional. 
 - Correct some errors in comments and documentation.
Remove scripts/checkpatch.pl WARNINGs and most CHECKs:
 - Replace msleep(1) with usleep_range()
 - Remove unnecessary parentheses
 - Adjust indenting

Egil Hjelmeland (5):
  net: dsa: lan9303: Correct register names in comments
  net: dsa: lan9303: Fix syntax errors in device tree examples
  net: dsa: lan9303: Replace msleep(1) with usleep_range()
  net: dsa: lan9303: Remove unnecessary parentheses
  net: dsa: lan9303: Adjust indenting

 Documentation/devicetree/bindings/net/dsa/lan9303.txt | 4 ++--
 drivers/net/dsa/lan9303-core.c| 6 +++---
 drivers/net/dsa/lan9303_i2c.c | 2 +-
 drivers/net/dsa/lan9303_mdio.c| 2 +-
 include/linux/dsa/lan9303.h   | 8 +---
 net/dsa/tag_lan9303.c | 2 +-
 6 files changed, 13 insertions(+), 11 deletions(-)

-- 
2.11.0

[PATCH net-next 3/5] net: dsa: lan9303: Replace msleep(1) with usleep_range()

[Egil Hjelmeland]

Remove scripts/checkpatch.pl WARNING by replacing msleep(1) with usleep_range()

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index c4afc8f1a66d..70ecd18a5e7d 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -284,7 +284,7 @@ static int lan9303_indirect_phy_wait_for_completion(struct 
lan9303 *chip)
}
if (!(reg & LAN9303_PMI_ACCESS_MII_BUSY))
return 0;
-   msleep(1);
+   usleep_range(1000, 2000);
}
 
return -EIO;
@@ -376,7 +376,7 @@ static int lan9303_switch_wait_for_completion(struct 
lan9303 *chip)
}
if (!(reg & LAN9303_SWITCH_CSR_CMD_BUSY))
return 0;
-   msleep(1);
+   usleep_range(1000, 2000);
}
 
return -EIO;
-- 
2.11.0

[PATCH net-next 1/5] net: dsa: lan9303: Correct register names in comments

[Egil Hjelmeland]

Two comments refer to registers, but lack the LAN9303_ prefix.
Fix that.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 include/linux/dsa/lan9303.h | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/dsa/lan9303.h b/include/linux/dsa/lan9303.h
index 05d8d136baab..f48a85c377de 100644
--- a/include/linux/dsa/lan9303.h
+++ b/include/linux/dsa/lan9303.h
@@ -13,8 +13,8 @@ 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 */
+   u8  port_map; /* Bitmap of ports. Zero if unused entry */
+   u8  stp_override; /* non zero if set LAN9303_ALR_DAT1_AGE_OVERRID */
 };
 
 struct lan9303 {
@@ -28,7 +28,9 @@ struct lan9303 {
struct mutex indirect_mutex; /* protect indexed register access */
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 */
+
+   /* remember LAN9303_SWE_PORT_STATE while not bridged */
+   u32 swe_port_state;
/* LAN9303 do not offer reading specific ALR entry. Cache all
 * static entries in a flat table
 **/
-- 
2.11.0

[PATCH net-next] net: Define eth_stp_addr in linux/etherdevice.h

[Egil Hjelmeland]

Why:
The lan9303 driver defines eth_stp_addr as a synonym to
eth_reserved_addr_base to get the STP ethernet address 01:80:c2:00:00:00.

eth_reserved_addr_base is also used to define the start of Bridge Reserved
ethernet address range, which happen to be the STP address.

br_dev_setup refer to eth_reserved_addr_base as a definition of STP
address.

Clean up by:
 - Move the eth_stp_addr definition to linux/etherdevice.h
 - Use eth_stp_addr instead of eth_reserved_addr_base in br_dev_setup.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 include/linux/dsa/lan9303.h | 2 --
 include/linux/etherdevice.h | 1 +
 net/bridge/br_device.c  | 2 +-
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/dsa/lan9303.h b/include/linux/dsa/lan9303.h
index b2110e69630f..05d8d136baab 100644
--- a/include/linux/dsa/lan9303.h
+++ b/include/linux/dsa/lan9303.h
@@ -34,5 +34,3 @@ struct lan9303 {
 **/
struct lan9303_alr_cache_entry alr_cache[LAN9303_NUM_ALR_RECORDS];
 };
-
-#define eth_stp_addr eth_reserved_addr_base
diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 2d9f80848d4b..263dbcad22fc 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -66,6 +66,7 @@ int eth_gro_complete(struct sk_buff *skb, int nhoff);
 /* Reserved Ethernet Addresses per IEEE 802.1Q */
 static const u8 eth_reserved_addr_base[ETH_ALEN] __aligned(2) =
 { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x00 };
+#define eth_stp_addr eth_reserved_addr_base
 
 /**
  * is_link_local_ether_addr - Determine if given Ethernet address is link-local
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 28bb22186fa0..af5b8c87f590 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -421,7 +421,7 @@ void br_dev_setup(struct net_device *dev)
br->bridge_id.prio[0] = 0x80;
br->bridge_id.prio[1] = 0x00;
 
-   ether_addr_copy(br->group_addr, eth_reserved_addr_base);
+   ether_addr_copy(br->group_addr, eth_stp_addr);
 
br->stp_enabled = BR_NO_STP;
br->group_fwd_mask = BR_GROUPFWD_DEFAULT;
-- 
2.11.0

[PATCH net-next] net: dsa: lan9303: Added Documentation/networking/dsa/lan9303.txt

[Egil Hjelmeland]

From: Egil Hjelmeland <egil.hjelmel...@zenitel.com>

Provide a rough overview of the state of the driver. And explain that the
driver operates in two modes: bridged and port-separated.

Signed-off-by: Egil Hjelmeland <egil.hjelmel...@zenitel.com>
---
 Documentation/networking/dsa/lan9303.txt | 37 
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/networking/dsa/lan9303.txt

diff --git a/Documentation/networking/dsa/lan9303.txt 
b/Documentation/networking/dsa/lan9303.txt
new file mode 100644
index ..ec28683d107d
--- /dev/null
+++ b/Documentation/networking/dsa/lan9303.txt
@@ -0,0 +1,37 @@
+LAN9303 Ethernet switch driver
+==
+
+The LAN9303 is a three port 10/100 ethernet switch with integrated phys for the
+two external ethernet ports. The third port is an RMII/MII interface to a host
+master network interface (e.g. fixed link).
+
+
+Driver details
+==
+
+The driver is implemented as a DSA driver, see
+Documentation/networking/dsa/dsa.txt.
+
+See Documentation/devicetree/bindings/net/dsa/lan9303.txt for device tree
+binding.
+
+The LAN9303 can be managed both via MDIO and I2C, both supported by this 
driver.
+
+At startup the driver configures the device to provide two separate network
+interfaces (which is the default state of a DSA device). Due to HW limitations,
+no HW MAC learning takes place in this mode.
+
+When both user ports are joined to the same bridge, the normal HW MAC learning
+is enabled. This means that unicast traffic is forwarded in HW. Broadcast and
+multicast is flooded in HW. STP is also supported in this mode. The driver
+support fdb/mdb operations as well, meaning IGMP snooping is supported.
+
+If one of the user ports leave the bridge, the ports goes back to the initial
+separated operation.
+
+
+Driver limitations
+==
+
+ - Support for VLAN filtering is not implemented
+ - The HW does not support VLAN-specific fdb entries
-- 
2.11.0

Re: [PATCH net-next 0/3] net: dsa: lan9303: Fix STP and flooding issues

[Egil Hjelmeland]

On 01. nov. 2017 13:30, David Miller wrote:

From: Egil Hjelmeland <pri...@egil-hjelmeland.no>
Date: Tue, 31 Oct 2017 15:47:59 +0100


This patch set finishes the STP support, and fixes flooding issues.



Series applied, thank you.



Thanks! Based on happy testing on my desktop, I conclude that the 
net-next lan9303 driver has now reached a level where my employer 
Zenitel can use it out of the box. We have covered everything we need 
from our proprietary driver.


Our old proprietary driver can not separate the interfaces. Being able 
to do so may be interesting in some custom usages of our product. And we 
are very much looking forward to Andrew progressing his series for IGMP 
snooping for local traffic!


Egil

[PATCH net-next 2/3] net: dsa: lan9303: Add STP ALR entry on port 0

[Egil Hjelmeland]

STP BPDUs arriving on user ports must sent to CPU port only,
for processing by the SW bridge.

Add an ALR entry with STP state override to fix that.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 2 ++
 include/linux/dsa/lan9303.h| 2 ++
 net/dsa/tag_lan9303.c  | 1 -
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 4c412bd52319..c4afc8f1a66d 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -773,6 +773,7 @@ static int lan9303_separate_ports(struct lan9303 *chip)
 {
int ret;
 
+   lan9303_alr_del_port(chip, eth_stp_addr, 0);
ret = lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR,
LAN9303_SWE_PORT_MIRROR_SNIFFER_PORT0 |
LAN9303_SWE_PORT_MIRROR_MIRRORED_PORT1 |
@@ -797,6 +798,7 @@ static void lan9303_bridge_ports(struct lan9303 *chip)
 
lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
 chip->swe_port_state);
+   lan9303_alr_add_port(chip, eth_stp_addr, 0, true);
 }
 
 static int lan9303_handle_reset(struct lan9303 *chip)
diff --git a/include/linux/dsa/lan9303.h b/include/linux/dsa/lan9303.h
index 05d8d136baab..b2110e69630f 100644
--- a/include/linux/dsa/lan9303.h
+++ b/include/linux/dsa/lan9303.h
@@ -34,3 +34,5 @@ struct lan9303 {
 **/
struct lan9303_alr_cache_entry alr_cache[LAN9303_NUM_ALR_RECORDS];
 };
+
+#define eth_stp_addr eth_reserved_addr_base
diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 537ca991fafe..18f45cd9f625 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -42,7 +42,6 @@
 #define LAN9303_TAG_LEN 4
 # define LAN9303_TAG_TX_USE_ALR BIT(3)
 # define LAN9303_TAG_TX_STP_OVERRIDE BIT(4)
-#define eth_stp_addr eth_reserved_addr_base
 
 /* Decide whether to transmit using ALR lookup, or transmit directly to
  * port using tag. ALR learning is performed only when using ALR lookup.
-- 
2.11.0

[PATCH net-next 1/3] net: dsa: lan9303: Transmit using ALR when unicast

[Egil Hjelmeland]

lan9303_xmit_use_arl() introduced in previous patch set is wrong.
The chip flood broadcast and unknown multicast frames. The effect is that
broadcasts and multicasts are duplicated on egress. It is not possible to
configure the chip to direct unknown multicasts to CPU port only.

This means that only unicast frames can be transmitted using ALR lookup.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 net/dsa/tag_lan9303.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 64092325aac3..537ca991fafe 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -46,7 +46,7 @@
 
 /* Decide whether to transmit using ALR lookup, or transmit directly to
  * port using tag. ALR learning is performed only when using ALR lookup.
- * If the two external ports are bridged and the packet is not STP BPDU,
+ * If the two external ports are bridged and the frame is unicast,
  * then use ALR lookup to allow ALR learning on CPU port.
  * Otherwise transmit directly to port with STP state override.
  * See also: lan9303_separate_ports() and lan9303.pdf 6.4.10.1
@@ -55,7 +55,7 @@ static int lan9303_xmit_use_arl(struct dsa_port *dp, u8 
*dest_addr)
 {
struct lan9303 *chip = dp->ds->priv;
 
-   return chip->is_bridged && !ether_addr_equal(dest_addr, eth_stp_addr);
+   return chip->is_bridged && !is_multicast_ether_addr(dest_addr);
 }
 
 static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device 
*dev)
-- 
2.11.0

[PATCH net-next 0/3] net: dsa: lan9303: Fix STP and flooding issues

[Egil Hjelmeland]

This patch set finishes the STP support, and fixes flooding issues.

Patch 1 fixes a flooding issue in the previous patch set.
Patch 2 finishes STP support by adding a ALR entry.
Patch 3 prevent duplicate flooding in HW and SW bridge.


Egil Hjelmeland (3):
  net: dsa: lan9303: Transmit using ALR when unicast
  net: dsa: lan9303: Add STP ALR entry on port 0
  net: dsa: lan9303: lan9303_rcv set skb->offload_fwd_mark

 drivers/net/dsa/lan9303-core.c | 2 ++
 include/linux/dsa/lan9303.h| 2 ++
 net/dsa/tag_lan9303.c  | 7 ---
 3 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.11.0

[PATCH net-next 3/3] net: dsa: lan9303: lan9303_rcv set skb->offload_fwd_mark

[Egil Hjelmeland]

The chip flood broadcast and unknown multicast frames.
On receive set skb->offload_fwd_mark to prevent the SW from flooding to the
same ports.

One exception: Because the ALR is set up to forward STP BPDUs only to CPU,
the SW bridge should flood STP BPDUs if local STP is not enabled.
This is archived by not setting skb->offload_fwd_mark on STP BPDUs.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 net/dsa/tag_lan9303.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 18f45cd9f625..e526c8967b98 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -126,6 +126,8 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, 
struct net_device *dev,
skb_pull_rcsum(skb, 2 + 2);
memmove(skb->data - ETH_HLEN, skb->data - (ETH_HLEN + LAN9303_TAG_LEN),
2 * ETH_ALEN);
+   skb->offload_fwd_mark = !ether_addr_equal(skb->data - ETH_HLEN,
+ eth_stp_addr);
 
return skb;
 }
-- 
2.11.0

Re: [PATCH net-next 0/9] net: dsa: define port types

[Egil Hjelmeland]

Den 27. okt. 2017 21:37, skrev Andrew Lunn:

On Fri, Oct 27, 2017 at 02:56:51PM +0200, Egil Hjelmeland wrote:

The DSA code currently has 3 bitmaps in the dsa_switch structure:
cpu_port_mask, dsa_port_mask and enabled_port_mask.



Hi Vivien

First I must apologize to everybody for not replying in-thread. Problem
is that I was not subscribed to netdev. But now I am, so I promise it
will not happen again :-)

So to the point. I think DSA need to keep track of multicast
memberships. As it is now, dsa_switch_mdb_add() include
the CPU/DSA port(s) in the multicast. But  multicast is never
removed from the CPU/DSA port(s).


Hi Egil

You should take a look at my patches from a few weeks back. I hope to
make another version next week. These deal with multicast on the CPU
port, or better said, the host wanting to receive a multicast group.

   Andrew



Hi Andrew

I suppose you refer to https://www.spinics.net/lists/netdev/msg453848.html.
That will be great to have for my employer.

Regarding the offload_fwd_mark discussion: I did some experiments
yesterday afternoon on the lan9303 driver.

- Driver add the STP address to the ALR(fdb) pointing to CPU port
- In tag_lan9303.c rcv() set offload_fwd_mark if destination is
  not STP address.

In addition; I found out that my fresh lan9303_xmit_use_arl() is wrong:
Multicast and broadcasts must not be sent via the ALR.

 - use ALR on transmit if bridged and destination is unicast.

Then it looks as if the system is doing the right thing, both for
incomming and outgoing packets. Including STP BPDUs both when
STP is enabled and not.

I will have to test more though when back at office next week.

Egil

Re: net-next Compile error

[Egil Hjelmeland]

Den 27. okt. 2017 15:50, skrev Egil Hjelmeland:
Starting with net-next commit 60e2a7780793bae0debc275a9ccd57f7da0cf195 
"tcp: TCP experimental option for SMC", I can not cross compile the kernel.


In file included from ./include/asm-generic/unaligned.h:17:0,
  from ./arch/arm/include/generated/asm/unaligned.h:1,
  from net/ipv4/tcp_input.c:76:
./include/linux/unaligned/le_struct.h:6:19: note: previous definition of 
'get_unaligned_le16' was here

Hi Egil
Long since you did "make distclean"?

BR
Egil

net-next Compile error

[Egil Hjelmeland]

Starting with net-next commit 60e2a7780793bae0debc275a9ccd57f7da0cf195 
"tcp: TCP experimental option for SMC", I can not cross compile the kernel.


ARCH=arm

arm-v5te-linux-gnueabi-gcc --version
arm-v5te-linux-gnueabi-gcc (OSELAS.Toolchain-2012.12.1) 4.7.2

make zImage


  CC  net/ipv4/netfilter/ipt_MASQUERADE.o
In file included from net/ipv4/tcp_input.c:79:0:
./include/linux/unaligned/access_ok.h:7:28: error: redefinition of 
'get_unaligned_le16'

In file included from ./include/asm-generic/unaligned.h:17:0,
 from ./arch/arm/include/generated/asm/unaligned.h:1,
 from net/ipv4/tcp_input.c:76:
./include/linux/unaligned/le_struct.h:6:19: note: previous definition of 
'get_unaligned_le16' was here

In file included from net/ipv4/tcp_input.c:79:0:
./include/linux/unaligned/access_ok.h:12:28: error: redefinition of 
'get_unaligned_le32'

In file included from ./include/asm-generic/unaligned.h:17:0,
 from ./arch/arm/include/generated/asm/unaligned.h:1,
 from net/ipv4/tcp_input.c:76:
./include/linux/unaligned/le_struct.h:11:19: note: previous definition 
of 'get_unaligned_le32' was here

In file included from net/ipv4/tcp_input.c:79:0:
./include/linux/unaligned/access_ok.h:17:28: error: redefinition of 
'get_unaligned_le64'

In file included from ./include/asm-generic/unaligned.h:17:0,
 from ./arch/arm/include/generated/asm/unaligned.h:1,
 from net/ipv4/tcp_input.c:76:
./include/linux/unaligned/le_struct.h:16:19: note: previous definition 
of 'get_unaligned_le64' was here

In file included from net/ipv4/tcp_input.c:79:0:
./include/linux/unaligned/access_ok.h:22:28: error: redefinition of 
'get_unaligned_be16'

In file included from ./include/asm-generic/unaligned.h:18:0,
 from ./arch/arm/include/generated/asm/unaligned.h:1,
 from net/ipv4/tcp_input.c:76:
./include/linux/unaligned/be_byteshift.h:40:19: note: previous 
definition of 'get_unaligned_be16' was here

In file included from net/ipv4/tcp_input.c:79:0:
./include/linux/unaligned/access_ok.h:27:28: error: redefinition of 
'get_unaligned_be32'

In file included from ./include/asm-generic/unaligned.h:18:0,
 from ./arch/arm/include/generated/asm/unaligned.h:1,
 from net/ipv4/tcp_input.c:76:
./include/linux/unaligned/be_byteshift.h:45:19: note: previous 
definition of 'get_unaligned_be32' was here

In file included from net/ipv4/tcp_input.c:79:0:
./include/linux/unaligned/access_ok.h:32:28: error: redefinition of 
'get_unaligned_be64'

In file included from ./include/asm-generic/unaligned.h:18:0,
 from ./arch/arm/include/generated/asm/unaligned.h:1,
 from net/ipv4/tcp_input.c:76:
./include/linux/unaligned/be_byteshift.h:50:19: note: previous 
definition of 'get_unaligned_be64' was here

In file included from net/ipv4/tcp_input.c:79:0:
./include/linux/unaligned/access_ok.h:37:29: error: redefinition of 
'put_unaligned_le16'

In file included from ./include/asm-generic/unaligned.h:17:0,
 from ./arch/arm/include/generated/asm/unaligned.h:1,
 from net/ipv4/tcp_input.c:76:
./include/linux/unaligned/le_struct.h:21:20: note: previous definition 
of 'put_unaligned_le16' was here

In file included from net/ipv4/tcp_input.c:79:0:
./include/linux/unaligned/access_ok.h:42:29: error: redefinition of 
'put_unaligned_le32'

In file included from ./include/asm-generic/unaligned.h:17:0,
 from ./arch/arm/include/generated/asm/unaligned.h:1,
 from net/ipv4/tcp_input.c:76:
./include/linux/unaligned/le_struct.h:26:20: note: previous definition 
of 'put_unaligned_le32' was here

In file included from net/ipv4/tcp_input.c:79:0:
./include/linux/unaligned/access_ok.h:47:29: error: redefinition of 
'put_unaligned_le64'

In file included from ./include/asm-generic/unaligned.h:17:0,
 from ./arch/arm/include/generated/asm/unaligned.h:1,
 from net/ipv4/tcp_input.c:76:
./include/linux/unaligned/le_struct.h:31:20: note: previous definition 
of 'put_unaligned_le64' was here

In file included from net/ipv4/tcp_input.c:79:0:
./include/linux/unaligned/access_ok.h:52:29: error: redefinition of 
'put_unaligned_be16'

In file included from ./include/asm-generic/unaligned.h:18:0,
 from ./arch/arm/include/generated/asm/unaligned.h:1,
 from net/ipv4/tcp_input.c:76:
./include/linux/unaligned/be_byteshift.h:55:20: note: previous 
definition of 'put_unaligned_be16' was here

In file included from net/ipv4/tcp_input.c:79:0:
./include/linux/unaligned/access_ok.h:57:29: error: redefinition of 
'put_unaligned_be32'

In file included from ./include/asm-generic/unaligned.h:18:0,
 from ./arch/arm/include/generated/asm/unaligned.h:1,
 from net/ipv4/tcp_input.c:76:
./include/linux/unaligned/be_byteshift.h:60:20: note: previous 
definition of 'put_unaligned_be32' 

Re: [PATCH net-next 0/9] net: dsa: define port types

[Egil Hjelmeland]

> The DSA code currently has 3 bitmaps in the dsa_switch structure:
> cpu_port_mask, dsa_port_mask and enabled_port_mask.


Hi Vivien

First I must apologize to everybody for not replying in-thread. Problem
is that I was not subscribed to netdev. But now I am, so I promise it
will not happen again :-)

So to the point. I think DSA need to keep track of multicast
memberships. As it is now, dsa_switch_mdb_add() include
the CPU/DSA port(s) in the multicast. But  multicast is never
removed from the CPU/DSA port(s).

One option could be that DSA remember mulitcast memberships as bitmaps
of ports. Then it could be handy to have the CPU and DSA ports as
bitmaps too.

It might not fall out that way in the end. But anyway, my suggestion is:
Do patch 1 - 6, which isolates the drivers from the internal structure
of dsa. But hold on to 7 - 9 until there is a plan for better multicast
handling in DSA.

BR
Egil

[PATCH v2 net-next 1/2] net: dsa: lan9303: Move struct lan9303 to include/linux/dsa/lan9303.h

[Egil Hjelmeland]

The next patch require net/dsa/tag_lan9303.c to access struct lan9303.
Therefore move struct lan9303 definitions from drivers/net/dsa/lan9303.h
to new file include/linux/dsa/lan9303.h.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
Reviewed-by: Andrew Lunn <and...@lunn.ch>
---
 MAINTAINERS |  1 +
 drivers/net/dsa/lan9303.h   | 34 +-
 include/linux/dsa/lan9303.h | 36 
 3 files changed, 38 insertions(+), 33 deletions(-)
 create mode 100644 include/linux/dsa/lan9303.h

diff --git a/MAINTAINERS b/MAINTAINERS
index e3a7ca9d2783..c9ee7abf4627 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9415,6 +9415,7 @@ M:Florian Fainelli <f.faine...@gmail.com>
 S: Maintained
 F: net/dsa/
 F: include/net/dsa.h
+F: include/linux/dsa/
 F: drivers/net/dsa/
 
 NETWORKING [GENERAL]
diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
index d807b1be35f2..b868e5040830 100644
--- a/drivers/net/dsa/lan9303.h
+++ b/drivers/net/dsa/lan9303.h
@@ -2,39 +2,7 @@
 #include 
 #include 
 
-struct lan9303;
-
-struct lan9303_phy_ops {
-   /* PHY 1 and 2 access*/
-   int (*phy_read)(struct lan9303 *chip, int port, int regnum);
-   int (*phy_write)(struct lan9303 *chip, int port,
-int regnum, u16 val);
-};
-
-#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;
-   struct regmap *regmap;
-   struct regmap_irq_chip_data *irq_data;
-   struct gpio_desc *reset_gpio;
-   u32 reset_duration; /* in [ms] */
-   bool phy_addr_sel_strap;
-   struct dsa_switch *ds;
-   struct mutex indirect_mutex; /* protect indexed register access */
-   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];
-};
+#include 
 
 extern const struct regmap_access_table lan9303_register_set;
 extern const struct lan9303_phy_ops lan9303_indirect_phy_ops;
diff --git a/include/linux/dsa/lan9303.h b/include/linux/dsa/lan9303.h
new file mode 100644
index ..05d8d136baab
--- /dev/null
+++ b/include/linux/dsa/lan9303.h
@@ -0,0 +1,36 @@
+/* Included by drivers/net/dsa/lan9303.h and net/dsa/tag_lan9303.c */
+#include 
+
+struct lan9303;
+
+struct lan9303_phy_ops {
+   /* PHY 1 and 2 access*/
+   int (*phy_read)(struct lan9303 *chip, int port, int regnum);
+   int (*phy_write)(struct lan9303 *chip, int port,
+int regnum, u16 val);
+};
+
+#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;
+   struct regmap *regmap;
+   struct regmap_irq_chip_data *irq_data;
+   struct gpio_desc *reset_gpio;
+   u32 reset_duration; /* in [ms] */
+   bool phy_addr_sel_strap;
+   struct dsa_switch *ds;
+   struct mutex indirect_mutex; /* protect indexed register access */
+   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];
+};
-- 
2.11.0

[PATCH v2 net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Egil Hjelmeland]

When CPU transmit directly to port using tag, the LAN9303 does not
learn MAC addresses received on the CPU port into the ALR.
ALR learning is performed only when transmitting using ALR lookup.

Solution:
If the two external ports are bridged and the packet is not STP BPDU,
then use ALR lookup to allow ALR learning on CPU port.
Otherwise transmit directly to port with STP state override.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 net/dsa/tag_lan9303.c | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 57519597c6fc..64092325aac3 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -11,6 +11,7 @@
  * GNU General Public License for more details.
  *
  */
+#include 
 #include 
 #include 
 #include 
@@ -39,6 +40,23 @@
  */
 
 #define LAN9303_TAG_LEN 4
+# define LAN9303_TAG_TX_USE_ALR BIT(3)
+# define LAN9303_TAG_TX_STP_OVERRIDE BIT(4)
+#define eth_stp_addr eth_reserved_addr_base
+
+/* Decide whether to transmit using ALR lookup, or transmit directly to
+ * port using tag. ALR learning is performed only when using ALR lookup.
+ * If the two external ports are bridged and the packet is not STP BPDU,
+ * then use ALR lookup to allow ALR learning on CPU port.
+ * Otherwise transmit directly to port with STP state override.
+ * See also: lan9303_separate_ports() and lan9303.pdf 6.4.10.1
+ */
+static int lan9303_xmit_use_arl(struct dsa_port *dp, u8 *dest_addr)
+{
+   struct lan9303 *chip = dp->ds->priv;
+
+   return chip->is_bridged && !ether_addr_equal(dest_addr, eth_stp_addr);
+}
 
 static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device 
*dev)
 {
@@ -62,7 +80,10 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, 
struct net_device *dev)
 
lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
lan9303_tag[0] = htons(ETH_P_8021Q);
-   lan9303_tag[1] = htons(dp->index | BIT(4));
+   lan9303_tag[1] = lan9303_xmit_use_arl(dp, skb->data) ?
+   LAN9303_TAG_TX_USE_ALR :
+   dp->index | LAN9303_TAG_TX_STP_OVERRIDE;
+   lan9303_tag[1] = htons(lan9303_tag[1]);
 
return skb;
 }
-- 
2.11.0

[PATCH v2 net-next 0/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Egil Hjelmeland]

When CPU transmit directly to port using tag, the LAN9303 does not
learn MAC addresses received on the CPU port into the ALR table.
ALR learning is performed only when transmitting using ALR lookup.

Solution:
If the two external ports are bridged and the packet is not STP BPDU,
then use ALR lookup to allow ALR learning on CPU port.
Otherwise transmit directly to port with STP state override.

The first patch moves struct lan9303 to include/linux/dsa/lan9303.h in
order to prepare for the second patch. 

Changes v1 -> v2:
 - new file: include/linux/dsa/lan9303.h instead of include/linux/lan9303.h
 - include linux/if_ether.h in include/linux/dsa/lan9303.h
 - renamed lan9303_tx_use_arl to lan9303_xmit_use_arl for consistency.
 - removed inline keyword to lan9303_xmit_use_arl


Egil Hjelmeland (2):
  net: dsa: lan9303: Move struct lan9303 to include/linux/dsa/lan9303.h
  net: dsa: lan9303: Learn addresses on CPU port when bridged

 MAINTAINERS |  1 +
 drivers/net/dsa/lan9303.h   | 34 +-
 include/linux/dsa/lan9303.h | 36 
 net/dsa/tag_lan9303.c   | 23 ++-
 4 files changed, 60 insertions(+), 34 deletions(-)
 create mode 100644 include/linux/dsa/lan9303.h

-- 
2.11.0

Re: [PATCH net-next 1/2] net: dsa: lan9303: Move struct lan9303 to include/linux/lan9303.h

[Egil Hjelmeland]

On 24. okt. 2017 18:31, Andrew Lunn wrote:

On Tue, Oct 24, 2017 at 11:35:14AM +0200, Egil Hjelmeland wrote:

The next patch require net/dsa/tag_lan9303.c to access struct lan9303.
Therefore move struct lan9303 definitions from drivers/net/dsa/lan9303.h
to new file include/linux/lan9303.h.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>


O.K, so not too bad.

I am now however wondering if include/linux/dsa/lan9303.h would be
better?



Me too. I can do that.



Anyway,

Reviewed-by: Andrew Lunn <and...@lunn.ch>

 Andrew



Egil

Re: [PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Egil Hjelmeland]

Hi Woojung!

On 24. okt. 2017 19:18, woojung@microchip.com wrote:

Hi Egil,


+static inline int lan9303_tx_use_arl(struct dsa_port *dp, u8 *dest_addr)
+{
+   struct lan9303 *chip = dp->ds->priv;
+
+   return chip->is_bridged && !ether_addr_equal(dest_addr,
eth_stp_addr);
+}

  static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device
*dev)
  {
@@ -62,7 +80,10 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb,
struct net_device *dev)

lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
lan9303_tag[0] = htons(ETH_P_8021Q);
-   lan9303_tag[1] = htons(dp->index | BIT(4));
+   lan9303_tag[1] = lan9303_tx_use_arl(dp, skb->data) ?


How about using skb_mac_header(skb) than skb->data?


+   LAN9303_TAG_TX_USE_ALR :
+   dp->index |




I am not the expert here.

I see that skb_mac_header() is (skb->head + skb->mac_header). So it will
cost a few nano seconds per packet. Not the end of the world though.
But I see that other net/dsa/tag_*.c use skb->data, assuming that
skb->data point to mac header.

Anyway, it may be an idea to decrement skb->mac_header, in case the
master interface driver uses it? What about skb->mac_len?

If to use skb_mac_header() at all, I would replace all use of skb->data,
like this:

---
/* provide 'LAN9303_TAG_LEN' bytes additional space */
skb_push(skb, LAN9303_TAG_LEN);

/* make room between MACs and Ether-Type */
memmove(skb_mac_header(skb) - LAN9303_TAG_LEN, skb_mac_header(skb),
2 * ETH_ALEN);
skb->mac_header -= LAN9303_TAG_LEN;

lan9303_tag = (u16 *)(skb_mac_header(skb) + 2 * ETH_ALEN);
lan9303_tag[0] = htons(ETH_P_8021Q);
lan9303_tag[1] = lan9303_tx_use_arl(dp, skb_mac_header(skb)) ?
LAN9303_TAG_TX_USE_ALR :
dp->index | LAN9303_TAG_TX_STP_OVERRIDE;
lan9303_tag[1] = htons(lan9303_tag[1]);
---

But I will really like to hear the opinion from more people on this
before going down that road. Anyway, I think it would belong to a
separate patch.



Thanks.
Woojung




Regards
Egil

Re: [PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Egil Hjelmeland]

On 24. okt. 2017 18:33, Andrew Lunn wrote:

On Tue, Oct 24, 2017 at 11:35:15AM +0200, Egil Hjelmeland wrote:

+ */
+static inline int lan9303_tx_use_arl(struct dsa_port *dp, u8 *dest_addr)


Hi Egil

There is no need for the inline. The compiler will do that anyway, for
a function like this. >

Will fix.


 Andrew



Thanks
Egil

[PATCH net-next] net: dsa: lan9303: Do not disable switch fabric port 0 at .probe

[Egil Hjelmeland]

Make the LAN9303 work when lan9303_probe() is called twice.

For some unknown reason the LAN9303 switch fail to forward data when switch
fabric port 0 TX is disabled during probe. (Write of LAN9303_MAC_TX_CFG_0
in lan9303_disable_processing_port().)

In that situation the switch fabric seem to receive frames, because the ALR
is learning addresses. But no frames are transmitted on any of the ports.

In our system lan9303_probe() is called twice, first time
dsa_register_switch() return -EPROBE_DEFER. As an experiment, modified the
code to skip writing LAN9303_MAC_TX_CFG_0, port 0 during the first probe.
Then the switch works as expected.

Resolve the problem by not calling lan9303_disable_processing_port() on
port 0 during probe. Ports 1 and 2 are still disabled.

Although unsatisfying that the exact failure mechanism is not known,
the patch should not cause any harm.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 87f919f0e641..4c412bd52319 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -818,7 +818,7 @@ static int lan9303_disable_processing(struct lan9303 *chip)
 {
int p;
 
-   for (p = 0; p < LAN9303_NUM_PORTS; p++) {
+   for (p = 1; p < LAN9303_NUM_PORTS; p++) {
int ret = lan9303_disable_processing_port(chip, p);
 
if (ret)
-- 
2.11.0

[PATCH net-next 0/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Egil Hjelmeland]

When CPU transmit directly to port using tag, the LAN9303 does not
learn MAC addresses received on the CPU port into the ALR table.
ALR learning is performed only when transmitting using ALR lookup.

Solution:
If the two external ports are bridged and the packet is not STP BPDU,
then use ALR lookup to allow ALR learning on CPU port.
Otherwise transmit directly to port with STP state override.

The first patch moves struct lan9303 to include/linux/lan9303.h in order
to prepare for the second patch. 

Egil Hjelmeland (2):
  net: dsa: lan9303: Move struct lan9303 to include/linux/lan9303.h
  net: dsa: lan9303: Learn addresses on CPU port when bridged

 MAINTAINERS   |  1 +
 drivers/net/dsa/lan9303.h | 34 +-
 include/linux/lan9303.h   | 35 +++
 net/dsa/tag_lan9303.c | 23 ++-
 4 files changed, 59 insertions(+), 34 deletions(-)
 create mode 100644 include/linux/lan9303.h

-- 
2.11.0

[PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Egil Hjelmeland]

When CPU transmit directly to port using tag, the LAN9303 does not
learn MAC addresses received on the CPU port into the ALR.
ALR learning is performed only when transmitting using ALR lookup.

Solution:
If the two external ports are bridged and the packet is not STP BPDU,
then use ALR lookup to allow ALR learning on CPU port.
Otherwise transmit directly to port with STP state override.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 net/dsa/tag_lan9303.c | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 57519597c6fc..174721293a34 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -12,6 +12,7 @@
  *
  */
 #include 
+#include 
 #include 
 #include 
 
@@ -39,6 +40,23 @@
  */
 
 #define LAN9303_TAG_LEN 4
+# define LAN9303_TAG_TX_USE_ALR BIT(3)
+# define LAN9303_TAG_TX_STP_OVERRIDE BIT(4)
+#define eth_stp_addr eth_reserved_addr_base
+
+/* Decide whether to transmit using ALR lookup, or transmit directly to
+ * port using tag. ALR learning is performed only when using ALR lookup.
+ * If the two external ports are bridged and the packet is not STP BPDU,
+ * then use ALR lookup to allow ALR learning on CPU port.
+ * Otherwise transmit directly to port with STP state override.
+ * See also: lan9303_separate_ports() and lan9303.pdf 6.4.10.1
+ */
+static inline int lan9303_tx_use_arl(struct dsa_port *dp, u8 *dest_addr)
+{
+   struct lan9303 *chip = dp->ds->priv;
+
+   return chip->is_bridged && !ether_addr_equal(dest_addr, eth_stp_addr);
+}
 
 static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device 
*dev)
 {
@@ -62,7 +80,10 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, 
struct net_device *dev)
 
lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
lan9303_tag[0] = htons(ETH_P_8021Q);
-   lan9303_tag[1] = htons(dp->index | BIT(4));
+   lan9303_tag[1] = lan9303_tx_use_arl(dp, skb->data) ?
+   LAN9303_TAG_TX_USE_ALR :
+   dp->index | LAN9303_TAG_TX_STP_OVERRIDE;
+   lan9303_tag[1] = htons(lan9303_tag[1]);
 
return skb;
 }
-- 
2.11.0

[PATCH net-next 1/2] net: dsa: lan9303: Move struct lan9303 to include/linux/lan9303.h

[Egil Hjelmeland]

The next patch require net/dsa/tag_lan9303.c to access struct lan9303.
Therefore move struct lan9303 definitions from drivers/net/dsa/lan9303.h
to new file include/linux/lan9303.h.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 MAINTAINERS   |  1 +
 drivers/net/dsa/lan9303.h | 34 +-
 include/linux/lan9303.h   | 35 +++
 3 files changed, 37 insertions(+), 33 deletions(-)
 create mode 100644 include/linux/lan9303.h

diff --git a/MAINTAINERS b/MAINTAINERS
index e3a7ca9d2783..9535e32bd421 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9416,6 +9416,7 @@ S:Maintained
 F: net/dsa/
 F: include/net/dsa.h
 F: drivers/net/dsa/
+F: include/linux/lan9303.h
 
 NETWORKING [GENERAL]
 M: "David S. Miller" <da...@davemloft.net>
diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
index d807b1be35f2..e6675e11c833 100644
--- a/drivers/net/dsa/lan9303.h
+++ b/drivers/net/dsa/lan9303.h
@@ -2,39 +2,7 @@
 #include 
 #include 
 
-struct lan9303;
-
-struct lan9303_phy_ops {
-   /* PHY 1 and 2 access*/
-   int (*phy_read)(struct lan9303 *chip, int port, int regnum);
-   int (*phy_write)(struct lan9303 *chip, int port,
-int regnum, u16 val);
-};
-
-#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;
-   struct regmap *regmap;
-   struct regmap_irq_chip_data *irq_data;
-   struct gpio_desc *reset_gpio;
-   u32 reset_duration; /* in [ms] */
-   bool phy_addr_sel_strap;
-   struct dsa_switch *ds;
-   struct mutex indirect_mutex; /* protect indexed register access */
-   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];
-};
+#include 
 
 extern const struct regmap_access_table lan9303_register_set;
 extern const struct lan9303_phy_ops lan9303_indirect_phy_ops;
diff --git a/include/linux/lan9303.h b/include/linux/lan9303.h
new file mode 100644
index ..5810bdb43581
--- /dev/null
+++ b/include/linux/lan9303.h
@@ -0,0 +1,35 @@
+/* Included by drivers/net/dsa/lan9303.h and net/dsa/tag_lan9303.c */
+
+struct lan9303;
+
+struct lan9303_phy_ops {
+   /* PHY 1 and 2 access*/
+   int (*phy_read)(struct lan9303 *chip, int port, int regnum);
+   int (*phy_write)(struct lan9303 *chip, int port,
+int regnum, u16 val);
+};
+
+#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;
+   struct regmap *regmap;
+   struct regmap_irq_chip_data *irq_data;
+   struct gpio_desc *reset_gpio;
+   u32 reset_duration; /* in [ms] */
+   bool phy_addr_sel_strap;
+   struct dsa_switch *ds;
+   struct mutex indirect_mutex; /* protect indexed register access */
+   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];
+};
-- 
2.11.0

Re: [RFC net-next] net: dsa: lan9303 Cpu port and ARL

[Egil Hjelmeland]

Thanks for the response, Andrew!

On 23. okt. 2017 14:58, Andrew Lunn wrote:

  #include "dsa_priv.h"
+#include "../../drivers/net/dsa/lan9303.h"


Don't do that. Export the needed parts in an include file in
include/linux.



Do you mean moving struct definitions from drivers/net/dsa/lan9303.h to 
a include/linux/lan9303.h? That would be like 80% of

drivers/net/dsa/lan9303.h. I am a bit surprised that is deemed better
than the slightly ugly include.

Or do you have other suggestions?

One possibility could be to have lan9303_tx_use_arl() in
drivers/net/dsa/lan9303-core.c. But it has to be exported, and can
not be inlined.

It is also possible to replace the  chip->is_bridged term with:

  struct dsa_switch *ds = dp->ds;
  if (!dsa_to_port(ds, 1)->bridge_dev)
  return false;
  if (dsa_to_port(ds, 1)->bridge_dev != dsa_to_port(ds,2)->bridge_dev)
  return false;

But it feels silly to duplicate that calculation in the data path.



Otherwise, i think your approach is O.K.

   Andrew



Egil

[RFC net-next] net: dsa: lan9303 Cpu port and ARL

[Egil Hjelmeland]

Our proprietary lan9303 driver used .set_addr to add the master interface
MAC to the lan9303 ALR (Address Logic Resolution) table. Now that .set_addr
is gone, I found out that the lan9303 does not learn the master MAC address.
Which means that traffic to the local CPU is flooded to the other external
port as well. The problem is that when net/dsa/tag_lan9303.c use the
tagging mechanism to bypass ALR lookup, then the ALR does not learn addresses. 

We could add the static ALR entry like this:

---
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 87f919f0e641..507586c13a60 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -791,12 +791,17 @@ static int lan9303_separate_ports(struct lan9303 *chip)
 
 static void lan9303_bridge_ports(struct lan9303 *chip)
 {
+   u8 *br_mac = dsa_to_port(chip->ds, 1)->bridge_dev->dev_addr;
+
/* ports bridged: remove mirroring */
lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR,
 LAN9303_SWE_PORT_MIRROR_DISABLED);
 
lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
 chip->swe_port_state);
+
+   dev_info(chip->dev, "lan9303_bridge_ports: %pM\n", br_mac);
+   lan9303_alr_set_entry(chip, br_mac, BIT(0), false);
 }
 
 static int lan9303_handle_reset(struct lan9303 *chip)
--

That would cover our use case, and has the benefit of no added overhead in the
data path. 

However, if some other (virtual) interface is added to the SW bridge, we still
have the same problem. So I suspect that you want to see something like the 
following: 


---
 net/dsa/tag_lan9303.c | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 57519597c6fc..1003fd91755c 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -16,6 +16,7 @@
 #include 
 
 #include "dsa_priv.h"
+#include "../../drivers/net/dsa/lan9303.h"
 
 /* To define the outgoing port and to discover the incoming port a regular
  * VLAN tag is used by the LAN9303. But its VID meaning is 'special':
@@ -39,6 +40,23 @@
  */
 
 #define LAN9303_TAG_LEN 4
+# define LAN9303_TAG_TX_USE_ALR BIT(3)
+# define LAN9303_TAG_TX_STP_OVRD BIT(4)
+#define eth_stp_addr eth_reserved_addr_base
+
+/* Decide whether to transmit using ALR lookup, or transmit directly to
+ * port using tag. ALR learning is performed only when using ALR lookup.
+ * If the two external ports are bridged and the packet is not STP BPDU,
+ * then use ALR lookup to allow ALR learning on CPU port.
+ * Otherwise transmit directly to port with STP state override.
+ * See also: lan9303_separate_ports() and lan9303.pdf 6.4.10.1
+ */
+static inline int lan9303_tx_use_arl(struct dsa_port *dp, u8 *dest_addr)
+{
+   struct lan9303 *chip = dp->ds->priv;
+
+   return chip->is_bridged && !ether_addr_equal(dest_addr, eth_stp_addr);
+}
 
 static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device 
*dev)
 {
@@ -62,7 +80,10 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, 
struct net_device *dev)
 
lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
lan9303_tag[0] = htons(ETH_P_8021Q);
-   lan9303_tag[1] = htons(dp->index | BIT(4));
+   lan9303_tag[1] = lan9303_tx_use_arl(dp, skb->data) ?
+   LAN9303_TAG_TX_USE_ALR :
+   dp->index | LAN9303_TAG_TX_STP_OVRD;
+   lan9303_tag[1] = htons(lan9303_tag[1]);
 
return skb;
 }
-- 

Comments?

Egil Hjelmeland

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

[Egil Hjelmeland]

On 20. okt. 2017 16:03, Vivien Didelot wrote:

Hi Egil,

It looks much cleaner, thanks!

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


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 v2 -> v3:
  - Whitespace polishing. Removed some "section" comments.
  - Prefixed ALR constants with LAN9303_ for consistency.
  - Patch 2: lan9303_port_fast_age() wrap the "port" into a struct for passing
as context to alr_loop_cb_del_port_learned. Safer in event of type change.
  - Patch 2: Reviewed-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>


You'll usually directly add the Reviewed-by: tags right under your
Signed-off-by: tag of the concerned patches in next iterations.



Oh, I thought I had done it, but now I see I forgot to rebuild the
patches after that last minute git-amend where I put in your
Reviewed-by:.


(don't worry though I've added it back to patch 2/2.)


Thanks,

 Vivien



Egil

[PATCH v3 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 <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 173 +
 drivers/net/dsa/lan9303.h  |   9 +++
 2 files changed, 182 insertions(+)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 48cae87bdcb7..87f919f0e641 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "lan9303.h"
 
@@ -497,6 +498,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 };
 
+/* 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;
+}
+
 /* 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)
@@ -609,6 +641,73 @@ static void alr_loop_cb_fdb_port_dump(struct lan9303 
*chip, u32 dat0,
dump_ctx->cb(mac, 0, is_static, dump_ctx->data);
 }
 
+/* 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 = LAN9303_ALR_DAT1_STATIC;
+   if (port_map)
+   dat1 |= LAN9303_ALR_DAT1_VALID;
+   /* otherwise no ports: delete entry */
+   if (stp_override)
+   dat1 |= LAN9303_ALR_DAT1_AGE_OVERRID;
+
+   alr_port = portmap_2_alrport[port_map & 7];
+   dat1 &= ~LAN9303_ALR_DAT1_PORT_MASK;
+   dat1 |= alr_port << LAN9303_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(>port_map);
+   lan9303_alr_set_entry(chip, mac, entr->port_map, entr->stp_override);
+
+   return 0;
+}
+
 static int lan9303_disable_processing_port(struct lan9303 *chip,
   unsigned int port)
 {
@@ -1065,6 +1164,32 @@ static void lan9303_port_fast_age(struct dsa_switch *

[PATCH v3 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 <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 160 +
 drivers/net/dsa/lan9303.h  |   2 +
 2 files changed, 162 insertions(+)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 09a748327fc6..48cae87bdcb7 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 LAN9303_ALR_CMD_MAKE_ENTRYBIT(2)
+# define LAN9303_ALR_CMD_GET_FIRST BIT(1)
+# define LAN9303_ALR_CMD_GET_NEXT  BIT(0)
+#define LAN9303_SWE_ALR_WR_DAT_0 0x1801
+#define LAN9303_SWE_ALR_WR_DAT_1 0x1802
+# define LAN9303_ALR_DAT1_VALIDBIT(26)
+# define LAN9303_ALR_DAT1_END_OF_TABL  BIT(25)
+# define LAN9303_ALR_DAT1_AGE_OVERRID  BIT(25)
+# define LAN9303_ALR_DAT1_STATIC   BIT(24)
+# define LAN9303_ALR_DAT1_PORT_BITOFFS  16
+# define LAN9303_ALR_DAT1_PORT_MASK(7 << LAN9303_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,122 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
return 0;
 }
 
+/* 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 };
+
+/* 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,
+LAN9303_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,
+LAN9303_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, );
+   lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_1, );
+   if (dat1 & LAN9303_ALR_DAT1_END_OF_TABL)
+   break;
+
+   alrport = (dat1 & LAN9303_ALR_DAT1_PORT_MASK) >>
+   LAN9303_ALR_DAT1_PORT_BITOFFS;
+   portmap = alrport_2_portmap[alrport];
+
+   cb(chip, dat0, dat1, portmap, ctx);
+
+   lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD,
+LAN9303_ALR_CMD_GET_NEXT);
+   lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
+   }
+}
+
+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;
+}
+
+struct del_port_learned_ctx {
+   int port;
+};
+
+/* 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)
+{
+   struct del_port_learned_ctx *del_ctx = ctx;
+   int port = del_ctx->port;
+
+   if (((BIT(port) & portmap) == 0) || (dat1 & LAN9303_ALR_DAT1_STATIC))
+   return;
+
+  

[PATCH v3 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 v2 -> v3:
 - Whitespace polishing. Removed some "section" comments.
 - Prefixed ALR constants with LAN9303_ for consistency.
 - Patch 2: lan9303_port_fast_age() wrap the "port" into a struct for passing
   as context to alr_loop_cb_del_port_learned. Safer in event of type change.
 - Patch 2: Reviewed-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
  
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 | 333 +
 drivers/net/dsa/lan9303.h  |  11 ++
 2 files changed, 344 insertions(+)

-- 
2.11.0

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

[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]

On 19. okt. 2017 17:12, Vivien Didelot wrote:

Hi Egil,

Egil Hjelmeland <pri...@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 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 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 <pri...@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 <pri...@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_ENTRYBIT(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_VALIDBIT(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, );
+   lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_1, );
+   if (dat1 & ALR_DAT1_END_OF_TABL)
+  

[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 <pri...@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_ENTRYBIT(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_VALIDBIT(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, );
+   lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_1, );
+   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 */
+   lan

[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 <pri...@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 
 #include 
 #include 
+#include 
 
 #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(>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_swit

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

[Egil Hjelmeland]

On 18. okt. 2017 16:38, Vivien Didelot wrote:

Hi Egil,

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


+/* 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 */
+   /* Question: Should we delete any learned entry?
+* { lan9303_alr_set_entry(chip, mac, 0, false); return 0; }


.port_fdb_del is meant to remove the association between a port and a
MAC address in a given forwarding database. Deleting any learned entry
is therefore out of scope of this function.

Please mark such patchset as RFC next time so that the maintainer knows
that it is not meant to be applied.


Thanks. And I will keep the RFC remark in mind.


+*/
+
+   entr->port_map &= ~BIT(port);
+   if (entr->port_map == 0) /* zero means its free again */
+   eth_zero_addr(>port_map);
+   lan9303_alr_set_entry(chip, mac, entr->port_map, entr->stp_override);
+   return 0;
+}


...


+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;
+}


I don't remember, this chip has a single forwarding database for the
whole switch, is that correct?


Correct.

And the forwarding database (ALR) does not handle VLAN.
VLAN filtering is a separate step, with its own table.



Thanks,

 Vivien

[PATCH 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 I have have a question in the second patch, in lan9303_alr_del_port.

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!

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 | 334 +
 drivers/net/dsa/lan9303.h  |  11 ++
 2 files changed, 345 insertions(+)

-- 
2.11.0

[PATCH 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 <pri...@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 fecfe1fe67ea..8b5202f3c0b0 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_ENTRYBIT(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_VALIDBIT(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, );
+   lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_1, );
+   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 */
+   lan

[PATCH 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 <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 175 +
 drivers/net/dsa/lan9303.h  |   9 +++
 2 files changed, 184 insertions(+)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 8b5202f3c0b0..4177e9d2e8ae 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #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,76 @@ 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 */
+   /* Question: Should we delete any learned entry?
+* { lan9303_alr_set_entry(chip, mac, 0, false); return 0; }
+*/
+
+   entr->port_map &= ~BIT(port);
+   if (entr->port_map == 0) /* zero means its free again */
+   eth_zero_addr(>port_map);
+   lan9303_alr_set_entry(chip, mac, entr->port_map, entr->stp_override);
+   return 0;
+}
+
 /* - Various chip setup ---

Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging

[Egil Hjelmeland]

On 10. okt. 2017 17:51, woojung@microchip.com wrote:

Specific reason to use val then using

LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0

like previous line?


Specific reason was to please a reviewer that did not like my
indenting in first version. I did not agree with him, but since
nobody else spoke up, I changed the code.

Got it. Missed previous patch/comment.


@@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds)
return -EINVAL;
}

+   ret = lan9303_setup_tagging(chip);
+   if (ret)
+   dev_err(chip->dev, "failed to setup port tagging %d\n", ret);
+

Still move on when error happens?


Good question. I just followed the pattern from the original function,
which was not made by me. Actually I did once reflect on whether this
was the correct way. Perhaps it could be argued that it is better to
allow the device to come up, so the problem can be investigated?

Maybe depends on severity of setting?
BTW, lan9303_setup() still returns ZERO at the end?

I did quick survey of the _setup functions of the other dsa drivers.
Some return on error, some ignore errors.
If you think so, I can make a v3 series that return on error. Otherwise
I leave it as it is.



Thanks.
Woojung



Thank you for polite review.

Egil

Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging

[Egil Hjelmeland]

On 10. okt. 2017 17:14, woojung@microchip.com wrote:

+/* forward special tagged packets from port 0 to port 1 *or* port 2 */
+static int lan9303_setup_tagging(struct lan9303 *chip)
+{
+   int ret;
+   u32 val;
+   /* enable defining the destination port via special VLAN tagging
+* for port 0
+*/
+   ret = lan9303_write_switch_reg(chip,
LAN9303_SWE_INGRESS_PORT_TYPE,
+
LAN9303_SWE_INGRESS_PORT_TYPE_VLAN);
+   if (ret)
+   return ret;
+
+   /* tag incoming packets at port 1 and 2 on their way to port 0 to be
+* able to discover their source port
+*/
+   val = LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0;
+   return lan9303_write_switch_reg(chip,
LAN9303_BM_EGRSS_PORT_TYPE, val);

Specific reason to use val then using 
LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0
like previous line?


Specific reason was to please a reviewer that did not like my
indenting in first version. I did not agree with him, but since
nobody else spoke up, I changed the code.


@@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds)
return -EINVAL;
}

+   ret = lan9303_setup_tagging(chip);
+   if (ret)
+   dev_err(chip->dev, "failed to setup port tagging %d\n", ret);
+

Still move on when error happens?


Good question. I just followed the pattern from the original function,
which was not made by me. Actually I did once reflect on whether this 
was the correct way. Perhaps it could be argued that it is better to 
allow the device to come up, so the problem can be investigated?



ret = lan9303_separate_ports(chip);
if (ret)
dev_err(chip->dev, "failed to separate ports %d\n", ret);
--
2.11.0


- Woojung

[PATCH v2 net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

When both user ports are joined to the same bridge, the normal
HW MAC learning is enabled. This means that unicast traffic is forwarded
in HW.

If one of the user ports leave the bridge,
the ports goes back to the initial separated operation.

Port separation relies on disabled HW MAC learning. Hence the condition
that both ports must join same bridge.

Add brigde methods port_bridge_join, port_bridge_leave and
port_stp_state_set.

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 2215ec1fbe1e..fecfe1fe67ea 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "lan9303.h"
 
@@ -146,6 +147,7 @@
 # define LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 (0)
 # define LAN9303_SWE_PORT_STATE_LEARNING_PORT0 BIT(1)
 # define LAN9303_SWE_PORT_STATE_BLOCKING_PORT0 BIT(0)
+# define LAN9303_SWE_PORT_STATE_DISABLED_PORT0 (3)
 #define LAN9303_SWE_PORT_MIRROR 0x1846
 # define LAN9303_SWE_PORT_MIRROR_SNIFF_ALL BIT(8)
 # define LAN9303_SWE_PORT_MIRROR_SNIFFER_PORT2 BIT(7)
@@ -156,6 +158,7 @@
 # define LAN9303_SWE_PORT_MIRROR_MIRRORED_PORT0 BIT(2)
 # define LAN9303_SWE_PORT_MIRROR_ENABLE_RX_MIRRORING BIT(1)
 # define LAN9303_SWE_PORT_MIRROR_ENABLE_TX_MIRRORING BIT(0)
+# define LAN9303_SWE_PORT_MIRROR_DISABLED 0
 #define LAN9303_SWE_INGRESS_PORT_TYPE 0x1847
 #define  LAN9303_SWE_INGRESS_PORT_TYPE_VLAN 3
 #define LAN9303_BM_CFG 0x1c00
@@ -556,6 +559,16 @@ static int lan9303_separate_ports(struct lan9303 *chip)
LAN9303_SWE_PORT_STATE_BLOCKING_PORT2);
 }
 
+static void lan9303_bridge_ports(struct lan9303 *chip)
+{
+   /* ports bridged: remove mirroring */
+   lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR,
+LAN9303_SWE_PORT_MIRROR_DISABLED);
+
+   lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
+chip->swe_port_state);
+}
+
 static int lan9303_handle_reset(struct lan9303 *chip)
 {
if (!chip->reset_gpio)
@@ -844,6 +857,72 @@ static void lan9303_port_disable(struct dsa_switch *ds, 
int port,
}
 }
 
+static int lan9303_port_bridge_join(struct dsa_switch *ds, int port,
+   struct net_device *br)
+{
+   struct lan9303 *chip = ds->priv;
+
+   dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
+   if (ds->ports[1].bridge_dev ==  ds->ports[2].bridge_dev) {
+   lan9303_bridge_ports(chip);
+   chip->is_bridged = true;  /* unleash stp_state_set() */
+   }
+
+   return 0;
+}
+
+static void lan9303_port_bridge_leave(struct dsa_switch *ds, int port,
+ struct net_device *br)
+{
+   struct lan9303 *chip = ds->priv;
+
+   dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
+   if (chip->is_bridged) {
+   lan9303_separate_ports(chip);
+   chip->is_bridged = false;
+   }
+}
+
+static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
+  u8 state)
+{
+   int portmask, portstate;
+   struct lan9303 *chip = ds->priv;
+
+   dev_dbg(chip->dev, "%s(port %d, state %d)\n",
+   __func__, port, state);
+
+   switch (state) {
+   case BR_STATE_DISABLED:
+   portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
+   break;
+   case BR_STATE_BLOCKING:
+   case BR_STATE_LISTENING:
+   portstate = LAN9303_SWE_PORT_STATE_BLOCKING_PORT0;
+   break;
+   case BR_STATE_LEARNING:
+   portstate = LAN9303_SWE_PORT_STATE_LEARNING_PORT0;
+   break;
+   case BR_STATE_FORWARDING:
+   portstate = LAN9303_SWE_PORT_STATE_FORWARDING_PORT0;
+   break;
+   default:
+   portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
+   dev_err(chip->dev, "unknown stp state: port %d, state %d\n",
+   port, state);
+   }
+
+   portmask = 0x3 << (port * 2);
+   portstate <<= (port * 2);
+
+   chip->swe_port_state = (chip->swe_port_state & ~portmask) | portstate;
+
+   if (chip->is_bridged)
+   lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
+chip->swe_port_state);
+   /* else: touching SWE_PORT_STATE would break port separation */
+}
+
 static const struct dsa_switch_ops lan9303_switch_ops = {
.get_tag_protocol = lan9303_get_tag_protocol,
.setup = lan9303_setup,
@@ -855,6 +934,9 @@ static const struct dsa_

[PATCH v2 net-next 0/2] lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

This series add basic offloading of unicast traffic to the lan9303
DSA driver.

Review welcome!
 
Changes v1 -> v2:
 - Patch 1: Codestyle linting.
 - Patch 2: Remember SWE_PORT_STATE while not bridged.
Added constant LAN9303_SWE_PORT_MIRROR_DISABLED.


Egil Hjelmeland (2):
  net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging
  net: dsa: lan9303: Add basic offloading of unicast traffic

 drivers/net/dsa/lan9303-core.c | 124 +++--
 drivers/net/dsa/lan9303.h  |   2 +
 2 files changed, 109 insertions(+), 17 deletions(-)

-- 
2.11.0

[PATCH v2 net-next 1/2] net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging

[Egil Hjelmeland]

Prepare for next patch:
Move tag setup from lan9303_separate_ports() to new function
lan9303_setup_tagging()

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 42 +-
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 07355db2ad81..2215ec1fbe1e 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -157,6 +157,7 @@
 # define LAN9303_SWE_PORT_MIRROR_ENABLE_RX_MIRRORING BIT(1)
 # define LAN9303_SWE_PORT_MIRROR_ENABLE_TX_MIRRORING BIT(0)
 #define LAN9303_SWE_INGRESS_PORT_TYPE 0x1847
+#define  LAN9303_SWE_INGRESS_PORT_TYPE_VLAN 3
 #define LAN9303_BM_CFG 0x1c00
 #define LAN9303_BM_EGRSS_PORT_TYPE 0x1c0c
 # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT2 (BIT(17) | BIT(16))
@@ -510,11 +511,30 @@ static int lan9303_enable_processing_port(struct lan9303 
*chip,
LAN9303_MAC_TX_CFG_X_TX_ENABLE);
 }
 
+/* forward special tagged packets from port 0 to port 1 *or* port 2 */
+static int lan9303_setup_tagging(struct lan9303 *chip)
+{
+   int ret;
+   u32 val;
+   /* enable defining the destination port via special VLAN tagging
+* for port 0
+*/
+   ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
+  LAN9303_SWE_INGRESS_PORT_TYPE_VLAN);
+   if (ret)
+   return ret;
+
+   /* tag incoming packets at port 1 and 2 on their way to port 0 to be
+* able to discover their source port
+*/
+   val = LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0;
+   return lan9303_write_switch_reg(chip, LAN9303_BM_EGRSS_PORT_TYPE, val);
+}
+
 /* We want a special working switch:
  * - do not forward packets between port 1 and 2
  * - forward everything from port 1 to port 0
  * - forward everything from port 2 to port 0
- * - forward special tagged packets from port 0 to port 1 *or* port 2
  */
 static int lan9303_separate_ports(struct lan9303 *chip)
 {
@@ -529,22 +549,6 @@ static int lan9303_separate_ports(struct lan9303 *chip)
if (ret)
return ret;
 
-   /* enable defining the destination port via special VLAN tagging
-* for port 0
-*/
-   ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
-  0x03);
-   if (ret)
-   return ret;
-
-   /* tag incoming packets at port 1 and 2 on their way to port 0 to be
-* able to discover their source port
-*/
-   ret = lan9303_write_switch_reg(chip, LAN9303_BM_EGRSS_PORT_TYPE,
-   LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0);
-   if (ret)
-   return ret;
-
/* prevent port 1 and 2 from forwarding packets by their own */
return lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 |
@@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds)
return -EINVAL;
}
 
+   ret = lan9303_setup_tagging(chip);
+   if (ret)
+   dev_err(chip->dev, "failed to setup port tagging %d\n", ret);
+
ret = lan9303_separate_ports(chip);
if (ret)
dev_err(chip->dev, "failed to separate ports %d\n", ret);
-- 
2.11.0

Re: [PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

Den 23. sep. 2017 16:31, skrev Andrew Lunn:

The point is: Once both external ports are in "forwarding", I see no way
to prevent traffic flowing directly between the external ports.


Generally, there are port vectors. Port X can send frames only to Port
Y.

If you don't have that, there are possibilities with VLANs. Each port
is given a unique VLAN. All incoming untagged traffic is tagged with
the VLAN. You just need to keep the VLAN separated and add/remove the
VLAN tag in the dsa tag driver.

  Andrew


Thanks. The lan9303 has nothing like "port vectors". The port tagging
scheme is VLAN based, but is does not prevent direct forwarding between
the external ports.

In order to not break the strong port separation in the current driver;
I will stick to my solution, and only add caching of the STP state
register.

Egil

Re: [PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

Den 22. sep. 2017 22:08, skrev Andrew Lunn:

I'm wondering how this is supposed to work. Please add a good comment
here, since the hardware is forcing you to do something odd.

Maybe it would be a good idea to save the STP state in chip.  And then
when chip->is_bridged is set true, change the state in the hardware to
the saved value?

What happens when port 0 is added to the bridge, there is then a
minute pause and then port 1 is added? I would expect that as soon as
port 0 is added, the STP state machine for port 0 will start and move
into listening and then forwarding. Due to hardware limitations it
looks like you cannot do this. So what state is the hardware
effectively in? Blocking? Forwarding?

Then port 1 is added. You can then can respect the states. port 1 will
do blocking->listening->forwarding, but what about port 0? The calls
won't get repeated? How does it transition to forwarding?

   Andrew



I see your point with the "minute pause" argument. Although a bit
contrived use case, it is easy to fix by caching the STP state, as
you suggest. So I can do that.


I don't think it is contrived. I've done bridge configuration by hand
for testing purposes. I've also set the forwarding delay to very small
values, so there is a clear race condition here.


How does other DSA HW chips handle port separation? Knowing that
could perhaps help me know what to look for.


They have better hardware :-)

Generally each port is totally independent. You can change the STP
state per port without restrictions.


We can indeed change the STP state per lan9303 port "without
restrictions".

The point is: Once both external ports are in "forwarding", I see no way
to prevent traffic flowing directly between the external ports.



   Andrew



Egil

Re: [PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

Den 21. sep. 2017 16:26, skrev Vivien Didelot:

Hi Egil,

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


When both user ports are joined to the same bridge, the normal
HW MAC learning is enabled. This means that unicast traffic is forwarded
in HW.

If one of the user ports leave the bridge,
the ports goes back to the initial separated operation.

Port separation relies on disabled HW MAC learning. Hence the condition
that both ports must join same bridge.

Add brigde methods port_bridge_join, port_bridge_leave and
port_stp_state_set.

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>


Styling nitpicks below, other than that, the patch LGTM:

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


  #include 
  #include 
  #include 
+#include 


It's nice to order header inclusions alphabetically.

  
  #include "lan9303.h"
  
@@ -146,6 +147,7 @@

  # define LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 (0)
  # define LAN9303_SWE_PORT_STATE_LEARNING_PORT0 BIT(1)
  # define LAN9303_SWE_PORT_STATE_BLOCKING_PORT0 BIT(0)
+# define LAN9303_SWE_PORT_STATE_DISABLED_PORT0 (3)
  #define LAN9303_SWE_PORT_MIRROR 0x1846
  # define LAN9303_SWE_PORT_MIRROR_SNIFF_ALL BIT(8)
  # define LAN9303_SWE_PORT_MIRROR_SNIFFER_PORT2 BIT(7)
@@ -431,6 +433,20 @@ static int lan9303_read_switch_reg(struct lan9303 *chip, 
u16 regnum, u32 *val)
return ret;
  }
  
+static int lan9303_write_switch_reg_mask(

+   struct lan9303 *chip, u16 regnum, u32 val, u32 mask)


That is unlikely to respect the Kernel Coding Style. Please fill the
line as much as possible and align with the opening parenthesis:

 static int lan9303_write_switch_reg_mask(struct lan9303 *chip, u16 regnum,
  u32 val, u32 mask)


OK. Probably this function will go away in v2 due to Andrews comment.


+{
+   int ret;
+   u32 reg;
+
+   ret = lan9303_read_switch_reg(chip, regnum, );
+   if (ret)
+   return ret;
+   reg = (reg & ~mask) | val;
+
+   return lan9303_write_switch_reg(chip, regnum, reg);
+}


...


+
+   portmask = 0x3 << (port * 2);
+   portstate <<= (port * 2);


Unnecessary alignment, please remove the extra space characters.



I think the alignment makes the logic more clear.



+   lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
+ portstate, portmask);
+}
+
  static const struct dsa_switch_ops lan9303_switch_ops = {
.get_tag_protocol = lan9303_get_tag_protocol,
.setup = lan9303_setup,
@@ -855,6 +940,9 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
.get_sset_count = lan9303_get_sset_count,
.port_enable = lan9303_port_enable,
.port_disable = lan9303_port_disable,
+   .port_bridge_join   = lan9303_port_bridge_join,
+   .port_bridge_leave  = lan9303_port_bridge_leave,
+   .port_stp_state_set = lan9303_port_stp_state_set,


Same here, alignment unnecessary, especially since the rest of the
structure does not do that.


  };
  
  static int lan9303_register_switch(struct lan9303 *chip)

diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
index 4d8be555ff4d..5be246f05965 100644
--- a/drivers/net/dsa/lan9303.h
+++ b/drivers/net/dsa/lan9303.h
@@ -21,6 +21,7 @@ struct lan9303 {
struct dsa_switch *ds;
struct mutex indirect_mutex; /* protect indexed register access */
const struct lan9303_phy_ops *ops;
+   bool is_bridged; /* true if port 1 and 2 is bridged */
  };
  
  extern const struct regmap_access_table lan9303_register_set;


Please use the checkpatch.pl script to ensure your patch respects the
kernel conventions before submitting, it can spot nice stuffs!


I have checked _every_ patch with checkpatch.pl and weeded all warnings
before I submitted them.



I use a Git alias(*) to check a commit which does basically this:

 git format-patch --stdout -1 | ./scripts/checkpatch.pl -


(*) in details, especially convenient during interactive rebases:

 $ git config alias.checkcommit '!f () { git format-patch --stdout -1 
${1:-HEAD} | ./scripts/checkpatch.pl - ; }; f'
 $ git checkcommit # i.e. current one
 $ git checkcommit HEAD^^
 $ git checkcommit d329ac88eb21
 ...


Thanks,

 Vivien

Re: [PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

Den 21. sep. 2017 16:21, skrev Andrew Lunn:

Hi Egil


+static void lan9303_bridge_ports(struct lan9303 *chip)
+{
+   /* ports bridged: remove mirroring */
+   lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR, 0);
+}


Could you replace the 0 with something symbolic which makes this
easier to understand.

#define LAN9303_SWE_PORT_MIRROR_DISABLED 0



OK


+
  static int lan9303_handle_reset(struct lan9303 *chip)
  {
if (!chip->reset_gpio)
@@ -844,6 +866,69 @@ static void lan9303_port_disable(struct dsa_switch *ds, 
int port,
}
  }
  
+static int lan9303_port_bridge_join(struct dsa_switch *ds, int port,

+   struct net_device *br)
+{
+   struct lan9303 *chip = ds->priv;
+
+   dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
+   if (ds->ports[1].bridge_dev ==  ds->ports[2].bridge_dev) {
+   lan9303_bridge_ports(chip);
+   chip->is_bridged = true;  /* unleash stp_state_set() */
+   }
+
+   return 0;
+}
+
+static void lan9303_port_bridge_leave(struct dsa_switch *ds, int port,
+ struct net_device *br)
+{
+   struct lan9303 *chip = ds->priv;
+
+   dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
+   if (chip->is_bridged) {
+   lan9303_separate_ports(chip);
+   chip->is_bridged = false;
+   }
+}
+
+static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
+  u8 state)
+{
+   int portmask, portstate;
+   struct lan9303 *chip = ds->priv;
+
+   dev_dbg(chip->dev, "%s(port %d, state %d)\n",
+   __func__, port, state);
+   if (!chip->is_bridged)
+   return; /* touching SWE_PORT_STATE will break port separation */


I'm wondering how this is supposed to work. Please add a good comment
here, since the hardware is forcing you to do something odd.

Maybe it would be a good idea to save the STP state in chip.  And then
when chip->is_bridged is set true, change the state in the hardware to
the saved value?

What happens when port 0 is added to the bridge, there is then a
minute pause and then port 1 is added? I would expect that as soon as
port 0 is added, the STP state machine for port 0 will start and move
into listening and then forwarding. Due to hardware limitations it
looks like you cannot do this. So what state is the hardware
effectively in? Blocking? Forwarding?

Then port 1 is added. You can then can respect the states. port 1 will
do blocking->listening->forwarding, but what about port 0? The calls
won't get repeated? How does it transition to forwarding?

   Andrew



I see your point with the "minute pause" argument. Although a bit
contrived use case, it is easy to fix by caching the STP state, as
you suggest. So I can do that.

The port separation method is from the original version of the driver,
not by me.

I have read through the datasheet to see if there are other ways
to make port separation, but I could not find anything.
If anybody care to read through the 300+ page lan9303.pdf and prove
me wrong, I am happy to do it differently.

How does other DSA HW chips handle port separation? Knowing that
could perhaps help me know what to look for.

Egil
'


+
+   switch (state) {
+   case BR_STATE_DISABLED:
+   portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
+   break;
+   case BR_STATE_BLOCKING:
+   case BR_STATE_LISTENING:
+   portstate = LAN9303_SWE_PORT_STATE_BLOCKING_PORT0;
+   break;
+   case BR_STATE_LEARNING:
+   portstate = LAN9303_SWE_PORT_STATE_LEARNING_PORT0;
+   break;
+   case BR_STATE_FORWARDING:
+   portstate = LAN9303_SWE_PORT_STATE_FORWARDING_PORT0;
+   break;
+   default:
+   portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
+   dev_err(chip->dev, "unknown stp state: port %d, state %d\n",
+   port, state);
+   }
+
+   portmask = 0x3 << (port * 2);
+   portstate <<= (port * 2);
+   lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
+ portstate, portmask);
+}

[PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

When both user ports are joined to the same bridge, the normal
HW MAC learning is enabled. This means that unicast traffic is forwarded
in HW.

If one of the user ports leave the bridge,
the ports goes back to the initial separated operation.

Port separation relies on disabled HW MAC learning. Hence the condition
that both ports must join same bridge.

Add brigde methods port_bridge_join, port_bridge_leave and
port_stp_state_set.

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index bba875e114e7..76112674fe6a 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "lan9303.h"
 
@@ -146,6 +147,7 @@
 # define LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 (0)
 # define LAN9303_SWE_PORT_STATE_LEARNING_PORT0 BIT(1)
 # define LAN9303_SWE_PORT_STATE_BLOCKING_PORT0 BIT(0)
+# define LAN9303_SWE_PORT_STATE_DISABLED_PORT0 (3)
 #define LAN9303_SWE_PORT_MIRROR 0x1846
 # define LAN9303_SWE_PORT_MIRROR_SNIFF_ALL BIT(8)
 # define LAN9303_SWE_PORT_MIRROR_SNIFFER_PORT2 BIT(7)
@@ -431,6 +433,20 @@ static int lan9303_read_switch_reg(struct lan9303 *chip, 
u16 regnum, u32 *val)
return ret;
 }
 
+static int lan9303_write_switch_reg_mask(
+   struct lan9303 *chip, u16 regnum, u32 val, u32 mask)
+{
+   int ret;
+   u32 reg;
+
+   ret = lan9303_read_switch_reg(chip, regnum, );
+   if (ret)
+   return ret;
+   reg = (reg & ~mask) | val;
+
+   return lan9303_write_switch_reg(chip, regnum, reg);
+}
+
 static int lan9303_write_switch_port(struct lan9303 *chip, int port,
 u16 regnum, u32 val)
 {
@@ -556,6 +572,12 @@ static int lan9303_separate_ports(struct lan9303 *chip)
LAN9303_SWE_PORT_STATE_BLOCKING_PORT2);
 }
 
+static void lan9303_bridge_ports(struct lan9303 *chip)
+{
+   /* ports bridged: remove mirroring */
+   lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR, 0);
+}
+
 static int lan9303_handle_reset(struct lan9303 *chip)
 {
if (!chip->reset_gpio)
@@ -844,6 +866,69 @@ static void lan9303_port_disable(struct dsa_switch *ds, 
int port,
}
 }
 
+static int lan9303_port_bridge_join(struct dsa_switch *ds, int port,
+   struct net_device *br)
+{
+   struct lan9303 *chip = ds->priv;
+
+   dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
+   if (ds->ports[1].bridge_dev ==  ds->ports[2].bridge_dev) {
+   lan9303_bridge_ports(chip);
+   chip->is_bridged = true;  /* unleash stp_state_set() */
+   }
+
+   return 0;
+}
+
+static void lan9303_port_bridge_leave(struct dsa_switch *ds, int port,
+ struct net_device *br)
+{
+   struct lan9303 *chip = ds->priv;
+
+   dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
+   if (chip->is_bridged) {
+   lan9303_separate_ports(chip);
+   chip->is_bridged = false;
+   }
+}
+
+static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
+  u8 state)
+{
+   int portmask, portstate;
+   struct lan9303 *chip = ds->priv;
+
+   dev_dbg(chip->dev, "%s(port %d, state %d)\n",
+   __func__, port, state);
+   if (!chip->is_bridged)
+   return; /* touching SWE_PORT_STATE will break port separation */
+
+   switch (state) {
+   case BR_STATE_DISABLED:
+   portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
+   break;
+   case BR_STATE_BLOCKING:
+   case BR_STATE_LISTENING:
+   portstate = LAN9303_SWE_PORT_STATE_BLOCKING_PORT0;
+   break;
+   case BR_STATE_LEARNING:
+   portstate = LAN9303_SWE_PORT_STATE_LEARNING_PORT0;
+   break;
+   case BR_STATE_FORWARDING:
+   portstate = LAN9303_SWE_PORT_STATE_FORWARDING_PORT0;
+   break;
+   default:
+   portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
+   dev_err(chip->dev, "unknown stp state: port %d, state %d\n",
+   port, state);
+   }
+
+   portmask = 0x3 << (port * 2);
+   portstate <<= (port * 2);
+   lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
+ portstate, portmask);
+}
+
 static const struct dsa_switch_ops lan9303_switch_ops = {
.get_tag_protocol = lan9303_get_tag_protocol,
.setup = lan9303_setup,
@@ -855,6 +940,9 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
 

[PATCH net-next 1/2] net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging

[Egil Hjelmeland]

Prepare for next patch:
Move tag setup from lan9303_separate_ports() to new function
lan9303_setup_tagging()

Signed-off-by: Egil Hjelmeland <pri...@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 42 +-
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 07355db2ad81..bba875e114e7 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -157,6 +157,7 @@
 # define LAN9303_SWE_PORT_MIRROR_ENABLE_RX_MIRRORING BIT(1)
 # define LAN9303_SWE_PORT_MIRROR_ENABLE_TX_MIRRORING BIT(0)
 #define LAN9303_SWE_INGRESS_PORT_TYPE 0x1847
+#define  LAN9303_SWE_INGRESS_PORT_TYPE_VLAN 3
 #define LAN9303_BM_CFG 0x1c00
 #define LAN9303_BM_EGRSS_PORT_TYPE 0x1c0c
 # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT2 (BIT(17) | BIT(16))
@@ -510,11 +511,30 @@ static int lan9303_enable_processing_port(struct lan9303 
*chip,
LAN9303_MAC_TX_CFG_X_TX_ENABLE);
 }
 
+/* forward special tagged packets from port 0 to port 1 *or* port 2 */
+static int lan9303_setup_tagging(struct lan9303 *chip)
+{
+   int ret;
+   /* enable defining the destination port via special VLAN tagging
+* for port 0
+*/
+   ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
+  LAN9303_SWE_INGRESS_PORT_TYPE_VLAN);
+   if (ret)
+   return ret;
+
+   /* tag incoming packets at port 1 and 2 on their way to port 0 to be
+* able to discover their source port
+*/
+   return lan9303_write_switch_reg(
+   chip, LAN9303_BM_EGRSS_PORT_TYPE,
+   LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0);
+}
+
 /* We want a special working switch:
  * - do not forward packets between port 1 and 2
  * - forward everything from port 1 to port 0
  * - forward everything from port 2 to port 0
- * - forward special tagged packets from port 0 to port 1 *or* port 2
  */
 static int lan9303_separate_ports(struct lan9303 *chip)
 {
@@ -529,22 +549,6 @@ static int lan9303_separate_ports(struct lan9303 *chip)
if (ret)
return ret;
 
-   /* enable defining the destination port via special VLAN tagging
-* for port 0
-*/
-   ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
-  0x03);
-   if (ret)
-   return ret;
-
-   /* tag incoming packets at port 1 and 2 on their way to port 0 to be
-* able to discover their source port
-*/
-   ret = lan9303_write_switch_reg(chip, LAN9303_BM_EGRSS_PORT_TYPE,
-   LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0);
-   if (ret)
-   return ret;
-
/* prevent port 1 and 2 from forwarding packets by their own */
return lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 |
@@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds)
return -EINVAL;
}
 
+   ret = lan9303_setup_tagging(chip);
+   if (ret)
+   dev_err(chip->dev, "failed to setup port tagging %d\n", ret);
+
ret = lan9303_separate_ports(chip);
if (ret)
dev_err(chip->dev, "failed to separate ports %d\n", ret);
-- 
2.11.0

[PATCH net-next 0/2] lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

This series add basic offloading of unicast traffic to the lan9303
DSA driver.

Comments welcome!

Egil Hjelmeland (2):
  net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging
  net: dsa: lan9303: Add basic offloading of unicast traffic

 drivers/net/dsa/lan9303-core.c | 130 +++--
 drivers/net/dsa/lan9303.h  |   1 +
 2 files changed, 114 insertions(+), 17 deletions(-)

-- 
2.11.0