linux-kernel - Re: [net-next PATCH 12/19] net: dsa: qca8k: add set_ageing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211119014759.ailhsinh5ahqoymg@skbuf>
Date:   Fri, 19 Nov 2021 03:47:59 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Ansuel Smith <ansuelsmth@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [net-next PATCH 12/19] net: dsa: qca8k: add set_ageing_time
 support

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@...il.com>
> ---
>  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
>