Re: [PATCH v2] net: ethernet: ti: am65-cpsw: add mqprio qdisc offload in channel mode

[Roger Quadros <rogerq@kernel.org>]

Hi Paolo,

On 19/09/2023 14:32, Paolo Abeni wrote:
> 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 = 0, max_rate_total = 0;
>> +	u32 min_rate_msk = 0, max_rate_msk = 0;
>> +	bool has_min_rate, has_max_rate;
>> +	int num_tc, i;
>> +	struct am65_cpsw_mqprio *p_mqprio = &port->qos.mqprio;
>> +	struct netlink_ext_ack *extack = mqprio->extack;
> 
> Please, respect the reverse x-mas tree order.
> 
>> +
>> +	if (!(mqprio->flags & TC_MQPRIO_F_SHAPER))
>> +		return 0;
>> +
>> +	if (mqprio->shaper != TC_MQPRIO_SHAPER_BW_RATE)
>> +		return 0;
>> +
>> +	has_min_rate = !!(mqprio->flags & TC_MQPRIO_F_MIN_RATE);
>> +	has_max_rate = !!(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 = mqprio->qopt.num_tc;
>> +
>> +	for (i = num_tc - 1; i >= 0; i--) {
>> +		u32 ch_msk;
>> +
>> +		if (mqprio->min_rate[i])
>> +			min_rate_msk |= BIT(i);
>> +		min_rate_total +=  mqprio->min_rate[i];
>> +
>> +		if (has_max_rate) {
>> +			if (mqprio->max_rate[i])
>> +				max_rate_msk |= BIT(i);
>> +			max_rate_total +=  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=0\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 = 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 *= 8;
>> +	min_rate_total /= 1000 * 1000;
>> +	max_rate_total *= 8;
>> +	max_rate_total /= 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 != 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 = 1;
>> +	p_mqprio->max_rate_total = 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 = am65_ndev_to_port(ndev);
>> +	struct am65_cpsw_mqprio *p_mqprio = &port->qos.mqprio;
>> +	struct am65_cpsw_common *common = port->common;
>> +
>> +	p_mqprio->shaper_en = 0;
>> +	p_mqprio->max_rate_total = 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_data)
>> +{
>> +	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
>> +	struct am65_cpsw_mqprio *p_mqprio = &port->qos.mqprio;
>> +	struct tc_mqprio_qopt_offload *mqprio = type_data;
>> +	struct am65_cpsw_common *common = port->common;
>> +	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
>> +	int tc, offset, count, ret, prio;
>> +	u8 num_tc = qopt->num_tc;
>> +	u32 tx_prio_map = 0;
>> +	int i;
>> +
>> +	memcpy(&p_mqprio->mqprio_hw, mqprio, sizeof(*mqprio));
>> +
>> +	if (!num_tc) {
>> +		am65_cpsw_reset_tc_mqprio(ndev);
>> +		return 0;
>> +	}
>> +
>> +	ret = pm_runtime_get_sync(common->dev);
>> +	if (ret < 0) {
>> +		pm_runtime_put_noidle(common->dev);
>> +		return ret;
>> +	}
>> +
>> +	ret = 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 = 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/ethernet/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>
>>  
>>  struct am65_cpsw_common;
>> +struct am65_cpsw_port;
>>  
>>  struct am65_cpsw_est {
>>  	int buf;
>> @@ -16,6 +17,12 @@ struct am65_cpsw_est {
>>  	struct tc_taprio_qopt_offload taprio;
>>  };
>>  
>> +struct am65_cpsw_mqprio {
>> +	struct tc_mqprio_qopt_offload mqprio_hw;
>> +	u64 max_rate_total;
>> +	unsigned shaper_en:1;
> 
> Perhaps 'bool' instead?

Thanks for the review. I'll fix them in next revision.

-- 
cheers,
-roger