* [PATCH net] ethtool: fix the check logic of at least one channel for RX/TX
@ 2021-02-23 13:24 Simon Horman
2021-02-23 17:01 ` Jakub Kicinski
2021-02-24 0:32 ` Michal Kubecek
0 siblings, 2 replies; 7+ messages in thread
From: Simon Horman @ 2021-02-23 13:24 UTC (permalink / raw)
To: David Miller, Jakub Kicinski
Cc: netdev, oss-drivers, Yinjun Zhang, Simon Horman, Louis Peens
From: Yinjun Zhang <yinjun.zhang@corigine.com>
The command "ethtool -L <intf> combined 0" may clean the RX/TX channel
count and skip the error path, since the attrs
tb[ETHTOOL_A_CHANNELS_RX_COUNT] and tb[ETHTOOL_A_CHANNELS_TX_COUNT]
are NULL in this case when recent ethtool is used.
Tested using ethtool v5.10.
Fixes: 7be92514b99c ("ethtool: check if there is at least one channel for TX/RX in the core")
Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
Signed-off-by: Simon Horman <simon.horman@netronome.com>
Signed-off-by: Louis Peens <louis.peens@netronome.com>
---
net/ethtool/channels.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/ethtool/channels.c b/net/ethtool/channels.c
index 25a9e566ef5c..e35ef627f61f 100644
--- a/net/ethtool/channels.c
+++ b/net/ethtool/channels.c
@@ -175,14 +175,14 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info)
/* ensure there is at least one RX and one TX channel */
if (!channels.combined_count && !channels.rx_count)
- err_attr = tb[ETHTOOL_A_CHANNELS_RX_COUNT];
+ err_attr = mod_combined ? tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT] :
+ tb[ETHTOOL_A_CHANNELS_RX_COUNT];
else if (!channels.combined_count && !channels.tx_count)
- err_attr = tb[ETHTOOL_A_CHANNELS_TX_COUNT];
+ err_attr = mod_combined ? tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT] :
+ tb[ETHTOOL_A_CHANNELS_TX_COUNT];
else
err_attr = NULL;
if (err_attr) {
- if (mod_combined)
- err_attr = tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT];
ret = -EINVAL;
NL_SET_ERR_MSG_ATTR(info->extack, err_attr, "requested channel counts would result in no RX or TX channel being configured");
goto out_ops;
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net] ethtool: fix the check logic of at least one channel for RX/TX 2021-02-23 13:24 [PATCH net] ethtool: fix the check logic of at least one channel for RX/TX Simon Horman @ 2021-02-23 17:01 ` Jakub Kicinski 2021-02-24 0:43 ` Michal Kubecek 2021-02-24 0:32 ` Michal Kubecek 1 sibling, 1 reply; 7+ messages in thread From: Jakub Kicinski @ 2021-02-23 17:01 UTC (permalink / raw) To: Simon Horman, Michal Kubecek Cc: David Miller, netdev, oss-drivers, Yinjun Zhang, Louis Peens On Tue, 23 Feb 2021 14:24:40 +0100 Simon Horman wrote: > From: Yinjun Zhang <yinjun.zhang@corigine.com> > > The command "ethtool -L <intf> combined 0" may clean the RX/TX channel > count and skip the error path, since the attrs > tb[ETHTOOL_A_CHANNELS_RX_COUNT] and tb[ETHTOOL_A_CHANNELS_TX_COUNT] > are NULL in this case when recent ethtool is used. > > Tested using ethtool v5.10. > > Fixes: 7be92514b99c ("ethtool: check if there is at least one channel for TX/RX in the core") > Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com> > Signed-off-by: Simon Horman <simon.horman@netronome.com> > Signed-off-by: Louis Peens <louis.peens@netronome.com> Please make sure you CC Michal on ethtool patches. > diff --git a/net/ethtool/channels.c b/net/ethtool/channels.c > index 25a9e566ef5c..e35ef627f61f 100644 > --- a/net/ethtool/channels.c > +++ b/net/ethtool/channels.c > @@ -175,14 +175,14 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info) > > /* ensure there is at least one RX and one TX channel */ > if (!channels.combined_count && !channels.rx_count) > - err_attr = tb[ETHTOOL_A_CHANNELS_RX_COUNT]; > + err_attr = mod_combined ? tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT] : > + tb[ETHTOOL_A_CHANNELS_RX_COUNT]; > else if (!channels.combined_count && !channels.tx_count) > - err_attr = tb[ETHTOOL_A_CHANNELS_TX_COUNT]; > + err_attr = mod_combined ? tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT] : > + tb[ETHTOOL_A_CHANNELS_TX_COUNT]; > else > err_attr = NULL; > if (err_attr) { > - if (mod_combined) > - err_attr = tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]; > ret = -EINVAL; > NL_SET_ERR_MSG_ATTR(info->extack, err_attr, "requested channel counts would result in no RX or TX channel being configured"); > goto out_ops; In case driver decides to adjust max counts - I'd lean towards: diff --git a/net/ethtool/channels.c b/net/ethtool/channels.c index 5635604cb9ba..73d267415819 100644 --- a/net/ethtool/channels.c +++ b/net/ethtool/channels.c @@ -116,10 +116,10 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info) struct ethtool_channels channels = {}; struct ethnl_req_info req_info = {}; struct nlattr **tb = info->attrs; - const struct nlattr *err_attr; const struct ethtool_ops *ops; struct net_device *dev; u32 max_rx_in_use = 0; + u32 err_attr; int ret; ret = ethnl_parse_header_dev_get(&req_info, @@ -157,34 +157,34 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info) /* ensure new channel counts are within limits */ if (channels.rx_count > channels.max_rx) - err_attr = tb[ETHTOOL_A_CHANNELS_RX_COUNT]; + err_attr = ETHTOOL_A_CHANNELS_RX_COUNT; else if (channels.tx_count > channels.max_tx) - err_attr = tb[ETHTOOL_A_CHANNELS_TX_COUNT]; + err_attr = ETHTOOL_A_CHANNELS_TX_COUNT; else if (channels.other_count > channels.max_other) - err_attr = tb[ETHTOOL_A_CHANNELS_OTHER_COUNT]; + err_attr = ETHTOOL_A_CHANNELS_OTHER_COUNT; else if (channels.combined_count > channels.max_combined) - err_attr = tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]; + err_attr = ETHTOOL_A_CHANNELS_COMBINED_COUNT; else - err_attr = NULL; + err_attr = 0; if (err_attr) { ret = -EINVAL; - NL_SET_ERR_MSG_ATTR(info->extack, err_attr, + NL_SET_ERR_MSG_ATTR(info->extack, tb[err_attr], "requested channel count exceeds maximum"); goto out_ops; } /* ensure there is at least one RX and one TX channel */ if (!channels.combined_count && !channels.rx_count) - err_attr = tb[ETHTOOL_A_CHANNELS_RX_COUNT]; + err_attr = ETHTOOL_A_CHANNELS_RX_COUNT; else if (!channels.combined_count && !channels.tx_count) - err_attr = tb[ETHTOOL_A_CHANNELS_TX_COUNT]; + err_attr = ETHTOOL_A_CHANNELS_TX_COUNT; else - err_attr = NULL; + err_attr = 0; if (err_attr) { if (mod_combined) - err_attr = tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]; + err_attr = ETHTOOL_A_CHANNELS_COMBINED_COUNT; ret = -EINVAL; - NL_SET_ERR_MSG_ATTR(info->extack, err_attr, "requested channel counts would result in no RX or TX channel being configured"); + NL_SET_ERR_MSG_ATTR(info->extack, tb[err_attr], "requested channel counts would result in no RX or TX channel being configured"); goto out_ops; } ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net] ethtool: fix the check logic of at least one channel for RX/TX 2021-02-23 17:01 ` Jakub Kicinski @ 2021-02-24 0:43 ` Michal Kubecek 0 siblings, 0 replies; 7+ messages in thread From: Michal Kubecek @ 2021-02-24 0:43 UTC (permalink / raw) To: Jakub Kicinski Cc: Simon Horman, David Miller, netdev, oss-drivers, Yinjun Zhang, Louis Peens On Tue, Feb 23, 2021 at 09:01:06AM -0800, Jakub Kicinski wrote: > On Tue, 23 Feb 2021 14:24:40 +0100 Simon Horman wrote: > > From: Yinjun Zhang <yinjun.zhang@corigine.com> > > > > The command "ethtool -L <intf> combined 0" may clean the RX/TX channel > > count and skip the error path, since the attrs > > tb[ETHTOOL_A_CHANNELS_RX_COUNT] and tb[ETHTOOL_A_CHANNELS_TX_COUNT] > > are NULL in this case when recent ethtool is used. > > > > Tested using ethtool v5.10. > > > > Fixes: 7be92514b99c ("ethtool: check if there is at least one channel for TX/RX in the core") > > Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com> > > Signed-off-by: Simon Horman <simon.horman@netronome.com> > > Signed-off-by: Louis Peens <louis.peens@netronome.com> > > Please make sure you CC Michal on ethtool patches. > > > diff --git a/net/ethtool/channels.c b/net/ethtool/channels.c > > index 25a9e566ef5c..e35ef627f61f 100644 > > --- a/net/ethtool/channels.c > > +++ b/net/ethtool/channels.c > > @@ -175,14 +175,14 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info) > > > > /* ensure there is at least one RX and one TX channel */ > > if (!channels.combined_count && !channels.rx_count) > > - err_attr = tb[ETHTOOL_A_CHANNELS_RX_COUNT]; > > + err_attr = mod_combined ? tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT] : > > + tb[ETHTOOL_A_CHANNELS_RX_COUNT]; > > else if (!channels.combined_count && !channels.tx_count) > > - err_attr = tb[ETHTOOL_A_CHANNELS_TX_COUNT]; > > + err_attr = mod_combined ? tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT] : > > + tb[ETHTOOL_A_CHANNELS_TX_COUNT]; > > else > > err_attr = NULL; > > if (err_attr) { > > - if (mod_combined) > > - err_attr = tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]; > > ret = -EINVAL; > > NL_SET_ERR_MSG_ATTR(info->extack, err_attr, "requested channel counts would result in no RX or TX channel being configured"); > > goto out_ops; > > In case driver decides to adjust max counts - I'd lean towards: I missed this note while reading the e-mail for the first time so I thouoght this was just a cleanup. You are right, this would address both issues. Michal > > diff --git a/net/ethtool/channels.c b/net/ethtool/channels.c > index 5635604cb9ba..73d267415819 100644 > --- a/net/ethtool/channels.c > +++ b/net/ethtool/channels.c > @@ -116,10 +116,10 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info) > struct ethtool_channels channels = {}; > struct ethnl_req_info req_info = {}; > struct nlattr **tb = info->attrs; > - const struct nlattr *err_attr; > const struct ethtool_ops *ops; > struct net_device *dev; > u32 max_rx_in_use = 0; > + u32 err_attr; > int ret; > > ret = ethnl_parse_header_dev_get(&req_info, > @@ -157,34 +157,34 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info) > > /* ensure new channel counts are within limits */ > if (channels.rx_count > channels.max_rx) > - err_attr = tb[ETHTOOL_A_CHANNELS_RX_COUNT]; > + err_attr = ETHTOOL_A_CHANNELS_RX_COUNT; > else if (channels.tx_count > channels.max_tx) > - err_attr = tb[ETHTOOL_A_CHANNELS_TX_COUNT]; > + err_attr = ETHTOOL_A_CHANNELS_TX_COUNT; > else if (channels.other_count > channels.max_other) > - err_attr = tb[ETHTOOL_A_CHANNELS_OTHER_COUNT]; > + err_attr = ETHTOOL_A_CHANNELS_OTHER_COUNT; > else if (channels.combined_count > channels.max_combined) > - err_attr = tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]; > + err_attr = ETHTOOL_A_CHANNELS_COMBINED_COUNT; > else > - err_attr = NULL; > + err_attr = 0; > if (err_attr) { > ret = -EINVAL; > - NL_SET_ERR_MSG_ATTR(info->extack, err_attr, > + NL_SET_ERR_MSG_ATTR(info->extack, tb[err_attr], > "requested channel count exceeds maximum"); > goto out_ops; > } > > /* ensure there is at least one RX and one TX channel */ > if (!channels.combined_count && !channels.rx_count) > - err_attr = tb[ETHTOOL_A_CHANNELS_RX_COUNT]; > + err_attr = ETHTOOL_A_CHANNELS_RX_COUNT; > else if (!channels.combined_count && !channels.tx_count) > - err_attr = tb[ETHTOOL_A_CHANNELS_TX_COUNT]; > + err_attr = ETHTOOL_A_CHANNELS_TX_COUNT; > else > - err_attr = NULL; > + err_attr = 0; > if (err_attr) { > if (mod_combined) > - err_attr = tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]; > + err_attr = ETHTOOL_A_CHANNELS_COMBINED_COUNT; > ret = -EINVAL; > - NL_SET_ERR_MSG_ATTR(info->extack, err_attr, "requested channel counts would result in no RX or TX channel being configured"); > + NL_SET_ERR_MSG_ATTR(info->extack, tb[err_attr], "requested channel counts would result in no RX or TX channel being configured"); > goto out_ops; > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ethtool: fix the check logic of at least one channel for RX/TX 2021-02-23 13:24 [PATCH net] ethtool: fix the check logic of at least one channel for RX/TX Simon Horman 2021-02-23 17:01 ` Jakub Kicinski @ 2021-02-24 0:32 ` Michal Kubecek 2021-02-24 1:02 ` Jakub Kicinski 1 sibling, 1 reply; 7+ messages in thread From: Michal Kubecek @ 2021-02-24 0:32 UTC (permalink / raw) To: Simon Horman Cc: David Miller, Jakub Kicinski, netdev, oss-drivers, Yinjun Zhang, Louis Peens [-- Attachment #1: Type: text/plain, Size: 1945 bytes --] On Tue, Feb 23, 2021 at 02:24:40PM +0100, Simon Horman wrote: > From: Yinjun Zhang <yinjun.zhang@corigine.com> > > The command "ethtool -L <intf> combined 0" may clean the RX/TX channel > count and skip the error path, since the attrs > tb[ETHTOOL_A_CHANNELS_RX_COUNT] and tb[ETHTOOL_A_CHANNELS_TX_COUNT] > are NULL in this case when recent ethtool is used. > > Tested using ethtool v5.10. > > Fixes: 7be92514b99c ("ethtool: check if there is at least one channel for TX/RX in the core") > Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com> > Signed-off-by: Simon Horman <simon.horman@netronome.com> > Signed-off-by: Louis Peens <louis.peens@netronome.com> Reviewed-by: Michal Kubecek <mkubecek@suse.cz> > --- > net/ethtool/channels.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/ethtool/channels.c b/net/ethtool/channels.c > index 25a9e566ef5c..e35ef627f61f 100644 > --- a/net/ethtool/channels.c > +++ b/net/ethtool/channels.c > @@ -175,14 +175,14 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info) > > /* ensure there is at least one RX and one TX channel */ > if (!channels.combined_count && !channels.rx_count) > - err_attr = tb[ETHTOOL_A_CHANNELS_RX_COUNT]; > + err_attr = mod_combined ? tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT] : > + tb[ETHTOOL_A_CHANNELS_RX_COUNT]; > else if (!channels.combined_count && !channels.tx_count) > - err_attr = tb[ETHTOOL_A_CHANNELS_TX_COUNT]; > + err_attr = mod_combined ? tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT] : > + tb[ETHTOOL_A_CHANNELS_TX_COUNT]; > else > err_attr = NULL; > if (err_attr) { > - if (mod_combined) > - err_attr = tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]; > ret = -EINVAL; > NL_SET_ERR_MSG_ATTR(info->extack, err_attr, "requested channel counts would result in no RX or TX channel being configured"); > goto out_ops; > -- > 2.20.1 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ethtool: fix the check logic of at least one channel for RX/TX 2021-02-24 0:32 ` Michal Kubecek @ 2021-02-24 1:02 ` Jakub Kicinski 2021-02-24 1:10 ` Michal Kubecek 2021-02-24 1:21 ` Jakub Kicinski 0 siblings, 2 replies; 7+ messages in thread From: Jakub Kicinski @ 2021-02-24 1:02 UTC (permalink / raw) To: Michal Kubecek Cc: Simon Horman, David Miller, netdev, oss-drivers, Yinjun Zhang, Louis Peens On Wed, 24 Feb 2021 01:32:51 +0100 Michal Kubecek wrote: > On Tue, Feb 23, 2021 at 02:24:40PM +0100, Simon Horman wrote: > > From: Yinjun Zhang <yinjun.zhang@corigine.com> > > > > The command "ethtool -L <intf> combined 0" may clean the RX/TX channel > > count and skip the error path, since the attrs > > tb[ETHTOOL_A_CHANNELS_RX_COUNT] and tb[ETHTOOL_A_CHANNELS_TX_COUNT] > > are NULL in this case when recent ethtool is used. > > > > Tested using ethtool v5.10. > > > > Fixes: 7be92514b99c ("ethtool: check if there is at least one channel for TX/RX in the core") > > Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com> > > Signed-off-by: Simon Horman <simon.horman@netronome.com> > > Signed-off-by: Louis Peens <louis.peens@netronome.com> > > Reviewed-by: Michal Kubecek <mkubecek@suse.cz> IOW you prefer this to what I proposed? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ethtool: fix the check logic of at least one channel for RX/TX 2021-02-24 1:02 ` Jakub Kicinski @ 2021-02-24 1:10 ` Michal Kubecek 2021-02-24 1:21 ` Jakub Kicinski 1 sibling, 0 replies; 7+ messages in thread From: Michal Kubecek @ 2021-02-24 1:10 UTC (permalink / raw) To: Jakub Kicinski Cc: Simon Horman, David Miller, netdev, oss-drivers, Yinjun Zhang, Louis Peens On Tue, Feb 23, 2021 at 05:02:06PM -0800, Jakub Kicinski wrote: > On Wed, 24 Feb 2021 01:32:51 +0100 Michal Kubecek wrote: > > On Tue, Feb 23, 2021 at 02:24:40PM +0100, Simon Horman wrote: > > > From: Yinjun Zhang <yinjun.zhang@corigine.com> > > > > > > The command "ethtool -L <intf> combined 0" may clean the RX/TX channel > > > count and skip the error path, since the attrs > > > tb[ETHTOOL_A_CHANNELS_RX_COUNT] and tb[ETHTOOL_A_CHANNELS_TX_COUNT] > > > are NULL in this case when recent ethtool is used. > > > > > > Tested using ethtool v5.10. > > > > > > Fixes: 7be92514b99c ("ethtool: check if there is at least one channel for TX/RX in the core") > > > Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com> > > > Signed-off-by: Simon Horman <simon.horman@netronome.com> > > > Signed-off-by: Louis Peens <louis.peens@netronome.com> > > > > Reviewed-by: Michal Kubecek <mkubecek@suse.cz> > > IOW you prefer this to what I proposed? No, that was my misunderstanding, please see my reply to your e-mail. Michal ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ethtool: fix the check logic of at least one channel for RX/TX 2021-02-24 1:02 ` Jakub Kicinski 2021-02-24 1:10 ` Michal Kubecek @ 2021-02-24 1:21 ` Jakub Kicinski 1 sibling, 0 replies; 7+ messages in thread From: Jakub Kicinski @ 2021-02-24 1:21 UTC (permalink / raw) To: Michal Kubecek Cc: Simon Horman, David Miller, netdev, oss-drivers, Yinjun Zhang, Louis Peens On Tue, 23 Feb 2021 17:02:06 -0800 Jakub Kicinski wrote: > On Wed, 24 Feb 2021 01:32:51 +0100 Michal Kubecek wrote: > > On Tue, Feb 23, 2021 at 02:24:40PM +0100, Simon Horman wrote: > > > From: Yinjun Zhang <yinjun.zhang@corigine.com> > > > > > > The command "ethtool -L <intf> combined 0" may clean the RX/TX channel > > > count and skip the error path, since the attrs > > > tb[ETHTOOL_A_CHANNELS_RX_COUNT] and tb[ETHTOOL_A_CHANNELS_TX_COUNT] > > > are NULL in this case when recent ethtool is used. > > > > > > Tested using ethtool v5.10. > > > > > > Fixes: 7be92514b99c ("ethtool: check if there is at least one channel for TX/RX in the core") > > > Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com> > > > Signed-off-by: Simon Horman <simon.horman@netronome.com> > > > Signed-off-by: Louis Peens <louis.peens@netronome.com> > > > > Reviewed-by: Michal Kubecek <mkubecek@suse.cz> > > IOW you prefer this to what I proposed? Hah, now I missed your email :) ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-02-24 1:28 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-23 13:24 [PATCH net] ethtool: fix the check logic of at least one channel for RX/TX Simon Horman 2021-02-23 17:01 ` Jakub Kicinski 2021-02-24 0:43 ` Michal Kubecek 2021-02-24 0:32 ` Michal Kubecek 2021-02-24 1:02 ` Jakub Kicinski 2021-02-24 1:10 ` Michal Kubecek 2021-02-24 1:21 ` Jakub Kicinski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).