Netdev List
 help / color / mirror / Atom feed
* [PATCH iwl-next v2 0/5] ice: five small fixes and cleanups
@ 2026-05-04 14:24 Aleksandr Loktionov
  2026-05-04 14:24 ` [PATCH iwl-next v2 1/5] ice: fix ETH56G Rx offset type description in kernel-doc comment Aleksandr Loktionov
                   ` (5 more replies)
  0 siblings, 6 replies; 9+ messages in thread
From: Aleksandr Loktionov @ 2026-05-04 14:24 UTC (permalink / raw)
  To: intel-wired-lan, anthony.l.nguyen, aleksandr.loktionov; +Cc: netdev

Three correctness fixes and two cleanups for the ice driver.

Patch 1 corrects a kernel-doc comment in ice_ptp_hw.h that described the
ETH56G MAC Rx offset field as unsigned when it is signed (trivial doc fix,
no functional change).

Patch 2 removes the PF_SB_REM_DEV_CTL sideband register write from
ice_ptp_init_phc_e82x().  PHY access is enabled by default on E82X and
the register write was a leftover from an earlier SWITCH_MODE workaround
that is no longer needed.

Patch 3 renames ICE_SMA2_UFL2_RX_DIS to ICE_SMA2_UFL2_RX_EN to match
the actual active-high hardware semantics and inverts the three use sites
in ice_dpll.c so that the logic remains correct.

Patch 4 replaces the static per-type frequency tables for CGU pins with a
single DPLL_PIN_FREQUENCY_RANGE(1, 25 MHz) entry.  The firmware defines
an any_freq capability for configurable CGU inputs, but the old tables
restricted users to 1 PPS or 10 MHz.  GNSS pins retain a 1 PPS-only
entry since they are physically constrained.

Patch 5 exports ice_dcb_need_recfg() and calls it in the four SW LLDP
netlink setters instead of memcmp() on a non-packed struct, which is
undefined behaviour due to uninitialised padding bytes.  The redundant
memcmp in ice_pf_dcb_cfg() is removed since callers now guard it.

Aleksandr Loktionov (2):
  ice: add correct handling of SMA/u.FL states
  ice: use element-by-element comparison for DCB config changes

Arkadiusz Kubalewski (1):
  ice: fix DPLL pin frequency range in CGU pin descriptors

Karol Kolacinski (2):
  ice: fix ETH56G Rx offset type description in kernel-doc comment
  ice: remove unnecessary PF_SB_REM_DEV_CTL write for E82X

v1 -> v2 updated mail subject with PATCH iwl-next


 drivers/net/ethernet/intel/ice/ice_dcb_lib.c |  13 +-
 drivers/net/ethernet/intel/ice/ice_dcb_lib.h |   2 +
 drivers/net/ethernet/intel/ice/ice_dcb_nl.c  |  30 +++-
 drivers/net/ethernet/intel/ice/ice_dpll.c    |   6 +-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c  | 141 ++++++++++---------
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h  |   8 +-
 6 files changed, 113 insertions(+), 87 deletions(-)

-- 
2.52.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH iwl-next v2 1/5] ice: fix ETH56G Rx offset type description in kernel-doc comment
  2026-05-04 14:24 [PATCH iwl-next v2 0/5] ice: five small fixes and cleanups Aleksandr Loktionov
@ 2026-05-04 14:24 ` Aleksandr Loktionov
  2026-05-04 14:24 ` [PATCH iwl-next v2 2/5] ice: remove unnecessary PF_SB_REM_DEV_CTL write for E82X Aleksandr Loktionov
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Aleksandr Loktionov @ 2026-05-04 14:24 UTC (permalink / raw)
  To: intel-wired-lan, anthony.l.nguyen, aleksandr.loktionov; +Cc: netdev

From: Karol Kolacinski <karol.kolacinski@intel.com>

