[PATCH iwl-net] ice: dpll: refuse pin states that are not valid for the pin

[PATCH iwl-net] ice: dpll: refuse pin states that are not valid for the pin

[Petr Oros]

The DPLL core checks only that a pin may change its state, which states
a pin accepts is left to the driver. The ice callbacks turn the
requested state into an enable flag by comparing it with the one state
that enables the pin, so every other state disables it, including the
one that has no meaning for the pin.

The inputs of the EEC and PPS DPLLs are enabled by selectable, as these
DPLLs select their reference automatically. A request for connected,
which Documentation/driver-api/dpll.rst reserves for manual mode and
for pins with the state-connected-override capability, disables the
input. On an E810:

  # dpll pin set id 1 parent-device 0 state connected
  # dpll pin show id 1
  ... state disconnected on both DPLLs

A recovered clock pin is connected to or disconnected from its parent
pin, and a request for selectable disconnects it:

  # dpll pin set id 2109 parent-pin 2094 state selectable
  # dpll pin show id 2109
  ... state disconnected on parent 2094

Add a helper that translates the state for a pin given the state that
enables it. Disconnected disables the pin and anything else is refused
with -EINVAL and an extack message. Use it for the input and recovered
clock pins, and for the output pins, which already refused selectable
and now tell the user why.

The SMA pins have the same problem. It is left out here, as the rework
of the SMA and U.FL pin control that is queued for net-next refuses the
states these pins cannot serve.

Found with the pin state selftest of the dpllsim series running against
an ice E810.

Fixes: d7999f5ea64bb1 ("ice: implement dpll interface to control cgu")
Signed-off-by: Petr Oros <poros@redhat.com>
---
 drivers/net/ethernet/intel/ice/ice_dpll.c | 61 ++++++++++++++++++++---
 1 file changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
index ade1fb3dd06743..b2b515420bc8cd 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -961,6 +961,40 @@ static int ice_dpll_phase_offset_monitor_get(const struct dpll_device *dpll,
 	return 0;
 }
 
+/**
+ * ice_dpll_pin_state_to_enable - translate a requested pin state
+ * @state: pin state requested by the user
+ * @on_state: the state that enables the pin
+ * @enable: on success true to enable the pin, false to disable it
+ * @extack: error reporting
+ *
+ * A pin is enabled by exactly one state: selectable for an input of the
+ * EEC and PPS DPLLs, which select their reference automatically, and
+ * connected for an output and for a recovered clock on its parent pin.
+ * Disconnected disables the pin. No other state has a meaning for the
+ * pin, so it is refused rather than taken for one of the two.
+ *
+ * Return:
+ * * 0 - success, @enable is set
+ * * -EINVAL - @state is not valid for the pin
+ */
+static int
+ice_dpll_pin_state_to_enable(enum dpll_pin_state state,
+			     enum dpll_pin_state on_state, bool *enable,
+			     struct netlink_ext_ack *extack)
+{
+	if (state != on_state && state != DPLL_PIN_STATE_DISCONNECTED) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "pin state must be %s or disconnected",
+				   on_state == DPLL_PIN_STATE_SELECTABLE ?
+				   "selectable" : "connected");
+		return -EINVAL;
+	}
+	*enable = state == on_state;
+
+	return 0;
+}
+
 /**
  * ice_dpll_pin_state_set - set pin's state on dpll
  * @pin: pointer to a pin
@@ -1027,12 +1061,15 @@ ice_dpll_output_state_set(const struct dpll_pin *pin, void *pin_priv,
 			  enum dpll_pin_state state,
 			  struct netlink_ext_ack *extack)
 {
-	bool enable = state == DPLL_PIN_STATE_CONNECTED;
 	struct ice_dpll_pin *p = pin_priv;
 	struct ice_dpll *d = dpll_priv;
+	bool enable;
+	int ret;
 
-	if (state == DPLL_PIN_STATE_SELECTABLE)
-		return -EINVAL;
+	ret = ice_dpll_pin_state_to_enable(state, DPLL_PIN_STATE_CONNECTED,
+					   &enable, extack);
+	if (ret)
+		return ret;
 	if (!enable && p->state[d->dpll_idx] == DPLL_PIN_STATE_DISCONNECTED)
 		return 0;
 
@@ -1062,7 +1099,13 @@ ice_dpll_input_state_set(const struct dpll_pin *pin, void *pin_priv,
 			 enum dpll_pin_state state,
 			 struct netlink_ext_ack *extack)
 {
-	bool enable = state == DPLL_PIN_STATE_SELECTABLE;
+	bool enable;
+	int ret;
+
+	ret = ice_dpll_pin_state_to_enable(state, DPLL_PIN_STATE_SELECTABLE,
+					   &enable, extack);
+	if (ret)
+		return ret;
 
 	return ice_dpll_pin_state_set(pin, pin_priv, dpll, dpll_priv, enable,
 				      extack, ICE_DPLL_PIN_TYPE_INPUT);
@@ -2518,18 +2561,24 @@ ice_dpll_rclk_state_on_pin_set(const struct dpll_pin *pin, void *pin_priv,
 			       enum dpll_pin_state state,
 			       struct netlink_ext_ack *extack)
 {
-	bool enable = state == DPLL_PIN_STATE_CONNECTED;
 	struct ice_dpll_pin *p = pin_priv;
 	struct ice_pf *pf = p->pf;
 	struct ice_hw *hw;
-	int ret = -EINVAL;
+	bool enable;
 	int hw_idx;
+	int ret;
 
 	hw = &pf->hw;
 
 	if (ice_dpll_is_reset(pf, extack))
 		return -EBUSY;
 
+	ret = ice_dpll_pin_state_to_enable(state, DPLL_PIN_STATE_CONNECTED,
+					   &enable, extack);
+	if (ret)
+		return ret;
+
+	ret = -EINVAL;
 	mutex_lock(&pf->dplls.lock);
 	hw_idx = ice_dpll_pin_get_parent_idx(p, parent_pin);
 	if (hw_idx < 0)
-- 
2.56.0

Re: [PATCH iwl-net] ice: dpll: refuse pin states that are not valid for the pin

[Ivan Vecera]

On 11/10/26 15:31, Petr Oros wrote:
> The DPLL core checks only that a pin may change its state, which states
> a pin accepts is left to the driver. The ice callbacks turn the
> requested state into an enable flag by comparing it with the one state
> that enables the pin, so every other state disables it, including the
> one that has no meaning for the pin.
> 
> The inputs of the EEC and PPS DPLLs are enabled by selectable, as these
> DPLLs select their reference automatically. A request for connected,
> which Documentation/driver-api/dpll.rst reserves for manual mode and
> for pins with the state-connected-override capability, disables the
> input. On an E810:
> 
>    # dpll pin set id 1 parent-device 0 state connected
>    # dpll pin show id 1
>    ... state disconnected on both DPLLs
> 
> A recovered clock pin is connected to or disconnected from its parent
> pin, and a request for selectable disconnects it:
> 
>    # dpll pin set id 2109 parent-pin 2094 state selectable
>    # dpll pin show id 2109
>    ... state disconnected on parent 2094
> 
> Add a helper that translates the state for a pin given the state that
> enables it. Disconnected disables the pin and anything else is refused
> with -EINVAL and an extack message. Use it for the input and recovered
> clock pins, and for the output pins, which already refused selectable
> and now tell the user why.
> 
> The SMA pins have the same problem. It is left out here, as the rework
> of the SMA and U.FL pin control that is queued for net-next refuses the
> states these pins cannot serve.
> 
> Found with the pin state selftest of the dpllsim series running against
> an ice E810.

;-) good catch!
> 
> Fixes: d7999f5ea64bb1 ("ice: implement dpll interface to control cgu")
> Signed-off-by: Petr Oros<poros@redhat.com>

Reviewed-by: Ivan Vecera <ivecera@redhat.com>