RE: [PATCH iwl-net v2] ice: fix SMA and U.FL pin state changes affecting paired pin

[Kubalewski, Arkadiusz]

>From: Petr Oros <poros@xxxxxxxxxx>
>Sent: Wednesday, April 8, 2026 1:05 PM
>
>SMA and U.FL pins share physical signal paths in pairs (SMA1/U.FL1 and
>SMA2/U.FL2) controlled by the PCA9575 GPIO expander.  Each pair can
>only have one active pin at a time: SMA1 output and U.FL1 output share
>the same CGU output, SMA2 input and U.FL2 input share the same CGU
>input.  The PCA9575 register bits determine which connector in each
>pair owns the signal path.
>
>The driver does not account for this pairing in two places:
>
>ice_dpll_ufl_pin_state_set() modifies PCA9575 bits and disables the
>backing CGU pin without checking whether the U.FL pin is currently
>active.  Disconnecting an already inactive U.FL pin flips bits that
>the paired SMA pin relies on, breaking its connection.
>
>ice_dpll_sma_direction_set() does not propagate direction changes to
>the paired U.FL pin.  For SMA2/U.FL2 the ICE_SMA2_UFL2_RX_DIS bit is
>never managed, so U.FL2 stays disconnected after SMA2 switches to
>output.  For both pairs the backing CGU pin of the U.FL side is never
>enabled when a direction change activates it, so userspace sees the
>pin as disconnected even though the routing is correct.
>
>Fix by guarding the U.FL disconnect path against inactive pins and by
>updating the paired U.FL pin fully on SMA direction changes: manage
>ICE_SMA2_UFL2_RX_DIS for the SMA2/U.FL2 pair and enable the backing
>CGU pin whenever the peer becomes active.
>
>Fixes: 2dd5d03c77e2 ("ice: redesign dpll sma/u.fl pins control")

LGTM,

Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>

>Signed-off-by: Petr Oros <poros@xxxxxxxxxx>
>---
>v2:
> - fix ice_dpll_sma_direction_set() to manage ICE_SMA2_UFL2_RX_DIS
>   when SMA2 direction changes
> - enable paired U.FL backing CGU pin when direction change makes
>   it active, so it reports as connected immediately
> - (both reported by Intel test on the SMA init and notification
>   patch threads)
>v1: https://lore.kernel.org/all/20260325151050.2081977-1-poros@xxxxxxxxxx/
>---
> drivers/net/ethernet/intel/ice/ice_dpll.c | 50 ++++++++++++++++++++++-
> 1 file changed, 49 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c
>b/drivers/net/ethernet/intel/ice/ice_dpll.c
>index 498ec2c045f384..3f8cd5b8298b57 100644
>--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
>@@ -1171,6 +1171,8 @@ static int ice_dpll_sma_direction_set(struct
>ice_dpll_pin *p,
> 				      enum dpll_pin_direction direction,
> 				      struct netlink_ext_ack *extack)
> {
>+	struct ice_dplls *d = &p->pf->dplls;
>+	struct ice_dpll_pin *peer;
> 	u8 data;
> 	int ret;
>
>@@ -1189,8 +1191,9 @@ static int ice_dpll_sma_direction_set(struct
>ice_dpll_pin *p,
> 	case ICE_DPLL_PIN_SW_2_IDX:
> 		if (direction == DPLL_PIN_DIRECTION_INPUT) {
> 			data &= ~ICE_SMA2_DIR_EN;
>+			data |= ICE_SMA2_UFL2_RX_DIS;
> 		} else {
>-			data &= ~ICE_SMA2_TX_EN;
>+			data &= ~(ICE_SMA2_TX_EN | ICE_SMA2_UFL2_RX_DIS);
> 			data |= ICE_SMA2_DIR_EN;
> 		}
> 		break;
>@@ -1202,6 +1205,34 @@ static int ice_dpll_sma_direction_set(struct
>ice_dpll_pin *p,
> 		ret = ice_dpll_pin_state_update(p->pf, p,
> 						ICE_DPLL_PIN_TYPE_SOFTWARE,
> 						extack);
>+	if (ret)
>+		return ret;
>+
>+	/* When a direction change activates the paired U.FL pin, enable
>+	 * its backing CGU pin so the pin reports as connected. Without
>+	 * this the U.FL routing is correct but the CGU pin stays disabled
>+	 * and userspace sees the pin as disconnected.  Do not disable the
>+	 * backing pin when U.FL becomes inactive because the SMA pin may
>+	 * still be using it.
>+	 */
>+	peer = &d->ufl[p->idx];
>+	if (peer->active) {
>+		struct ice_dpll_pin *target;
>+		enum ice_dpll_pin_type type;
>+
>+		if (peer->output) {
>+			target = peer->output;
>+			type = ICE_DPLL_PIN_TYPE_OUTPUT;
>+		} else {
>+			target = peer->input;
>+			type = ICE_DPLL_PIN_TYPE_INPUT;
>+		}
>+		ret = ice_dpll_pin_enable(&p->pf->hw, target,
>+					  d->eec.dpll_idx, type, extack);
>+		if (!ret)
>+			ret = ice_dpll_pin_state_update(p->pf, target,
>+							type, extack);
>+	}
>
> 	return ret;
> }
>@@ -1253,6 +1284,14 @@ ice_dpll_ufl_pin_state_set(const struct dpll_pin
>*pin, void *pin_priv,
> 			data &= ~ICE_SMA1_MASK;
> 			enable = true;
> 		} else if (state == DPLL_PIN_STATE_DISCONNECTED) {
>+			/* Skip if U.FL1 is not active, setting TX_EN
>+			 * while DIR_EN is set would also deactivate
>+			 * the paired SMA1 output.
>+			 */
>+			if (data & (ICE_SMA1_DIR_EN | ICE_SMA1_TX_EN)) {
>+				ret = 0;
>+				goto unlock;
>+			}
> 			data |= ICE_SMA1_TX_EN;
> 			enable = false;
> 		} else {
>@@ -1267,6 +1306,15 @@ ice_dpll_ufl_pin_state_set(const struct dpll_pin
>*pin, void *pin_priv,
> 			data &= ~ICE_SMA2_UFL2_RX_DIS;
> 			enable = true;
> 		} else if (state == DPLL_PIN_STATE_DISCONNECTED) {
>+			/* Skip if U.FL2 is not active, setting
>+			 * UFL2_RX_DIS could also disable the paired
>+			 * SMA2 input.
>+			 */
>+			if (!(data & ICE_SMA2_DIR_EN) ||
>+			    (data & ICE_SMA2_UFL2_RX_DIS)) {
>+				ret = 0;
>+				goto unlock;
>+			}
> 			data |= ICE_SMA2_UFL2_RX_DIS;
> 			enable = false;
> 		} else {
>--
>2.52.0