[PATCH 12/13] net: dsa: lan9303: Added "stp_enable" sysfs attribute

[PATCH 12/13] net: dsa: lan9303: Added "stp_enable" sysfs attribute

[Egil Hjelmeland]

Must be set to 1 by user space when STP is used on the lan9303.
If bridging without local STP, leave at 0, so external STP BPDUs
are forwarded.

Hopefully the kernel can be improved so the driver can handle this
without user intervention, and this control can be removed.

Signed-off-by: Egil Hjelmeland <egil.hjelmeland@zenitel.com>
---
 Documentation/networking/dsa/lan9303.txt | 23 ++++++++++++++++++++++
 drivers/net/dsa/lan9303-core.c           | 33 ++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/Documentation/networking/dsa/lan9303.txt b/Documentation/networking/dsa/lan9303.txt
index ace91c821ce7..0694b6646d2a 100644
--- a/Documentation/networking/dsa/lan9303.txt
+++ b/Documentation/networking/dsa/lan9303.txt
@@ -40,6 +40,10 @@ When a user port is enabled, the driver creates sysfs directory
  - alr_dump (RO): List the 168 first entries of the ALR table.
       Including port 0 entires. This file is identical for both ports.
       Format: MAC; list of ports; (l)earned / (s)tatic
+ - stp_enable (RW): Must be set to 1 when STP is used. Installs an ALR
+      entry so that received STP BPDUs are only sent to port 0.
+      When 0 (default) received STP BPDUs are forwarded to all ports.
+      This file is identical for both ports.
  - 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).
@@ -49,3 +53,22 @@ 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
+
+If enabling local STP, the LAN9303 must be configured to forward
+BPDUs only to port 0, by writing 1 to "stp_enable" of one of the ports:
+
+   echo 1 > /sys/class/net/p1/lan9303/stp_enable
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index b682aa4f1fca..54646eb38185 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -187,6 +187,8 @@
 #define MII_LAN911X_SPECIAL_MODES 0x12
 #define MII_LAN911X_SPECIAL_CONTROL_STATUS 0x1f
 
+#define eth_stp_addr eth_reserved_addr_base
+
 static const struct regmap_range lan9303_valid_regs[] = {
 	regmap_reg_range(0x14, 0x17), /* misc, interrupt */
 	regmap_reg_range(0x19, 0x19), /* endian test */
@@ -988,9 +990,40 @@ alr_dump_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(alr_dump);
 
+static ssize_t
+stp_enable_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;
+	char result = lan9303_alr_cache_find_mac(chip, eth_stp_addr) ?
+		'1' : '0';
+	return scnprintf(buf, PAGE_SIZE, "%c\n", result);
+}
+
+static ssize_t
+stp_enable_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;
+	unsigned long enable;
+	int ret = kstrtoul(buf, 0, &enable);
+
+	if (ret)
+		return ret;
+	if (enable)
+		lan9303_alr_add_port(chip, eth_stp_addr, 0, true);
+	else
+		lan9303_alr_del_port(chip, eth_stp_addr, 0);
+	return len;
+}
+static DEVICE_ATTR_RW(stp_enable);
+
 static struct attribute *lan9303_attrs[] = {
 	&dev_attr_swe_bcst_throt.attr,
 	&dev_attr_alr_dump.attr,
+	&dev_attr_stp_enable.attr,
 	NULL
 };
 
-- 
2.11.0


DISCLAIMER:
This e-mail may contain confidential and privileged material for the sole use of the intended recipient. Any review, use, distribution or disclosure by others is strictly prohibited. If you are not the intended recipient (or authorized to receive for the recipient), please contact the sender by reply e-mail and delete all copies of this message.

Re: [PATCH 12/13] net: dsa: lan9303: Added "stp_enable" sysfs attribute

[Florian Fainelli]

On 07/20/2017 06:42 AM, Egil Hjelmeland wrote:
> Must be set to 1 by user space when STP is used on the lan9303.
> If bridging without local STP, leave at 0, so external STP BPDUs
> are forwarded.
> 
> Hopefully the kernel can be improved so the driver can handle this
> without user intervention, and this control can be removed.

Same here, we can't have a driver-specific sysfs attribute just for
this, either we find a way to have the bridge's STP settings propagate
correctly to the switch driver, or you have to make better decisions
based on hints/calls you are getting from switchdev -> dsa -> driver.

