[PATCH net-next v2 00/10] net: dsa: lan9303: unicast offload, fdb,mdb,STP

[PATCH net-next v2 00/10] net: dsa: lan9303: unicast offload, fdb,mdb,STP

[Egil Hjelmeland]

This series extends the LAN9303 3 port switch DSA driver. Highlights:
 - Make the MDIO interface work
 - Bridging: Unicast offload
 - Bridging: Added fdb/mdb handling
 - Bridging: STP support
 - Documentation


Changes v1 -> v2:
- sorted out emailing issues, threading and date. And sent from private
  account in order to avoid company disclaimer in emails.
- Removed the three last "work around" patches. But first moved one doc 
  paragraph to the document patch.  

Egil Hjelmeland (10):
  net: dsa: lan9303: Fixed MDIO interface
  net: dsa: lan9303: Do not disable/enable switch fabric port 0 at
    startup
  net: dsa: lan9303: Refactor lan9303_enable_packet_processing()
  net: dsa: lan9303: Added adjust_link() method
  net: dsa: added dsa_net_device_to_dsa_port()
  net: dsa: lan9303: added sysfs node swe_bcst_throt
  net: dsa: lan9303: Added basic offloading of unicast traffic
  net: dsa: lan9303: Added ALR/fdb/mdb handling
  net: dsa: lan9303: Added Documentation/networking/dsa/lan9303.txt
  net: dsa: lan9303: Only allocate 3 ports

 Documentation/networking/dsa/lan9303.txt |  63 +++
 drivers/net/dsa/lan9303-core.c           | 709 ++++++++++++++++++++++++++++---
 drivers/net/dsa/lan9303.h                |  23 +
 drivers/net/dsa/lan9303_i2c.c            |   2 +
 drivers/net/dsa/lan9303_mdio.c           |  34 ++
 include/net/dsa.h                        |   1 +
 net/dsa/slave.c                          |  10 +
 7 files changed, 772 insertions(+), 70 deletions(-)
 create mode 100644 Documentation/networking/dsa/lan9303.txt

-- 
2.11.0

[PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

[Egil Hjelmeland]

Fixes after testing on actual HW:

- lan9303_mdio_write()/_read() must multiply register number
  by 4 to get offset

- Indirect access (PMI) to phy register only work in I2C mode. In
  MDIO mode phy registers must be accessed directly. Introduced
  struct lan9303_phy_ops to handle the two modes. Renamed functions
  to clarify.

- lan9303_detect_phy_setup() : Failed MDIO read return 0xffff.
  Handle that.

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 42 +++++++++++++++++++++++++++---------------
 drivers/net/dsa/lan9303.h      | 11 +++++++++++
 drivers/net/dsa/lan9303_i2c.c  |  2 ++
 drivers/net/dsa/lan9303_mdio.c | 34 ++++++++++++++++++++++++++++++++++
 4 files changed, 74 insertions(+), 15 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index cd76e61f1fca..e622db586c3d 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -20,6 +20,9 @@
 
 #include "lan9303.h"
 
+/* 13.2 System Control and Status Registers
+ * Multiply register number by 4 to get address offset.
+ */
 #define LAN9303_CHIP_REV 0x14
 # define LAN9303_CHIP_ID 0x9303
 #define LAN9303_IRQ_CFG 0x15
@@ -53,6 +56,9 @@
 #define LAN9303_VIRT_PHY_BASE 0x70
 #define LAN9303_VIRT_SPECIAL_CTRL 0x77
 
+/*13.4 Switch Fabric Control and Status Registers
+ * Accessed indirectly via SWITCH_CSR_CMD, SWITCH_CSR_DATA.
+ */
 #define LAN9303_SW_DEV_ID 0x0000
 #define LAN9303_SW_RESET 0x0001
 #define LAN9303_SW_RESET_RESET BIT(0)
@@ -242,7 +248,7 @@ static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val)
 	return regmap_write(chip->regmap, LAN9303_VIRT_PHY_BASE + regnum, val);
 }
 
-static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
+static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)
 {
 	int ret, i;
 	u32 reg;
@@ -262,7 +268,7 @@ static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
 	return -EIO;
 }
 
-static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
+static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int regnum)
 {
 	int ret;
 	u32 val;
@@ -272,7 +278,7 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
 
 	mutex_lock(&chip->indirect_mutex);
 
-	ret = lan9303_port_phy_reg_wait_for_completion(chip);
+	ret = lan9303_indirect_phy_wait_for_completion(chip);
 	if (ret)
 		goto on_error;
 
@@ -281,7 +287,7 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
 	if (ret)
 		goto on_error;
 
-	ret = lan9303_port_phy_reg_wait_for_completion(chip);
+	ret = lan9303_indirect_phy_wait_for_completion(chip);
 	if (ret)
 		goto on_error;
 
@@ -299,8 +305,8 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
 	return ret;
 }
 
-static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
-				 unsigned int val)
+static int lan9303_indirect_phy_write(struct lan9303 *chip, int addr,
+				      int regnum, u16 val)
 {
 	int ret;
 	u32 reg;
@@ -311,7 +317,7 @@ static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
 
 	mutex_lock(&chip->indirect_mutex);
 
-	ret = lan9303_port_phy_reg_wait_for_completion(chip);
+	ret = lan9303_indirect_phy_wait_for_completion(chip);
 	if (ret)
 		goto on_error;
 
@@ -328,6 +334,11 @@ static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
 	return ret;
 }
 
+const struct lan9303_phy_ops lan9303_indirect_phy_ops = {
+	.phy_read = lan9303_indirect_phy_read,
+	.phy_write = lan9303_indirect_phy_write,
+};
+
 static int lan9303_switch_wait_for_completion(struct lan9303 *chip)
 {
 	int ret, i;
@@ -427,14 +438,15 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
 	 * Special reg 18 of phy 3 reads as 0x0000, if 'phy_addr_sel_strap' is 0
 	 * and the IDs are 0-1-2, else it contains something different from
 	 * 0x0000, which means 'phy_addr_sel_strap' is 1 and the IDs are 1-2-3.
+	 * 0xffff is returned for failed MDIO access.
 	 */
-	reg = lan9303_port_phy_reg_read(chip, 3, MII_LAN911X_SPECIAL_MODES);
+	reg = chip->ops->phy_read(chip, 3, MII_LAN911X_SPECIAL_MODES);
 	if (reg < 0) {
 		dev_err(chip->dev, "Failed to detect phy config: %d\n", reg);
 		return reg;
 	}
 
-	if (reg != 0)
+	if ((reg != 0) && (reg != 0xffff))
 		chip->phy_addr_sel_strap = 1;
 	else
 		chip->phy_addr_sel_strap = 0;
@@ -719,7 +731,7 @@ static int lan9303_phy_read(struct dsa_switch *ds, int phy, int regnum)
 	if (phy > phy_base + 2)
 		return -ENODEV;
 
-	return lan9303_port_phy_reg_read(chip, phy, regnum);
+	return chip->ops->phy_read(chip, phy, regnum);
 }
 
 static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
@@ -733,7 +745,7 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
 	if (phy > phy_base + 2)
 		return -ENODEV;
 
-	return lan9303_phy_reg_write(chip, phy, regnum, val);
+	return chip->ops->phy_write(chip, phy, regnum, val);
 }
 
 static int lan9303_port_enable(struct dsa_switch *ds, int port,
@@ -766,13 +778,13 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
 	switch (port) {
 	case 1:
 		lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
-		lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 1,
-				      MII_BMCR, BMCR_PDOWN);
+		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
+				  MII_BMCR, BMCR_PDOWN);
 		break;
 	case 2:
 		lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
-		lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 2,
-				      MII_BMCR, BMCR_PDOWN);
+		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
+				  MII_BMCR, BMCR_PDOWN);
 		break;
 	default:
 		dev_dbg(chip->dev,
diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
index d1512dad2d90..444d00b460e1 100644
--- a/drivers/net/dsa/lan9303.h
+++ b/drivers/net/dsa/lan9303.h
@@ -2,6 +2,15 @@
 #include <linux/device.h>
 #include <net/dsa.h>
 
+struct lan9303;
+
+struct lan9303_phy_ops {
+	/* PHY 1 &2 access*/
+	int	(*phy_read)(struct lan9303 *chip, int port, int regnum);
+	int	(*phy_write)(struct lan9303 *chip, int port,
+			     int regnum, u16 val);
+};
+
 struct lan9303 {
 	struct device *dev;
 	struct regmap *regmap;
@@ -11,9 +20,11 @@ struct lan9303 {
 	bool phy_addr_sel_strap;
 	struct dsa_switch *ds;
 	struct mutex indirect_mutex; /* protect indexed register access */
+	const struct lan9303_phy_ops *ops;
 };
 
 extern const struct regmap_access_table lan9303_register_set;
+extern const struct lan9303_phy_ops lan9303_indirect_phy_ops;
 
 int lan9303_probe(struct lan9303 *chip, struct device_node *np);
 int lan9303_remove(struct lan9303 *chip);
diff --git a/drivers/net/dsa/lan9303_i2c.c b/drivers/net/dsa/lan9303_i2c.c
index ab3ce0da5071..24ec20f7f444 100644
--- a/drivers/net/dsa/lan9303_i2c.c
+++ b/drivers/net/dsa/lan9303_i2c.c
@@ -63,6 +63,8 @@ static int lan9303_i2c_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, sw_dev);
 	sw_dev->chip.dev = &client->dev;
 
+	sw_dev->chip.ops = &lan9303_indirect_phy_ops;
+
 	ret = lan9303_probe(&sw_dev->chip, client->dev.of_node);
 	if (ret != 0)
 		return ret;
diff --git a/drivers/net/dsa/lan9303_mdio.c b/drivers/net/dsa/lan9303_mdio.c
index 93c36c0541cf..94df12c5362f 100644
--- a/drivers/net/dsa/lan9303_mdio.c
+++ b/drivers/net/dsa/lan9303_mdio.c
@@ -39,6 +39,7 @@ static void lan9303_mdio_real_write(struct mdio_device *mdio, int reg, u16 val)
 static int lan9303_mdio_write(void *ctx, uint32_t reg, uint32_t val)
 {
 	struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx;
+	reg <<= 2; /* reg num to offset */
 
 	mutex_lock(&sw_dev->device->bus->mdio_lock);
 	lan9303_mdio_real_write(sw_dev->device, reg, val & 0xffff);
@@ -56,6 +57,7 @@ static u16 lan9303_mdio_real_read(struct mdio_device *mdio, int reg)
 static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val)
 {
 	struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx;
+	reg <<= 2; /* reg num to offset */
 
 	mutex_lock(&sw_dev->device->bus->mdio_lock);
 	*val = lan9303_mdio_real_read(sw_dev->device, reg);
@@ -65,6 +67,36 @@ static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val)
 	return 0;
 }
 
+int lan9303_mdio_phy_write(struct lan9303 *chip, int phy, int regnum, u16 val)
+{
+	struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev);
+	struct mdio_device *mdio = sw_dev->device;
+
+	mutex_lock(&mdio->bus->mdio_lock);
+	mdio->bus->write(mdio->bus, phy, regnum, val);
+	mutex_unlock(&mdio->bus->mdio_lock);
+
+	return 0;
+}
+
+int lan9303_mdio_phy_read(struct lan9303 *chip, int phy,  int reg)
+{
+	struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev);
+	struct mdio_device *mdio = sw_dev->device;
+	int val;
+
+	mutex_lock(&mdio->bus->mdio_lock);
+	val  =  mdio->bus->read(mdio->bus, phy, reg);
+	mutex_unlock(&mdio->bus->mdio_lock);
+
+	return val;
+}
+
+static const struct lan9303_phy_ops lan9303_mdio_phy_ops = {
+	.phy_read = lan9303_mdio_phy_read,
+	.phy_write = lan9303_mdio_phy_write,
+};
+
 static const struct regmap_config lan9303_mdio_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 32,
