From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id F03C9CDB47F for ; Wed, 24 Jun 2026 16:40:46 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id B1E2A40AA5; Wed, 24 Jun 2026 16:40:46 +0000 (UTC) X-Virus-Scanned: amavis at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavis, port 10024) with ESMTP id Z7zgGh79i6cM; Wed, 24 Jun 2026 16:40:44 +0000 (UTC) X-Comment: SPF check N/A for local connections - client-ip=140.211.166.142; helo=lists1.osuosl.org; envelope-from=intel-wired-lan-bounces@osuosl.org; receiver= DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 6673340A9B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=osuosl.org; s=default; t=1782319244; bh=Vn4fFZoy5PlSeydUMxpIJV+IuqKhUGEhHOXrG3ACH98=; h=From:To:Cc:Date:In-Reply-To:References:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=dSKZHv/vZjs8XgfDK5dYy7js3DnL0+Wsh+F/TiKZRH3n0EPwrCqxl7rilbcX4vSY9 IxQEhs6e90J+7rmcj8wudNDwb7XkRVb+ZNWLWxoZmzTPBX7tbx49IfaxnnvchdGy/G 2oCdMjjstJJZcQhRT/plSDTZi1KgxubsIcQdxZHAvwMPSpNAstJS5I/ch4LwQOuY9k mcOsFhTYx0BVoAYvdsWdh8Z8ddI6AdnKIwAMlb2Y5e5LRVbhv0nt8gRjU3WXcrfIpL snE6cEIAsJxHdL3hhTAIwNOJf85Wo1MrRlKsgMuOzfahgygdTRnNWYJPmWeQNUM6Gq TwHgRuQdBCfjQ== Received: from lists1.osuosl.org (lists1.osuosl.org [140.211.166.142]) by smtp4.osuosl.org (Postfix) with ESMTP id 6673340A9B; Wed, 24 Jun 2026 16:40:44 +0000 (UTC) Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists1.osuosl.org (Postfix) with ESMTP id 2E07C367 for ; Wed, 24 Jun 2026 16:40:43 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 249B2812F8 for ; Wed, 24 Jun 2026 16:40:43 +0000 (UTC) X-Virus-Scanned: amavis at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavis, port 10024) with ESMTP id kmGKqF4P187D for ; Wed, 24 Jun 2026 16:40:42 +0000 (UTC) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=2600:3c04:e001:324:0:1991:8:25; helo=tor.source.kernel.org; envelope-from=horms@kernel.org; receiver= DMARC-Filter: OpenDMARC Filter v1.4.2 smtp1.osuosl.org 2436581204 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 2436581204 Received: from tor.source.kernel.org (tor.source.kernel.org [IPv6:2600:3c04:e001:324:0:1991:8:25]) by smtp1.osuosl.org (Postfix) with ESMTPS id 2436581204 for ; Wed, 24 Jun 2026 16:40:41 +0000 (UTC) Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) by tor.source.kernel.org (Postfix) with ESMTP id 9A0E560018; Wed, 24 Jun 2026 16:40:40 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1401D1F00A3A; Wed, 24 Jun 2026 16:40:37 +0000 (UTC) From: Simon Horman To: jtornosm@redhat.com Cc: Simon Horman , netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org, przemyslaw.kitszel@intel.com, aleksandr.loktionov@intel.com, jacob.e.keller@intel.com, anthony.l.nguyen@intel.com, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com Date: Wed, 24 Jun 2026 17:31:49 +0100 Message-ID: <20260624163148.1146093-2-horms@kernel.org> X-Mailer: git-send-email 2.54.0 In-Reply-To: <20260623101800.991293-4-jtornosm@redhat.com> References: <20260623101800.991293-4-jtornosm@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Mailman-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1782319240; bh=Vn4fFZoy5PlSeydUMxpIJV+IuqKhUGEhHOXrG3ACH98=; h=From:To:Cc:Subject:Date:In-Reply-To:References; b=YKXSriNJ2iEGZzOocLfUXXQEv0axYK9sWp7ILG6hS3ZRG9dkoH/OqQLvQ/2oWSQfg NyNiIGXaczJ2H/5073TQJ7IaHIvYUTgSpfAxJFOXN656nAqWpLw/hJHKZtyW8hQUW1 EhCJ3s+bbTpATKV+Yy20Etj0zAm89cYf1T0jtFYWitIHCN7jmWW28E1aEokea2+dzH iDYaBnp+phUcbiF/pzivdX6oN7Adi/OFt6gN+TphZKQOuThfOudQQrbWKKsap8QKdT bs1U4u7HsnpPCXWXsEeIAR3iJsp5KHt3aV8yxY/gZAj7Ok+MmBa93iMndgR4FHhriH 85uoMQq+pGAiw== X-Mailman-Original-Authentication-Results: smtp1.osuosl.org; dmarc=pass (p=quarantine dis=none) header.from=kernel.org X-Mailman-Original-Authentication-Results: smtp1.osuosl.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20260515 header.b=YKXSriNJ Subject: Re: [Intel-wired-lan] [PATCH net v7 3/4] iavf: send MAC change request synchronously X-BeenThere: intel-wired-lan@osuosl.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Intel Wired Ethernet Linux Kernel Driver Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-wired-lan-bounces@osuosl.org Sender: "Intel-wired-lan" 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 --- iavf: send MAC change request synchronously This commit fixes a deadlock in iavf_set_mac() by making the MAC address change synchronous. It introduces iavf_poll_virtchnl_response() to directly poll the admin queue instead of waiting on the watchdog task. > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c > index 630388e9d28cb..3fa288e3798ae 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c [ ... ] > @@ -1049,25 +1103,23 @@ static int iavf_set_mac(struct net_device *netdev, void *p) > return -EADDRNOTAVAIL; > > ret = iavf_replace_primary_mac(adapter, addr->sa_data); > - > if (ret) > return ret; > > - ret = wait_event_interruptible_timeout(adapter->vc_waitqueue, > - iavf_is_mac_set_handled(netdev, addr->sa_data), > - msecs_to_jiffies(2500)); > - > - /* If ret < 0 then it means wait was interrupted. > - * If ret == 0 then it means we got a timeout. > - * else it means we got response for set MAC from PF, > - * check if netdev MAC was updated to requested MAC, > - * if yes then set MAC succeeded otherwise it failed return -EACCES > - */ > - if (ret < 0) > + ret = iavf_set_mac_sync(adapter, addr->sa_data); > + if (ret) { > + /* Rollback only if send failed (message never reached PF). > + * Don't rollback on timeout (-EAGAIN) because the message was > + * sent and PF will eventually respond. When the response arrives, > + * iavf_virtchnl_completion() will handle rollback (on PF error) > + * or acceptance (on PF success) automatically. > + */ > + if (ret != -EAGAIN) { > + iavf_mac_add_reject(adapter); > + ether_addr_copy(adapter->hw.mac.addr, netdev->dev_addr); > + } > return ret; > - > - if (!ret) > - return -EAGAIN; > > if (!ether_addr_equal(netdev->dev_addr, addr->sa_data)) > return -EACCES; [Severity: High] Will this cause a persistent state mismatch if a timeout is followed by a retry? If iavf_set_mac_sync() times out, it returns -EAGAIN and bypasses this rollback, leaving adapter->hw.mac.addr updated but netdev->dev_addr at the old MAC. If the user then retries before the PF asynchronously completes the original request, iavf_add_ether_addrs() returns -EBUSY. Since -EBUSY != -EAGAIN, the rollback runs, forcibly reverting adapter->hw.mac.addr to the old MAC. When the PF's delayed success response finally arrives, iavf_virtchnl_completion() sees the old MAC in both hw.mac.addr and netdev->dev_addr, bypassing the netdev MAC update. The PF hardware is now programmed with the new MAC, but the kernel retains the old MAC, dropping traffic. > diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > index ec234cc8bd9db..e6b7e8f82c7c1 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c [ ... ] > @@ -555,20 +556,23 @@ iavf_set_mac_addr_type(struct virtchnl_ether_addr *virtchnl_ether_addr, > * @adapter: adapter structure > * > * Request that the PF add one or more addresses to our filters. > - **/ > -void iavf_add_ether_addrs(struct iavf_adapter *adapter) > + * > + * Return: 0 on success, negative on failure > + */ > +int iavf_add_ether_addrs(struct iavf_adapter *adapter) > { > struct virtchnl_ether_addr_list *veal; > struct iavf_mac_filter *f; > int i = 0, count = 0; > bool more = false; > size_t len; > + int ret; > > if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) { > /* bail because we already have a command pending */ > dev_err(&adapter->pdev->dev, "Cannot add filters, command %d pending\n", > adapter->current_op); > - return; > + return -EBUSY; > } [Severity: High] Could routine background tasks cause MAC changes to frequently fail? The watchdog task routinely sets adapter->current_op for background commands like VIRTCHNL_OP_GET_STATS. If iavf_set_mac() is called during this window, iavf_set_mac_sync() will hit this check and immediately fail the MAC change with -EBUSY. Because there is no retry logic in iavf_set_mac(), this error propagates directly to userspace, making MAC address configuration randomly fail based on watchdog timing. [ ... ] > @@ -712,8 +724,8 @@ static void iavf_mac_add_ok(struct iavf_adapter *adapter) > * @adapter: adapter structure > * > * Remove filters from list based on PF response. > - **/ > -static void iavf_mac_add_reject(struct iavf_adapter *adapter) > + */ > +void iavf_mac_add_reject(struct iavf_adapter *adapter) > { > struct net_device *netdev = adapter->netdev; > struct iavf_mac_filter *f, *ftmp; [Severity: Medium] Does exporting and reusing iavf_mac_add_reject() for local rollback cause unintended background MAC changes? When the user requests a MAC address that is already in the filter list (e.g., a secondary MAC), iavf_add_filter() sets f->is_new_mac = false and f->add = true, and flags IAVF_FLAG_AQ_ADD_MAC_FILTER. If the synchronous operation fails locally (like the -EBUSY return above), iavf_set_mac() calls this function. However, iavf_mac_add_reject() only clears the pending state and removes the filter if f->is_new_mac is true: if (f->is_new_mac) { list_del(&f->list); kfree(f); } Reused filters are ignored, leaving f->add = true and the IAVF_FLAG_AQ_ADD_MAC_FILTER flag active. The background watchdog task will eventually process this flag and blindly send the MAC configuration to the PF, even though the VF already aborted the operation locally. 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 7DF7B329C71 for ; Wed, 24 Jun 2026 16:40:40 +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=1782319241; cv=none; b=qT0hr6I7BpU8Btnzd0AYxFys4ADM149l6ZU+Bzn1fYObOub1yPVjvcGBqGcWrCN+FUguv8S5+lKRXQGKsOSShhlxtrz5IH//zr0E/G0p+AWNdx5FdFAd+6GWI37fz/r+vLqwo7SdkmjsPMlzRcQP4tQmtMX8laL+kRuJM9jdIhY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782319241; c=relaxed/simple; bh=ERgO9fR+JNxq7Wf7Mihb00HfnEaaOROeM3Vj1Fmqhnc=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=BD3aALWiElljSXdeNjB2x1up+hit59DxlROOd2+0EubYSIdEbm1p8/DnoWsbzEuvfBjbVR7QQ1BQO72Gh+XObUD6Qcnwrq4/8HxzxDMXFV01+MqNXUwJ64lR8FgsRF5eT3MxKMSpzswiAL7mM8sKFYH7CeNC3ynmVRzIszPW5QE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=YKXSriNJ; 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="YKXSriNJ" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1401D1F00A3A; Wed, 24 Jun 2026 16:40:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1782319240; bh=Vn4fFZoy5PlSeydUMxpIJV+IuqKhUGEhHOXrG3ACH98=; h=From:To:Cc:Subject:Date:In-Reply-To:References; b=YKXSriNJ2iEGZzOocLfUXXQEv0axYK9sWp7ILG6hS3ZRG9dkoH/OqQLvQ/2oWSQfg NyNiIGXaczJ2H/5073TQJ7IaHIvYUTgSpfAxJFOXN656nAqWpLw/hJHKZtyW8hQUW1 EhCJ3s+bbTpATKV+Yy20Etj0zAm89cYf1T0jtFYWitIHCN7jmWW28E1aEokea2+dzH iDYaBnp+phUcbiF/pzivdX6oN7Adi/OFt6gN+TphZKQOuThfOudQQrbWKKsap8QKdT bs1U4u7HsnpPCXWXsEeIAR3iJsp5KHt3aV8yxY/gZAj7Ok+MmBa93iMndgR4FHhriH 85uoMQq+pGAiw== From: Simon Horman To: jtornosm@redhat.com Cc: Simon Horman , netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org, przemyslaw.kitszel@intel.com, aleksandr.loktionov@intel.com, jacob.e.keller@intel.com, anthony.l.nguyen@intel.com, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com Subject: Re: [PATCH net v7 3/4] iavf: send MAC change request synchronously Date: Wed, 24 Jun 2026 17:31:49 +0100 Message-ID: <20260624163148.1146093-2-horms@kernel.org> X-Mailer: git-send-email 2.54.0 In-Reply-To: <20260623101800.991293-4-jtornosm@redhat.com> References: <20260623101800.991293-4-jtornosm@redhat.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 --- iavf: send MAC change request synchronously This commit fixes a deadlock in iavf_set_mac() by making the MAC address change synchronous. It introduces iavf_poll_virtchnl_response() to directly poll the admin queue instead of waiting on the watchdog task. > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c > index 630388e9d28cb..3fa288e3798ae 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c [ ... ] > @@ -1049,25 +1103,23 @@ static int iavf_set_mac(struct net_device *netdev, void *p) > return -EADDRNOTAVAIL; > > ret = iavf_replace_primary_mac(adapter, addr->sa_data); > - > if (ret) > return ret; > > - ret = wait_event_interruptible_timeout(adapter->vc_waitqueue, > - iavf_is_mac_set_handled(netdev, addr->sa_data), > - msecs_to_jiffies(2500)); > - > - /* If ret < 0 then it means wait was interrupted. > - * If ret == 0 then it means we got a timeout. > - * else it means we got response for set MAC from PF, > - * check if netdev MAC was updated to requested MAC, > - * if yes then set MAC succeeded otherwise it failed return -EACCES > - */ > - if (ret < 0) > + ret = iavf_set_mac_sync(adapter, addr->sa_data); > + if (ret) { > + /* Rollback only if send failed (message never reached PF). > + * Don't rollback on timeout (-EAGAIN) because the message was > + * sent and PF will eventually respond. When the response arrives, > + * iavf_virtchnl_completion() will handle rollback (on PF error) > + * or acceptance (on PF success) automatically. > + */ > + if (ret != -EAGAIN) { > + iavf_mac_add_reject(adapter); > + ether_addr_copy(adapter->hw.mac.addr, netdev->dev_addr); > + } > return ret; > - > - if (!ret) > - return -EAGAIN; > > if (!ether_addr_equal(netdev->dev_addr, addr->sa_data)) > return -EACCES; [Severity: High] Will this cause a persistent state mismatch if a timeout is followed by a retry? If iavf_set_mac_sync() times out, it returns -EAGAIN and bypasses this rollback, leaving adapter->hw.mac.addr updated but netdev->dev_addr at the old MAC. If the user then retries before the PF asynchronously completes the original request, iavf_add_ether_addrs() returns -EBUSY. Since -EBUSY != -EAGAIN, the rollback runs, forcibly reverting adapter->hw.mac.addr to the old MAC. When the PF's delayed success response finally arrives, iavf_virtchnl_completion() sees the old MAC in both hw.mac.addr and netdev->dev_addr, bypassing the netdev MAC update. The PF hardware is now programmed with the new MAC, but the kernel retains the old MAC, dropping traffic. > diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > index ec234cc8bd9db..e6b7e8f82c7c1 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c [ ... ] > @@ -555,20 +556,23 @@ iavf_set_mac_addr_type(struct virtchnl_ether_addr *virtchnl_ether_addr, > * @adapter: adapter structure > * > * Request that the PF add one or more addresses to our filters. > - **/ > -void iavf_add_ether_addrs(struct iavf_adapter *adapter) > + * > + * Return: 0 on success, negative on failure > + */ > +int iavf_add_ether_addrs(struct iavf_adapter *adapter) > { > struct virtchnl_ether_addr_list *veal; > struct iavf_mac_filter *f; > int i = 0, count = 0; > bool more = false; > size_t len; > + int ret; > > if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) { > /* bail because we already have a command pending */ > dev_err(&adapter->pdev->dev, "Cannot add filters, command %d pending\n", > adapter->current_op); > - return; > + return -EBUSY; > } [Severity: High] Could routine background tasks cause MAC changes to frequently fail? The watchdog task routinely sets adapter->current_op for background commands like VIRTCHNL_OP_GET_STATS. If iavf_set_mac() is called during this window, iavf_set_mac_sync() will hit this check and immediately fail the MAC change with -EBUSY. Because there is no retry logic in iavf_set_mac(), this error propagates directly to userspace, making MAC address configuration randomly fail based on watchdog timing. [ ... ] > @@ -712,8 +724,8 @@ static void iavf_mac_add_ok(struct iavf_adapter *adapter) > * @adapter: adapter structure > * > * Remove filters from list based on PF response. > - **/ > -static void iavf_mac_add_reject(struct iavf_adapter *adapter) > + */ > +void iavf_mac_add_reject(struct iavf_adapter *adapter) > { > struct net_device *netdev = adapter->netdev; > struct iavf_mac_filter *f, *ftmp; [Severity: Medium] Does exporting and reusing iavf_mac_add_reject() for local rollback cause unintended background MAC changes? When the user requests a MAC address that is already in the filter list (e.g., a secondary MAC), iavf_add_filter() sets f->is_new_mac = false and f->add = true, and flags IAVF_FLAG_AQ_ADD_MAC_FILTER. If the synchronous operation fails locally (like the -EBUSY return above), iavf_set_mac() calls this function. However, iavf_mac_add_reject() only clears the pending state and removes the filter if f->is_new_mac is true: if (f->is_new_mac) { list_del(&f->list); kfree(f); } Reused filters are ignored, leaving f->add = true and the IAVF_FLAG_AQ_ADD_MAC_FILTER flag active. The background watchdog task will eventually process this flag and blindly send the MAC configuration to the PF, even though the VF already aborted the operation locally.