diff mbox series

[RFC,net-next,v5,2/3] r8169: Use PCIe ASPM status for NIC ASPM enablement

Message ID 20210916154417.664323-3-kai.heng.feng@canonical.com
State New
Headers show
Series r8169: Implement dynamic ASPM mechanism for recent 1.0/2.5Gbps Realtek NICs | expand

Commit Message

Kai-Heng Feng Sept. 16, 2021, 3:44 p.m. UTC 
Because ASPM control may not be granted by BIOS while ASPM is enabled,
and ASPM can be enabled via sysfs, so use pcie_aspm_enabled() directly
to check current ASPM enable status.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v5:
 - New patch.

 drivers/net/ethernet/realtek/r8169_main.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Bjorn Helgaas Sept. 16, 2021, 5:07 p.m. UTC | #1 
On Thu, Sep 16, 2021 at 11:44:16PM +0800, Kai-Heng Feng wrote:
> Because ASPM control may not be granted by BIOS while ASPM is enabled,
> and ASPM can be enabled via sysfs, so use pcie_aspm_enabled() directly
> to check current ASPM enable status.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v5:
>  - New patch.
> 
>  drivers/net/ethernet/realtek/r8169_main.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 0199914440abc..6f1a9bec40c05 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -622,7 +622,6 @@ struct rtl8169_private {
>  	} wk;
>  
>  	unsigned supports_gmii:1;
> -	unsigned aspm_manageable:1;
>  	dma_addr_t counters_phys_addr;
>  	struct rtl8169_counters *counters;
>  	struct rtl8169_tc_offsets tc_offset;
> @@ -2664,8 +2663,13 @@ static void rtl_enable_exit_l1(struct rtl8169_private *tp)
>  
>  static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
>  {
> -	/* Don't enable ASPM in the chip if OS can't control ASPM */
> -	if (enable && tp->aspm_manageable) {
> +	struct pci_dev *pdev = tp->pci_dev;
> +
> +	/* Don't enable ASPM in the chip if PCIe ASPM isn't enabled */
> +	if (!pcie_aspm_enabled(pdev) && enable)
> +		return;

What happens when the user enables or disables ASPM via sysfs (see
https://git.kernel.org/linus/72ea91afbfb0)?

The driver is not going to know about that change.

> +	if (enable) {
>  		RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
>  		RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
>  	} else {
> @@ -5272,8 +5276,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	/* Disable ASPM L1 as that cause random device stop working
>  	 * problems as well as full system hangs for some PCIe devices users.
>  	 */
> -	rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> -	tp->aspm_manageable = !rc;
> +	pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
>  
>  	/* enable device (incl. PCI PM wakeup and hotplug setup) */
>  	rc = pcim_enable_device(pdev);
> -- 
> 2.32.0
>
Kai-Heng Feng Sept. 17, 2021, 4:09 a.m. UTC | #2 
On Fri, Sep 17, 2021 at 1:07 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Sep 16, 2021 at 11:44:16PM +0800, Kai-Heng Feng wrote:
> > Because ASPM control may not be granted by BIOS while ASPM is enabled,
> > and ASPM can be enabled via sysfs, so use pcie_aspm_enabled() directly
> > to check current ASPM enable status.
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> > v5:
> >  - New patch.
> >
> >  drivers/net/ethernet/realtek/r8169_main.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> > index 0199914440abc..6f1a9bec40c05 100644
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > @@ -622,7 +622,6 @@ struct rtl8169_private {
> >       } wk;
> >
> >       unsigned supports_gmii:1;
> > -     unsigned aspm_manageable:1;
> >       dma_addr_t counters_phys_addr;
> >       struct rtl8169_counters *counters;
> >       struct rtl8169_tc_offsets tc_offset;
> > @@ -2664,8 +2663,13 @@ static void rtl_enable_exit_l1(struct rtl8169_private *tp)
> >
> >  static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
> >  {
> > -     /* Don't enable ASPM in the chip if OS can't control ASPM */
> > -     if (enable && tp->aspm_manageable) {
> > +     struct pci_dev *pdev = tp->pci_dev;
> > +
> > +     /* Don't enable ASPM in the chip if PCIe ASPM isn't enabled */
> > +     if (!pcie_aspm_enabled(pdev) && enable)
> > +             return;
>
> What happens when the user enables or disables ASPM via sysfs (see
> https://git.kernel.org/linus/72ea91afbfb0)?
>
> The driver is not going to know about that change.

So it's still better to fold this patch into next one? So the periodic
delayed_work can toggle ASPM accordingly.

Kai-Heng

>
> > +     if (enable) {
> >               RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
> >               RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
> >       } else {
> > @@ -5272,8 +5276,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> >       /* Disable ASPM L1 as that cause random device stop working
> >        * problems as well as full system hangs for some PCIe devices users.
> >        */
> > -     rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> > -     tp->aspm_manageable = !rc;
> > +     pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> >
> >       /* enable device (incl. PCI PM wakeup and hotplug setup) */
> >       rc = pcim_enable_device(pdev);
> > --
> > 2.32.0
> >
Bjorn Helgaas Sept. 17, 2021, 3:26 p.m. UTC | #3 
On Fri, Sep 17, 2021 at 12:09:08PM +0800, Kai-Heng Feng wrote:
> On Fri, Sep 17, 2021 at 1:07 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Sep 16, 2021 at 11:44:16PM +0800, Kai-Heng Feng wrote:
> > > Because ASPM control may not be granted by BIOS while ASPM is enabled,
> > > and ASPM can be enabled via sysfs, so use pcie_aspm_enabled() directly
> > > to check current ASPM enable status.
> > >
> > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > ---
> > > v5:
> > >  - New patch.
> > >
> > >  drivers/net/ethernet/realtek/r8169_main.c | 13 ++++++++-----
> > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> > > index 0199914440abc..6f1a9bec40c05 100644
> > > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > > @@ -622,7 +622,6 @@ struct rtl8169_private {
> > >       } wk;
> > >
> > >       unsigned supports_gmii:1;
> > > -     unsigned aspm_manageable:1;
> > >       dma_addr_t counters_phys_addr;
> > >       struct rtl8169_counters *counters;
> > >       struct rtl8169_tc_offsets tc_offset;
> > > @@ -2664,8 +2663,13 @@ static void rtl_enable_exit_l1(struct rtl8169_private *tp)
> > >
> > >  static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
> > >  {
> > > -     /* Don't enable ASPM in the chip if OS can't control ASPM */
> > > -     if (enable && tp->aspm_manageable) {
> > > +     struct pci_dev *pdev = tp->pci_dev;
> > > +
> > > +     /* Don't enable ASPM in the chip if PCIe ASPM isn't enabled */
> > > +     if (!pcie_aspm_enabled(pdev) && enable)
> > > +             return;
> >
> > What happens when the user enables or disables ASPM via sysfs (see
> > https://git.kernel.org/linus/72ea91afbfb0)?
> >
> > The driver is not going to know about that change.
> 
> So it's still better to fold this patch into next one? So the periodic
> delayed_work can toggle ASPM accordingly.

No, my point is that the user can enable/disable ASPM via sysfs, and
the driver will not know anything about it.  There's no callback that
tells the driver when this happens.

My question is whether this code works when that happens.  I doubt it
works, because if ASPM is not enabled at this moment, you return
without doing enabling ASPM in the chip below.

If the user subsequently enables ASPM via sysfs, the chip setup below
will not be done.

If there's chip-specific setup to make ASPM work, I think the
chip-specific part needs to be done unconditionally.

> > > +     if (enable) {
> > >               RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
> > >               RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
> > >       } else {
> > > @@ -5272,8 +5276,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> > >       /* Disable ASPM L1 as that cause random device stop working
> > >        * problems as well as full system hangs for some PCIe devices users.
> > >        */
> > > -     rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> > > -     tp->aspm_manageable = !rc;
> > > +     pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> > >
> > >       /* enable device (incl. PCI PM wakeup and hotplug setup) */
> > >       rc = pcim_enable_device(pdev);
> > > --
> > > 2.32.0
> > >
Kai-Heng Feng Oct. 1, 2021, 4:32 a.m. UTC | #4 
On Fri, Sep 17, 2021 at 11:26 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Sep 17, 2021 at 12:09:08PM +0800, Kai-Heng Feng wrote:
> > On Fri, Sep 17, 2021 at 1:07 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Thu, Sep 16, 2021 at 11:44:16PM +0800, Kai-Heng Feng wrote:
> > > > Because ASPM control may not be granted by BIOS while ASPM is enabled,
> > > > and ASPM can be enabled via sysfs, so use pcie_aspm_enabled() directly
> > > > to check current ASPM enable status.
> > > >
> > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > > ---
> > > > v5:
> > > >  - New patch.
> > > >
> > > >  drivers/net/ethernet/realtek/r8169_main.c | 13 ++++++++-----
> > > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> > > > index 0199914440abc..6f1a9bec40c05 100644
> > > > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > > > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > > > @@ -622,7 +622,6 @@ struct rtl8169_private {
> > > >       } wk;
> > > >
> > > >       unsigned supports_gmii:1;
> > > > -     unsigned aspm_manageable:1;
> > > >       dma_addr_t counters_phys_addr;
> > > >       struct rtl8169_counters *counters;
> > > >       struct rtl8169_tc_offsets tc_offset;
> > > > @@ -2664,8 +2663,13 @@ static void rtl_enable_exit_l1(struct rtl8169_private *tp)
> > > >
> > > >  static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
> > > >  {
> > > > -     /* Don't enable ASPM in the chip if OS can't control ASPM */
> > > > -     if (enable && tp->aspm_manageable) {
> > > > +     struct pci_dev *pdev = tp->pci_dev;
> > > > +
> > > > +     /* Don't enable ASPM in the chip if PCIe ASPM isn't enabled */
> > > > +     if (!pcie_aspm_enabled(pdev) && enable)
> > > > +             return;
> > >
> > > What happens when the user enables or disables ASPM via sysfs (see
> > > https://git.kernel.org/linus/72ea91afbfb0)?
> > >
> > > The driver is not going to know about that change.
> >
> > So it's still better to fold this patch into next one? So the periodic
> > delayed_work can toggle ASPM accordingly.
>
> No, my point is that the user can enable/disable ASPM via sysfs, and
> the driver will not know anything about it.  There's no callback that
> tells the driver when this happens.
>
> My question is whether this code works when that happens.  I doubt it
> works, because if ASPM is not enabled at this moment, you return
> without doing enabling ASPM in the chip below.
>
> If the user subsequently enables ASPM via sysfs, the chip setup below
> will not be done.
>
> If there's chip-specific setup to make ASPM work, I think the
> chip-specific part needs to be done unconditionally.

So it's either adding a callback to notify driver about ASPM change,
or doing chip-specific ASPM unconditionally.
Which one do you prefer?

Kai-Heng

>
> > > > +     if (enable) {
> > > >               RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
> > > >               RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
> > > >       } else {
> > > > @@ -5272,8 +5276,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> > > >       /* Disable ASPM L1 as that cause random device stop working
> > > >        * problems as well as full system hangs for some PCIe devices users.
> > > >        */
> > > > -     rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> > > > -     tp->aspm_manageable = !rc;
> > > > +     pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> > > >
> > > >       /* enable device (incl. PCI PM wakeup and hotplug setup) */
> > > >       rc = pcim_enable_device(pdev);
> > > > --
> > > > 2.32.0
> > > >
diff mbox series

Patch

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 0199914440abc..6f1a9bec40c05 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -622,7 +622,6 @@  struct rtl8169_private {
 	} wk;
 
 	unsigned supports_gmii:1;
-	unsigned aspm_manageable:1;
 	dma_addr_t counters_phys_addr;
 	struct rtl8169_counters *counters;
 	struct rtl8169_tc_offsets tc_offset;
@@ -2664,8 +2663,13 @@  static void rtl_enable_exit_l1(struct rtl8169_private *tp)
 
 static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
 {
-	/* Don't enable ASPM in the chip if OS can't control ASPM */
-	if (enable && tp->aspm_manageable) {
+	struct pci_dev *pdev = tp->pci_dev;
+
+	/* Don't enable ASPM in the chip if PCIe ASPM isn't enabled */
+	if (!pcie_aspm_enabled(pdev) && enable)
+		return;
+
+	if (enable) {
 		RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
 		RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
 	} else {
@@ -5272,8 +5276,7 @@  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* Disable ASPM L1 as that cause random device stop working
 	 * problems as well as full system hangs for some PCIe devices users.
 	 */
-	rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
-	tp->aspm_manageable = !rc;
+	pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
 
 	/* enable device (incl. PCI PM wakeup and hotplug setup) */
 	rc = pcim_enable_device(pdev);