linux-kernel - RE: [PATCH iwl-next v1] ice: add support for unmanaged dpll on E830 NIC

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: <SJ2PR11MB845213F9695C92849A2ADDB19B3AA@SJ2PR11MB8452.namprd11.prod.outlook.com>
Date: Fri, 29 Aug 2025 08:30:26 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: "Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>, "Loktionov,
 Aleksandr" <aleksandr.loktionov@...el.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "intel-wired-lan@...ts.osuosl.org"
	<intel-wired-lan@...ts.osuosl.org>
Subject: RE: [PATCH iwl-next v1] ice: add support for unmanaged dpll on E830
 NIC

>From: Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com>
>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@...el.com>
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.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(-)
>>
>
>
>> +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