@@ -106,6 +138,8 @@ static int lan9303_mdio_probe(struct mdio_device *mdiodev)
 	dev_set_drvdata(&mdiodev->dev, sw_dev);
 	sw_dev->chip.dev = &mdiodev->dev;
 
+	sw_dev->chip.ops = &lan9303_mdio_phy_ops;
+
 	ret = lan9303_probe(&sw_dev->chip, mdiodev->dev.of_node);
 	if (ret != 0)
 		return ret;
-- 
2.11.0

Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

[Vivien Didelot]

Hi Egil,

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

> Fixes after testing on actual HW:
>
> - lan9303_mdio_write()/_read() must multiply register number
>   by 4 to get offset
>
> - Indirect access (PMI) to phy register only work in I2C mode. In
>   MDIO mode phy registers must be accessed directly. Introduced
>   struct lan9303_phy_ops to handle the two modes. Renamed functions
>   to clarify.
>
> - lan9303_detect_phy_setup() : Failed MDIO read return 0xffff.
>   Handle that.

Small patch series when possible are better. Bullet points in commit
messages are likely to describe how a patch or series may be split up
;-)

This patch seems to be the unique patch of the series resolving what is
described in the cover letter as "Make the MDIO interface work".

I'd suggest you to split up this one commit in several *atomic* and easy
to review patches and send them separately as on thread named "net: dsa:
lan9303: fix MDIO interface" (also note that imperative is prefered for
subject lines, see: https://chris.beams.io/posts/git-commit/#imperative)

<...>

> -static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
> +static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)

For instance you can have a first commit only renaming the functions.
The reason for it is to separate the functional changes from cosmetic
changes, which makes it easier for review.

<...>

> -	if (reg != 0)
> +	if ((reg != 0) && (reg != 0xffff))

if (reg && reg != 0xffff) should be enough.

>  		chip->phy_addr_sel_strap = 1;
>  	else
>  		chip->phy_addr_sel_strap = 0;

<...>

> +struct lan9303;
> +
> +struct lan9303_phy_ops {
> +	/* PHY 1 &2 access*/

The spacing is weird in the comment. "/* PHY 1 & 2 access */" maybe?

<...>

> +int lan9303_mdio_phy_write(struct lan9303 *chip, int phy, int regnum, u16 val)
> +{
> +	struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev);
> +	struct mdio_device *mdio = sw_dev->device;
> +
> +	mutex_lock(&mdio->bus->mdio_lock);
> +	mdio->bus->write(mdio->bus, phy, regnum, val);
> +	mutex_unlock(&mdio->bus->mdio_lock);

This is exactly what mdiobus_write(mdio->bus, phy, regnum, val) is
doing. There are very few valid reasons to go play in the mii_bus
structure, using generic APIs are strongly prefered. Plus you have
checks and traces for free!

> +
> +	return 0;
> +}
> +
> +int lan9303_mdio_phy_read(struct lan9303 *chip, int phy,  int reg)
> +{
> +	struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev);
> +	struct mdio_device *mdio = sw_dev->device;
> +	int val;
> +
> +	mutex_lock(&mdio->bus->mdio_lock);
> +	val  =  mdio->bus->read(mdio->bus, phy, reg);
> +	mutex_unlock(&mdio->bus->mdio_lock);

Same here, mdiobus_read().


Thanks,

        Vivien

Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

[Egil Hjelmeland]

On 25. juli 2017 21:15, Vivien Didelot wrote:
> Hi Egil,
> 
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> 
>> Fixes after testing on actual HW:
>>
>> - lan9303_mdio_write()/_read() must multiply register number
>>    by 4 to get offset
>>
>> - Indirect access (PMI) to phy register only work in I2C mode. In
>>    MDIO mode phy registers must be accessed directly. Introduced
>>    struct lan9303_phy_ops to handle the two modes. Renamed functions
>>    to clarify.
>>
>> - lan9303_detect_phy_setup() : Failed MDIO read return 0xffff.
>>    Handle that.
> 
> Small patch series when possible are better. Bullet points in commit
> messages are likely to describe how a patch or series may be split up
> ;-)
> 
> This patch seems to be the unique patch of the series resolving what is
> described in the cover letter as "Make the MDIO interface work".
> 
> I'd suggest you to split up this one commit in several *atomic* and easy
> to review patches and send them separately as on thread named "net: dsa:
> lan9303: fix MDIO interface" (also note that imperative is prefered for
> subject lines, see: https://chris.beams.io/posts/git-commit/#imperative)
> 
> <...>
> 
>> -static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
>> +static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)
> 
> For instance you can have a first commit only renaming the functions.
> The reason for it is to separate the functional changes from cosmetic
> changes, which makes it easier for review.
> 
> <...>

Thank you for reviewing.

I can split the first patch.

I can also split the patch series to more digestible series. But
since most of the patches touches the same file, I assume that each
series must be completed and applied before starting on a new one.
So I really want to group the patches into only a few series in order
to not spend months on the process.


>> +	if ((reg != 0) && (reg != 0xffff))
> 
> if (reg && reg != 0xffff) should be enough.

Of course.

