In D58258#1367599, @adrian wrote:

In D58258#1367549, @dsl wrote:

@adrian well, I'm not sure that the contributor actually understands the code. It seems AI/ML generated to me and isn't aligned with the idea of mine about maclink. I'm against the changes.

What do you mean by "maclink" ? They said they used AI assistance in developing it, but it seems mostly simple enough:

• sfp is an i2c device on a bus;
• the i2c bus doesn't HAVE to be hooked up to the MAC in any way; it in theory could be hanging off of some other i2c controller in the system;
• there's information about where said bus is linked to in FDT;
• some simple shenanigans are required to be able to talk to it and fetch configuration parameters.

What doesn't quite jive with your assumptions of stuff?

I used to think it's still there!

@adrian well, I'm not sure that the contributor actually understands the code. It seems AI/ML generated to me and isn't aligned with the idea of mine about maclink. I'm against the changes.

Could you rebase to the latest main as well as it doesn't apply cleanly with https://reviews.freebsd.org/D59463 pushed?

Pushed to main during the devsummit. It works just fine as far as I can tell and according to the stress tests conducted during the hackathon 202609. Please, complain if it breaks something.

Several years ago I added a draft version of the "sff,sfp" driver to https://cgit.freebsd.org/src/tree/sys/dev/sff. I wonder whether all of those sfp_i2c operations can be moved there and be hidden behind an API. dpaa2_ni would only need to obtain "sff,sfp" device (via xref, I think) and call it.

My Honeycomb doesn't have SFP cages exposes to the operating system, i.e. I cannot try it there directly. Besides, I don't like an idea to obtain info about the I2C config for SFPs using tunables on the ACPI systems. It is already described in the ACPI tables (partially, though):

dsl@castle:~ $ devinfo -v | grep I2C0
    vf_i2c_acpi0 <Vybrid Family Inter-Integrated Circuit (I2C)> pnpinfo _HID=NXP0001 _UID=0 _CID=none at handle=\_SB_.I2C0
        unknown pnpinfo _HID=NXP0002 _UID=0 _CID=none at addr=0x77 handle=\_SB_.I2C0.MUX0
    unknown pnpinfo _HID=PRP0001 _UID=0 _CID=none at handle=\_SB_.I2C0.MUX0.CH01.FAN1
    unknown pnpinfo _HID=PRP0001 _UID=1 _CID=none at handle=\_SB_.I2C0.MUX0.CH03.THE1

Only a fan controller (TI's amc6821) and a temperature sensor (NXP's sa56004) sitting on channels 1 and 3 respectively are described. However, according to the Honeycomb schematic, mux channels 4,5,6,7 are for the SFP cages. You can check whether a board you're booting up is LX2160A and set correct default values.

I can read a DAC on my Ten64 now. Looks good!

dpni8: flags=8802<BROADCAST,SIMPLEX,MULTICAST> metric 0 mtu 1500
        options=60002b<RXCSUM,TXCSUM,VLAN_MTU,JUMBO_MTU,RXCSUM_IPV6,TXCSUM_IPV6>
        ether 00:0a:fa:24:2b:28
ifconfig: dpni8: no media types?
        nd6 options=829<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL,STABLEADDR>
        drivername: dpni8
        plugged: SFP/SFP+/SFP28 Unknown (Copper pigtail)
        vendor: FS PN: SFPP-PC03 SN: F2210XXXXX-2 DATE: 2022-08-12
root@cannon-tower:~ # ifconfig -v dpni9
dpni9: flags=8802<BROADCAST,SIMPLEX,MULTICAST> metric 0 mtu 1500
        options=60002b<RXCSUM,TXCSUM,VLAN_MTU,JUMBO_MTU,RXCSUM_IPV6,TXCSUM_IPV6>
        ether 00:0a:fa:24:2b:27
ifconfig: dpni9: no media types?
        nd6 options=829<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL,STABLEADDR>
        drivername: dpni9
        plugged: SFP/SFP+/SFP28 Unknown (Copper pigtail)
        vendor: FS PN: SFPP-PC03 SN: F2210XXXXX-1 DATE: 2022-08-12

Okay, thanks for opening a review. Please, provide a full context for the patch, i.e. -U999999.

Committed in https://cgit.freebsd.org/ports/commit/?id=9cd256080a694234243e2c35afefdeeaa22ffd07, no idea why no update

@bz I'd rebase this review and commit if you don't have time now.

In D56383#1293150, @bz wrote:

