[PATCH iwl-next v2 0/2] net: intel: cleanup power ops

[PATCH iwl-next v2 0/2] net: intel: cleanup power ops

[Jesse Brandeburg]

Do a quick refactor of igb to clean up some unnecessary declarations,
noticed while doing the real work of 2/2.

Follow that with a change of all the Intel drivers to use the current
power management declaration APIs, to avoid complication and maintenance
issues with CONFIG_PM=<m|y|n>. This is as per [1]

Mostly compile-tested only, the ice driver currently has a bug in it
that causes a panic that is being fixed via -net.

Changes in v2:
- ice driver simple changes added which go with this series
- igb compilation issues of the patch when standalone with CONFIG_PM=n
  fixed by adding missing ifdef, which is then cleaned up in 2/2

original v1:
Link: https://lore.kernel.org/netdev/20240210220109.3179408-1-jesse.brandeburg@intel.com/

[1] https://lore.kernel.org/netdev/20211207002102.26414-1-paul@crapouillou.net/

Jesse Brandeburg (2):
  igb: simplify pci ops declaration
  net: intel: implement modern PM ops declarations

 drivers/net/ethernet/intel/e100.c             |  8 +--
 drivers/net/ethernet/intel/e1000/e1000_main.c | 14 ++---
 drivers/net/ethernet/intel/e1000e/netdev.c    | 22 +++----
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c  | 10 ++--
 drivers/net/ethernet/intel/i40e/i40e_main.c   | 10 ++--
 drivers/net/ethernet/intel/iavf/iavf_main.c   |  8 +--
 drivers/net/ethernet/intel/ice/ice_main.c     | 12 ++--
 drivers/net/ethernet/intel/igb/igb_main.c     | 59 ++++++++-----------
 drivers/net/ethernet/intel/igbvf/netdev.c     |  6 +-
 drivers/net/ethernet/intel/igc/igc_main.c     | 24 +++-----
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  8 +--
 .../net/ethernet/intel/ixgbevf/ixgbevf_main.c |  8 +--
 12 files changed, 78 insertions(+), 111 deletions(-)


base-commit: 60d06425e04558be21634a719b5c60c9bd862c34
-- 
2.39.3

[PATCH iwl-next v2 1/2] igb: simplify pci ops declaration

[Jesse Brandeburg]

The igb driver was pre-declaring tons of functions just so that it could
have an early declaration of the pci_driver struct.

Delete a bunch of the declarations and move the struct to the bottom of the
file, after all the functions are declared.

Reviewed-by: Alan Brady <alan.brady@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
v2: address compilation failure when CONFIG_PM=n, which is then updated
    in patch 2/2, fix alignment.
    changes in v1 reviewed by Simon Horman
    changes in v1 reviewed by Paul Menzel
v1: original net-next posting
---
 drivers/net/ethernet/intel/igb/igb_main.c | 53 ++++++++++-------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 518298bbdadc..e749bf5164b8 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -106,8 +106,6 @@ static int igb_setup_all_rx_resources(struct igb_adapter *);
 static void igb_free_all_tx_resources(struct igb_adapter *);
 static void igb_free_all_rx_resources(struct igb_adapter *);
 static void igb_setup_mrqc(struct igb_adapter *);
-static int igb_probe(struct pci_dev *, const struct pci_device_id *);
-static void igb_remove(struct pci_dev *pdev);
 static void igb_init_queue_configuration(struct igb_adapter *adapter);
 static int igb_sw_init(struct igb_adapter *);
 int igb_open(struct net_device *);
@@ -178,20 +176,6 @@ static int igb_vf_configure(struct igb_adapter *adapter, int vf);
 static int igb_disable_sriov(struct pci_dev *dev, bool reinit);
 #endif
 
-static int igb_suspend(struct device *);
-static int igb_resume(struct device *);
-static int igb_runtime_suspend(struct device *dev);
-static int igb_runtime_resume(struct device *dev);
-static int igb_runtime_idle(struct device *dev);
-#ifdef CONFIG_PM
-static const struct dev_pm_ops igb_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
-	SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
-			igb_runtime_idle)
-};
-#endif
-static void igb_shutdown(struct pci_dev *);
-static int igb_pci_sriov_configure(struct pci_dev *dev, int num_vfs);
 #ifdef CONFIG_IGB_DCA
 static int igb_notify_dca(struct notifier_block *, unsigned long, void *);
 static struct notifier_block dca_notifier = {
@@ -219,19 +203,6 @@ static const struct pci_error_handlers igb_err_handler = {
 
 static void igb_init_dmac(struct igb_adapter *adapter, u32 pba);
 
-static struct pci_driver igb_driver = {
-	.name     = igb_driver_name,
-	.id_table = igb_pci_tbl,
-	.probe    = igb_probe,
-	.remove   = igb_remove,
-#ifdef CONFIG_PM
-	.driver.pm = &igb_pm_ops,
-#endif
-	.shutdown = igb_shutdown,
-	.sriov_configure = igb_pci_sriov_configure,
-	.err_handler = &igb_err_handler
-};
-
 MODULE_AUTHOR("Intel Corporation, <e1000-devel@lists.sourceforge.net>");
 MODULE_DESCRIPTION("Intel(R) Gigabit Ethernet Network Driver");
 MODULE_LICENSE("GPL v2");
@@ -647,6 +618,8 @@ struct net_device *igb_get_hw_dev(struct e1000_hw *hw)
 	return adapter->netdev;
 }
 
+static struct pci_driver igb_driver;
+
 /**
  *  igb_init_module - Driver Registration Routine
  *
@@ -10170,4 +10143,26 @@ static void igb_nfc_filter_restore(struct igb_adapter *adapter)
 
 	spin_unlock(&adapter->nfc_lock);
 }
+
+#ifdef CONFIG_PM
+static const struct dev_pm_ops igb_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
+	SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
+			   igb_runtime_idle)
+};
+#endif
+
+static struct pci_driver igb_driver = {
+	.name     = igb_driver_name,
+	.id_table = igb_pci_tbl,
+	.probe    = igb_probe,
+	.remove   = igb_remove,
+#ifdef CONFIG_PM
+	.driver.pm = &igb_pm_ops,
+#endif
+	.shutdown = igb_shutdown,
+	.sriov_configure = igb_pci_sriov_configure,
+	.err_handler = &igb_err_handler
+};
+
 /* igb_main.c */
-- 
2.39.3

[PATCH iwl-next v2 2/2] net: intel: implement modern PM ops declarations

[Jesse Brandeburg]

Switch the Intel networking drivers to use the new power management ops
declaration formats and macros, which allows us to drop __maybe_unused,
as well as a bunch of ifdef checking CONFIG_PM.

This is safe to do because the compiler drops the unused functions,
verified by checking for any of the power management function symbols
being present in System.map for a build without CONFIG_PM.

If a driver has runtime PM, define the ops with pm_ptr(), and if the
driver has Simple PM, use pm_sleep_ptr(), as well as the new versions of
the macros for declaring the members of the pm_ops structs.

Checked with network-enabled allnoconfig, allyesconfig, allmodconfig on
x64_64.

Reviewed-by: Alan Brady <alan.brady@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
v2: added ice driver changes to series
v1: original net-next posting
    all changes except for ice were reviewed by Simon Horman
    no other changes besides to ice
---
 drivers/net/ethernet/intel/e100.c             |  8 +++---
 drivers/net/ethernet/intel/e1000/e1000_main.c | 14 +++++-----
 drivers/net/ethernet/intel/e1000e/netdev.c    | 22 +++++++---------
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c  | 10 +++----
 drivers/net/ethernet/intel/i40e/i40e_main.c   | 10 +++----
 drivers/net/ethernet/intel/iavf/iavf_main.c   |  8 +++---
 drivers/net/ethernet/intel/ice/ice_main.c     | 12 +++------
 drivers/net/ethernet/intel/igb/igb_main.c     | 26 +++++++------------
 drivers/net/ethernet/intel/igbvf/netdev.c     |  6 ++---
 drivers/net/ethernet/intel/igc/igc_main.c     | 24 ++++++-----------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  8 +++---
 .../net/ethernet/intel/ixgbevf/ixgbevf_main.c |  8 +++---
 12 files changed, 64 insertions(+), 92 deletions(-)

diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
index 3fcb8daaa243..9b068d40778d 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -3037,7 +3037,7 @@ static int __e100_power_off(struct pci_dev *pdev, bool wake)
 	return 0;
 }
 
-static int __maybe_unused e100_suspend(struct device *dev_d)
+static int e100_suspend(struct device *dev_d)
 {
 	bool wake;
 
@@ -3046,7 +3046,7 @@ static int __maybe_unused e100_suspend(struct device *dev_d)
 	return 0;
 }
 
