Received-SPF: pass (google.com: domain of linux-kernel+bounces-129673-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 3 Apr 2024 14:36:22 +0300
From: Vladimir Oltean <olteanv@gmail.com>
To: Oleksij Rempel <o.rempel@pengutronix.de>
Cc: "David S. Miller" <davem@davemloft.net>, Andrew Lunn <andrew@lunn.ch>,
	Eric Dumazet <edumazet@google.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Woojung Huh <woojung.huh@microchip.com>,
	Arun Ramadoss <arun.ramadoss@microchip.com>, kernel@pengutronix.de,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	UNGLinuxDriver@microchip.com, David Ahern <dsahern@kernel.org>,
	Simon Horman <horms@kernel.org>,
	Willem de Bruijn <willemb@google.com>,
	=?utf-8?B?U8O4cmVu?= Andersen <san@skov.dk>
Subject: Re: [PATCH net-next v2 2/9] net: dsa: microchip: add IPV information
 support
Message-ID: <20240403113622.cg7gf3ejh4kb2uji@skbuf>
References: <20240403092905.2107522-1-o.rempel@pengutronix.de>
 <20240403092905.2107522-3-o.rempel@pengutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240403092905.2107522-3-o.rempel@pengutronix.de>

On Wed, Apr 03, 2024 at 11:28:58AM +0200, Oleksij Rempel wrote:
> Most of Microchip KSZ switches use Internal Priority Value associated
> with every frame. For example, it is possible to map any VLAN PCP or
> DSCP value to IPV and at the end, map IPV to a queue.
> 
> Since amount of IPVs is not equal to amount of queues, add this
> information and make use of it in some functions.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> ---
> @@ -2296,6 +2313,13 @@ static int ksz_setup(struct dsa_switch *ds)
>  
>  	dev->dev_ops->enable_stp_addr(dev);
>  
> +	/* Make sure driver provide plausible queue and IPV values */

provides

> +	if (!dev->info->num_tx_queues ||
> +	    dev->info->num_tx_queues > dev->info->max_ipvs) {

This only works because "max_ipvs" actually holds the _number_ of IPVs.
If it actually held _max_, it would have been off by one. Conclusion:
please rename it num_ipvs.

> +		dev_err(dev->dev, "Number of TX queues exceeds maximum supported IPVs\n");
> +		return -EINVAL;

Is the validation actually that helpful? FWIW, more TX queues than IPVs
is nothing out of the ordinary (maybe not for this hardware IP). You can
do multi-queue egress scheduling for a traffic class, even mqprio allows
configuring this (e.g. "2@0 2@2" which means 4 queues using 2 TCs).

> +	}
> +
>  	ds->num_tx_queues = dev->info->num_tx_queues;
>  
>  	regmap_update_bits(ksz_regmap_8(dev), regs[S_MULTICAST_CTRL],
> @@ -3522,7 +3546,7 @@ static int ksz_tc_ets_add(struct ksz_device *dev, int port,
>  	for (tc_prio = 0; tc_prio < ARRAY_SIZE(p->priomap); tc_prio++) {
>  		int queue;
>  
> -		if (tc_prio > KSZ9477_MAX_TC_PRIO)
> +		if (tc_prio >= dev->info->max_ipvs)

Same here (please rename max_ipvs to num_ipvs). Otherwise, the operator
change makes no sense (we have "max" -> "max" but ">" -> ">=").

>  			break;
>  
>  		queue = ksz_ets_band_to_queue(p, p->priomap[tc_prio]);
> @@ -3564,7 +3588,7 @@ static int ksz_tc_ets_del(struct ksz_device *dev, int port)
>  	/* Revert the queue mapping for TC-priority to its default setting on
>  	 * the chip.
>  	 */
> -	for (tc_prio = 0; tc_prio <= KSZ9477_MAX_TC_PRIO; tc_prio++) {
> +	for (tc_prio = 0; tc_prio < dev->info->max_ipvs; tc_prio++) {
>  		int queue;
>  
>  		queue = tc_prio >> s;
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index 40c11b0d6b625..1bedd240cbbe4 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -58,6 +58,7 @@ struct ksz_chip_data {
>  	int port_cnt;
>  	u8 port_nirqs;
>  	u8 num_tx_queues;
> +	u8 max_ipvs; /* max number of Internal Priority Values */
>  	bool tc_cbs_supported;
>  	bool tc_ets_supported;
>  	const struct ksz_dev_ops *ops;
> @@ -722,7 +723,6 @@ static inline int is_lan937x(struct ksz_device *dev)
>  #define KSZ9477_PORT_MRI_TC_MAP__4	0x0808
>  
>  #define KSZ9477_PORT_TC_MAP_S		4
> -#define KSZ9477_MAX_TC_PRIO		7
>  
>  /* CBS related registers */
>  #define REG_PORT_MTI_QUEUE_INDEX__4	0x0900
> -- 
> 2.39.2
>