netdev - RE: [PATCH net-next v2 6/9] ice: add admin commands to access cgu configuration

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM6PR11MB46577E9609111FDE34A4F7469B0CA@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Mon, 7 Aug 2023 23:08:13 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Simon Horman <horms@...nel.org>, Vadim Fedorenko
	<vadim.fedorenko@...ux.dev>
CC: Jakub Kicinski <kuba@...nel.org>, Jiri Pirko <jiri@...nulli.us>, "Jonathan
 Lemon" <jonathan.lemon@...il.com>, Paolo Abeni <pabeni@...hat.com>, "Olech,
 Milena" <milena.olech@...el.com>, "Michalik, Michal"
	<michal.michalik@...el.com>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, poros <poros@...hat.com>, mschmidt
	<mschmidt@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>, Bart Van Assche
	<bvanassche@....org>, "intel-wired-lan@...ts.osuosl.org"
	<intel-wired-lan@...ts.osuosl.org>
Subject: RE: [PATCH net-next v2 6/9] ice: add admin commands to access cgu
 configuration

>From: Simon Horman <horms@...nel.org>
>Sent: Sunday, August 6, 2023 7:32 PM
>
>On Fri, Aug 04, 2023 at 08:04:51PM +0100, Vadim Fedorenko wrote:
>> From: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>>
>> Add firmware admin command to access clock generation unit
>> configuration, it is required to enable Extended PTP and SyncE features
>> in the driver.
>> Add definitions of possible hardware variations of input and output pins
>> related to clock generation unit and functions to access the data.
>>
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
>
>Hi Arkadiusz and Vadim,
>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
>b/drivers/net/ethernet/intel/ice/ice_common.c
>
>...
>
>> +/**
>> + * ice_aq_get_cgu_dpll_status - get dpll status
>> + * @hw: pointer to the HW struct
>> + * @dpll_num: DPLL index
>> + * @ref_state: Reference clock state
>> + * @config: current DPLL config
>> + * @dpll_state: current DPLL state
>> + * @phase_offset: Phase offset in ns
>> + * @eec_mode: EEC_mode
>> + *
>> + * Get CGU DPLL status (0x0C66)
>> + * Return: 0 on success or negative value on failure.
>> + */
>> +int
>> +ice_aq_get_cgu_dpll_status(struct ice_hw *hw, u8 dpll_num, u8
>>*ref_state,
>> +			   u8 *dpll_state, u8 *config, s64 *phase_offset,
>> +			   u8 *eec_mode)
>> +{
>> +	struct ice_aqc_get_cgu_dpll_status *cmd;
>> +	const s64 NSEC_PER_PSEC = 1000LL;
>
>Probably this should be in lower case, or an (upper case) #define.
>In the case of the latter it should probably be moved outside of the
>function.

Hi Simon,

Sure, will fix.

>
>> +	struct ice_aq_desc desc;
>> +	int status;
>> +
>> +	ice_fill_dflt_direct_cmd_desc(&desc,
>>ice_aqc_opc_get_cgu_dpll_status);
>> +	cmd = &desc.params.get_cgu_dpll_status;
>> +	cmd->dpll_num = dpll_num;
>> +
>> +	status = ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
>> +	if (!status) {
>> +		*ref_state = cmd->ref_state;
>> +		*dpll_state = cmd->dpll_state;
>> +		*config = cmd->config;
>> +		*phase_offset = le32_to_cpu(cmd->phase_offset_h);
>> +		*phase_offset <<= 32;
>> +		*phase_offset += le32_to_cpu(cmd->phase_offset_l);
>> +		*phase_offset = sign_extend64(*phase_offset, 47) /
>> +			NSEC_PER_PSEC;
>
>This causes a build failure on x86_32.
>
>  ERROR: modpost: "__divdi3" [drivers/net/ethernet/intel/ice/ice.ko]
>undefined!
>
>Possibly you want (please do check for yourself):
>
>		*phase_offset = div64_s64(sign_extend64(*phase_offset, 47),
>					  NSEC_PER_PSEC);
>

Yes, makes sense, thanks for catching this, will fix.

>> +		*eec_mode = cmd->eec_mode;
>> +	}
>> +
>> +	return status;
>> +}
>> +
>> +/**
>> + * ice_aq_set_cgu_dpll_config - set dpll config
>> + * @hw: pointer to the HW struct
>> + * @dpll_num: DPLL index
>> + * @ref_state: Reference clock state
>> + * @config: DPLL config
>> + * @eec_mode: EEC mode
>> + *
>> + * Set CGU DPLL config (0x0C67)
>> + * Return: 0 on success or negative value on failure.
>> + */
>> +int
>> +ice_aq_set_cgu_dpll_config(struct ice_hw *hw, u8 dpll_num, u8 ref_state,
>> +			   u8 config, u8 eec_mode)
>> +{
>> +	struct ice_aqc_set_cgu_dpll_config *cmd;
>> +	struct ice_aq_desc desc;
>> +
>> +	ice_fill_dflt_direct_cmd_desc(&desc,
>>ice_aqc_opc_set_cgu_dpll_config);
>> +	cmd = &desc.params.set_cgu_dpll_config;
>> +	cmd->dpll_num = dpll_num;
>> +	cmd->ref_state = ref_state;
>> +	cmd->config = config;
>> +	cmd->eec_mode = eec_mode;
>> +
>> +	return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
>> +}
>> +
>> +/**
>> + * ice_aq_set_cgu_ref_prio - set input refernce priority
>
>nit: refernce -> reference
>

Sure, will fix.

Thank you!
Arkadiusz

>> + * @hw: pointer to the HW struct
>> + * @dpll_num: DPLL index
>> + * @ref_idx: Reference pin index
>> + * @ref_priority: Reference input priority
>> + *
>> + * Set CGU reference priority (0x0C68)
>> + * Return: 0 on success or negative value on failure.
>> + */
>
>...
>
>--
>pw-bot: changes-requested