| Message ID | 20210509115513.4121549-1-olteanv@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dsa: fix error code getting shifted with 4 in dsa_slave_get_sset_count | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: buytenh@wantstofly.org; 10 maintainers not CCed: buytenh@wantstofly.org yhs@fb.com kpsingh@kernel.org daniel@iogearbox.net andrii@kernel.org bpf@vger.kernel.org kafai@fb.com ast@kernel.org john.fastabend@gmail.com songliubraving@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 20 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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.
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/
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.
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(); }