> 
> Signed-off-by: Egil Hjelmeland <egil.hjelmeland@zenitel.com>
> ---
>  Documentation/networking/dsa/lan9303.txt | 23 ++++++++++++++++++++++
>  drivers/net/dsa/lan9303-core.c           | 33 ++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/Documentation/networking/dsa/lan9303.txt b/Documentation/networking/dsa/lan9303.txt
> index ace91c821ce7..0694b6646d2a 100644
> --- a/Documentation/networking/dsa/lan9303.txt
> +++ b/Documentation/networking/dsa/lan9303.txt
> @@ -40,6 +40,10 @@ When a user port is enabled, the driver creates sysfs directory
>   - alr_dump (RO): List the 168 first entries of the ALR table.
>        Including port 0 entires. This file is identical for both ports.
>        Format: MAC; list of ports; (l)earned / (s)tatic
> + - stp_enable (RW): Must be set to 1 when STP is used. Installs an ALR
> +      entry so that received STP BPDUs are only sent to port 0.
> +      When 0 (default) received STP BPDUs are forwarded to all ports.
> +      This file is identical for both ports.
>   - 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).
> @@ -49,3 +53,22 @@ 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
> +
> +If enabling local STP, the LAN9303 must be configured to forward
> +BPDUs only to port 0, by writing 1 to "stp_enable" of one of the ports:
> +
> +   echo 1 > /sys/class/net/p1/lan9303/stp_enable
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index b682aa4f1fca..54646eb38185 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -187,6 +187,8 @@
>  #define MII_LAN911X_SPECIAL_MODES 0x12
>  #define MII_LAN911X_SPECIAL_CONTROL_STATUS 0x1f
>  
> +#define eth_stp_addr eth_reserved_addr_base
> +
>  static const struct regmap_range lan9303_valid_regs[] = {
>  	regmap_reg_range(0x14, 0x17), /* misc, interrupt */
>  	regmap_reg_range(0x19, 0x19), /* endian test */
> @@ -988,9 +990,40 @@ alr_dump_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(alr_dump);
>  
> +static ssize_t
> +stp_enable_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;
> +	char result = lan9303_alr_cache_find_mac(chip, eth_stp_addr) ?
> +		'1' : '0';
> +	return scnprintf(buf, PAGE_SIZE, "%c\n", result);
> +}
> +
> +static ssize_t
> +stp_enable_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;
> +	unsigned long enable;
> +	int ret = kstrtoul(buf, 0, &enable);
> +
> +	if (ret)
> +		return ret;
> +	if (enable)
> +		lan9303_alr_add_port(chip, eth_stp_addr, 0, true);
> +	else
> +		lan9303_alr_del_port(chip, eth_stp_addr, 0);
> +	return len;
> +}
> +static DEVICE_ATTR_RW(stp_enable);
> +
>  static struct attribute *lan9303_attrs[] = {
>  	&dev_attr_swe_bcst_throt.attr,
>  	&dev_attr_alr_dump.attr,
> +	&dev_attr_stp_enable.attr,
>  	NULL
>  };
>  
> 


-- 
Florian

Re: [PATCH 12/13] net: dsa: lan9303: Added "stp_enable" sysfs attribute

[Egil Hjelmeland]

On 24. juli 2017 18:55, Florian Fainelli wrote:
> On 07/20/2017 06:42 AM, Egil Hjelmeland wrote:
>> Must be set to 1 by user space when STP is used on the lan9303.
>> If bridging without local STP, leave at 0, so external STP BPDUs
>> are forwarded.
>>
>> Hopefully the kernel can be improved so the driver can handle this
>> without user intervention, and this control can be removed.
> 
> Same here, we can't have a driver-specific sysfs attribute just for
> this, either we find a way to have the bridge's STP settings propagate
> correctly to the switch driver, or you have to make better decisions
> based on hints/calls you are getting from switchdev -> dsa -> driver.
> 

I can't see that the driver gets enough information now. But please
correct me if I am wrong. Problem is that when disabling
multicast_flood, then the BPDUs are not forwarded by the SW bridge,
so I can not have the 01:80:c2:00:00:00 entry in always.

Perhaps the kernel could do port_fdb_add/del on 01:80:c2:00:00:00
when STP is turned on/off? Or could that break other DSA chips?

When we are at it, it would be good if the driver could return some
capability information to the kernel, so it can adapt accordingly.
It does not feel right that user space has to disable the xxxx_flood
flags, the kernel should be able to figure that out by it self.

DISCLAIMER:
This e-mail may contain confidential and privileged material for the sole use of the intended recipient. Any review, use, distribution or disclosure by others is strictly prohibited. If you are not the intended recipient (or authorized to receive for the recipient), please contact the sender by reply e-mail and delete all copies of this message.