Message-ID: <Zeh8qadiTGf413YU@boxer>
Date: Wed, 6 Mar 2024 15:24:41 +0100
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Jesse Brandeburg <jesse.brandeburg@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<horms@...nel.org>, <pmenzel@...gen.mpg.de>, Alan Brady
	<alan.brady@...el.com>, Tony Nguyen <anthony.l.nguyen@...el.com>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, "Jakub
 Kicinski" <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH iwl-next v2 1/2] igb: simplify pci ops declaration

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@...el.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@...el.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.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@...ts.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
> 
> 

Powered by blists - more mailing lists