>> +struct lan9303_phy_ops {
>> +	/* PHY 1 &2 access*/
> 
> The spacing is weird in the comment. "/* PHY 1 & 2 access */" maybe?
> 

Yes.

>> +int lan9303_mdio_phy_write(struct lan9303 *chip, int phy, int regnum, u16 val)
>> +{
>> +	struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev);
>> +	struct mdio_device *mdio = sw_dev->device;
>> +
>> +	mutex_lock(&mdio->bus->mdio_lock);
>> +	mdio->bus->write(mdio->bus, phy, regnum, val);
>> +	mutex_unlock(&mdio->bus->mdio_lock);
> 
> This is exactly what mdiobus_write(mdio->bus, phy, regnum, val) is
> doing. There are very few valid reasons to go play in the mii_bus
> structure, using generic APIs are strongly prefered. Plus you have
> checks and traces for free!
> 

Lack of oversight was the only reason. I just adapted stuff from
lan9303_mdio_phy_write above. Will switch to mdiobus_write of course.

> Same here, mdiobus_read().
> 
Ditto.

> 
> Thanks,
> 
>          Vivien
> 

Appreciated,
Egil

Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

[Vivien Didelot]

Hi Egil,

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

>> I'd suggest you to split up this one commit in several *atomic* and easy
>> to review patches and send them separately as on thread named "net: dsa:
>> lan9303: fix MDIO interface" (also note that imperative is prefered for
>> subject lines, see: https://chris.beams.io/posts/git-commit/#imperative)
>
> I can split the first patch.
>
> I can also split the patch series to more digestible series. But
> since most of the patches touches the same file, I assume that each
> series must be completed and applied before starting on a new one.
> So I really want to group the patches into only a few series in order
> to not spend months on the process.

I understand. But believe me, your patches are very likely to land
mainline faster if you send them in small chunks. This might not be true
for every subsystems, but netdev is very responsive. This is even more
true since this series has no-no (such as the sysfs entries) which
guarantees the whole patch series to be rejected.

Sending portions of your local work branch then rebase it against
net-next/master is a usual development process.


Thanks,

        Vivien

Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

[Egil Hjelmeland]

On 26. juli 2017 16:30, Vivien Didelot wrote:
> Hi Egil,
> 
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> 
>>> I'd suggest you to split up this one commit in several *atomic* and easy
>>> to review patches and send them separately as on thread named "net: dsa:
>>> lan9303: fix MDIO interface" (also note that imperative is prefered for
>>> subject lines, see: https://chris.beams.io/posts/git-commit/#imperative)
>>
>> I can split the first patch.
>>
>> I can also split the patch series to more digestible series. But
>> since most of the patches touches the same file, I assume that each
>> series must be completed and applied before starting on a new one.
>> So I really want to group the patches into only a few series in order
>> to not spend months on the process.
> 
> I understand. But believe me, your patches are very likely to land
> mainline faster if you send them in small chunks. This might not be true
> for every subsystems, but netdev is very responsive. This is even more
> true since this series has no-no (such as the sysfs entries) which
> guarantees the whole patch series to be rejected.
> 
> Sending portions of your local work branch then rebase it against
> net-next/master is a usual development process.
> 
> 
> Thanks,
> 
>          Vivien
> 

Thank you for the advice. I got some other NMIs today that I have to
serve. Hope to come back with MDIO patch series soon.

Egil

Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

[Andrew Lunn]

> > So I really want to group the patches into only a few series in order
> > to not spend months on the process.

I strongly agree with Vivien here. Good patches get accepted in about
3 days. You should expect feedback within a day or two. That allows
you to have fast cycle times for getting patches in.

    Andrew

Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Wed, 26 Jul 2017 19:52:24 +0200

>> > So I really want to group the patches into only a few series in order
>> > to not spend months on the process.
> 
> I strongly agree with Vivien here. Good patches get accepted in about
> 3 days. You should expect feedback within a day or two. That allows
> you to have fast cycle times for getting patches in.

+1

Small simple patches will get everything in 10 times fast than if
you clump everything together into larger, harder to review ones.

Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

[Egil Hjelmeland]

Den 26. juli 2017 22:07, skrev David Miller:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Wed, 26 Jul 2017 19:52:24 +0200
> 
>>>> So I really want to group the patches into only a few series in order
>>>> to not spend months on the process.
>>
>> I strongly agree with Vivien here. Good patches get accepted in about
>> 3 days. You should expect feedback within a day or two. That allows
>> you to have fast cycle times for getting patches in.
> 
> +1
> 
> Small simple patches will get everything in 10 times fast than if
> you clump everything together into larger, harder to review ones.
> 

Good. Just one question about process. Could I have posted my work
as a RFC? To get one round of initial feedback before chopping into
small patch requests. As well as indicating where I am heading. Or is
that just waste of human bandwidth?

Egil

Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

[Andrew Lunn]

> Good. Just one question about process. Could I have posted my work
> as a RFC? To get one round of initial feedback before chopping into
> small patch requests. As well as indicating where I am heading. Or is
> that just waste of human bandwidth?

Depends. Post 100 RFC patches, i won't look at them. Post 21 with a
cover note making it clear you are planning to submit them in blocks
of 7, i might.

But it is best to assume reviewers have small blocks of time. 21
patches take 3 times a long to review as 7. The block of time might
not be enough for 21, so the review gets differed. 7 are more likely
to fit in the available time, so it happens quickly.

   Andrew

Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

[Andrew Lunn]

On Tue, Jul 25, 2017 at 06:15:44PM +0200, Egil Hjelmeland wrote:
> Fixes after testing on actual HW:
> 
> - lan9303_mdio_write()/_read() must multiply register number
>   by 4 to get offset
> 
> - Indirect access (PMI) to phy register only work in I2C mode. In
>   MDIO mode phy registers must be accessed directly. Introduced
>   struct lan9303_phy_ops to handle the two modes. Renamed functions
>   to clarify.
> 
> - lan9303_detect_phy_setup() : Failed MDIO read return 0xffff.
>   Handle that.
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> ---
>  drivers/net/dsa/lan9303-core.c | 42 +++++++++++++++++++++++++++---------------
>  drivers/net/dsa/lan9303.h      | 11 +++++++++++
>  drivers/net/dsa/lan9303_i2c.c  |  2 ++
>  drivers/net/dsa/lan9303_mdio.c | 34 ++++++++++++++++++++++++++++++++++
>  4 files changed, 74 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index cd76e61f1fca..e622db586c3d 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -20,6 +20,9 @@
>  
>  #include "lan9303.h"
>  
> +/* 13.2 System Control and Status Registers
> + * Multiply register number by 4 to get address offset.
> + */
>  #define LAN9303_CHIP_REV 0x14
>  # define LAN9303_CHIP_ID 0x9303
>  #define LAN9303_IRQ_CFG 0x15
> @@ -53,6 +56,9 @@
>  #define LAN9303_VIRT_PHY_BASE 0x70
>  #define LAN9303_VIRT_SPECIAL_CTRL 0x77
>  
> +/*13.4 Switch Fabric Control and Status Registers
> + * Accessed indirectly via SWITCH_CSR_CMD, SWITCH_CSR_DATA.
> + */
>  #define LAN9303_SW_DEV_ID 0x0000
>  #define LAN9303_SW_RESET 0x0001
>  #define LAN9303_SW_RESET_RESET BIT(0)
> @@ -242,7 +248,7 @@ static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val)
>  	return regmap_write(chip->regmap, LAN9303_VIRT_PHY_BASE + regnum, val);
>  }
>  
> -static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
> +static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)
>  {
>  	int ret, i;
>  	u32 reg;
> @@ -262,7 +268,7 @@ static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
>  	return -EIO;
>  }
>  
> -static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
> +static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int regnum)
>  {
>  	int ret;
>  	u32 val;
> @@ -272,7 +278,7 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
>  
>  	mutex_lock(&chip->indirect_mutex);
>  
> -	ret = lan9303_port_phy_reg_wait_for_completion(chip);
> +	ret = lan9303_indirect_phy_wait_for_completion(chip);
>  	if (ret)
>  		goto on_error;
>  
> @@ -281,7 +287,7 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
>  	if (ret)
>  		goto on_error;
>  
> -	ret = lan9303_port_phy_reg_wait_for_completion(chip);
> +	ret = lan9303_indirect_phy_wait_for_completion(chip);
>  	if (ret)
>  		goto on_error;
>  
> @@ -299,8 +305,8 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
>  	return ret;
>  }
>  
> -static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
> -				 unsigned int val)
> +static int lan9303_indirect_phy_write(struct lan9303 *chip, int addr,
> +				      int regnum, u16 val)
>  {
>  	int ret;
>  	u32 reg;
> @@ -311,7 +317,7 @@ static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
>  
>  	mutex_lock(&chip->indirect_mutex);
>  
> -	ret = lan9303_port_phy_reg_wait_for_completion(chip);
> +	ret = lan9303_indirect_phy_wait_for_completion(chip);
>  	if (ret)
>  		goto on_error;
>  
> @@ -328,6 +334,11 @@ static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
>  	return ret;
>  }
>  
> +const struct lan9303_phy_ops lan9303_indirect_phy_ops = {
> +	.phy_read = lan9303_indirect_phy_read,
> +	.phy_write = lan9303_indirect_phy_write,
> +};
> +
>  static int lan9303_switch_wait_for_completion(struct lan9303 *chip)
>  {
>  	int ret, i;
> @@ -427,14 +438,15 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
>  	 * Special reg 18 of phy 3 reads as 0x0000, if 'phy_addr_sel_strap' is 0
>  	 * and the IDs are 0-1-2, else it contains something different from
>  	 * 0x0000, which means 'phy_addr_sel_strap' is 1 and the IDs are 1-2-3.
> +	 * 0xffff is returned for failed MDIO access.

Hi Egil

0xffff is not really a failure. It just means there is nothing on the
bus at that address. The bus has a weak pull-up, so defaults to one.

>  	 */
> -	reg = lan9303_port_phy_reg_read(chip, 3, MII_LAN911X_SPECIAL_MODES);
> +	reg = chip->ops->phy_read(chip, 3, MII_LAN911X_SPECIAL_MODES);
>  	if (reg < 0) {
>  		dev_err(chip->dev, "Failed to detect phy config: %d\n", reg);
>  		return reg;
>  	}
>  
> -	if (reg != 0)
> +	if ((reg != 0) && (reg != 0xffff))
>  		chip->phy_addr_sel_strap = 1;
>  	else
>  		chip->phy_addr_sel_strap = 0;
> @@ -719,7 +731,7 @@ static int lan9303_phy_read(struct dsa_switch *ds, int phy, int regnum)
>  	if (phy > phy_base + 2)
>  		return -ENODEV;
>  
> -	return lan9303_port_phy_reg_read(chip, phy, regnum);
> +	return chip->ops->phy_read(chip, phy, regnum);
>  }
>  
>  static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
> @@ -733,7 +745,7 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
>  	if (phy > phy_base + 2)
>  		return -ENODEV;
>  
> -	return lan9303_phy_reg_write(chip, phy, regnum, val);
> +	return chip->ops->phy_write(chip, phy, regnum, val);
>  }
>  
>  static int lan9303_port_enable(struct dsa_switch *ds, int port,
> @@ -766,13 +778,13 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
>  	switch (port) {
>  	case 1:
>  		lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
> -		lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 1,
> -				      MII_BMCR, BMCR_PDOWN);
> +		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
> +				  MII_BMCR, BMCR_PDOWN);
>  		break;
>  	case 2:
>  		lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
> -		lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 2,
> -				      MII_BMCR, BMCR_PDOWN);
> +		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
> +				  MII_BMCR, BMCR_PDOWN);
>  		break;
>  	default:
>  		dev_dbg(chip->dev,
> diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
> index d1512dad2d90..444d00b460e1 100644
> --- a/drivers/net/dsa/lan9303.h
> +++ b/drivers/net/dsa/lan9303.h
> @@ -2,6 +2,15 @@
>  #include <linux/device.h>
>  #include <net/dsa.h>
>  
> +struct lan9303;
> +
> +struct lan9303_phy_ops {
> +	/* PHY 1 &2 access*/

A space would be nice here after the &.

> +	int	(*phy_read)(struct lan9303 *chip, int port, int regnum);
> +	int	(*phy_write)(struct lan9303 *chip, int port,
> +			     int regnum, u16 val);
> +};
> +
>  struct lan9303 {
>  	struct device *dev;
>  	struct regmap *regmap;
> @@ -11,9 +20,11 @@ struct lan9303 {
>  	bool phy_addr_sel_strap;
>  	struct dsa_switch *ds;
>  	struct mutex indirect_mutex; /* protect indexed register access */
> +	const struct lan9303_phy_ops *ops;
>  };
>  
>  extern const struct regmap_access_table lan9303_register_set;
> +extern const struct lan9303_phy_ops lan9303_indirect_phy_ops;
>  
>  int lan9303_probe(struct lan9303 *chip, struct device_node *np);
>  int lan9303_remove(struct lan9303 *chip);
> diff --git a/drivers/net/dsa/lan9303_i2c.c b/drivers/net/dsa/lan9303_i2c.c
> index ab3ce0da5071..24ec20f7f444 100644
> --- a/drivers/net/dsa/lan9303_i2c.c
> +++ b/drivers/net/dsa/lan9303_i2c.c
> @@ -63,6 +63,8 @@ static int lan9303_i2c_probe(struct i2c_client *client,
>  	i2c_set_clientdata(client, sw_dev);
>  	sw_dev->chip.dev = &client->dev;
>  
> +	sw_dev->chip.ops = &lan9303_indirect_phy_ops;
> +
>  	ret = lan9303_probe(&sw_dev->chip, client->dev.of_node);
>  	if (ret != 0)
>  		return ret;
> diff --git a/drivers/net/dsa/lan9303_mdio.c b/drivers/net/dsa/lan9303_mdio.c
> index 93c36c0541cf..94df12c5362f 100644
> --- a/drivers/net/dsa/lan9303_mdio.c
> +++ b/drivers/net/dsa/lan9303_mdio.c
> @@ -39,6 +39,7 @@ static void lan9303_mdio_real_write(struct mdio_device *mdio, int reg, u16 val)
>  static int lan9303_mdio_write(void *ctx, uint32_t reg, uint32_t val)
>  {
>  	struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx;
> +	reg <<= 2; /* reg num to offset */
>  
>  	mutex_lock(&sw_dev->device->bus->mdio_lock);
>  	lan9303_mdio_real_write(sw_dev->device, reg, val & 0xffff);
> @@ -56,6 +57,7 @@ static u16 lan9303_mdio_real_read(struct mdio_device *mdio, int reg)
>  static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val)
>  {
>  	struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx;
> +	reg <<= 2; /* reg num to offset */
>  
>  	mutex_lock(&sw_dev->device->bus->mdio_lock);
>  	*val = lan9303_mdio_real_read(sw_dev->device, reg);
> @@ -65,6 +67,36 @@ static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val)
>  	return 0;
>  }
>  
> +int lan9303_mdio_phy_write(struct lan9303 *chip, int phy, int regnum, u16 val)
> +{
> +	struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev);
> +	struct mdio_device *mdio = sw_dev->device;
> +
> +	mutex_lock(&mdio->bus->mdio_lock);
> +	mdio->bus->write(mdio->bus, phy, regnum, val);
> +	mutex_unlock(&mdio->bus->mdio_lock);

It is better to use mdiobus_read/write or if you are nesting mdio
busses, mdiobus_read_nested/mdiobus_write_nested. Please test this
code with lockdep enabled.

And as others have pointed out, there are too many changes in this one
patch.

Thanks
	Andrew

Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

[Egil Hjelmeland]

On 26. juli 2017 18:55, Andrew Lunn wrote:
> On Tue, Jul 25, 2017 at 06:15:44PM +0200, Egil Hjelmeland wrote:
> It is better to use mdiobus_read/write or if you are nesting mdio
> busses, mdiobus_read_nested/mdiobus_write_nested. Please test this
> code with lockdep enabled.
> 

I have CONFIG_DEBUG_SPINLOCK, CONFIG_DEBUG_MUTEXES. Should I enable
more?

Egil

Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

[Andrew Lunn]

On Fri, Jul 28, 2017 at 01:08:25PM +0200, Egil Hjelmeland wrote:
> On 26. juli 2017 18:55, Andrew Lunn wrote:
> >On Tue, Jul 25, 2017 at 06:15:44PM +0200, Egil Hjelmeland wrote:
> >It is better to use mdiobus_read/write or if you are nesting mdio
> >busses, mdiobus_read_nested/mdiobus_write_nested. Please test this
> >code with lockdep enabled.
> >
> 
> I have CONFIG_DEBUG_SPINLOCK, CONFIG_DEBUG_MUTEXES. Should I enable
> more?

Hi Egil

Enable CONFIG_LOCKDEP and CONFIG_PROVE_LOCKING.

Any lockdep splat you get while accessing the mdio bus at this point
are probably false positives, since it is a different mutex. Using the
_nested() version should avoid these false positives. But you might
find other places your locking is not right.

	Andrew

Re: [PATCH net-next v2 01/10] net: dsa: lan9303: Fixed MDIO interface

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

Hi Egil,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Egil-Hjelmeland/net-dsa-lan9303-unicast-offload-fdb-mdb-STP/20170727-074246
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "lan9303_indirect_phy_ops" [drivers/net/dsa/lan9303_i2c.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 60986 bytes --]

[PATCH net-next v2 02/10] net: dsa: lan9303: Do not disable/enable switch fabric port 0 at startup

[Egil Hjelmeland]

For some mysterious reason enable switch fabric port 0 TX fails to
work, when the TX has previous been disabled. Resolved by not
disable/enable switch fabric port 0 at startup. Port 1 and 2 are
still disabled in early init.

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index e622db586c3d..c2b53659f58f 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -557,9 +557,6 @@ static int lan9303_disable_processing(struct lan9303 *chip)
 {
 	int ret;
 
-	ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
-	if (ret)
-		return ret;
 	ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
 	if (ret)
 		return ret;
@@ -633,10 +630,6 @@ static int lan9303_setup(struct dsa_switch *ds)
 	if (ret)
 		dev_err(chip->dev, "failed to separate ports %d\n", ret);
 
-	ret = lan9303_enable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
-	if (ret)
-		dev_err(chip->dev, "failed to re-enable switching %d\n", ret);
-
 	return 0;
 }
 
-- 
2.11.0

Re: [PATCH net-next v2 02/10] net: dsa: lan9303: Do not disable/enable switch fabric port 0 at startup

[Andrew Lunn]

On Tue, Jul 25, 2017 at 06:15:45PM +0200, Egil Hjelmeland wrote:
> For some mysterious reason enable switch fabric port 0 TX fails to
> work, when the TX has previous been disabled. Resolved by not
> disable/enable switch fabric port 0 at startup. Port 1 and 2 are
> still disabled in early init.
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> ---
>  drivers/net/dsa/lan9303-core.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index e622db586c3d..c2b53659f58f 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -557,9 +557,6 @@ static int lan9303_disable_processing(struct lan9303 *chip)
>  {
>  	int ret;
>  
> -	ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
> -	if (ret)
> -		return ret;
>  	ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
>  	if (ret)
>  		return ret;
> @@ -633,10 +630,6 @@ static int lan9303_setup(struct dsa_switch *ds)
>  	if (ret)
>  		dev_err(chip->dev, "failed to separate ports %d\n", ret);
>  
> -	ret = lan9303_enable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
> -	if (ret)
> -		dev_err(chip->dev, "failed to re-enable switching %d\n", ret);
> -

Does this mean you are relying on something else enabling port 0? The
bootloader?

I'm wondering if it is better to keep the enable, but remove the
disable?

	Andrew

Re: [PATCH net-next v2 02/10] net: dsa: lan9303: Do not disable/enable switch fabric port 0 at startup

[Egil Hjelmeland]

On 26. juli 2017 18:58, Andrew Lunn wrote:
> On Tue, Jul 25, 2017 at 06:15:45PM +0200, Egil Hjelmeland wrote:
>> For some mysterious reason enable switch fabric port 0 TX fails to
>> work, when the TX has previous been disabled. Resolved by not
>> disable/enable switch fabric port 0 at startup. Port 1 and 2 are
>> still disabled in early init.
>>
>> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
>> ---
>>   drivers/net/dsa/lan9303-core.c | 7 -------
>>   1 file changed, 7 deletions(-)
>>
>> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
>> index e622db586c3d..c2b53659f58f 100644
>> --- a/drivers/net/dsa/lan9303-core.c
>> +++ b/drivers/net/dsa/lan9303-core.c
>> @@ -557,9 +557,6 @@ static int lan9303_disable_processing(struct lan9303 *chip)
>>   {
>>   	int ret;
>>   
>> -	ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
>> -	if (ret)
>> -		return ret;
>>   	ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
>>   	if (ret)
>>   		return ret;
>> @@ -633,10 +630,6 @@ static int lan9303_setup(struct dsa_switch *ds)
>>   	if (ret)
>>   		dev_err(chip->dev, "failed to separate ports %d\n", ret);
>>   
>> -	ret = lan9303_enable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
>> -	if (ret)
>> -		dev_err(chip->dev, "failed to re-enable switching %d\n", ret);
>> -
> 
> Does this mean you are relying on something else enabling port 0? The
> bootloader?
> 
> I'm wondering if it is better to keep the enable, but remove the
> disable?
> 
> 	Andrew
> 

The (switch engine) ports are enabled by default. The only thing our
bootloader does is to set gpo so the lan9303 is kept in reset until
the linux driver starts. When I test with the next-next kernel I just
specify the reset-gpo in DTS and the driver pulls it out of reset.

Keeping the enable does no harm, as far as I recall, but I can
double check that when I get time. I have no idea why the original
mainline code does not work for me. Maybe it is a timing issue?

[PATCH net-next v2 03/10] net: dsa: lan9303: Refactor lan9303_enable_packet_processing()

[Egil Hjelmeland]

lan9303_enable_packet_processing, lan9303_disable_packet_processing()
Pass port number (0,1,2) as parameter instead of port offset.
Simplify accordingly.

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index c2b53659f58f..0806a0684d55 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -159,9 +159,7 @@
 # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT1 (BIT(9) | BIT(8))
 # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0 (BIT(1) | BIT(0))
 
-#define LAN9303_PORT_0_OFFSET 0x400
-#define LAN9303_PORT_1_OFFSET 0x800
-#define LAN9303_PORT_2_OFFSET 0xc00
+#define LAN9303_SWITCH_PORT_REG(port, reg0) (0x400 * (port) + (reg0))
 
 /* the built-in PHYs are of type LAN911X */
 #define MII_LAN911X_SPECIAL_MODES 0x12
@@ -457,24 +455,25 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
 	return 0;
 }
 
-#define LAN9303_MAC_RX_CFG_OFFS (LAN9303_MAC_RX_CFG_0 - LAN9303_PORT_0_OFFSET)
-#define LAN9303_MAC_TX_CFG_OFFS (LAN9303_MAC_TX_CFG_0 - LAN9303_PORT_0_OFFSET)
-
 static int lan9303_disable_packet_processing(struct lan9303 *chip,
 					     unsigned int port)
 {
 	int ret;
 
 	/* disable RX, but keep register reset default values else */
-	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_OFFS + port,
-				       LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES);
+	ret = lan9303_write_switch_reg(
+			chip,
+			LAN9303_SWITCH_PORT_REG(port, LAN9303_MAC_RX_CFG_0),
+			LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES);
 	if (ret)
 		return ret;
 
 	/* disable TX, but keep register reset default values else */
-	return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_OFFS + port,
-				LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
-				LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE);
+	return lan9303_write_switch_reg(
+			chip,
+			LAN9303_SWITCH_PORT_REG(port, LAN9303_MAC_TX_CFG_0),
+			LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
+			LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE);
 }
 
 static int lan9303_enable_packet_processing(struct lan9303 *chip,
@@ -483,17 +482,21 @@ static int lan9303_enable_packet_processing(struct lan9303 *chip,
 	int ret;
 
 	/* enable RX and keep register reset default values else */
-	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_OFFS + port,
-				       LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES |
-				       LAN9303_MAC_RX_CFG_X_RX_ENABLE);
+	ret = lan9303_write_switch_reg(
+			chip,
+			LAN9303_SWITCH_PORT_REG(port, LAN9303_MAC_RX_CFG_0),
+			LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES |
+			LAN9303_MAC_RX_CFG_X_RX_ENABLE);
 	if (ret)
 		return ret;
 
 	/* enable TX and keep register reset default values else */
-	return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_OFFS + port,
-				LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
-				LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE |
-				LAN9303_MAC_TX_CFG_X_TX_ENABLE);
+	return lan9303_write_switch_reg(
+			chip,
+			LAN9303_SWITCH_PORT_REG(port, LAN9303_MAC_TX_CFG_0),
+			LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
+			LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE |
+			LAN9303_MAC_TX_CFG_X_TX_ENABLE);
 }
 
 /* We want a special working switch:
@@ -555,12 +558,14 @@ static int lan9303_handle_reset(struct lan9303 *chip)
 /* stop processing packets for all ports */
 static int lan9303_disable_processing(struct lan9303 *chip)
 {
-	int ret;
+	int ret, p;
 
-	ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
-	if (ret)
-		return ret;
-	return lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
+	for (p = 1; p <= 2; p++) {
+		ret = lan9303_disable_packet_processing(chip, p);
+		if (ret)
+			return ret;
+	}
+	return 0;
 }
 
 static int lan9303_check_device(struct lan9303 *chip)
