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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 31 Aug 2022 07:51:02 +0200
From:   Mattias Forsblad <mattias.forsblad@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v2 3/3] rmon: Use RMU if available

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@...il.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
>>