Re: [PATCH net-next v7 4/8] net/funeth: ethtool operations

On Thu, Feb 24, 2022 at 11:52 PM Andrew Lunn [off-list ref] wrote:

On Thu, Feb 24, 2022 at 04:57:36PM -0800, Dimitris Michailidis wrote:

quoted

On Thu, Feb 24, 2022 at 12:30 PM Andrew Lunn [off-list ref] wrote:

quoted

quoted

+static void fun_link_modes_to_ethtool(u64 modes,
+                                   unsigned long *ethtool_modes_map)
+{
+#define ADD_LINK_MODE(mode) \
+     __set_bit(ETHTOOL_LINK_MODE_ ## mode ## _BIT, ethtool_modes_map)
+
+     if (modes & FUN_PORT_CAP_AUTONEG)
+             ADD_LINK_MODE(Autoneg);
+     if (modes & FUN_PORT_CAP_1000_X)
+             ADD_LINK_MODE(1000baseX_Full);
+     if (modes & FUN_PORT_CAP_10G_R) {
+             ADD_LINK_MODE(10000baseCR_Full);
+             ADD_LINK_MODE(10000baseSR_Full);
+             ADD_LINK_MODE(10000baseLR_Full);
+             ADD_LINK_MODE(10000baseER_Full);
+     }

quoted

+static unsigned int fun_port_type(unsigned int xcvr)
+{
+     if (!xcvr)
+             return PORT_NONE;
+
+     switch (xcvr & 7) {
+     case FUN_XCVR_BASET:
+             return PORT_TP;

You support twisted pair, so should you also have the BaseT_FULL link
modes above?

I agree with that but FW currently doesn't report BASE-T speeds in its
port capabilities and the link modes are based on them. Looks simple to fix
but needs future FW.

Maybe you should drop PORT_TP until you do have the firmware fixed?

FW can report a port as BASET regardless of anything the driver
does, and it needs to be translated into something ethtool understands.
If the driver doesn't use PORT_TP what would it be then?

quoted

quoted

quoted

+static int fun_set_pauseparam(struct net_device *netdev,
+                           struct ethtool_pauseparam *pause)
+{
+     struct funeth_priv *fp = netdev_priv(netdev);
+     u64 new_advert;
+
+     if (fp->port_caps & FUN_PORT_CAP_VPORT)
+             return -EOPNOTSUPP;
+     /* Forcing PAUSE settings with AN enabled is unsupported. */
+     if (!pause->autoneg && (fp->advertising & FUN_PORT_CAP_AUTONEG))
+             return -EOPNOTSUPP;

This seems wrong. You don't advertise you cannot advertise. You simply
don't advertise. It could just be you have a bad variable name here?

advertising & FUN_PORT_CAP_AUTONEG means that AN is enabled, and
when this bit is off AN is disabled.

So, i was correct, the name of the variable is not so good. Maybe
fp->advertising need splitting into two, fp->cap_enabled for
capabilities of the firmware that are enabled, and fp->advertising for
what is actually been advertised to the link partner?

There are two fields currently: port_caps is the RO capabilities field, and
advertising are the settings we are asking FW to use. Some of them may be
used in AN and advertised to the partner, FW will sort that out. The variable
may have different name and there may be more than one but eventually all
the settings need to be passed to FW as one value. It made sense to maintain
them as one value all the way. Keep in mind these settings are not consumed
by a port and need not be sensible for a port. The consumer is FW.
And while there is a close relationship these settings do not fully match what
a link partner receives in case of AN.

     Andrew