The ETH56G MAC register configuration Rx offset field stores a signed
integer, not an unsigned one. Correct the struct comment that incorrectly
described it as '11 bit unsigned int'. Also update 'unsigned ints' to
'unsigned integers' for consistency.

Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index 9bfd3e7..c1aa408 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -144,9 +144,9 @@ struct ice_vernier_info_e82x {
  * @tx_offset: total Tx offset, fixed point
  * @rx_offset: total Rx offset, contains value for bitslip/deskew, fixed point
  *
- * All fixed point registers except Rx offset are 23 bit unsigned ints with
+ * All fixed point registers except Rx offset are 23 bit unsigned integers with
  * a 9 bit fractional.
- * Rx offset is 11 bit unsigned int with a 9 bit fractional.
+ * Rx offset is 11 bit signed integer with a 9 bit fractional.
  */
 struct ice_eth56g_mac_reg_cfg {
 	struct {
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH iwl-next v2 2/5] ice: remove unnecessary PF_SB_REM_DEV_CTL write for E82X
  2026-05-04 14:24 [PATCH iwl-next v2 0/5] ice: five small fixes and cleanups Aleksandr Loktionov
  2026-05-04 14:24 ` [PATCH iwl-next v2 1/5] ice: fix ETH56G Rx offset type description in kernel-doc comment Aleksandr Loktionov
@ 2026-05-04 14:24 ` Aleksandr Loktionov
  2026-05-04 14:24 ` [PATCH iwl-next v2 3/5] ice: add correct handling of SMA/u.FL states Aleksandr Loktionov
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Aleksandr Loktionov @ 2026-05-04 14:24 UTC (permalink / raw)
  To: intel-wired-lan, anthony.l.nguyen, aleksandr.loktionov; +Cc: netdev

From: Karol Kolacinski <karol.kolacinski@intel.com>

Remove the PF_SB_REM_DEV_CTL register write from ice_ptp_init_phc_e82x().
PHY access is enabled by default on E82X devices and the driver does not
need to configure switch device access. The register write was a
remnant of an earlier SWITCH_MODE workaround for a FIFO issue and is
no longer needed.

Also update the kernel-doc comment to refer to the E82X family rather than
E822.

Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 61c0a0d..7b1b402 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -2767,22 +2767,13 @@ static int ice_ptp_set_vernier_wl(struct ice_hw *hw)
 }
 
 /**
- * ice_ptp_init_phc_e82x - Perform E822 specific PHC initialization
+ * ice_ptp_init_phc_e82x - Perform E82X specific PHC initialization
  * @hw: pointer to HW struct
  *
- * Perform PHC initialization steps specific to E822 devices.
+ * Perform PHC initialization steps specific to E82X devices.
  */
 static int ice_ptp_init_phc_e82x(struct ice_hw *hw)
 {
-	u32 val;
-
-	/* Enable reading switch and PHY registers over the sideband queue */
-#define PF_SB_REM_DEV_CTL_SWITCH_READ BIT(1)
-#define PF_SB_REM_DEV_CTL_PHY0 BIT(2)
-	val = rd32(hw, PF_SB_REM_DEV_CTL);
-	val |= (PF_SB_REM_DEV_CTL_SWITCH_READ | PF_SB_REM_DEV_CTL_PHY0);
-	wr32(hw, PF_SB_REM_DEV_CTL, val);
-
 	/* Set window length for all the ports */
 	return ice_ptp_set_vernier_wl(hw);
 }
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH iwl-next v2 3/5] ice: add correct handling of SMA/u.FL states
  2026-05-04 14:24 [PATCH iwl-next v2 0/5] ice: five small fixes and cleanups Aleksandr Loktionov
  2026-05-04 14:24 ` [PATCH iwl-next v2 1/5] ice: fix ETH56G Rx offset type description in kernel-doc comment Aleksandr Loktionov
  2026-05-04 14:24 ` [PATCH iwl-next v2 2/5] ice: remove unnecessary PF_SB_REM_DEV_CTL write for E82X Aleksandr Loktionov
@ 2026-05-04 14:24 ` Aleksandr Loktionov
  2026-05-07 11:45   ` Simon Horman
  2026-05-04 14:24 ` [PATCH iwl-next v2 4/5] ice: fix DPLL pin frequency range in CGU pin descriptors Aleksandr Loktionov
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 9+ messages in thread
From: Aleksandr Loktionov @ 2026-05-04 14:24 UTC (permalink / raw)
  To: intel-wired-lan, anthony.l.nguyen, aleksandr.loktionov; +Cc: netdev

The ICE_SMA2_UFL2_RX_DIS bit name is wrong: the bit is active high
(setting it *enables* RX for u.FL2 / SMA2), not active low.  Rename
it to ICE_SMA2_UFL2_RX_EN and invert the use sites in ice_dpll.c so
that enabling the u.FL2 pin clears the bit (as it used to do) and
disabling sets it.

Fixes: 2dd5d03c77e2 ("ice: redesign dpll sma/u.fl pins control")
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_dpll.c   | 6 +++---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
index 62f75701d..7e8bb63 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -672,7 +672,7 @@ ice_dpll_sw_pins_update(struct ice_pf *pf)
 		p->active = false;
 
 	p = &d->ufl[ICE_DPLL_PIN_SW_2_IDX];
-	p->active = (data & ICE_SMA2_DIR_EN) && !(data & ICE_SMA2_UFL2_RX_DIS);
+	p->active = (data & ICE_SMA2_DIR_EN) && !(data & ICE_SMA2_UFL2_RX_EN);
 	d->sma_data = data;
 
 	return 0;
@@ -1264,10 +1264,10 @@ ice_dpll_ufl_pin_state_set(const struct dpll_pin *pin, void *pin_priv,
 	case ICE_DPLL_PIN_SW_2_IDX:
 		if (state == DPLL_PIN_STATE_SELECTABLE) {
 			data |= ICE_SMA2_DIR_EN;
-			data &= ~ICE_SMA2_UFL2_RX_DIS;
+			data &= ~ICE_SMA2_UFL2_RX_EN;
 			enable = true;
 		} else if (state == DPLL_PIN_STATE_DISCONNECTED) {
-			data |= ICE_SMA2_UFL2_RX_DIS;
+			data |= ICE_SMA2_UFL2_RX_EN;
 			enable = false;
 		} else {
 			goto unlock;
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index c1aa408..278d757 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -655,12 +655,12 @@ static inline u64 ice_get_base_incval(struct ice_hw *hw)
 /* SMA controller pin control */
 #define ICE_SMA1_DIR_EN		BIT(4)
 #define ICE_SMA1_TX_EN		BIT(5)
-#define ICE_SMA2_UFL2_RX_DIS	BIT(3)
+#define ICE_SMA2_UFL2_RX_EN	BIT(3)
 #define ICE_SMA2_DIR_EN		BIT(6)
 #define ICE_SMA2_TX_EN		BIT(7)
 
 #define ICE_SMA1_MASK		(ICE_SMA1_DIR_EN | ICE_SMA1_TX_EN)
-#define ICE_SMA2_MASK		(ICE_SMA2_UFL2_RX_DIS | ICE_SMA2_DIR_EN | \
+#define ICE_SMA2_MASK		(ICE_SMA2_UFL2_RX_EN | ICE_SMA2_DIR_EN | \
 				 ICE_SMA2_TX_EN)
 #define ICE_SMA2_INACTIVE_MASK	(ICE_SMA2_DIR_EN | ICE_SMA2_TX_EN)
 #define ICE_ALL_SMA_MASK	(ICE_SMA1_MASK | ICE_SMA2_MASK)
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH iwl-next v2 4/5] ice: fix DPLL pin frequency range in CGU pin descriptors
  2026-05-04 14:24 [PATCH iwl-next v2 0/5] ice: five small fixes and cleanups Aleksandr Loktionov
                   ` (2 preceding siblings ...)
  2026-05-04 14:24 ` [PATCH iwl-next v2 3/5] ice: add correct handling of SMA/u.FL states Aleksandr Loktionov
@ 2026-05-04 14:24 ` Aleksandr Loktionov
  2026-05-04 14:24 ` [PATCH iwl-next v2 5/5] ice: use element-by-element comparison for DCB config changes Aleksandr Loktionov
  2026-05-06 22:53 ` [Intel-wired-lan] [PATCH iwl-next v2 0/5] ice: five small fixes and cleanups Jacob Keller
  5 siblings, 0 replies; 9+ messages in thread
From: Aleksandr Loktionov @ 2026-05-04 14:24 UTC (permalink / raw)
  To: intel-wired-lan, anthony.l.nguyen, aleksandr.loktionov
  Cc: netdev, Arkadiusz Kubalewski

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

Replace the per-type frequency tables (ice_cgu_pin_freq_1_hz and
ice_cgu_pin_freq_10_mhz) and the two-entry ice_cgu_pin_freq_common array
with a named range constant ICE_CGU_MAX_FREQ_HZ (25 MHz), a new
ice_cgu_pin_freq_range array containing DPLL_PIN_FREQUENCY_RANGE(1,
ICE_CGU_MAX_FREQ_HZ), and a separate ice_cgu_pin_freq_gnss array that
retains DPLL_PIN_FREQUENCY_1PPS for GNSS input pins.

The hardware firmware spec defines an any_freq capability for CGU inputs
(ICE_AQC_GET_CGU_IN_CFG_FLG1_ANYFREQ), but the static pin descriptor
tables constrained configurable pins to 1PPS or 10MHz, preventing users
from setting valid intermediate frequencies. Use a range entry so the
DPLL netlink interface correctly reflects what the firmware will accept.
The firmware validates the actual value and rejects out-of-range requests.

MUX, SyncE ETH port, and configurable EXT pins now advertise the full
frequency range, matching the hardware capability. GNSS input pins retain
the 1PPS-only advertisement since a GNSS receiver is physically
constrained to 1 Hz.

Fixes: 6db5f2cd9ebb ("ice: dpll: fix output pin capabilities")
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 128 +++++++++++---------
 1 file changed, 72 insertions(+), 56 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 7b1b402..3949138 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -7,127 +7,143 @@
 #include "ice_ptp_hw.h"
 #include "ice_ptp_consts.h"
 
-static struct dpll_pin_frequency ice_cgu_pin_freq_common[] = {
-	DPLL_PIN_FREQUENCY_1PPS,
-	DPLL_PIN_FREQUENCY_10MHZ,
-};
+/* Maximum frequency supported by CGU pins, in Hz */
+#define ICE_CGU_MAX_FREQ_HZ	25000000
 
-static struct dpll_pin_frequency ice_cgu_pin_freq_1_hz[] = {
-	DPLL_PIN_FREQUENCY_1PPS,
+static struct dpll_pin_frequency ice_cgu_pin_freq_range[] = {
+	DPLL_PIN_FREQUENCY_RANGE(1, ICE_CGU_MAX_FREQ_HZ),
 };
 
-static struct dpll_pin_frequency ice_cgu_pin_freq_10_mhz[] = {
-	DPLL_PIN_FREQUENCY_10MHZ,
+static struct dpll_pin_frequency ice_cgu_pin_freq_gnss[] = {
+	DPLL_PIN_FREQUENCY_1PPS,
 };
 
 static const struct ice_cgu_pin_desc ice_e810t_sfp_cgu_inputs[] = {
 	{ "CVL-SDP22",	  ZL_REF0P, DPLL_PIN_TYPE_INT_OSCILLATOR,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "CVL-SDP20",	  ZL_REF0N, DPLL_PIN_TYPE_INT_OSCILLATOR,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
-	{ "C827_0-RCLKA", ZL_REF1P, DPLL_PIN_TYPE_MUX, 0, },
-	{ "C827_0-RCLKB", ZL_REF1N, DPLL_PIN_TYPE_MUX, 0, },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "C827_0-RCLKA", ZL_REF1P, DPLL_PIN_TYPE_MUX,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "C827_0-RCLKB", ZL_REF1N, DPLL_PIN_TYPE_MUX,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "SMA1",	  ZL_REF3P, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "SMA2/U.FL2",	  ZL_REF3N, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "GNSS-1PPS",	  ZL_REF4P, DPLL_PIN_TYPE_GNSS,
-		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
+		ARRAY_SIZE(ice_cgu_pin_freq_gnss), ice_cgu_pin_freq_gnss },
 };
 
 static const struct ice_cgu_pin_desc ice_e810t_qsfp_cgu_inputs[] = {
 	{ "CVL-SDP22",	  ZL_REF0P, DPLL_PIN_TYPE_INT_OSCILLATOR,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "CVL-SDP20",	  ZL_REF0N, DPLL_PIN_TYPE_INT_OSCILLATOR,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
-	{ "C827_0-RCLKA", ZL_REF1P, DPLL_PIN_TYPE_MUX, },
-	{ "C827_0-RCLKB", ZL_REF1N, DPLL_PIN_TYPE_MUX, },
-	{ "C827_1-RCLKA", ZL_REF2P, DPLL_PIN_TYPE_MUX, },
-	{ "C827_1-RCLKB", ZL_REF2N, DPLL_PIN_TYPE_MUX, },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "C827_0-RCLKA", ZL_REF1P, DPLL_PIN_TYPE_MUX,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "C827_0-RCLKB", ZL_REF1N, DPLL_PIN_TYPE_MUX,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "C827_1-RCLKA", ZL_REF2P, DPLL_PIN_TYPE_MUX,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "C827_1-RCLKB", ZL_REF2N, DPLL_PIN_TYPE_MUX,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "SMA1",	  ZL_REF3P, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "SMA2/U.FL2",	  ZL_REF3N, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "GNSS-1PPS",	  ZL_REF4P, DPLL_PIN_TYPE_GNSS,
-		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
+		ARRAY_SIZE(ice_cgu_pin_freq_gnss), ice_cgu_pin_freq_gnss },
 };
 
 static const struct ice_cgu_pin_desc ice_e810t_sfp_cgu_outputs[] = {
 	{ "REF-SMA1",	    ZL_OUT0, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "REF-SMA2/U.FL2", ZL_OUT1, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
-	{ "PHY-CLK",	    ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT, },
-	{ "MAC-CLK",	    ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "PHY-CLK",	    ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "MAC-CLK",	    ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "CVL-SDP21",	    ZL_OUT4, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "CVL-SDP23",	    ZL_OUT5, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 };
 
 static const struct ice_cgu_pin_desc ice_e810t_qsfp_cgu_outputs[] = {
 	{ "REF-SMA1",	    ZL_OUT0, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "REF-SMA2/U.FL2", ZL_OUT1, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
-	{ "PHY-CLK",	    ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
-	{ "PHY2-CLK",	    ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
-	{ "MAC-CLK",	    ZL_OUT4, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "PHY-CLK",	    ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "PHY2-CLK",	    ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "MAC-CLK",	    ZL_OUT4, DPLL_PIN_TYPE_SYNCE_ETH_PORT,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "CVL-SDP21",	    ZL_OUT5, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "CVL-SDP23",	    ZL_OUT6, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 };
 
 static const struct ice_cgu_pin_desc ice_e823_si_cgu_inputs[] = {
 	{ "NONE",	  SI_REF0P, 0, 0 },
 	{ "NONE",	  SI_REF0N, 0, 0 },
-	{ "SYNCE0_DP",	  SI_REF1P, DPLL_PIN_TYPE_MUX, 0 },
-	{ "SYNCE0_DN",	  SI_REF1N, DPLL_PIN_TYPE_MUX, 0 },
+	{ "SYNCE0_DP",	  SI_REF1P, DPLL_PIN_TYPE_MUX,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "SYNCE0_DN",	  SI_REF1N, DPLL_PIN_TYPE_MUX,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "EXT_CLK_SYNC", SI_REF2P, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "NONE",	  SI_REF2N, 0, 0 },
 	{ "EXT_PPS_OUT",  SI_REF3,  DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "INT_PPS_OUT",  SI_REF4,  DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 };
 
 static const struct ice_cgu_pin_desc ice_e823_si_cgu_outputs[] = {
 	{ "1588-TIME_SYNC", SI_OUT0, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
-	{ "PHY-CLK",	    SI_OUT1, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "PHY-CLK",	    SI_OUT1, DPLL_PIN_TYPE_SYNCE_ETH_PORT,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "10MHZ-SMA2",	    SI_OUT2, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_10_mhz), ice_cgu_pin_freq_10_mhz },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "PPS-SMA1",	    SI_OUT3, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 };
 
 static const struct ice_cgu_pin_desc ice_e823_zl_cgu_inputs[] = {
 	{ "NONE",	  ZL_REF0P, 0, 0 },
 	{ "INT_PPS_OUT",  ZL_REF0N, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
-	{ "SYNCE0_DP",	  ZL_REF1P, DPLL_PIN_TYPE_MUX, 0 },
-	{ "SYNCE0_DN",	  ZL_REF1N, DPLL_PIN_TYPE_MUX, 0 },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "SYNCE0_DP",	  ZL_REF1P, DPLL_PIN_TYPE_MUX,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "SYNCE0_DN",	  ZL_REF1N, DPLL_PIN_TYPE_MUX,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "NONE",	  ZL_REF2P, 0, 0 },
 	{ "NONE",	  ZL_REF2N, 0, 0 },
 	{ "EXT_CLK_SYNC", ZL_REF3P, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "NONE",	  ZL_REF3N, 0, 0 },
 	{ "EXT_PPS_OUT",  ZL_REF4P, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "OCXO",	  ZL_REF4N, DPLL_PIN_TYPE_INT_OSCILLATOR, 0 },
 };
 
 static const struct ice_cgu_pin_desc ice_e823_zl_cgu_outputs[] = {
 	{ "PPS-SMA1",	   ZL_OUT0, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "10MHZ-SMA2",	   ZL_OUT1, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_10_mhz), ice_cgu_pin_freq_10_mhz },
-	{ "PHY-CLK",	   ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
-	{ "1588-TIME_REF", ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "PHY-CLK",	   ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
+	{ "1588-TIME_REF", ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT,
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "CPK-TIME_SYNC", ZL_OUT4, DPLL_PIN_TYPE_EXT,
-		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
+		ARRAY_SIZE(ice_cgu_pin_freq_range), ice_cgu_pin_freq_range },
 	{ "NONE",	   ZL_OUT5, 0, 0 },
 };
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH iwl-next v2 5/5] ice: use element-by-element comparison for DCB config changes
  2026-05-04 14:24 [PATCH iwl-next v2 0/5] ice: five small fixes and cleanups Aleksandr Loktionov
                   ` (3 preceding siblings ...)
  2026-05-04 14:24 ` [PATCH iwl-next v2 4/5] ice: fix DPLL pin frequency range in CGU pin descriptors Aleksandr Loktionov
@ 2026-05-04 14:24 ` Aleksandr Loktionov
  2026-05-07 11:46   ` Simon Horman
  2026-05-06 22:53 ` [Intel-wired-lan] [PATCH iwl-next v2 0/5] ice: five small fixes and cleanups Jacob Keller
  5 siblings, 1 reply; 9+ messages in thread
From: Aleksandr Loktionov @ 2026-05-04 14:24 UTC (permalink / raw)
  To: intel-wired-lan, anthony.l.nguyen, aleksandr.loktionov; +Cc: netdev

Comparing two ice_dcbx_cfg structs with memcmp() is unreliable on
non-packed structs due to uninitialised padding bytes.  The HW DCB
path already has ice_dcb_need_recfg() that compares fields
individually; export it and use it in the SW DCB netlink setters
(setets, setpfc, setapp, cee_set_all) instead of the unsafe memcmp.

Remove the now-redundant memcmp check from ice_pf_dcb_cfg() so that
function always attempts the HW reconfiguration when called.

Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 13 ++-------
 drivers/net/ethernet/intel/ice/ice_dcb_lib.h |  2 ++
 drivers/net/ethernet/intel/ice/ice_dcb_nl.c  | 30 ++++++++++++++++++--
 3 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
index bd77f1c..2e85fae 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
@@ -353,15 +353,11 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked)
 	struct ice_dcbx_cfg *old_cfg, *curr_cfg;
 	struct device *dev = ice_pf_to_dev(pf);
 	struct iidc_rdma_event *event;
-	int ret = ICE_DCB_NO_HW_CHG;
 	struct ice_vsi *pf_vsi;
+	int ret;
 
 	curr_cfg = &pf->hw.port_info->qos_cfg.local_dcbx_cfg;
 
-	/* FW does not care if change happened */
-	if (!pf->hw.port_info->qos_cfg.is_sw_lldp)
-		ret = ICE_DCB_HW_CHG_RST;
-
 	/* Enable DCB tagging only when more than one TC */
 	if (ice_dcb_get_num_tc(new_cfg) > 1) {
 		dev_dbg(dev, "DCB tagging enabled (num TC > 1)\n");
@@ -377,11 +373,6 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked)
 		clear_bit(ICE_FLAG_DCB_ENA, pf->flags);
 	}
 
