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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx]

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@xxxxxxxxx>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@xxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxx>");
   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




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux