| Message ID | 20210702045120.22855-2-aaron.ma@canonical.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] igc: don't rd/wr iomem when PCI is removed | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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 <aaron.ma@canonical.com> > --- > 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) > + > /* Initialize skew-specific constants */ > err = ei->get_invariants(hw); > if (err) > Thanks Aaron, sasha
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 <aaron.ma@canonical.com> >> --- >> 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
On 7/5/2021 10:38, Aaron Ma wrote: > > > 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 <aaron.ma@canonical.com> >>> --- >>> 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. Ok. > The delay is necessary. > > Refer to "627239-Intel® Ethernet Controller > I225-MAC-Address-Passthrough-rev1.2" > section "3.5 > Timing Considerations": Hello Aaron, Thanks to point me on this document. I see... This is recommendation for Windows driver. Anyway, "delay" approach is error-prone. We need rather ask for MNG FW confirmation (message) that MAC address is copied. Can we call (in case we know that MNG FW copied MAC address): igc_rar_set (method from igc_mac.c), update the mac.addr and then perform": memcpy(netdev->dev_addr, hw->mac.addr, netdev->addr_len);? > "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 Sasha
On 7/5/21 7:54 PM, Neftin, Sasha wrote: > Hello Aaron, Thanks to point me on this document. I see... This is recommendation for Windows driver. Anyway, "delay" approach is error-prone. We need rather ask for MNG FW confirmation (message) that MAC address is copied. > Can we call (in case we know that MNG FW copied MAC address): > igc_rar_set (method from igc_mac.c), update the mac.addr and then perform": memcpy(netdev->dev_addr, hw->mac.addr, netdev->addr_len);? Without delay, after igc_rar_set, the MAC address is all 0. The MAC addr is the from dock instead of MAC passthrough with the original driver. Thanks, Aaron
On 7/6/2021 09:46, Aaron Ma wrote: > > On 7/5/21 7:54 PM, Neftin, Sasha wrote: >> Hello Aaron, Thanks to point me on this document. I see... This is >> recommendation for Windows driver. Anyway, "delay" approach is >> error-prone. We need rather ask for MNG FW confirmation (message) that >> MAC address is copied. >> Can we call (in case we know that MNG FW copied MAC address): >> igc_rar_set (method from igc_mac.c), update the mac.addr and then >> perform": memcpy(netdev->dev_addr, hw->mac.addr, netdev->addr_len);? > > Without delay, after igc_rar_set, the MAC address is all 0. > The MAC addr is the from dock instead of MAC passthrough with the > original driver. I would to like suggest checking the following direction: 1. principal question: can we update the netdev device address after it is already set during probe? I meant perform another: memcpy(netdev->dev_addr, hw->mac.addr, netdev->addr_len) up to demand 2. We need to work with Intel's firmware engineer/group and define the message/event: MAC addressis changed and should be updated. As I know MNG FW updates shadow registers. Since shadow registers are different from RAL/RAH registers - it could be a notification that the MAC address changed. Let's check it. > > Thanks, > Aaron
On 7/8/21 12:24 PM, Neftin, Sasha wrote: > I would to like suggest checking the following direction: > 1. principal question: can we update the netdev device address after it is already set during probe? I meant perform another: > memcpy(netdev->dev_addr, hw->mac.addr, netdev->addr_len) up to demand Updating MAC addr may work. Even at the end of probe, it still got the wrong MAC address, delay is still needed. Aaron > 2. We need to work with Intel's firmware engineer/group and define the message/event: MAC addressis changed and should be updated. > As I know MNG FW updates shadow registers. Since shadow registers are different from RAL/RAH registers - it could be a notification that the MAC address changed. Let's check it.
Hello, Aaron, Sasha, On 13/07/2021 16:45, Aaron Ma wrote: > > On 7/8/21 12:24 PM, Neftin, Sasha wrote: >> I would to like suggest checking the following direction: >> 1. principal question: can we update the netdev device address after >> it is already set during probe? I meant perform another: >> memcpy(netdev->dev_addr, hw->mac.addr, netdev->addr_len) up to demand > > Updating MAC addr may work. > Even at the end of probe, it still got the wrong MAC address, delay is > still needed. > > Aaron > >> 2. We need to work with Intel's firmware engineer/group and define the >> message/event: MAC addressis changed and should be updated. >> As I know MNG FW updates shadow registers. Since shadow registers are >> different from RAL/RAH registers - it could be a notification that the >> MAC address changed. Let's check it. There is an interrupt which the FW can issue to the driver to indicate that MAC address has been changed. At that point the driver can update the MAC in its internal structures. The important question is - is there away to update the OS structures at that point so that the MAC address change propagates through all the network stack. Some network stacks do not support such an update, except during device initialization (probe), so in such environments a delay is the only workaround, and it is a problematic one as we know. If we find a mechanism by which the device driver can tell the Linux network stack - "My MAC address has changed; please update it", we can implement it differently, and not need this delay. Who can help us with this inquiry? Thanks, --Dima --------------------------------------------------------------------- Intel Israel (74) Limited This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
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); + /* Initialize skew-specific constants */ err = ei->get_invariants(hw); if (err)
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 <aaron.ma@canonical.com> --- drivers/net/ethernet/intel/igc/igc_main.c | 3 +++ 1 file changed, 3 insertions(+)