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;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\))
Subject: Re: [PATCH] igb: Fix WARN_ONCE on runtime suspend
From:   Kai Heng Feng <kai.heng.feng@canonical.com>
In-Reply-To: <20190302160115.GA6172@rani.riverdale.lan>
Date:   Mon, 4 Mar 2019 16:31:45 +0800
Cc:     intel-wired-lan@lists.osuosl.org,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-pm@vger.kernel.org
Content-Transfer-Encoding: 8BIT
Message-Id: <E8841527-C294-4E2D-BB19-3C0B1F1EBBC7@canonical.com>
References: <20190302160115.GA6172@rani.riverdale.lan>
To:     Arvind Sankar <niveditas98@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Mar 3, 2019, at 12:01 AM, Arvind Sankar <niveditas98@gmail.com> wrote:
> 
> The runtime_suspend device callbacks are not supposed to save
> configuration state or change the power state. Commit fb29f76cc566
> ("igb: Fix an issue that PME is not enabled during runtime suspend")
> changed the driver to not save configuration state during runtime
> suspend, however the driver callback still put the device into a
> low-power state. This causes a warning in the pci pm core and results in
> pci_pm_runtime_suspend not calling pci_save_state or pci_finish_runtime_suspend.
> 
> Fix this by not changing the power state either, leaving that to pci pm
> core, and make the same change for suspend callback as well.
> 
> Also move a couple of defines into the appropriate header file instead
> of inline in the .c file.

Header changes can be a separate patch, but that’s just a small nit.

> 
> Fixes: fb29f76cc566 ("igb: Fix an issue that PME is not enabled during runtime suspend")
> Signed-off-by: Arvind Sankar <niveditas98@gmail.com>

Reviewed-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

> ---
> .../net/ethernet/intel/igb/e1000_defines.h    |  2 +
> drivers/net/ethernet/intel/igb/igb_main.c     | 57 +++----------------
> 2 files changed, 10 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
> index 8a28f3388f69..dca671591ef6 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_defines.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
> @@ -194,6 +194,8 @@
> /* enable link status from external LINK_0 and LINK_1 pins */
> #define E1000_CTRL_SWDPIN0  0x00040000  /* SWDPIN 0 value */
> #define E1000_CTRL_SWDPIN1  0x00080000  /* SWDPIN 1 value */
> +#define E1000_CTRL_ADVD3WUC 0x00100000  /* D3 WUC */
> +#define E1000_CTRL_EN_PHY_PWR_MGMT 0x00200000 /* PHY PM enable */
> #define E1000_CTRL_SDP0_DIR 0x00400000  /* SDP0 Data direction */
> #define E1000_CTRL_SDP1_DIR 0x00800000  /* SDP1 Data direction */
> #define E1000_CTRL_RST      0x04000000  /* Global reset */
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 93f150784cfc..c2b3a2ff1471 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -8754,9 +8754,7 @@ static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
> 	struct e1000_hw *hw = &adapter->hw;
> 	u32 ctrl, rctl, status;
> 	u32 wufc = runtime ? E1000_WUFC_LNKC : adapter->wol;
> -#ifdef CONFIG_PM
> -	int retval = 0;
> -#endif
> +	bool wake;
> 
> 	rtnl_lock();
> 	netif_device_detach(netdev);
> @@ -8769,14 +8767,6 @@ static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
> 	igb_clear_interrupt_scheme(adapter);
> 	rtnl_unlock();
> 
> -#ifdef CONFIG_PM
> -	if (!runtime) {
> -		retval = pci_save_state(pdev);
> -		if (retval)
> -			return retval;
> -	}
> -#endif
> -
> 	status = rd32(E1000_STATUS);
> 	if (status & E1000_STATUS_LU)
> 		wufc &= ~E1000_WUFC_LNKC;
> @@ -8793,10 +8783,6 @@ static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
> 		}
> 
> 		ctrl = rd32(E1000_CTRL);
> -		/* advertise wake from D3Cold */
> -		#define E1000_CTRL_ADVD3WUC 0x00100000
> -		/* phy power management enable */
> -		#define E1000_CTRL_EN_PHY_PWR_MGMT 0x00200000
> 		ctrl |= E1000_CTRL_ADVD3WUC;
> 		wr32(E1000_CTRL, ctrl);
> 
> @@ -8810,12 +8796,15 @@ static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
> 		wr32(E1000_WUFC, 0);
> 	}
> 
> -	*enable_wake = wufc || adapter->en_mng_pt;
> -	if (!*enable_wake)
> +	wake = wufc || adapter->en_mng_pt;
> +	if (!wake)
> 		igb_power_down_link(adapter);
> 	else
> 		igb_power_up_link(adapter);
> 
> +	if (enable_wake)
> +		*enable_wake = wake;
> +
> 	/* Release control of h/w to f/w.  If f/w is AMT enabled, this
> 	 * would have already happened in close and is redundant.
> 	 */
> @@ -8858,22 +8847,7 @@ static void igb_deliver_wake_packet(struct net_device *netdev)
> 
> static int __maybe_unused igb_suspend(struct device *dev)
> {
> -	int retval;
> -	bool wake;
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -
> -	retval = __igb_shutdown(pdev, &wake, 0);
> -	if (retval)
> -		return retval;
> -
> -	if (wake) {
> -		pci_prepare_to_sleep(pdev);
> -	} else {
> -		pci_wake_from_d3(pdev, false);
> -		pci_set_power_state(pdev, PCI_D3hot);
> -	}
> -
> -	return 0;
> +	return __igb_shutdown(to_pci_dev(dev), NULL, 0);
> }
> 
> static int __maybe_unused igb_resume(struct device *dev)
> @@ -8944,22 +8918,7 @@ static int __maybe_unused igb_runtime_idle(struct device *dev)
> 
> static int __maybe_unused igb_runtime_suspend(struct device *dev)
> {
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	int retval;
> -	bool wake;
> -
> -	retval = __igb_shutdown(pdev, &wake, 1);
> -	if (retval)
> -		return retval;
> -
> -	if (wake) {
> -		pci_prepare_to_sleep(pdev);
> -	} else {
> -		pci_wake_from_d3(pdev, false);
> -		pci_set_power_state(pdev, PCI_D3hot);
> -	}
> -
> -	return 0;
> +	return __igb_shutdown(to_pci_dev(dev), NULL, 1);
> }
> 
> static int __maybe_unused igb_runtime_resume(struct device *dev)
> -- 
> 2.19.2
>