lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <DM6PR11MB4657B554D7E31C6157E60D5A9B8F2@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Thu, 22 Aug 2024 22:30:54 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"vadim.fedorenko@...ux.dev" <vadim.fedorenko@...ux.dev>, "corbet@....net"
	<corbet@....net>, "davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "kuba@...nel.org"
	<kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"donald.hunter@...il.com" <donald.hunter@...il.com>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, "intel-wired-lan@...ts.osuosl.org"
	<intel-wired-lan@...ts.osuosl.org>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Loktionov, Aleksandr"
	<aleksandr.loktionov@...el.com>
Subject: RE: [PATCH net-next v2 2/2] ice: add callbacks for Embedded SYNC
 enablement on dpll pins

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Thursday, August 22, 2024 12:29 PM
>
>Wed, Aug 21, 2024 at 11:32:18PM CEST, arkadiusz.kubalewski@...el.com wrote:
>>Allow the user to get and set configuration of Embedded SYNC feature
>>on the ice driver dpll pins.
>>
>>Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>>---
>>v2:
>>- align to v2 changes of "dpll: add Embedded SYNC feature for a pin"
>>
>> drivers/net/ethernet/intel/ice/ice_dpll.c | 230 +++++++++++++++++++++-
>> drivers/net/ethernet/intel/ice/ice_dpll.h |   1 +
>> 2 files changed, 228 insertions(+), 3 deletions(-)
>
>Looks ok, couple of nitpicks below:
>
>
>
>>
>>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c
>>b/drivers/net/ethernet/intel/ice/ice_dpll.c
>>index e92be6f130a3..aa6b87281ea6 100644
>>--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
>>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
>>@@ -9,6 +9,7 @@
>> #define ICE_CGU_STATE_ACQ_ERR_THRESHOLD		50
>> #define ICE_DPLL_PIN_IDX_INVALID		0xff
>> #define ICE_DPLL_RCLK_NUM_PER_PF		1
>>+#define ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT	25
>>
>> /**
>>  * enum ice_dpll_pin_type - enumerate ice pin types:
>>@@ -30,6 +31,10 @@ static const char * const pin_type_name[] = {
>> 	[ICE_DPLL_PIN_TYPE_RCLK_INPUT] = "rclk-input",
>> };
>>
>>+static const struct dpll_pin_frequency ice_esync_range[] = {
>>+	DPLL_PIN_FREQUENCY_RANGE(0, DPLL_PIN_FREQUENCY_1_HZ),
>>+};
>>+
>> /**
>>  * ice_dpll_is_reset - check if reset is in progress
>>  * @pf: private board structure
>>@@ -394,8 +399,8 @@ ice_dpll_pin_state_update(struct ice_pf *pf, struct
>>ice_dpll_pin *pin,
>>
>> 	switch (pin_type) {
>> 	case ICE_DPLL_PIN_TYPE_INPUT:
>>-		ret = ice_aq_get_input_pin_cfg(&pf->hw, pin->idx, NULL, NULL,
>>-					       NULL, &pin->flags[0],
>>+		ret = ice_aq_get_input_pin_cfg(&pf->hw, pin->idx, &pin->status,
>>+					       NULL, NULL, &pin->flags[0],
>> 					       &pin->freq, &pin->phase_adjust);
>> 		if (ret)
>> 			goto err;
>>@@ -430,7 +435,7 @@ ice_dpll_pin_state_update(struct ice_pf *pf, struct
>>ice_dpll_pin *pin,
>> 			goto err;
>>
>> 		parent &= ICE_AQC_GET_CGU_OUT_CFG_DPLL_SRC_SEL;
>>-		if (ICE_AQC_SET_CGU_OUT_CFG_OUT_EN & pin->flags[0]) {
>>+		if (ICE_AQC_GET_CGU_OUT_CFG_OUT_EN & pin->flags[0]) {
>> 			pin->state[pf->dplls.eec.dpll_idx] =
>> 				parent == pf->dplls.eec.dpll_idx ?
>> 				DPLL_PIN_STATE_CONNECTED :
>>@@ -1098,6 +1103,221 @@ ice_dpll_phase_offset_get(const struct dpll_pin *pin,
>>void *pin_priv,
>> 	return 0;
>> }
>>
>>+/**
>>+ * ice_dpll_output_esync_set - callback for setting embedded sync
>>+ * @pin: pointer to a pin
>>+ * @pin_priv: private data pointer passed on pin registration
>>+ * @dpll: registered dpll pointer
>>+ * @dpll_priv: private data pointer passed on dpll registration
>>+ * @esync_freq: requested embedded sync frequency
>>+ * @extack: error reporting
>>+ *
>>+ * Dpll subsystem callback. Handler for setting embedded sync frequency
>>value
>>+ * on output pin.
>>+ *
>>+ * Context: Acquires pf->dplls.lock
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - error
>>+ */
>>+static int
>>+ice_dpll_output_esync_set(const struct dpll_pin *pin, void *pin_priv,
>>+			  const struct dpll_device *dpll, void *dpll_priv,
>>+			  u64 esync_freq, struct netlink_ext_ack *extack)
>
>s/esync_freq/freq/
>

