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
