Page MenuHomeFreeBSD

cxgbe: use new cloners KPI.
Needs ReviewPublic

Authored by melifaro on Apr 28 2023, 9:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 30, 3:51 PM
Unknown Object (File)
Tue, Apr 30, 3:50 PM
Unknown Object (File)
Tue, Apr 30, 3:50 PM
Unknown Object (File)
Tue, Apr 30, 3:50 PM
Unknown Object (File)
Tue, Apr 30, 3:50 PM
Unknown Object (File)
Tue, Apr 30, 8:32 AM
Unknown Object (File)
Sun, Apr 28, 5:39 AM
Unknown Object (File)
Sun, Apr 14, 12:58 AM
Subscribers

Details

Reviewers
np

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 51269
Build 48160: arc lint + arc unit

Event Timeline

np requested changes to this revision.EditedApr 28 2023, 5:08 PM

I tried this change but the cloned interface was not created.

  1. ifconfig t6nex0 create ifconfig: SIOCIFCREATE2 (t6nex0): Invalid argument

DTrace shows that the t4_clone_match routine was called and returned 1 but the t4_clone_create routine was not called after that.

  1. dtrace -n 'fbt::t4_clone_*:return {trace(arg1)}' dtrace: description 'fbt::t4_clone_*:return ' matched 3 probes

CPU ID FUNCTION:NAME

1 127467            t4_clone_match:return                 1
This revision now requires changes to proceed.Apr 28 2023, 5:08 PM

The if_name2unit call in if_clone_createif_nl -> ifc_handle_unit seems to be the problem. It is returning EINVAL because it doesn't like the 0 in "t6nex0".

I don't get why 0 is not a valid unit in if_name2unit? Also, this driver does _not_ really want the name2unit code to run at all -- it passed in the AUTO flag and wants an automatically allocated unit and NOT one that is derived from the name passed to the cloner. This was allowed by the old KPI.

Use manual unit allocation.

In D39865#907659, @np wrote:

The if_name2unit call in if_clone_createif_nl -> ifc_handle_unit seems to be the problem. It is returning EINVAL because it doesn't like the 0 in "t6nex0".

I don't get why 0 is not a valid unit in if_name2unit? Also, this driver does _not_ really want the name2unit code to run at all -- it passed in the AUTO flag and wants an automatically allocated unit and NOT one that is derived from the name passed to the cloner. This was allowed by the old KPI.

Ack. Sorry, I don't have the HW so wasn't able to do it. I removed the IFC_F_AUTOUNIT flag from the creation option, so the driver will allocate units manually, as before.

Almost there. Now the new ifnet is created successfully but there is an extra 0 in its name. The ifnet should have been t6nex0 and not t6nex00.

# ifconfig t6nex0 create                                                             
t6nex00 
# ifconfig t6nex00
t6nex00: flags=840<RUNNING,SIMPLEX> metric 0 mtu 1500
       ether 00:00:00:00:00:00
       groups: tXnex
       media: Ethernet none <full-duplex>
       status: active
       nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>
sys/dev/cxgbe/t4_tracer.c
186

Does this change work for you?

Ping :-)
I'd love to get rid of if_clone_advanced() before 14 and cxgbe is the only remaining user :-)

Ping :-)
I'd love to get rid of if_clone_advanced() before 14 and cxgbe is the only remaining user :-)

It looks very dodgy for the driver to write to the name passed to it by the kernel. Is this the final fix you're suggesting?

In D39865#909198, @np wrote:

Almost there. Now the new ifnet is created successfully but there is an extra 0 in its name. The ifnet should have been t6nex0 and not t6nex00.

Are you sure it's t6nex0 without this change? Could you share what's the if_dunit value of this interface?
I'm struggling to understand the code path here :-(

# ifconfig t6nex0 create                                                             
t6nex00 
# ifconfig t6nex00
t6nex00: flags=840<RUNNING,SIMPLEX> metric 0 mtu 1500
       ether 00:00:00:00:00:00
       groups: tXnex
       media: Ethernet none <full-duplex>
       status: active
       nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>