You could write this "This is effectively a refined version of the code written by bz@ in D55320." as part of the final commit message as:

Obtained from: bz (initial version, D55320)
or
Based on: initial version from bz (D55320)

I'll try to give it a pass again tonight once I am done with merges from vendor trees but I think we are good to go if @tuexen has not further comments.

As soon as we were interested in the FD[ERR} field, I added another counter to track other ingress frames with errors. It can be detailed in future.

In D56383#1292511, @bz wrote:

In D56383#1292509, @dsl wrote:

Uhh, I've double-checked and dpaa2_ni_setup_if_caps is actually called on my side and reports correct changes in the csum validation/generation, but it doesn't affect the ingress frames at all:

dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff

These FAS values make no sense if you look at the bits. Is this on the 2160 or on 1088?

In D56383#1292507, @bz wrote:

Unrelated but is that

1637         (void)DPAA2_CMD_NI_CLOSE(dev, child, &cmd);

at the end of dpaa2_ni_setup_if_caps correct passing &cmd instead of DPAA2_CMD_TK(&cmd, ni_token) ? I think that should be fixed.
Reported by: bz

Uhh, I've double-checked and dpaa2_ni_setup_if_caps is actually called on my side and reports correct changes in the csum validation/generation, but it doesn't affect the ingress frames at all:

In D56383#1292028, @tuexen wrote:

When enabling rx csum offload, I do the the expected flags in incoming TCP segments. So this works now. But when disabling rx csum offload, I still see the flags:

tuexen@ten64:~ % ifconfig dpni1
dpni1: flags=1008843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
	options=48002a<TXCSUM,VLAN_MTU,JUMBO_MTU,LINKSTATE,TXCSUM_IPV6>
	ether 00:0a:fa:24:2f:c6
	inet 212.201.121.124 netmask 0xffffffe0 broadcast 212.201.121.127
	inet6 fe80::20a:faff:fe24:2fc6%dpni1 prefixlen 64 scopeid 0x2
	inet6 2a02:c6a0:4015:10::124 prefixlen 64
	media: Ethernet autoselect (1000baseT <full-duplex,master>)
	status: active
	nd6 options=821<PERFORMNUD,AUTO_LINKLOCAL,STABLEADDR>

Output of:

#!/usr/sbin/dtrace -s

#pragma D option quiet

/* 
 * Trace tcp_input to capture incoming segments. 
 * arg0 is typically a pointer to the mbuf (struct mbuf *).
 */
fbt::tcp_input:entry
{
    /* Cast arg0 to access the mbuf structure and its packet header */
    this->m = *(struct mbuf **)arg0;
    this->flags = this->m->m_pkthdr.csum_flags;

    printf("%-20Y  Flags: 0x%08x\n", walltimestamp, this->flags);
}
fbt::tcp6_input:entry
{
    /* Cast arg0 to access the mbuf structure and its packet header */
    this->m = *(struct mbuf **)arg0;
    this->flags = this->m->m_pkthdr.csum_flags;

    printf("%-20Y  Flags: 0x%08x\n", walltimestamp, this->flags);
}

is:

2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000

This is for TCP/IPv6. I would have expected 0. Is my expectation wrong?

In D56383#1291193, @tuexen wrote:

Looks good to me, will test it tomorrow. Right now my test system is not reachable and I am only back to my lab tomorrow.

Fixed

I've fixed the comments and added another review (D56383) which is currently in progress.

Usually, there's drm-drivers.adoc report which summarizes activities regarding the drm-kmod drivers. It might be a good idea to include mine if the drm-drivers.adoc will appear.

@tuexen Thanks for a quick response!

Okay, I see. if_inc_counter() is implemented with counter_u64_add which is atomic on arm64, i.e. no synchronization is required. I don't think there's noticeable overhead either, i.e. lgtm.

Can we hide those counters behind a macro (e.g. DPAA2_DEBUG)?

e.g. sysctl dev.dpaa2_ni.0 is not enough?

@bz you meant IFCAP_HWCSUM_IPV6, didn't you? Hmm, I haven't tested it yet. Probably a good idea before committing changes.

In D53436#1224250, @bz wrote:

Beware that this (L4), if I read it correctly, will also enable RX/TX checksum validation/generation for upper layer protocols for IPv6 packets even if that is not not advertised or enabled.

This change seems trivial to me. I'd simply commit it if there are no objections.

I'd like to know objections, if any.

Updated to 0.1.4