RE: [PATCH net v2] r8169: fix rtl8125b dmar pte write access not set error

[Hau <hau@realtek.com>]

> 
> On 23.12.2022 08:43, Chunhao Lin wrote:
> > When close device, if wol is enabled, rx will be enabled. When open
> > device it will cause rx to dma to the wrong memory address after
> pci_set_master().
> > System log will show blow messages.
> >
> > DMAR: DRHD: handling fault status reg 3
> > DMAR: [DMA Write] Request device [02:00.0] PASID ffffffff fault addr
> > ffdd4000 [fault reason 05] PTE Write access is not set
> >
> > In this patch, driver disable tx/rx when close device. If wol is
> > enabled, only enable rx filter and disable rxdv_gate to let hardware
> > only receive packet to fifo but not to dma it.
> >
> > Signed-off-by: Chunhao Lin <hau@realtek.com>
> > ---
> > v1 -> v2: update commit message and adjust the code according to current
> kernel code.
> >
> >  drivers/net/ethernet/realtek/r8169_main.c | 58
> > +++++++++++------------
> >  1 file changed, 29 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> > b/drivers/net/ethernet/realtek/r8169_main.c
> > index a9dcc98b6af1..24592d972523 100644
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > @@ -2210,28 +2210,6 @@ static int rtl_set_mac_address(struct net_device
> *dev, void *p)
> >  	return 0;
> >  }
> >
> > -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);
> > -}
> > -
> > -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
> > -	if (tp->dash_type != RTL_DASH_NONE)
> > -		return;
> > -
> > -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> > -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> > -		rtl_ephy_write(tp, 0x19, 0xff64);
> > -
> > -	if (device_may_wakeup(tp_to_dev(tp))) {
> > -		phy_speed_down(tp->phydev, false);
> > -		rtl_wol_enable_rx(tp);
> > -	}
> > -}
> > -
> >  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
> >  	switch (tp->mac_version) {
> > @@ -2455,6 +2433,31 @@ static void rtl_enable_rxdvgate(struct
> rtl8169_private *tp)
> >  	rtl_wait_txrx_fifo_empty(tp);
> >  }
> >
> > +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_disable_rxdvgate(tp);
> > +}
> > +
> > +static void rtl_prepare_power_down(struct rtl8169_private *tp) {
> > +	if (tp->dash_type != RTL_DASH_NONE)
> > +		return;
> > +
> > +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> > +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> > +		rtl_ephy_write(tp, 0x19, 0xff64);
> > +
> > +	if (device_may_wakeup(tp_to_dev(tp))) {
> > +		phy_speed_down(tp->phydev, false);
> > +		rtl_wol_enable_rx(tp);
> > +	}
> > +}
> > +
> >  static void rtl_set_tx_config_registers(struct rtl8169_private *tp)
> > {
> >  	u32 val = TX_DMA_BURST << TxDMAShift | @@ -3872,7 +3875,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);
> >
> > @@ -3884,9 +3887,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:
> > @@ -3907,7 +3907,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);
> >  }
> > @@ -3918,7 +3918,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); @@ -4605,7
> +4605,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);
> >  }
> 
> The change affects also chip versions other than RTL8125B.
> Is the problem you're fixing really specific to RTL8125B?
> Or could it happen also with other chip versions?
>
All chips that supported by r8169 will be affect by this patch.

> Either patch or commit message need to be changed.
> And it would be better to split the patch:
> 1. Moving rtl_wol_enable_rx() and rtl_prepare_power_down() in the code 2.
> Actual change This makes it easier to track the actual change.
>
I will follow your suggestion and submit this patch again.
 
> ------Please consider the environment before printing this e-mail.