RE: [PATCH net-next] r8169: add module param for control of ASPM disable

[Hau]

> -Original Message-
> From: Francois Romieu [mailto:rom...@fr.zoreil.com]
> Sent: Friday, February 2, 2018 7:27 AM
> To: Hau <h...@realtek.com>
> Cc: netdev@vger.kernel.org; nic_swsd <nic_s...@realtek.com>; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH net-next] r8169: add module param for control of ASPM
> disable
> 
> Chunhao Lin <h...@realtek.com> :
> [...]
> > @@ -5878,6 +5881,20 @@ static void rtl_pcie_state_l2l3_enable(struct
> rtl8169_private *tp, bool enable)
> > RTL_W8(Config3, data);
> >  }
> >
> > +static void rtl_hw_internal_aspm_clkreq_enable(struct rtl8169_private
> *tp,
> > +   bool enable)
> > +{
> > +   void __iomem *ioaddr = tp->mmio_addr;
> > +
> > +   if (enable) {
> > +   RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
> > +   RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
> > +   } else {
> > +   RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
> > +   RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en);
> > +   }
> > +}
> 
> s/enable(..., false)/disable()/
> 
> static void rtl_hw_internal_aspm_clkreq_enable(truct rtl8169_private *tp) {
>   void __iomem *ioaddr = tp->mmio_addr;
> 
>   RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
>   RTL_W8(Config5, RTL_R8(Config5) | ASPM_en); }
> 
> static void rtl_hw_internal_aspm_clkreq_disable(truct rtl8169_private *tp) {
>   void __iomem *ioaddr = tp->mmio_addr;
> 
>   RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
>   RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en); }
> 
> If you really want to factor something out, you may use helpers that set or
> clear bits according to the 3-uple (tp, register, bits) but foo_enable(..., 
> false)
> is pointlessly convoluted.
> 
> --
> Ueimor
> 
> --Please consider the environment before printing this e-mail.

This two bits are related to hardware ASPM function, so I put them together 
into one function.
I could use helper to set these bits, if doing this can make the patch more 
readable for kernel team.

Thanks for inform me that net-next is closed. I will resubmit this patch when 
net-next open.

Thanks.

--Please consider the environment before printing this e-mail.

RE: r8169 take too long to complete driver initialization

[Hau]

> -Original Message-
> From: Chris Chiu [mailto:c...@endlessm.com]
> Sent: Friday, February 2, 2018 10:03 AM
> To: Hau <h...@realtek.com>
> Cc: nic_swsd <nic_s...@realtek.com>; netdev@vger.kernel.org; Linux
> Kernel <linux-ker...@vger.kernel.org>; Linux Upstreaming Team
> <li...@endlessm.com>
> Subject: Re: r8169 take too long to complete driver initialization
> 
> On Tue, Jan 30, 2018 at 8:07 PM, Chris Chiu <c...@endlessm.com> wrote:
> > On Mon, Jan 29, 2018 at 11:24 PM, Hau <h...@realtek.com> wrote:
> >> Hi Chris,
> >>
> >> Could you test following patch?
> >>
> >>  DECLARE_RTL_COND(rtl_ocp_tx_cond)
> >>  {
> >> void __iomem *ioaddr = tp->mmio_addr;
> >>
> >> -   return RTL_R8(IBISR0) & 0x02;
> >> +   return RTL_R8(IBISR0) & 0x20;
> >>  }
> >>
> >>  static void rtl8168ep_stop_cmac(struct rtl8169_private *tp)  {
> >> void __iomem *ioaddr = tp->mmio_addr;
> >>
> >> RTL_W8(IBCR2, RTL_R8(IBCR2) & ~0x01);
> >> -   rtl_msleep_loop_wait_low(tp, _ocp_tx_cond, 50, 2000);
> >> +   rtl_msleep_loop_wait_high(tp, _ocp_tx_cond, 50, 2000);
> >> RTL_W8(IBISR0, RTL_R8(IBISR0) | 0x20);
> >> RTL_W8(IBCR0, RTL_R8(IBCR0) & ~0x01);  }
> >>
> >> Thanks.
> >>
> >
> > Yes. It completes the initialization in 70 ms. So it means the
> > rtl_ocp_tx_cond are waiting for incorrect register bit? Can you help work
> out a patch for this?
> >
> > Chris
> >
> >
> 
> Gentle ping,
> cheers.
> 
> Chris
> 

