| 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
| ||
|
Message-ID: <7c05ec2c-fe13-5324-f537-4f12697d1566@kernel.org>
Date: Wed, 20 Sep 2023 10:09:05 +0300
From: Roger Quadros <rogerq@...nel.org>
To: Paolo Abeni <pabeni@...hat.com>, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, vladimir.oltean@....com
Cc: horms@...nel.org, s-vadapalli@...com, srk@...com, vigneshr@...com,
p-varis@...com, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, rogerq@...nel.rog
Subject: Re: [PATCH v2] net: ethernet: ti: am65-cpsw: add mqprio qdisc offload
in channel mode
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
Powered by blists - more mailing lists