* [PATCH net-next 0/3] ethtool: set_channels: add a few more checks
@ 2020-05-15 19:48 Jakub Kicinski
2020-05-15 19:49 ` [PATCH net-next 1/3] ethtool: check if there is at least one channel for TX/RX in the core Jakub Kicinski
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Jakub Kicinski @ 2020-05-15 19:48 UTC (permalink / raw)
To: davem; +Cc: mkubecek, netdev, simon.horman, kernel-team, Jakub Kicinski
There seems to be a few more things we can check in the core before
we call drivers' ethtool_ops->set_channels. Adding the checks to
the core simplifies the drivers. This set only includes changes
to the NFP driver as an example.
There is a small risk in the first patch that someone actually
purposefully accepts a strange configuration without RX or TX
channels, but I couldn't find such a driver in the tree.
Jakub Kicinski (3):
ethtool: check if there is at least one channel for TX/RX in the core
nfp: don't check lack of RX/TX channels
ethtool: don't call set_channels in drivers if config didn't change
.../ethernet/netronome/nfp/nfp_net_ethtool.c | 3 +--
net/ethtool/channels.c | 20 +++++++++++++++++--
net/ethtool/ioctl.c | 11 ++++++++++
3 files changed, 30 insertions(+), 4 deletions(-)
--
2.25.4
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next 1/3] ethtool: check if there is at least one channel for TX/RX in the core
2020-05-15 19:48 [PATCH net-next 0/3] ethtool: set_channels: add a few more checks Jakub Kicinski
@ 2020-05-15 19:49 ` Jakub Kicinski
2020-05-15 20:56 ` Michal Kubecek
2020-05-15 19:49 ` [PATCH net-next 2/3] nfp: don't check lack of RX/TX channels Jakub Kicinski
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2020-05-15 19:49 UTC (permalink / raw)
To: davem; +Cc: mkubecek, netdev, simon.horman, kernel-team, Jakub Kicinski
Having a channel config with no ability to RX or TX traffic is
clearly wrong. Check for this in the core so the drivers don't
have to.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
net/ethtool/channels.c | 20 ++++++++++++++++++--
net/ethtool/ioctl.c | 5 +++++
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/net/ethtool/channels.c b/net/ethtool/channels.c
index 389924b65d05..3aa4975919d7 100644
--- a/net/ethtool/channels.c
+++ b/net/ethtool/channels.c
@@ -129,13 +129,13 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info)
{
struct nlattr *tb[ETHTOOL_A_CHANNELS_MAX + 1];
unsigned int from_channel, old_total, i;
+ bool mod = false, mod_combined = false;
struct ethtool_channels channels = {};
struct ethnl_req_info req_info = {};
const struct nlattr *err_attr;
const struct ethtool_ops *ops;
struct net_device *dev;
u32 max_rx_in_use = 0;
- bool mod = false;
int ret;
ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
@@ -170,7 +170,8 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info)
ethnl_update_u32(&channels.other_count,
tb[ETHTOOL_A_CHANNELS_OTHER_COUNT], &mod);
ethnl_update_u32(&channels.combined_count,
- tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT], &mod);
+ tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT], &mod_combined);
+ mod |= mod_combined;
ret = 0;
if (!mod)
goto out_ops;
@@ -193,6 +194,21 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info)
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];
+ else if (!channels.combined_count && !channels.tx_count)
+ err_attr = 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;
+ }
+
/* ensure the new Rx count fits within the configured Rx flow
* indirection table settings
*/
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 52102ab1709b..a574d60111fa 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1676,6 +1676,11 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
channels.other_count > curr.max_other)
return -EINVAL;
+ /* ensure there is at least one RX and one TX channel */
+ if (!channels.combined_count &&
+ (!channels.rx_count || !channels.tx_count))
+ return -EINVAL;
+
/* ensure the new Rx count fits within the configured Rx flow
* indirection table settings */
if (netif_is_rxfh_configured(dev) &&
--
2.25.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 2/3] nfp: don't check lack of RX/TX channels
2020-05-15 19:48 [PATCH net-next 0/3] ethtool: set_channels: add a few more checks Jakub Kicinski
2020-05-15 19:49 ` [PATCH net-next 1/3] ethtool: check if there is at least one channel for TX/RX in the core Jakub Kicinski
@ 2020-05-15 19:49 ` Jakub Kicinski
2020-05-15 20:59 ` Michal Kubecek
2020-05-18 11:44 ` Simon Horman
2020-05-15 19:49 ` [PATCH net-next 3/3] ethtool: don't call set_channels in drivers if config didn't change Jakub Kicinski
2020-05-16 20:56 ` [PATCH net-next 0/3] ethtool: set_channels: add a few more checks David Miller
3 siblings, 2 replies; 11+ messages in thread
From: Jakub Kicinski @ 2020-05-15 19:49 UTC (permalink / raw)
To: davem; +Cc: mkubecek, netdev, simon.horman, kernel-team, Jakub Kicinski
Core will now perform this check.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index a5aa3219d112..6eb9fb9a1814 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -1438,8 +1438,7 @@ static int nfp_net_set_channels(struct net_device *netdev,
unsigned int total_rx, total_tx;
/* Reject unsupported */
- if (!channel->combined_count ||
- channel->other_count != NFP_NET_NON_Q_VECTORS ||
+ if (channel->other_count != NFP_NET_NON_Q_VECTORS ||
(channel->rx_count && channel->tx_count))
return -EINVAL;
--
2.25.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 3/3] ethtool: don't call set_channels in drivers if config didn't change
2020-05-15 19:48 [PATCH net-next 0/3] ethtool: set_channels: add a few more checks Jakub Kicinski
2020-05-15 19:49 ` [PATCH net-next 1/3] ethtool: check if there is at least one channel for TX/RX in the core Jakub Kicinski
2020-05-15 19:49 ` [PATCH net-next 2/3] nfp: don't check lack of RX/TX channels Jakub Kicinski
@ 2020-05-15 19:49 ` Jakub Kicinski
2020-05-15 21:01 ` Michal Kubecek
2020-05-16 20:56 ` [PATCH net-next 0/3] ethtool: set_channels: add a few more checks David Miller
3 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2020-05-15 19:49 UTC (permalink / raw)
To: davem; +Cc: mkubecek, netdev, simon.horman, kernel-team, Jakub Kicinski
Don't call drivers if nothing changed. Netlink code already
contains this logic.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
net/ethtool/ioctl.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index a574d60111fa..eeb1137a3f23 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1669,6 +1669,12 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
dev->ethtool_ops->get_channels(dev, &curr);
+ if (channels.rx_count == curr.rx_count &&
+ channels.tx_count == curr.tx_count &&
+ channels.combined_count == curr.combined_count &&
+ channels.other_count == curr.other_count)
+ return 0;
+
/* ensure new counts are within the maximums */
if (channels.rx_count > curr.max_rx ||
channels.tx_count > curr.max_tx ||
--
2.25.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/3] ethtool: check if there is at least one channel for TX/RX in the core
2020-05-15 19:49 ` [PATCH net-next 1/3] ethtool: check if there is at least one channel for TX/RX in the core Jakub Kicinski
@ 2020-05-15 20:56 ` Michal Kubecek
2020-05-15 22:26 ` Jakub Kicinski
0 siblings, 1 reply; 11+ messages in thread
From: Michal Kubecek @ 2020-05-15 20:56 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, davem, simon.horman, kernel-team
On Fri, May 15, 2020 at 12:49:00PM -0700, Jakub Kicinski wrote:
> Having a channel config with no ability to RX or TX traffic is
> clearly wrong. Check for this in the core so the drivers don't
> have to.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Michal Kubecek <mkubecek@suse.cz>
Only one formal problem:
> @@ -170,7 +170,8 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info)
> ethnl_update_u32(&channels.other_count,
> tb[ETHTOOL_A_CHANNELS_OTHER_COUNT], &mod);
> ethnl_update_u32(&channels.combined_count,
> - tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT], &mod);
> + tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT], &mod_combined);
> + mod |= mod_combined;
> ret = 0;
> if (!mod)
> goto out_ops;
Bitwise or should do the right thing but using "|=" on bool variables
looks strange.
Michal
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/3] nfp: don't check lack of RX/TX channels
2020-05-15 19:49 ` [PATCH net-next 2/3] nfp: don't check lack of RX/TX channels Jakub Kicinski
@ 2020-05-15 20:59 ` Michal Kubecek
2020-05-18 11:44 ` Simon Horman
1 sibling, 0 replies; 11+ messages in thread
From: Michal Kubecek @ 2020-05-15 20:59 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, davem, simon.horman, kernel-team
On Fri, May 15, 2020 at 12:49:01PM -0700, Jakub Kicinski wrote:
> Core will now perform this check.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Michal Kubecek <mkubecek@suse.cz>
> ---
> drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> index a5aa3219d112..6eb9fb9a1814 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> @@ -1438,8 +1438,7 @@ static int nfp_net_set_channels(struct net_device *netdev,
> unsigned int total_rx, total_tx;
>
> /* Reject unsupported */
> - if (!channel->combined_count ||
> - channel->other_count != NFP_NET_NON_Q_VECTORS ||
> + if (channel->other_count != NFP_NET_NON_Q_VECTORS ||
> (channel->rx_count && channel->tx_count))
> return -EINVAL;
>
> --
> 2.25.4
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 3/3] ethtool: don't call set_channels in drivers if config didn't change
2020-05-15 19:49 ` [PATCH net-next 3/3] ethtool: don't call set_channels in drivers if config didn't change Jakub Kicinski
@ 2020-05-15 21:01 ` Michal Kubecek
0 siblings, 0 replies; 11+ messages in thread
From: Michal Kubecek @ 2020-05-15 21:01 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, davem, simon.horman, kernel-team
On Fri, May 15, 2020 at 12:49:02PM -0700, Jakub Kicinski wrote:
> Don't call drivers if nothing changed. Netlink code already
> contains this logic.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Michal Kubecek <mkubecek@suse.cz>
> ---
> net/ethtool/ioctl.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index a574d60111fa..eeb1137a3f23 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1669,6 +1669,12 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
>
> dev->ethtool_ops->get_channels(dev, &curr);
>
> + if (channels.rx_count == curr.rx_count &&
> + channels.tx_count == curr.tx_count &&
> + channels.combined_count == curr.combined_count &&
> + channels.other_count == curr.other_count)
> + return 0;
> +
> /* ensure new counts are within the maximums */
> if (channels.rx_count > curr.max_rx ||
> channels.tx_count > curr.max_tx ||
> --
> 2.25.4
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/3] ethtool: check if there is at least one channel for TX/RX in the core
2020-05-15 20:56 ` Michal Kubecek
@ 2020-05-15 22:26 ` Jakub Kicinski
0 siblings, 0 replies; 11+ messages in thread
From: Jakub Kicinski @ 2020-05-15 22:26 UTC (permalink / raw)
To: Michal Kubecek; +Cc: netdev, davem, simon.horman, kernel-team
On Fri, 15 May 2020 22:56:56 +0200 Michal Kubecek wrote:
> On Fri, May 15, 2020 at 12:49:00PM -0700, Jakub Kicinski wrote:
> > Having a channel config with no ability to RX or TX traffic is
> > clearly wrong. Check for this in the core so the drivers don't
> > have to.
> >
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>
> Reviewed-by: Michal Kubecek <mkubecek@suse.cz>
Thanks for the reviews!
> > @@ -170,7 +170,8 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info)
> > ethnl_update_u32(&channels.other_count,
> > tb[ETHTOOL_A_CHANNELS_OTHER_COUNT], &mod);
> > ethnl_update_u32(&channels.combined_count,
> > - tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT], &mod);
> > + tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT], &mod_combined);
> > + mod |= mod_combined;
> > ret = 0;
> > if (!mod)
> > goto out_ops;
>
> Bitwise or should do the right thing but using "|=" on bool variables
> looks strange.
Interesting, I never thought about it in this way.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 0/3] ethtool: set_channels: add a few more checks
2020-05-15 19:48 [PATCH net-next 0/3] ethtool: set_channels: add a few more checks Jakub Kicinski
` (2 preceding siblings ...)
2020-05-15 19:49 ` [PATCH net-next 3/3] ethtool: don't call set_channels in drivers if config didn't change Jakub Kicinski
@ 2020-05-16 20:56 ` David Miller
2020-05-16 21:13 ` Michal Kubecek
3 siblings, 1 reply; 11+ messages in thread
From: David Miller @ 2020-05-16 20:56 UTC (permalink / raw)
To: kuba; +Cc: mkubecek, netdev, simon.horman, kernel-team
From: Jakub Kicinski <kuba@kernel.org>
Date: Fri, 15 May 2020 12:48:59 -0700
> There seems to be a few more things we can check in the core before
> we call drivers' ethtool_ops->set_channels. Adding the checks to
> the core simplifies the drivers. This set only includes changes
> to the NFP driver as an example.
>
> There is a small risk in the first patch that someone actually
> purposefully accepts a strange configuration without RX or TX
> channels, but I couldn't find such a driver in the tree.
Series applied, thanks Jakub.
And for the record I accept logical 'or' of booleans as valid :-)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 0/3] ethtool: set_channels: add a few more checks
2020-05-16 20:56 ` [PATCH net-next 0/3] ethtool: set_channels: add a few more checks David Miller
@ 2020-05-16 21:13 ` Michal Kubecek
0 siblings, 0 replies; 11+ messages in thread
From: Michal Kubecek @ 2020-05-16 21:13 UTC (permalink / raw)
To: David Miller; +Cc: kuba, netdev, simon.horman, kernel-team
On Sat, May 16, 2020 at 01:56:58PM -0700, David Miller wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> Date: Fri, 15 May 2020 12:48:59 -0700
>
> > There seems to be a few more things we can check in the core before
> > we call drivers' ethtool_ops->set_channels. Adding the checks to
> > the core simplifies the drivers. This set only includes changes
> > to the NFP driver as an example.
> >
> > There is a small risk in the first patch that someone actually
> > purposefully accepts a strange configuration without RX or TX
> > channels, but I couldn't find such a driver in the tree.
>
> Series applied, thanks Jakub.
>
> And for the record I accept logical 'or' of booleans as valid :-)
For the record, my point was that "|=" is a bitwise operator, not logical.
Michal
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/3] nfp: don't check lack of RX/TX channels
2020-05-15 19:49 ` [PATCH net-next 2/3] nfp: don't check lack of RX/TX channels Jakub Kicinski
2020-05-15 20:59 ` Michal Kubecek
@ 2020-05-18 11:44 ` Simon Horman
1 sibling, 0 replies; 11+ messages in thread
From: Simon Horman @ 2020-05-18 11:44 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, mkubecek, netdev, kernel-team
On Fri, May 15, 2020 at 12:49:01PM -0700, Jakub Kicinski wrote:
> Core will now perform this check.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
> ---
> drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> index a5aa3219d112..6eb9fb9a1814 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> @@ -1438,8 +1438,7 @@ static int nfp_net_set_channels(struct net_device *netdev,
> unsigned int total_rx, total_tx;
>
> /* Reject unsupported */
> - if (!channel->combined_count ||
> - channel->other_count != NFP_NET_NON_Q_VECTORS ||
> + if (channel->other_count != NFP_NET_NON_Q_VECTORS ||
> (channel->rx_count && channel->tx_count))
> return -EINVAL;
>
> --
> 2.25.4
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-05-18 11:44 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-15 19:48 [PATCH net-next 0/3] ethtool: set_channels: add a few more checks Jakub Kicinski
2020-05-15 19:49 ` [PATCH net-next 1/3] ethtool: check if there is at least one channel for TX/RX in the core Jakub Kicinski
2020-05-15 20:56 ` Michal Kubecek
2020-05-15 22:26 ` Jakub Kicinski
2020-05-15 19:49 ` [PATCH net-next 2/3] nfp: don't check lack of RX/TX channels Jakub Kicinski
2020-05-15 20:59 ` Michal Kubecek
2020-05-18 11:44 ` Simon Horman
2020-05-15 19:49 ` [PATCH net-next 3/3] ethtool: don't call set_channels in drivers if config didn't change Jakub Kicinski
2020-05-15 21:01 ` Michal Kubecek
2020-05-16 20:56 ` [PATCH net-next 0/3] ethtool: set_channels: add a few more checks David Miller
2020-05-16 21:13 ` Michal Kubecek
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.