Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Björn Töpel]

Hey!

On Mon, 23 Feb 2026 at 15:30, Andrew Lunn <andrew@xxxxxxx> wrote:
>
> > /* Loopback Direction (XXX is local/remote easier to understand?) */
> > enum ethtool_loopback_dir {
> >     ETHTOOL_LB_DIR_NEAR_END = 0,    /* Host -> Loop -> Host  */
> >     ETHTOOL_LB_DIR_FAR_END,        /* Line -> Loop -> Line  */
> > };
>
> I like host->loop->host, it is much clearer than NEAR_END or
> FAR_END. Where there is space, would use this description, even if it
> is a bit verbose.

Ok!

> > struct ethtool_loopback_layer_cfg {
> >     enum ethtool_loopback_layer layer;    /* ETHTOOL_LB_L_MAC, etc. */
> >     enum ethtool_loopback_dir direction;  /* NEAR or FAR */
> >     u32 lane_mask;                        /* Specific lanes */
> >     u32  flags;                           /* patterns? reserved... */
> >     bool enabled;
> >     char name[16];
>
> What would name be used for. I don't see it in your example. The nice
> thing about netlink messages is that they are extendable, unlike
> system calls. If there is no current use for a field, don't add it. It
> can be added later when actually needed. So i would drop flags and
> name.

Yeah, good call (I dropped flags on my local hack), name *and*
lane_mask. More on that below.

> Does CMIS, when used with a splitter cable, allow you to set loopback
> on lanes? What is your use case for lane_mask?

At least the spec exposed that it could be supported. Thinking more
about it, I think it can be added later if someone cares about it.
IOW, flags, name, and lane_mask shouldn't be part of the initial
patches.

> > };
> >
> > struct ethtool_loopback_cfg {
> >     struct ethtool_loopback_layer_cfg *entries;
> >     u32 num_layers;
>
> What is num_layers used for?

I'll change the _cfg to have a proper array instead. The idea is that
_cfg is a set of layers (MAC, PMA, etc.). The num_layers is the number
of entries. I'll make sure this is more obvious.

...

> > # ethtool --show-loopback eth0
> > Loopback Status for eth0:
> >   Layer: SW  | State: OFF
> >   Layer: MAC   | State: OFF
> >   Layer: PMA   | State: ON  | Lanes: 0x1 (Lane 0) | Direction: Near-End (Local)
> >   Layer: PMD   | State: ON  | Lanes: 0xF (All)    | Direction: Far-End (Remote)
>
> ETHTOOL_LINK_MODE_800000baseKR8_Full_BIT has 8 lanes, so 0xff would be
> All in this case. Lanes adds quite a bit of complexity. Do we have a
> real use case for it?

I don't. As outlined above -- let's leave it out for now. If somebody
needs it in the future, we can have the discussion then.

> >   Layer: EXT   | State: ON  | Detected: External Loopback Plug
> >
> > # ethtool --set-loopback <dev> [layer[:lanes][:direction]] ... [off]
> >
> > # # Simple MAC loopback:
> > # ethtool --set-loopback eth0 mac (Defaults: lanes=all, dir=near)
> > # # Specific SerDes (PMA) lane:
> > # ethtool --set-loopback eth0 pma:lane0
> > # # Complex multi-layer (PMA Near + PMD Far):
> > # ethtool --set-loopback eth0 pma:0x1:near pmd:all:far
>
> Is this something we actually want? Again it adds complexity,
> especially in the error handling, when pma:0x1:near works, but
> pmd:all:far fails, and you need to unwind the pma:0x1:near. Is there a
> use case for atomically setting two loopbacks, rather than having the
> user make two different calls?

Good point! Simpler is better.

> > # # Disable all loopbacks:
> > ethtool --set-loopback eth0 off
> >
> > Thoughts? Is this somewhat close to what you had in mind, Andrew?
>
> I'm happy with the basic shape of this. I just needs the details
> nailing down.

Ok! I'll roll something we can discuss more. Thanks for all input!


Björn