Re: [PATCH iwl-next v4 5/5] ixgbe: Enable link management in E610 device

[Jiri Pirko <jiri@resnulli.us>]

Mon, Apr 22, 2024 at 03:06:11PM CEST, piotr.kwapulinski@intel.com wrote:

[...]


>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>index 559b443..ea6df1e 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>@@ -1,5 +1,5 @@
> /* SPDX-License-Identifier: GPL-2.0 */
>-/* Copyright(c) 1999 - 2018 Intel Corporation. */
>+/* Copyright(c) 1999 - 2024 Intel Corporation. */
> 
> #ifndef _IXGBE_H_
> #define _IXGBE_H_
>@@ -20,6 +20,7 @@
> #include "ixgbe_type.h"
> #include "ixgbe_common.h"
> #include "ixgbe_dcb.h"
>+#include "ixgbe_e610.h"
> #if IS_ENABLED(CONFIG_FCOE)
> #define IXGBE_FCOE
> #include "ixgbe_fcoe.h"
>@@ -28,7 +29,6 @@
> #include <linux/dca.h>
> #endif
> #include "ixgbe_ipsec.h"
>-

Leftover hunk?


> #include <net/xdp.h>
> 
> /* common prefix used by pr_<> macros */

[...]


>+static const struct ixgbe_mac_operations mac_ops_e610 = {
>+	.init_hw			= ixgbe_init_hw_generic,
>+	.start_hw			= ixgbe_start_hw_X540,
>+	.clear_hw_cntrs			= ixgbe_clear_hw_cntrs_generic,
>+	.enable_rx_dma			= ixgbe_enable_rx_dma_generic,
>+	.get_mac_addr			= ixgbe_get_mac_addr_generic,
>+	.get_device_caps		= ixgbe_get_device_caps_generic,
>+	.stop_adapter			= ixgbe_stop_adapter_generic,
>+	.set_lan_id			= ixgbe_set_lan_id_multi_port_pcie,
>+	.read_analog_reg8		= NULL,

Pointless initialization, it's null already. You have many cases of
this below.



>+	.write_analog_reg8		= NULL,
>+	.set_rxpba			= ixgbe_set_rxpba_generic,
>+	.check_link			= ixgbe_check_link_e610,
>+	.blink_led_start		= ixgbe_blink_led_start_X540,
>+	.blink_led_stop			= ixgbe_blink_led_stop_X540,
>+	.set_rar			= ixgbe_set_rar_generic,
>+	.clear_rar			= ixgbe_clear_rar_generic,
>+	.set_vmdq			= ixgbe_set_vmdq_generic,
>+	.set_vmdq_san_mac		= ixgbe_set_vmdq_san_mac_generic,
>+	.clear_vmdq			= ixgbe_clear_vmdq_generic,
>+	.init_rx_addrs			= ixgbe_init_rx_addrs_generic,
>+	.update_mc_addr_list		= ixgbe_update_mc_addr_list_generic,
>+	.enable_mc			= ixgbe_enable_mc_generic,
>+	.disable_mc			= ixgbe_disable_mc_generic,
>+	.clear_vfta			= ixgbe_clear_vfta_generic,
>+	.set_vfta			= ixgbe_set_vfta_generic,
>+	.fc_enable			= ixgbe_fc_enable_generic,
>+	.set_fw_drv_ver			= ixgbe_set_fw_drv_ver_x550,
>+	.init_uta_tables		= ixgbe_init_uta_tables_generic,
>+	.set_mac_anti_spoofing		= ixgbe_set_mac_anti_spoofing,
>+	.set_vlan_anti_spoofing		= ixgbe_set_vlan_anti_spoofing,
>+	.set_source_address_pruning	=
>+				ixgbe_set_source_address_pruning_x550,
>+	.set_ethertype_anti_spoofing	=
>+				ixgbe_set_ethertype_anti_spoofing_x550,
>+	.disable_rx_buff		= ixgbe_disable_rx_buff_generic,
>+	.enable_rx_buff			= ixgbe_enable_rx_buff_generic,
>+	.get_thermal_sensor_data	= NULL,
>+	.init_thermal_sensor_thresh	= NULL,
>+	.fw_recovery_mode		= NULL,
>+	.enable_rx			= ixgbe_enable_rx_generic,
>+	.disable_rx			= ixgbe_disable_rx_e610,
>+	.led_on				= ixgbe_led_on_generic,
>+	.led_off			= ixgbe_led_off_generic,
>+	.init_led_link_act		= ixgbe_init_led_link_act_generic,
>+	.reset_hw			= ixgbe_reset_hw_e610,
>+	.get_media_type			= ixgbe_get_media_type_e610,
>+	.get_san_mac_addr		= NULL,
>+	.get_wwn_prefix			= NULL,
>+	.setup_link			= ixgbe_setup_link_e610,
>+	.get_link_capabilities		= ixgbe_get_link_capabilities_e610,
>+	.get_bus_info			= ixgbe_get_bus_info_generic,
>+	.setup_sfp			= NULL,
>+	.acquire_swfw_sync		= ixgbe_acquire_swfw_sync_X540,
>+	.release_swfw_sync		= ixgbe_release_swfw_sync_X540,
>+	.init_swfw_sync			= ixgbe_init_swfw_sync_X540,
>+	.prot_autoc_read		= prot_autoc_read_generic,
>+	.prot_autoc_write		= prot_autoc_write_generic,
>+	.setup_fc			= ixgbe_setup_fc_e610,
>+	.fc_autoneg			= ixgbe_fc_autoneg_e610,
>+};
>+
>+static const struct ixgbe_phy_operations phy_ops_e610 = {
>+	.init				= ixgbe_init_phy_ops_e610,
>+	.identify			= ixgbe_identify_phy_e610,
>+	.read_reg			= NULL,
>+	.write_reg			= NULL,
>+	.read_reg_mdi			= NULL,
>+	.write_reg_mdi			= NULL,
>+	.identify_sfp			= ixgbe_identify_module_e610,
>+	.reset				= NULL,
>+	.setup_link_speed		= ixgbe_setup_phy_link_speed_generic,
>+	.read_i2c_byte			= NULL,
>+	.write_i2c_byte			= NULL,
>+	.read_i2c_sff8472		= NULL,
>+	.read_i2c_eeprom		= NULL,
>+	.write_i2c_eeprom		= NULL,
>+	.setup_link			= ixgbe_setup_phy_link_e610,
>+	.set_phy_power			= NULL,
>+	.check_overtemp			= NULL,
>+	.enter_lplu			= ixgbe_enter_lplu_e610,
>+	.handle_lasi			= NULL,
>+	.read_i2c_byte_unlocked		= NULL,
>+};
>+
>+static const struct ixgbe_eeprom_operations eeprom_ops_e610 = {
>+	.init_params			= NULL,
>+	.read				= ixgbe_read_ee_aci_e610,
>+	.read_buffer			= NULL,
>+	.write				= NULL,
>+	.write_buffer			= NULL,
>+	.validate_checksum		= ixgbe_validate_eeprom_checksum_e610,
>+	.update_checksum		= NULL,
>+	.calc_checksum			= NULL,
>+};
>+

