netdev - Re: [PATCH net-next v7 4/6] net: marvell: prestera: Add ethtool interface support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VdowNYwS76QEq6YqyvD84y61jTLACLLWoBDF3K27g45tA@mail.gmail.com>
Date:   Fri, 4 Sep 2020 22:38:15 +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: [PATCH net-next v7 4/6] net: marvell: prestera: Add ethtool
 interface support

On Fri, Sep 4, 2020 at 7:52 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

...

> +#include <linux/kernel.h>
> +#include <linux/netdevice.h>
> +#include <linux/ethtool.h>

Sorted?

...

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

Traditional pattern?

if (err)
 return err;
...
return 0;

...

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

Why not err : SPEED_... : speed; ?

...

> +static int
> +prestera_ethtool_get_link_ksettings(struct net_device *dev,

One line?

> +                                   struct ethtool_link_ksettings *ecmd)

...

> +       err = prestera_hw_port_link_mode_get(port, &curr_mode);
> +       if (err || curr_mode >= PRESTERA_LINK_MODE_MAX)
> +               return -EINVAL;

Why shadowing error code?

...

> +/*
> + * Copyright (c) 2019-2020 Marvell International Ltd. All rights reserved.
> + *
> + */

One line.

...

> +#include <linux/netdevice.h>

Is this being used?

> +#include <linux/ethtool.h>
> +
> +extern const struct ethtool_ops prestera_ethtool_ops;

...

> +enum {
> +       PRESTERA_FC_NONE,
> +       PRESTERA_FC_SYMMETRIC,
> +       PRESTERA_FC_ASYMMETRIC,
> +       PRESTERA_FC_SYMM_ASYMM

Comma?

> +};

...

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

Comma?

> +       };

...

> +       err = prestera_cmd_ret(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_GET,
> +                              &req.cmd, sizeof(req), &resp.ret, sizeof(resp));
> +       if (err)
> +               return err;
> +
> +       *link_mode_bitmap = resp.param.cap.link_mode;
> +
> +       return err;

return 0;

I think I have talked that any of the given comments applies to *all*
similar code pieces!

This file is full of repetitions of the above.

...

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

default.
Also I have a deja vu about such.

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

...

> +enum {
> +       PRESTERA_PORT_DUPLEX_HALF,
> +       PRESTERA_PORT_DUPLEX_FULL

Comma ?

> +};

-- 
With Best Regards,
Andy Shevchenko