Re: [PATCH v2 net-next 3/6] net: dsa: mv88e6xxx: Fix mv88e6352_serdes_get_stats error path

[Tobias Waldekranz <tobias@waldekranz.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?