Messages in this thread |  | | From | "Kubalewski, Arkadiusz" <> | | Subject | RE: [PATCH iwl-net v2] ice: fix SMA and U.FL pin state changes affecting paired pin | | Date | Mon, 27 Apr 2026 14:14:33 +0000 |
| |
>From: Petr Oros <poros@redhat.com> >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@intel.com>
>Signed-off-by: Petr Oros <poros@redhat.com> >--- >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@redhat.com/ >--- > 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
|  |