| 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
| ||
|
Date: Fri, 6 Oct 2023 15:32:40 +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 3/4] net: ethernet: ti: am65-cpsw-qos: Add
Frame Preemption MAC Merge support
On 05/10/2023 12:29, Vladimir Oltean wrote:
> On Wed, Sep 27, 2023 at 10:27:40AM +0300, Roger Quadros wrote:
>> Add driver support for viewing / changing the MAC Merge sublayer
>> parameters and seeing the verification state machine's current state
>> via ethtool.
>>
>> As hardware does not support interrupt notification for verification
>> events we resort to polling on link up. On link up we try a couple of
>> times for verification success and if unsuccessful then give up.
>>
>> The Frame Preemption feature is described in the Technical Reference
>> Manual [1] in section:
>> 12.3.1.4.6.7 Intersperced Express Traffic (IET – P802.3br/D2.0)
>>
>> Due to Silicon Errata i2208 [2] we set limit min IET fragment size to 124.
>>
>> [1] AM62x TRM - https://www.ti.com/lit/ug/spruiv7a/spruiv7a.pdf
>> [2] AM62x Silicon Errata - https://www.ti.com/lit/er/sprz487c/sprz487c.pdf
>>
>> Signed-off-by: Roger Quadros <rogerq@...nel.org>
>> ---
>> drivers/net/ethernet/ti/am65-cpsw-ethtool.c | 150 ++++++++++++
>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 2 +
>> drivers/net/ethernet/ti/am65-cpsw-nuss.h | 5 +
>> drivers/net/ethernet/ti/am65-cpsw-qos.c | 240 ++++++++++++++++----
>> drivers/net/ethernet/ti/am65-cpsw-qos.h | 104 +++++++++
>> 5 files changed, 454 insertions(+), 47 deletions(-)
>>
>> Changelog:
>> v5:
>> - No change
>>
>> v4:
>> - Rebase and include in the same series as mqprio support.
>>
>> v3:
>> - Rebase on top of v6.6-rc1 and mqprio support [1]
>> - Support ethtool_ops :: get_mm_stats()
>> - drop unused variables cmn_ctrl and verify_cnt
>> - make am65_cpsw_iet_link_state_update() and
>> am65_cpsw_iet_change_preemptible_tcs() static
>>
>> [1] https://lore.kernel.org/all/20230918075358.5878-1-rogerq@kernel.org/
>>
>> v2:
>> - Use proper control bits for PMAC enable (AM65_CPSW_PN_CTL_IET_PORT_EN)
>> and TX enable (AM65_CPSW_PN_IET_MAC_PENABLE)
>> - Common IET Enable (AM65_CPSW_CTL_IET_EN) is set if any port has
>> AM65_CPSW_PN_CTL_IET_PORT_EN set.
>> - Fix workaround for erratum i2208. i.e. Limit rx_min_frag_size to 124
>> - Fix am65_cpsw_iet_get_verify_timeout_ms() to default to timeout for
>> 1G link if link is inactive.
>> - resize the RX FIFO based on pmac_enabled, not tx_enabled.
>>
>> Test Procedure:
>>
>> - 2 EVMs with AM65-CPSW network port connected to each other
>> - Run iet-setup-mqprio.sh on both
>>
>> #!/bin/sh
>> #iet-setup-mqprio.sh
>>
>> ifconfig eth0 down
>> ifconfig eth1 down
>> ethtool -L eth0 tx 4
>> ethtool --set-mm eth0 pmac-enabled on tx-enabled on verify-enabled on verify-time 10 tx-min-frag-size 124
>> ifconfig eth0 up
>> sleep 10
>>
>> tc qdisc add dev eth0 handle 100: root mqprio \
>> num_tc 4 \
>> map 0 1 2 3 \
>> queues 1@0 1@1 1@2 1@3 \
>> hw 1 \
>> mode dcb \
>> fp P P P E
>>
>> tc -g class show dev eth0
>> tc qdisc add dev eth0 clsact
>> tc filter add dev eth0 egress protocol ip prio 1 u32 match ip dport 5002 0xffff action skbedit priority 2
>> tc filter add dev eth0 egress protocol ip prio 1 u32 match ip dport 5003 0xffff action skbedit priority 3
>> ip addr add 192.168.3.102/24 dev eth0
>>
>> - check that MAC merge verification has succeeded
>>
>> ethtool --show-mm eth0
>>
>> MAC Merge layer state for eth0:
>> pMAC enabled: on
>> TX enabled: on
>> TX active: on
>> TX minimum fragment size: 124
>> RX minimum fragment size: 124
>> Verify enabled: on
>> Verify time: 10
>> Max verify time: 134
>> Verification status: SUCCEEDED
>>
>> - On receiver EVM run 2 iperf instances
>>
>> iperf3 -s -i30 -p5002&
>> iperf3 -s -i30 -p5003&
>>
>> - On sender EVM run 2 iperf instances
>>
>> iperf3 -c 192.168.3.102 -u -b200M -l1472 -u -t5 -i30 -p5002&
>> iperf3 -c 192.168.3.102 -u -b50M -l1472 -u -t5 -i30 -p5003&
>>
>> - Check IET stats on sender. Look for MACMergeFragCountTx: increments
>>
>> ethtool -I --show-mm eth0
>> MAC Merge layer state for eth0:
>> pMAC enabled: on
>> TX enabled: on
>> TX active: on
>> TX minimum fragment size: 124
>> RX minimum fragment size: 124
>> Verify enabled: on
>> Verify time: 10
>> Max verify time: 134
>> Verification status: SUCCEEDED
>> Statistics:
>> MACMergeFrameAssErrorCount: 0
>> MACMergeFrameSmdErrorCount: 0
>> MACMergeFrameAssOkCount: 0
>> MACMergeFragCountRx: 0
>> MACMergeFragCountTx: 57824
>> MACMergeHoldCount: 0
>>
>> - Check IET stats on receiver. Look for MACMergeFragCountRx: and
>> MACMergeFrameAssOkCount:
>>
>> ethtool -I --show-mm eth0
>> MAC Merge layer state for eth0:
>> pMAC enabled: on
>> TX enabled: on
>> TX active: on
>> TX minimum fragment size: 124
>> RX minimum fragment size: 124
>> Verify enabled: on
>> Verify time: 10
>> Max verify time: 134
>> Verification status: SUCCEEDED
>> Statistics:
>> MACMergeFrameAssErrorCount: 0
>> MACMergeFrameSmdErrorCount: 0
>> MACMergeFrameAssOkCount: 57018
>> MACMergeFragCountRx: 57824
>> MACMergeFragCountTx: 0
>> MACMergeHoldCount: 0
>
> Nice of you to post commands, but could you also please clearly state
> whether the implementation passes tools/testing/selftests/net/forwarding/ethtool_mm.sh?
Honestly, I didn't spend much time with it. I will try to run
those tests and get back.
>
>> + val &= ~AM65_CPSW_PN_IET_MAC_MAC_ADDFRAGSIZE_MASK;
>> + val |= AM65_CPSW_PN_IET_MAC_SET_ADDFRAGSIZE(add_frag_size);
>> + writel(val, port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
>> +
>> + /* verify_timeout_count can only be set at valid link */
>> + port->qos.iet.verify_time_ms = cfg->verify_time;
>> +
>> + /* enable/disable pre-emption based on link status */
>
> For the benefit of grep, I would appreciate if it was spelled
> "preemption" everywhere.
OK.
>
>> + am65_cpsw_iet_commit_preemptible_tcs(port);
>> +
>> + mutex_unlock(&priv->mm_lock);
>> +
>> + return 0;
>> +}
>> +
>> static int am65_cpsw_port_est_enabled(struct am65_cpsw_port *port)
>> {
>> return port->qos.est_oper || port->qos.est_admin;
>> @@ -602,6 +743,8 @@ static int am65_cpsw_setup_taprio(struct net_device *ndev, void *type_data)
>> if (port->qos.link_speed == SPEED_UNKNOWN)
>> return -ENOLINK;
>>
>> + am65_cpsw_iet_change_preemptible_tcs(port, taprio->mqprio.preemptible_tcs);
>> +
>
> Hmm, why just look at the preemptible traffic classes and not at
> taprio's entire mqprio configuration? This bypasses the mapping between
> Linux traffic classes and switch priorities that you've established in
> am65_cpsw_setup_mqprio().
>
> With the addition of the "mqprio" structure in tc_taprio_qopt_offload,
> my intention was to facilitate calling am65_cpsw_setup_mqprio() from
> am65_cpsw_setup_taprio().
OK. I will take a look at this. Thanks!
>
>> return am65_cpsw_set_taprio(ndev, type_data);
>> }
--
cheers,
-roger
Powered by blists - more mailing lists