| Message ID | 20231205160418.3770042-4-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: mv88e6xxx: Add "eth-mac" and "rmon" counter group support | expand |
On Tue, Dec 05, 2023 at 05:04:15PM +0100, Tobias Waldekranz wrote: > mv88e6xxx_get_stats, which collects stats from various sources, > expects all callees to return the number of stats read. If an error > occurs, 0 should be returned. > > Prevent future mishaps of this kind by updating the return type to > reflect this contract. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- > diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c > index 3b4b42651fa3..01ea53940786 100644 > --- a/drivers/net/dsa/mv88e6xxx/serdes.c > +++ b/drivers/net/dsa/mv88e6xxx/serdes.c > @@ -187,7 +187,7 @@ int mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port, > > err = mv88e6352_g2_scratch_port_has_serdes(chip, port); > if (err <= 0) > - return err; > + return 0; Ok, you're saying we don't care enough about handling the catastrophic event where an MDIO access error takes place in mv88e6xxx_g2_scratch_read() to submit this to "stable". I guess the impact in such a case is that the error (interpreted as negative count) makes us go back by -EIO (5) entries or whatever into the "data" array provided to user space, overwriting some previous stats and making everything after the failed counter minus the error code be reported in the wrong place relative to its string. I don't think that the error codes are high enough to overcome the ~60 port stats and cause memory accesses behind the "data" array. Anyway, for the patch content: Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
On tis, dec 05, 2023 at 19:50, Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > On Tue, Dec 05, 2023 at 05:04:15PM +0100, Tobias Waldekranz wrote: >> mv88e6xxx_get_stats, which collects stats from various sources, >> expects all callees to return the number of stats read. If an error >> occurs, 0 should be returned. >> >> Prevent future mishaps of this kind by updating the return type to >> reflect this contract. >> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> --- >> diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c >> index 3b4b42651fa3..01ea53940786 100644 >> --- a/drivers/net/dsa/mv88e6xxx/serdes.c >> +++ b/drivers/net/dsa/mv88e6xxx/serdes.c >> @@ -187,7 +187,7 @@ int mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port, >> >> err = mv88e6352_g2_scratch_port_has_serdes(chip, port); >> if (err <= 0) >> - return err; >> + return 0; > > Ok, you're saying we don't care enough about handling the catastrophic > event where an MDIO access error takes place in mv88e6xxx_g2_scratch_read() > to submit this to "stable". It just felt like one of those theoretical bugs that, if you were to hit it, you most likely have way bigger issues than not getting at your SERDES counters; and since, as you say... > I guess the impact in such a case is that the error (interpreted as negative > count) makes us go back by -EIO (5) entries or whatever into the "data" > array provided to user space, overwriting some previous stats and making > everything after the failed counter minus the error code be reported in > the wrong place relative to its string. I don't think that the error > codes are high enough to overcome the ~60 port stats and cause memory > accesses behind the "data" array. ...the potential for data corruption seems low. But I could send a v3 and split this into one change that only fixes the return value (which could go into -net), and another one that changes the type. Do you think it's worth it?
On Tue, Dec 05, 2023 at 10:13:12PM +0100, Tobias Waldekranz wrote: > > Ok, you're saying we don't care enough about handling the catastrophic > > event where an MDIO access error takes place in mv88e6xxx_g2_scratch_read() > > to submit this to "stable". > > It just felt like one of those theoretical bugs that, if you were to hit > it, you most likely have way bigger issues than not getting at your > SERDES counters; and since, as you say... > > > I guess the impact in such a case is that the error (interpreted as negative > > count) makes us go back by -EIO (5) entries or whatever into the "data" > > array provided to user space, overwriting some previous stats and making > > everything after the failed counter minus the error code be reported in > > the wrong place relative to its string. I don't think that the error > > codes are high enough to overcome the ~60 port stats and cause memory > > accesses behind the "data" array. > > ...the potential for data corruption seems low. But I could send a v3 > and split this into one change that only fixes the return value (which > could go into -net), and another one that changes the type. Do you think > it's worth it? Reading Documentation/process/stable-kernel-rules.rst, I think that consistent error checking for register access on a non-hotpluggable bus is the type of bug fix that is exceedingly unlikely to have any measurable impact on end users, so it might not even qualify for "net". To me, this is good enough. Let's spend our time doing meaningful things, while also keeping the material for "net.git" meaningful.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index c3c53ef543e5..85eb293381a7 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -613,8 +613,8 @@ struct mv88e6xxx_ops { int (*serdes_get_sset_count)(struct mv88e6xxx_chip *chip, int port); int (*serdes_get_strings)(struct mv88e6xxx_chip *chip, int port, uint8_t *data); - int (*serdes_get_stats)(struct mv88e6xxx_chip *chip, int port, - uint64_t *data); + size_t (*serdes_get_stats)(struct mv88e6xxx_chip *chip, int port, + uint64_t *data); /* SERDES registers for ethtool */ int (*serdes_get_regs_len)(struct mv88e6xxx_chip *chip, int port); diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c index 3b4b42651fa3..01ea53940786 100644 --- a/drivers/net/dsa/mv88e6xxx/serdes.c +++ b/drivers/net/dsa/mv88e6xxx/serdes.c @@ -177,8 +177,8 @@ static uint64_t mv88e6352_serdes_get_stat(struct mv88e6xxx_chip *chip, return val; } -int mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port, - uint64_t *data) +size_t mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port, + uint64_t *data) { struct mv88e6xxx_port *mv88e6xxx_port = &chip->ports[port]; struct mv88e6352_serdes_hw_stat *stat; @@ -187,7 +187,7 @@ int mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port, err = mv88e6352_g2_scratch_port_has_serdes(chip, port); if (err <= 0) - return err; + return 0; BUILD_BUG_ON(ARRAY_SIZE(mv88e6352_serdes_hw_stats) > ARRAY_SIZE(mv88e6xxx_port->serdes_stats)); @@ -429,8 +429,8 @@ static uint64_t mv88e6390_serdes_get_stat(struct mv88e6xxx_chip *chip, int lane, return reg[0] | ((u64)reg[1] << 16) | ((u64)reg[2] << 32); } -int mv88e6390_serdes_get_stats(struct mv88e6xxx_chip *chip, int port, - uint64_t *data) +size_t mv88e6390_serdes_get_stats(struct mv88e6xxx_chip *chip, int port, + uint64_t *data) { struct mv88e6390_serdes_hw_stat *stat; int lane; diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h index aac95cab46e3..ff5c3ab31e15 100644 --- a/drivers/net/dsa/mv88e6xxx/serdes.h +++ b/drivers/net/dsa/mv88e6xxx/serdes.h @@ -127,13 +127,13 @@ unsigned int mv88e6390_serdes_irq_mapping(struct mv88e6xxx_chip *chip, int mv88e6352_serdes_get_sset_count(struct mv88e6xxx_chip *chip, int port); int mv88e6352_serdes_get_strings(struct mv88e6xxx_chip *chip, int port, uint8_t *data); -int mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port, - uint64_t *data); +size_t mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port, + uint64_t *data); int mv88e6390_serdes_get_sset_count(struct mv88e6xxx_chip *chip, int port); int mv88e6390_serdes_get_strings(struct mv88e6xxx_chip *chip, int port, uint8_t *data); -int mv88e6390_serdes_get_stats(struct mv88e6xxx_chip *chip, int port, - uint64_t *data); +size_t mv88e6390_serdes_get_stats(struct mv88e6xxx_chip *chip, int port, + uint64_t *data); int mv88e6352_serdes_get_regs_len(struct mv88e6xxx_chip *chip, int port); void mv88e6352_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p);
mv88e6xxx_get_stats, which collects stats from various sources, expects all callees to return the number of stats read. If an error occurs, 0 should be returned. Prevent future mishaps of this kind by updating the return type to reflect this contract. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- drivers/net/dsa/mv88e6xxx/chip.h | 4 ++-- drivers/net/dsa/mv88e6xxx/serdes.c | 10 +++++----- drivers/net/dsa/mv88e6xxx/serdes.h | 8 ++++---- 3 files changed, 11 insertions(+), 11 deletions(-)