-	if (!memcmp(new_cfg, curr_cfg, sizeof(*new_cfg))) {
-		dev_dbg(dev, "No change in DCB config required\n");
-		return ret;
-	}
-
 	if (ice_dcb_bwchk(pf, new_cfg))
 		return -EINVAL;
 
@@ -481,7 +472,7 @@ static void ice_cfg_etsrec_defaults(struct ice_port_info *pi)
  * @old_cfg: current DCB config
  * @new_cfg: new DCB config
  */
-static bool
+bool
 ice_dcb_need_recfg(struct ice_pf *pf, struct ice_dcbx_cfg *old_cfg,
 		   struct ice_dcbx_cfg *new_cfg)
 {
diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.h b/drivers/net/ethernet/intel/ice/ice_dcb_lib.h
index da9ba81..a7eaa2f9 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.h
@@ -20,6 +20,8 @@ u8 ice_dcb_get_num_tc(struct ice_dcbx_cfg *dcbcfg);
 void ice_vsi_set_dcb_tc_cfg(struct ice_vsi *vsi);
 bool ice_is_pfc_causing_hung_q(struct ice_pf *pf, unsigned int txqueue);
 u8 ice_dcb_get_tc(struct ice_vsi *vsi, int queue_index);
+bool ice_dcb_need_recfg(struct ice_pf *pf, struct ice_dcbx_cfg *old_cfg,
+			struct ice_dcbx_cfg *new_cfg);
 int
 ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked);
 int ice_dcb_bwchk(struct ice_pf *pf, struct ice_dcbx_cfg *dcbcfg);
diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
index a10c1c8d..13a52c1 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
@@ -108,11 +108,17 @@ static int ice_dcbnl_setets(struct net_device *netdev, struct ieee_ets *ets)
 	if (!bwrec)
 		new_cfg->etsrec.tcbwtable[0] = 100;
 
+	if (!ice_dcb_need_recfg(pf, &pf->hw.port_info->qos_cfg.local_dcbx_cfg,
+				new_cfg)) {
+		err = ICE_DCB_NO_HW_CHG;
+		goto ets_out;
+	}
+
 	err = ice_pf_dcb_cfg(pf, new_cfg, true);
 	/* return of zero indicates new cfg applied */
-	if (err == ICE_DCB_HW_CHG_RST)
+	if (!err)
 		ice_dcbnl_devreset(netdev);
-	if (err == ICE_DCB_NO_HW_CHG)
+	else if (err == ICE_DCB_NO_HW_CHG)
 		err = ICE_DCB_HW_CHG_RST;
 
 ets_out:
@@ -287,11 +293,18 @@ static int ice_dcbnl_setpfc(struct net_device *netdev, struct ieee_pfc *pfc)
 
 	new_cfg->pfc.pfcena = pfc->pfc_en;
 
+	if (!ice_dcb_need_recfg(pf, &pf->hw.port_info->qos_cfg.local_dcbx_cfg,
+				new_cfg)) {
+		err = ICE_DCB_NO_HW_CHG;
+		goto pfc_out;
+	}
+
 	err = ice_pf_dcb_cfg(pf, new_cfg, true);
 	if (err == ICE_DCB_HW_CHG_RST)
 		ice_dcbnl_devreset(netdev);
 	if (err == ICE_DCB_NO_HW_CHG)
 		err = ICE_DCB_HW_CHG_RST;
