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 X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3FD4AC07E9A for ; Mon, 5 Jul 2021 07:38:41 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 24FBF611ED for ; Mon, 5 Jul 2021 07:38:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230023AbhGEHlQ (ORCPT ); Mon, 5 Jul 2021 03:41:16 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:47160 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229817AbhGEHlO (ORCPT ); Mon, 5 Jul 2021 03:41:14 -0400 Received: from [222.129.38.159] (helo=[192.168.1.18]) by youngberry.canonical.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.93) (envelope-from ) id 1m0JBc-0001cn-4h; Mon, 05 Jul 2021 07:38:36 +0000 To: "Neftin, Sasha" , jesse.brandeburg@intel.com, anthony.l.nguyen@intel.com, davem@davemloft.net, kuba@kernel.org, intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, "Edri, Michael" , "Ruinskiy, Dima" References: <20210702045120.22855-1-aaron.ma@canonical.com> <20210702045120.22855-2-aaron.ma@canonical.com> <613e2106-940a-49ed-6621-0bb00bc7dca5@intel.com> From: Aaron Ma Subject: Re: [Intel-wired-lan] [PATCH 2/2] igc: wait for the MAC copy when enabled MAC passthrough Message-ID: Date: Mon, 5 Jul 2021 15:38:29 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <613e2106-940a-49ed-6621-0bb00bc7dca5@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 7/4/21 1:36 PM, Neftin, Sasha wrote: > On 7/2/2021 07:51, Aaron Ma wrote: >> Such as dock hot plug event when runtime, for hardware implementation, >> the MAC copy takes less than one second when BIOS enabled MAC passthrough. >> After test on Lenovo TBT4 dock, 600ms is enough to update the >> MAC address. >> Otherwise ethernet fails to work. >> >> Signed-off-by: Aaron Ma >> --- >>   drivers/net/ethernet/intel/igc/igc_main.c | 3 +++ >>   1 file changed, 3 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> index 606b72cb6193..c8bc5f089255 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -5468,6 +5468,9 @@ static int igc_probe(struct pci_dev *pdev, >>       memcpy(&hw->mac.ops, ei->mac_ops, sizeof(hw->mac.ops)); >>       memcpy(&hw->phy.ops, ei->phy_ops, sizeof(hw->phy.ops)); >> +    if (pci_is_thunderbolt_attached(pdev) > +        msleep(600); > I believe it is a bit fragile. I would recommend here look for another indication instead of delay. Can we poll for a 'pci_channel_io_normal' state? (igc->pdev->error_state == pci_channel_io_normal) Hi sasha, In this situation, the error_state is always pci_channel_io_normal. The delay is necessary. Refer to "627239-Intel® Ethernet Controller I225-MAC-Address-Passthrough-rev1.2" section "3.5 Timing Considerations": "For hardware implementation, when the operating system is already running, the MAC copy must happen not more than one second after TBT link is established. the I225 Windows driver prevents the operating system from detecting the I225 for one second. This allows enough time for hardware to update the MAC address." Thanks sasha, Aaron >> + >>       /* Initialize skew-specific constants */ >>       err = ei->get_invariants(hw); >>       if (err) >> > Thanks Aaron, > sasha From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aaron Ma Date: Mon, 5 Jul 2021 15:38:29 +0800 Subject: [Intel-wired-lan] [PATCH 2/2] igc: wait for the MAC copy when enabled MAC passthrough In-Reply-To: <613e2106-940a-49ed-6621-0bb00bc7dca5@intel.com> References: <20210702045120.22855-1-aaron.ma@canonical.com> <20210702045120.22855-2-aaron.ma@canonical.com> <613e2106-940a-49ed-6621-0bb00bc7dca5@intel.com> Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: On 7/4/21 1:36 PM, Neftin, Sasha wrote: > On 7/2/2021 07:51, Aaron Ma wrote: >> Such as dock hot plug event when runtime, for hardware implementation, >> the MAC copy takes less than one second when BIOS enabled MAC passthrough. >> After test on Lenovo TBT4 dock, 600ms is enough to update the >> MAC address. >> Otherwise ethernet fails to work. >> >> Signed-off-by: Aaron Ma >> --- >> ? drivers/net/ethernet/intel/igc/igc_main.c | 3 +++ >> ? 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> index 606b72cb6193..c8bc5f089255 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -5468,6 +5468,9 @@ static int igc_probe(struct pci_dev *pdev, >> ????? memcpy(&hw->mac.ops, ei->mac_ops, sizeof(hw->mac.ops)); >> ????? memcpy(&hw->phy.ops, ei->phy_ops, sizeof(hw->phy.ops)); >> +??? if (pci_is_thunderbolt_attached(pdev) > +??????? msleep(600); > I believe it is a bit fragile. I would recommend here look for another indication instead of delay. Can we poll for a 'pci_channel_io_normal' state? (igc->pdev->error_state == pci_channel_io_normal) Hi sasha, In this situation, the error_state is always pci_channel_io_normal. The delay is necessary. Refer to "627239-Intel? Ethernet Controller I225-MAC-Address-Passthrough-rev1.2" section "3.5 Timing Considerations": "For hardware implementation, when the operating system is already running, the MAC copy must happen not more than one second after TBT link is established. the I225 Windows driver prevents the operating system from detecting the I225 for one second. This allows enough time for hardware to update the MAC address." Thanks sasha, Aaron >> + >> ????? /* Initialize skew-specific constants */ >> ????? err = ei->get_invariants(hw); >> ????? if (err) >> > Thanks Aaron, > sasha