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

[Andrew Lunn]

> /* 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.

> 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.

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

> };
> 
> struct ethtool_loopback_cfg {
>     struct ethtool_loopback_layer_cfg *entries;
>     u32 num_layers;

What is num_layers used for?

> };
> 
> struct ethtool_ops {
>     /* ... */
> 
>     /* Query which layers/lane-combos are physically possible */
>     int (*get_loopback_caps)(struct net_device *dev,
>                              struct ethtool_loopback_cfg *caps);
> 
>     /* Get current active status for all layers */
>     int (*get_loopback_state)(struct net_device *dev,
>                               struct ethtool_loopback_cfg *state);
> 
>     /* Set one or more layer/lane configurations atomically */
>     int (*set_loopback)(struct net_device *dev,
>                         const struct ethtool_loopback_cfg *cfg,
>                         struct netlink_ext_ack *extack);
> };
> 
> As for layers; EXT vs PMD? EXT could be a loopback plug, whereas PMD
> would be CMIS, or whatever the driver detects.
> 
> Userland would be something like:
> 
> # 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?

>   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?

> # # 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.

    Andrew