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
