Re: [PATCH v6 net-next 6/7] net: ethernet: ti: am65-cpsw-qos: Add Frame Preemption MAC Merge support

[Vladimir Oltean]

On Tue, Nov 21, 2023 at 01:02:50PM +0200, Roger Quadros wrote:
> Yes I'm using openlldp master.
> 
> So I just dumped the "ethtool --show-mm" right before the "lldptool -i $h1 -t -n -V addEthCaps"
> and this is what I see
> 
> # MAC Merge layer state for eth0:
> # pMAC enabled: on
> # TX enabled: off
> # TX active: off
> # TX minimum fragment size: 252
> # RX minimum fragment size: 124
> # Verify enabled: off
> # Verify time: 10
> # Max verify time: 134
> # Verification status: DISABLED
> # 
> # MAC Merge layer state for eth1:
> # pMAC enabled: on
> # TX enabled: off
> # TX active: off
> # TX minimum fragment size: 124
> # RX minimum fragment size: 124
> # Verify enabled: off
> # Verify time: 10
> # Max verify time: 134
> # Verification status: DISABLED
> # 
> # Additional Ethernet Capabilities TLV
> #       Preemption capability supported
> #       Preemption capability not enabled
> #       Preemption capability not active
> #       Additional fragment size: 3 (252 octets)
> # Additional Ethernet Capabilities TLV
> #       Preemption capability supported
> #       Preemption capability not enabled
> #       Preemption capability not active
> #       Additional fragment size: 1 (124 octets)
> # Warning: Stopping lldpad.service, but it can still be activated by:
> #   lldpad.socket
> # TEST: LLDP                                                          [FAIL]
> 
> 
> If I add the following lines at the beginning of lldp() routine,
> then it works.
> 
> lldp()
> {
>         RET=0
> 
> +        ethtool --set-mm $h1 tx-enabled on verify-enabled on
> +        ethtool --set-mm $h2 tx-enabled on verify-enabled on
> ...
> }
> 
> Is lldp supposed to turn on tx-enabled and verify-enabled for us
> or it is test scritps responsibility?

lldpad should absolutely do that.
https://github.com/intel/openlldp/blob/master/lldp_8023.c#L701

Try to see what goes on and if there isn't, in fact, an error during the
netlink communication with the kernel.

Edit /usr/local/lib/systemd/system/lldpad.service:
ExecStart=/usr/local/sbin/lldpad -t -V 7
                                   ~~~~~
                                   increases log level
Then run:

$ systemctl daemon-reload
$ journalctl -u lldpad.service -f &
$ ./ethtool_mm.sh eno0 swp0

During the test you should see:

lldpad[4764]: eno0: Link partner preemption capability supported
lldpad[4764]: eno0: Link partner preemption capability not enabled
lldpad[4764]: eno0: Link partner preemption capability not active
lldpad[4764]: eno0: Link partner minimum fragment size: 252 octets
lldpad[4764]: eno0: initiating MM verification with a retry interval of 127 ms...
lldpad[4764]: rxProcessFrame: allocated TLV 0 was not stored! 0xaaaafd7cfbe0
lldpad[4764]: swp0: Link partner preemption capability supported
lldpad[4764]: swp0: Link partner preemption capability not enabled
lldpad[4764]: swp0: Link partner preemption capability not active
lldpad[4764]: swp0: Link partner minimum fragment size: 60 octets
lldpad[4764]: swp0: initiating MM verification with a retry interval of 128 ms...
lldpad[4764]: rxProcessFrame: allocated TLV 0 was not stored! 0xaaaafd7cfd30

> 
> The test fails later at "addFragSize 0", but that is because we don't
> support RX fragment size 60 due to errata.
> If I skip that test then all the rest of the tests pass.

Hmm, yeah, the test is dumb. lldpad has this logic, so if we request 0
it should still advertise 1.

	if (config_add_frag_size < add_frag_size) {
		LLDPAD_WARN("%s: Configured addFragSize (%d) smaller than the minimum value requested by kernel (%d). Using the latter\n",
			    bd->ifname, config_add_frag_size, add_frag_size);
		config_add_frag_size = add_frag_size;
	}

I guess that logic does engage, but the selftest doesn't expect that it
will, because it expects that lldpad will report back exactly the
requested value - and it will report the true value instead.

Luckily I know what to do here, see the patch below.