Skip to content

Commit 5e7695e

Browse files
walking-machinePaolo Abeni
authored andcommitted
idpf: Interpret .set_channels() input differently
Unlike ice, idpf does not check, if user has requested at least 1 combined channel. Instead, it relies on a check in the core code. Unfortunately, the check does not trigger for us because of the hacky .set_channels() interpretation logic that is not consistent with the core code. This naturally leads to user being able to trigger a crash with an invalid input. This is how: 1. ethtool -l <IFNAME> -> combined: 40 2. ethtool -L <IFNAME> rx 0 tx 0 combined number is not specified, so command becomes {rx_count = 0, tx_count = 0, combined_count = 40}. 3. ethnl_set_channels checks, if there is at least 1 RX and 1 TX channel, comparing (combined_count + rx_count) and (combined_count + tx_count) to zero. Obviously, (40 + 0) is greater than zero, so the core code deems the input OK. 4. idpf interprets `rx 0 tx 0` as 0 channels and tries to proceed with such configuration. The issue has to be solved fundamentally, as current logic is also known to cause AF_XDP problems in ice [0]. Interpret the command in a way that is more consistent with ethtool manual [1] (--show-channels and --set-channels) and new ice logic. Considering that in the idpf driver only the difference between RX and TX queues forms dedicated channels, change the correct way to set number of channels to: ethtool -L <IFNAME> combined 10 /* For symmetric queues */ ethtool -L <IFNAME> combined 8 tx 2 rx 0 /* For asymmetric queues */ [0] https://lore.kernel.org/netdev/[email protected]/ [1] https://man7.org/linux/man-pages/man8/ethtool.8.html Fixes: 02cbfba ("idpf: add ethtool callbacks") Reviewed-by: Przemek Kitszel <[email protected]> Reviewed-by: Igor Bagnucki <[email protected]> Signed-off-by: Larysa Zaremba <[email protected]> Tested-by: Krishneil Singh <[email protected]> Reviewed-by: Simon Horman <[email protected]> Signed-off-by: Jacob Keller <[email protected]> Signed-off-by: Paolo Abeni <[email protected]>
1 parent 05d6f44 commit 5e7695e

File tree

1 file changed

+6
-15
lines changed

1 file changed

+6
-15
lines changed

drivers/net/ethernet/intel/idpf/idpf_ethtool.c

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -222,14 +222,19 @@ static int idpf_set_channels(struct net_device *netdev,
222222
struct ethtool_channels *ch)
223223
{
224224
struct idpf_vport_config *vport_config;
225-
u16 combined, num_txq, num_rxq;
226225
unsigned int num_req_tx_q;
227226
unsigned int num_req_rx_q;
228227
struct idpf_vport *vport;
228+
u16 num_txq, num_rxq;
229229
struct device *dev;
230230
int err = 0;
231231
u16 idx;
232232

233+
if (ch->rx_count && ch->tx_count) {
234+
netdev_err(netdev, "Dedicated RX or TX channels cannot be used simultaneously\n");
235+
return -EINVAL;
236+
}
237+
233238
idpf_vport_ctrl_lock(netdev);
234239
vport = idpf_netdev_to_vport(netdev);
235240

@@ -239,20 +244,6 @@ static int idpf_set_channels(struct net_device *netdev,
239244
num_txq = vport_config->user_config.num_req_tx_qs;
240245
num_rxq = vport_config->user_config.num_req_rx_qs;
241246

242-
combined = min(num_txq, num_rxq);
243-
244-
/* these checks are for cases where user didn't specify a particular
245-
* value on cmd line but we get non-zero value anyway via
246-
* get_channels(); look at ethtool.c in ethtool repository (the user
247-
* space part), particularly, do_schannels() routine
248-
*/
249-
if (ch->combined_count == combined)
250-
ch->combined_count = 0;
251-
if (ch->combined_count && ch->rx_count == num_rxq - combined)
252-
ch->rx_count = 0;
253-
if (ch->combined_count && ch->tx_count == num_txq - combined)
254-
ch->tx_count = 0;
255-
256247
num_req_tx_q = ch->combined_count + ch->tx_count;
257248
num_req_rx_q = ch->combined_count + ch->rx_count;
258249

0 commit comments

Comments
 (0)