+pfc_out:
 	mutex_unlock(&pf->tc_mutex);
 	return err;
 }
@@ -845,6 +858,12 @@ static int ice_dcbnl_setapp(struct net_device *netdev, struct dcb_app *app)
 	new_cfg->dscp_map[app->protocol] = app->priority;
 	new_cfg->app[new_cfg->numapps++] = new_app;
 
+	if (!ice_dcb_need_recfg(pf, &pf->hw.port_info->qos_cfg.local_dcbx_cfg,
+				new_cfg)) {
+		ret = ICE_DCB_NO_HW_CHG;
+		goto setapp_out;
+	}
+
 	ret = ice_pf_dcb_cfg(pf, new_cfg, true);
 	/* return of zero indicates new cfg applied */
 	if (ret == ICE_DCB_HW_CHG_RST)
@@ -991,8 +1010,15 @@ static u8 ice_dcbnl_cee_set_all(struct net_device *netdev)
 
 	mutex_lock(&pf->tc_mutex);
 
+	if (!ice_dcb_need_recfg(pf, &pf->hw.port_info->qos_cfg.local_dcbx_cfg,
+				new_cfg)) {
+		err = ICE_DCB_NO_HW_CHG;
+		goto out;
+	}
+
 	err = ice_pf_dcb_cfg(pf, new_cfg, true);
 