@@ -696,7 +701,7 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
 	unsigned int u, poff;
 	int ret;
 
-	poff = port * 0x400;
+	poff = LAN9303_SWITCH_PORT_REG(port, 0);
 
 	for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
 		ret = lan9303_read_switch_reg(chip,
@@ -749,11 +754,8 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
 	/* enable internal packet processing */
 	switch (port) {
 	case 1:
-		return lan9303_enable_packet_processing(chip,
-							LAN9303_PORT_1_OFFSET);
 	case 2:
-		return lan9303_enable_packet_processing(chip,
-							LAN9303_PORT_2_OFFSET);
+		return lan9303_enable_packet_processing(chip, port);
 	default:
 		dev_dbg(chip->dev,
 			"Error: request to power up invalid port %d\n", port);
@@ -770,13 +772,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
 	/* disable internal packet processing */
 	switch (port) {
 	case 1:
-		lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
-		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
-				  MII_BMCR, BMCR_PDOWN);
-		break;
 	case 2:
-		lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
-		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
+		lan9303_disable_packet_processing(chip, port);
+		lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
 				  MII_BMCR, BMCR_PDOWN);
 		break;
 	default:
-- 
2.11.0

[PATCH net-next v2 04/10] net: dsa: lan9303: Added adjust_link() method

[Egil Hjelmeland]

This makes the driver react to device tree "fixed-link" declaration
on CPU port.

- turn off autonegotiation
- force speed 10 or 100 mb/s
- force duplex mode

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 0806a0684d55..be6d78f45a5f 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -17,6 +17,7 @@
 #include <linux/regmap.h>
 #include <linux/mutex.h>
 #include <linux/mii.h>
+#include <linux/phy.h>
 
 #include "lan9303.h"
 
@@ -746,6 +747,37 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
 	return chip->ops->phy_write(chip, phy, regnum, val);
 }
 
