From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3C11B4779BE for ; Tue, 14 Jul 2026 13:08:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784034483; cv=none; b=IGR/Kl3LudhHNTMBA5XqVGe6ypL6WEqgLMNUTX0bSFIqSjgwR6LKqxCbIreGOUo4UdF/vuQsudMVCIk6gJS0vhLBW4Wk9sYXoOv+Rq99UNj+PwEMM4Yh+26zZ+arD8ZYlY+iQzxlh7YgDCoETOsQD1/AnDjgUz8NsMyu9AaTYQo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784034483; c=relaxed/simple; bh=vMJAz5vcCLrHE1eCIBfVMLV3Q+CDyKl69z7IQv21VZc=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=J1KHXPAFJx0LPe6TPvYv/DeVwlVPenAZsA+vY9hRfQHu2HwCt6tLvcABSwpq241eHbh0ulO7/2dDjyVHTEI5xLvpMNdtJckBrtgpfVKYAiUh/FIzJP0bArykkhM7eie3kcH6yYQoOayxkVXcRu6nqJnP/kgcA8eOg/0ekmzyZQ0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=RfSX1rbs; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="RfSX1rbs" Received: by smtp.kernel.org (Postfix) with ESMTPSA id C16021F000E9; Tue, 14 Jul 2026 13:07:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1784034482; bh=OR+TALXUbrDqxIOAyEwXOdpwUaeeSd6LxkFLp/LQ0xY=; h=From:To:Cc:Subject:Date:In-Reply-To:References; b=RfSX1rbszvo/lzps5iz4Gm8FU0W6pSskBwPU8lPBHXly0vlOL/70m/UOWKYGzhlrr 2/EZo2jDlOpmSwU6j/Hv2vHvmIctbhJ/5HwmRGTW1NtkFkXyylt7Gjl4DNfmsv59g9 8muh0afsUEHpFHcqptKt3spGlSQ4mcFLjV9VwEQjp1ZrOINtMREMYwU06DL3Z5clK3 P5vK5nVmH+BQu7wP58z1ldhse0HYqQ7Dv1pPgm8wQudq9jvZvyJAAEdjnjjjOYlohP WN9qhgJK8EoBMbtgRedIKczJd8fT3SPn8LOiLmqMJmmzHvjuYgDzX4a8cD+Z6bVvv7 eBK+sB+LqioiA== From: Simon Horman To: mmc@linux.ibm.com Cc: Simon Horman , netdev@vger.kernel.org, bjking1@linux.ibm.com, haren@linux.ibm.com, ricklind@linux.ibm.com, kuba@kernel.org, edumazet@google.com, pabeni@redhat.com, linuxppc-dev@lists.ozlabs.org, maddy@linux.ibm.com, mpe@ellerman.id.au, davemarq@linux.ibm.com Subject: Re: [PATCH net-next v3 14/15] ibmveth: Wire ethtool set_channels to MQ RX queue resize Date: Tue, 14 Jul 2026 14:07:52 +0100 Message-ID: <20260714130752.1844153-1-horms@kernel.org> X-Mailer: git-send-email 2.54.0 In-Reply-To: <20260706193603.8039-15-mmc@linux.ibm.com> References: <20260706193603.8039-15-mmc@linux.ibm.com> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev --- ibmveth: Wire ethtool set_channels to MQ RX queue resize Expose incremental RX resize through ethtool channel control. get_channels() reports rx_count from adapter->num_rx_queues and max_rx as IBMVETH_MAX_RX_QUEUES when MQ firmware is enabled, else 1. set_channels() validates rx_count is within 1..IBMVETH_MAX_RX_QUEUES. > diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c > index ac4d89a66a8db..50a332ab83fd3 100644 > --- a/drivers/net/ethernet/ibm/ibmveth.c > +++ b/drivers/net/ethernet/ibm/ibmveth.c > @@ -2534,19 +2534,69 @@ static int ibmveth_set_channels(struct net_device *netdev, > struct ethtool_channels *channels) > { > struct ibmveth_adapter *adapter = netdev_priv(netdev); [ ... ] > - if (!(netdev->flags & IFF_UP)) > + if (!(netdev->flags & IFF_UP)) { > + if (goal_rx > 1 && !adapter->multi_queue) { [ ... ] > + /* Stash desired RX count; open() publishes it via > + * netif_set_real_num_rx_queues() after queue registration. > + */ > + if (goal_rx != adapter->num_rx_queues) > + adapter->num_rx_queues = goal_rx; > + > return netif_set_real_num_tx_queues(netdev, goal); [Severity: Medium] If netif_set_real_num_tx_queues() fails and returns an error, the TX configuration command will fail, but the RX queue count has already been permanently modified in adapter->num_rx_queues. Will this leave the device in an inconsistent state where the new RX count is applied on the next open despite the command failing? > + } [ ... ] > + if (goal_rx != old_rx) { > + rc = ibmveth_resize_rx_queues_incremental(adapter, goal_rx, > + rxq_entries); [Severity: High] Does this new ethtool scale-down path expose an interrupt storm? Looking at ibmveth_resize_rx_queues_incremental(), when scaling down queues it disables NAPI before disabling the hardware interrupt: drivers/net/ethernet/ibm/ibmveth.c:ibmveth_resize_rx_queues_incremental() { ... for (i = new_count; i < old_count; i++) napi_disable(&adapter->napi[i]); for (i = new_count; i < old_count; i++) ibmveth_drain_rx_queue(adapter, i); synchronize_net(); ... /* Disable hypervisor interrupts and wait for handlers to complete * before updating num_rx_queues. */ for (i = new_count; i < old_count; i++) { ibmveth_disable_irq(adapter, i); ... } If the hypervisor delivers a packet during this window, ibmveth_interrupt() will execute. Since NAPI is disabled, napi_schedule_prep() fails, causing the handler to return IRQ_HANDLED without disabling the hardware interrupt. Will this leave the virtual interrupt asserted and force an infinite interrupt loop? > + if (rc) { > + netdev_err(netdev, "Failed to resize RX queues: %d\n", rc); > + return rc; > + } > + } > > /* We have IBMVETH_MAX_QUEUES netdev_queue's allocated > * but we may need to alloc/free the ltb's. > */ [Severity: High] This is a pre-existing issue, but does this function use an uninitialized variable in the error path? When decreasing the number of TX queues (goal < old), the allocation loop for (i = old; i < goal; i++) is skipped, leaving 'i' uninitialized. If netif_set_real_num_tx_queues() then fails, the error path assigns old = i;. The cleanup loop then uses this garbage value: for (i = old; i > goal; i--) { if (adapter->tx_ltb_ptr[i - 1]) Will this access adapter->tx_ltb_ptr out of bounds and pass arbitrary pointers to ibmveth_free_tx_ltb()? > @@ -2580,7 +2630,7 @@ static int ibmveth_set_channels(struct net_device *netdev, > > netif_tx_wake_all_queues(netdev); > > - return rc; > + return 0; [Severity: High] Does this unconditional return of 0 mask TX queue configuration failures? If applying the new TX queue count via netif_set_real_num_tx_queues() fails, the code handles it locally, but the function exits with 0 regardless. Will this mislead userspace into believing the TX channel count was successfully modified? > }