| 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: <ZL6zMmyIUObOY+6i@corigine.com>
Date: Mon, 24 Jul 2023 19:21:54 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: Jakub Kicinski <kuba@...nel.org>, Jiri Pirko <jiri@...nulli.us>,
Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
Jonathan Lemon <jonathan.lemon@...il.com>,
Paolo Abeni <pabeni@...hat.com>,
Milena Olech <milena.olech@...el.com>,
Michal Michalik <michal.michalik@...el.com>,
linux-arm-kernel@...ts.infradead.org, poros@...hat.com,
mschmidt@...hat.com, netdev@...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
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.
> + * @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.
> + * @phase_offset: pointer to a buffer for returning phase offset
> + * @dpll_state: state of the DPLL (output)
And @mode is missing here.
> + *
> + * 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.
> + 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?
Also, perhaps it makes things a bit clearer to return 0 here.
...
> 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.
> +
> +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