>From: Kitszel, Przemyslaw <przemyslaw.kitszel@xxxxxxxxx>
>Sent: Tuesday, August 26, 2025 10:33 PM
>
>On 8/26/25 17:31, Arkadiusz Kubalewski wrote:
>> 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.
>>
>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@xxxxxxxxx>
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>
>> ---
>> .../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(-)
>>
>
>
>> +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
>
>> + 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
>
>> +
>> + 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
>
>> + 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)
>
>> +
>> + 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
>
>> + memcpy(out, &buff[last_status], sizeof(*out));
>> + else
>> + memset(out, 0, sizeof(*out));
>
>weird, but let it be
>
>> +
>> + return ret;
>> +}
Sure, agree with all the comments, will fix in v2.
Thank you!
Arkadiusz
