| 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
| ||
|
Message-ID: <DM6PR11MB4657AE3296BEBC3A094EF7CF9B06A@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Fri, 28 Jul 2023 12:46:39 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Simon Horman <simon.horman@...igine.com>, 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>
Subject: RE: [PATCH net-next 08/11] ice: add admin commands to access cgu
configuration
>From: Simon Horman <simon.horman@...igine.com>
>Sent: Monday, July 24, 2023 7:22 PM
>
>On Thu, Jul 20, 2023 at 10:19:00AM +0100, Vadim Fedorenko wrote:
>
>...
>
>Hi 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
>> + * @hw: pointer to the HW struct
>> + * @dpll_num: DPLL index
>> + * @ref_state: Reference clock state
>> + * @dpll_state: DPLL state
>
>./scripts/kernel-doc says that @config is missing here.
>
Sure, will fix.
>> + * @phase_offset: Phase offset in ns
>> + * @eec_mode: EEC_mode
>> + *
>> + * Get CGU DPLL status (0x0C66)
>> + */
>> +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)
>
>...
>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>
>...
>
>> +/**
>> + * ice_get_cgu_state - get the state of the DPLL
>> + * @hw: pointer to the hw struct
>> + * @dpll_idx: Index of internal DPLL unit
>> + * @last_dpll_state: last known state of DPLL
>> + * @pin: pointer to a buffer for returning currently active pin
>> + * @ref_state: reference clock state
>
>Likewise, @eec_mode is missing here.
Sure, will fix.
>
>> + * @phase_offset: pointer to a buffer for returning phase offset
>> + * @dpll_state: state of the DPLL (output)
>
>And @mode is missing here.
>
Sure, will fix.
>> + *
>> + * This function will read the state of the DPLL(dpll_idx). Non-null
>> + * 'pin', 'ref_state', 'eec_mode' and 'phase_offset' parameters are used to
>> + * retrieve currently active pin, state, mode and phase_offset respectively.
>> + *
>> + * Return: state of the DPLL
>> + */
>> +int ice_get_cgu_state(struct ice_hw *hw, u8 dpll_idx,
>> + enum dpll_lock_status last_dpll_state, u8 *pin,
>> + u8 *ref_state, u8 *eec_mode, s64 *phase_offset,
>> + enum dpll_lock_status *dpll_state,
>> + enum dpll_mode *mode)
>> +{
>> + u8 hw_ref_state, hw_dpll_state, hw_eec_mode, hw_config;
>> + s64 hw_phase_offset;
>> + int status;
>> +
>> + status = ice_aq_get_cgu_dpll_status(hw, dpll_idx, &hw_ref_state,
>> + &hw_dpll_state, &hw_config,
>> + &hw_phase_offset, &hw_eec_mode);
>> + if (status) {
>> + *dpll_state = ICE_CGU_STATE_INVALID;
>
>dpll_state is of type enum dpll_lock_status.
>But the type of ICE_CGU_STATE_INVALID is enum ice_cgu_state.
>Is this intended?
>
>As flagged by gcc-12 W=1 and clang-16 W=1 builds.
>
No it's leftover, thanks for catching!
>> + return status;
>> + }
>> +
>> + if (pin)
>> + /* current ref pin in dpll_state_refsel_status_X register */
>> + *pin = hw_config & ICE_AQC_GET_CGU_DPLL_CONFIG_CLK_REF_SEL;
>> + if (phase_offset)
>> + *phase_offset = hw_phase_offset;
>> + if (ref_state)
>> + *ref_state = hw_ref_state;
>> + if (eec_mode)
>> + *eec_mode = hw_eec_mode;
>> + if (!dpll_state)
>> + return status;
>
>Here dpll_state is checked for NULL.
>But, above, it is dereferenced in the case where ice_aq_get_cgu_dpll_status
>fails. Is that safe?
>
Yes, will fix.
>Also, perhaps it makes things a bit clearer to return 0 here.
True, will fix.
>
>...
>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
>b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
>
>...
>
>> +static const struct ice_cgu_pin_desc ice_e810t_sfp_cgu_inputs[] = {
>> + { "CVL-SDP22", ZL_REF0P, DPLL_PIN_TYPE_INT_OSCILLATOR,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "CVL-SDP20", ZL_REF0N, DPLL_PIN_TYPE_INT_OSCILLATOR,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "C827_0-RCLKA", ZL_REF1P, DPLL_PIN_TYPE_MUX, 0, },
>> + { "C827_0-RCLKB", ZL_REF1N, DPLL_PIN_TYPE_MUX, 0, },
>> + { "SMA1", ZL_REF3P, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "SMA2/U.FL2", ZL_REF3N, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "GNSS-1PPS", ZL_REF4P, DPLL_PIN_TYPE_GNSS,
>> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
>> + { "OCXO", ZL_REF4N, DPLL_PIN_TYPE_INT_OSCILLATOR, 0, },
>> +};
>
>A gcc-12 W=1 build warns that ice_e810t_sfp_cgu_inputs, and
>the similar static variables below, are unused when ice_ptp_hw.h
>is included in ice_main.c via ice.h.
>
>Looking at ice_e823_zl_cgu_outputs[], it seems to only be used
>in ice_ptp_hw.c, so perhaps it could be defined there.
>
>Perhaps that is also true of the other static variables below,
>but I didn't check that.
Yes, great catches, will fix.
Thank you for all of them!
Arkadiusz
>
>> +
>> +static const struct ice_cgu_pin_desc ice_e810t_qsfp_cgu_inputs[] = {
>> + { "CVL-SDP22", ZL_REF0P, DPLL_PIN_TYPE_INT_OSCILLATOR,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "CVL-SDP20", ZL_REF0N, DPLL_PIN_TYPE_INT_OSCILLATOR,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "C827_0-RCLKA", ZL_REF1P, DPLL_PIN_TYPE_MUX, },
>> + { "C827_0-RCLKB", ZL_REF1N, DPLL_PIN_TYPE_MUX, },
>> + { "C827_1-RCLKA", ZL_REF2P, DPLL_PIN_TYPE_MUX, },
>> + { "C827_1-RCLKB", ZL_REF2N, DPLL_PIN_TYPE_MUX, },
>> + { "SMA1", ZL_REF3P, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "SMA2/U.FL2", ZL_REF3N, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "GNSS-1PPS", ZL_REF4P, DPLL_PIN_TYPE_GNSS,
>> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
>> + { "OCXO", ZL_REF4N, DPLL_PIN_TYPE_INT_OSCILLATOR, },
>> +};
>> +
>> +static const struct ice_cgu_pin_desc ice_e810t_sfp_cgu_outputs[] = {
>> + { "REF-SMA1", ZL_OUT0, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "REF-SMA2/U.FL2", ZL_OUT1, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "PHY-CLK", ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT, },
>> + { "MAC-CLK", ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, },
>> + { "CVL-SDP21", ZL_OUT4, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
>> + { "CVL-SDP23", ZL_OUT5, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
>> +};
>> +
>> +static const struct ice_cgu_pin_desc ice_e810t_qsfp_cgu_outputs[] = {
>> + { "REF-SMA1", ZL_OUT0, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "REF-SMA2/U.FL2", ZL_OUT1, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "PHY-CLK", ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
>> + { "PHY2-CLK", ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
>> + { "MAC-CLK", ZL_OUT4, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
>> + { "CVL-SDP21", ZL_OUT5, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
>> + { "CVL-SDP23", ZL_OUT6, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
>> +};
>> +
>> +static const struct ice_cgu_pin_desc ice_e823_si_cgu_inputs[] = {
>> + { "NONE", SI_REF0P, 0, 0 },
>> + { "NONE", SI_REF0N, 0, 0 },
>> + { "SYNCE0_DP", SI_REF1P, DPLL_PIN_TYPE_MUX, 0 },
>> + { "SYNCE0_DN", SI_REF1N, DPLL_PIN_TYPE_MUX, 0 },
>> + { "EXT_CLK_SYNC", SI_REF2P, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "NONE", SI_REF2N, 0, 0 },
>> + { "EXT_PPS_OUT", SI_REF3, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "INT_PPS_OUT", SI_REF4, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> +};
>> +
>> +static const struct ice_cgu_pin_desc ice_e823_si_cgu_outputs[] = {
>> + { "1588-TIME_SYNC", SI_OUT0, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "PHY-CLK", SI_OUT1, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
>> + { "10MHZ-SMA2", SI_OUT2, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_10_mhz), ice_cgu_pin_freq_10_mhz },
>> + { "PPS-SMA1", SI_OUT3, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> +};
>> +
>> +static const struct ice_cgu_pin_desc ice_e823_zl_cgu_inputs[] = {
>> + { "NONE", ZL_REF0P, 0, 0 },
>> + { "INT_PPS_OUT", ZL_REF0N, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
>> + { "SYNCE0_DP", ZL_REF1P, DPLL_PIN_TYPE_MUX, 0 },
>> + { "SYNCE0_DN", ZL_REF1N, DPLL_PIN_TYPE_MUX, 0 },
>> + { "NONE", ZL_REF2P, 0, 0 },
>> + { "NONE", ZL_REF2N, 0, 0 },
>> + { "EXT_CLK_SYNC", ZL_REF3P, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "NONE", ZL_REF3N, 0, 0 },
>> + { "EXT_PPS_OUT", ZL_REF4P, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
>> + { "OCXO", ZL_REF4N, DPLL_PIN_TYPE_INT_OSCILLATOR, 0 },
>> +};
>> +
>> +static const struct ice_cgu_pin_desc ice_e823_zl_cgu_outputs[] = {
>> + { "PPS-SMA1", ZL_OUT0, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
>> + { "10MHZ-SMA2", ZL_OUT1, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_10_mhz), ice_cgu_pin_freq_10_mhz },
>> + { "PHY-CLK", ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
>> + { "1588-TIME_REF", ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
>> + { "CPK-TIME_SYNC", ZL_OUT4, DPLL_PIN_TYPE_EXT,
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> + { "NONE", ZL_OUT5, 0, 0 },
>> +};
>> +
>> extern const struct
>> ice_cgu_pll_params_e822 e822_cgu_params[NUM_ICE_TIME_REF_FREQ];
>>
>
>...
Powered by blists - more mailing lists