+out:
 	mutex_unlock(&pf->tc_mutex);
 	return (err != ICE_DCB_HW_CHG_RST) ? ICE_DCB_NO_HW_CHG : err;
 }
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Intel-wired-lan] [PATCH iwl-next v2 0/5] ice: five small fixes and cleanups
  2026-05-04 14:24 [PATCH iwl-next v2 0/5] ice: five small fixes and cleanups Aleksandr Loktionov
                   ` (4 preceding siblings ...)
  2026-05-04 14:24 ` [PATCH iwl-next v2 5/5] ice: use element-by-element comparison for DCB config changes Aleksandr Loktionov
@ 2026-05-06 22:53 ` Jacob Keller
  5 siblings, 0 replies; 9+ messages in thread
From: Jacob Keller @ 2026-05-06 22:53 UTC (permalink / raw)
  To: Aleksandr Loktionov, intel-wired-lan, anthony.l.nguyen; +Cc: netdev

On 5/4/2026 7:24 AM, Aleksandr Loktionov wrote:
> Three correctness fixes and two cleanups for the ice driver.
> 
> Patch 1 corrects a kernel-doc comment in ice_ptp_hw.h that described the
> ETH56G MAC Rx offset field as unsigned when it is signed (trivial doc fix,
> no functional change).
> 
> Patch 2 removes the PF_SB_REM_DEV_CTL sideband register write from
> ice_ptp_init_phc_e82x().  PHY access is enabled by default on E82X and
> the register write was a leftover from an earlier SWITCH_MODE workaround
> that is no longer needed.
> 
> Patch 3 renames ICE_SMA2_UFL2_RX_DIS to ICE_SMA2_UFL2_RX_EN to match
> the actual active-high hardware semantics and inverts the three use sites
> in ice_dpll.c so that the logic remains correct.
> 
> Patch 4 replaces the static per-type frequency tables for CGU pins with a
> single DPLL_PIN_FREQUENCY_RANGE(1, 25 MHz) entry.  The firmware defines
> an any_freq capability for configurable CGU inputs, but the old tables
> restricted users to 1 PPS or 10 MHz.  GNSS pins retain a 1 PPS-only
> entry since they are physically constrained.
> 
> Patch 5 exports ice_dcb_need_recfg() and calls it in the four SW LLDP
> netlink setters instead of memcmp() on a non-packed struct, which is
> undefined behaviour due to uninitialised padding bytes.  The redundant
> memcmp in ice_pf_dcb_cfg() is removed since callers now guard it.
> 

Some of these seem like they belong as net fixes, not cleanups
targetting next.

Specifically patch 3 and 4 I feel should be separated. Could you please
either justify why those issues are not "fixes" worthy of net, or
separate them into their own series?

Thanks,
Jake

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH iwl-next v2 3/5] ice: add correct handling of SMA/u.FL states
  2026-05-04 14:24 ` [PATCH iwl-next v2 3/5] ice: add correct handling of SMA/u.FL states Aleksandr Loktionov
