Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200727122242.32337-1-vadym.kochan@plvision.eu> <20200727122242.32337-5-vadym.kochan@plvision.eu>
In-Reply-To: <20200727122242.32337-5-vadym.kochan@plvision.eu>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 27 Jul 2020 16:17:04 +0300
Message-ID: <CAHp75Ve6YtrAW60FfT8QYsb6B3ZQuS7dZdz7dD9zB9b1=cpfog@mail.gmail.com>
Subject: Re: [net-next v4 4/6] net: marvell: prestera: Add ethtool interface support
To:     Vadym Kochan <vadym.kochan@plvision.eu>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Jiri Pirko <jiri@mellanox.com>,
        Ido Schimmel <idosch@mellanox.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Oleksandr Mazur <oleksandr.mazur@plvision.eu>,
        Serhiy Boiko <serhiy.boiko@plvision.eu>,
        Serhiy Pshyk <serhiy.pshyk@plvision.eu>,
        Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu>,
        Taras Chornyi <taras.chornyi@plvision.eu>,
        Andrii Savka <andrii.savka@plvision.eu>,
        netdev <netdev@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Mickey Rachamim <mickeyr@marvell.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jul 27, 2020 at 3:23 PM Vadym Kochan <vadym.kochan@plvision.eu> wrote:
>
> The ethtool API provides support for the configuration of the following
> features: speed and duplex, auto-negotiation, MDI-x, forward error
> correction, port media type. The API also provides information about the
> port status, hardware and software statistic. The following limitation
> exists:
>
>     - port media type should be configured before speed setting
>     - ethtool -m option is not supported
>     - ethtool -p option is not supported
>     - ethtool -r option is supported for RJ45 port only
>     - the following combination of parameters is not supported:
>
>           ethtool -s sw1pX port XX autoneg on
>
>     - forward error correction feature is supported only on SFP ports, 10G
>       speed
>
>     - auto-negotiation and MDI-x features are not supported on
>       Copper-to-Fiber SFP module

...

> +       if (new_mode < PRESTERA_LINK_MODE_MAX)
> +               err = prestera_hw_port_link_mode_set(port, new_mode);
> +       else
> +               err = -EINVAL;
> +
> +       if (!err) {
> +               port->caps.type = type;
> +               port->autoneg = false;
> +       }
> +
> +       return err;

Again

  if (new_mode >= ...)
    return -EINVAL;

  err = ...
  if (err)
    return err;

  ...
  return 0;

...

> +       ecmd->base.speed = !err ? speed : SPEED_UNKNOWN;

Why not positive conditional?

...

> +       if (!prestera_hw_port_duplex_get(port, &duplex)) {

Ditto.

...

> +static int
> +prestera_ethtool_set_link_ksettings(struct net_device *dev,
> +                                   const struct ethtool_link_ksettings *ecmd)
> +{
> +       struct prestera_port *port = netdev_priv(dev);

> +       u64 adver_modes = 0;
> +       u8 adver_fec = 0;

Redundant assignments?

> +       int err;
> +
> +       err = prestera_port_type_set(ecmd, port);
> +       if (err)
> +               return err;
> +
> +       if (port->caps.transceiver == PRESTERA_PORT_TCVR_COPPER) {
> +               err = prestera_port_mdix_set(ecmd, port);
> +               if (err)
> +                       return err;
> +       }
> +
> +       prestera_modes_from_eth(ecmd->link_modes.advertising, &adver_modes,
> +                               &adver_fec, port->caps.type);

> +       return 0;
> +}

...

> +       struct prestera_msg_port_attr_req req = {
> +               .attr = PRESTERA_CMD_PORT_ATTR_REMOTE_CAPABILITY,
> +               .port = port->hw_id,
> +               .dev = port->dev_id

+ comma

> +       };

...

> +       struct prestera_msg_port_attr_req req = {
> +               .attr = PRESTERA_CMD_PORT_ATTR_REMOTE_FC,
> +               .port = port->hw_id,
> +               .dev = port->dev_id

Ditto.

> +       };

...

> +       switch (resp.param.fc) {
> +       case PRESTERA_FC_SYMMETRIC:
> +               *pause = true;
> +               *asym_pause = false;
> +               break;
> +       case PRESTERA_FC_ASYMMETRIC:
> +               *pause = false;
> +               *asym_pause = true;
> +               break;
> +       case PRESTERA_FC_SYMM_ASYMM:
> +               *pause = true;
> +               *asym_pause = true;
> +               break;
> +       default:
> +               *pause = false;
> +               *asym_pause = false;
> +       }
> +
> +       return err;

return 0;

...

> +int prestera_hw_port_type_get(const struct prestera_port *port, u8 *type)
> +{

> +       struct prestera_msg_port_attr_req req = {
> +               .attr = PRESTERA_CMD_PORT_ATTR_TYPE,
> +               .port = port->hw_id,
> +               .dev = port->dev_id
> +       };

> +       return err;

Same comments as above.
And seems more code needs the same.

> +}

...

> +static u8 prestera_hw_mdix_to_eth(u8 mode)
> +{
> +       switch (mode) {
> +       case PRESTERA_PORT_TP_MDI:
> +               return ETH_TP_MDI;
> +       case PRESTERA_PORT_TP_MDIX:
> +               return ETH_TP_MDI_X;
> +       case PRESTERA_PORT_TP_AUTO:
> +               return ETH_TP_MDI_AUTO;
> +       }
> +
> +       return ETH_TP_MDI_INVALID;

Use the default case.

> +}
> +
> +static u8 prestera_hw_mdix_from_eth(u8 mode)
> +{
> +       switch (mode) {
> +       case ETH_TP_MDI:
> +               return PRESTERA_PORT_TP_MDI;
> +       case ETH_TP_MDI_X:
> +               return PRESTERA_PORT_TP_MDIX;
> +       case ETH_TP_MDI_AUTO:
> +               return PRESTERA_PORT_TP_AUTO;
> +       }
> +
> +       return PRESTERA_PORT_TP_NA;

Ditto.

> +}

-- 
With Best Regards,
Andy Shevchenko