>-----Original Message-----
>From: Jiri Pirko <[email protected]>
>Sent: Monday, April 22, 2024 4:26 PM
>To: Kwapulinski, Piotr <[email protected]>
>Cc: [email protected]; [email protected];
>[email protected]; Wyborny, Carolyn <[email protected]>; Jagielski,
>Jedrzej <[email protected]>; Glaza, Jan <[email protected]>
>Subject: Re: [PATCH iwl-next v4 5/5] ixgbe: Enable link management in E610
>device
>
>Mon, Apr 22, 2024 at 03:06:11PM CEST, [email protected] 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?
Will do
>
>
>> #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.
Will do
>
>
>
>>+ .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 "."?
Will do.
Thank you for review.
>
>
>>+ */
>>+ 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");
>>+ }
>
>[...]