I have submitted the patch to kernel team.
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=086ca23d03c0d2f4088f472386778d293e15c5f6


--Please consider the environment before printing this e-mail.

> >>> -Original Message-
> >>> From: Chris Chiu [mailto:c...@endlessm.com]
> >>> Sent: Monday, January 29, 2018 6:12 PM
> >>> To: nic_swsd <nic_s...@realtek.com>; netdev@vger.kernel.org; Linux
> >>> Kernel <linux-ker...@vger.kernel.org>; Linux Upstreaming Team
> >>> <li...@endlessm.com>
> >>> Subject: Re: r8169 take too long to complete driver initialization
> >>>
> >>> On Fri, Jan 5, 2018 at 10:17 AM, Chris Chiu <c...@endlessm.com> wrote:
> >>> > On Wed, Dec 20, 2017 at 4:41 PM, Chris Chiu <c...@endlessm.com>
> wrote:
> >>> >> Hi,
> >>> >> We've hit a suspend/resume issue on a Acer desktop caused by
> >>> >> r8169 driver. The dmseg
> >>> >>
> https://gist.github.com/mschiu77/b741849b5070281daaead8dfee312d1a
> >>> >> shows it's still in msleep() within a mutex lock.
> >>> >> After looking into the code, it's caused by the
> >>> >> rtl8168ep_stop_cmac() which is waiting 100 seconds for
> >>> >> rtl_ocp_tx_cond. The following dmesg states that the r8169 driver
> >>> >> is loaded.
> >>> >>
> >>> >> [   20.270526] r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
> >>> >>
> >>> >> But it takes > 100 seconds to get the following messages
> >>> >>
> >>> >> [  140.400223] r8169 :02:00.0 (unnamed net_device)
> >>> >> (uninitialized): rtl_ocp_tx_cond == 1 (loop: 2000, delay: 50).
> >>> >> [  140.413294] r8169 :02:00.0 eth0: RTL8168ep/8111ep at
> >>> >> 0xb16c80db1000, f8:0f:41:ea:74:0d, XID 10200800 IRQ 46 [
> >>> >> 140.413297] r8169 :02:00.0 eth0: jumbo features [frames: 9200
> >>> >> bytes, tx checksumming: ko]
> >>> >>
> >>> >> So any trial to suspend the machine during this period would
> >>> >> always get device/resource busy message then abort. Is this
> >>> >> rtl_ocp_tx_cond necessary? Because the ethernet is still working
> >>> >> and I don't see any problem. I don't know it should be considered
> >>> >> normal or not. Please let me know if any more information
> >>> >> required. Thanks
> >>> >>
> >>> >> Chris
> >>> >
> >>> > gentle ping,
> >>> >
> >>> > cheers.
> >>>
> >>> Hi,
> >>> Just found a r8168 driver which seems to be authrized by realtek
> >>> for cross comparison. I tried applying the patch to latest 4.15
> >>> kernel and the driver done it's initialization in faily short time. The 
> >>> patch
> file is here
> >>> https://gist.github.com/mschiu77/fcf406e64a1a437f46cf2be643f1057d.
> >>>
> >>> In mainline r8169.c, the IBISR0 register need to be polled in
> >>> the rtl8168ep_stop_cmac().
> >>> In the patch file, there's also the same IBISR0 polling code in
> >>> Dash2DisableTx(), but it's been bypassed since the chipset maches
> >>> HW_DASH_SUPPORT_TYPE_2.
> >>> Per the rtl_chip_info[] in r8168_n.c, CFG_METHOD_23/27/28 are
> >>> HW_DASH_SUPPORT_TYPE_2, and they happens to be the only 3 named
> >>> RTL8168EP/8111EP in the rtl_chip_info[].
> >>>
> >>> To find the same matches in r8169.c, RTL_GIGA_MAC_VER_49/50/51
> >>> seems share the same config. Can anyone clarify if the
> >>> rtl_ocp_tx_cond() really necessary for 8168EP/8111EP?
> >>> Or we can just ignore the condition check for
> RTL_GIGA_MAC_VER_49/50/51?
> >>>
> >>> Chris
> >>>
> >>> --Please consider the environment before printing this e-mail.

RE: r8169 take too long to complete driver initialization

[Hau]

Hi Chris,

Could you test following patch?

 DECLARE_RTL_COND(rtl_ocp_tx_cond)
 {
void __iomem *ioaddr = tp->mmio_addr;
 
-   return RTL_R8(IBISR0) & 0x02;
+   return RTL_R8(IBISR0) & 0x20;
 }
 
 static void rtl8168ep_stop_cmac(struct rtl8169_private *tp)
 {
void __iomem *ioaddr = tp->mmio_addr;
 
RTL_W8(IBCR2, RTL_R8(IBCR2) & ~0x01);
-   rtl_msleep_loop_wait_low(tp, _ocp_tx_cond, 50, 2000);
+   rtl_msleep_loop_wait_high(tp, _ocp_tx_cond, 50, 2000);
RTL_W8(IBISR0, RTL_R8(IBISR0) | 0x20);
RTL_W8(IBCR0, RTL_R8(IBCR0) & ~0x01);
 }

Thanks.

--Please consider the environment before printing this e-mail.

> -Original Message-
> From: Chris Chiu [mailto:c...@endlessm.com]
> Sent: Monday, January 29, 2018 6:12 PM
> To: nic_swsd ; netdev@vger.kernel.org; Linux
> Kernel ; Linux Upstreaming Team
> 
> Subject: Re: r8169 take too long to complete driver initialization
> 
> On Fri, Jan 5, 2018 at 10:17 AM, Chris Chiu  wrote:
> > On Wed, Dec 20, 2017 at 4:41 PM, Chris Chiu  wrote:
> >> Hi,
> >> We've hit a suspend/resume issue on a Acer desktop caused by
> >> r8169 driver. The dmseg
> >> https://gist.github.com/mschiu77/b741849b5070281daaead8dfee312d1a
> >> shows it's still in msleep() within a mutex lock.
> >> After looking into the code, it's caused by the
> >> rtl8168ep_stop_cmac() which is waiting 100 seconds for
> >> rtl_ocp_tx_cond. The following dmesg states that the r8169 driver is
> >> loaded.
> >>
> >> [   20.270526] r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
> >>
> >> But it takes > 100 seconds to get the following messages
> >>
> >> [  140.400223] r8169 :02:00.0 (unnamed net_device)
> >> (uninitialized): rtl_ocp_tx_cond == 1 (loop: 2000, delay: 50).
> >> [  140.413294] r8169 :02:00.0 eth0: RTL8168ep/8111ep at
> >> 0xb16c80db1000, f8:0f:41:ea:74:0d, XID 10200800 IRQ 46 [
> >> 140.413297] r8169 :02:00.0 eth0: jumbo features [frames: 9200
> >> bytes, tx checksumming: ko]
> >>
> >> So any trial to suspend the machine during this period would always
> >> get device/resource busy message then abort. Is this  rtl_ocp_tx_cond
> >> necessary? Because the ethernet is still working and I don't see any
> >> problem. I don't know it should be considered normal or not. Please
> >> let me know if any more information required. Thanks
> >>
> >> Chris
> >
> > gentle ping,
> >
> > cheers.
> 
> Hi,
> Just found a r8168 driver which seems to be authrized by realtek for cross
> comparison. I tried applying the patch to latest 4.15 kernel and the driver
> done it's initialization in faily short time. The patch file is here
> https://gist.github.com/mschiu77/fcf406e64a1a437f46cf2be643f1057d.
> 
> In mainline r8169.c, the IBISR0 register need to be polled in the
> rtl8168ep_stop_cmac().
> In the patch file, there's also the same IBISR0 polling code in
> Dash2DisableTx(), but it's been bypassed since the chipset maches
> HW_DASH_SUPPORT_TYPE_2.
> Per the rtl_chip_info[] in r8168_n.c, CFG_METHOD_23/27/28 are
> HW_DASH_SUPPORT_TYPE_2, and they happens to be the only 3 named
> RTL8168EP/8111EP in the rtl_chip_info[].
> 
> To find the same matches in r8169.c, RTL_GIGA_MAC_VER_49/50/51
> seems share the same config. Can anyone clarify if the rtl_ocp_tx_cond()
> really necessary for 8168EP/8111EP?
> Or we can just ignore the condition check for RTL_GIGA_MAC_VER_49/50/51?
> 
> Chris
> 
> --Please consider the environment before printing this e-mail.

