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
| ||
|
Message-ID: <c735b39c-7685-fc4c-ab0f-527f7d8262fb@gmail.com> 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 >>
Powered by blists - more mailing lists