-static int __maybe_unused e100_resume(struct device *dev_d)
+static int e100_resume(struct device *dev_d)
 {
 	struct net_device *netdev = dev_get_drvdata(dev_d);
 	struct nic *nic = netdev_priv(netdev);
@@ -3163,7 +3163,7 @@ static const struct pci_error_handlers e100_err_handler = {
 	.resume = e100_io_resume,
 };
 
-static SIMPLE_DEV_PM_OPS(e100_pm_ops, e100_suspend, e100_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(e100_pm_ops, e100_suspend, e100_resume);
 
 static struct pci_driver e100_driver = {
 	.name =         DRV_NAME,
@@ -3172,7 +3172,7 @@ static struct pci_driver e100_driver = {
 	.remove =       e100_remove,
 
 	/* Power Management hooks */
-	.driver.pm =	&e100_pm_ops,
+	.driver.pm =	pm_sleep_ptr(&e100_pm_ops),
 
 	.shutdown =     e100_shutdown,
 	.err_handler = &e100_err_handler,
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 1d1e93686af2..5b43f9b194fc 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -149,8 +149,8 @@ static int e1000_vlan_rx_kill_vid(struct net_device *netdev,
 				  __be16 proto, u16 vid);
 static void e1000_restore_vlan(struct e1000_adapter *adapter);
 
-static int __maybe_unused e1000_suspend(struct device *dev);
-static int __maybe_unused e1000_resume(struct device *dev);
+static int e1000_suspend(struct device *dev);
+static int e1000_resume(struct device *dev);
 static void e1000_shutdown(struct pci_dev *pdev);
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
@@ -175,16 +175,14 @@ static const struct pci_error_handlers e1000_err_handler = {
 	.resume = e1000_io_resume,
 };
 
-static SIMPLE_DEV_PM_OPS(e1000_pm_ops, e1000_suspend, e1000_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(e1000_pm_ops, e1000_suspend, e1000_resume);
 
 static struct pci_driver e1000_driver = {
 	.name     = e1000_driver_name,
 	.id_table = e1000_pci_tbl,
 	.probe    = e1000_probe,
 	.remove   = e1000_remove,
-	.driver = {
-		.pm = &e1000_pm_ops,
-	},
+	.driver.pm = pm_sleep_ptr(&e1000_pm_ops),
 	.shutdown = e1000_shutdown,
 	.err_handler = &e1000_err_handler
 };
@@ -5135,7 +5133,7 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake)
 	return 0;
 }
 
-static int __maybe_unused e1000_suspend(struct device *dev)
+static int e1000_suspend(struct device *dev)
 {
 	int retval;
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -5147,7 +5145,7 @@ static int __maybe_unused e1000_suspend(struct device *dev)
 	return retval;
 }
 
-static int __maybe_unused e1000_resume(struct device *dev)
+static int e1000_resume(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct net_device *netdev = pci_get_drvdata(pdev);
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index cc8c531ec3df..1c91dece75a8 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6950,13 +6950,13 @@ static int __e1000_resume(struct pci_dev *pdev)
 	return 0;
 }
 
-static __maybe_unused int e1000e_pm_prepare(struct device *dev)
+static int e1000e_pm_prepare(struct device *dev)
 {
 	return pm_runtime_suspended(dev) &&
 		pm_suspend_via_firmware();
 }
 
-static __maybe_unused int e1000e_pm_suspend(struct device *dev)
+static int e1000e_pm_suspend(struct device *dev)
 {
 	struct net_device *netdev = pci_get_drvdata(to_pci_dev(dev));
 	struct e1000_adapter *adapter = netdev_priv(netdev);
@@ -6979,7 +6979,7 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev)
 	return rc;
 }
 
-static __maybe_unused int e1000e_pm_resume(struct device *dev)
+static int e1000e_pm_resume(struct device *dev)
 {
 	struct net_device *netdev = pci_get_drvdata(to_pci_dev(dev));
 	struct e1000_adapter *adapter = netdev_priv(netdev);
@@ -7013,7 +7013,7 @@ static __maybe_unused int e1000e_pm_runtime_idle(struct device *dev)
 	return -EBUSY;
 }
 
-static __maybe_unused int e1000e_pm_runtime_resume(struct device *dev)
+static int e1000e_pm_runtime_resume(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct net_device *netdev = pci_get_drvdata(pdev);
@@ -7032,7 +7032,7 @@ static __maybe_unused int e1000e_pm_runtime_resume(struct device *dev)
 	return rc;
 }
 
-static __maybe_unused int e1000e_pm_runtime_suspend(struct device *dev)
+static int e1000e_pm_runtime_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct net_device *netdev = pci_get_drvdata(pdev);
@@ -7919,8 +7919,7 @@ static const struct pci_device_id e1000_pci_tbl[] = {
 };
 MODULE_DEVICE_TABLE(pci, e1000_pci_tbl);
 
-static const struct dev_pm_ops e1000_pm_ops = {
-#ifdef CONFIG_PM_SLEEP
+static const struct dev_pm_ops e1000e_pm_ops = {
 	.prepare	= e1000e_pm_prepare,
 	.suspend	= e1000e_pm_suspend,
 	.resume		= e1000e_pm_resume,
@@ -7928,9 +7927,8 @@ static const struct dev_pm_ops e1000_pm_ops = {
 	.thaw		= e1000e_pm_thaw,
 	.poweroff	= e1000e_pm_suspend,
 	.restore	= e1000e_pm_resume,
-#endif
-	SET_RUNTIME_PM_OPS(e1000e_pm_runtime_suspend, e1000e_pm_runtime_resume,
-			   e1000e_pm_runtime_idle)
+	RUNTIME_PM_OPS(e1000e_pm_runtime_suspend, e1000e_pm_runtime_resume,
+		       e1000e_pm_runtime_idle)
 };
 
 /* PCI Device API Driver */
@@ -7939,9 +7937,7 @@ static struct pci_driver e1000_driver = {
 	.id_table = e1000_pci_tbl,
 	.probe    = e1000_probe,
 	.remove   = e1000_remove,
-	.driver   = {
-		.pm = &e1000_pm_ops,
-	},
+	.driver.pm = pm_ptr(&e1000e_pm_ops),
 	.shutdown = e1000_shutdown,
 	.err_handler = &e1000_err_handler
 };
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
index d748b98274e7..92de609b7218 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
@@ -2342,7 +2342,7 @@ static int fm10k_handle_resume(struct fm10k_intfc *interface)
  * suspend or hibernation. This function does not need to handle lower PCIe
  * device state as the stack takes care of that for us.
  **/
-static int __maybe_unused fm10k_resume(struct device *dev)
+static int fm10k_resume(struct device *dev)
 {
 	struct fm10k_intfc *interface = dev_get_drvdata(dev);
 	struct net_device *netdev = interface->netdev;
@@ -2369,7 +2369,7 @@ static int __maybe_unused fm10k_resume(struct device *dev)
  * system suspend or hibernation. This function does not need to handle lower
  * PCIe device state as the stack takes care of that for us.
  **/
-static int __maybe_unused fm10k_suspend(struct device *dev)
+static int fm10k_suspend(struct device *dev)
 {
 	struct fm10k_intfc *interface = dev_get_drvdata(dev);
 	struct net_device *netdev = interface->netdev;
@@ -2502,16 +2502,14 @@ static const struct pci_error_handlers fm10k_err_handler = {
 	.reset_done = fm10k_io_reset_done,
 };
 
-static SIMPLE_DEV_PM_OPS(fm10k_pm_ops, fm10k_suspend, fm10k_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(fm10k_pm_ops, fm10k_suspend, fm10k_resume);
 
 static struct pci_driver fm10k_driver = {
 	.name			= fm10k_driver_name,
 	.id_table		= fm10k_pci_tbl,
 	.probe			= fm10k_probe,
 	.remove			= fm10k_remove,
-	.driver = {
-		.pm		= &fm10k_pm_ops,
-	},
+	.driver.pm		= pm_sleep_ptr(&fm10k_pm_ops),
 	.sriov_configure	= fm10k_iov_configure,
 	.err_handler		= &fm10k_err_handler
 };
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 3fada49b8ae2..0628abeb5674 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -16509,7 +16509,7 @@ static void i40e_shutdown(struct pci_dev *pdev)
  * i40e_suspend - PM callback for moving to D3
  * @dev: generic device information structure
  **/
-static int __maybe_unused i40e_suspend(struct device *dev)
+static int i40e_suspend(struct device *dev)
 {
 	struct i40e_pf *pf = dev_get_drvdata(dev);
 	struct i40e_hw *hw = &pf->hw;
@@ -16560,7 +16560,7 @@ static int __maybe_unused i40e_suspend(struct device *dev)
  * i40e_resume - PM callback for waking up from D3
  * @dev: generic device information structure
  **/
-static int __maybe_unused i40e_resume(struct device *dev)
+static int i40e_resume(struct device *dev)
 {
 	struct i40e_pf *pf = dev_get_drvdata(dev);
 	int err;
@@ -16606,16 +16606,14 @@ static const struct pci_error_handlers i40e_err_handler = {
 	.resume = i40e_pci_error_resume,
 };
 
-static SIMPLE_DEV_PM_OPS(i40e_pm_ops, i40e_suspend, i40e_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(i40e_pm_ops, i40e_suspend, i40e_resume);
 
 static struct pci_driver i40e_driver = {
 	.name     = i40e_driver_name,
 	.id_table = i40e_pci_tbl,
 	.probe    = i40e_probe,
 	.remove   = i40e_remove,
-	.driver   = {
-		.pm = &i40e_pm_ops,
-	},
+	.driver.pm = pm_sleep_ptr(&i40e_pm_ops),
 	.shutdown = i40e_shutdown,
 	.err_handler = &i40e_err_handler,
 	.sriov_configure = i40e_pci_sriov_configure,
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index aefec6bd3b67..6010ce71fd66 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -5032,7 +5032,7 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
  *
  * Called when the system (VM) is entering sleep/suspend.
  **/
-static int __maybe_unused iavf_suspend(struct device *dev_d)
+static int iavf_suspend(struct device *dev_d)
 {
 	struct net_device *netdev = dev_get_drvdata(dev_d);
 	struct iavf_adapter *adapter = netdev_priv(netdev);
@@ -5060,7 +5060,7 @@ static int __maybe_unused iavf_suspend(struct device *dev_d)
  *
  * Called when the system (VM) is resumed from sleep/suspend.
  **/
-static int __maybe_unused iavf_resume(struct device *dev_d)
+static int iavf_resume(struct device *dev_d)
 {
 	struct pci_dev *pdev = to_pci_dev(dev_d);
 	struct iavf_adapter *adapter;
@@ -5247,14 +5247,14 @@ static void iavf_shutdown(struct pci_dev *pdev)
 		pci_set_power_state(pdev, PCI_D3hot);
 }
 
-static SIMPLE_DEV_PM_OPS(iavf_pm_ops, iavf_suspend, iavf_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(iavf_pm_ops, iavf_suspend, iavf_resume);
 
 static struct pci_driver iavf_driver = {
 	.name      = iavf_driver_name,
 	.id_table  = iavf_pci_tbl,
 	.probe     = iavf_probe,
 	.remove    = iavf_remove,
-	.driver.pm = &iavf_pm_ops,
+	.driver.pm = pm_sleep_ptr(&iavf_pm_ops),
 	.shutdown  = iavf_shutdown,
 };
 
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 8f73ba77e835..48fdcf883365 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5321,7 +5321,6 @@ static void ice_shutdown(struct pci_dev *pdev)
 	}
 }
 
-#ifdef CONFIG_PM
 /**
  * ice_prepare_for_shutdown - prep for PCI shutdown
  * @pf: board private structure
@@ -5410,7 +5409,7 @@ static int ice_reinit_interrupt_scheme(struct ice_pf *pf)
  * Power Management callback to quiesce the device and prepare
  * for D3 transition.
  */
-static int __maybe_unused ice_suspend(struct device *dev)
+static int ice_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct ice_pf *pf;
@@ -5477,7 +5476,7 @@ static int __maybe_unused ice_suspend(struct device *dev)
  * ice_resume - PM callback for waking up from D3
  * @dev: generic device information structure
  */
-static int __maybe_unused ice_resume(struct device *dev)
+static int ice_resume(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	enum ice_reset_req reset_type;
@@ -5528,7 +5527,6 @@ static int __maybe_unused ice_resume(struct device *dev)
 
 	return 0;
 }
-#endif /* CONFIG_PM */
 
 /**
  * ice_pci_err_detected - warning that PCI error has been detected
@@ -5702,7 +5700,7 @@ static const struct pci_device_id ice_pci_tbl[] = {
 };
 MODULE_DEVICE_TABLE(pci, ice_pci_tbl);
 
-static __maybe_unused SIMPLE_DEV_PM_OPS(ice_pm_ops, ice_suspend, ice_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(ice_pm_ops, ice_suspend, ice_resume);
 
 static const struct pci_error_handlers ice_pci_err_handler = {
 	.error_detected = ice_pci_err_detected,
@@ -5717,9 +5715,7 @@ static struct pci_driver ice_driver = {
 	.id_table = ice_pci_tbl,
 	.probe = ice_probe,
 	.remove = ice_remove,
-#ifdef CONFIG_PM
-	.driver.pm = &ice_pm_ops,
-#endif /* CONFIG_PM */
+	.driver.pm = pm_sleep_ptr(&ice_pm_ops),
 	.shutdown = ice_shutdown,
 	.sriov_configure = ice_sriov_configure,
 	.sriov_get_vf_total_msix = ice_sriov_get_vf_total_msix,
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index e749bf5164b8..820f6688ce38 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -9439,12 +9439,12 @@ static void igb_deliver_wake_packet(struct net_device *netdev)
 	netif_rx(skb);
 }
 
-static int __maybe_unused igb_suspend(struct device *dev)
+static int igb_suspend(struct device *dev)
 {
 	return __igb_shutdown(to_pci_dev(dev), NULL, 0);
 }
 
-static int __maybe_unused __igb_resume(struct device *dev, bool rpm)
+static int __igb_resume(struct device *dev, bool rpm)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct net_device *netdev = pci_get_drvdata(pdev);
@@ -9500,12 +9500,12 @@ static int __maybe_unused __igb_resume(struct device *dev, bool rpm)
 	return err;
 }
 
-static int __maybe_unused igb_resume(struct device *dev)
+static int igb_resume(struct device *dev)
 {
 	return __igb_resume(dev, false);
 }
 
-static int __maybe_unused igb_runtime_idle(struct device *dev)
+static int igb_runtime_idle(struct device *dev)
 {
 	struct net_device *netdev = dev_get_drvdata(dev);
 	struct igb_adapter *adapter = netdev_priv(netdev);
@@ -9516,12 +9516,12 @@ static int __maybe_unused igb_runtime_idle(struct device *dev)
 	return -EBUSY;
 }
 
-static int __maybe_unused igb_runtime_suspend(struct device *dev)
+static int igb_runtime_suspend(struct device *dev)
 {
 	return __igb_shutdown(to_pci_dev(dev), NULL, 1);
 }
 
-static int __maybe_unused igb_runtime_resume(struct device *dev)
+static int igb_runtime_resume(struct device *dev)
 {
 	return __igb_resume(dev, true);
 }
@@ -10144,22 +10144,16 @@ static void igb_nfc_filter_restore(struct igb_adapter *adapter)
 	spin_unlock(&adapter->nfc_lock);
 }
 
-#ifdef CONFIG_PM
-static const struct dev_pm_ops igb_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
-	SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
-			   igb_runtime_idle)
-};
-#endif
+static _DEFINE_DEV_PM_OPS(igb_pm_ops, igb_suspend, igb_resume,
+			  igb_runtime_suspend, igb_runtime_resume,
+			  igb_runtime_idle);
 
 static struct pci_driver igb_driver = {
 	.name     = igb_driver_name,
 	.id_table = igb_pci_tbl,
 	.probe    = igb_probe,
 	.remove   = igb_remove,
-#ifdef CONFIG_PM
-	.driver.pm = &igb_pm_ops,
-#endif
+	.driver.pm = pm_ptr(&igb_pm_ops),
 	.shutdown = igb_shutdown,
 	.sriov_configure = igb_pci_sriov_configure,
 	.err_handler = &igb_err_handler
diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index b0cf310e6f7b..40ccd24ffc53 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -2470,7 +2470,7 @@ static int igbvf_suspend(struct device *dev_d)
 	return 0;
 }
 
-static int __maybe_unused igbvf_resume(struct device *dev_d)
+static int igbvf_resume(struct device *dev_d)
 {
 	struct pci_dev *pdev = to_pci_dev(dev_d);
 	struct net_device *netdev = pci_get_drvdata(pdev);
@@ -2957,7 +2957,7 @@ static const struct pci_device_id igbvf_pci_tbl[] = {
 };
 MODULE_DEVICE_TABLE(pci, igbvf_pci_tbl);
 
-static SIMPLE_DEV_PM_OPS(igbvf_pm_ops, igbvf_suspend, igbvf_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(igbvf_pm_ops, igbvf_suspend, igbvf_resume);
 
 /* PCI Device API Driver */
 static struct pci_driver igbvf_driver = {
@@ -2965,7 +2965,7 @@ static struct pci_driver igbvf_driver = {
 	.id_table	= igbvf_pci_tbl,
 	.probe		= igbvf_probe,
 	.remove		= igbvf_remove,
-	.driver.pm	= &igbvf_pm_ops,
+	.driver.pm	= pm_sleep_ptr(&igbvf_pm_ops),
 	.shutdown	= igbvf_shutdown,
 	.err_handler	= &igbvf_err_handler
 };
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 34820f6a78b9..8d1415ca70bb 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -7120,8 +7120,7 @@ static int __igc_shutdown(struct pci_dev *pdev, bool *enable_wake,
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int __maybe_unused igc_runtime_suspend(struct device *dev)
+static int igc_runtime_suspend(struct device *dev)
 {
 	return __igc_shutdown(to_pci_dev(dev), NULL, 1);
 }
@@ -7156,7 +7155,7 @@ static void igc_deliver_wake_packet(struct net_device *netdev)
 	netif_rx(skb);
 }
 
-static int __maybe_unused igc_resume(struct device *dev)
+static int igc_resume(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct net_device *netdev = pci_get_drvdata(pdev);
@@ -7209,12 +7208,12 @@ static int __maybe_unused igc_resume(struct device *dev)
 	return err;
 }
 
-static int __maybe_unused igc_runtime_resume(struct device *dev)
+static int igc_runtime_resume(struct device *dev)
 {
 	return igc_resume(dev);
 }
 
-static int __maybe_unused igc_suspend(struct device *dev)
+static int igc_suspend(struct device *dev)
 {
 	return __igc_shutdown(to_pci_dev(dev), NULL, 0);
 }
@@ -7229,7 +7228,6 @@ static int __maybe_unused igc_runtime_idle(struct device *dev)
 
 	return -EBUSY;
 }
-#endif /* CONFIG_PM */
 
 static void igc_shutdown(struct pci_dev *pdev)
 {
@@ -7344,22 +7342,16 @@ static const struct pci_error_handlers igc_err_handler = {
 	.resume = igc_io_resume,
 };
 
-#ifdef CONFIG_PM
-static const struct dev_pm_ops igc_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(igc_suspend, igc_resume)
-	SET_RUNTIME_PM_OPS(igc_runtime_suspend, igc_runtime_resume,
-			   igc_runtime_idle)
-};
-#endif
+static _DEFINE_DEV_PM_OPS(igc_pm_ops, igc_suspend, igc_resume,
+			  igc_runtime_suspend, igc_runtime_resume,
+			  igc_runtime_idle);
 
 static struct pci_driver igc_driver = {
 	.name     = igc_driver_name,
 	.id_table = igc_pci_tbl,
 	.probe    = igc_probe,
 	.remove   = igc_remove,
-#ifdef CONFIG_PM
-	.driver.pm = &igc_pm_ops,
-#endif
+	.driver.pm = pm_ptr(&igc_pm_ops),
 	.shutdown = igc_shutdown,
 	.err_handler = &igc_err_handler,
 };
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 595098a4c488..4fec48143035 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6944,7 +6944,7 @@ int ixgbe_close(struct net_device *netdev)
 	return 0;
 }
 
-static int __maybe_unused ixgbe_resume(struct device *dev_d)
+static int ixgbe_resume(struct device *dev_d)
 {
 	struct pci_dev *pdev = to_pci_dev(dev_d);
 	struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
@@ -7052,7 +7052,7 @@ static int __ixgbe_shutdown(struct pci_dev *pdev, bool *enable_wake)
 	return 0;
 }
 
-static int __maybe_unused ixgbe_suspend(struct device *dev_d)
+static int ixgbe_suspend(struct device *dev_d)
 {
 	struct pci_dev *pdev = to_pci_dev(dev_d);
 	int retval;
@@ -11516,14 +11516,14 @@ static const struct pci_error_handlers ixgbe_err_handler = {
 	.resume = ixgbe_io_resume,
 };
 
-static SIMPLE_DEV_PM_OPS(ixgbe_pm_ops, ixgbe_suspend, ixgbe_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(ixgbe_pm_ops, ixgbe_suspend, ixgbe_resume);
 
 static struct pci_driver ixgbe_driver = {
 	.name      = ixgbe_driver_name,
 	.id_table  = ixgbe_pci_tbl,
 	.probe     = ixgbe_probe,
 	.remove    = ixgbe_remove,
-	.driver.pm = &ixgbe_pm_ops,
+	.driver.pm = pm_sleep_ptr(&ixgbe_pm_ops),
 	.shutdown  = ixgbe_shutdown,
 	.sriov_configure = ixgbe_pci_sriov_configure,
 	.err_handler = &ixgbe_err_handler
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 9c960017a6de..3161a13079fe 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -4300,7 +4300,7 @@ static int ixgbevf_change_mtu(struct net_device *netdev, int new_mtu)
 	return 0;
 }
 
-static int __maybe_unused ixgbevf_suspend(struct device *dev_d)
+static int ixgbevf_suspend(struct device *dev_d)
 {
 	struct net_device *netdev = dev_get_drvdata(dev_d);
 	struct ixgbevf_adapter *adapter = netdev_priv(netdev);
@@ -4317,7 +4317,7 @@ static int __maybe_unused ixgbevf_suspend(struct device *dev_d)
 	return 0;
 }
 
-static int __maybe_unused ixgbevf_resume(struct device *dev_d)
+static int ixgbevf_resume(struct device *dev_d)
 {
 	struct pci_dev *pdev = to_pci_dev(dev_d);
 	struct net_device *netdev = pci_get_drvdata(pdev);
@@ -4854,7 +4854,7 @@ static const struct pci_error_handlers ixgbevf_err_handler = {
 	.resume = ixgbevf_io_resume,
 };
 
-static SIMPLE_DEV_PM_OPS(ixgbevf_pm_ops, ixgbevf_suspend, ixgbevf_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(ixgbevf_pm_ops, ixgbevf_suspend, ixgbevf_resume);
 
 static struct pci_driver ixgbevf_driver = {
 	.name		= ixgbevf_driver_name,
@@ -4863,7 +4863,7 @@ static struct pci_driver ixgbevf_driver = {
 	.remove		= ixgbevf_remove,
 
 	/* Power Management Hooks */
-	.driver.pm	= &ixgbevf_pm_ops,
+	.driver.pm	= pm_sleep_ptr(&ixgbevf_pm_ops),
 
 	.shutdown	= ixgbevf_shutdown,
 	.err_handler	= &ixgbevf_err_handler
-- 
2.39.3

Re: [PATCH iwl-next v2 1/2] igb: simplify pci ops declaration

[Paul Menzel]

[Cc: +linux-pci@vger.kernel.org]


Dear Jesse,


Am 06.03.24 um 03:50 schrieb Jesse Brandeburg:
> The igb driver was pre-declaring tons of functions just so that it could
> have an early declaration of the pci_driver struct.
> 
> Delete a bunch of the declarations and move the struct to the bottom of the
> file, after all the functions are declared.
> 
> Reviewed-by: Alan Brady <alan.brady@intel.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
> v2: address compilation failure when CONFIG_PM=n, which is then updated
>      in patch 2/2, fix alignment.
>      changes in v1 reviewed by Simon Horman
>      changes in v1 reviewed by Paul Menzel
> v1: original net-next posting
> ---
>   drivers/net/ethernet/intel/igb/igb_main.c | 53 ++++++++++-------------
>   1 file changed, 24 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 518298bbdadc..e749bf5164b8 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -106,8 +106,6 @@ static int igb_setup_all_rx_resources(struct igb_adapter *);
>   static void igb_free_all_tx_resources(struct igb_adapter *);
>   static void igb_free_all_rx_resources(struct igb_adapter *);
>   static void igb_setup_mrqc(struct igb_adapter *);
> -static int igb_probe(struct pci_dev *, const struct pci_device_id *);
> -static void igb_remove(struct pci_dev *pdev);
>   static void igb_init_queue_configuration(struct igb_adapter *adapter);
>   static int igb_sw_init(struct igb_adapter *);
>   int igb_open(struct net_device *);
> @@ -178,20 +176,6 @@ static int igb_vf_configure(struct igb_adapter *adapter, int vf);
>   static int igb_disable_sriov(struct pci_dev *dev, bool reinit);
>   #endif
>   
> -static int igb_suspend(struct device *);
> -static int igb_resume(struct device *);
> -static int igb_runtime_suspend(struct device *dev);
> -static int igb_runtime_resume(struct device *dev);
> -static int igb_runtime_idle(struct device *dev);
> -#ifdef CONFIG_PM
> -static const struct dev_pm_ops igb_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
> -	SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
> -			igb_runtime_idle)
> -};
> -#endif
> -static void igb_shutdown(struct pci_dev *);
> -static int igb_pci_sriov_configure(struct pci_dev *dev, int num_vfs);
>   #ifdef CONFIG_IGB_DCA
>   static int igb_notify_dca(struct notifier_block *, unsigned long, void *);
>   static struct notifier_block dca_notifier = {
> @@ -219,19 +203,6 @@ static const struct pci_error_handlers igb_err_handler = {
>   
>   static void igb_init_dmac(struct igb_adapter *adapter, u32 pba);
>   
> -static struct pci_driver igb_driver = {
> -	.name     = igb_driver_name,
> -	.id_table = igb_pci_tbl,
> -	.probe    = igb_probe,
> -	.remove   = igb_remove,
> -#ifdef CONFIG_PM
> -	.driver.pm = &igb_pm_ops,
> -#endif
> -	.shutdown = igb_shutdown,
> -	.sriov_configure = igb_pci_sriov_configure,
> -	.err_handler = &igb_err_handler
> -};
> -
>   MODULE_AUTHOR("Intel Corporation, <e1000-devel@lists.sourceforge.net>");
>   MODULE_DESCRIPTION("Intel(R) Gigabit Ethernet Network Driver");
>   MODULE_LICENSE("GPL v2");

A lot of other drivers also have this at the end.

> @@ -647,6 +618,8 @@ struct net_device *igb_get_hw_dev(struct e1000_hw *hw)
>   	return adapter->netdev;
>   }
>   
> +static struct pci_driver igb_driver;
> +
>   /**
>    *  igb_init_module - Driver Registration Routine
>    *
> @@ -10170,4 +10143,26 @@ static void igb_nfc_filter_restore(struct igb_adapter *adapter)
>   
>   	spin_unlock(&adapter->nfc_lock);
>   }
> +
> +#ifdef CONFIG_PM
> +static const struct dev_pm_ops igb_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
> +	SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
> +			   igb_runtime_idle)
> +};
> +#endif
> +
> +static struct pci_driver igb_driver = {
> +	.name     = igb_driver_name,
> +	.id_table = igb_pci_tbl,
> +	.probe    = igb_probe,
> +	.remove   = igb_remove,
> +#ifdef CONFIG_PM
> +	.driver.pm = &igb_pm_ops,
> +#endif
> +	.shutdown = igb_shutdown,
> +	.sriov_configure = igb_pci_sriov_configure,
> +	.err_handler = &igb_err_handler
> +};
> +
>   /* igb_main.c */

I looked through `drivers/` and .driver.pm is unguarded there. Example 
`drivers/video/fbdev/geode/gxfb_core.c`:

     static const struct dev_pm_ops gxfb_pm_ops = {
     #ifdef CONFIG_PM_SLEEP
             .suspend        = gxfb_suspend,
             .resume         = gxfb_resume,
             .freeze         = NULL,
             .thaw           = gxfb_resume,
             .poweroff       = NULL,
             .restore        = gxfb_resume,
     #endif
     };

     static struct pci_driver gxfb_driver = {
             .name           = "gxfb",
             .id_table       = gxfb_id_table,
             .probe          = gxfb_probe,
             .remove         = gxfb_remove,
             .driver.pm      = &gxfb_pm_ops,
     };

No idea, what driver follows the best practices though, and if it would 
belong into a separate commit.


Kind regards,

Paul

Re: [PATCH iwl-next v2 1/2] igb: simplify pci ops declaration

[Heiner Kallweit]

On 06.03.2024 07:24, Paul Menzel wrote:
> [Cc: +linux-pci@vger.kernel.org]
> 
> 
> Dear Jesse,
> 
> 
> Am 06.03.24 um 03:50 schrieb Jesse Brandeburg:
>> The igb driver was pre-declaring tons of functions just so that it could
>> have an early declaration of the pci_driver struct.
>>
>> Delete a bunch of the declarations and move the struct to the bottom of the
>> file, after all the functions are declared.
>>
>> Reviewed-by: Alan Brady <alan.brady@intel.com>
>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>> ---
>> v2: address compilation failure when CONFIG_PM=n, which is then updated
>>      in patch 2/2, fix alignment.
>>      changes in v1 reviewed by Simon Horman
>>      changes in v1 reviewed by Paul Menzel
>> v1: original net-next posting
>> ---
>>   drivers/net/ethernet/intel/igb/igb_main.c | 53 ++++++++++-------------
>>   1 file changed, 24 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index 518298bbdadc..e749bf5164b8 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -106,8 +106,6 @@ static int igb_setup_all_rx_resources(struct igb_adapter *);
>>   static void igb_free_all_tx_resources(struct igb_adapter *);
>>   static void igb_free_all_rx_resources(struct igb_adapter *);
>>   static void igb_setup_mrqc(struct igb_adapter *);
>> -static int igb_probe(struct pci_dev *, const struct pci_device_id *);
>> -static void igb_remove(struct pci_dev *pdev);
>>   static void igb_init_queue_configuration(struct igb_adapter *adapter);
>>   static int igb_sw_init(struct igb_adapter *);
>>   int igb_open(struct net_device *);
>> @@ -178,20 +176,6 @@ static int igb_vf_configure(struct igb_adapter *adapter, int vf);
>>   static int igb_disable_sriov(struct pci_dev *dev, bool reinit);
>>   #endif
>>   -static int igb_suspend(struct device *);
>> -static int igb_resume(struct device *);
>> -static int igb_runtime_suspend(struct device *dev);
>> -static int igb_runtime_resume(struct device *dev);
>> -static int igb_runtime_idle(struct device *dev);
>> -#ifdef CONFIG_PM
>> -static const struct dev_pm_ops igb_pm_ops = {
>> -    SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
>> -    SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
>> -            igb_runtime_idle)
>> -};
>> -#endif
>> -static void igb_shutdown(struct pci_dev *);
>> -static int igb_pci_sriov_configure(struct pci_dev *dev, int num_vfs);
>>   #ifdef CONFIG_IGB_DCA
>>   static int igb_notify_dca(struct notifier_block *, unsigned long, void *);
>>   static struct notifier_block dca_notifier = {
>> @@ -219,19 +203,6 @@ static const struct pci_error_handlers igb_err_handler = {
>>     static void igb_init_dmac(struct igb_adapter *adapter, u32 pba);
>>   -static struct pci_driver igb_driver = {
>> -    .name     = igb_driver_name,
>> -    .id_table = igb_pci_tbl,
>> -    .probe    = igb_probe,
>> -    .remove   = igb_remove,
>> -#ifdef CONFIG_PM
>> -    .driver.pm = &igb_pm_ops,
>> -#endif
>> -    .shutdown = igb_shutdown,
>> -    .sriov_configure = igb_pci_sriov_configure,
>> -    .err_handler = &igb_err_handler
>> -};
>> -
>>   MODULE_AUTHOR("Intel Corporation, <e1000-devel@lists.sourceforge.net>");
>>   MODULE_DESCRIPTION("Intel(R) Gigabit Ethernet Network Driver");
>>   MODULE_LICENSE("GPL v2");
> 
> A lot of other drivers also have this at the end.
> 
>> @@ -647,6 +618,8 @@ struct net_device *igb_get_hw_dev(struct e1000_hw *hw)
>>       return adapter->netdev;
>>   }
>>   +static struct pci_driver igb_driver;
>> +
>>   /**
>>    *  igb_init_module - Driver Registration Routine
>>    *
>> @@ -10170,4 +10143,26 @@ static void igb_nfc_filter_restore(struct igb_adapter *adapter)
>>         spin_unlock(&adapter->nfc_lock);
>>   }
>> +
>> +#ifdef CONFIG_PM
>> +static const struct dev_pm_ops igb_pm_ops = {
>> +    SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
>> +    SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
>> +               igb_runtime_idle)
>> +};
>> +#endif
>> +
>> +static struct pci_driver igb_driver = {
>> +    .name     = igb_driver_name,
>> +    .id_table = igb_pci_tbl,
>> +    .probe    = igb_probe,
>> +    .remove   = igb_remove,
>> +#ifdef CONFIG_PM
>> +    .driver.pm = &igb_pm_ops,
>> +#endif
>> +    .shutdown = igb_shutdown,
>> +    .sriov_configure = igb_pci_sriov_configure,
>> +    .err_handler = &igb_err_handler
>> +};
>> +
>>   /* igb_main.c */
> 
> I looked through `drivers/` and .driver.pm is unguarded there. Example `drivers/video/fbdev/geode/gxfb_core.c`:
> 
>     static const struct dev_pm_ops gxfb_pm_ops = {
>     #ifdef CONFIG_PM_SLEEP
>             .suspend        = gxfb_suspend,
>             .resume         = gxfb_resume,
>             .freeze         = NULL,
>             .thaw           = gxfb_resume,
>             .poweroff       = NULL,
>             .restore        = gxfb_resume,
>     #endif
>     };
> 
>     static struct pci_driver gxfb_driver = {
>             .name           = "gxfb",
>             .id_table       = gxfb_id_table,
>             .probe          = gxfb_probe,
>             .remove         = gxfb_remove,
>             .driver.pm      = &gxfb_pm_ops,
>     };
> 
> No idea, what driver follows the best practices though, and if it would belong into a separate commit.
> 
The geode fbdev driver may be a bad example as it's ancient.
There's pm_sleep_ptr, SYSTEM_SLEEP_PM_OPS et al to avoid the conditional
compiling and use of __maybe_unused. And yes, I also think this should be
a separate patch.

> 
> Kind regards,
> 
> Paul
> 

Re: [PATCH iwl-next v2 1/2] igb: simplify pci ops declaration

[Paul Menzel]

Dear Linux folks,


Am 06.03.24 um 07:46 schrieb Heiner Kallweit:
> On 06.03.2024 07:24, Paul Menzel wrote:
>> [Cc: +linux-pci@vger.kernel.org]

>> Am 06.03.24 um 03:50 schrieb Jesse Brandeburg:
>>> The igb driver was pre-declaring tons of functions just so that it could
>>> have an early declaration of the pci_driver struct.
>>>
>>> Delete a bunch of the declarations and move the struct to the bottom of the
>>> file, after all the functions are declared.
>>>
>>> Reviewed-by: Alan Brady <alan.brady@intel.com>
>>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>> ---
>>> v2: address compilation failure when CONFIG_PM=n, which is then updated
>>>       in patch 2/2, fix alignment.
>>>       changes in v1 reviewed by Simon Horman
>>>       changes in v1 reviewed by Paul Menzel
>>> v1: original net-next posting
>>> ---
>>>    drivers/net/ethernet/intel/igb/igb_main.c | 53 ++++++++++-------------
>>>    1 file changed, 24 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>>> index 518298bbdadc..e749bf5164b8 100644
>>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>>> @@ -106,8 +106,6 @@ static int igb_setup_all_rx_resources(struct igb_adapter *);
>>>    static void igb_free_all_tx_resources(struct igb_adapter *);
>>>    static void igb_free_all_rx_resources(struct igb_adapter *);
>>>    static void igb_setup_mrqc(struct igb_adapter *);
>>> -static int igb_probe(struct pci_dev *, const struct pci_device_id *);
>>> -static void igb_remove(struct pci_dev *pdev);
>>>    static void igb_init_queue_configuration(struct igb_adapter *adapter);
>>>    static int igb_sw_init(struct igb_adapter *);
>>>    int igb_open(struct net_device *);
>>> @@ -178,20 +176,6 @@ static int igb_vf_configure(struct igb_adapter *adapter, int vf);
>>>    static int igb_disable_sriov(struct pci_dev *dev, bool reinit);
>>>    #endif
>>>    -static int igb_suspend(struct device *);
>>> -static int igb_resume(struct device *);
>>> -static int igb_runtime_suspend(struct device *dev);
>>> -static int igb_runtime_resume(struct device *dev);
>>> -static int igb_runtime_idle(struct device *dev);
>>> -#ifdef CONFIG_PM
>>> -static const struct dev_pm_ops igb_pm_ops = {
>>> -    SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
>>> -    SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
>>> -            igb_runtime_idle)
>>> -};
>>> -#endif
>>> -static void igb_shutdown(struct pci_dev *);
>>> -static int igb_pci_sriov_configure(struct pci_dev *dev, int num_vfs);
>>>    #ifdef CONFIG_IGB_DCA
>>>    static int igb_notify_dca(struct notifier_block *, unsigned long, void *);
>>>    static struct notifier_block dca_notifier = {
>>> @@ -219,19 +203,6 @@ static const struct pci_error_handlers igb_err_handler = {
>>>      static void igb_init_dmac(struct igb_adapter *adapter, u32 pba);
>>>    -static struct pci_driver igb_driver = {
>>> -    .name     = igb_driver_name,
>>> -    .id_table = igb_pci_tbl,
>>> -    .probe    = igb_probe,
>>> -    .remove   = igb_remove,
>>> -#ifdef CONFIG_PM
>>> -    .driver.pm = &igb_pm_ops,
>>> -#endif
>>> -    .shutdown = igb_shutdown,
>>> -    .sriov_configure = igb_pci_sriov_configure,
>>> -    .err_handler = &igb_err_handler
>>> -};
>>> -
>>>    MODULE_AUTHOR("Intel Corporation, <e1000-devel@lists.sourceforge.net>");
>>>    MODULE_DESCRIPTION("Intel(R) Gigabit Ethernet Network Driver");
>>>    MODULE_LICENSE("GPL v2");
>>
>> A lot of other drivers also have this at the end.
>>
>>> @@ -647,6 +618,8 @@ struct net_device *igb_get_hw_dev(struct e1000_hw *hw)
>>>        return adapter->netdev;
>>>    }
>>>    +static struct pci_driver igb_driver;
>>> +
>>>    /**
>>>     *  igb_init_module - Driver Registration Routine
>>>     *
>>> @@ -10170,4 +10143,26 @@ static void igb_nfc_filter_restore(struct igb_adapter *adapter)
>>>          spin_unlock(&adapter->nfc_lock);
>>>    }
>>> +
>>> +#ifdef CONFIG_PM
>>> +static const struct dev_pm_ops igb_pm_ops = {
>>> +    SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
>>> +    SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
>>> +               igb_runtime_idle)
>>> +};
>>> +#endif
>>> +
>>> +static struct pci_driver igb_driver = {
>>> +    .name     = igb_driver_name,
>>> +    .id_table = igb_pci_tbl,
>>> +    .probe    = igb_probe,
>>> +    .remove   = igb_remove,
>>> +#ifdef CONFIG_PM
>>> +    .driver.pm = &igb_pm_ops,
>>> +#endif
>>> +    .shutdown = igb_shutdown,
>>> +    .sriov_configure = igb_pci_sriov_configure,
>>> +    .err_handler = &igb_err_handler
>>> +};
>>> +
>>>    /* igb_main.c */
>>
>> I looked through `drivers/` and .driver.pm is unguarded there.
>> Example `drivers/video/fbdev/geode/gxfb_core.c`: >>
>>      static const struct dev_pm_ops gxfb_pm_ops = {
>>      #ifdef CONFIG_PM_SLEEP
>>              .suspend        = gxfb_suspend,
>>              .resume         = gxfb_resume,
>>              .freeze         = NULL,
>>              .thaw           = gxfb_resume,
>>              .poweroff       = NULL,
>>              .restore        = gxfb_resume,
>>      #endif
>>      };
>>
>>      static struct pci_driver gxfb_driver = {
>>              .name           = "gxfb",
>>              .id_table       = gxfb_id_table,
>>              .probe          = gxfb_probe,
>>              .remove         = gxfb_remove,
>>              .driver.pm      = &gxfb_pm_ops,
>>      };
>>
>> No idea, what driver follows the best practices though, and if it
>> would belong into a separate commit.
> 
> The geode fbdev driver may be a bad example as it's ancient. There's
> pm_sleep_ptr, SYSTEM_SLEEP_PM_OPS et al to avoid the conditional 
> compiling and use of __maybe_unused. And yes, I also think this
> should be a separate patch.

Sorry for the noise. I should looked at or remembered patch 2/2, doing 
exactly that.


Kind regards,

Paul

Re: [PATCH iwl-next v2 1/2] igb: simplify pci ops declaration

[Maciej Fijalkowski]

On Tue, Mar 05, 2024 at 06:50:21PM -0800, Jesse Brandeburg wrote:
> The igb driver was pre-declaring tons of functions just so that it could
> have an early declaration of the pci_driver struct.
> 
> Delete a bunch of the declarations and move the struct to the bottom of the
> file, after all the functions are declared.

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

However, that's just a drop in the ocean when we look at unneeded forward
declarations, isn't it?

I got side-tracked while looking at some stuff and saw such forward decls
in i40e_nvm.c and decided to fix this as this was rather a no-brainer that
just required to move code around. Now I feel encouraged to send this, but
what should we do with rest of those, though?

Being a regex amateur I came up with following command:
$ grep -hPonz 'static [a-z]+.+\([^()]+\);' drivers/net/ethernet/intel/igb/*.c | sed 's/1:/\n/g'

in order to catch all of the forward declarations in source files and this
results in very nasty list [0]...and this is only within igb!

ice driver is a nice example that reviews in netdev community matter. It
contains only 4 fwd decls and I believe it is very much related to times
when vendor pull requests started to be actually reviewed in the upstream,
not just taken as-is.

[0]:
static s32  igb_get_invariants_82575(struct e1000_hw *);
static s32  igb_acquire_phy_82575(struct e1000_hw *);
static void igb_release_phy_82575(struct e1000_hw *);
static s32  igb_acquire_nvm_82575(struct e1000_hw *);
static void igb_release_nvm_82575(struct e1000_hw *);
static s32  igb_check_for_link_82575(struct e1000_hw *);
static s32  igb_get_cfg_done_82575(struct e1000_hw *);
static s32  igb_init_hw_82575(struct e1000_hw *);
static s32  igb_phy_hw_reset_sgmii_82575(struct e1000_hw *);
static s32  igb_read_phy_reg_sgmii_82575(struct e1000_hw *, u32, u16 *);
static s32  igb_reset_hw_82575(struct e1000_hw *);
static s32  igb_reset_hw_82580(struct e1000_hw *);
static s32  igb_set_d0_lplu_state_82575(struct e1000_hw *, bool);
static s32  igb_set_d0_lplu_state_82580(struct e1000_hw *, bool);
static s32  igb_set_d3_lplu_state_82580(struct e1000_hw *, bool);
static s32  igb_setup_copper_link_82575(struct e1000_hw *);
static s32  igb_setup_serdes_link_82575(struct e1000_hw *);
static s32  igb_write_phy_reg_sgmii_82575(struct e1000_hw *, u32, u16);
static void igb_clear_hw_cntrs_82575(struct e1000_hw *);
static s32  igb_acquire_swfw_sync_82575(struct e1000_hw *, u16);
static s32  igb_get_pcs_speed_and_duplex_82575(struct e1000_hw *, u16 *
						 u16 *);
static s32  igb_get_phy_id_82575(struct e1000_hw *);
static void igb_release_swfw_sync_82575(struct e1000_hw *, u16);
static bool igb_sgmii_active_82575(struct e1000_hw *);
static s32  igb_reset_init_script_82575(struct e1000_hw *);
static s32  igb_read_mac_addr_82575(struct e1000_hw *);
static s32  igb_set_pcie_completion_timeout(struct e1000_hw *hw);
static s32  igb_reset_mdicnfg_82580(struct e1000_hw *hw);
static s32  igb_validate_nvm_checksum_82580(struct e1000_hw *hw);
static s32  igb_update_nvm_checksum_82580(struct e1000_hw *hw);
static s32 igb_validate_nvm_checksum_i350(struct e1000_hw *hw);
static s32 igb_update_nvm_checksum_i350(struct e1000_hw *hw);
static s32 igb_update_flash_i210(struct e1000_hw *hw);
static s32 igb_set_default_fc(struct e1000_hw *hw);
static void igb_set_fc_watermarks(struct e1000_hw *hw);
static s32  igb_phy_setup_autoneg(struct e1000_hw *hw);
static void igb_phy_force_speed_duplex_setup(struct e1000_hw *hw
					     u16 *phy_ctrl);
static s32  igb_wait_autoneg(struct e1000_hw *hw);
static s32  igb_set_master_slave_mode(struct e1000_hw *hw);
static int igb_setup_all_tx_resources(struct igb_adapter *);
static int igb_setup_all_rx_resources(struct igb_adapter *);
static void igb_free_all_tx_resources(struct igb_adapter *);
static void igb_free_all_rx_resources(struct igb_adapter *);
static void igb_setup_mrqc(struct igb_adapter *);
static int igb_probe(struct pci_dev *, const struct pci_device_id *);
static void igb_remove(struct pci_dev *pdev);
static void igb_init_queue_configuration(struct igb_adapter *adapter);
static int igb_sw_init(struct igb_adapter *);
static void igb_configure(struct igb_adapter *);
static void igb_configure_tx(struct igb_adapter *);
static void igb_configure_rx(struct igb_adapter *);
static void igb_clean_all_tx_rings(struct igb_adapter *);
static void igb_clean_all_rx_rings(struct igb_adapter *);
static void igb_clean_tx_ring(struct igb_ring *);
static void igb_clean_rx_ring(struct igb_ring *);
static void igb_set_rx_mode(struct net_device *);
static void igb_update_phy_info(struct timer_list *);
static void igb_watchdog(struct timer_list *);
static void igb_watchdog_task(struct work_struct *);
static netdev_tx_t igb_xmit_frame(struct sk_buff *skb, struct net_device *);
static void igb_get_stats64(struct net_device *dev
			    struct rtnl_link_stats64 *stats);
static int igb_change_mtu(struct net_device *, int);
static int igb_set_mac(struct net_device *, void *);
static void igb_set_uta(struct igb_adapter *adapter, bool set);
static irqreturn_t igb_intr(int irq, void *);
static irqreturn_t igb_intr_msi(int irq, void *);
static irqreturn_t igb_msix_other(int irq, void *);
static irqreturn_t igb_msix_ring(int irq, void *);
static void igb_update_dca(struct igb_q_vector *);
static void igb_setup_dca(struct igb_adapter *);
static int igb_poll(struct napi_struct *, int);
static bool igb_clean_tx_irq(struct igb_q_vector *, int);
static int igb_clean_rx_irq(struct igb_q_vector *, int);
static int igb_ioctl(struct net_device *, struct ifreq *, int cmd);
static void igb_tx_timeout(struct net_device *, unsigned int txqueue);
static void igb_reset_task(struct work_struct *);
static void igb_vlan_mode(struct net_device *netdev
			  netdev_features_t features);
static int igb_vlan_rx_add_vid(struct net_device *, __be16, u16);
static int igb_vlan_rx_kill_vid(struct net_device *, __be16, u16);
static void igb_restore_vlan(struct igb_adapter *);
static void igb_rar_set_index(struct igb_adapter *, u32);
static void igb_ping_all_vfs(struct igb_adapter *);
static void igb_msg_task(struct igb_adapter *);
static void igb_vmm_control(struct igb_adapter *);
static int igb_set_vf_mac(struct igb_adapter *, int, unsigned char *);
static void igb_flush_mac_table(struct igb_adapter *);
static int igb_available_rars(struct igb_adapter *, u8);
static void igb_set_default_mac_filter(struct igb_adapter *);
static int igb_uc_sync(struct net_device *, const unsigned char *);
static int igb_uc_unsync(struct net_device *, const unsigned char *);
static void igb_restore_vf_multicasts(struct igb_adapter *adapter);
static int igb_ndo_set_vf_mac(struct net_device *netdev, int vf, u8 *mac);
static int igb_ndo_set_vf_vlan(struct net_device *netdev
			       int vf, u16 vlan, u8 qos, __be16 vlan_proto);
static int igb_ndo_set_vf_bw(struct net_device *, int, int, int);
static int igb_ndo_set_vf_spoofchk(struct net_device *netdev, int vf
				   bool setting);
static int igb_ndo_set_vf_trust(struct net_device *netdev, int vf
				bool setting);
static int igb_ndo_get_vf_config(struct net_device *netdev, int vf
				 struct ifla_vf_info *ivi);
static void igb_check_vf_rate_limit(struct igb_adapter *);
static void igb_nfc_filter_exit(struct igb_adapter *adapter);
static void igb_nfc_filter_restore(struct igb_adapter *adapter);
static int igb_vf_configure(struct igb_adapter *adapter, int vf);
static int igb_disable_sriov(struct pci_dev *dev, bool reinit);
static int igb_suspend(struct device *);
static int igb_resume(struct device *);
static int igb_runtime_suspend(struct device *dev);
static int igb_runtime_resume(struct device *dev);
static int igb_runtime_idle(struct device *dev);
static void igb_shutdown(struct pci_dev *);
static int igb_pci_sriov_configure(struct pci_dev *dev, int num_vfs);
static int igb_notify_dca(struct notifier_block *, unsigned long, void *);
static pci_ers_result_t igb_io_error_detected(struct pci_dev *
		     pci_channel_state_t);
static pci_ers_result_t igb_io_slot_reset(struct pci_dev *);
static void igb_io_resume(struct pci_dev *);
static void igb_init_dmac(struct igb_adapter *adapter, u32 pba);
static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter);
static void igb_ptp_sdp_init(struct igb_adapter *adapter);

> 
> Reviewed-by: Alan Brady <alan.brady@intel.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
> v2: address compilation failure when CONFIG_PM=n, which is then updated
>     in patch 2/2, fix alignment.
>     changes in v1 reviewed by Simon Horman
>     changes in v1 reviewed by Paul Menzel
> v1: original net-next posting
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 53 ++++++++++-------------
>  1 file changed, 24 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 518298bbdadc..e749bf5164b8 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -106,8 +106,6 @@ static int igb_setup_all_rx_resources(struct igb_adapter *);
>  static void igb_free_all_tx_resources(struct igb_adapter *);
>  static void igb_free_all_rx_resources(struct igb_adapter *);
>  static void igb_setup_mrqc(struct igb_adapter *);
> -static int igb_probe(struct pci_dev *, const struct pci_device_id *);
> -static void igb_remove(struct pci_dev *pdev);
>  static void igb_init_queue_configuration(struct igb_adapter *adapter);
>  static int igb_sw_init(struct igb_adapter *);
>  int igb_open(struct net_device *);
> @@ -178,20 +176,6 @@ static int igb_vf_configure(struct igb_adapter *adapter, int vf);
>  static int igb_disable_sriov(struct pci_dev *dev, bool reinit);
>  #endif
>  
> -static int igb_suspend(struct device *);
> -static int igb_resume(struct device *);
> -static int igb_runtime_suspend(struct device *dev);
> -static int igb_runtime_resume(struct device *dev);
> -static int igb_runtime_idle(struct device *dev);
> -#ifdef CONFIG_PM
> -static const struct dev_pm_ops igb_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
> -	SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
> -			igb_runtime_idle)
> -};
> -#endif
> -static void igb_shutdown(struct pci_dev *);
> -static int igb_pci_sriov_configure(struct pci_dev *dev, int num_vfs);
>  #ifdef CONFIG_IGB_DCA
>  static int igb_notify_dca(struct notifier_block *, unsigned long, void *);
>  static struct notifier_block dca_notifier = {
> @@ -219,19 +203,6 @@ static const struct pci_error_handlers igb_err_handler = {
>  
>  static void igb_init_dmac(struct igb_adapter *adapter, u32 pba);
>  
> -static struct pci_driver igb_driver = {
> -	.name     = igb_driver_name,
> -	.id_table = igb_pci_tbl,
> -	.probe    = igb_probe,
> -	.remove   = igb_remove,
> -#ifdef CONFIG_PM
> -	.driver.pm = &igb_pm_ops,
> -#endif
> -	.shutdown = igb_shutdown,
> -	.sriov_configure = igb_pci_sriov_configure,
> -	.err_handler = &igb_err_handler
> -};
> -
>  MODULE_AUTHOR("Intel Corporation, <e1000-devel@lists.sourceforge.net>");
>  MODULE_DESCRIPTION("Intel(R) Gigabit Ethernet Network Driver");
>  MODULE_LICENSE("GPL v2");
> @@ -647,6 +618,8 @@ struct net_device *igb_get_hw_dev(struct e1000_hw *hw)
>  	return adapter->netdev;
>  }
>  
> +static struct pci_driver igb_driver;
> +
>  /**
>   *  igb_init_module - Driver Registration Routine
>   *
> @@ -10170,4 +10143,26 @@ static void igb_nfc_filter_restore(struct igb_adapter *adapter)
>  
>  	spin_unlock(&adapter->nfc_lock);
>  }
> +
> +#ifdef CONFIG_PM
> +static const struct dev_pm_ops igb_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
> +	SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
> +			   igb_runtime_idle)
> +};
> +#endif
> +
> +static struct pci_driver igb_driver = {
> +	.name     = igb_driver_name,
> +	.id_table = igb_pci_tbl,
> +	.probe    = igb_probe,
> +	.remove   = igb_remove,
> +#ifdef CONFIG_PM
> +	.driver.pm = &igb_pm_ops,
> +#endif
> +	.shutdown = igb_shutdown,
> +	.sriov_configure = igb_pci_sriov_configure,
> +	.err_handler = &igb_err_handler
> +};
> +
>  /* igb_main.c */
> -- 
> 2.39.3
> 
> 

Re: [PATCH iwl-next v2 2/2] net: intel: implement modern PM ops declarations

[Maciej Fijalkowski]

On Tue, Mar 05, 2024 at 06:50:22PM -0800, Jesse Brandeburg wrote:
> Switch the Intel networking drivers to use the new power management ops
> declaration formats and macros, which allows us to drop __maybe_unused,
> as well as a bunch of ifdef checking CONFIG_PM.
> 
> This is safe to do because the compiler drops the unused functions,
> verified by checking for any of the power management function symbols
> being present in System.map for a build without CONFIG_PM.
> 
> If a driver has runtime PM, define the ops with pm_ptr(), and if the
> driver has Simple PM, use pm_sleep_ptr(), as well as the new versions of
> the macros for declaring the members of the pm_ops structs.
> 
> Checked with network-enabled allnoconfig, allyesconfig, allmodconfig on
> x64_64.
> 
> Reviewed-by: Alan Brady <alan.brady@intel.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
> v2: added ice driver changes to series
> v1: original net-next posting
>     all changes except for ice were reviewed by Simon Horman
>     no other changes besides to ice

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> ---
>  drivers/net/ethernet/intel/e100.c             |  8 +++---
>  drivers/net/ethernet/intel/e1000/e1000_main.c | 14 +++++-----
>  drivers/net/ethernet/intel/e1000e/netdev.c    | 22 +++++++---------
>  drivers/net/ethernet/intel/fm10k/fm10k_pci.c  | 10 +++----
>  drivers/net/ethernet/intel/i40e/i40e_main.c   | 10 +++----
>  drivers/net/ethernet/intel/iavf/iavf_main.c   |  8 +++---
>  drivers/net/ethernet/intel/ice/ice_main.c     | 12 +++------
>  drivers/net/ethernet/intel/igb/igb_main.c     | 26 +++++++------------
>  drivers/net/ethernet/intel/igbvf/netdev.c     |  6 ++---
>  drivers/net/ethernet/intel/igc/igc_main.c     | 24 ++++++-----------
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  8 +++---
>  .../net/ethernet/intel/ixgbevf/ixgbevf_main.c |  8 +++---
>  12 files changed, 64 insertions(+), 92 deletions(-)
> 

Re: [PATCH iwl-next v2 1/2] igb: simplify pci ops declaration

[Bjorn Helgaas]

[+cc Paul for __maybe_unused cleanup]

On Tue, Mar 05, 2024 at 06:50:21PM -0800, Jesse Brandeburg wrote:
> The igb driver was pre-declaring tons of functions just so that it could
> have an early declaration of the pci_driver struct.
> 
> Delete a bunch of the declarations and move the struct to the bottom of the
> file, after all the functions are declared.

Nice fix, that was always annoying.

Seems like there's an opportunity to drop some of the __maybe_unused
annotations:

  static int __maybe_unused igb_suspend(struct device *dev)

after 1a3c7bb08826 ("PM: core: Add new *_PM_OPS macros, deprecate old ones").

I don't know if SET_RUNTIME_PM_OPS() makes __maybe_unused unnecessary
or not.

> +#ifdef CONFIG_PM
> +static const struct dev_pm_ops igb_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
> +	SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
> +			   igb_runtime_idle)
> +};
> +#endif
> +
> +static struct pci_driver igb_driver = {
> +	.name     = igb_driver_name,
> +	.id_table = igb_pci_tbl,
> +	.probe    = igb_probe,
> +	.remove   = igb_remove,
> +#ifdef CONFIG_PM
> +	.driver.pm = &igb_pm_ops,
> +#endif
> +	.shutdown = igb_shutdown,
> +	.sriov_configure = igb_pci_sriov_configure,
> +	.err_handler = &igb_err_handler
> +};
> +
>  /* igb_main.c */
> -- 
> 2.39.3
> 

Re: [PATCH iwl-next v2 1/2] igb: simplify pci ops declaration

[Bjorn Helgaas]

On Wed, Mar 06, 2024 at 05:14:12PM -0600, Bjorn Helgaas wrote:
> [+cc Paul for __maybe_unused cleanup]
> 
> On Tue, Mar 05, 2024 at 06:50:21PM -0800, Jesse Brandeburg wrote:
> > The igb driver was pre-declaring tons of functions just so that it could
> > have an early declaration of the pci_driver struct.
> > 
> > Delete a bunch of the declarations and move the struct to the bottom of the
> > file, after all the functions are declared.
> 
> Nice fix, that was always annoying.
> 
> Seems like there's an opportunity to drop some of the __maybe_unused
> annotations:
> 
>   static int __maybe_unused igb_suspend(struct device *dev)
> 
> after 1a3c7bb08826 ("PM: core: Add new *_PM_OPS macros, deprecate old ones").
> 
> I don't know if SET_RUNTIME_PM_OPS() makes __maybe_unused unnecessary
> or not.

Sorry, should have read 2/2 first ;)

RE: [Intel-wired-lan] [PATCH iwl-next v2 1/2] igb: simplify pci ops declaration

[Pucha, HimasekharX Reddy]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Jesse Brandeburg
> Sent: Wednesday, March 6, 2024 8:20 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: pmenzel@molgen.mpg.de; netdev@vger.kernel.org; Eric Dumazet <edumazet@google.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Brady, Alan <alan.brady@intel.com>; horms@kernel.org; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>
> Subject: [Intel-wired-lan] [PATCH iwl-next v2 1/2] igb: simplify pci ops declaration
>
> The igb driver was pre-declaring tons of functions just so that it could have an early declaration of the pci_driver struct.
>
> Delete a bunch of the declarations and move the struct to the bottom of the file, after all the functions are declared.
>
> Reviewed-by: Alan Brady <alan.brady@intel.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
> v2: address compilation failure when CONFIG_PM=n, which is then updated
>     in patch 2/2, fix alignment.
>     changes in v1 reviewed by Simon Horman
>     changes in v1 reviewed by Paul Menzel
> v1: original net-next posting
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 53 ++++++++++-------------
>  1 file changed, 24 insertions(+), 29 deletions(-)
>

Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)

RE: [Intel-wired-lan] [PATCH iwl-next v2 2/2] net: intel: implement modern PM ops declarations

[Pucha, HimasekharX Reddy]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Jesse Brandeburg
> Sent: Wednesday, March 6, 2024 8:20 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: pmenzel@molgen.mpg.de; netdev@vger.kernel.org; Eric Dumazet <edumazet@google.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Brady, Alan <alan.brady@intel.com>; horms@kernel.org; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>
> Subject: [Intel-wired-lan] [PATCH iwl-next v2 2/2] net: intel: implement modern PM ops declarations
>
> Switch the Intel networking drivers to use the new power management ops declaration formats and macros, which allows us to drop __maybe_unused, as well as a bunch of ifdef checking CONFIG_PM.
>
> This is safe to do because the compiler drops the unused functions, verified by checking for any of the power management function symbols being present in System.map for a build without CONFIG_PM.
>
> If a driver has runtime PM, define the ops with pm_ptr(), and if the driver has Simple PM, use pm_sleep_ptr(), as well as the new versions of the macros for declaring the members of the pm_ops structs.
>
> Checked with network-enabled allnoconfig, allyesconfig, allmodconfig on x64_64.
>
> Reviewed-by: Alan Brady <alan.brady@intel.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
> v2: added ice driver changes to series
> v1: original net-next posting
>     all changes except for ice were reviewed by Simon Horman
>     no other changes besides to ice
> ---
>  drivers/net/ethernet/intel/e100.c             |  8 +++---
>  drivers/net/ethernet/intel/e1000/e1000_main.c | 14 +++++-----
>  drivers/net/ethernet/intel/e1000e/netdev.c    | 22 +++++++---------
>  drivers/net/ethernet/intel/fm10k/fm10k_pci.c  | 10 +++----
>  drivers/net/ethernet/intel/i40e/i40e_main.c   | 10 +++----
>  drivers/net/ethernet/intel/iavf/iavf_main.c   |  8 +++---
>  drivers/net/ethernet/intel/ice/ice_main.c     | 12 +++------
>  drivers/net/ethernet/intel/igb/igb_main.c     | 26 +++++++------------
>  drivers/net/ethernet/intel/igbvf/netdev.c     |  6 ++---
>  drivers/net/ethernet/intel/igc/igc_main.c     | 24 ++++++-----------
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  8 +++---  .../net/ethernet/intel/ixgbevf/ixgbevf_main.c |  8 +++---
>  12 files changed, 64 insertions(+), 92 deletions(-)
>

Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)