| Message ID | 20220826063816.948397-4-mattias.forsblad@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: mv88e6xxx: Add RMU support | expand |
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. 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. > > 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? > + } > + } 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. > + 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 >
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 >>
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); + } + } 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) + 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)
If RMU is supported, use that interface to collect rmon data. Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com> --- drivers/net/dsa/mv88e6xxx/chip.c | 41 ++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 7 deletions(-)