@ 2026-05-07 11:45   ` Simon Horman
  0 siblings, 0 replies; 9+ messages in thread
From: Simon Horman @ 2026-05-07 11:45 UTC (permalink / raw)
  To: aleksandr.loktionov
  Cc: 'Simon Horman', intel-wired-lan, anthony.l.nguyen, netdev

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev

Simon says:

I would also like to add that as a fix the patch description should
describe the user-visible effects of the bug, and how the fix addresses
them.

Thanks!
---
ice: add correct handling of SMA/u.FL states

This commit renames ICE_SMA2_UFL2_RX_DIS to ICE_SMA2_UFL2_RX_EN, stating that
the hardware bit is actually active high for enabling RX. It updates the uses
of the macro in ice_dpll.c to reflect the new name.

>     The ICE_SMA2_UFL2_RX_DIS bit name is wrong: the bit is active high
>     (setting it *enables* RX for u.FL2 / SMA2), not active low.  Rename
>     it to ICE_SMA2_UFL2_RX_EN and invert the use sites in ice_dpll.c so
>     that enabling the u.FL2 pin clears the bit (as it used to do) and
>     disabling sets it.

The commit message claims that setting the bit enables RX (active high), but
then states that enabling the pin clears the bit (active low). Could this
description be clarified to explain exactly whether the hardware uses active
high or active low logic?

> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index 62f75701d6520..7e8bb63b262a0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
> @@ -672,7 +672,7 @@ ice_dpll_sw_pins_update(struct ice_pf *pf)
>  		p->active = false;
>  
>  	p = &d->ufl[ICE_DPLL_PIN_SW_2_IDX];
> -	p->active = (data & ICE_SMA2_DIR_EN) && !(data & ICE_SMA2_UFL2_RX_DIS);
> +	p->active = (data & ICE_SMA2_DIR_EN) && !(data & ICE_SMA2_UFL2_RX_EN);

If the bit is renamed to an _EN suffix and is active high, should the logic be
inverted here? This code still treats the bit as active low by checking for it
being cleared (!(data & ICE_SMA2_UFL2_RX_EN)).

[ ... ]

> @@ -1264,10 +1264,10 @@ ice_dpll_ufl_pin_state_set(const struct dpll_pin *pin, void *pin_priv,
>  	case ICE_DPLL_PIN_SW_2_IDX:
>  		if (state == DPLL_PIN_STATE_SELECTABLE) {
>  			data |= ICE_SMA2_DIR_EN;
> -			data &= ~ICE_SMA2_UFL2_RX_DIS;
> +			data &= ~ICE_SMA2_UFL2_RX_EN;
>  			enable = true;
>  		} else if (state == DPLL_PIN_STATE_DISCONNECTED) {
> -			data |= ICE_SMA2_UFL2_RX_DIS;
> +			data |= ICE_SMA2_UFL2_RX_EN;
>  			enable = false;
>  		} else {

Similarly, does this code still apply active low logic by clearing the _EN bit
when enable is true, and setting it when enable is false?

If the bit is truly active high as the commit message suggests ("setting it
enables RX"), shouldn't the bitwise operations be inverted to match?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH iwl-next v2 5/5] ice: use element-by-element comparison for DCB config changes
  2026-05-04 14:24 ` [PATCH iwl-next v2 5/5] ice: use element-by-element comparison for DCB config changes Aleksandr Loktionov
