netdev - Re: [PATCH net-next] net: phy: aquantia: add SGMII statistics

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] [day] [month] [year] [list]

Message-ID: <ee5f02f0-c7f9-38f1-a069-b6ab6a550d50@gmail.com>
Date:   Sun, 31 Mar 2019 16:59:36 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] net: phy: aquantia: add SGMII statistics

On 31.03.2019 16:45, Andrew Lunn wrote:
>> +static u64 aqr107_get_stat(struct phy_device *phydev, int index)
>> +{
>> +	const struct aqr107_hw_stat *stat = aqr107_hw_stats + index;
>> +	int len_l = min(stat->size, 16);
>> +	int len_h = stat->size - len_l;
>> +	u64 ret;
>> +	int val;
>> +
>> +	val = phy_read_mmd(phydev, MDIO_MMD_C22EXT, stat->reg);
>> +	if (val < 0) {
>> +		phydev_err(phydev, "Reading HW Statistics failed\n");
>> +		return 0;
>> +	}
>> +
>> +	ret = val & GENMASK(len_l - 1, 0);
>> +	if (len_h) {
>> +		val = phy_read_mmd(phydev, MDIO_MMD_C22EXT, stat->reg + 1);
>> +		if (val < 0) {
>> +			phydev_err(phydev, "Reading HW Statistics failed\n");
>> +			return 0;
> 
> Hi Heiner
> 
> When things go wrong, it seems to be reasonably normal to return
> U64_MAX, not zero. It is such a large value that is raises questions,
> where as 0 might be considered a correct value, not an error.
> 
>> +static void aqr107_get_stats(struct phy_device *phydev,
>> +			     struct ethtool_stats *stats, u64 *data)
>> +{
>> +	u64 *pstats = phydev->priv;
> 
> This seems like a trap waiting for somebody to fall into.
> 
> It would be more future proof to define a struct which just contains
> an array. 
> 
Both good points. I'll cook a v2.

>    Andrew
> 
Heiner