[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2025-01-07 (ice, igc)

[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2025-01-07 (ice, igc)

[Tony Nguyen]

For ice:

Arkadiusz corrects mask value being used to determine DPLL phase range.

Przemyslaw corrects frequency value for E823 devices.

For igc:

En-Wei Wu adds a check and, early, return for failed register read.

The following are changes since commit fd48f071a3d6d51e737e953bb43fe69785cf59a9:
  net: don't dump Tx and uninitialized NAPIs
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 100GbE

Arkadiusz Kubalewski (1):
  ice: fix max values for dpll pin phase adjust

En-Wei Wu (1):
  igc: return early when failing to read EECD register

Przemyslaw Korba (1):
  ice: fix incorrect PHY settings for 100 GB/s

 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  2 ++
 drivers/net/ethernet/intel/ice/ice_dpll.c     | 35 ++++++++++++-------
 .../net/ethernet/intel/ice/ice_ptp_consts.h   |  4 +--
 drivers/net/ethernet/intel/igc/igc_base.c     |  6 ++++
 4 files changed, 33 insertions(+), 14 deletions(-)

-- 
2.47.1

[PATCH net 1/3] ice: fix max values for dpll pin phase adjust

[Tony Nguyen]

From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

Mask admin command returned max phase adjust value for both input and
output pins. Only 31 bits are relevant, last released data sheet wrongly
points that 32 bits are valid - see [1] 3.2.6.4.1 Get CCU Capabilities
Command for reference. Fix of the datasheet itself is in progress.

Fix the min/max assignment logic, previously the value was wrongly
considered as negative value due to most significant bit being set.

Example of previous broken behavior:
$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \
--do pin-get --json '{"id":1}'| grep phase-adjust
 'phase-adjust': 0,
 'phase-adjust-max': 16723,
 'phase-adjust-min': -16723,

Correct behavior with the fix:
$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \
--do pin-get --json '{"id":1}'| grep phase-adjust
 'phase-adjust': 0,
 'phase-adjust-max': 2147466925,
 'phase-adjust-min': -2147466925,

[1] https://cdrdv2.intel.com/v1/dl/getContent/613875?explicitVersion=true

Fixes: 90e1c90750d7 ("ice: dpll: implement phase related callbacks")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  2 ++
 drivers/net/ethernet/intel/ice/ice_dpll.c     | 35 ++++++++++++-------
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 1489a8ceec51..ef14cff9a333 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -2264,6 +2264,8 @@ struct ice_aqc_get_pkg_info_resp {
 	struct ice_aqc_get_pkg_info pkg_info[];
 };
 
+#define ICE_AQC_GET_CGU_MAX_PHASE_ADJ	GENMASK(30, 0)
+
 /* Get CGU abilities command response data structure (indirect 0x0C61) */
 struct ice_aqc_get_cgu_abilities {
 	u8 num_inputs;
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
index d5ad6d84007c..38e151c7ea23 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -2064,6 +2064,18 @@ static int ice_dpll_init_worker(struct ice_pf *pf)
 	return 0;
 }
 
+/**
+ * ice_dpll_phase_range_set - initialize phase adjust range helper
+ * @range: pointer to phase adjust range struct to be initialized
+ * @phase_adj: a value to be used as min(-)/max(+) boundary
+ */
+static void ice_dpll_phase_range_set(struct dpll_pin_phase_adjust_range *range,
+				     u32 phase_adj)
+{
+	range->min = -phase_adj;
+	range->max = phase_adj;
+}
+
 /**
  * ice_dpll_init_info_pins_generic - initializes generic pins info
  * @pf: board private structure
@@ -2105,8 +2117,8 @@ static int ice_dpll_init_info_pins_generic(struct ice_pf *pf, bool input)
 	for (i = 0; i < pin_num; i++) {
 		pins[i].idx = i;
 		pins[i].prop.board_label = labels[i];
-		pins[i].prop.phase_range.min = phase_adj_max;
-		pins[i].prop.phase_range.max = -phase_adj_max;
+		ice_dpll_phase_range_set(&pins[i].prop.phase_range,
+					 phase_adj_max);
 		pins[i].prop.capabilities = cap;
 		pins[i].pf = pf;
 		ret = ice_dpll_pin_state_update(pf, &pins[i], pin_type, NULL);
@@ -2152,6 +2164,7 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
 	struct ice_hw *hw = &pf->hw;
 	struct ice_dpll_pin *pins;
 	unsigned long caps;
+	u32 phase_adj_max;
 	u8 freq_supp_num;
 	bool input;
 
@@ -2159,11 +2172,13 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
 	case ICE_DPLL_PIN_TYPE_INPUT:
 		pins = pf->dplls.inputs;
 		num_pins = pf->dplls.num_inputs;
+		phase_adj_max = pf->dplls.input_phase_adj_max;
 		input = true;
 		break;
 	case ICE_DPLL_PIN_TYPE_OUTPUT:
 		pins = pf->dplls.outputs;
 		num_pins = pf->dplls.num_outputs;
+		phase_adj_max = pf->dplls.output_phase_adj_max;
 		input = false;
 		break;
 	default:
@@ -2188,19 +2203,13 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
 				return ret;
 			caps |= (DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE |
 				 DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE);
-			pins[i].prop.phase_range.min =
-				pf->dplls.input_phase_adj_max;
-			pins[i].prop.phase_range.max =
-				-pf->dplls.input_phase_adj_max;
 		} else {
-			pins[i].prop.phase_range.min =
-				pf->dplls.output_phase_adj_max;
-			pins[i].prop.phase_range.max =
-				-pf->dplls.output_phase_adj_max;
 			ret = ice_cgu_get_output_pin_state_caps(hw, i, &caps);
 			if (ret)
 				return ret;
 		}
+		ice_dpll_phase_range_set(&pins[i].prop.phase_range,
+					 phase_adj_max);
 		pins[i].prop.capabilities = caps;
 		ret = ice_dpll_pin_state_update(pf, &pins[i], pin_type, NULL);
 		if (ret)
@@ -2308,8 +2317,10 @@ static int ice_dpll_init_info(struct ice_pf *pf, bool cgu)
 	dp->dpll_idx = abilities.pps_dpll_idx;
 	d->num_inputs = abilities.num_inputs;
 	d->num_outputs = abilities.num_outputs;
-	d->input_phase_adj_max = le32_to_cpu(abilities.max_in_phase_adj);
-	d->output_phase_adj_max = le32_to_cpu(abilities.max_out_phase_adj);
+	d->input_phase_adj_max = le32_to_cpu(abilities.max_in_phase_adj) &
+		ICE_AQC_GET_CGU_MAX_PHASE_ADJ;
+	d->output_phase_adj_max = le32_to_cpu(abilities.max_out_phase_adj) &
+		ICE_AQC_GET_CGU_MAX_PHASE_ADJ;
 
 	alloc_size = sizeof(*d->inputs) * d->num_inputs;
 	d->inputs = kzalloc(alloc_size, GFP_KERNEL);
-- 
2.47.1

[PATCH net 2/3] ice: fix incorrect PHY settings for 100 GB/s

[Tony Nguyen]

From: Przemyslaw Korba <przemyslaw.korba@intel.com>

ptp4l application reports too high offset when ran on E823 device
with a 100GB/s link. Those values cannot go under 100ns, like in a
working case when using 100 GB/s cable.

This is due to incorrect frequency settings on the PHY clocks for
100 GB/s speed. Changes are introduced to align with the internal
hardware documentation, and correctly initialize frequency in PHY
clocks with the frequency values that are in our HW spec.

To reproduce the issue run ptp4l as a Time Receiver on E823 device,
and observe the offset, which will never approach values seen
in the PTP working case.

Reproduction output:
ptp4l -i enp137s0f3 -m -2 -s -f /etc/ptp4l_8275.conf
ptp4l[5278.775]: master offset      12470 s2 freq  +41288 path delay -3002
ptp4l[5278.837]: master offset      10525 s2 freq  +39202 path delay -3002
ptp4l[5278.900]: master offset     -24840 s2 freq  -20130 path delay -3002
ptp4l[5278.963]: master offset      10597 s2 freq  +37908 path delay -3002
ptp4l[5279.025]: master offset       8883 s2 freq  +36031 path delay -3002
ptp4l[5279.088]: master offset       7267 s2 freq  +34151 path delay -3002
ptp4l[5279.150]: master offset       5771 s2 freq  +32316 path delay -3002
ptp4l[5279.213]: master offset       4388 s2 freq  +30526 path delay -3002
ptp4l[5279.275]: master offset     -30434 s2 freq  -28485 path delay -3002
ptp4l[5279.338]: master offset     -28041 s2 freq  -27412 path delay -3002
ptp4l[5279.400]: master offset       7870 s2 freq  +31118 path delay -3002

Fixes: 3a7496234d17 ("ice: implement basic E822 PTP support")
Reviewed-by: Milena Olech <milena.olech@intel.com>
Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_consts.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_consts.h b/drivers/net/ethernet/intel/ice/ice_ptp_consts.h
index 585ce200c60f..d75f0eddd631 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_consts.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_consts.h
@@ -761,9 +761,9 @@ const struct ice_vernier_info_e82x e822_vernier[NUM_ICE_PTP_LNK_SPD] = {
 		/* rx_desk_rsgb_par */
 		644531250, /* 644.53125 MHz Reed Solomon gearbox */
 		/* tx_desk_rsgb_pcs */
-		644531250, /* 644.53125 MHz Reed Solomon gearbox */
+		390625000, /* 390.625 MHz Reed Solomon gearbox */
 		/* rx_desk_rsgb_pcs */
-		644531250, /* 644.53125 MHz Reed Solomon gearbox */
+		390625000, /* 390.625 MHz Reed Solomon gearbox */
 		/* tx_fixed_delay */
 		1620,
 		/* pmd_adj_divisor */
-- 
2.47.1

Re: [PATCH net 2/3] ice: fix incorrect PHY settings for 100 GB/s

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 2127 bytes --]

On Wed, Jan 8, 2025 at 12:32 AM Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
>
> From: Przemyslaw Korba <przemyslaw.korba@intel.com>
>
> ptp4l application reports too high offset when ran on E823 device
> with a 100GB/s link. Those values cannot go under 100ns, like in a
> working case when using 100 GB/s cable.
>
> This is due to incorrect frequency settings on the PHY clocks for
> 100 GB/s speed. Changes are introduced to align with the internal
> hardware documentation, and correctly initialize frequency in PHY
> clocks with the frequency values that are in our HW spec.
>
> To reproduce the issue run ptp4l as a Time Receiver on E823 device,
> and observe the offset, which will never approach values seen
> in the PTP working case.
>
> Reproduction output:
> ptp4l -i enp137s0f3 -m -2 -s -f /etc/ptp4l_8275.conf
> ptp4l[5278.775]: master offset      12470 s2 freq  +41288 path delay -3002
> ptp4l[5278.837]: master offset      10525 s2 freq  +39202 path delay -3002
> ptp4l[5278.900]: master offset     -24840 s2 freq  -20130 path delay -3002
> ptp4l[5278.963]: master offset      10597 s2 freq  +37908 path delay -3002
> ptp4l[5279.025]: master offset       8883 s2 freq  +36031 path delay -3002
> ptp4l[5279.088]: master offset       7267 s2 freq  +34151 path delay -3002
> ptp4l[5279.150]: master offset       5771 s2 freq  +32316 path delay -3002
> ptp4l[5279.213]: master offset       4388 s2 freq  +30526 path delay -3002
> ptp4l[5279.275]: master offset     -30434 s2 freq  -28485 path delay -3002
> ptp4l[5279.338]: master offset     -28041 s2 freq  -27412 path delay -3002
> ptp4l[5279.400]: master offset       7870 s2 freq  +31118 path delay -3002
>
> Fixes: 3a7496234d17 ("ice: implement basic E822 PTP support")
> Reviewed-by: Milena Olech <milena.olech@intel.com>
> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
> Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
LGTM,
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>


-- 
Regards,
Kalesh AP

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

[PATCH net 3/3] igc: return early when failing to read EECD register

[Tony Nguyen]

From: En-Wei Wu <en-wei.wu@canonical.com>

When booting with a dock connected, the igc driver may get stuck for ~40
seconds if PCIe link is lost during initialization.

This happens because the driver access device after EECD register reads
return all F's, indicating failed reads. Consequently, hw->hw_addr is set
to NULL, which impacts subsequent rd32() reads. This leads to the driver
hanging in igc_get_hw_semaphore_i225(), as the invalid hw->hw_addr
prevents retrieving the expected value.

To address this, a validation check and a corresponding return value
catch is added for the EECD register read result. If all F's are
returned, indicating PCIe link loss, the driver will return -ENXIO
immediately. This avoids the 40-second hang and significantly improves
boot time when using a dock with an igc NIC.

Log before the patch:
[    0.911913] igc 0000:70:00.0: enabling device (0000 -> 0002)
[    0.912386] igc 0000:70:00.0: PTM enabled, 4ns granularity
[    1.571098] igc 0000:70:00.0 (unnamed net_device) (uninitialized): PCIe link lost, device now detached
[   43.449095] igc_get_hw_semaphore_i225: igc 0000:70:00.0 (unnamed net_device) (uninitialized): Driver can't access device - SMBI bit is set.
[   43.449186] igc 0000:70:00.0: probe with driver igc failed with error -13
[   46.345701] igc 0000:70:00.0: enabling device (0000 -> 0002)
[   46.345777] igc 0000:70:00.0: PTM enabled, 4ns granularity

Log after the patch:
[    1.031000] igc 0000:70:00.0: enabling device (0000 -> 0002)
[    1.032097] igc 0000:70:00.0: PTM enabled, 4ns granularity
[    1.642291] igc 0000:70:00.0 (unnamed net_device) (uninitialized): PCIe link lost, device now detached
[    5.480490] igc 0000:70:00.0: enabling device (0000 -> 0002)
[    5.480516] igc 0000:70:00.0: PTM enabled, 4ns granularity

Fixes: ab4056126813 ("igc: Add NVM support")
Cc: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
Reviewed-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
Tested-by: Mor Bar-Gabay <morx.bar.gabay@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_base.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc_base.c b/drivers/net/ethernet/intel/igc/igc_base.c
index 9fae8bdec2a7..1613b562d17c 100644
--- a/drivers/net/ethernet/intel/igc/igc_base.c
+++ b/drivers/net/ethernet/intel/igc/igc_base.c
@@ -68,6 +68,10 @@ static s32 igc_init_nvm_params_base(struct igc_hw *hw)
 	u32 eecd = rd32(IGC_EECD);
 	u16 size;
 
+	/* failed to read reg and got all F's */
+	if (!(~eecd))
+		return -ENXIO;
+
 	size = FIELD_GET(IGC_EECD_SIZE_EX_MASK, eecd);
 
 	/* Added to a constant, "size" becomes the left-shift value
@@ -221,6 +225,8 @@ static s32 igc_get_invariants_base(struct igc_hw *hw)
 
 	/* NVM initialization */
 	ret_val = igc_init_nvm_params_base(hw);
+	if (ret_val)
+		goto out;
 	switch (hw->mac.type) {
 	case igc_i225:
 		ret_val = igc_init_nvm_params_i225(hw);
-- 
2.47.1

Re: [PATCH net 3/3] igc: return early when failing to read EECD register

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 2405 bytes --]

On Wed, Jan 8, 2025 at 12:32 AM Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
>
> From: En-Wei Wu <en-wei.wu@canonical.com>
>
> When booting with a dock connected, the igc driver may get stuck for ~40
> seconds if PCIe link is lost during initialization.
>
> This happens because the driver access device after EECD register reads
> return all F's, indicating failed reads. Consequently, hw->hw_addr is set
> to NULL, which impacts subsequent rd32() reads. This leads to the driver
> hanging in igc_get_hw_semaphore_i225(), as the invalid hw->hw_addr
> prevents retrieving the expected value.
>
> To address this, a validation check and a corresponding return value
> catch is added for the EECD register read result. If all F's are
> returned, indicating PCIe link loss, the driver will return -ENXIO
> immediately. This avoids the 40-second hang and significantly improves
> boot time when using a dock with an igc NIC.
>
> Log before the patch:
> [    0.911913] igc 0000:70:00.0: enabling device (0000 -> 0002)
> [    0.912386] igc 0000:70:00.0: PTM enabled, 4ns granularity
> [    1.571098] igc 0000:70:00.0 (unnamed net_device) (uninitialized): PCIe link lost, device now detached
> [   43.449095] igc_get_hw_semaphore_i225: igc 0000:70:00.0 (unnamed net_device) (uninitialized): Driver can't access device - SMBI bit is set.
> [   43.449186] igc 0000:70:00.0: probe with driver igc failed with error -13
> [   46.345701] igc 0000:70:00.0: enabling device (0000 -> 0002)
> [   46.345777] igc 0000:70:00.0: PTM enabled, 4ns granularity
>
> Log after the patch:
> [    1.031000] igc 0000:70:00.0: enabling device (0000 -> 0002)
> [    1.032097] igc 0000:70:00.0: PTM enabled, 4ns granularity
> [    1.642291] igc 0000:70:00.0 (unnamed net_device) (uninitialized): PCIe link lost, device now detached
> [    5.480490] igc 0000:70:00.0: enabling device (0000 -> 0002)
> [    5.480516] igc 0000:70:00.0: PTM enabled, 4ns granularity
>
> Fixes: ab4056126813 ("igc: Add NVM support")
> Cc: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
> Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
> Reviewed-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Tested-by: Mor Bar-Gabay <morx.bar.gabay@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
LGTM,
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>


-- 
Regards,
Kalesh AP

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

Re: [PATCH net 3/3] igc: return early when failing to read EECD register

[Przemek Kitszel]

On 1/7/25 20:01, Tony Nguyen wrote:
> From: En-Wei Wu <en-wei.wu@canonical.com>
> 
> When booting with a dock connected, the igc driver may get stuck for ~40
> seconds if PCIe link is lost during initialization.
> 
> This happens because the driver access device after EECD register reads
> return all F's, indicating failed reads. Consequently, hw->hw_addr is set
> to NULL, which impacts subsequent rd32() reads. This leads to the driver
> hanging in igc_get_hw_semaphore_i225(), as the invalid hw->hw_addr
> prevents retrieving the expected value.
> 
> To address this, a validation check and a corresponding return value
> catch is added for the EECD register read result. If all F's are
> returned, indicating PCIe link loss, the driver will return -ENXIO
> immediately. This avoids the 40-second hang and significantly improves
> boot time when using a dock with an igc NIC.
> 
> Log before the patch:
> [    0.911913] igc 0000:70:00.0: enabling device (0000 -> 0002)
> [    0.912386] igc 0000:70:00.0: PTM enabled, 4ns granularity
> [    1.571098] igc 0000:70:00.0 (unnamed net_device) (uninitialized): PCIe link lost, device now detached
> [   43.449095] igc_get_hw_semaphore_i225: igc 0000:70:00.0 (unnamed net_device) (uninitialized): Driver can't access device - SMBI bit is set.
> [   43.449186] igc 0000:70:00.0: probe with driver igc failed with error -13
> [   46.345701] igc 0000:70:00.0: enabling device (0000 -> 0002)
> [   46.345777] igc 0000:70:00.0: PTM enabled, 4ns granularity
> 
> Log after the patch:
> [    1.031000] igc 0000:70:00.0: enabling device (0000 -> 0002)
> [    1.032097] igc 0000:70:00.0: PTM enabled, 4ns granularity
> [    1.642291] igc 0000:70:00.0 (unnamed net_device) (uninitialized): PCIe link lost, device now detached
> [    5.480490] igc 0000:70:00.0: enabling device (0000 -> 0002)
> [    5.480516] igc 0000:70:00.0: PTM enabled, 4ns granularity
> 
> Fixes: ab4056126813 ("igc: Add NVM support")
> Cc: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
> Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
> Reviewed-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Tested-by: Mor Bar-Gabay <morx.bar.gabay@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

Thank you,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

> ---
>   drivers/net/ethernet/intel/igc/igc_base.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_base.c b/drivers/net/ethernet/intel/igc/igc_base.c
> index 9fae8bdec2a7..1613b562d17c 100644
> --- a/drivers/net/ethernet/intel/igc/igc_base.c
> +++ b/drivers/net/ethernet/intel/igc/igc_base.c
> @@ -68,6 +68,10 @@ static s32 igc_init_nvm_params_base(struct igc_hw *hw)
>   	u32 eecd = rd32(IGC_EECD);
>   	u16 size;
>   
> +	/* failed to read reg and got all F's */
> +	if (!(~eecd))
> +		return -ENXIO;
> +
>   	size = FIELD_GET(IGC_EECD_SIZE_EX_MASK, eecd);
>   
>   	/* Added to a constant, "size" becomes the left-shift value
> @@ -221,6 +225,8 @@ static s32 igc_get_invariants_base(struct igc_hw *hw)
>   
>   	/* NVM initialization */
>   	ret_val = igc_init_nvm_params_base(hw);
> +	if (ret_val)
> +		goto out;
>   	switch (hw->mac.type) {
>   	case igc_i225:
>   		ret_val = igc_init_nvm_params_i225(hw);

Re: [PATCH net 3/3] igc: return early when failing to read EECD register

[Bjorn Helgaas]

[+cc Kalesh, Przemek, linux-pci]

On Tue, Jan 07, 2025 at 11:01:47AM -0800, Tony Nguyen wrote:
> From: En-Wei Wu <en-wei.wu@canonical.com>
> 
> When booting with a dock connected, the igc driver may get stuck for ~40
> seconds if PCIe link is lost during initialization.
> 
> This happens because the driver access device after EECD register reads
> return all F's, indicating failed reads. Consequently, hw->hw_addr is set
> to NULL, which impacts subsequent rd32() reads. This leads to the driver
> hanging in igc_get_hw_semaphore_i225(), as the invalid hw->hw_addr
> prevents retrieving the expected value.
> 
> To address this, a validation check and a corresponding return value
> catch is added for the EECD register read result. If all F's are
> returned, indicating PCIe link loss, the driver will return -ENXIO
> immediately. This avoids the 40-second hang and significantly improves
> boot time when using a dock with an igc NIC.
> 
> Log before the patch:
> [    0.911913] igc 0000:70:00.0: enabling device (0000 -> 0002)
> [    0.912386] igc 0000:70:00.0: PTM enabled, 4ns granularity
> [    1.571098] igc 0000:70:00.0 (unnamed net_device) (uninitialized): PCIe link lost, device now detached
> [   43.449095] igc_get_hw_semaphore_i225: igc 0000:70:00.0 (unnamed net_device) (uninitialized): Driver can't access device - SMBI bit is set.
> [   43.449186] igc 0000:70:00.0: probe with driver igc failed with error -13
> [   46.345701] igc 0000:70:00.0: enabling device (0000 -> 0002)
> [   46.345777] igc 0000:70:00.0: PTM enabled, 4ns granularity
> 
> Log after the patch:
> [    1.031000] igc 0000:70:00.0: enabling device (0000 -> 0002)
> [    1.032097] igc 0000:70:00.0: PTM enabled, 4ns granularity
> [    1.642291] igc 0000:70:00.0 (unnamed net_device) (uninitialized): PCIe link lost, device now detached
> [    5.480490] igc 0000:70:00.0: enabling device (0000 -> 0002)
> [    5.480516] igc 0000:70:00.0: PTM enabled, 4ns granularity
> 
> Fixes: ab4056126813 ("igc: Add NVM support")
> Cc: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
> Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
> Reviewed-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Tested-by: Mor Bar-Gabay <morx.bar.gabay@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_base.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_base.c b/drivers/net/ethernet/intel/igc/igc_base.c
> index 9fae8bdec2a7..1613b562d17c 100644
> --- a/drivers/net/ethernet/intel/igc/igc_base.c
> +++ b/drivers/net/ethernet/intel/igc/igc_base.c
> @@ -68,6 +68,10 @@ static s32 igc_init_nvm_params_base(struct igc_hw *hw)
>  	u32 eecd = rd32(IGC_EECD);
>  	u16 size;
>  
> +	/* failed to read reg and got all F's */
> +	if (!(~eecd))
> +		return -ENXIO;

I don't understand this.  It looks like a band-aid that makes boot
faster but doesn't solve the real problem.

In its defense, I guess that with this patch, the first igc probe
fails, and then for some reason we attempt another a few seconds
later, and the second igc probe works fine, so the NIC actually does
end up working correct, right?

I think the PCI core has some issues with configuring ASPM L1.2, and I
wonder if those are relevant here.  If somebody can repro the problem
(i.e., without this patch, which looks like it appeared in v6.13 as
bd2776e39c2a ("igc: return early when failing to read EECD
register")), I wonder if you could try booting with "pcie_port_pm=off
pcie_aspm.policy=performance" and see if that also avoids the problem?

If so, I'd like to see the dmesg log with "pci=earlydump" and the
"sudo lspci -vv" output when booted with and without "pcie_port_pm=off
pcie_aspm.policy=performance".

>  	size = FIELD_GET(IGC_EECD_SIZE_EX_MASK, eecd);
>  
>  	/* Added to a constant, "size" becomes the left-shift value
> @@ -221,6 +225,8 @@ static s32 igc_get_invariants_base(struct igc_hw *hw)
>  
>  	/* NVM initialization */
>  	ret_val = igc_init_nvm_params_base(hw);
> +	if (ret_val)
> +		goto out;
>  	switch (hw->mac.type) {
>  	case igc_i225:
>  		ret_val = igc_init_nvm_params_i225(hw);
> -- 
> 2.47.1
> 

Re: [PATCH net 3/3] igc: return early when failing to read EECD register

[Tony Nguyen]

On 4/18/2025 12:34 PM, Bjorn Helgaas wrote:
> [+cc Kalesh, Przemek, linux-pci]
> 
> On Tue, Jan 07, 2025 at 11:01:47AM -0800, Tony Nguyen wrote:
>> From: En-Wei Wu <en-wei.wu@canonical.com>
>>
>> When booting with a dock connected, the igc driver may get stuck for ~40
>> seconds if PCIe link is lost during initialization.
>>
>> This happens because the driver access device after EECD register reads
>> return all F's, indicating failed reads. Consequently, hw->hw_addr is set
>> to NULL, which impacts subsequent rd32() reads. This leads to the driver
>> hanging in igc_get_hw_semaphore_i225(), as the invalid hw->hw_addr
>> prevents retrieving the expected value.
>>
>> To address this, a validation check and a corresponding return value
>> catch is added for the EECD register read result. If all F's are
>> returned, indicating PCIe link loss, the driver will return -ENXIO
>> immediately. This avoids the 40-second hang and significantly improves
>> boot time when using a dock with an igc NIC.
>>
>> Log before the patch:
>> [    0.911913] igc 0000:70:00.0: enabling device (0000 -> 0002)
>> [    0.912386] igc 0000:70:00.0: PTM enabled, 4ns granularity
>> [    1.571098] igc 0000:70:00.0 (unnamed net_device) (uninitialized): PCIe link lost, device now detached
>> [   43.449095] igc_get_hw_semaphore_i225: igc 0000:70:00.0 (unnamed net_device) (uninitialized): Driver can't access device - SMBI bit is set.
>> [   43.449186] igc 0000:70:00.0: probe with driver igc failed with error -13
>> [   46.345701] igc 0000:70:00.0: enabling device (0000 -> 0002)
>> [   46.345777] igc 0000:70:00.0: PTM enabled, 4ns granularity
>>
>> Log after the patch:
>> [    1.031000] igc 0000:70:00.0: enabling device (0000 -> 0002)
>> [    1.032097] igc 0000:70:00.0: PTM enabled, 4ns granularity
>> [    1.642291] igc 0000:70:00.0 (unnamed net_device) (uninitialized): PCIe link lost, device now detached
>> [    5.480490] igc 0000:70:00.0: enabling device (0000 -> 0002)
>> [    5.480516] igc 0000:70:00.0: PTM enabled, 4ns granularity
>>
>> Fixes: ab4056126813 ("igc: Add NVM support")
>> Cc: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
>> Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
>> Reviewed-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>> Tested-by: Mor Bar-Gabay <morx.bar.gabay@intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>> ---
>>   drivers/net/ethernet/intel/igc/igc_base.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc_base.c b/drivers/net/ethernet/intel/igc/igc_base.c
>> index 9fae8bdec2a7..1613b562d17c 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_base.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_base.c
>> @@ -68,6 +68,10 @@ static s32 igc_init_nvm_params_base(struct igc_hw *hw)
>>   	u32 eecd = rd32(IGC_EECD);
>>   	u16 size;
>>   
>> +	/* failed to read reg and got all F's */
>> +	if (!(~eecd))
>> +		return -ENXIO;
> 
> I don't understand this.  It looks like a band-aid that makes boot
> faster but doesn't solve the real problem.
> 
> In its defense, I guess that with this patch, the first igc probe
> fails, and then for some reason we attempt another a few seconds
> later, and the second igc probe works fine, so the NIC actually does
> end up working correct, right?
> 
> I think the PCI core has some issues with configuring ASPM L1.2, and I
> wonder if those are relevant here.  If somebody can repro the problem
> (i.e., without this patch, which looks like it appeared in v6.13 as
> bd2776e39c2a ("igc: return early when failing to read EECD
> register")), I wonder if you could try booting with "pcie_port_pm=off
> pcie_aspm.policy=performance" and see if that also avoids the problem?
> 
> If so, I'd like to see the dmesg log with "pci=earlydump" and the
> "sudo lspci -vv" output when booted with and without "pcie_port_pm=off
> pcie_aspm.policy=performance".

We weren't able to get a repro here.

En-Wei would you be able to provide this to Bjorn?

Thanks,
Tony

>>   	size = FIELD_GET(IGC_EECD_SIZE_EX_MASK, eecd);
>>   
>>   	/* Added to a constant, "size" becomes the left-shift value
>> @@ -221,6 +225,8 @@ static s32 igc_get_invariants_base(struct igc_hw *hw)
>>   
>>   	/* NVM initialization */
>>   	ret_val = igc_init_nvm_params_base(hw);
>> +	if (ret_val)
>> +		goto out;
>>   	switch (hw->mac.type) {
>>   	case igc_i225:
>>   		ret_val = igc_init_nvm_params_i225(hw);
>> -- 
>> 2.47.1
>>

Re: [PATCH net 3/3] igc: return early when failing to read EECD register

[En-Wei WU]

Sure, I'll access the machine and do the debugging next week.

Thanks,
En-Wei.


On Wed, 23 Apr 2025 at 17:10, En-Wei WU <en-wei.wu@canonical.com> wrote:
>
> Sure, I'll access the machine and do the debugging next week.
>
> Thanks,
> En-Wei.
>
> On Wed, 23 Apr 2025 at 01:16, Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
>>
>>
>>
>> On 4/18/2025 12:34 PM, Bjorn Helgaas wrote:
>> > [+cc Kalesh, Przemek, linux-pci]
>> >
>> > On Tue, Jan 07, 2025 at 11:01:47AM -0800, Tony Nguyen wrote:
>> >> From: En-Wei Wu <en-wei.wu@canonical.com>
>> >>
>> >> When booting with a dock connected, the igc driver may get stuck for ~40
>> >> seconds if PCIe link is lost during initialization.
>> >>
>> >> This happens because the driver access device after EECD register reads
>> >> return all F's, indicating failed reads. Consequently, hw->hw_addr is set
>> >> to NULL, which impacts subsequent rd32() reads. This leads to the driver
>> >> hanging in igc_get_hw_semaphore_i225(), as the invalid hw->hw_addr
>> >> prevents retrieving the expected value.
>> >>
>> >> To address this, a validation check and a corresponding return value
>> >> catch is added for the EECD register read result. If all F's are
>> >> returned, indicating PCIe link loss, the driver will return -ENXIO
>> >> immediately. This avoids the 40-second hang and significantly improves
>> >> boot time when using a dock with an igc NIC.
>> >>
>> >> Log before the patch:
>> >> [    0.911913] igc 0000:70:00.0: enabling device (0000 -> 0002)
>> >> [    0.912386] igc 0000:70:00.0: PTM enabled, 4ns granularity
>> >> [    1.571098] igc 0000:70:00.0 (unnamed net_device) (uninitialized): PCIe link lost, device now detached
>> >> [   43.449095] igc_get_hw_semaphore_i225: igc 0000:70:00.0 (unnamed net_device) (uninitialized): Driver can't access device - SMBI bit is set.
>> >> [   43.449186] igc 0000:70:00.0: probe with driver igc failed with error -13
>> >> [   46.345701] igc 0000:70:00.0: enabling device (0000 -> 0002)
>> >> [   46.345777] igc 0000:70:00.0: PTM enabled, 4ns granularity
>> >>
>> >> Log after the patch:
>> >> [    1.031000] igc 0000:70:00.0: enabling device (0000 -> 0002)
>> >> [    1.032097] igc 0000:70:00.0: PTM enabled, 4ns granularity
>> >> [    1.642291] igc 0000:70:00.0 (unnamed net_device) (uninitialized): PCIe link lost, device now detached
>> >> [    5.480490] igc 0000:70:00.0: enabling device (0000 -> 0002)
>> >> [    5.480516] igc 0000:70:00.0: PTM enabled, 4ns granularity
>> >>
>> >> Fixes: ab4056126813 ("igc: Add NVM support")
>> >> Cc: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
>> >> Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
>> >> Reviewed-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>> >> Tested-by: Mor Bar-Gabay <morx.bar.gabay@intel.com>
>> >> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>> >> ---
>> >>   drivers/net/ethernet/intel/igc/igc_base.c | 6 ++++++
>> >>   1 file changed, 6 insertions(+)
>> >>
>> >> diff --git a/drivers/net/ethernet/intel/igc/igc_base.c b/drivers/net/ethernet/intel/igc/igc_base.c
>> >> index 9fae8bdec2a7..1613b562d17c 100644
>> >> --- a/drivers/net/ethernet/intel/igc/igc_base.c
>> >> +++ b/drivers/net/ethernet/intel/igc/igc_base.c
>> >> @@ -68,6 +68,10 @@ static s32 igc_init_nvm_params_base(struct igc_hw *hw)
>> >>      u32 eecd = rd32(IGC_EECD);
>> >>      u16 size;
>> >>
>> >> +    /* failed to read reg and got all F's */
>> >> +    if (!(~eecd))
>> >> +            return -ENXIO;
>> >
>> > I don't understand this.  It looks like a band-aid that makes boot
>> > faster but doesn't solve the real problem.
>> >
>> > In its defense, I guess that with this patch, the first igc probe
>> > fails, and then for some reason we attempt another a few seconds
>> > later, and the second igc probe works fine, so the NIC actually does
>> > end up working correct, right?
>> >
>> > I think the PCI core has some issues with configuring ASPM L1.2, and I
>> > wonder if those are relevant here.  If somebody can repro the problem
>> > (i.e., without this patch, which looks like it appeared in v6.13 as
>> > bd2776e39c2a ("igc: return early when failing to read EECD
>> > register")), I wonder if you could try booting with "pcie_port_pm=off
>> > pcie_aspm.policy=performance" and see if that also avoids the problem?
>> >
>> > If so, I'd like to see the dmesg log with "pci=earlydump" and the
>> > "sudo lspci -vv" output when booted with and without "pcie_port_pm=off
>> > pcie_aspm.policy=performance".
>>
>> We weren't able to get a repro here.
>>
>> En-Wei would you be able to provide this to Bjorn?
>>
>> Thanks,
>> Tony
>>
>> >>      size = FIELD_GET(IGC_EECD_SIZE_EX_MASK, eecd);
>> >>
>> >>      /* Added to a constant, "size" becomes the left-shift value
>> >> @@ -221,6 +225,8 @@ static s32 igc_get_invariants_base(struct igc_hw *hw)
>> >>
>> >>      /* NVM initialization */
>> >>      ret_val = igc_init_nvm_params_base(hw);
>> >> +    if (ret_val)
>> >> +            goto out;
>> >>      switch (hw->mac.type) {
>> >>      case igc_i225:
>> >>              ret_val = igc_init_nvm_params_i225(hw);
>> >> --
>> >> 2.47.1
>> >>
>>

Re: [PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2025-01-07 (ice, igc)

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Tony Nguyen <anthony.l.nguyen@intel.com>:

On Tue,  7 Jan 2025 11:01:44 -0800 you wrote:
> For ice:
> 
> Arkadiusz corrects mask value being used to determine DPLL phase range.
> 
> Przemyslaw corrects frequency value for E823 devices.
> 
> For igc:
> 
> [...]

Here is the summary with links:
  - [net,1/3] ice: fix max values for dpll pin phase adjust
    https://git.kernel.org/netdev/net/c/65104599b3a8
  - [net,2/3] ice: fix incorrect PHY settings for 100 GB/s
    https://git.kernel.org/netdev/net/c/6c5b98911608
  - [net,3/3] igc: return early when failing to read EECD register
    https://git.kernel.org/netdev/net/c/bd2776e39c2a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html