| 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: <6075c30f-5e2b-43b9-b57b-f4b9a918df22@kernel.org>
Date: Fri, 6 Oct 2023 15:29:49 +0300
From: Roger Quadros <rogerq@...nel.org>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, horms@...nel.org, s-vadapalli@...com,
srk@...com, vigneshr@...com, p-varis@...com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 net-next 2/4] net: ethernet: ti: am65-cpsw: add mqprio
qdisc offload in channel mode
On 05/10/2023 12:01, Vladimir Oltean wrote:
> On Wed, Sep 27, 2023 at 10:27:39AM +0300, Roger Quadros wrote:
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-qos.c b/drivers/net/ethernet/ti/am65-cpsw-qos.c
>> index f91137d8e73b..991221d8f148 100644
>> --- a/drivers/net/ethernet/ti/am65-cpsw-qos.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-qos.c
>> @@ -16,10 +16,19 @@
>> #include "cpsw_ale.h"
>>
>> #define AM65_CPSW_REG_CTL 0x004
>> +#define AM65_CPSW_P0_REG_TX_PRI_MAP 0x018
>> +#define AM65_CPSW_P0_REG_RX_PRI_MAP 0x020
>> +#define AM65_CPSW_P0_REG_FIFO_STATUS 0x050
>> +#define AM65_CPSW_P0_REG_PRI_CIR(pri) (0x140 + 4 * (pri))
>> +#define AM65_CPSW_P0_REG_PRI_EIR(pri) (0x160 + 4 * (pri))
>> +
>> #define AM65_CPSW_PN_REG_CTL 0x004
>> +#define AM65_CPSW_PN_REG_TX_PRI_MAP 0x018
>> +#define AM65_CPSW_PN_REG_RX_PRI_MAP 0x020
>> #define AM65_CPSW_PN_REG_FIFO_STATUS 0x050
>> #define AM65_CPSW_PN_REG_EST_CTL 0x060
>> #define AM65_CPSW_PN_REG_PRI_CIR(pri) (0x140 + 4 * (pri))
>> +#define AM65_CPSW_PN_REG_PRI_EIR(pri) (0x160 + 4 * (pri))
>>
>> /* AM65_CPSW_REG_CTL register fields */
>> #define AM65_CPSW_CTL_EST_EN BIT(18)
>> @@ -50,12 +59,18 @@
>> #define AM65_CPSW_FETCH_ALLOW_MSK GENMASK(7, 0)
>> #define AM65_CPSW_FETCH_ALLOW_MAX AM65_CPSW_FETCH_ALLOW_MSK
>>
>> +#define TO_MBPS(x) ((x) * 8 / 1000000)
>> +
>> enum timer_act {
>> TACT_PROG, /* need program timer */
>> TACT_NEED_STOP, /* need stop first */
>> TACT_SKIP_PROG, /* just buffer can be updated */
>> };
>>
>> +/* number of priority queues per port FIFO */
>> +#define AM65_CPSW_PN_FIFO_PRIO_NUM 8
>> +#define AM65_CPSW_PN_TX_PRI_MAP_DEFAULT 0x76543210
>
>> +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 = false;
>> + 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);
>
> If this never gets changed from the value set by am65_cpsw_nuss_ndo_slave_open(),
> then there is no reason to call netif_set_real_num_tx_queues() here.
>
>> +
>> + /* Reset all Queue priorities to 0 */
>> + writel(0, port->port_base + AM65_CPSW_PN_REG_TX_PRI_MAP);
>
> To 0 or to AM65_CPSW_PN_TX_PRI_MAP_DEFAULT (which is now unused)?
> Also, do I understand correctly that immediately after boot, TX_PRI_MAP
> will have a different value than after the deletion of the mqprio root qdisc
> (if AM65_CPSW_PN_TX_PRI_MAP_DEFAULT represents the hardware default value)?
> The behavior needs to be identical in the 2 cases, since both represent
> "no mqprio offload".
Good catch. I'll get rid of AM65_CPSW_PN_TX_PRI_MAP_DEFAULT and set this to
0 at init time.
>
>> +}
>> +
>> +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));
>> +
>> + ret = pm_runtime_get_sync(common->dev);
>> + if (ret < 0) {
>> + pm_runtime_put_noidle(common->dev);
>> + return ret;
>> + }
>> +
>> + if (!num_tc) {
>> + am65_cpsw_reset_tc_mqprio(ndev);
>> + goto exit_put;
>> + }
>> +
>> + ret = am65_cpsw_mqprio_verify_shaper(port, mqprio);
>> + if (ret)
>> + goto exit_put;
>
> At "exit_put" we have "return 0" and this discards the value of "ret".
Will fix.
>
>> +
>> + 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++) {
>> + prio = tc;
>> +
>> + /* For simplicity we assign the same priority (TCn) to
>> + * all queues of a Traffic Class.
>> + */
>> + for (i = qopt->offset[tc]; i < qopt->offset[tc] + qopt->count[tc]; i++)
>> + tx_prio_map |= prio << (4 * i);
>> +
>> + count = qopt->count[tc];
>> + offset = qopt->offset[tc];
>> + netdev_set_tc_queue(ndev, tc, count, offset);
>> + }
>
> I think this is okay.
>
>> +
>> + writel(tx_prio_map,
>> + port->port_base + AM65_CPSW_PN_REG_TX_PRI_MAP);
>
> Nit: This can be written on a single line.
>
OK.
>> +
>> + am65_cpsw_tx_pn_shaper_apply(port);
>> +
>> +exit_put:
>> + pm_runtime_put(common->dev);
>> + return 0;
>> +}
>> +
>> int am65_cpsw_qos_ndo_setup_tc(struct net_device *ndev, enum tc_setup_type type,
>> void *type_data)
>> {
--
cheers,
-roger
Powered by blists - more mailing lists