Re: [PATCH net-next v2 3/3] rmon: Use RMU if available

Mattias Forsblad
2022-08-31T05:51:29Z

On 2022-08-30 16:20, Vladimir Oltean wrote:
> Hi Forsblad,
>
> On Fri, Aug 26, 2022 at 08:38:16AM +0200, Mattias Forsblad wrote:
>> If RMU is supported, use that interface to
>> collect rmon data.
>
> A more adequate commit message would be:
>
> net: dsa: mv88e6xxx: use RMU to collect RMON stats if available
>
> But then, I don't think the splitting of patches is good. I think
> mv88e6xxx_inband_rcv(), the producer of rmu_raw_stats[], should be
> introduced along with its consumer. Otherwise I have to jump between one
> patch and another to be able to comment and see things.
>

I'll have that in mind for the next round. The next version will
look way different after Andrews suggestion.

> Also, it would be good if you could consider actually reporting the RMON
> stats through the standardized interface (ds->ops->get_rmon_stats) and
> ethtool -S lan0 --groups rmon, rather than just unstructured ethtool -S.
>

Cool, I didn't know it existed. I'll look into that.

>>
>> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
>> ---
>> drivers/net/dsa/mv88e6xxx/chip.c | 41 ++++++++++++++++++++++++++------
>> 1 file changed, 34 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 4c0510abd875..0d0241ace708 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -1226,16 +1226,30 @@ static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
>> u16 bank1_select, u16 histogram)
>> {
>> struct mv88e6xxx_hw_stat *stat;
>> + int offset = 0;
>> + u64 high;
>> int i, j;
>>
>> for (i = 0, j = 0; i < ARRAY_SIZE(mv88e6xxx_hw_stats); i++) {
>> stat = &mv88e6xxx_hw_stats[i];
>> if (stat->type & types) {
>> - mv88e6xxx_reg_lock(chip);
>> - data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
>> - bank1_select,
>> - histogram);
>> - mv88e6xxx_reg_unlock(chip);
>> + if (chip->rmu.ops && chip->rmu.ops->get_rmon &&
>> + !(stat->type & STATS_TYPE_PORT)) {
>> + if (stat->type & STATS_TYPE_BANK1)
>> + offset = 32;
>> +
>> + data[j] = chip->ports[port].rmu_raw_stats[stat->reg + offset];
>> + if (stat->size == 8) {
>> + high = chip->ports[port].rmu_raw_stats[stat->reg + offset
>> + + 1];
>> + data[j] += (high << 32);
>
> What exactly protects ethtool -S, a reader of rmu_raw_stats[], from
> reading an array that is concurrently overwritten by mv88e6xxx_inband_rcv?
>

So for the Marvell SOHO the RMU is purely a request/response protocol. The switchcore
will not send a frame unless requested, thus no barrier is needed. For other switchcores
which may have send frames spontaneous additional care may be needed.

>> + }
>> + } else {
>> + mv88e6xxx_reg_lock(chip);
>> + data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
>> + bank1_select, histogram);
>> + mv88e6xxx_reg_unlock(chip);
>> + }
>>
>> j++;
>> }
>> @@ -1304,8 +1318,8 @@ static void mv88e6xxx_get_stats(struct mv88e6xxx_chip *chip, int port,
>> mv88e6xxx_reg_unlock(chip);
>> }
>>
>> -static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
>> - uint64_t *data)
>> +static void mv88e6xxx_get_ethtool_stats_mdio(struct dsa_switch *ds, int port,
>> + uint64_t *data)
>> {
>> struct mv88e6xxx_chip *chip = ds->priv;
>> int ret;
>> @@ -1319,7 +1333,20 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
>> return;
>>
>> mv88e6xxx_get_stats(chip, port, data);
>> +}
>>
>> +static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
>> + uint64_t *data)
>> +{
>> + struct mv88e6xxx_chip *chip = ds->priv;
>> +
>> + /* If initialization of RMU isn't available
>> + * fall back to MDIO access.
>> + */
>> + if (chip->rmu.ops && chip->rmu.ops->get_rmon)
>
> I would create a helper
>
> static bool mv88e6xxx_rmu_available(struct mv88e6xxx_chip *chip)
>
> and use it here and everywhere, for clarity. Testing the presence of
> chip->rmu.ops is not wrong, but confusing.
>
> Also, testing chip->rmu.ops->get_rmon gains exactly nothing, since it is
> never NULL when chip->rmu.ops isn't NULL.
>

Agreed. The next version will draw inspiration from qca8k.
>> + chip->rmu.ops->get_rmon(chip, port, data);
>> + else
>> + mv88e6xxx_get_ethtool_stats_mdio(ds, port, data);
>> }
>>
>> static int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port)
>> --
>> 2.25.1
>>