Fixed in v3.

>
>>+{
>>+	struct ice_dpll_pin *p = pin_priv;
>>+	struct ice_dpll *d = dpll_priv;
>>+	struct ice_pf *pf = d->pf;
>>+	u8 flags = 0;
>>+	int ret;
>>+
>>+	if (ice_dpll_is_reset(pf, extack))
>>+		return -EBUSY;
>>+	mutex_lock(&pf->dplls.lock);
>>+	if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_OUT_EN)
>>+		flags = ICE_AQC_SET_CGU_OUT_CFG_OUT_EN;
>>+	if (esync_freq == DPLL_PIN_FREQUENCY_1_HZ) {
>>+		if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN) {
>>+			ret = 0;
>>+		} else {
>>+			flags |= ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN;
>>+			ret = ice_aq_set_output_pin_cfg(&pf->hw, p->idx, flags,
>>+							0, 0, 0);
>>+		}
>>+	} else {
>>+		if (!(p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN)) {
>>+			ret = 0;
>>+		} else {
>>+			flags &= ~ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN;
>>+			ret = ice_aq_set_output_pin_cfg(&pf->hw, p->idx, flags,
>>+							0, 0, 0);
>>+		}
>>+	}
>>+	mutex_unlock(&pf->dplls.lock);
>>+	if (ret)
>>+		NL_SET_ERR_MSG_FMT(extack,
>>+				   "err:%d %s failed to set e-sync freq\n",
>>+				   ret,
>>+				   ice_aq_str(pf->hw.adminq.sq_last_status));
>
>
>See my comment to ice_dpll_input_esync_set(), same applies here.
>

OK.

