Re: [PATCH net-next 2/2] bgmac: Add support for ethtool statistics

[Florian Fainelli <f.fainelli@gmail.com>]

On 06/03/2016 10:57 AM, Ben Hutchings wrote:
> On Fri, 2016-06-03 at 10:07 -0700, Florian Fainelli wrote:
> [...]
>> +static void bgmac_get_strings(struct net_device *dev, u32 stringset,
>> +			      u8 *data)
>> +{
>> +	int i;
>> +
>> +	if (stringset != ETH_SS_STATS)
>> +		return;
>> +
>> +	for (i = 0; i < BGMAC_STATS_LEN; i++)
>> +		memcpy(data + i * ETH_GSTRING_LEN,
>> +		       bgmac_get_strings_stats[i].name,
>> +		       ETH_GSTRING_LEN);
> 
> These strings are null-terminated, not padded to ETH_GSTRING_LEN.  So
> here you should be using strlcpy() instead of memcpy().
> 
>> +}
>> +
>> +static void bgmac_get_ethtool_stats(struct net_device *dev,
>> +				    struct ethtool_stats *ss, uint64_t *data)
>> +{
>> +	struct bgmac *bgmac = netdev_priv(dev);
>> +	const struct bgmac_stat *s;
>> +	unsigned int i;
>> +	u64 val;
>> +
>> +	if (!netif_running(dev))
>> +		return;
>> +
>> +	for (i = 0; i < BGMAC_STATS_LEN; i++) {
>> +		s = &bgmac_get_strings_stats[i];
>> +		val = 0;
>> +		if (s->size == 8)
>> +			val = (u64)bgmac_read(bgmac, s->offset + 4);
> 
> Isn't this missing a << 32?

It is, guess I should have made sure there was 4GB+ worth of traffic to
make sure this seemed reasonable.
> 
> Does reading the high 32 bits latch the value of the low 32 bits?  If
> not, you need to read the high bits again after the low bits and retry
> if they changed.

Yes these registers are latched.

> 
>> +		val |= bgmac_read(bgmac, s->offset);
>> +		data[i] = (u64)val;
> 
> Redundant cast.

Indeed, thanks.
-- 
Florian