| 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
| ||
|
Date: Fri, 13 Jan 2017 17:47:15 -0800
From: Florian Fainelli <f.fainelli@...il.com>
To: Alexander Loktionov <Alexander.Loktionov@...antia.com>,
netdev@...r.kernel.org, David VomLehn <vomlehn@...as.net>
Cc: "David S . Miller" <davem@...emloft.net>,
Simon Edelhaus <Simon.Edelhaus@...antia.com>,
Dmitrii Tarakanov <Dmitrii.Tarakanov@...antia.com>,
Pavel Belous <Pavel.Belous@...antia.com>,
Dmitry Bezrukov <Dmitry.Bezrukov@...antia.com>
Subject: Re: [PATCH v5 11/13] net: ethernet: aquantia: Ethtool support
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@...as.net>
>
> Add the driver interfaces required for support by the ethtool utility.
>
> Signed-off-by: Alexander Loktionov <Alexander.Loktionov@...antia.com>
> Signed-off-by: Dmitrii Tarakanov <Dmitrii.Tarakanov@...antia.com>
> Signed-off-by: Pavel Belous <Pavel.Belous@...antia.com>
> Signed-off-by: Dmitry Bezrukov <Dmitry.Bezrukov@...antia.com>
> Signed-off-by: David M. VomLehn <vomlehn@...as.net>
> ---
> drivers/net/ethernet/aquantia/aq_ethtool.c | 250 +++++++++++++++++++++++++++++
> drivers/net/ethernet/aquantia/aq_ethtool.h | 19 +++
> 2 files changed, 269 insertions(+)
> create mode 100644 drivers/net/ethernet/aquantia/aq_ethtool.c
> create mode 100644 drivers/net/ethernet/aquantia/aq_ethtool.h
>
> diff --git a/drivers/net/ethernet/aquantia/aq_ethtool.c b/drivers/net/ethernet/aquantia/aq_ethtool.c
> new file mode 100644
> index 0000000..f11bdb1
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/aq_ethtool.c
> @@ -0,0 +1,250 @@
> +/*
> + * aQuantia Corporation Network Driver
> + * Copyright (C) 2014-2017 aQuantia Corporation. All rights reserved
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + */
> +
> +/* File aq_ethtool.c: Definition of ethertool related functions. */
> +
> +#include "aq_ethtool.h"
> +#include "aq_nic.h"
> +
> +static void aq_ethtool_get_regs(struct net_device *ndev,
> + struct ethtool_regs *regs, void *p)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
netdev_priv() returns a void * which requires no casting, please fix
this through the entire 13 patches.
> + u32 regs_count = aq_nic_get_regs_count(aq_nic);
> +
> + memset(p, 0, regs_count * sizeof(u32));
> + aq_nic_get_regs(aq_nic, regs, p);
> +}
> +
> +static int aq_ethtool_get_regs_len(struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> + u32 regs_count = aq_nic_get_regs_count(aq_nic);
> +
> + return regs_count * sizeof(u32);
> +}
> +
> +static u32 aq_ethtool_get_link(struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> +
> + return aq_nic_get_link_speed(aq_nic) ? 1U : 0U;
Can you either use PHYLIB to interface to your PHY (which does all the
nice state machine management) or at the very least ethtool_op_get_link()?
> +}
> +
> +static int aq_ethtool_get_settings(struct net_device *ndev,
> + struct ethtool_cmd *cmd)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> +
> + cmd->port = PORT_TP;
> + cmd->transceiver = XCVR_EXTERNAL;
> +
> + ethtool_cmd_speed_set(cmd, netif_carrier_ok(ndev) ?
> + aq_nic_get_link_speed(aq_nic) : 0U);
> +
> + cmd->duplex = DUPLEX_FULL;
> + aq_nic_get_link_settings(aq_nic, cmd);
> + return 0;
Consider switching to the new ksettings API and filling in a bit more
information like cmd->autoneg?
> +}
> +
> +static int aq_ethtool_set_settings(struct net_device *ndev,
> + struct ethtool_cmd *cmd)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> +
> + return aq_nic_set_link_settings(aq_nic, cmd);
> +}
> +
> +static const char aq_ethtool_stat_names[][ETH_GSTRING_LEN] = {
> + "InPackets",
> + "InUCast",
> + "InMCast",
> + "InBCast",
> + "InErrors",
> + "OutPackets",
> + "OutUCast",
> + "OutMCast",
> + "OutBCast",
> + "InUCastOctects",
> + "OutUCastOctects",
> + "InMCastOctects",
> + "OutMCastOctects",
> + "InBCastOctects",
> + "OutBCastOctects",
> + "InOctects",
> + "OutOctects",
> + "InPacketsDma",
> + "OutPacketsDma",
> + "InOctetsDma",
> + "OutOctetsDma",
> + "InDroppedDma",
> + "Queue[0] InPackets",
> + "Queue[0] OutPackets",
> + "Queue[0] InJumboPackets",
> + "Queue[0] InLroPackets",
> + "Queue[0] InErrors",
> +#if 1 < AQ_CFG_VECS_DEF
Yoda notations are usually frowned upon. Instead of making this decision
here, can you push that down to the actual function reading the statistics?
> +static void aq_ethtool_get_strings(struct net_device *ndev,
> + u32 stringset, u8 *data)
> +{
You need to check that stringset == ETH_SS_STATS here since that's the
only thing you support. ethtool usually does not do it if the ioctl()
returning the data does not exist, but other bogus applications might.
> + memcpy(data, *aq_ethtool_stat_names, sizeof(aq_ethtool_stat_names));
> +}
> +
> +static int aq_ethtool_get_sset_count(struct net_device *ndev, int stringset)
> +{
Same here.
> + return ARRAY_SIZE(aq_ethtool_stat_names);
> +#ifndef AQ_ETHTOOL_H
> +#define AQ_ETHTOOL_H
> +
> +#include "aq_common.h"
> +
> +extern const struct ethtool_ops aq_ethtool_ops;
The extern does not appear necessary here. Alternatively you may define
all the ethtool function pointers in this header file, but leave the
struct ethtool_ops in the main driver file which registers them at the
time of the netdev creation, up to you.
> +
> +#endif /* AQ_ETHTOOL_H */
>
--
Florian
Powered by blists - more mailing lists