[...]


>+/**
>+ * ixgbe_process_link_status_event - process the link event
>+ * @adapter: pointer to adapter structure
>+ * @link_up: true if the physical link is up and false if it is down
>+ * @link_speed: current link speed received from the link event
>+ *
>+ * Return: 0 on success and negative on failure.
>+ */
>+static int
>+ixgbe_process_link_status_event(struct ixgbe_adapter *adapter, bool link_up,
>+				u16 link_speed)
>+{
>+	struct ixgbe_hw *hw = &adapter->hw;
>+	int status;
>+
>+	/* update the link info structures and re-enable link events,
>+	 * don't bail on failure due to other book keeping needed

Why don't you start the sentence with capital letter and end with "."?


>+	 */
>+	status = ixgbe_update_link_info(hw);
>+	if (status)
>+		e_dev_err("Failed to update link status, err %d aq_err %d\n",
>+			  status, hw->aci.last_status);
>+
>+	ixgbe_check_link_cfg_err(adapter, hw->link.link_info.link_cfg_err);
>+
>+	/* Check if the link state is up after updating link info, and treat
>+	 * this event as an UP event since the link is actually UP now.
>+	 */
>+	if (hw->link.link_info.link_info & IXGBE_ACI_LINK_UP)
>+		link_up = true;
>+
>+	/* turn off PHY if media was removed */
>+	if (!(adapter->flags2 & IXGBE_FLAG2_NO_MEDIA) &&
>+	    !(hw->link.link_info.link_info & IXGBE_ACI_MEDIA_AVAILABLE)) {
>+		(adapter->flags2 |= IXGBE_FLAG2_NO_MEDIA);
>+		if (ixgbe_aci_set_link_restart_an(hw, false))
>+			e_dev_err("can't set link to OFF\n");
>+	}

[...]