+static void lan9303_adjust_link(struct dsa_switch *ds, int port,
+				struct phy_device *phydev)
+{
+	struct lan9303 *chip = ds->priv;
+
+	int ctl, res;
+
+	ctl = lan9303_phy_read(ds, port, MII_BMCR);
+
+	if (!phy_is_pseudo_fixed_link(phydev))
+		return;
+
+	ctl &= ~BMCR_ANENABLE;
+	if (phydev->speed == SPEED_100)
+		ctl |= BMCR_SPEED100;
+
+	if (phydev->duplex == DUPLEX_FULL)
+		ctl |= BMCR_FULLDPLX;
+
+	res =  lan9303_phy_write(ds, port, MII_BMCR, ctl);
+
+	if (port == chip->phy_addr_sel_strap) {
+		/* Virtual Phy: Remove Turbo 200Mbit mode */
+		lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
+
+		ctl &= ~(1 << 10); // TURBO BIT
+		res =  regmap_write(chip->regmap,
+				    LAN9303_VIRT_SPECIAL_CTRL, ctl);
+	}
+}
+
 static int lan9303_port_enable(struct dsa_switch *ds, int port,
 			       struct phy_device *phy)
 {
@@ -789,6 +821,7 @@ static struct dsa_switch_ops lan9303_switch_ops = {
 	.get_strings = lan9303_get_strings,
 	.phy_read = lan9303_phy_read,
 	.phy_write = lan9303_phy_write,
+	.adjust_link = lan9303_adjust_link,
 	.get_ethtool_stats = lan9303_get_ethtool_stats,
 	.get_sset_count = lan9303_get_sset_count,
 	.port_enable = lan9303_port_enable,
-- 
2.11.0

Re: [PATCH net-next v2 04/10] net: dsa: lan9303: Added adjust_link() method

[Andrew Lunn]

On Tue, Jul 25, 2017 at 06:15:47PM +0200, Egil Hjelmeland wrote:
> This makes the driver react to device tree "fixed-link" declaration
> on CPU port.
> 
> - turn off autonegotiation
> - force speed 10 or 100 mb/s
> - force duplex mode
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> ---
>  drivers/net/dsa/lan9303-core.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 0806a0684d55..be6d78f45a5f 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -17,6 +17,7 @@
>  #include <linux/regmap.h>
>  #include <linux/mutex.h>
>  #include <linux/mii.h>
> +#include <linux/phy.h>
>  
>  #include "lan9303.h"
>  
> @@ -746,6 +747,37 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
>  	return chip->ops->phy_write(chip, phy, regnum, val);
>  }
>  
> +static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> +				struct phy_device *phydev)
> +{
> +	struct lan9303 *chip = ds->priv;
> +
> +	int ctl, res;
> +
> +	ctl = lan9303_phy_read(ds, port, MII_BMCR);
> +
> +	if (!phy_is_pseudo_fixed_link(phydev))
> +		return;
> +

Hi Egil

Maybe do this check before reading MII_BMCR?

> +	ctl &= ~BMCR_ANENABLE;

Should this also mask out BMCR_SPEED100 and DUPLEX_FULL? Otherwise how
do you select 10/Half if it is already configured for 100/Full?

> +	if (phydev->speed == SPEED_100)
> +		ctl |= BMCR_SPEED100;
> +
> +	if (phydev->duplex == DUPLEX_FULL)
> +		ctl |= BMCR_FULLDPLX;
> +
> +	res =  lan9303_phy_write(ds, port, MII_BMCR, ctl);
> +
> +	if (port == chip->phy_addr_sel_strap) {
> +		/* Virtual Phy: Remove Turbo 200Mbit mode */
> +		lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
> +
> +		ctl &= ~(1 << 10); // TURBO BIT

BIT(10), or better still something like BIT(LAN9303_VIRT_SPECIAL_TURBO)

> +		res =  regmap_write(chip->regmap,
> +				    LAN9303_VIRT_SPECIAL_CTRL, ctl);
> +	}
> +}

  Andrew

Re: [PATCH net-next v2 04/10] net: dsa: lan9303: Added adjust_link() method

[Egil Hjelmeland]

On 26. juli 2017 19:09, Andrew Lunn wrote:
> On Tue, Jul 25, 2017 at 06:15:47PM +0200, Egil Hjelmeland wrote:
>> This makes the driver react to device tree "fixed-link" declaration
>> on CPU port.
>>
>> - turn off autonegotiation
>> - force speed 10 or 100 mb/s
>> - force duplex mode
>>
>> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
>> ---
>>   drivers/net/dsa/lan9303-core.c | 33 +++++++++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
>> index 0806a0684d55..be6d78f45a5f 100644
>> --- a/drivers/net/dsa/lan9303-core.c
>> +++ b/drivers/net/dsa/lan9303-core.c
>> @@ -17,6 +17,7 @@
>>   #include <linux/regmap.h>
>>   #include <linux/mutex.h>
>>   #include <linux/mii.h>
>> +#include <linux/phy.h>
>>   
>>   #include "lan9303.h"
>>   
>> @@ -746,6 +747,37 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
>>   	return chip->ops->phy_write(chip, phy, regnum, val);
>>   }
>>   
>> +static void lan9303_adjust_link(struct dsa_switch *ds, int port,
>> +				struct phy_device *phydev)
>> +{
>> +	struct lan9303 *chip = ds->priv;
>> +
>> +	int ctl, res;
>> +
>> +	ctl = lan9303_phy_read(ds, port, MII_BMCR);
>> +
>> +	if (!phy_is_pseudo_fixed_link(phydev))
>> +		return;
>> +
> 
> Hi Egil
> 
> Maybe do this check before reading MII_BMCR?
> 

OK

>> +	ctl &= ~BMCR_ANENABLE;
> 
> Should this also mask out BMCR_SPEED100 and DUPLEX_FULL? Otherwise how
> do you select 10/Half if it is already configured for 100/Full?
> 

Yes you are right. I started out with setting ctl to the fixed default
value, and later changed code to start by reading it from HW.


>> +	if (phydev->speed == SPEED_100)
>> +		ctl |= BMCR_SPEED100;
>> +
>> +	if (phydev->duplex == DUPLEX_FULL)
>> +		ctl |= BMCR_FULLDPLX;
>> +
>> +	res =  lan9303_phy_write(ds, port, MII_BMCR, ctl);
>> +
>> +	if (port == chip->phy_addr_sel_strap) {
>> +		/* Virtual Phy: Remove Turbo 200Mbit mode */
>> +		lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
>> +
>> +		ctl &= ~(1 << 10); // TURBO BIT
> 
> BIT(10), or better still something like BIT(LAN9303_VIRT_SPECIAL_TURBO)
> 

Agree

>> +		res =  regmap_write(chip->regmap,
>> +				    LAN9303_VIRT_SPECIAL_CTRL, ctl);
>> +	}
>> +}
> 
>    Andrew
> 

[PATCH net-next v2 05/10] net: dsa: added dsa_net_device_to_dsa_port()

[Egil Hjelmeland]

Allowing dsa drivers to attach sysfs nodes.

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 include/net/dsa.h |  1 +
 net/dsa/slave.c   | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 88da272d20d0..a71c0a2401ee 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -450,6 +450,7 @@ void unregister_switch_driver(struct dsa_switch_driver *type);
 struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev);
 
 struct net_device *dsa_dev_to_net_device(struct device *dev);
+struct dsa_port *dsa_net_device_to_dsa_port(struct net_device *dev);
 
 /* Keep inline for faster access in hot path */
 static inline bool netdev_uses_dsa(struct net_device *dev)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 9507bd38cf04..40410f1740de 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -209,6 +209,16 @@ static int dsa_slave_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	return -EOPNOTSUPP;
 }
 
+struct dsa_port *dsa_net_device_to_dsa_port(struct net_device *dev)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+
+	if (!dsa_slave_dev_check(dev))
+		return NULL;
+	return p->dp;
+}
+EXPORT_SYMBOL_GPL(dsa_net_device_to_dsa_port);
+
 static int dsa_slave_port_attr_set(struct net_device *dev,
 				   const struct switchdev_attr *attr,
 				   struct switchdev_trans *trans)
-- 
2.11.0

[PATCH net-next v2 06/10] net: dsa: lan9303: added sysfs node swe_bcst_throt

[Egil Hjelmeland]

Allowing per-port access to Switch Engine Broadcast Throttling Register

Also added lan9303_write_switch_reg_mask()

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index be6d78f45a5f..b70acb73aad6 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -154,6 +154,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_BCST_THROT 0x1848
 #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))
@@ -426,6 +427,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, &reg);
+	if (ret)
+		return ret;
+	reg = (reg & ~mask) | val;
+
+	return lan9303_write_switch_reg(chip, regnum, reg);
+}
+
 static int lan9303_detect_phy_setup(struct lan9303 *chip)
 {
 	int reg;
@@ -614,6 +629,66 @@ static int lan9303_check_device(struct lan9303 *chip)
 	return 0;
 }
 
+/* ---------------------- Sysfs on slave port --------------------------*/
+/*13.4.3.23 Switch Engine Broadcast Throttling Register (SWE_BCST_THROT)*/
+static ssize_t
+swe_bcst_throt_show(struct device *dev, struct device_attribute *attr,
+		    char *buf)
+{
+	struct dsa_port *dp = dsa_net_device_to_dsa_port(to_net_dev(dev));
+	struct lan9303 *chip = dp->ds->priv;
+	int port = dp->index;
+	int reg;
+
+	if (lan9303_read_switch_reg(chip, LAN9303_SWE_BCST_THROT, &reg))
+		return 0;
+
+	reg = (reg >> (9 * port)) & 0x1ff; /*extract port N*/
+	if (reg & 0x100)
+		reg &= 0xff; /* remove enable bit */
+	else
+		reg = 0;     /* not enabled*/
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", reg);
+}
+
+static ssize_t
+swe_bcst_throt_store(struct device *dev, struct device_attribute *attr,
+		     const char *buf, size_t len)
+{
+	struct dsa_port *dp = dsa_net_device_to_dsa_port(to_net_dev(dev));
+	struct lan9303 *chip = dp->ds->priv;
+	int port = dp->index;
+	int ret;
+	unsigned long level;
+
+	ret = kstrtoul(buf, 0, &level);
+	if (ret)
+		return ret;
+	level &= 0xff; /* ensure valid range */
+	if (level)
+		level |= 0x100; /* Set enable bit  */
+
+	ret = lan9303_write_switch_reg_mask(chip, LAN9303_SWE_BCST_THROT,
+					    level << (9 * port),
+					    0x1ff << (9 * port));
+	if (ret)
+		return ret;
+	return len;
+}
+
+static DEVICE_ATTR_RW(swe_bcst_throt);
+
+static struct attribute *lan9303_attrs[] = {
+	&dev_attr_swe_bcst_throt.attr,
+	NULL
+};
+
+static struct attribute_group lan9303_group = {
+	.name = "lan9303",
+	.attrs = lan9303_attrs,
+};
+
 /* ---------------------------- DSA -----------------------------------*/
 
 static enum dsa_tag_protocol lan9303_get_tag_protocol(struct dsa_switch *ds)
@@ -787,6 +862,11 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
 	switch (port) {
 	case 1:
 	case 2:
+		/* lan9303_setup is too early to attach sysfs nodes... */
+		if (sysfs_create_group(
+				&ds->ports[port].netdev->dev.kobj,
+				&lan9303_group))
+			dev_dbg(chip->dev, "cannot create sysfs group\n");
 		return lan9303_enable_packet_processing(chip, port);
 	default:
 		dev_dbg(chip->dev,
@@ -805,6 +885,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
 	switch (port) {
 	case 1:
 	case 2:
+		sysfs_remove_group(&ds->ports[port].netdev->dev.kobj,
+				   &lan9303_group);
+
 		lan9303_disable_packet_processing(chip, port);
 		lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
 				  MII_BMCR, BMCR_PDOWN);
-- 
2.11.0

Re: [PATCH net-next v2 06/10] net: dsa: lan9303: added sysfs node swe_bcst_throt

[Andrew Lunn]

On Tue, Jul 25, 2017 at 06:15:49PM +0200, Egil Hjelmeland wrote:
> Allowing per-port access to Switch Engine Broadcast Throttling Register

Hi Egil

In general, we are against using sysfs. If there is a generic
mechanism, that applies for all sorts of network interfaces, it should
be used instead of sysfs.

Is this intended to reduce the effect of broadcast storms?

   Andrew

Re: [PATCH net-next v2 06/10] net: dsa: lan9303: added sysfs node swe_bcst_throt

[Egil Hjelmeland]

On 26. juli 2017 19:14, Andrew Lunn wrote:
> On Tue, Jul 25, 2017 at 06:15:49PM +0200, Egil Hjelmeland wrote:
>> Allowing per-port access to Switch Engine Broadcast Throttling Register
> 
> Hi Egil
> 
> In general, we are against using sysfs. If there is a generic
> mechanism, that applies for all sorts of network interfaces, it should
> be used instead of sysfs.
> 
> Is this intended to reduce the effect of broadcast storms?
> 
>     Andrew
> 

Yes, this setting can be used to reduce effect of broadcast storms.

I knew you all dislike using sysfs. Still I had a hope you could accept
the dsa_net_device_to_dsa_port() function it the previous patch.
I feel it would be convenient to be able to add driver sysfs nodes in
private branches for own debug and fine tuning, without having to patch
the networking core.



Egil

Re: [PATCH net-next v2 06/10] net: dsa: lan9303: added sysfs node swe_bcst_throt

[Andrew Lunn]

> Yes, this setting can be used to reduce effect of broadcast storms.

So one way to solve this is to teach the software bridge about
broadcast storm control. Put some rate limiting into its broadcast
flood handling. Then add a switchdev call which passes this down into
the switch.

Or look at doing it via TC. It is just a filter selecting broadcast
traffic and applying some shaping, which is what TC is all about.

Generic solutions which can be used by all switches are likely to be
accepted.

	Andrew

Re: [PATCH net-next v2 06/10] net: dsa: lan9303: added sysfs node swe_bcst_throt

[Jiri Pirko]

Thu, Jul 27, 2017 at 03:26:25PM CEST, andrew@lunn.ch wrote:
>> Yes, this setting can be used to reduce effect of broadcast storms.
>
>So one way to solve this is to teach the software bridge about
>broadcast storm control. Put some rate limiting into its broadcast
>flood handling. Then add a switchdev call which passes this down into
>the switch.
>
>Or look at doing it via TC. It is just a filter selecting broadcast
>traffic and applying some shaping, which is what TC is all about.

Ack. This is probably the way to do this. Definitelly no sysfs knob...

>
>Generic solutions which can be used by all switches are likely to be
>accepted.
>
>	Andrew

[PATCH net-next v2 07/10] net: dsa: lan9303: Added 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. Support for STP is also added.

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

Added brigde methods port_bridge_join, port_bridge_leave and
port_stp_state_set.

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index b70acb73aad6..426a75bd89f4 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -18,6 +18,7 @@
 #include <linux/mutex.h>
 #include <linux/mii.h>
 #include <linux/phy.h>
+#include <linux/if_bridge.h>
 
 #include "lan9303.h"
 
@@ -143,6 +144,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)
@@ -515,11 +517,30 @@ static int lan9303_enable_packet_processing(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,
+				       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
+	 */
+	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)
 {
@@ -534,22 +555,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 |
@@ -557,6 +562,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)
@@ -707,6 +718,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);
@@ -898,17 +913,81 @@ 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;
+
+	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:
+		dev_err(chip->dev, "%s(port %d, state %d)\n",
+			__func__, port, state);
+	}
+	portmask = 0x3 << (port * 2);
+	portstate     <<= (port * 2);
+	lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
+				      portstate, portmask);
+}
+
 static struct dsa_switch_ops lan9303_switch_ops = {
 	.get_tag_protocol = lan9303_get_tag_protocol,
 	.setup = lan9303_setup,
-	.get_strings = lan9303_get_strings,
 	.phy_read = lan9303_phy_read,
 	.phy_write = lan9303_phy_write,
 	.adjust_link = lan9303_adjust_link,
+	.get_strings = lan9303_get_strings,
 	.get_ethtool_stats = lan9303_get_ethtool_stats,
 	.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,
 };
 
 static int lan9303_register_switch(struct lan9303 *chip)
diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
index 444d00b460e1..2d74d02c9cef 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;
-- 
2.11.0

Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic offloading of unicast traffic

[Andrew Lunn]

Hi Egil

> +/* forward special tagged packets from port 0 to port 1 *or* port 2 */
> +static int lan9303_setup_tagging(struct lan9303 *chip)
> +{
> +	int ret;

Blank line please.


> +	/* enable defining the destination port via special VLAN tagging
> +	 * for port 0
> +	 */
> +	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
> +				       0x03);

#define for 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
> +	 */
> +	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)
>  {
> @@ -534,22 +555,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 |
> @@ -557,6 +562,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)
> @@ -707,6 +718,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);
> @@ -898,17 +913,81 @@ 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;

I think you are over-simplifying here. Say i have a layer 2 VPN and i
bridge port 1 and the VPN? The software bridge still wants to do STP
on port 1, in order to solve loops.

> +
> +	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:
> +		dev_err(chip->dev, "%s(port %d, state %d)\n",
> +			__func__, port, state);
> +	}
> +	portmask = 0x3 << (port * 2);
> +	portstate     <<= (port * 2);
> +	lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
> +				      portstate, portmask);
> +}




> +
>  static struct dsa_switch_ops lan9303_switch_ops = {
>  	.get_tag_protocol = lan9303_get_tag_protocol,
>  	.setup = lan9303_setup,
> -	.get_strings = lan9303_get_strings,

????

>  	.phy_read = lan9303_phy_read,
>  	.phy_write = lan9303_phy_write,
>  	.adjust_link = lan9303_adjust_link,
> +	.get_strings = lan9303_get_strings,

Please don't include other unrelated changes.

       Andrew

Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic offloading of unicast traffic

[Egil Hjelmeland]

On 26. juli 2017 19:24, Andrew Lunn wrote:
> Hi Egil
> 
>> +/* forward special tagged packets from port 0 to port 1 *or* port 2 */
>> +static int lan9303_setup_tagging(struct lan9303 *chip)
>> +{
>> +	int ret;
> 
> Blank line please.
> 
> 
>> +	/* enable defining the destination port via special VLAN tagging
>> +	 * for port 0
>> +	 */
>> +	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
>> +				       0x03);
> 
> #define for 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
>> +	 */
>> +	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)
>>   {
>> @@ -534,22 +555,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 |
>> @@ -557,6 +562,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)
>> @@ -707,6 +718,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);
>> @@ -898,17 +913,81 @@ 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;
> 
> I think you are over-simplifying here. Say i have a layer 2 VPN and i
> bridge port 1 and the VPN? The software bridge still wants to do STP
> on port 1, in order to solve loops.
> 

Problem is that the mainline lan9303_separate_ports() does its
work by setting port 1 & 2 in STP BLOCKING state (and port 0 in
FORWARDING state). So my understanding is that it would break port
separation if LAN9303_SWE_PORT_STATE is written while the driver
is in the non-bridged state.

I thought the SW bridge would carry doing its STP work even if
there is a port_stp_state_set method on a DSA port?

>> +
>> +	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:
>> +		dev_err(chip->dev, "%s(port %d, state %d)\n",
>> +			__func__, port, state);
>> +	}
>> +	portmask = 0x3 << (port * 2);
>> +	portstate     <<= (port * 2);
>> +	lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
>> +				      portstate, portmask);
>> +}
> 
> 
> 
> 
>> +
>>   static struct dsa_switch_ops lan9303_switch_ops = {
>>   	.get_tag_protocol = lan9303_get_tag_protocol,
>>   	.setup = lan9303_setup,
>> -	.get_strings = lan9303_get_strings,
> 
> ????
> 
>>   	.phy_read = lan9303_phy_read,
>>   	.phy_write = lan9303_phy_write,
>>   	.adjust_link = lan9303_adjust_link,
>> +	.get_strings = lan9303_get_strings,
> 
> Please don't include other unrelated changes.
> 
>         Andrew
> 

Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic offloading of unicast traffic

[Andrew Lunn]

> >I think you are over-simplifying here. Say i have a layer 2 VPN and i
> >bridge port 1 and the VPN? The software bridge still wants to do STP
> >on port 1, in order to solve loops.
> >
> 
> Problem is that the mainline lan9303_separate_ports() does its
> work by setting port 1 & 2 in STP BLOCKING state (and port 0 in
> FORWARDING state). So my understanding is that it would break port
> separation if LAN9303_SWE_PORT_STATE is written while the driver
> is in the non-bridged state.

If the hardware cannot do it, that is a different matter. But if the
hardware can do STP states per port, you should try to make use of it
here.

> I thought the SW bridge would carry doing its STP work even if
> there is a port_stp_state_set method on a DSA port?

It will, but it means you are dropping frames in software, adding
extra load to the CPU, reducing the available bandwidth for the other
port, etc.

       Andrew

Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic offloading of unicast traffic

[Egil Hjelmeland]

On 27. juli 2017 15:31, Andrew Lunn wrote:
>>> I think you are over-simplifying here. Say i have a layer 2 VPN and i
>>> bridge port 1 and the VPN? The software bridge still wants to do STP
>>> on port 1, in order to solve loops.
>>>
>>
>> Problem is that the mainline lan9303_separate_ports() does its
>> work by setting port 1 & 2 in STP BLOCKING state (and port 0 in
>> FORWARDING state). So my understanding is that it would break port
>> separation if LAN9303_SWE_PORT_STATE is written while the driver
>> is in the non-bridged state.
> 
> If the hardware cannot do it, that is a different matter. But if the
> hardware can do STP states per port, you should try to make use of it
> here.
> 

The HW does STP states per port, but not per pair of port.
I can set port 1 in learning, but I can not tell port 2
to ignore addresses learned on port 1. (Except by using VLAN).

Unless somebody can come up with an other way to implement the
port separation, I think this is how it has to be. I suppose
we don't want to break the port separation feature.


>> I thought the SW bridge would carry doing its STP work even if
>> there is a port_stp_state_set method on a DSA port?
> 
> It will, but it means you are dropping frames in software, adding
> extra load to the CPU, reducing the available bandwidth for the other
> port, etc.
> 

That is exactly the case with all traffic with the current mainline
driver.


>         Andrew
> 

Egil

Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic offloading of unicast traffic

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2199 bytes --]

Hi Egil,

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Egil-Hjelmeland/net-dsa-lan9303-unicast-offload-fdb-mdb-STP/20170727-074246
config: x86_64-randconfig-x000-201730 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/net//dsa/lan9303-core.c: In function 'lan9303_port_stp_state_set':
>> drivers/net//dsa/lan9303-core.c:945:16: warning: 'portstate' may be used uninitialized in this function [-Wmaybe-uninitialized]
     int portmask, portstate;
                   ^~~~~~~~~

vim +/portstate +945 drivers/net//dsa/lan9303-core.c

   941	
   942	static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
   943					       u8 state)
   944	{
 > 945		int portmask, portstate;
   946		struct lan9303 *chip = ds->priv;
   947	
   948		dev_dbg(chip->dev, "%s(port %d, state %d)\n",
   949			__func__, port, state);
   950		if (!chip->is_bridged)
   951			return;
   952	
   953		switch (state) {
   954		case BR_STATE_DISABLED:
   955			portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
   956			break;
   957		case BR_STATE_BLOCKING:
   958		case BR_STATE_LISTENING:
   959			portstate = LAN9303_SWE_PORT_STATE_BLOCKING_PORT0;
   960			break;
   961		case BR_STATE_LEARNING:
   962			portstate = LAN9303_SWE_PORT_STATE_LEARNING_PORT0;
   963			break;
   964		case BR_STATE_FORWARDING:
   965			portstate = LAN9303_SWE_PORT_STATE_FORWARDING_PORT0;
   966			break;
   967		default:
   968			dev_err(chip->dev, "%s(port %d, state %d)\n",
   969				__func__, port, state);
   970		}
   971		portmask = 0x3 << (port * 2);
   972		portstate     <<= (port * 2);
   973		lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
   974					      portstate, portmask);
   975	}
   976	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27789 bytes --]

[PATCH net-next v2 08/10] net: dsa: lan9303: Added ALR/fdb/mdb handling

[Egil Hjelmeland]

Added functions for accessing / managing the lan9303 ALR (Address Logic
Resolution).

Implemented DSA methods: set_addr, port_fast_age, port_fdb_prepare,
port_fdb_add, port_fdb_del, port_fdb_dump, port_mdb_prepare,
port_mdb_add and port_mdb_del.

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

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 426a75bd89f4..dc95973d62ed 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -19,6 +19,7 @@
 #include <linux/mii.h>
 #include <linux/phy.h>
 #include <linux/if_bridge.h>
+#include <linux/etherdevice.h>
 
 #include "lan9303.h"
 
@@ -121,6 +122,21 @@
 #define LAN9303_MAC_RX_CFG_2 0x0c01
 #define LAN9303_MAC_TX_CFG_2 0x0c40
 #define LAN9303_SWE_ALR_CMD 0x1800
+# define ALR_CMD_MAKE_ENTRY    BIT(2)
+# define ALR_CMD_GET_FIRST     BIT(1)
+# define ALR_CMD_GET_NEXT      BIT(0)
+#define LAN9303_SWE_ALR_WR_DAT_0 0x1801
+#define LAN9303_SWE_ALR_WR_DAT_1 0x1802
+# define ALR_DAT1_VALID        BIT(26)
+# define ALR_DAT1_END_OF_TABL  BIT(25)
+# define ALR_DAT1_AGE_OVERRID  BIT(25)
+# define ALR_DAT1_STATIC       BIT(24)
+# define ALR_DAT1_PORT_BITOFFS  16
+# define ALR_DAT1_PORT_MASK    (7 << ALR_DAT1_PORT_BITOFFS)
+#define LAN9303_SWE_ALR_RD_DAT_0 0x1805
+#define LAN9303_SWE_ALR_RD_DAT_1 0x1806
+#define LAN9303_SWE_ALR_CMD_STS 0x1808
+# define ALR_STS_MAKE_PEND     BIT(0)
 #define LAN9303_SWE_VLAN_CMD 0x180b
 # define LAN9303_SWE_VLAN_CMD_RNW BIT(5)
 # define LAN9303_SWE_VLAN_CMD_PVIDNVLAN BIT(4)
@@ -473,6 +489,229 @@ 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: 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 */
+/* Otherwise, return timeout */
+static int lan9303_csr_reg_wait(struct lan9303 *chip, int regno,
+				int mask, char value)
+{
+	int i;
+
+	for (i = 0; i < 0x1000; i++) {
+		u32 reg;
+
+		lan9303_read_switch_reg(chip, regno, &reg);
+		if ((reg & mask) == value)
+			return 0;
+	}
+	return -ETIMEDOUT;
+}
+
+static int _lan9303_alr_make_entry_raw(struct lan9303 *chip, u32 dat0, u32 dat1)
+{
+	lan9303_write_switch_reg(
+		chip, LAN9303_SWE_ALR_WR_DAT_0, dat0);
+	lan9303_write_switch_reg(
+		chip, LAN9303_SWE_ALR_WR_DAT_1, dat1);
+	lan9303_write_switch_reg(
+		chip, LAN9303_SWE_ALR_CMD, ALR_CMD_MAKE_ENTRY);
+	lan9303_csr_reg_wait(
+		chip, LAN9303_SWE_ALR_CMD_STS, ALR_STS_MAKE_PEND, 0);
+	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
+	return 0;
+}
+
+typedef void alr_loop_cb_t(
+	struct lan9303 *chip, u32 dat0, u32 dat1, int portmap, void *ctx);
+
+static void lan9303_alr_loop(struct lan9303 *chip, alr_loop_cb_t *cb, void *ctx)
+{
+	int i;
+
+	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_FIRST);
+	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
+
+	for (i = 1; i < LAN9303_NUM_ALR_RECORDS; i++) {
+		u32 dat0, dat1;
+		int alrport, portmap;
+
+		lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_0, &dat0);
+		lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_1, &dat1);
+		if (dat1 & ALR_DAT1_END_OF_TABL)
+			break;
+
+		alrport = (dat1 & ALR_DAT1_PORT_MASK) >> ALR_DAT1_PORT_BITOFFS;
+		portmap = alrport_2_portmap[alrport];
+
+		cb(chip, dat0, dat1, portmap, ctx);
+
+		lan9303_write_switch_reg(
+			chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_NEXT);
+		lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
+	}
+}
+
+/* ALR: lan9303_alr_loop callback functions */
+
+static void _alr_reg_to_mac(u32 dat0, u32 dat1, u8 mac[6])
+{
+	mac[0] = (dat0 >>  0) & 0xff;
+	mac[1] = (dat0 >>  8) & 0xff;
+	mac[2] = (dat0 >> 16) & 0xff;
+	mac[3] = (dat0 >> 24) & 0xff;
+	mac[4] = (dat1 >>  0) & 0xff;
+	mac[5] = (dat1 >>  8) & 0xff;
+}
+
+/* Clear learned (non-static) entry on given port */
+static void alr_loop_cb_del_port_learned(
+	struct lan9303 *chip, u32 dat0, u32 dat1, int portmap, void *ctx)
+{
+	int *port = ctx;
+
+	if (((BIT(*port) & portmap) == 0) || (dat1 & ALR_DAT1_STATIC))
+		return;
+
+	/* learned entries has only one port, we can just delete */
+	dat1 &= ~ALR_DAT1_VALID; /* delete entry */
+	_lan9303_alr_make_entry_raw(chip, dat0, dat1);
+}
+
+struct port_fdb_dump_ctx {
+	int port;
+	struct switchdev_obj_port_fdb *fdb;
+	switchdev_obj_dump_cb_t       *cb;
+};
+
+static void alr_loop_cb_fdb_port_dump(
+	struct lan9303 *chip, u32 dat0, u32 dat1, int portmap, void *ctx)
+{
+	struct port_fdb_dump_ctx *dump_ctx = ctx;
+	struct switchdev_obj_port_fdb *fdb = dump_ctx->fdb;
+	u8 mac[ETH_ALEN];
+
+	if ((BIT(dump_ctx->port) & portmap) == 0)
+		return;
+
+	_alr_reg_to_mac(dat0, dat1, mac);
+	ether_addr_copy(fdb->addr, mac);
+	fdb->vid = 0;
+	fdb->ndm_state = (dat1 & ALR_DAT1_STATIC) ?
+		NUD_NOARP : NUD_REACHABLE;
+	dump_ctx->cb(&fdb->obj);
+}
+
+/* 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;
+
+	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);
+
+	dev_dbg(chip->dev, "%s %pM %d %08x %08x\n",
+		__func__, mac, port_map, dat0, dat1);
+	_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 = 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 = lan9303_alr_cache_find_mac(
+		chip, mac);
+
+	if (!entr) { /* no static entry found */
+		/* Should we delete any learned entry?
+		 * _lan9303_alr_set_entry(chip, mac, 0, false);
+		 */
+		return 0;
+	}
+	entr->port_map &= ~BIT(port); /* zero means its free again */
+	if (entr->port_map == 0)
+		eth_zero_addr(&entr->port_map);
+	_lan9303_alr_set_entry(chip, mac, entr->port_map, entr->stp_override);
+	return 0;
+}
+
+/* --------------------- Various chip setup ----------------------*/
 static int lan9303_disable_packet_processing(struct lan9303 *chip,
 					     unsigned int port)
 {
@@ -729,6 +968,14 @@ static int lan9303_setup(struct dsa_switch *ds)
 	return 0;
 }
 
+static int lan9303_set_addr(struct dsa_switch *ds, u8 *addr)
+{
+	struct lan9303 *chip = ds->priv;
+
+	lan9303_alr_add_port(chip, addr, 0, false);
+	return 0;
+}
+
 struct lan9303_mib_desc {
 	unsigned int offset; /* offset of first MAC */
 	const char *name;
@@ -974,9 +1221,123 @@ static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
 				      portstate, portmask);
 }
 
