From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
Sent: Tuesday, August 26, 2025 10:33 PM

On 8/26/25 17:31, Arkadiusz Kubalewski wrote:

quoted

Hardware variants of E830 may support an unmanaged DPLL where the
configuration is hardcoded within the hardware and firmware, meaning
users cannot modify settings. However, users are able to check the
DPLL lock status and obtain configuration information through the
Linux DPLL subsystem.

Availability of 'loss of lock' health status code determines if such
support is available, if true, register single dpll device with 1
input and 1 output and provide hardcoded/read only properties of a pin
and dpll device. User is only allowed to check dpll device status and
receive notifications on dpll lock status change.

When present, the DPLL device locks to an external signal provided
through the PCIe/OCP pin. The expected input signal is 1PPS
(1 Pulse Per Second) embedded on a 10MHz reference clock.
The DPLL produces output:
- for MAC (Media Access Control) & PHY (Physical Layer) clocks,
- 1PPS for synchronization of onboard PHC (Precision Hardware Clock)

timer.

quoted

Reviewed-by: Aleksandr Loktionov <redacted>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
  .../device_drivers/ethernet/intel/ice.rst     |  83 +++++
  .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   4 +
  drivers/net/ethernet/intel/ice/ice_common.c   | 110 +++++++
  drivers/net/ethernet/intel/ice/ice_common.h   |   7 +
  drivers/net/ethernet/intel/ice/ice_dpll.c     | 306 ++++++++++++++++--
  drivers/net/ethernet/intel/ice/ice_dpll.h     |  11 +
  drivers/net/ethernet/intel/ice/ice_main.c     |  14 +-
  drivers/net/ethernet/intel/ice/ice_ptp_hw.c   |  46 +++
  drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |   1 +
  9 files changed, 560 insertions(+), 22 deletions(-)

quoted

+int ice_is_health_status_code_supported(struct ice_hw *hw, u16 code,
+					bool *supported)
+{
+	struct ice_aqc_health_status_elem *buff __free(kfree) = NULL;

observation: netdev maintainers don't like __free

quoted

+	const int BUFF_SIZE = ICE_AQC_HEALTH_STATUS_CODE_NUM;
+	int ret;
+
+	*supported = false;

it's best to don't change output pointers on error

quoted

+
+	buff = kcalloc(BUFF_SIZE, sizeof(*buff), GFP_KERNEL);
+	if (!buff)
+		return -ENOMEM;
+	ret = ice_aq_get_health_status_supported(hw, buff, BUFF_SIZE);
+	if (ret)
+		return ret;
+	for (int i = 0; i < BUFF_SIZE && buff[i].health_status_code; i++)
+		if (le16_to_cpu(buff[i].health_status_code) == code) {
+			*supported = true;

so, this line should be enough

quoted

+			break;
+		}
+
+	return 0;
+}
+
+/**
+ * ice_get_last_health_status_code - get last health status for given
+code
+ * @hw: pointer to the hardware structure
+ * @out: pointer to the health status struct to be filled
+ * @code: health status code to check
+ *
+ * Return: 0 on success, negative error code otherwise  */ int
+ice_get_last_health_status_code(struct ice_hw *hw,
+				    struct ice_aqc_health_status_elem *out,
+				    u16 code)
+{
+	struct ice_aqc_health_status_elem *buff __free(kfree) = NULL;
+	const int BUFF_SIZE = ICE_AQC_HEALTH_STATUS_CODE_NUM;
+	int last_status = -1, ret;

nit: variables with initial value set should be on the end of given
declaration line (here @ret should be first)

quoted

+
+	buff = kcalloc(BUFF_SIZE, sizeof(*buff), GFP_KERNEL);
+	if (!buff)
+		return -ENOMEM;
+	ret = ice_aq_get_health_status(hw, buff, BUFF_SIZE);
+	if (ret)
+		return ret;
+	for (int i = 0; i < BUFF_SIZE && buff[i].health_status_code; i++)
+		if (le16_to_cpu(buff[i].health_status_code) == code)
+			last_status = i;
+
+	if (last_status >= 0 && last_status < BUFF_SIZE)

second part of the contidion is always true

quoted

+		memcpy(out, &buff[last_status], sizeof(*out));
+	else
+		memset(out, 0, sizeof(*out));

weird, but let it be

quoted

+
+	return ret;
+}

Sure, agree with all the comments, will fix in v2.

Thank you!
Arkadiusz