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

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Feb 24, 2022 at 04:57:36PM -0800, Dimitris Michailidis wrote:
> On Thu, Feb 24, 2022 at 12:30 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > +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);
> > > +     }
> >
> > > +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?

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

     Andrew