>
>>+	return ret;
>>+}
>>+
>>+/**
>>+ * ice_dpll_output_esync_get - callback for getting embedded sync config
>>+ * @pin: pointer to a pin
>>+ * @pin_priv: private data pointer passed on pin registration
>>+ * @dpll: registered dpll pointer
>>+ * @dpll_priv: private data pointer passed on dpll registration
>>+ * @esync: on success holds embedded sync pin properties
>>+ * @extack: error reporting
>>+ *
>>+ * Dpll subsystem callback. Handler for getting embedded sync frequency
>>value
>>+ * and capabilities on output pin.
>>+ *
>>+ * Context: Acquires pf->dplls.lock
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - error
>>+ */
>>+static int
>>+ice_dpll_output_esync_get(const struct dpll_pin *pin, void *pin_priv,
>>+			  const struct dpll_device *dpll, void *dpll_priv,
>>+			  struct dpll_pin_esync *esync,
>>+			  struct netlink_ext_ack *extack)
>>+{
>>+	struct ice_dpll_pin *p = pin_priv;
>>+	struct ice_dpll *d = dpll_priv;
>>+	struct ice_pf *pf = d->pf;
>>+
>>+	if (ice_dpll_is_reset(pf, extack))
>>+		return -EBUSY;
>>+	mutex_lock(&pf->dplls.lock);
>>+	if (!(p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_ABILITY) ||
>>+	    p->freq != DPLL_PIN_FREQUENCY_10_MHZ) {
>>+		mutex_unlock(&pf->dplls.lock);
>>+		return -EOPNOTSUPP;
>>+	}
>>+	esync->range = ice_esync_range;
>>+	esync->range_num = ARRAY_SIZE(ice_esync_range);
>>+	if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN) {
>>+		esync->freq = DPLL_PIN_FREQUENCY_1_HZ;
>>+		esync->pulse = ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT;
>>+	} else {
>>+		esync->freq = 0;
>>+		esync->pulse = 0;
>>+	}
>>+	mutex_unlock(&pf->dplls.lock);
>>+	return 0;
>>+}
>>+
>>+/**
>>+ * ice_dpll_input_esync_set - callback for setting embedded sync
>>+ * @pin: pointer to a pin
>>+ * @pin_priv: private data pointer passed on pin registration
>>+ * @dpll: registered dpll pointer
>>+ * @dpll_priv: private data pointer passed on dpll registration
>>+ * @esync_freq: requested embedded sync frequency
>>+ * @extack: error reporting
>>+ *
>>+ * Dpll subsystem callback. Handler for setting embedded sync frequency
>>value
>>+ * on input pin.
>>+ *
>>+ * Context: Acquires pf->dplls.lock
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - error
>>+ */
>>+static int
>>+ice_dpll_input_esync_set(const struct dpll_pin *pin, void *pin_priv,
>>+			 const struct dpll_device *dpll, void *dpll_priv,
>>+			 u64 esync_freq, struct netlink_ext_ack *extack)
>>+{
>>+	struct ice_dpll_pin *p = pin_priv;
>>+	struct ice_dpll *d = dpll_priv;
>>+	struct ice_pf *pf = d->pf;
>>+	u8 flags_en = 0;
>>+	int ret;
>>+
>>+	if (ice_dpll_is_reset(pf, extack))
>>+		return -EBUSY;
>>+	mutex_lock(&pf->dplls.lock);
>>+	if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_INPUT_EN)
>>+		flags_en = ICE_AQC_SET_CGU_IN_CFG_FLG2_INPUT_EN;
>>+	if (esync_freq == DPLL_PIN_FREQUENCY_1_HZ) {
>>+		if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) {
>>+			ret = 0;
>>+		} else {
>>+			flags_en |= ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN;
>>+			ret = ice_aq_set_input_pin_cfg(&pf->hw, p->idx, 0,
>>+						       flags_en, 0, 0);
>>+		}
>>+	} else {
>>+		if (!(p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN)) {
>>+			ret = 0;
>>+		} else {
>>+			flags_en &= ~ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN;
>>+			ret = ice_aq_set_input_pin_cfg(&pf->hw, p->idx, 0,
>>+						       flags_en, 0, 0);
>>+		}
>>+	}
>>+	mutex_unlock(&pf->dplls.lock);
>>+	if (ret)
>>+		NL_SET_ERR_MSG_FMT(extack,
>>+				   "err:%d %s failed to set e-sync freq\n",
>
>Not sure how you do that in ice, but there should be a space after ":".
>But, in this case, print ret value in the message is redundant as you
>return ret value to the user. Remove.
>
>Moreover, this extack message has no value, as you basically say, that
>the command user executed failed, which he already knows by non-0 return
>value :) Either provide some useful details or avoid the extack message
>completely.
>

OK, makes sense to me, removed in v3.

Thank you!
Arkadiusz