@ 2026-05-07 11:46   ` Simon Horman
  0 siblings, 0 replies; 9+ messages in thread
From: Simon Horman @ 2026-05-07 11:46 UTC (permalink / raw)
  To: aleksandr.loktionov
  Cc: 'Simon Horman', intel-wired-lan, anthony.l.nguyen, netdev

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
Subject: ice: use element-by-element comparison for DCB config changes

This commit replaces the use of memcmp() for DCB configurations with an
element-by-element comparison using ice_dcb_need_recfg(). It exports this
function and applies it to the SW DCB netlink setters to prevent issues
with uninitialized padding bytes.

> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
> index a10c1c8d86977..13a52c1a4da3e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
> @@ -108,11 +108,17 @@ static int ice_dcbnl_setets(struct net_device *netdev, struct ieee_ets *ets)
>  	if (!bwrec)
>  		new_cfg->etsrec.tcbwtable[0] = 100;
>  
> +	if (!ice_dcb_need_recfg(pf, &pf->hw.port_info->qos_cfg.local_dcbx_cfg,
> +				new_cfg)) {
> +		err = ICE_DCB_NO_HW_CHG;
> +		goto ets_out;
> +	}

Does ice_dcb_need_recfg() correctly detect bandwidth changes?

Looking at the implementation of ice_dcb_need_recfg(), it only logs a debug
message but doesn't set need_reconfig to true when only the ETS Bandwidth
Table or TSA Table changes:

drivers/net/ethernet/intel/ice/ice_dcb_lib.c:ice_dcb_need_recfg() {
    ...
    if (memcmp(&new_cfg->etscfg.tcbwtable,
               &old_cfg->etscfg.tcbwtable,
               sizeof(new_cfg->etscfg.tcbwtable)))
        dev_dbg(dev, "ETS TC BW Table changed.\n");
    ...
}

Since the function returns false, will this cause ice_dcbnl_setets() to
abort early and silently drop user-requested bandwidth modifications?

Also, since new_cfg points to the shared desired_dcbx_cfg state, does this
early return leave uncommitted modifications in the software state?

If a subsequent unrelated operation (like setting PFC) correctly returns
true from ice_dcb_need_recfg(), it appears it could unintentionally push
these abandoned ETS bandwidth changes to the hardware.

>  	err = ice_pf_dcb_cfg(pf, new_cfg, true);
>  	/* return of zero indicates new cfg applied */
> -	if (err == ICE_DCB_HW_CHG_RST)
> +	if (!err)
>  		ice_dcbnl_devreset(netdev);

This isn't a bug introduced by this patch, but can ice_dcbnl_devreset()
cause a deadlock against hardware reset tasks?

The ice_dcbnl_devreset() function spins in a usleep_range() loop while
ice_is_reset_in_progress() is true.

Since DCBNL callbacks like ice_dcbnl_setets() run with the global
rtnl_lock held, and a concurrent hardware or PCIe reset worker likely
requires rtnl_lock to complete the reset, will the reset task block
forever while this thread waits for it?

> -	if (err == ICE_DCB_NO_HW_CHG)
> +	else if (err == ICE_DCB_NO_HW_CHG)
>  		err = ICE_DCB_HW_CHG_RST;
>  
>  ets_out:

Does this change cause the function to return a positive error code to
userspace when applying an identical configuration?

By bypassing the translation from ICE_DCB_NO_HW_CHG (which is 1) to
ICE_DCB_HW_CHG_RST (which evaluates to 0) in the early goto added
above, err remains 1 at the ets_out label.

The DCBNL core evaluates any non-zero return value as an error:

net/dcb/dcbnl.c:dcbnl_ieee_set() {
    ...
    err = netdev->dcbnl_ops->ieee_setets(netdev, ets);
    if (err)
        goto err;
    ...
}

Will this cause tools like lldptool to fail when configuring an
existing setup?

[ ... ]

> @@ -845,6 +858,12 @@ static int ice_dcbnl_setapp(struct net_device *netdev, struct dcb_app *app)
>  	new_cfg->dscp_map[app->protocol] = app->priority;
>  	new_cfg->app[new_cfg->numapps++] = new_app;

This isn't a bug introduced by this patch, but does this array append
lack bounds checking against ICE_DCBX_MAX_APPS?

The app array has a capacity of 64. The function restricts duplicate
DSCP protocols using the dscp_mapped bitmap, but since DSCP_MAX is also
64, a user could map all 64 unique values.

If numapps starts greater than 0 due to existing EtherType applications,
adding 64 valid DSCP mappings would cause numapps to exceed 64, leading
to an out-of-bounds write that overwrites the adjacent dscp_mapped
bitmap.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-05-07 11:47 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-04 14:24 [PATCH iwl-next v2 0/5] ice: five small fixes and cleanups Aleksandr Loktionov
2026-05-04 14:24 ` [PATCH iwl-next v2 1/5] ice: fix ETH56G Rx offset type description in kernel-doc comment Aleksandr Loktionov
2026-05-04 14:24 ` [PATCH iwl-next v2 2/5] ice: remove unnecessary PF_SB_REM_DEV_CTL write for E82X Aleksandr Loktionov
2026-05-04 14:24 ` [PATCH iwl-next v2 3/5] ice: add correct handling of SMA/u.FL states Aleksandr Loktionov
2026-05-07 11:45   ` Simon Horman
2026-05-04 14:24 ` [PATCH iwl-next v2 4/5] ice: fix DPLL pin frequency range in CGU pin descriptors Aleksandr Loktionov
2026-05-04 14:24 ` [PATCH iwl-next v2 5/5] ice: use element-by-element comparison for DCB config changes Aleksandr Loktionov
2026-05-07 11:46   ` Simon Horman
2026-05-06 22:53 ` [Intel-wired-lan] [PATCH iwl-next v2 0/5] ice: five small fixes and cleanups Jacob Keller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox