Re: [PATCH net 11/11] ice: add dpll peer notification for paired SMA and U.FL pins

[Jacob Keller <jacob.e.keller@intel.com>]

On 4/27/2026 10:22 PM, Jacob Keller wrote:
> From: Petr Oros <poros@redhat.com>
> 
> SMA and U.FL pins share physical signal paths in pairs (SMA1/U.FL1 and
> SMA2/U.FL2).  When one pin's state changes via a PCA9575 GPIO write,
> the paired pin's state also changes, but no notification is sent for
> the peer pin.  Userspace consumers monitoring the peer via dpll netlink
> subscribe never learn about the update.
> 
> Add ice_dpll_sw_pin_notify_peer() which sends a change notification for
> the paired SW pin.  Call it from ice_dpll_pin_sma_direction_set(),
> ice_dpll_sma_pin_state_set(), and ice_dpll_ufl_pin_state_set() after
> pf->dplls.lock is released.  Use __dpll_pin_change_ntf() because
> dpll_lock is still held by the dpll netlink layer (dpll_pin_pre_doit).
> 
> Fixes: 2dd5d03c77e2 ("ice: redesign dpll sma/u.fl pins control")
> Signed-off-by: Petr Oros <poros@redhat.com>
> Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>
> Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---

Sashiko.dev says the following:

> commit f5e294c42ef0c3cf4bca4497a268b68f562bce96
> Author: Petr Oros <poros@redhat.com>
> ice: add dpll peer notification for paired SMA and U.FL pins
> SMA and U.FL pins share physical signal paths in pairs. This patch adds
> ice_dpll_sw_pin_notify_peer() to send a change notification for the
> paired SW pin when its state changes.
>> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
>> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
> [ ... ]
>> @@ -1344,6 +1370,8 @@ ice_dpll_ufl_pin_state_set(const struct dpll_pin *pin, void *pin_priv,
>>  
>>  unlock:
>>  	mutex_unlock(&pf->dplls.lock);
>> +	if (!ret)
>> +		ice_dpll_sw_pin_notify_peer(&pf->dplls, p);
> Could the hardware routing state become permanently out of sync with the
> userspace view if an error occurs here?
> While this patch doesn't introduce the underlying behavior, if enabling or
> disabling the backing CGU pin via ice_dpll_pin_enable() or
> ice_dpll_pin_disable() fails earlier in this function, an error is returned
> without rolling back the successful SMA control register write in
> ice_write_sma_ctrl().
> Since ret is non-zero on this error path, the ice_dpll_sw_pin_notify_peer()
> call is skipped, and the DPLL netlink core suppresses the change notification
> for the original pin.
> Does this leave the hardware state irreversibly altered while userspace
> receives an error, assumes no change occurred, and receives no notifications?
> This same question applies to ice_dpll_sma_pin_state_set() below.
>>  
This reads to me like there is a possible issue in the existing code
which is separate from the issues this tries to address.

It looks like ice_dpll_pin_enable() can fail if the associated AdminQ
command fails. If we want to cleanly handle this, we likely need
unrolling logic which I think is an independent fix from anything in
this series.

I'm not sure what that unrolling logic would look like though. Maybe we
just need to undo the ice_write_sma_ctrl? But that also looks like an
AdminQ command we have to send.

I'm not sure how careful we need to be here in this flow.

Either way, to my eyes this looks like a pre-existing issue that may
need some attention but I don't feel like it should block these fixes.

Thanks,
Jake