| Message ID | 20221004081037.34064-1-hau@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] r8169: fix rtl8125b dmar pte write access not set error | expand |
On 04.10.2022 10:10, Chunhao Lin wrote: > When close device, rx will be enabled if wol is enabeld. When open device > it will cause rx to dma to wrong address after pci_set_master(). > Hi Hau, I never experienced this problem. Is it an edge case that can occur under specific circumstances? > In this patch, driver will disable tx/rx when close device. If wol is > eanbled only enable rx filter and disable rxdv_gate to let hardware > can receive packet to fifo but not to dma it. > > Fixes: 120068481405 ("r8169: fix failing WoL") > Signed-off-by: Chunhao Lin <hau@realtek.com> > --- > drivers/net/ethernet/realtek/r8169_main.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 1b7fdb4f056b..c09cfbe1d3f0 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -2239,6 +2239,9 @@ static void rtl_wol_enable_rx(struct rtl8169_private *tp) > if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > AcceptBroadcast | AcceptMulticast | AcceptMyPhys); > + > + if (tp->mac_version >= RTL_GIGA_MAC_VER_40) > + RTL_W32(tp, MISC, RTL_R32(tp, MISC) & ~RXDV_GATED_EN); In the commit title you reference RTL8125b only, but the actual change affects all chip versions from RTL8168g. So either title or patch need to be adjusted. Is the actual issue restricted to RTL8125b (hw issue?) or can it occur on all chip versions that use RXDV_GATED_EN? > } > > static void rtl_prepare_power_down(struct rtl8169_private *tp) > @@ -3981,7 +3984,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp) > netdev_reset_queue(tp->dev); > } > > -static void rtl8169_cleanup(struct rtl8169_private *tp, bool going_down) > +static void rtl8169_cleanup(struct rtl8169_private *tp) > { > napi_disable(&tp->napi); > > @@ -3993,9 +3996,6 @@ static void rtl8169_cleanup(struct rtl8169_private *tp, bool going_down) > > rtl_rx_close(tp); > > - if (going_down && tp->dev->wol_enabled) > - goto no_reset; > - Here you change the behavior for various other chip versions too. This should not be done in a fix, even if it should be safe. > switch (tp->mac_version) { > case RTL_GIGA_MAC_VER_28: > case RTL_GIGA_MAC_VER_31: > @@ -4016,7 +4016,7 @@ static void rtl8169_cleanup(struct rtl8169_private *tp, bool going_down) > } > > rtl_hw_reset(tp); > -no_reset: > + > rtl8169_tx_clear(tp); > rtl8169_init_ring_indexes(tp); > } > @@ -4027,7 +4027,7 @@ static void rtl_reset_work(struct rtl8169_private *tp) > > netif_stop_queue(tp->dev); > > - rtl8169_cleanup(tp, false); > + rtl8169_cleanup(tp); > > for (i = 0; i < NUM_RX_DESC; i++) > rtl8169_mark_to_asic(tp->RxDescArray + i); > @@ -4715,7 +4715,7 @@ static void rtl8169_down(struct rtl8169_private *tp) > pci_clear_master(tp->pci_dev); > rtl_pci_commit(tp); > > - rtl8169_cleanup(tp, true); > + rtl8169_cleanup(tp); > rtl_disable_exit_l1(tp); > rtl_prepare_power_down(tp); > }
> On 04.10.2022 10:10, Chunhao Lin wrote: > > When close device, rx will be enabled if wol is enabeld. When open > > device it will cause rx to dma to wrong address after pci_set_master(). > > > Hi Hau, > > I never experienced this problem. Is it an edge case that can occur under > specific circumstances? > This issue is happen on google chromebook with IOMMU enabled. Because rx is enabled when wol is enabled, so I think there is a chance that the packet receive in device close will be dma to invalid memory address when device is open. ------Please consider the environment before printing this e-mail.
On 04.10.2022 10:10, Chunhao Lin wrote: > When close device, rx will be enabled if wol is enabeld. When open device > it will cause rx to dma to wrong address after pci_set_master(). > > In this patch, driver will disable tx/rx when close device. If wol is > eanbled only enable rx filter and disable rxdv_gate to let hardware > can receive packet to fifo but not to dma it. > > Fixes: 120068481405 ("r8169: fix failing WoL") > Signed-off-by: Chunhao Lin <hau@realtek.com> > --- > drivers/net/ethernet/realtek/r8169_main.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 1b7fdb4f056b..c09cfbe1d3f0 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -2239,6 +2239,9 @@ static void rtl_wol_enable_rx(struct rtl8169_private *tp) > if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > AcceptBroadcast | AcceptMulticast | AcceptMyPhys); > + > + if (tp->mac_version >= RTL_GIGA_MAC_VER_40) > + RTL_W32(tp, MISC, RTL_R32(tp, MISC) & ~RXDV_GATED_EN); > } > > static void rtl_prepare_power_down(struct rtl8169_private *tp) > @@ -3981,7 +3984,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp) > netdev_reset_queue(tp->dev); > } > > -static void rtl8169_cleanup(struct rtl8169_private *tp, bool going_down) > +static void rtl8169_cleanup(struct rtl8169_private *tp) > { > napi_disable(&tp->napi); > > @@ -3993,9 +3996,6 @@ static void rtl8169_cleanup(struct rtl8169_private *tp, bool going_down) > > rtl_rx_close(tp); > > - if (going_down && tp->dev->wol_enabled) > - goto no_reset; > - > switch (tp->mac_version) { > case RTL_GIGA_MAC_VER_28: > case RTL_GIGA_MAC_VER_31: > @@ -4016,7 +4016,7 @@ static void rtl8169_cleanup(struct rtl8169_private *tp, bool going_down) > } > > rtl_hw_reset(tp); > -no_reset: > + > rtl8169_tx_clear(tp); > rtl8169_init_ring_indexes(tp); > } > @@ -4027,7 +4027,7 @@ static void rtl_reset_work(struct rtl8169_private *tp) > > netif_stop_queue(tp->dev); > > - rtl8169_cleanup(tp, false); > + rtl8169_cleanup(tp); > > for (i = 0; i < NUM_RX_DESC; i++) > rtl8169_mark_to_asic(tp->RxDescArray + i); > @@ -4715,7 +4715,7 @@ static void rtl8169_down(struct rtl8169_private *tp) > pci_clear_master(tp->pci_dev); > rtl_pci_commit(tp); > > - rtl8169_cleanup(tp, true); > + rtl8169_cleanup(tp); > rtl_disable_exit_l1(tp); > rtl_prepare_power_down(tp); > } Hi Hau, I think the following simple change should also fix the issue. DMA is enabled only after the chip has been reset in rtl_reset_work(). This should ensure that there are no stale RX DMA descriptors any longer. Could you please test it? diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 114f88497..1d72691a4 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -4610,13 +4610,13 @@ static void rtl8169_down(struct rtl8169_private *tp) static void rtl8169_up(struct rtl8169_private *tp) { - pci_set_master(tp->pci_dev); phy_init_hw(tp->phydev); phy_resume(tp->phydev); rtl8169_init_phy(tp); napi_enable(&tp->napi); set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags); rtl_reset_work(tp); + pci_set_master(tp->pci_dev); phy_start(tp->phydev); }
> > I think the following simple change should also fix the issue. > DMA is enabled only after the chip has been reset in rtl_reset_work(). > This should ensure that there are no stale RX DMA descriptors any longer. > Could you please test it? > > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c > b/drivers/net/ethernet/realtek/r8169_main.c > index 114f88497..1d72691a4 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -4610,13 +4610,13 @@ static void rtl8169_down(struct rtl8169_private > *tp) > > static void rtl8169_up(struct rtl8169_private *tp) { > - pci_set_master(tp->pci_dev); > phy_init_hw(tp->phydev); > phy_resume(tp->phydev); > rtl8169_init_phy(tp); > napi_enable(&tp->napi); > set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags); > rtl_reset_work(tp); > + pci_set_master(tp->pci_dev); > > phy_start(tp->phydev); > } > -- > 2.38.0 > This can fix the issue. But it will cause another error message " rtl_rxtx_empty_cond == 0 (loop: 42, delay: 100)". Because if rx is enabled, packet will be move to fifo. When driver check if fifo is empty on device open, hardware will not dma this packet because bus master is disabled, fifo will always not empty. So it might be better to disable txrx when close device. ------Please consider the environment before printing this e-mail.
On 04.10.2022 10:10, Chunhao Lin wrote: > When close device, rx will be enabled if wol is enabeld. When open device > it will cause rx to dma to wrong address after pci_set_master(). > > In this patch, driver will disable tx/rx when close device. If wol is > eanbled only enable rx filter and disable rxdv_gate to let hardware > can receive packet to fifo but not to dma it. > > Fixes: 120068481405 ("r8169: fix failing WoL") > Signed-off-by: Chunhao Lin <hau@realtek.com> > --- > drivers/net/ethernet/realtek/r8169_main.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 1b7fdb4f056b..c09cfbe1d3f0 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -2239,6 +2239,9 @@ static void rtl_wol_enable_rx(struct rtl8169_private *tp) > if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > AcceptBroadcast | AcceptMulticast | AcceptMyPhys); > + > + if (tp->mac_version >= RTL_GIGA_MAC_VER_40) > + RTL_W32(tp, MISC, RTL_R32(tp, MISC) & ~RXDV_GATED_EN); Wouldn't this be sufficient? Why is the change to rtl8169_cleanup() needed? > } > > static void rtl_prepare_power_down(struct rtl8169_private *tp) > @@ -3981,7 +3984,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp) > netdev_reset_queue(tp->dev); > } > > -static void rtl8169_cleanup(struct rtl8169_private *tp, bool going_down) > +static void rtl8169_private (struct rtl8169_private *tp) > { > napi_disable(&tp->napi); > > @@ -3993,9 +3996,6 @@ static void rtl8169_cleanup(struct rtl8169_private *tp, bool going_down) > > rtl_rx_close(tp); > > - if (going_down && tp->dev->wol_enabled) > - goto no_reset; > - > switch (tp->mac_version) { > case RTL_GIGA_MAC_VER_28: > case RTL_GIGA_MAC_VER_31: > @@ -4016,7 +4016,7 @@ static void rtl8169_cleanup(struct rtl8169_private *tp, bool going_down) > } > > rtl_hw_reset(tp); > -no_reset: > + > rtl8169_tx_clear(tp); > rtl8169_init_ring_indexes(tp); > } > @@ -4027,7 +4027,7 @@ static void rtl_reset_work(struct rtl8169_private *tp) > > netif_stop_queue(tp->dev); > > - rtl8169_cleanup(tp, false); > + rtl8169_cleanup(tp); > > for (i = 0; i < NUM_RX_DESC; i++) > rtl8169_mark_to_asic(tp->RxDescArray + i); > @@ -4715,7 +4715,7 @@ static void rtl8169_down(struct rtl8169_private *tp) > pci_clear_master(tp->pci_dev); > rtl_pci_commit(tp); > > - rtl8169_cleanup(tp, true); > + rtl8169_cleanup(tp); > rtl_disable_exit_l1(tp); > rtl_prepare_power_down(tp); > }
On 04.10.2022 10:10, Chunhao Lin wrote: > When close device, rx will be enabled if wol is enabeld. When open device > it will cause rx to dma to wrong address after pci_set_master(). > > In this patch, driver will disable tx/rx when close device. If wol is > eanbled only enable rx filter and disable rxdv_gate to let hardware > can receive packet to fifo but not to dma it. > > Fixes: 120068481405 ("r8169: fix failing WoL") > Signed-off-by: Chunhao Lin <hau@realtek.com> > --- > drivers/net/ethernet/realtek/r8169_main.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 1b7fdb4f056b..c09cfbe1d3f0 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -2239,6 +2239,9 @@ static void rtl_wol_enable_rx(struct rtl8169_private *tp) > if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > AcceptBroadcast | AcceptMulticast | AcceptMyPhys); > + > + if (tp->mac_version >= RTL_GIGA_MAC_VER_40) > + RTL_W32(tp, MISC, RTL_R32(tp, MISC) & ~RXDV_GATED_EN); Is this correct anyway? Supposedly you want to set this bit to disable DMA.
> > On 04.10.2022 10:10, Chunhao Lin wrote: > > When close device, rx will be enabled if wol is enabeld. When open > > device it will cause rx to dma to wrong address after pci_set_master(). > > > > In this patch, driver will disable tx/rx when close device. If wol is > > eanbled only enable rx filter and disable rxdv_gate to let hardware > > can receive packet to fifo but not to dma it. > > > > Fixes: 120068481405 ("r8169: fix failing WoL") > > Signed-off-by: Chunhao Lin <hau@realtek.com> > > --- > > drivers/net/ethernet/realtek/r8169_main.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c > > b/drivers/net/ethernet/realtek/r8169_main.c > > index 1b7fdb4f056b..c09cfbe1d3f0 100644 > > --- a/drivers/net/ethernet/realtek/r8169_main.c > > +++ b/drivers/net/ethernet/realtek/r8169_main.c > > @@ -2239,6 +2239,9 @@ static void rtl_wol_enable_rx(struct > rtl8169_private *tp) > > if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > > RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > > AcceptBroadcast | AcceptMulticast | AcceptMyPhys); > > + > > + if (tp->mac_version >= RTL_GIGA_MAC_VER_40) > > + RTL_W32(tp, MISC, RTL_R32(tp, MISC) & ~RXDV_GATED_EN); > > Is this correct anyway? Supposedly you want to set this bit to disable DMA. > If wol is enabled, driver need to disable hardware rxdv_gate for receiving packets. ------Please consider the environment before printing this e-mail.
On 12.10.2022 09:59, Hau wrote: >> >> On 04.10.2022 10:10, Chunhao Lin wrote: >>> When close device, rx will be enabled if wol is enabeld. When open >>> device it will cause rx to dma to wrong address after pci_set_master(). >>> >>> In this patch, driver will disable tx/rx when close device. If wol is >>> eanbled only enable rx filter and disable rxdv_gate to let hardware >>> can receive packet to fifo but not to dma it. >>> >>> Fixes: 120068481405 ("r8169: fix failing WoL") >>> Signed-off-by: Chunhao Lin <hau@realtek.com> >>> --- >>> drivers/net/ethernet/realtek/r8169_main.c | 14 +++++++------- >>> 1 file changed, 7 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c >>> b/drivers/net/ethernet/realtek/r8169_main.c >>> index 1b7fdb4f056b..c09cfbe1d3f0 100644 >>> --- a/drivers/net/ethernet/realtek/r8169_main.c >>> +++ b/drivers/net/ethernet/realtek/r8169_main.c >>> @@ -2239,6 +2239,9 @@ static void rtl_wol_enable_rx(struct >> rtl8169_private *tp) >>> if (tp->mac_version >= RTL_GIGA_MAC_VER_25) >>> RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | >>> AcceptBroadcast | AcceptMulticast | AcceptMyPhys); >>> + >>> + if (tp->mac_version >= RTL_GIGA_MAC_VER_40) >>> + RTL_W32(tp, MISC, RTL_R32(tp, MISC) & ~RXDV_GATED_EN); >> >> Is this correct anyway? Supposedly you want to set this bit to disable DMA. >> > If wol is enabled, driver need to disable hardware rxdv_gate for receiving packets. > OK, I see. But why disable it here? I see no scenario where rxdv_gate would be enabled when we get here.
> On 12.10.2022 09:59, Hau wrote: > >> > >> On 04.10.2022 10:10, Chunhao Lin wrote: > >>> When close device, rx will be enabled if wol is enabeld. When open > >>> device it will cause rx to dma to wrong address after pci_set_master(). > >>> > >>> In this patch, driver will disable tx/rx when close device. If wol > >>> is eanbled only enable rx filter and disable rxdv_gate to let > >>> hardware can receive packet to fifo but not to dma it. > >>> > >>> Fixes: 120068481405 ("r8169: fix failing WoL") > >>> Signed-off-by: Chunhao Lin <hau@realtek.com> > >>> --- > >>> drivers/net/ethernet/realtek/r8169_main.c | 14 +++++++------- > >>> 1 file changed, 7 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c > >>> b/drivers/net/ethernet/realtek/r8169_main.c > >>> index 1b7fdb4f056b..c09cfbe1d3f0 100644 > >>> --- a/drivers/net/ethernet/realtek/r8169_main.c > >>> +++ b/drivers/net/ethernet/realtek/r8169_main.c > >>> @@ -2239,6 +2239,9 @@ static void rtl_wol_enable_rx(struct > >> rtl8169_private *tp) > >>> if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > >>> RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > >>> AcceptBroadcast | AcceptMulticast | AcceptMyPhys); > >>> + > >>> + if (tp->mac_version >= RTL_GIGA_MAC_VER_40) > >>> + RTL_W32(tp, MISC, RTL_R32(tp, MISC) & ~RXDV_GATED_EN); > >> > >> Is this correct anyway? Supposedly you want to set this bit to disable DMA. > >> > > If wol is enabled, driver need to disable hardware rxdv_gate for receiving > packets. > > > OK, I see. But why disable it here? I see no scenario where rxdv_gate would > be enabled when we get here. > rxdv_gate will be enabled in rtl8169_cleanup(). When suspend or close and wol is enabled driver will call rtl8169_down() -> rtl8169_cleanup()-> rtl_prepare_power_down()-> rtl_wol_enable_rx(). So disabled rxdv_gate in rtl_wol_enable_rx() for receiving packets. ------Please consider the environment before printing this e-mail.
On 13.10.2022 08:04, Hau wrote: >> On 12.10.2022 09:59, Hau wrote: >>>> >>>> On 04.10.2022 10:10, Chunhao Lin wrote: >>>>> When close device, rx will be enabled if wol is enabeld. When open >>>>> device it will cause rx to dma to wrong address after pci_set_master(). >>>>> >>>>> In this patch, driver will disable tx/rx when close device. If wol >>>>> is eanbled only enable rx filter and disable rxdv_gate to let >>>>> hardware can receive packet to fifo but not to dma it. >>>>> >>>>> Fixes: 120068481405 ("r8169: fix failing WoL") >>>>> Signed-off-by: Chunhao Lin <hau@realtek.com> >>>>> --- >>>>> drivers/net/ethernet/realtek/r8169_main.c | 14 +++++++------- >>>>> 1 file changed, 7 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c >>>>> b/drivers/net/ethernet/realtek/r8169_main.c >>>>> index 1b7fdb4f056b..c09cfbe1d3f0 100644 >>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c >>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c >>>>> @@ -2239,6 +2239,9 @@ static void rtl_wol_enable_rx(struct >>>> rtl8169_private *tp) >>>>> if (tp->mac_version >= RTL_GIGA_MAC_VER_25) >>>>> RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | >>>>> AcceptBroadcast | AcceptMulticast | AcceptMyPhys); >>>>> + >>>>> + if (tp->mac_version >= RTL_GIGA_MAC_VER_40) >>>>> + RTL_W32(tp, MISC, RTL_R32(tp, MISC) & ~RXDV_GATED_EN); >>>> >>>> Is this correct anyway? Supposedly you want to set this bit to disable DMA. >>>> >>> If wol is enabled, driver need to disable hardware rxdv_gate for receiving >> packets. >>> >> OK, I see. But why disable it here? I see no scenario where rxdv_gate would >> be enabled when we get here. >> > rxdv_gate will be enabled in rtl8169_cleanup(). When suspend or close and wol is enabled > driver will call rtl8169_down() -> rtl8169_cleanup()-> rtl_prepare_power_down()-> rtl_wol_enable_rx(). > So disabled rxdv_gate in rtl_wol_enable_rx() for receiving packets. > rtl8169_cleanup() skips the call to rtl_enable_rxdvgate() when being called from rtl8169_down() and wol is enabled. This means rxdv gate is still disabled.
> On 13.10.2022 08:04, Hau wrote: > >> On 12.10.2022 09:59, Hau wrote: > >>>> > >>>> On 04.10.2022 10:10, Chunhao Lin wrote: > >>>>> When close device, rx will be enabled if wol is enabeld. When open > >>>>> device it will cause rx to dma to wrong address after pci_set_master(). > >>>>> > >>>>> In this patch, driver will disable tx/rx when close device. If wol > >>>>> is eanbled only enable rx filter and disable rxdv_gate to let > >>>>> hardware can receive packet to fifo but not to dma it. > >>>>> > >>>>> Fixes: 120068481405 ("r8169: fix failing WoL") > >>>>> Signed-off-by: Chunhao Lin <hau@realtek.com> > >>>>> --- > >>>>> drivers/net/ethernet/realtek/r8169_main.c | 14 +++++++------- > >>>>> 1 file changed, 7 insertions(+), 7 deletions(-) > >>>>> > >>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c > >>>>> b/drivers/net/ethernet/realtek/r8169_main.c > >>>>> index 1b7fdb4f056b..c09cfbe1d3f0 100644 > >>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c > >>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c > >>>>> @@ -2239,6 +2239,9 @@ static void rtl_wol_enable_rx(struct > >>>> rtl8169_private *tp) > >>>>> if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > >>>>> RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > >>>>> AcceptBroadcast | AcceptMulticast | AcceptMyPhys); > >>>>> + > >>>>> + if (tp->mac_version >= RTL_GIGA_MAC_VER_40) > >>>>> + RTL_W32(tp, MISC, RTL_R32(tp, MISC) & > ~RXDV_GATED_EN); > >>>> > >>>> Is this correct anyway? Supposedly you want to set this bit to disable > DMA. > >>>> > >>> If wol is enabled, driver need to disable hardware rxdv_gate for > >>> receiving > >> packets. > >>> > >> OK, I see. But why disable it here? I see no scenario where rxdv_gate > >> would be enabled when we get here. > >> > > rxdv_gate will be enabled in rtl8169_cleanup(). When suspend or close > > and wol is enabled driver will call rtl8169_down() -> rtl8169_cleanup()-> > rtl_prepare_power_down()-> rtl_wol_enable_rx(). > > So disabled rxdv_gate in rtl_wol_enable_rx() for receiving packets. > > > rtl8169_cleanup() skips the call to rtl_enable_rxdvgate() when being called > from > rtl8169_down() and wol is enabled. This means rxdv gate is still disabled. > Yes, it will keep rxdv_gate disable. But it will also keep tx/rx on. If OS have an unexpected reboot hardware may dma to invalid memory address. If possible I prefer to keep tx/rx off when exit driver control. ------Please consider the environment before printing this e-mail.
On 17.10.2022 19:23, Hau wrote: >> On 13.10.2022 08:04, Hau wrote: >>>> On 12.10.2022 09:59, Hau wrote: >>>>>> >>>>>> On 04.10.2022 10:10, Chunhao Lin wrote: >>>>>>> When close device, rx will be enabled if wol is enabeld. When open >>>>>>> device it will cause rx to dma to wrong address after pci_set_master(). >>>>>>> >>>>>>> In this patch, driver will disable tx/rx when close device. If wol >>>>>>> is eanbled only enable rx filter and disable rxdv_gate to let >>>>>>> hardware can receive packet to fifo but not to dma it. >>>>>>> >>>>>>> Fixes: 120068481405 ("r8169: fix failing WoL") >>>>>>> Signed-off-by: Chunhao Lin <hau@realtek.com> >>>>>>> --- >>>>>>> drivers/net/ethernet/realtek/r8169_main.c | 14 +++++++------- >>>>>>> 1 file changed, 7 insertions(+), 7 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c >>>>>>> b/drivers/net/ethernet/realtek/r8169_main.c >>>>>>> index 1b7fdb4f056b..c09cfbe1d3f0 100644 >>>>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c >>>>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c >>>>>>> @@ -2239,6 +2239,9 @@ static void rtl_wol_enable_rx(struct >>>>>> rtl8169_private *tp) >>>>>>> if (tp->mac_version >= RTL_GIGA_MAC_VER_25) >>>>>>> RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | >>>>>>> AcceptBroadcast | AcceptMulticast | AcceptMyPhys); >>>>>>> + >>>>>>> + if (tp->mac_version >= RTL_GIGA_MAC_VER_40) >>>>>>> + RTL_W32(tp, MISC, RTL_R32(tp, MISC) & >> ~RXDV_GATED_EN); >>>>>> >>>>>> Is this correct anyway? Supposedly you want to set this bit to disable >> DMA. >>>>>> >>>>> If wol is enabled, driver need to disable hardware rxdv_gate for >>>>> receiving >>>> packets. >>>>> >>>> OK, I see. But why disable it here? I see no scenario where rxdv_gate >>>> would be enabled when we get here. >>>> >>> rxdv_gate will be enabled in rtl8169_cleanup(). When suspend or close >>> and wol is enabled driver will call rtl8169_down() -> rtl8169_cleanup()-> >> rtl_prepare_power_down()-> rtl_wol_enable_rx(). >>> So disabled rxdv_gate in rtl_wol_enable_rx() for receiving packets. >>> >> rtl8169_cleanup() skips the call to rtl_enable_rxdvgate() when being called >> from >> rtl8169_down() and wol is enabled. This means rxdv gate is still disabled. >> > Yes, it will keep rxdv_gate disable. But it will also keep tx/rx on. If OS have an unexpected > reboot hardware may dma to invalid memory address. If possible I prefer to keep > tx/rx off when exit driver control. > When you say "keep tx/rx off", do you refer to the rxconfig bits in register RxConfig, or to CmdTxEnb and CmdRxEnb in ChipCmd? If we talk about the first option, then my guess would be: According to rtl_wol_enable_rx() the rx config bits are required for WoL to work on certain chip versions. With the introduction of rxdvgate this changed and setting these bits isn't needed any longer. I tested on RTL8168h and WoL worked w/o the Accept bits set in RxConfig. Please confirm or correct my understanding. static void rtl_wol_enable_rx(struct rtl8169_private *tp) { if (tp->mac_version >= RTL_GIGA_MAC_VER_25) RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | AcceptBroadcast | AcceptMulticast | AcceptMyPhys); }
> On 17.10.2022 19:23, Hau wrote: > >> On 13.10.2022 08:04, Hau wrote: > >>>> On 12.10.2022 09:59, Hau wrote: > >>>>>> > >>>>>> On 04.10.2022 10:10, Chunhao Lin wrote: > >>>>>>> When close device, rx will be enabled if wol is enabeld. When > >>>>>>> open device it will cause rx to dma to wrong address after > pci_set_master(). > >>>>>>> > >>>>>>> In this patch, driver will disable tx/rx when close device. If > >>>>>>> wol is eanbled only enable rx filter and disable rxdv_gate to > >>>>>>> let hardware can receive packet to fifo but not to dma it. > >>>>>>> > >>>>>>> Fixes: 120068481405 ("r8169: fix failing WoL") > >>>>>>> Signed-off-by: Chunhao Lin <hau@realtek.com> > >>>>>>> --- > >>>>>>> drivers/net/ethernet/realtek/r8169_main.c | 14 +++++++------- > >>>>>>> 1 file changed, 7 insertions(+), 7 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c > >>>>>>> b/drivers/net/ethernet/realtek/r8169_main.c > >>>>>>> index 1b7fdb4f056b..c09cfbe1d3f0 100644 > >>>>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c > >>>>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c > >>>>>>> @@ -2239,6 +2239,9 @@ static void rtl_wol_enable_rx(struct > >>>>>> rtl8169_private *tp) > >>>>>>> if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > >>>>>>> RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > >>>>>>> AcceptBroadcast | AcceptMulticast | > AcceptMyPhys); > >>>>>>> + > >>>>>>> + if (tp->mac_version >= RTL_GIGA_MAC_VER_40) > >>>>>>> + RTL_W32(tp, MISC, RTL_R32(tp, MISC) & > >> ~RXDV_GATED_EN); > >>>>>> > >>>>>> Is this correct anyway? Supposedly you want to set this bit to > >>>>>> disable > >> DMA. > >>>>>> > >>>>> If wol is enabled, driver need to disable hardware rxdv_gate for > >>>>> receiving > >>>> packets. > >>>>> > >>>> OK, I see. But why disable it here? I see no scenario where > >>>> rxdv_gate would be enabled when we get here. > >>>> > >>> rxdv_gate will be enabled in rtl8169_cleanup(). When suspend or > >>> close and wol is enabled driver will call rtl8169_down() -> > >>> rtl8169_cleanup()-> > >> rtl_prepare_power_down()-> rtl_wol_enable_rx(). > >>> So disabled rxdv_gate in rtl_wol_enable_rx() for receiving packets. > >>> > >> rtl8169_cleanup() skips the call to rtl_enable_rxdvgate() when being > >> called from > >> rtl8169_down() and wol is enabled. This means rxdv gate is still disabled. > >> > > Yes, it will keep rxdv_gate disable. But it will also keep tx/rx on. > > If OS have an unexpected reboot hardware may dma to invalid memory > > address. If possible I prefer to keep tx/rx off when exit driver control. > > > > When you say "keep tx/rx off", do you refer to the rxconfig bits in register > RxConfig, or to CmdTxEnb and CmdRxEnb in ChipCmd? > > If we talk about the first option, then my guess would be: > According to rtl_wol_enable_rx() the rx config bits are required for WoL to > work on certain chip versions. With the introduction of rxdvgate this changed > and setting these bits isn't needed any longer. > I tested on RTL8168h and WoL worked w/o the Accept bits set in RxConfig. > Please confirm or correct my understanding. > > static void rtl_wol_enable_rx(struct rtl8169_private *tp) { > if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > AcceptBroadcast | AcceptMulticast | > AcceptMyPhys); } > > We expect wol packet should be filtered by Accept bits set in RxConfig. But for magic packet wakeup it seems it does not perform as expected. We need some time to figure this out. Once we have any update we will let you know. ------Please consider the environment before printing this e-mail.
> > On 17.10.2022 19:23, Hau wrote: > >> On 13.10.2022 08:04, Hau wrote: > >>>> On 12.10.2022 09:59, Hau wrote: > >>>>>> > >>>>>> On 04.10.2022 10:10, Chunhao Lin wrote: > >>>>>>> When close device, rx will be enabled if wol is enabeld. When > >>>>>>> open device it will cause rx to dma to wrong address after > pci_set_master(). > >>>>>>> > >>>>>>> In this patch, driver will disable tx/rx when close device. If > >>>>>>> wol is eanbled only enable rx filter and disable rxdv_gate to > >>>>>>> let hardware can receive packet to fifo but not to dma it. > >>>>>>> > >>>>>>> Fixes: 120068481405 ("r8169: fix failing WoL") > >>>>>>> Signed-off-by: Chunhao Lin <hau@realtek.com> > >>>>>>> --- > >>>>>>> drivers/net/ethernet/realtek/r8169_main.c | 14 +++++++------- > >>>>>>> 1 file changed, 7 insertions(+), 7 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c > >>>>>>> b/drivers/net/ethernet/realtek/r8169_main.c > >>>>>>> index 1b7fdb4f056b..c09cfbe1d3f0 100644 > >>>>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c > >>>>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c > >>>>>>> @@ -2239,6 +2239,9 @@ static void rtl_wol_enable_rx(struct > >>>>>> rtl8169_private *tp) > >>>>>>> if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > >>>>>>> RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > >>>>>>> AcceptBroadcast | AcceptMulticast | > AcceptMyPhys); > >>>>>>> + > >>>>>>> + if (tp->mac_version >= RTL_GIGA_MAC_VER_40) > >>>>>>> + RTL_W32(tp, MISC, RTL_R32(tp, MISC) & > >> ~RXDV_GATED_EN); > >>>>>> > >>>>>> Is this correct anyway? Supposedly you want to set this bit to > >>>>>> disable > >> DMA. > >>>>>> > >>>>> If wol is enabled, driver need to disable hardware rxdv_gate for > >>>>> receiving > >>>> packets. > >>>>> > >>>> OK, I see. But why disable it here? I see no scenario where > >>>> rxdv_gate would be enabled when we get here. > >>>> > >>> rxdv_gate will be enabled in rtl8169_cleanup(). When suspend or > >>> close and wol is enabled driver will call rtl8169_down() -> > >>> rtl8169_cleanup()-> > >> rtl_prepare_power_down()-> rtl_wol_enable_rx(). > >>> So disabled rxdv_gate in rtl_wol_enable_rx() for receiving packets. > >>> > >> rtl8169_cleanup() skips the call to rtl_enable_rxdvgate() when being > >> called from > >> rtl8169_down() and wol is enabled. This means rxdv gate is still disabled. > >> > > Yes, it will keep rxdv_gate disable. But it will also keep tx/rx on. > > If OS have an unexpected reboot hardware may dma to invalid memory > > address. If possible I prefer to keep tx/rx off when exit driver control. > > > > When you say "keep tx/rx off", do you refer to the rxconfig bits in register > RxConfig, or to CmdTxEnb and CmdRxEnb in ChipCmd? > > If we talk about the first option, then my guess would be: > According to rtl_wol_enable_rx() the rx config bits are required for WoL to > work on certain chip versions. With the introduction of rxdvgate this changed > and setting these bits isn't needed any longer. > I tested on RTL8168h and WoL worked w/o the Accept bits set in RxConfig. > Please confirm or correct my understanding. > > static void rtl_wol_enable_rx(struct rtl8169_private *tp) { > if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > AcceptBroadcast | AcceptMulticast | > AcceptMyPhys); } > > When enter d3cold hardware will pull pcie reset to low. If pcie reset is pulled low hardware will set rcr acpt_phy/mar/brd . That is why you still get wol worked w/o set rcr. Although hardware will set rcr bits when pcie reset is pull low. But there is no pcie reset when hardware enter d3hot. So driver still needs to set rcr bits when hardware go to d3 state. ------Please consider the environment before printing this e-mail.
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 1b7fdb4f056b..c09cfbe1d3f0 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -2239,6 +2239,9 @@ static void rtl_wol_enable_rx(struct rtl8169_private *tp) if (tp->mac_version >= RTL_GIGA_MAC_VER_25) RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | AcceptBroadcast | AcceptMulticast | AcceptMyPhys); + + if (tp->mac_version >= RTL_GIGA_MAC_VER_40) + RTL_W32(tp, MISC, RTL_R32(tp, MISC) & ~RXDV_GATED_EN); } static void rtl_prepare_power_down(struct rtl8169_private *tp) @@ -3981,7 +3984,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp) netdev_reset_queue(tp->dev); } -static void rtl8169_cleanup(struct rtl8169_private *tp, bool going_down) +static void rtl8169_cleanup(struct rtl8169_private *tp) { napi_disable(&tp->napi); @@ -3993,9 +3996,6 @@ static void rtl8169_cleanup(struct rtl8169_private *tp, bool going_down) rtl_rx_close(tp); - if (going_down && tp->dev->wol_enabled) - goto no_reset; - switch (tp->mac_version) { case RTL_GIGA_MAC_VER_28: case RTL_GIGA_MAC_VER_31: @@ -4016,7 +4016,7 @@ static void rtl8169_cleanup(struct rtl8169_private *tp, bool going_down) } rtl_hw_reset(tp); -no_reset: + rtl8169_tx_clear(tp); rtl8169_init_ring_indexes(tp); } @@ -4027,7 +4027,7 @@ static void rtl_reset_work(struct rtl8169_private *tp) netif_stop_queue(tp->dev); - rtl8169_cleanup(tp, false); + rtl8169_cleanup(tp); for (i = 0; i < NUM_RX_DESC; i++) rtl8169_mark_to_asic(tp->RxDescArray + i); @@ -4715,7 +4715,7 @@ static void rtl8169_down(struct rtl8169_private *tp) pci_clear_master(tp->pci_dev); rtl_pci_commit(tp); - rtl8169_cleanup(tp, true); + rtl8169_cleanup(tp); rtl_disable_exit_l1(tp); rtl_prepare_power_down(tp); }
When close device, rx will be enabled if wol is enabeld. When open device it will cause rx to dma to wrong address after pci_set_master(). In this patch, driver will disable tx/rx when close device. If wol is eanbled only enable rx filter and disable rxdv_gate to let hardware can receive packet to fifo but not to dma it. Fixes: 120068481405 ("r8169: fix failing WoL") Signed-off-by: Chunhao Lin <hau@realtek.com> --- drivers/net/ethernet/realtek/r8169_main.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)