Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <ba0063d31a926b1775fe98e56b15976b4d8726cd.camel@redhat.com>
Subject: Re: [PATCH v2] net: ethernet: ti: am65-cpsw: add mqprio qdisc
 offload in channel mode
From:   Paolo Abeni <pabeni@redhat.com>
To:     Roger Quadros <rogerq@kernel.org>, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, vladimir.oltean@nxp.com
Cc:     horms@kernel.org, s-vadapalli@ti.com, srk@ti.com, vigneshr@ti.com,
        p-varis@ti.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, rogerq@kernel.rog
Date:   Tue, 19 Sep 2023 13:32:11 +0200
In-Reply-To: <20230918075358.5878-1-rogerq@kernel.org>
References: <20230918075358.5878-1-rogerq@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2023-09-18 at 10:53 +0300, Roger Quadros wrote:
> @@ -937,3 +918,296 @@ void am65_cpsw_qos_tx_p0_rate_init(struct am65_cpsw=
_common *common)
>  		       host->port_base + AM65_CPSW_PN_REG_PRI_CIR(tx_ch));

[...]

> +static int am65_cpsw_mqprio_verify_shaper(struct am65_cpsw_port *port,
> +					  struct tc_mqprio_qopt_offload *mqprio)
> +{
> +	u64 min_rate_total =3D 0, max_rate_total =3D 0;
> +	u32 min_rate_msk =3D 0, max_rate_msk =3D 0;
> +	bool has_min_rate, has_max_rate;
> +	int num_tc, i;
> +	struct am65_cpsw_mqprio *p_mqprio =3D &port->qos.mqprio;
> +	struct netlink_ext_ack *extack =3D mqprio->extack;

Please, respect the reverse x-mas tree order.

> +
> +	if (!(mqprio->flags & TC_MQPRIO_F_SHAPER))
> +		return 0;
> +
> +	if (mqprio->shaper !=3D TC_MQPRIO_SHAPER_BW_RATE)
> +		return 0;
> +
> +	has_min_rate =3D !!(mqprio->flags & TC_MQPRIO_F_MIN_RATE);
> +	has_max_rate =3D !!(mqprio->flags & TC_MQPRIO_F_MAX_RATE);
> +
> +	if (!has_min_rate && has_max_rate) {
> +		NL_SET_ERR_MSG_MOD(extack, "min_rate is required with max_rate");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!has_min_rate)
> +		return 0;
> +
> +	num_tc =3D mqprio->qopt.num_tc;
> +
> +	for (i =3D num_tc - 1; i >=3D 0; i--) {
> +		u32 ch_msk;
> +
> +		if (mqprio->min_rate[i])
> +			min_rate_msk |=3D BIT(i);
> +		min_rate_total +=3D  mqprio->min_rate[i];
> +
> +		if (has_max_rate) {
> +			if (mqprio->max_rate[i])
> +				max_rate_msk |=3D BIT(i);
> +			max_rate_total +=3D  mqprio->max_rate[i];
> +
> +			if (!mqprio->min_rate[i] && mqprio->max_rate[i]) {
> +				NL_SET_ERR_MSG_FMT_MOD(extack,
> +						       "TX tc%d rate max>0 but min=3D0\n",
> +						       i);
> +				return -EINVAL;
> +			}
> +
> +			if (mqprio->max_rate[i] &&
> +			    mqprio->max_rate[i] < mqprio->min_rate[i]) {
> +				NL_SET_ERR_MSG_FMT_MOD(extack,
> +						       "TX tc%d rate min(%llu)>max(%llu)\n",
> +						       i, mqprio->min_rate[i],
> +						       mqprio->max_rate[i]);
> +				return -EINVAL;
> +			}
> +		}
> +
> +		ch_msk =3D GENMASK(num_tc - 1, i);
> +		if ((min_rate_msk & BIT(i)) && (min_rate_msk ^ ch_msk)) {
> +			NL_SET_ERR_MSG_FMT_MOD(extack,
> +					       "TX min rate limiting has to be enabled sequentially hi->lo =
tx_rate_msk%x\n",
> +					       min_rate_msk);
> +			return -EINVAL;
> +		}
> +
> +		if ((max_rate_msk & BIT(i)) && (max_rate_msk ^ ch_msk)) {
> +			NL_SET_ERR_MSG_FMT_MOD(extack,
> +					       "TX max rate limiting has to be enabled sequentially hi->lo =
tx_rate_msk%x\n",
> +					       max_rate_msk);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	min_rate_total *=3D 8;
> +	min_rate_total /=3D 1000 * 1000;
> +	max_rate_total *=3D 8;
> +	max_rate_total /=3D 1000 * 1000;

For consistency with other code doing the same algebra, you could use a
single statement for both '*' and '/'. You could also add an helper for
that conversion, as there are multiple use-case already.

> +
> +	if (port->qos.link_speed !=3D SPEED_UNKNOWN) {
> +		if (min_rate_total > port->qos.link_speed) {
> +			NL_SET_ERR_MSG_FMT_MOD(extack, "TX rate min %llu exceeds link speed %=
d\n",
> +					       min_rate_total, port->qos.link_speed);
> +			return -EINVAL;
> +		}
> +
> +		if (max_rate_total > port->qos.link_speed) {
> +			NL_SET_ERR_MSG_FMT_MOD(extack, "TX rate max %llu exceeds link speed %=
d\n",
> +					       max_rate_total, port->qos.link_speed);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	p_mqprio->shaper_en =3D 1;
> +	p_mqprio->max_rate_total =3D max_t(u64, min_rate_total, max_rate_total)=
;
> +
> +	return 0;
> +}
> +
> +static void am65_cpsw_reset_tc_mqprio(struct net_device *ndev)
> +{
> +	struct am65_cpsw_port *port =3D am65_ndev_to_port(ndev);
> +	struct am65_cpsw_mqprio *p_mqprio =3D &port->qos.mqprio;
> +	struct am65_cpsw_common *common =3D port->common;
> +
> +	p_mqprio->shaper_en =3D 0;
> +	p_mqprio->max_rate_total =3D 0;
> +
> +	am65_cpsw_tx_pn_shaper_reset(port);
> +	netdev_reset_tc(ndev);
> +	netif_set_real_num_tx_queues(ndev, common->tx_ch_num);
> +
> +	/* Reset all Queue priorities to 0 */
> +	writel(0,
> +	       port->port_base + AM65_CPSW_PN_REG_TX_PRI_MAP);

No need to wrap the above statement on multiple lines.

> +}
> +
> +static int am65_cpsw_setup_mqprio(struct net_device *ndev, void *type_da=
ta)
> +{
> +	struct am65_cpsw_port *port =3D am65_ndev_to_port(ndev);
> +	struct am65_cpsw_mqprio *p_mqprio =3D &port->qos.mqprio;
> +	struct tc_mqprio_qopt_offload *mqprio =3D type_data;
> +	struct am65_cpsw_common *common =3D port->common;
> +	struct tc_mqprio_qopt *qopt =3D &mqprio->qopt;
> +	int tc, offset, count, ret, prio;
> +	u8 num_tc =3D qopt->num_tc;
> +	u32 tx_prio_map =3D 0;
> +	int i;
> +
> +	memcpy(&p_mqprio->mqprio_hw, mqprio, sizeof(*mqprio));
> +
> +	if (!num_tc) {
> +		am65_cpsw_reset_tc_mqprio(ndev);
> +		return 0;
> +	}
> +
> +	ret =3D pm_runtime_get_sync(common->dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(common->dev);
> +		return ret;
> +	}
> +
> +	ret =3D am65_cpsw_mqprio_verify_shaper(port, mqprio);
> +	if (ret)
> +		goto exit_put;
> +
> +	netdev_set_num_tc(ndev, num_tc);
> +
> +	/* Multiple Linux priorities can map to a Traffic Class
> +	 * A Traffic Class can have multiple contiguous Queues,
> +	 * Queues get mapped to Channels (thread_id),
> +	 *	if not VLAN tagged, thread_id is used as packet_priority
> +	 *	if VLAN tagged. VLAN priority is used as packet_priorit
> +	 * packet_priority gets mapped to header_priority in p0_rx_pri_map,
> +	 * header_priority gets mapped to switch_priority in pn_tx_pri_map.
> +	 * As p0_rx_pri_map is left at defaults (0x76543210), we can
> +	 * assume that Queue_n gets mapped to header_priority_n. We can then
> +	 * set the switch priority in pn_tx_pri_map.
> +	 */
> +
> +	for (tc =3D 0; tc < num_tc; tc++) {
> +	 /* For simplicity we assign the same priority (TCn) to all queues
> +	  * of a Traffic Class.
> +	  */

Please align the comment with the relevant code.

[...]

> +diff --git a/drivers/net/ethernet/ti/am65-cpsw-qos.h b/drivers/net/ether=
net/ti/am65-cpsw-qos.h
> index 0cc2a3b3d7f9..5431fbf8b6e0 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-qos.h
> +++ b/drivers/net/ethernet/ti/am65-cpsw-qos.h
> @@ -9,6 +9,7 @@
>  #include <net/pkt_sched.h>
> =20
>  struct am65_cpsw_common;
> +struct am65_cpsw_port;
> =20
>  struct am65_cpsw_est {
>  	int buf;
> @@ -16,6 +17,12 @@ struct am65_cpsw_est {
>  	struct tc_taprio_qopt_offload taprio;
>  };
> =20
> +struct am65_cpsw_mqprio {
> +	struct tc_mqprio_qopt_offload mqprio_hw;
> +	u64 max_rate_total;
> +	unsigned shaper_en:1;

Perhaps 'bool' instead?


Cheers,

Paolo