| Message ID | 20240306025023.800029-2-jesse.brandeburg@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | net: intel: cleanup power ops | expand |
[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
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 >
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
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 > >
[+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 >
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 ;)
> -----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)
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 */