RE: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.

[Hau]

[...]
> Nit: you may directly use "struct device *d = >pci_dev->dev;"
> 

I will do that on my next version patch.

Thanks.
--Please consider the environment before printing this e-mail.

RE: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.

[Hau]

[...]
> > @@ -1852,12 +1863,17 @@ static int rtl8169_set_wol(struct net_device
> *dev, struct ethtool_wolinfo *wol)
> > tp->features |= RTL_FEATURE_WOL;
> > else
> > tp->features &= ~RTL_FEATURE_WOL;
> > -   __rtl8169_set_wol(tp, wol->wolopts);
> > +   if (pm_runtime_active(>dev))
> > +   __rtl8169_set_wol(tp, wol->wolopts);
> > +   else
> > +   tp->saved_wolopts = wol->wolopts;
> >
> > rtl_unlock_work(tp);
> >
> > device_set_wakeup_enable(>pci_dev->dev, wol->wolopts);
> >
> > +   pm_runtime_put_noidle(>dev);
> > +
> > return 0;
> 
> Either the driver resumes the device so that it can perform requested
> operation or it signals .set_wol failure when the device is suspended.
> 
> If the driver does something else, "spam removal" translates to "silent
> failure".

Because "tp->saved_wolopts" will be used to set hardware wol capability in 
rtl8169_runtime_resume().  So I prefer to keep "wol->wolopts" to " 
tp->saved_wolopts " in runtime suspend state and set this to this 
"wol->wolopts" to hardware in in rtl8169_runtime_resume(). 

Thanks.

--Please consider the environment before printing this e-mail.

RE: [PATCH] r8169: default to 64-bit DMA on systems without memory below 4 GB

[Hau]

