>From: Kitszel, Przemyslaw <[email protected]> >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 <[email protected]> >> Signed-off-by: Arkadiusz Kubalewski <[email protected]> >> --- >> .../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
