[LEDE-DEV,RFC] AR8327 driver: implementing get_port_stats function

Submitted by Daniel Gonzalez Cabanelas on Feb. 25, 2017, 9:51 p.m.

Details

Message ID 8021994.eIdPYGzJH3@tool
State New
Headers show

Commit Message

Daniel Gonzalez Cabanelas Feb. 25, 2017, 9:51 p.m.
Currently the switch LED trigger only shows link status, but not traffic activity by blinking
the LEDs on the AR8316/AR8327 switch. It turns out that the get_port_stats is missing.

I've made this patch, which works ok. But I'm not sure if there is any reason to not implement this 
function. And probably the function ar8xxx_sw_get_port_stats can be improved.

There are a few boards with an  AR8316/AR8327 switch that need this trigger, usually on the WAN port.
As said without get_port_stats the light stays always fixed, like if there wasn't traffic activity. We can use 
the netdev trigger but then the light is always on even when the cable is unplugged, and there isn't the
possibility of using a second LED trigger on the same port for indicating a reduced link speed.

Comments

Felix Fietkau Feb. 27, 2017, 11:40 a.m.
On 2017-02-25 22:51, Daniel Gonzalez Cabanelas wrote:
> Currently the switch LED trigger only shows link status, but not traffic activity by blinking
> the LEDs on the AR8316/AR8327 switch. It turns out that the get_port_stats is missing.
> 
> I've made this patch, which works ok. But I'm not sure if there is any reason to not implement this 
> function. And probably the function ar8xxx_sw_get_port_stats can be improved.
> 
> There are a few boards with an  AR8316/AR8327 switch that need this trigger, usually on the WAN port.
> As said without get_port_stats the light stays always fixed, like if there wasn't traffic activity. We can use 
> the netdev trigger but then the light is always on even when the cable is unplugged, and there isn't the
> possibility of using a second LED trigger on the same port for indicating a reduced link speed.
> 
> diff --git a/target/linux/generic/files/drivers/net/phy/ar8216.c b/target/linux/generic/files/drivers/net/phy/ar8216.c
> index 37877d5..ffaa77e 100644
> --- a/target/linux/generic/files/drivers/net/phy/ar8216.c
> +++ b/target/linux/generic/files/drivers/net/phy/ar8216.c
> @@ -1001,6 +1001,41 @@ ar8xxx_sw_get_port_link(struct switch_dev *dev, int port,
>  	return 0;
>  }
>  
> +int
> +ar8xxx_sw_get_port_stats(struct switch_dev *dev, int port,
> +		       struct switch_port_stats *stats)
> +{
> +	struct ar8xxx_priv *priv = swdev_to_ar8xxx(dev);
> +	const struct ar8xxx_chip *chip = priv->chip;
> +	u64 *mib_stats, mib_data;
> +	const char *mib_name;
> +	int i;
> +
> +	if (!ar8xxx_has_mib_counters(priv))
> +		return -EOPNOTSUPP;
> +
> +	if (port >= dev->ports)
> +		return -EINVAL;
> +
> +	mutex_lock(&priv->mib_lock);
> +
> +	ar8xxx_mib_fetch_port_stat(priv, port, false);
> +
> +	mib_stats = &priv->mib_stats[port * chip->num_mibs];
> +	for (i = 0; i < chip->num_mibs; i++) {
> +		mib_name = chip->mib_decs[i].name;
> +		mib_data = mib_stats[i];
> +		if (!strcmp(mib_name, "TxByte"))
> +			stats->tx_bytes = mib_data;
> +		if (!strcmp(mib_name, "RxGoodByte"))
> +			stats->rx_bytes = mib_data;
> +	}

Fetching the entire port stats only to look up two fields seems rather
excessive. Please make a function instead that will look up the register
number and fetch only the relevant registers.
The lookup can be further simplified by adding an enum for the mib_stats
array index.

- Felix
Daniel Gonzalez Cabanelas Feb. 27, 2017, 9:11 p.m.
> Fetching the entire port stats only to look up two fields seems rather
> excessive. Please make a function instead that will look up the register
> number and fetch only the relevant registers.
> The lookup can be further simplified by adding an enum for the mib_stats
> array index.
> 
> - Felix

Hi Felix. Could this function be valid?. I'm afraid I cannot make anything more
sophisticated

ar8xxx_sw_get_port_stats(struct switch_dev *dev, int port,
		       struct switch_port_stats *stats)
{
  	struct ar8xxx_priv *priv = swdev_to_ar8xxx(dev);
	unsigned int base;
	unsigned int rx_offset, tx_offset;

	if (port >= dev->ports)
		return -EINVAL;

	base = priv->chip->reg_port_stats_start +
	       priv->chip->reg_port_stats_length * port;

	if (priv->chip->num_mibs == ARRAY_SIZE(ar8236_mibs)){
		rx_offset = AR8236_STATS_RXGOODBYTE;
		tx_offset = AR8236_STATS_TXBYTE;
	}

	else if (priv->chip->num_mibs == ARRAY_SIZE(ar8216_mibs)){
		rx_offset = AR8216_STATS_RXGOODBYTE;
		tx_offset = AR8216_STATS_TXBYTE;
	}

	stats->rx_bytes = ar8xxx_read(priv, base + rx_offset);
	stats->tx_bytes = ar8xxx_read(priv, base + tx_offset);

	return 0;
}

Also tested and working ok.

Regards
Daniel

Patch hide | download patch | download mbox

diff --git a/target/linux/generic/files/drivers/net/phy/ar8216.c b/target/linux/generic/files/drivers/net/phy/ar8216.c
index 37877d5..ffaa77e 100644
--- a/target/linux/generic/files/drivers/net/phy/ar8216.c
+++ b/target/linux/generic/files/drivers/net/phy/ar8216.c
@@ -1001,6 +1001,41 @@  ar8xxx_sw_get_port_link(struct switch_dev *dev, int port,
 	return 0;
 }
 
+int
+ar8xxx_sw_get_port_stats(struct switch_dev *dev, int port,
+		       struct switch_port_stats *stats)
+{
+	struct ar8xxx_priv *priv = swdev_to_ar8xxx(dev);
+	const struct ar8xxx_chip *chip = priv->chip;
+	u64 *mib_stats, mib_data;
+	const char *mib_name;
+	int i;
+
+	if (!ar8xxx_has_mib_counters(priv))
+		return -EOPNOTSUPP;
+
+	if (port >= dev->ports)
+		return -EINVAL;
+
+	mutex_lock(&priv->mib_lock);
+
+	ar8xxx_mib_fetch_port_stat(priv, port, false);
+
+	mib_stats = &priv->mib_stats[port * chip->num_mibs];
+	for (i = 0; i < chip->num_mibs; i++) {
+		mib_name = chip->mib_decs[i].name;
+		mib_data = mib_stats[i];
+		if (!strcmp(mib_name, "TxByte"))
+			stats->tx_bytes = mib_data;
+		if (!strcmp(mib_name, "RxGoodByte"))
+			stats->rx_bytes = mib_data;
+	}
+
+	mutex_unlock(&priv->mib_lock);
+
+	return 0;
+}
+
 static int
 ar8xxx_sw_get_ports(struct switch_dev *dev, struct switch_val *val)
 {
@@ -1696,6 +1731,7 @@  static const struct switch_dev_ops ar8xxx_sw_ops = {
 	.apply_config = ar8xxx_sw_hw_apply,
 	.reset_switch = ar8xxx_sw_reset_switch,
 	.get_port_link = ar8xxx_sw_get_port_link,
+	.get_port_stats = ar8xxx_sw_get_port_stats,
 };
 
 static const struct ar8xxx_chip ar8216_chip = {
diff --git a/target/linux/generic/files/drivers/net/phy/ar8216.h b/target/linux/generic/files/drivers/net/phy/ar8216.h
index d9508b9..8b05cf3 100644
--- a/target/linux/generic/files/drivers/net/phy/ar8216.h
+++ b/target/linux/generic/files/drivers/net/phy/ar8216.h
@@ -538,6 +538,9 @@  int
 ar8xxx_sw_get_port_link(struct switch_dev *dev, int port,
 			struct switch_port_link *link);
 int
+ar8xxx_sw_get_port_stats(struct switch_dev *dev,
+		       int port, struct switch_port_stats *stats);
+int
 ar8xxx_sw_set_port_reset_mib(struct switch_dev *dev,
                              const struct switch_attr *attr,
                              struct switch_val *val);
diff --git a/target/linux/generic/files/drivers/net/phy/ar8327.c b/target/linux/generic/files/drivers/net/phy/ar8327.c
index 9b40cd7..e82563f 100644
--- a/target/linux/generic/files/drivers/net/phy/ar8327.c
+++ b/target/linux/generic/files/drivers/net/phy/ar8327.c
@@ -1372,6 +1372,7 @@  static const struct switch_dev_ops ar8327_sw_ops = {
 	.apply_config = ar8327_sw_hw_apply,
 	.reset_switch = ar8xxx_sw_reset_switch,
 	.get_port_link = ar8xxx_sw_get_port_link,
+	.get_port_stats = ar8xxx_sw_get_port_stats,
 };
 
 const struct ar8xxx_chip ar8327_chip = {