> -Original Message-
> From: Francois Romieu [mailto:rom...@fr.zoreil.com]
> Sent: Thursday, May 12, 2016 10:09 PM
> To: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Cc: David Miller <da...@davemloft.net>; netdev@vger.kernel.org; Ricardo
> Salveti <ricardo.salv...@linaro.org>; Leo Duran <leo.du...@amd.com>; G
> Gregory <graeme.greg...@linaro.org>; nic_swsd <nic_s...@realtek.com>;
> Hau <h...@realtek.com>
> Subject: Re: [PATCH] r8169: default to 64-bit DMA on systems without memory
> below 4 GB
> 
> > On 12 May 2016 at 01:58, David Miller <da...@davemloft.net> wrote:
> > > From: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > > Date: Wed, 11 May 2016 09:47:49 +0200
> [...]
> > > I think we should just seriously consider changing the default, it's
> > > a really outdated reasoning behind the current default setting.
> > > Maybe relevant a decade ago, but probably not now.
> > >
> > > And if the card is completely disfunctional in said configuration,
> > > the default is definitely wrong.
> >
> > The card is indeed completely disfunctional. So we could try to
> > resurrect 353176888386 ("r8169: enable 64-bit DMA by default for PCI
> > Express devices"), and instead of backing it out again if regressions
> > are reported, blacklist the particular chips. This is a much better
> > approach, since then we can also print some kind of diagnostic on
> > those arm64 systems why such a blacklisted NIC is not supported.
> 
> I doubt there will be much *reporting* from broken systems that include plain
> old PCI realtek chipsets (r8169.c::RTL_CFG_0). Changing the default for those 
> is
> imnsho asking for troubles without clear benefit (experimental evidence
> suggests that smsc etherpower II grows older more easily than plain pci
> 8169 :o/ ).
> 
> I'd rather leave these alone and change the default for the PCI Express 
> chipsets.
> Btw, while it does not seem to hurt, they should not need any CPlusCmd Dual
> Access Cycle tweak either. Realtek may establish it (Lin ?)

You don't have to set CPlusCmd Dual Access Cycle for the PCI Express chipsets.

--Please consider the environment before printing this e-mail.

RE: [PATCH net] r8169:Remove unnecessary phy reset for pcie nic when setting link spped.

[Hau]

[...] 
> Can you clarify:
> - actually this patch does not care about the link at all. So when there's
>   link no phy reset is needed either, right ?
> - does "this" in "to do this" means that
>   1. phy reset prevents phy from auto speed down
>   2. avoiding phy reset prevents phy from auto speed down
>   I would expect 1. from the rtl_wol_pll_power_down + rtl_speed_down +
>   rtl8169_set_speed combo (i.e. we want the driver to allow auto speed
> down)
>   but it's a bit ambiguous.

Unless pcie nic has bug, pcie nic does not need to reset phy to let phy link on.

There is a counter for phy speed down. If phy is in link down state, this 
counter will start to count down. When it count to 0, phy will speed down. 
Reset phy will reset this counter and prevent phy from speed down.

 --Please consider the environment before printing this e-mail.

RE: [PATCH net 0/3] r8169:issues fix.

[Hau]

> I don't agree with changes #1 and #2.
> 
> If you are going to go to a model where every single configuration operation
> is recorded in software and performed at resume time, then really do it and
> fix it in the whole driver.  As currently coded you are leaving lots of known
> bugs in the driver.
> 
> #2 is even a worse situation.  If you are going to handle things this way you
> must sync the counters when the suspend happens, so that the statistics get
> call receives up to date values.

Thanks for your advice. I will send #3 patch first. Then modify #1 and #2 patch 
and resend these two patches.

 --Please consider the environment before printing this e-mail.

RE: [PATCH net 1/3] r8169:fix nic sometimes doesn't work after changing the mac address.

[Hau]

 > Instead of taking the device out of suspended mode to perform the required
> action, the driver is moving to a model where 1) said action may be
> scheduled to a later time - or result from past time work - and 2) rpm handler
> must handle a lot of pm unrelated work.
> 
> rtl8169_ethtool_ops.{get_wol, get_regs, get_settings} aren't even fixed yet
> (what about the .set_xyz handlers ?).
> 
> I can't help thinking that the driver should return to a state where it 
> stupidly
> does what it is asked to. No software caching, plain device access, resume
> when needed, suspend as "suspend" instead of suspend as "anticipate
> whatever may happen to avoid waking up".
>

This rpm related patches just the workaround for the issues reported by end 
users.  As you say, the Linux kernel should handle these events when driver is 
in runtime suspend state.
 
--Please consider the environment before printing this e-mail.

RE: [PATCH net v4] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.

[Hau]

> Fine with me.
> 
> Is there any chance for the set of chipset dependent registers that are safe 
> to
> be read when in D3 state to be documented ?
> 

I think except registers in PCI configuration, all other registers should be 
read in D0 state.

--Please consider the environment before printing this e-mail.

RE: [PATCH net v2] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.

[Hau]

> Nits:
> 
> - the tp->TxDescArray test provides the required synchronization: see
>   rtl8169_{open/close} and their pm_runtime_{get / put}.
> 
> - ioaddr is not really needed : tp->mmio_addr appears only once and it does
>   not mess the 72..80 cols limit.
> 
> - even if the device can only be automatically runtime suspended some time
>   after a link down event, you may address davem's point regarding stats
>   reliability and move rtl8169_rx_missed + rtl8169_update_counters after
>   rtl8169_net_suspend.

I will submit the new patch according to your advice.

Thanks.

Please consider the environment before printing this e-mail.

RE: [PATCH net] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.

[Hau]

> Chunhao Lin  :
> [...]
> > I add checking driver's pm runtime status in rtl8169_get_stats64() to fix
> > this issue.
> 
> Would you consider taking the device out of suspended mode during
> rtl8169_get_stats64 to prevent outdated stats ?
> 

When in runtime suspend, it mean there is no traffic on NIC, so I did not wake 
the device during rtl8169_get_stats64().
Maybe we can update tally counter before entering runtime suspend mode.

--Please consider the environment before printing this e-mail.