On 03/28/2018 12:33 PM, Andrew Lunn wrote:
> On Wed, Mar 28, 2018 at 11:17:19AM -0700, Florian Fainelli wrote:
>> On 03/27/2018 02:59 PM, Andrew Lunn wrote:
>>> Count the numbers of various ATU and VTU violation statistics and
>>> return them as part of the ethtool -S statistics.
>>>
>>> Signed-off-by: Andrew Lunn <and...@lunn.ch>
>>> ---
>>> drivers/net/dsa/mv88e6xxx/chip.c | 50
>>> ++++++++++++++++++++++++++++-----
>>> drivers/net/dsa/mv88e6xxx/chip.h | 13 ++++++---
>>> drivers/net/dsa/mv88e6xxx/global1_atu.c | 12 +++++---
>>> drivers/net/dsa/mv88e6xxx/global1_vtu.c | 8 ++++--
>>> drivers/net/dsa/mv88e6xxx/serdes.c | 15 ++++++----
>>> drivers/net/dsa/mv88e6xxx/serdes.h | 8 +++---
>>> 6 files changed, 78 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
>>> b/drivers/net/dsa/mv88e6xxx/chip.c
>>> index 9a5d786b4885..186021f98c5d 100644
>>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>>> @@ -723,6 +723,24 @@ static int mv88e6320_stats_get_strings(struct
>>> mv88e6xxx_chip *chip,
>>> STATS_TYPE_BANK0 | STATS_TYPE_BANK1);
>>> }
>>>
>>> +static const uint8_t *mv88e6xxx_atu_vtu_stats_strings[] = {
>>
>> Why not const char *?
>
> The ethtool call passes i uint8_t *data to receive the copy into. I'm
> keeping it consistent.
Fair enough.
>
>>> +static void mv88e6xxx_atu_vtu_get_strings(uint8_t *data)
>>> +{
>>> + int i;
>>
>> unsigned int i?
>
> I could do, but it seems unlikely it will overflow 31 bits.
The size cannot be negative, so unsigned int would seem like a natural
choice.
>
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(mv88e6xxx_atu_vtu_stats_strings); i++)
>>> + strlcpy(data + i * ETH_GSTRING_LEN,
>>> + mv88e6xxx_atu_vtu_stats_strings[i],
>>> + ETH_GSTRING_LEN);
>>> +}
>>> +
>>> static void mv88e6xxx_get_strings(struct dsa_switch *ds, int port,
>>> uint8_t *data)
>>> {
>>> @@ -736,9 +754,12 @@ static void mv88e6xxx_get_strings(struct dsa_switch
>>> *ds, int port,
>>>
>>> if (chip->info->ops->serdes_get_strings) {
>>> data += count * ETH_GSTRING_LEN;
>>> - chip->info->ops->serdes_get_strings(chip, port, data);
>>> + count = chip->info->ops->serdes_get_strings(chip, port, data);
>>> }
>>>
>>> + data += count * ETH_GSTRING_LEN;
>>> + mv88e6xxx_atu_vtu_get_strings(data);
>>> +
>>> mutex_unlock(&chip->reg_lock);
>>> }
>>>
>>> @@ -783,10 +804,13 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch
>>> *ds, int port)
>>> if (chip->info->ops->serdes_get_sset_count)
>>> serdes_count = chip->info->ops->serdes_get_sset_count(chip,
>>> port);
>>> - if (serdes_count < 0)
>>> + if (serdes_count < 0) {
>>> count = serdes_count;
>>> - else
>>> - count += serdes_count;
>>> + goto out;
>>> + }
>>> + count += serdes_count;
>>> + count += ARRAY_SIZE(mv88e6xxx_atu_vtu_stats_strings);
>>> +
>>> out:
>>> mutex_unlock(&chip->reg_lock);
>>>
>>> @@ -841,6 +865,16 @@ static int mv88e6390_stats_get_stats(struct
>>> mv88e6xxx_chip *chip, int port,
>>> 0);
>>> }
>>>
>>> +static void mv88e6xxx_atu_vtu_get_stats(struct mv88e6xxx_chip *chip, int
>>> port,
>>> + uint64_t *data)
>>> +{
>>> + *data++ = chip->ports[port].atu_member_violation;
>>> + *data++ = chip->ports[port].atu_miss_violation;
>>> + *data++ = chip->ports[port].atu_full_violation;
>>> + *data++ = chip->ports[port].vtu_member_violation;
>>> + *data++ = chip->ports[port].vtu_miss_violation;
>>
>> This looks fine, but I suppose you could just have an u64 pointer which
>> is initialized to point to atu_member_violation, and then just do
>> pointer arithmetics to iterate, this would avoid possibly missing that
>> function in case new ATU/VTU violations are handled in the future?
>
> KISS. This works and is obvious.
Fair enough.
--
Florian
