| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75Ve6YtrAW60FfT8QYsb6B3ZQuS7dZdz7dD9zB9b1=cpfog@mail.gmail.com>
Date: Mon, 27 Jul 2020 16:17:04 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Vadym Kochan <vadym.kochan@...ision.eu>
Cc: "David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Jiri Pirko <jiri@...lanox.com>,
Ido Schimmel <idosch@...lanox.com>,
Andrew Lunn <andrew@...n.ch>,
Oleksandr Mazur <oleksandr.mazur@...ision.eu>,
Serhiy Boiko <serhiy.boiko@...ision.eu>,
Serhiy Pshyk <serhiy.pshyk@...ision.eu>,
Volodymyr Mytnyk <volodymyr.mytnyk@...ision.eu>,
Taras Chornyi <taras.chornyi@...ision.eu>,
Andrii Savka <andrii.savka@...ision.eu>,
netdev <netdev@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Mickey Rachamim <mickeyr@...vell.com>
Subject: Re: [net-next v4 4/6] net: marvell: prestera: Add ethtool interface support
On Mon, Jul 27, 2020 at 3:23 PM Vadym Kochan <vadym.kochan@...ision.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
Powered by blists - more mailing lists