* [PATCH net] net: dsa: fix error code getting shifted with 4 in dsa_slave_get_sset_count
@ 2021-05-09 11:55 Vladimir Oltean
2021-05-09 17:35 ` Florian Fainelli
0 siblings, 1 reply; 4+ messages in thread
From: Vladimir Oltean @ 2021-05-09 11:55 UTC (permalink / raw
To: David S . Miller, Jakub Kicinski, netdev
Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Dan Carpenter,
Vladimir Oltean
From: Vladimir Oltean <vladimir.oltean@nxp.com>
DSA implements a bunch of 'standardized' ethtool statistics counters,
namely tx_packets, tx_bytes, rx_packets, rx_bytes. So whatever the
hardware driver returns in .get_sset_count(), we need to add 4 to that.
That is ok, except that .get_sset_count() can return a negative error
code, for example:
b53_get_sset_count
-> phy_ethtool_get_sset_count
-> return -EIO
-EIO is -5, and with 4 added to it, it becomes -1, aka -EPERM. One can
imagine that certain error codes may even become positive, although
based on code inspection I did not see instances of that.
Check the error code first, if it is negative return it as-is.
Based on a similar patch for dsa_master_get_strings from Dan Carpenter:
https://patchwork.kernel.org/project/netdevbpf/patch/YJaSe3RPgn7gKxZv@mwanda/
Fixes: 91da11f870f0 ("net: Distributed Switch Architecture protocol support")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/slave.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 3689ffa2dbb8..dda232a8d6f5 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -798,13 +798,15 @@ static int dsa_slave_get_sset_count(struct net_device *dev, int sset)
struct dsa_switch *ds = dp->ds;
if (sset == ETH_SS_STATS) {
- int count;
+ int err = 0;
- count = 4;
- if (ds->ops->get_sset_count)
- count += ds->ops->get_sset_count(ds, dp->index, sset);
+ if (ds->ops->get_sset_count) {
+ err = ds->ops->get_sset_count(ds, dp->index, sset);
+ if (err < 0)
+ return err;
+ }
- return count;
+ return err + 4;
} else if (sset == ETH_SS_TEST) {
return net_selftest_get_count();
}
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] net: dsa: fix error code getting shifted with 4 in dsa_slave_get_sset_count
2021-05-09 11:55 [PATCH net] net: dsa: fix error code getting shifted with 4 in dsa_slave_get_sset_count Vladimir Oltean
@ 2021-05-09 17:35 ` Florian Fainelli
2021-05-09 21:54 ` Vladimir Oltean
0 siblings, 1 reply; 4+ messages in thread
From: Florian Fainelli @ 2021-05-09 17:35 UTC (permalink / raw
To: Vladimir Oltean, David S . Miller, Jakub Kicinski, netdev
Cc: Andrew Lunn, Vivien Didelot, Dan Carpenter, Vladimir Oltean
On 5/9/2021 4:55 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> DSA implements a bunch of 'standardized' ethtool statistics counters,
> namely tx_packets, tx_bytes, rx_packets, rx_bytes. So whatever the
> hardware driver returns in .get_sset_count(), we need to add 4 to that.
>
> That is ok, except that .get_sset_count() can return a negative error
> code, for example:
>
> b53_get_sset_count
> -> phy_ethtool_get_sset_count
> -> return -EIO
>
> -EIO is -5, and with 4 added to it, it becomes -1, aka -EPERM. One can
> imagine that certain error codes may even become positive, although
> based on code inspection I did not see instances of that.
>
> Check the error code first, if it is negative return it as-is.
>
> Based on a similar patch for dsa_master_get_strings from Dan Carpenter:
> https://patchwork.kernel.org/project/netdevbpf/patch/YJaSe3RPgn7gKxZv@mwanda/
>
> Fixes: 91da11f870f0 ("net: Distributed Switch Architecture protocol support")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Just one nit below:
> ---
> net/dsa/slave.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 3689ffa2dbb8..dda232a8d6f5 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -798,13 +798,15 @@ static int dsa_slave_get_sset_count(struct net_device *dev, int sset)
> struct dsa_switch *ds = dp->ds;
>
> if (sset == ETH_SS_STATS) {
> - int count;
> + int err = 0;
>
> - count = 4;
> - if (ds->ops->get_sset_count)
> - count += ds->ops->get_sset_count(ds, dp->index, sset);
> + if (ds->ops->get_sset_count) {
> + err = ds->ops->get_sset_count(ds, dp->index, sset);
> + if (err < 0)
> + return err;
> + }
>
> - return count;
I would have a preference for keeping the count variable and treating it
specifically in case it is negative.
--
Florian
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] net: dsa: fix error code getting shifted with 4 in dsa_slave_get_sset_count
2021-05-09 17:35 ` Florian Fainelli
@ 2021-05-09 21:54 ` Vladimir Oltean
2021-05-10 1:18 ` Florian Fainelli
0 siblings, 1 reply; 4+ messages in thread
From: Vladimir Oltean @ 2021-05-09 21:54 UTC (permalink / raw
To: Florian Fainelli
Cc: David S . Miller, Jakub Kicinski, netdev, Andrew Lunn,
Vivien Didelot, Dan Carpenter, Vladimir Oltean
On Sun, May 09, 2021 at 10:35:27AM -0700, Florian Fainelli wrote:
> On 5/9/2021 4:55 AM, Vladimir Oltean wrote:
> I would have a preference for keeping the count variable and treating it
> specifically in case it is negative.
Thanks. I hope I've understood you correctly that renaming "err" back to
"count" was all that you were asking for. Anyway, patch is superseded by
https://patchwork.kernel.org/project/netdevbpf/patch/20210509193338.451174-1-olteanv@gmail.com/
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] net: dsa: fix error code getting shifted with 4 in dsa_slave_get_sset_count
2021-05-09 21:54 ` Vladimir Oltean
@ 2021-05-10 1:18 ` Florian Fainelli
0 siblings, 0 replies; 4+ messages in thread
From: Florian Fainelli @ 2021-05-10 1:18 UTC (permalink / raw
To: Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev, Andrew Lunn,
Vivien Didelot, Dan Carpenter, Vladimir Oltean
On 5/9/2021 2:54 PM, Vladimir Oltean wrote:
> On Sun, May 09, 2021 at 10:35:27AM -0700, Florian Fainelli wrote:
>> On 5/9/2021 4:55 AM, Vladimir Oltean wrote:
>> I would have a preference for keeping the count variable and treating it
>> specifically in case it is negative.
>
> Thanks. I hope I've understood you correctly that renaming "err" back to
> "count" was all that you were asking for. Anyway, patch is superseded by
> https://patchwork.kernel.org/project/netdevbpf/patch/20210509193338.451174-1-olteanv@gmail.com/
You did, thanks for the quick spin.
--
Florian
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-10 1:18 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-05-09 11:55 [PATCH net] net: dsa: fix error code getting shifted with 4 in dsa_slave_get_sset_count Vladimir Oltean
2021-05-09 17:35 ` Florian Fainelli
2021-05-09 21:54 ` Vladimir Oltean
2021-05-10 1:18 ` Florian Fainelli
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.