Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190708063751.16234-1-acelan.kao@canonical.com>
 <53f82481-ed41-abc5-2e4e-ac1026617219@gmail.com> <CAFv23Q=mA9t0j2F4fKdOkgG6sao0m7rR_9-d9OvAmSerZf_=ew@mail.gmail.com>
In-Reply-To: <CAFv23Q=mA9t0j2F4fKdOkgG6sao0m7rR_9-d9OvAmSerZf_=ew@mail.gmail.com>
From:   AceLan Kao <acelan.kao@canonical.com>
Date:   Wed, 10 Jul 2019 15:05:38 +0800
Message-ID: <CAFv23QmqjJtUD-iAwzsXg2MCNbe_p1zOcZ7C-ywG5n-iT-N-YA@mail.gmail.com>
Subject: Re: [PATCH] r8169: add enable_aspm parameter
To:     Heiner Kallweit <hkallweit1@gmail.com>
Cc:     Realtek linux nic maintainers <nic_swsd@realtek.com>,
        "David S. Miller" <davem@davemloft.net>, netdev@vger.kernel.org,
        "Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Heiner,

I've tried and verified your PCI ASPM patches and it works well.
I've replied the patch thread and hope this can make it get some progress.

BTW, do you think we can revert commit b75bb8a5b755 ("r8169: disable
ASPM again") once the PCI ASPM patches get merged?

Best regards,
AceLan Kao.

AceLan Kao <acelan.kao@canonical.com> 於 2019年7月9日 週二 上午11:19寫道：
>
> Heiner Kallweit <hkallweit1@gmail.com> 於 2019年7月9日 週二 上午2:27寫道：
> >
> > On 08.07.2019 08:37, AceLan Kao wrote:
> > > We have many commits in the driver which enable and then disable ASPM
> > > function over and over again.
> > >    commit b75bb8a5b755 ("r8169: disable ASPM again")
> > >    commit 0866cd15029b ("r8169: enable ASPM on RTL8106E")
> > >    commit 94235460f9ea ("r8169: Align ASPM/CLKREQ setting function with vendor driver")
> > >    commit aa1e7d2c31ef ("r8169: enable ASPM on RTL8168E-VL")
> > >    commit f37658da21aa ("r8169: align ASPM entry latency setting with vendor driver")
> > >    commit a99790bf5c7f ("r8169: Reinstate ASPM Support")
> > >    commit 671646c151d4 ("r8169: Don't disable ASPM in the driver")
> > >    commit 4521e1a94279 ("Revert "r8169: enable internal ASPM and clock request settings".")
> > >    commit d64ec841517a ("r8169: enable internal ASPM and clock request settings")
> > >
> > > This function is very important for production, and if we can't come out
> > > a solution to make both happy, I'd suggest we add a parameter in the
> > > driver to toggle it.
> > >
> > The usage of a module parameter to control ASPM is discouraged.
> > There have been more such attempts in the past that have been declined.
> >
> > Pending with the PCI maintainers is a series adding ASPM control
> > via sysfs, see here: https://www.spinics.net/lists/linux-pci/msg83228.html
> Cool, I'll try your patches and reply on that thread.
>
> >
> > Also more details than just stating "it's important for production"
> > would have been appreciated in the commit message, e.g. which
> > power-savings you can achieve with ASPM on which systems.
> I should use more specific wordings rather than "important for
> production", thanks.