+static void lan9303_port_fast_age(struct dsa_switch *ds, int port)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(%d)\n", __func__, port);
+	lan9303_alr_loop(chip, alr_loop_cb_del_port_learned, &port);
+}
+
+static int _lan9303_port_fdb_check(
+	struct lan9303 *chip, const u8 *mac, int vid)
+{
+	if (vid)
+		return -EOPNOTSUPP;
+	if (lan9303_alr_cache_find_mac(chip, mac))
+		return 0;
+	if (!lan9303_alr_cache_find_free(chip))
+		return -ENOSPC;
+	return 0;
+}
+
+static int lan9303_port_fdb_prepare(
+		struct dsa_switch *ds, int port,
+		const struct switchdev_obj_port_fdb *fdb,
+		struct switchdev_trans *trans)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(port %d, vid %d, %pM)\n",
+		__func__, port, fdb->vid, fdb->addr);
+	return _lan9303_port_fdb_check(chip, fdb->addr, fdb->vid);
+}
+
+static void lan9303_port_fdb_add(
+		struct dsa_switch *ds, int port,
+		const struct switchdev_obj_port_fdb *fdb,
+		struct switchdev_trans *trans)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(port %d, vid %d, %pM)\n",
+		__func__, port, fdb->vid, fdb->addr);
+	lan9303_alr_add_port(chip, fdb->addr, port, false);
+}
+
+static int lan9303_port_fdb_del(
+		struct dsa_switch *ds, int port,
+		const struct switchdev_obj_port_fdb *fdb)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(port %d, vid %d, %pM)\n",
+		__func__, port, fdb->vid, fdb->addr);
+	if (fdb->vid)
+		return -EOPNOTSUPP;
+	lan9303_alr_del_port(chip, fdb->addr, port);
+	return 0;
+}
+
+static int lan9303_port_fdb_dump(
+		struct dsa_switch *ds, int port,
+		struct switchdev_obj_port_fdb *fdb,
+		switchdev_obj_dump_cb_t *cb)
+{
+	struct lan9303 *chip = ds->priv;
+	struct port_fdb_dump_ctx dump_ctx = {
+		.port = port,
+		.fdb  = fdb,
+		.cb   = cb,
+	};
+
+	dev_dbg(chip->dev, "%s(%d)\n", __func__, port);
+	lan9303_alr_loop(chip, alr_loop_cb_fdb_port_dump, &dump_ctx);
+	return 0;
+}
+
+static int lan9303_port_mdb_prepare(
+		struct dsa_switch *ds, int port,
+		const struct switchdev_obj_port_mdb *mdb,
+		struct switchdev_trans *trans)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(port %d, vid %d, %pM)\n",
+		__func__, port, mdb->vid, mdb->addr);
+	return _lan9303_port_fdb_check(chip, mdb->addr, mdb->vid);
+}
+
+static void lan9303_port_mdb_add(
+		struct dsa_switch *ds, int port,
+		const struct switchdev_obj_port_mdb *mdb,
+		struct switchdev_trans *trans)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(port %d, vid %d, %pM)\n",
+		__func__, port, mdb->vid, mdb->addr);
+	lan9303_alr_add_port(chip, mdb->addr, port, false);
+}
+
+static int lan9303_port_mdb_del(
+		struct dsa_switch *ds, int port,
+		const struct switchdev_obj_port_mdb *mdb)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(port %d, vid %d, %pM)\n",
+		__func__, port, mdb->vid, mdb->addr);
+	if (mdb->vid)
+		return -EOPNOTSUPP;
+	lan9303_alr_del_port(chip, mdb->addr, port);
+	return 0;
+}
+
 static struct dsa_switch_ops lan9303_switch_ops = {
 	.get_tag_protocol = lan9303_get_tag_protocol,
 	.setup = lan9303_setup,
+	.set_addr = lan9303_set_addr,
 	.phy_read = lan9303_phy_read,
 	.phy_write = lan9303_phy_write,
 	.adjust_link = lan9303_adjust_link,
@@ -988,6 +1349,14 @@ static struct dsa_switch_ops lan9303_switch_ops = {
 	.port_bridge_join       = lan9303_port_bridge_join,
 	.port_bridge_leave      = lan9303_port_bridge_leave,
 	.port_stp_state_set     = lan9303_port_stp_state_set,
+	.port_fast_age          = lan9303_port_fast_age,
+	.port_fdb_prepare       = lan9303_port_fdb_prepare,
+	.port_fdb_add           = lan9303_port_fdb_add,
+	.port_fdb_del           = lan9303_port_fdb_del,
+	.port_fdb_dump          = lan9303_port_fdb_dump,
+	.port_mdb_prepare       = lan9303_port_mdb_prepare,
+	.port_mdb_add           = lan9303_port_mdb_add,
+	.port_mdb_del           = lan9303_port_mdb_del,
 };
 
 static int lan9303_register_switch(struct lan9303 *chip)
diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
index 2d74d02c9cef..f714addbf1e2 100644
--- a/drivers/net/dsa/lan9303.h
+++ b/drivers/net/dsa/lan9303.h
@@ -11,6 +11,13 @@ struct lan9303_phy_ops {
 			     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;
@@ -22,6 +29,10 @@ 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 is bridged */
+	/* LAN9303 do not offer reading specific ALR entry. Cache all
+	 * static entries in a flat table
+	 **/
+	struct lan9303_alr_cache_entry alr_cache[LAN9303_NUM_ALR_RECORDS];
 };
 
 extern const struct regmap_access_table lan9303_register_set;
-- 
2.11.0

Re: [PATCH net-next v2 08/10] net: dsa: lan9303: Added ALR/fdb/mdb handling

[Andrew Lunn]

Hi Egil

> +/* This function will wait a while until mask & reg == value */
> +/* Otherwise, return timeout */
> +static int lan9303_csr_reg_wait(struct lan9303 *chip, int regno,
> +				int mask, char value)
> +{
> +	int i;
> +
> +	for (i = 0; i < 0x1000; i++) {
> +		u32 reg;
> +
> +		lan9303_read_switch_reg(chip, regno, &reg);
> +		if ((reg & mask) == value)
> +			return 0;
> +	}
> +	return -ETIMEDOUT;

Busy looping is probably not a good idea. Can you add a usleep()?

> +}
> +
> +static int _lan9303_alr_make_entry_raw(struct lan9303 *chip, u32 dat0, u32 dat1)

What does the _ indicate. I could understand having it when you have
lan9303_alr_make_entry_raw() call _lan9303_alr_make_entry_raw() after
taking a lock, but i don't see anything like that here.

> +{
> +	lan9303_write_switch_reg(
> +		chip, LAN9303_SWE_ALR_WR_DAT_0, dat0);
> +	lan9303_write_switch_reg(
> +		chip, LAN9303_SWE_ALR_WR_DAT_1, dat1);
> +	lan9303_write_switch_reg(
> +		chip, LAN9303_SWE_ALR_CMD, ALR_CMD_MAKE_ENTRY);
> +	lan9303_csr_reg_wait(
> +		chip, LAN9303_SWE_ALR_CMD_STS, ALR_STS_MAKE_PEND, 0);
> +	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
> +	return 0;
> +}
> +
> +typedef void alr_loop_cb_t(
> +	struct lan9303 *chip, u32 dat0, u32 dat1, int portmap, void *ctx);
> +
> +static void lan9303_alr_loop(struct lan9303 *chip, alr_loop_cb_t *cb, void *ctx)
> +{
> +	int i;
> +
> +	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_FIRST);
> +	lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
> +
> +	for (i = 1; i < LAN9303_NUM_ALR_RECORDS; i++) {
> +		u32 dat0, dat1;
> +		int alrport, portmap;
> +
> +		lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_0, &dat0);
> +		lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_1, &dat1);
> +		if (dat1 & ALR_DAT1_END_OF_TABL)
> +			break;
> +
> +		alrport = (dat1 & ALR_DAT1_PORT_MASK) >> ALR_DAT1_PORT_BITOFFS;
> +		portmap = alrport_2_portmap[alrport];
> +
> +		cb(chip, dat0, dat1, portmap, ctx);
> +
> +		lan9303_write_switch_reg(
> +			chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_NEXT);
> +		lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0);
> +	}
> +}
> +
> +/* ALR: lan9303_alr_loop callback functions */
> +
> +static void _alr_reg_to_mac(u32 dat0, u32 dat1, u8 mac[6])
> +{
> +	mac[0] = (dat0 >>  0) & 0xff;
> +	mac[1] = (dat0 >>  8) & 0xff;
> +	mac[2] = (dat0 >> 16) & 0xff;
> +	mac[3] = (dat0 >> 24) & 0xff;
> +	mac[4] = (dat1 >>  0) & 0xff;
> +	mac[5] = (dat1 >>  8) & 0xff;
> +}
> +
> +/* Clear learned (non-static) entry on given port */
> +static void alr_loop_cb_del_port_learned(
> +	struct lan9303 *chip, u32 dat0, u32 dat1, int portmap, void *ctx)
> +{
> +	int *port = ctx;
> +
> +	if (((BIT(*port) & portmap) == 0) || (dat1 & ALR_DAT1_STATIC))
> +		return;
> +
> +	/* learned entries has only one port, we can just delete */
> +	dat1 &= ~ALR_DAT1_VALID; /* delete entry */
> +	_lan9303_alr_make_entry_raw(chip, dat0, dat1);
> +}
> +
> +struct port_fdb_dump_ctx {
> +	int port;
> +	struct switchdev_obj_port_fdb *fdb;
> +	switchdev_obj_dump_cb_t       *cb;
> +};
> +
> +static void alr_loop_cb_fdb_port_dump(
> +	struct lan9303 *chip, u32 dat0, u32 dat1, int portmap, void *ctx)
> +{
> +	struct port_fdb_dump_ctx *dump_ctx = ctx;
> +	struct switchdev_obj_port_fdb *fdb = dump_ctx->fdb;
> +	u8 mac[ETH_ALEN];
> +
> +	if ((BIT(dump_ctx->port) & portmap) == 0)
> +		return;
> +
> +	_alr_reg_to_mac(dat0, dat1, mac);
> +	ether_addr_copy(fdb->addr, mac);
> +	fdb->vid = 0;
> +	fdb->ndm_state = (dat1 & ALR_DAT1_STATIC) ?
> +		NUD_NOARP : NUD_REACHABLE;
> +	dump_ctx->cb(&fdb->obj);
> +}
> +
> +/* 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;
> +
> +	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);
> +
> +	dev_dbg(chip->dev, "%s %pM %d %08x %08x\n",
> +		__func__, mac, port_map, dat0, dat1);
> +	_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 = lan9303_alr_cache_find_mac(
> +		chip, mac);

A long line like this should be split into a declaration and an
assignment.

> +
> +	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 = lan9303_alr_cache_find_mac(
> +		chip, mac);
> +
> +	if (!entr) { /* no static entry found */
> +		/* Should we delete any learned entry?
> +		 * _lan9303_alr_set_entry(chip, mac, 0, false);
> +		 */
> +		return 0;
> +	}
> +	entr->port_map &= ~BIT(port); /* zero means its free again */
> +	if (entr->port_map == 0)
> +		eth_zero_addr(&entr->port_map);
> +	_lan9303_alr_set_entry(chip, mac, entr->port_map, entr->stp_override);
> +	return 0;
> +}
> +
> +/* --------------------- Various chip setup ----------------------*/
>  static int lan9303_disable_packet_processing(struct lan9303 *chip,
>  					     unsigned int port)
>  {
> @@ -729,6 +968,14 @@ static int lan9303_setup(struct dsa_switch *ds)
>  	return 0;
>  }
>  
> +static int lan9303_set_addr(struct dsa_switch *ds, u8 *addr)
> +{
> +	struct lan9303 *chip = ds->priv;
> +
> +	lan9303_alr_add_port(chip, addr, 0, false);
> +	return 0;
> +}
> +

I would probably make this a separate patch.

  Andrew

Re: [PATCH net-next v2 08/10] net: dsa: lan9303: Added ALR/fdb/mdb handling

[Egil Hjelmeland]

On 26. juli 2017 19:41, Andrew Lunn wrote:
> Hi Egil
> 
>> +/* This function will wait a while until mask & reg == value */
>> +/* Otherwise, return timeout */
>> +static int lan9303_csr_reg_wait(struct lan9303 *chip, int regno,
>> +				int mask, char value)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < 0x1000; i++) {
>> +		u32 reg;
>> +
>> +		lan9303_read_switch_reg(chip, regno, &reg);
>> +		if ((reg & mask) == value)
>> +			return 0;
>> +	}
>> +	return -ETIMEDOUT;
> 
> Busy looping is probably not a good idea. Can you add a usleep()?
> 

Yes

>> +}
>> +
>> +static int _lan9303_alr_make_entry_raw(struct lan9303 *chip, u32 dat0, u32 dat1)
> 
> What does the _ indicate. I could understand having it when you have
> lan9303_alr_make_entry_raw() call _lan9303_alr_make_entry_raw() after
> taking a lock, but i don't see anything like that here.
> 
Just my sloppy convention for something private, deep down. I can remove
the _.

>> +{
>> +	struct lan9303_alr_cache_entry *entr = lan9303_alr_cache_find_mac(
>> +		chip, mac);
> 
> A long line like this should be split into a declaration and an
> assignment.
> 

OK


> 
> I would probably make this a separate patch.
> 
>    Andrew
> 

Got it.

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

[Egil Hjelmeland]

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 Documentation/networking/dsa/lan9303.txt | 63 ++++++++++++++++++++++++++++++++
 1 file changed, 63 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 000000000000..ef5b3ca12a29
--- /dev/null
+++ b/Documentation/networking/dsa/lan9303.txt
@@ -0,0 +1,63 @@
+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).
+
+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. STP is also supported in this mode.
+
+If one of the user ports leave the bridge,
+the ports goes back to the initial separated operation.
+
+The driver implements the port_fdb_xxx/port_mdb_xxx methods.
+
+
+Sysfs nodes
+===========
+
+When a user port is enabled, the driver creates sysfs directory
+/sys/class/net/xxx/lan9303 with the following files:
+
+ - swe_bcst_throt (RW): Set/get 6.4.7 Broadcast Storm Control
+      Throttle Level for the port. Accesses the corresponding bits of
+      the SWE_BCST_THROT register (13.4.3.23).
+
+
+Driver limitations
+==================
+
+ - No support for VLAN
+
+
+Bridging notes
+==============
+When the user ports are bridged, broadcasts, multicasts and unknown
+frames with unknown destination are flooded by the chip. Therefore SW
+flooding must be disabled by:
+
+   echo 0 > /sys/class/net/p1/brport/broadcast_flood
+   echo 0 > /sys/class/net/p1/brport/multicast_flood
+   echo 0 > /sys/class/net/p1/brport/unicast_flood
+   echo 0 > /sys/class/net/p2/brport/broadcast_flood
+   echo 0 > /sys/class/net/p2/brport/multicast_flood
+   echo 0 > /sys/class/net/p2/brport/unicast_flood
+
-- 
2.11.0

[PATCH net-next v2 10/10] net: dsa: lan9303: Only allocate 3 ports

[Egil Hjelmeland]

Saving 2628 bytes.

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/lan9303-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index dc95973d62ed..ad7a4c72e1fb 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -23,6 +23,8 @@
 
 #include "lan9303.h"
 
+#define LAN9303_NUM_PORTS 3
+
 /* 13.2 System Control and Status Registers
  * Multiply register number by 4 to get address offset.
  */
@@ -1361,7 +1363,7 @@ static struct dsa_switch_ops lan9303_switch_ops = {
 
 static int lan9303_register_switch(struct lan9303 *chip)
 {
-	chip->ds = dsa_switch_alloc(chip->dev, DSA_MAX_PORTS);
+	chip->ds = dsa_switch_alloc(chip->dev, LAN9303_NUM_PORTS);
 	if (!chip->ds)
 		return -ENOMEM;
 
-- 
2.11.0