>
>>+				   ret,
>>+				   ice_aq_str(pf->hw.adminq.sq_last_status));
>>+
>>+	return ret;
>>+}
>>+
>>+/**
>>+ * ice_dpll_input_esync_get - callback for getting embedded sync config
>>+ * @pin: pointer to a pin
>>+ * @pin_priv: private data pointer passed on pin registration
>>+ * @dpll: registered dpll pointer
>>+ * @dpll_priv: private data pointer passed on dpll registration
>>+ * @esync: on success holds embedded sync pin properties
>>+ * @extack: error reporting
>>+ *
>>+ * Dpll subsystem callback. Handler for getting embedded sync frequency
>>value
>>+ * and capabilities on input pin.
>>+ *
>>+ * Context: Acquires pf->dplls.lock
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - error
>>+ */
>>+static int
>>+ice_dpll_input_esync_get(const struct dpll_pin *pin, void *pin_priv,
>>+			 const struct dpll_device *dpll, void *dpll_priv,
>>+			 struct dpll_pin_esync *esync,
>>+			 struct netlink_ext_ack *extack)
>>+{
>>+	struct ice_dpll_pin *p = pin_priv;
>>+	struct ice_dpll *d = dpll_priv;
>>+	struct ice_pf *pf = d->pf;
>>+
>>+	if (ice_dpll_is_reset(pf, extack))
>>+		return -EBUSY;
>>+	mutex_lock(&pf->dplls.lock);
>>+	if (!(p->status & ICE_AQC_GET_CGU_IN_CFG_STATUS_ESYNC_CAP) ||
>>+	    p->freq != DPLL_PIN_FREQUENCY_10_MHZ) {
>>+		mutex_unlock(&pf->dplls.lock);
>>+		return -EOPNOTSUPP;
>>+	}
>>+	esync->range = ice_esync_range;
>>+	esync->range_num = ARRAY_SIZE(ice_esync_range);
>>+	if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) {
>>+		esync->freq = DPLL_PIN_FREQUENCY_1_HZ;
>>+		esync->pulse = ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT;
>>+	} else {
>>+		esync->freq = 0;
>>+		esync->pulse = 0;
>>+	}
>>+	mutex_unlock(&pf->dplls.lock);
>>+	return 0;
>>+}
>>+
>> /**
>>  * ice_dpll_rclk_state_on_pin_set - set a state on rclk pin
>>  * @pin: pointer to a pin
>>@@ -1222,6 +1442,8 @@ static const struct dpll_pin_ops ice_dpll_input_ops = {
>> 	.phase_adjust_get = ice_dpll_pin_phase_adjust_get,
>> 	.phase_adjust_set = ice_dpll_input_phase_adjust_set,
>> 	.phase_offset_get = ice_dpll_phase_offset_get,
>>+	.esync_set = ice_dpll_input_esync_set,
>>+	.esync_get = ice_dpll_input_esync_get,
>> };
>>
>> static const struct dpll_pin_ops ice_dpll_output_ops = {
>>@@ -1232,6 +1454,8 @@ static const struct dpll_pin_ops ice_dpll_output_ops =
>>{
>> 	.direction_get = ice_dpll_output_direction,
>> 	.phase_adjust_get = ice_dpll_pin_phase_adjust_get,
>> 	.phase_adjust_set = ice_dpll_output_phase_adjust_set,
>>+	.esync_set = ice_dpll_output_esync_set,
>>+	.esync_get = ice_dpll_output_esync_get,
>> };
>>
>> static const struct dpll_device_ops ice_dpll_ops = {
>>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h
>>b/drivers/net/ethernet/intel/ice/ice_dpll.h
>>index 93172e93995b..c320f1bf7d6d 100644
>>--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
>>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
>>@@ -31,6 +31,7 @@ struct ice_dpll_pin {
>> 	struct dpll_pin_properties prop;
>> 	u32 freq;
>> 	s32 phase_adjust;
>>+	u8 status;
>> };
>>
>> /** ice_dpll - store info required for DPLL control
>>--
>>2.38.1
>>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