Re: [net-next PATCH 12/19] net: dsa: qca8k: add set_ageing_time support

From: Vladimir Oltean
Date: Thu Nov 18 2021 - 20:48:06 EST


On Wed, Nov 17, 2021 at 10:04:44PM +0100, Ansuel Smith wrote:
> qca8k support setting ageing time in set of 7s. Add support for it and
> return error with value greater than the max value accepted of 7645m.
>
> Signed-off-by: Ansuel Smith <ansuelsmth@xxxxxxxxx>
> ---
> drivers/net/dsa/qca8k.c | 18 ++++++++++++++++++
> drivers/net/dsa/qca8k.h | 3 +++
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a74099131e3d..50f19549b97d 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -1833,6 +1833,23 @@ qca8k_port_fast_age(struct dsa_switch *ds, int port)
> mutex_unlock(&priv->reg_mutex);
> }
>
> +static int
> +qca8k_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
> +{
> + struct qca8k_priv *priv = ds->priv;
> + unsigned int secs = msecs / 1000;
> + u32 val;
> +
> + /* AGE_TIME reg is set in 7s step */
> + val = secs / 7;

A typical bug is when the user gives you an ageing time smaller than 7
seconds, like 6, and you divide and end up writing 0 to hardware, which
effectively disables learning and is not what the user intended.
Maybe DIV_ROUND_UP? Or just catch "val == 0" and replace it with 1.

> +
> + if (val > FIELD_MAX(QCA8K_ATU_AGE_TIME_MASK))
> + return -ERANGE;

You can set ds->ageing_time_max and it could do the check for you.

> +
> + return regmap_update_bits(priv->regmap, QCA8K_REG_ATU_CTRL, QCA8K_ATU_AGE_TIME_MASK,
> + QCA8K_ATU_AGE_TIME(val));
> +}
> +
> static int
> qca8k_port_enable(struct dsa_switch *ds, int port,
> struct phy_device *phy)
> @@ -2125,6 +2142,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> .get_strings = qca8k_get_strings,
> .get_ethtool_stats = qca8k_get_ethtool_stats,
> .get_sset_count = qca8k_get_sset_count,
> + .set_ageing_time = qca8k_set_ageing_time,
> .get_mac_eee = qca8k_get_mac_eee,
> .set_mac_eee = qca8k_set_mac_eee,
> .port_enable = qca8k_port_enable,
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index d25afdab4dea..e1298179d7cb 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -175,6 +175,9 @@
> #define QCA8K_VTU_FUNC1_BUSY BIT(31)
> #define QCA8K_VTU_FUNC1_VID_MASK GENMASK(27, 16)
> #define QCA8K_VTU_FUNC1_FULL BIT(4)
> +#define QCA8K_REG_ATU_CTRL 0x618
> +#define QCA8K_ATU_AGE_TIME_MASK GENMASK(15, 0)
> +#define QCA8K_ATU_AGE_TIME(x) FIELD_PREP(QCA8K_ATU_AGE_TIME_MASK, (x))
> #define QCA8K_REG_GLOBAL_FW_CTRL0 0x620
> #define QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN BIT(10)
> #define QCA8K_GLOBAL_FW_CTRL0_MIRROR_PORT_NUM GENMASK(7, 4)
> --
> 2.32.0
>