| 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: <DM6PR11MB46576BB60940799A98DAC7949B06A@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Fri, 28 Jul 2023 23:10:36 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>, Simon Horman
<simon.horman@...igine.com>
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 09/11] ice: implement dpll interface to control cgu
>From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
>Sent: Monday, July 24, 2023 7:59 PM
>
>On 24.07.2023 18:41, Simon Horman wrote:
>> On Thu, Jul 20, 2023 at 10:19:01AM +0100, Vadim Fedorenko wrote:
>>
>> ...
>>
>> Hi Vadim,
>>
>
>Hi Simon!
>Thanks for the review. I believe Arkadiusz as the author of the patch will
>adjust the code accordingly
>
Yes, will fix all the findings, thank you Simon for pointing them out!
Arkadiusz
>>> +/**
>>> + * ice_dpll_cb_unlock - unlock dplls mutex in callback context
>>> + * @pf: private board structure
>>> + *
>>> + * Unlock the mutex from the callback operations invoked by dpll
>>>subsystem.
>>> + */
>>> +static void ice_dpll_cb_unlock(struct ice_pf *pf)
>>> +{
>>> + mutex_unlock(&pf->dplls.lock);
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_pin_freq_set - set pin's frequency
>>> + * @pf: private board structure
>>> + * @pin: pointer to a pin
>>> + * @pin_type: type of pin being configured
>>> + * @freq: frequency to be set
>>> + * @extack: error reporting
>>> + *
>>> + * Set requested frequency on a pin.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - error on AQ or wrong pin type given
>>> + */
>>> +static int
>>> +ice_dpll_pin_freq_set(struct ice_pf *pf, struct ice_dpll_pin *pin,
>>> + enum ice_dpll_pin_type pin_type, const u32 freq,
>>> + struct netlink_ext_ack *extack)
>>> +{
>>> + int ret;
>>> + u8 flags;
>>
>> Please arrange local variable declarations for new Networking
>> code in reverse xmas tree order - longest line to shortest.
>>
>>> +
>>> + switch (pin_type) {
>>> + case ICE_DPLL_PIN_TYPE_INPUT:
>>> + flags = ICE_AQC_SET_CGU_IN_CFG_FLG1_UPDATE_FREQ;
>>> + ret = ice_aq_set_input_pin_cfg(&pf->hw, pin->idx, flags,
>>> + pin->flags[0], freq, 0);
>>> + break;
>>> + case ICE_DPLL_PIN_TYPE_OUTPUT:
>>> + flags = ICE_AQC_SET_CGU_OUT_CFG_UPDATE_FREQ;
>>> + ret = ice_aq_set_output_pin_cfg(&pf->hw, pin->idx, flags,
>>> + 0, freq, 0);
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> + if (ret) {
>>> + NL_SET_ERR_MSG_FMT(extack,
>>> + "err:%d %s failed to set pin freq:%u on
>>>pin:%u\n",
>>> + ret,
>>> + ice_aq_str(pf->hw.adminq.sq_last_status),
>>> + freq, pin->idx);
>>> + return ret;
>>> + }
>>> + pin->freq = freq;
>>> +
>>> + return 0;
>>> +}
>>
>> ...
>>
>>> +/**
>>> + * ice_dpll_pin_state_update - update pin's state
>>> + * @pf: private board struct
>>> + * @pin: structure with pin attributes to be updated
>>> + * @pin_type: type of pin being updated
>>> + * @extack: error reporting
>>> + *
>>> + * Determine pin current state and frequency, then update struct
>>> + * holding the pin info. For input pin states are separated for each
>>> + * dpll, for rclk pins states are separated for each parent.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - OK
>>> + * * negative - error
>>> + */
>>> +int
>>> +ice_dpll_pin_state_update(struct ice_pf *pf, struct ice_dpll_pin *pin,
>>> + enum ice_dpll_pin_type pin_type,
>>> + struct netlink_ext_ack *extack)
>>
>>> +/**
>>> + * ice_dpll_frequency_set - wrapper for pin callback for set frequency
>>> + * @pin: pointer to a pin
>>> + * @pin_priv: private data pointer passed on pin registration
>>> + * @dpll: pointer to dpll
>>> + * @dpll_priv: private data pointer passed on dpll registration
>>> + * @frequency: frequency to be set
>>> + * @extack: error reporting
>>> + * @pin_type: type of pin being configured
>>> + *
>>> + * Wraps internal set frequency command on a pin.
>>> + *
>>> + * Context: Acquires pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - error pin not found or couldn't set in hw
>>> + */
>>> +static int
>>> +ice_dpll_frequency_set(const struct dpll_pin *pin, void *pin_priv,
>>> + const struct dpll_device *dpll, void *dpll_priv,
>>> + const u32 frequency,
>>> + struct netlink_ext_ack *extack,
>>> + enum ice_dpll_pin_type pin_type)
>>> +{
>>> + struct ice_dpll_pin *p = pin_priv;
>>> + struct ice_dpll *d = dpll_priv;
>>> + struct ice_pf *pf = d->pf;
>>> + int ret;
>>> +
>>> + ret = ice_dpll_cb_lock(pf, extack);
>>> + if (ret)
>>> + return ret;
>>> + ret = ice_dpll_pin_freq_set(pf, p, pin_type, frequency, extack);
>>> + ice_dpll_cb_unlock(pf);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_input_frequency_set - input pin callback for set frequency
>>> + * @pin: pointer to a pin
>>> + * @pin_priv: private data pointer passed on pin registration
>>> + * @dpll: pointer to dpll
>>> + * @dpll_priv: private data pointer passed on dpll registration
>>> + * @frequency: frequency to be set
>>> + * @extack: error reporting
>>> + *
>>> + * Wraps internal set frequency command on a pin.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - error pin not found or couldn't set in hw
>>> + */
>>> +static int
>>> +ice_dpll_input_frequency_set(const struct dpll_pin *pin, void
>>>*pin_priv,
>>> + const struct dpll_device *dpll, void *dpll_priv,
>>> + u64 frequency, struct netlink_ext_ack *extack)
>>> +{
>>> + return ice_dpll_frequency_set(pin, pin_priv, dpll, dpll_priv,
>>>frequency,
>>> + extack, ICE_DPLL_PIN_TYPE_INPUT);
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_output_frequency_set - output pin callback for set
>>>frequency
>>> + * @pin: pointer to a pin
>>> + * @pin_priv: private data pointer passed on pin registration
>>> + * @dpll: pointer to dpll
>>> + * @dpll_priv: private data pointer passed on dpll registration
>>> + * @frequency: frequency to be set
>>> + * @extack: error reporting
>>> + *
>>> + * Wraps internal set frequency command on a pin.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - error pin not found or couldn't set in hw
>>> + */
>>> +static int
>>> +ice_dpll_output_frequency_set(const struct dpll_pin *pin, void
>>>*pin_priv,
>>> + const struct dpll_device *dpll, void *dpll_priv,
>>> + u64 frequency, struct netlink_ext_ack *extack)
>>> +{
>>> + return ice_dpll_frequency_set(pin, pin_priv, dpll, dpll_priv,
>>>frequency,
>>> + extack, ICE_DPLL_PIN_TYPE_OUTPUT);
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_frequency_get - wrapper for pin callback for get frequency
>>> + * @pin: pointer to a pin
>>> + * @pin_priv: private data pointer passed on pin registration
>>> + * @dpll: pointer to dpll
>>> + * @dpll_priv: private data pointer passed on dpll registration
>>> + * @frequency: on success holds pin's frequency
>>> + * @extack: error reporting
>>> + * @pin_type: type of pin being configured
>>> + *
>>> + * Wraps internal get frequency command of a pin.
>>> + *
>>> + * Context: Acquires pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - error pin not found or couldn't get from hw
>>> + */
>>> +static int
>>> +ice_dpll_frequency_get(const struct dpll_pin *pin, void *pin_priv,
>>> + const struct dpll_device *dpll, void *dpll_priv,
>>> + u64 *frequency, struct netlink_ext_ack *extack,
>>> + enum ice_dpll_pin_type pin_type)
>>> +{
>>> + struct ice_dpll_pin *p = pin_priv;
>>> + struct ice_dpll *d = dpll_priv;
>>> + struct ice_pf *pf = d->pf;
>>> + int ret;
>>> +
>>> + ret = ice_dpll_cb_lock(pf, extack);
>>> + if (ret)
>>> + return ret;
>>> + *frequency = p->freq;
>>> + ice_dpll_cb_unlock(pf);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_input_frequency_get - input pin callback for get frequency
>>> + * @pin: pointer to a pin
>>> + * @pin_priv: private data pointer passed on pin registration
>>> + * @dpll: pointer to dpll
>>> + * @dpll_priv: private data pointer passed on dpll registration
>>> + * @frequency: on success holds pin's frequency
>>> + * @extack: error reporting
>>> + *
>>> + * Wraps internal get frequency command of a input pin.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - error pin not found or couldn't get from hw
>>> + */
>>> +static int
>>> +ice_dpll_input_frequency_get(const struct dpll_pin *pin, void
>>>*pin_priv,
>>> + const struct dpll_device *dpll, void *dpll_priv,
>>> + u64 *frequency, struct netlink_ext_ack *extack)
>>> +{
>>> + return ice_dpll_frequency_get(pin, pin_priv, dpll, dpll_priv,
>>>frequency,
>>> + extack, ICE_DPLL_PIN_TYPE_INPUT);
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_output_frequency_get - output pin callback for get
>>>frequency
>>> + * @pin: pointer to a pin
>>> + * @pin_priv: private data pointer passed on pin registration
>>> + * @dpll: pointer to dpll
>>> + * @dpll_priv: private data pointer passed on dpll registration
>>> + * @frequency: on success holds pin's frequency
>>> + * @extack: error reporting
>>> + *
>>> + * Wraps internal get frequency command of a pin.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - error pin not found or couldn't get from hw
>>> + */
>>> +static int
>>> +ice_dpll_output_frequency_get(const struct dpll_pin *pin, void
>>>*pin_priv,
>>> + const struct dpll_device *dpll, void *dpll_priv,
>>> + u64 *frequency, struct netlink_ext_ack *extack)
>>> +{
>>> + return ice_dpll_frequency_get(pin, pin_priv, dpll, dpll_priv,
>>>frequency,
>>> + extack, ICE_DPLL_PIN_TYPE_OUTPUT);
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_pin_enable - enable a pin on dplls
>>> + * @hw: board private hw structure
>>> + * @pin: pointer to a pin
>>> + * @pin_type: type of pin being enabled
>>> + * @extack: error reporting
>>> + *
>>> + * Enable a pin on both dplls. Store current state in pin->flags.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - OK
>>> + * * negative - error
>>> + */
>>> +static int
>>> +ice_dpll_pin_enable(struct ice_hw *hw, struct ice_dpll_pin *pin,
>>> + enum ice_dpll_pin_type pin_type,
>>> + struct netlink_ext_ack *extack)
>>> +{
>>> + u8 flags = 0;
>>> + int ret;
>>> +
>>> + switch (pin_type) {
>>> + case ICE_DPLL_PIN_TYPE_INPUT:
>>> + if (pin->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN)
>>> + flags |= ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN;
>>> + flags |= ICE_AQC_SET_CGU_IN_CFG_FLG2_INPUT_EN;
>>> + ret = ice_aq_set_input_pin_cfg(hw, pin->idx, 0, flags, 0, 0);
>>> + break;
>>> + case ICE_DPLL_PIN_TYPE_OUTPUT:
>>> + if (pin->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN)
>>> + flags |= ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN;
>>> + flags |= ICE_AQC_SET_CGU_OUT_CFG_OUT_EN;
>>> + ret = ice_aq_set_output_pin_cfg(hw, pin->idx, flags, 0, 0, 0);
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> + if (ret)
>>> + NL_SET_ERR_MSG_FMT(extack,
>>> + "err:%d %s failed to enable %s pin:%u\n",
>>> + ret, ice_aq_str(hw->adminq.sq_last_status),
>>> + pin_type_name[pin_type], pin->idx);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_pin_disable - disable a pin on dplls
>>> + * @hw: board private hw structure
>>> + * @pin: pointer to a pin
>>> + * @pin_type: type of pin being disabled
>>> + * @extack: error reporting
>>> + *
>>> + * Disable a pin on both dplls. Store current state in pin->flags.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - OK
>>> + * * negative - error
>>> + */
>>> +static int
>>> +ice_dpll_pin_disable(struct ice_hw *hw, struct ice_dpll_pin *pin,
>>> + enum ice_dpll_pin_type pin_type,
>>> + struct netlink_ext_ack *extack)
>>> +{
>>> + u8 flags = 0;
>>> + int ret;
>>> +
>>> + switch (pin_type) {
>>> + case ICE_DPLL_PIN_TYPE_INPUT:
>>> + if (pin->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN)
>>> + flags |= ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN;
>>> + ret = ice_aq_set_input_pin_cfg(hw, pin->idx, 0, flags, 0, 0);
>>> + break;
>>> + case ICE_DPLL_PIN_TYPE_OUTPUT:
>>> + if (pin->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN)
>>> + flags |= ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN;
>>> + ret = ice_aq_set_output_pin_cfg(hw, pin->idx, flags, 0, 0, 0);
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> + if (ret)
>>> + NL_SET_ERR_MSG_FMT(extack,
>>> + "err:%d %s failed to disable %s pin:%u\n",
>>> + ret, ice_aq_str(hw->adminq.sq_last_status),
>>> + pin_type_name[pin_type], pin->idx);
>>> +
>>> + return ret;
>>> +}
>>
>>> +/**
>>> + * ice_dpll_frequency_set - wrapper for pin callback for set frequency
>>> + * @pin: pointer to a pin
>>> + * @pin_priv: private data pointer passed on pin registration
>>> + * @dpll: pointer to dpll
>>> + * @dpll_priv: private data pointer passed on dpll registration
>>> + * @frequency: frequency to be set
>>> + * @extack: error reporting
>>> + * @pin_type: type of pin being configured
>>> + *
>>> + * Wraps internal set frequency command on a pin.
>>> + *
>>> + * Context: Acquires pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - error pin not found or couldn't set in hw
>>> + */
>>> +static int
>>> +ice_dpll_frequency_set(const struct dpll_pin *pin, void *pin_priv,
>>> + const struct dpll_device *dpll, void *dpll_priv,
>>> + const u32 frequency,
>>> + struct netlink_ext_ack *extack,
>>> + enum ice_dpll_pin_type pin_type)
>>> +{
>>> + struct ice_dpll_pin *p = pin_priv;
>>> + struct ice_dpll *d = dpll_priv;
>>> + struct ice_pf *pf = d->pf;
>>> + int ret;
>>> +
>>> + ret = ice_dpll_cb_lock(pf, extack);
>>> + if (ret)
>>> + return ret;
>>> + ret = ice_dpll_pin_freq_set(pf, p, pin_type, frequency, extack);
>>> + ice_dpll_cb_unlock(pf);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_input_frequency_set - input pin callback for set frequency
>>> + * @pin: pointer to a pin
>>> + * @pin_priv: private data pointer passed on pin registration
>>> + * @dpll: pointer to dpll
>>> + * @dpll_priv: private data pointer passed on dpll registration
>>> + * @frequency: frequency to be set
>>> + * @extack: error reporting
>>> + *
>>> + * Wraps internal set frequency command on a pin.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - error pin not found or couldn't set in hw
>>> + */
>>> +static int
>>> +ice_dpll_input_frequency_set(const struct dpll_pin *pin, void
>>>*pin_priv,
>>> + const struct dpll_device *dpll, void *dpll_priv,
>>> + u64 frequency, struct netlink_ext_ack *extack)
>>> +{
>>> + return ice_dpll_frequency_set(pin, pin_priv, dpll, dpll_priv,
>>>frequency,
>>> + extack, ICE_DPLL_PIN_TYPE_INPUT);
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_output_frequency_set - output pin callback for set
>>>frequency
>>> + * @pin: pointer to a pin
>>> + * @pin_priv: private data pointer passed on pin registration
>>> + * @dpll: pointer to dpll
>>> + * @dpll_priv: private data pointer passed on dpll registration
>>> + * @frequency: frequency to be set
>>> + * @extack: error reporting
>>> + *
>>> + * Wraps internal set frequency command on a pin.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - error pin not found or couldn't set in hw
>>> + */
>>> +static int
>>> +ice_dpll_output_frequency_set(const struct dpll_pin *pin, void
>>>*pin_priv,
>>> + const struct dpll_device *dpll, void *dpll_priv,
>>> + u64 frequency, struct netlink_ext_ack *extack)
>>> +{
>>> + return ice_dpll_frequency_set(pin, pin_priv, dpll, dpll_priv,
>>>frequency,
>>> + extack, ICE_DPLL_PIN_TYPE_OUTPUT);
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_frequency_get - wrapper for pin callback for get frequency
>>> + * @pin: pointer to a pin
>>> + * @pin_priv: private data pointer passed on pin registration
>>> + * @dpll: pointer to dpll
>>> + * @dpll_priv: private data pointer passed on dpll registration
>>> + * @frequency: on success holds pin's frequency
>>> + * @extack: error reporting
>>> + * @pin_type: type of pin being configured
>>> + *
>>> + * Wraps internal get frequency command of a pin.
>>> + *
>>> + * Context: Acquires pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - error pin not found or couldn't get from hw
>>> + */
>>> +static int
>>> +ice_dpll_frequency_get(const struct dpll_pin *pin, void *pin_priv,
>>> + const struct dpll_device *dpll, void *dpll_priv,
>>> + u64 *frequency, struct netlink_ext_ack *extack,
>>> + enum ice_dpll_pin_type pin_type)
>>> +{
>>> + struct ice_dpll_pin *p = pin_priv;
>>> + struct ice_dpll *d = dpll_priv;
>>> + struct ice_pf *pf = d->pf;
>>> + int ret;
>>> +
>>> + ret = ice_dpll_cb_lock(pf, extack);
>>> + if (ret)
>>> + return ret;
>>> + *frequency = p->freq;
>>> + ice_dpll_cb_unlock(pf);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_input_frequency_get - input pin callback for get frequency
>>> + * @pin: pointer to a pin
>>> + * @pin_priv: private data pointer passed on pin registration
>>> + * @dpll: pointer to dpll
>>> + * @dpll_priv: private data pointer passed on dpll registration
>>> + * @frequency: on success holds pin's frequency
>>> + * @extack: error reporting
>>> + *
>>> + * Wraps internal get frequency command of a input pin.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - error pin not found or couldn't get from hw
>>> + */
>>> +static int
>>> +ice_dpll_input_frequency_get(const struct dpll_pin *pin, void
>>>*pin_priv,
>>> + const struct dpll_device *dpll, void *dpll_priv,
>>> + u64 *frequency, struct netlink_ext_ack *extack)
>>> +{
>>> + return ice_dpll_frequency_get(pin, pin_priv, dpll, dpll_priv,
>>>frequency,
>>> + extack, ICE_DPLL_PIN_TYPE_INPUT);
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_output_frequency_get - output pin callback for get
>>>frequency
>>> + * @pin: pointer to a pin
>>> + * @pin_priv: private data pointer passed on pin registration
>>> + * @dpll: pointer to dpll
>>> + * @dpll_priv: private data pointer passed on dpll registration
>>> + * @frequency: on success holds pin's frequency
>>> + * @extack: error reporting
>>> + *
>>> + * Wraps internal get frequency command of a pin.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - error pin not found or couldn't get from hw
>>> + */
>>> +static int
>>> +ice_dpll_output_frequency_get(const struct dpll_pin *pin, void
>>>*pin_priv,
>>> + const struct dpll_device *dpll, void *dpll_priv,
>>> + u64 *frequency, struct netlink_ext_ack *extack)
>>> +{
>>> + return ice_dpll_frequency_get(pin, pin_priv, dpll, dpll_priv,
>>>frequency,
>>> + extack, ICE_DPLL_PIN_TYPE_OUTPUT);
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_pin_enable - enable a pin on dplls
>>> + * @hw: board private hw structure
>>> + * @pin: pointer to a pin
>>> + * @pin_type: type of pin being enabled
>>> + * @extack: error reporting
>>> + *
>>> + * Enable a pin on both dplls. Store current state in pin->flags.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - OK
>>> + * * negative - error
>>> + */
>>> +static int
>>> +ice_dpll_pin_enable(struct ice_hw *hw, struct ice_dpll_pin *pin,
>>> + enum ice_dpll_pin_type pin_type,
>>> + struct netlink_ext_ack *extack)
>>> +{
>>> + u8 flags = 0;
>>> + int ret;
>>> +
>>> + switch (pin_type) {
>>> + case ICE_DPLL_PIN_TYPE_INPUT:
>>> + if (pin->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN)
>>> + flags |= ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN;
>>> + flags |= ICE_AQC_SET_CGU_IN_CFG_FLG2_INPUT_EN;
>>> + ret = ice_aq_set_input_pin_cfg(hw, pin->idx, 0, flags, 0, 0);
>>> + break;
>>> + case ICE_DPLL_PIN_TYPE_OUTPUT:
>>> + if (pin->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN)
>>> + flags |= ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN;
>>> + flags |= ICE_AQC_SET_CGU_OUT_CFG_OUT_EN;
>>> + ret = ice_aq_set_output_pin_cfg(hw, pin->idx, flags, 0, 0, 0);
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> + if (ret)
>>> + NL_SET_ERR_MSG_FMT(extack,
>>> + "err:%d %s failed to enable %s pin:%u\n",
>>> + ret, ice_aq_str(hw->adminq.sq_last_status),
>>> + pin_type_name[pin_type], pin->idx);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +/**
>>> + * ice_dpll_pin_disable - disable a pin on dplls
>>> + * @hw: board private hw structure
>>> + * @pin: pointer to a pin
>>> + * @pin_type: type of pin being disabled
>>> + * @extack: error reporting
>>> + *
>>> + * Disable a pin on both dplls. Store current state in pin->flags.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - OK
>>> + * * negative - error
>>> + */
>>> +static int
>>> +ice_dpll_pin_disable(struct ice_hw *hw, struct ice_dpll_pin *pin,
>>> + enum ice_dpll_pin_type pin_type,
>>> + struct netlink_ext_ack *extack)
>>> +{
>>> + u8 flags = 0;
>>> + int ret;
>>> +
>>> + switch (pin_type) {
>>> + case ICE_DPLL_PIN_TYPE_INPUT:
>>> + if (pin->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN)
>>> + flags |= ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN;
>>> + ret = ice_aq_set_input_pin_cfg(hw, pin->idx, 0, flags, 0, 0);
>>> + break;
>>> + case ICE_DPLL_PIN_TYPE_OUTPUT:
>>> + if (pin->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN)
>>> + flags |= ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN;
>>> + ret = ice_aq_set_output_pin_cfg(hw, pin->idx, flags, 0, 0, 0);
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> + if (ret)
>>> + NL_SET_ERR_MSG_FMT(extack,
>>> + "err:%d %s failed to disable %s pin:%u\n",
>>> + ret, ice_aq_str(hw->adminq.sq_last_status),
>>> + pin_type_name[pin_type], pin->idx);
>>> +
>>> + return ret;
>>> +}
>>
>> Should this function be static?
>>
>>> +{
>>> + int ret;
>>> +
>>> + switch (pin_type) {
>>> + case ICE_DPLL_PIN_TYPE_INPUT:
>>> + ret = ice_aq_get_input_pin_cfg(&pf->hw, pin->idx, NULL, NULL,
>>> + NULL, &pin->flags[0],
>>> + &pin->freq, NULL);
>>> + if (ret)
>>> + goto err;
>>> + if (ICE_AQC_GET_CGU_IN_CFG_FLG2_INPUT_EN & pin->flags[0]) {
>>> + if (pin->pin) {
>>> + pin->state[pf->dplls.eec.dpll_idx] =
>>> + pin->pin == pf->dplls.eec.active_input ?
>>> + DPLL_PIN_STATE_CONNECTED :
>>> + DPLL_PIN_STATE_SELECTABLE;
>>> + pin->state[pf->dplls.pps.dpll_idx] =
>>> + pin->pin == pf->dplls.pps.active_input ?
>>> + DPLL_PIN_STATE_CONNECTED :
>>> + DPLL_PIN_STATE_SELECTABLE;
>>> + } else {
>>> + pin->state[pf->dplls.eec.dpll_idx] =
>>> + DPLL_PIN_STATE_SELECTABLE;
>>> + pin->state[pf->dplls.pps.dpll_idx] =
>>> + DPLL_PIN_STATE_SELECTABLE;
>>> + }
>>> + } else {
>>> + pin->state[pf->dplls.eec.dpll_idx] =
>>> + DPLL_PIN_STATE_DISCONNECTED;
>>> + pin->state[pf->dplls.pps.dpll_idx] =
>>> + DPLL_PIN_STATE_DISCONNECTED;
>>> + }
>>> + break;
>>> + case ICE_DPLL_PIN_TYPE_OUTPUT:
>>> + ret = ice_aq_get_output_pin_cfg(&pf->hw, pin->idx,
>>> + &pin->flags[0], NULL,
>>> + &pin->freq, NULL);
>>> + if (ret)
>>> + goto err;
>>> + if (ICE_AQC_SET_CGU_OUT_CFG_OUT_EN & pin->flags[0])
>>> + pin->state[0] = DPLL_PIN_STATE_CONNECTED;
>>> + else
>>> + pin->state[0] = DPLL_PIN_STATE_DISCONNECTED;
>>> + break;
>>> + case ICE_DPLL_PIN_TYPE_RCLK_INPUT:
>>
>> clang-16 complains that:
>>
>> drivers/net/ethernet/intel/ice/ice_dpll.c:461:3: error: expected
>>expression
>> u8 parent, port_num =
>>ICE_AQC_SET_PHY_REC_CLK_OUT_CURR_PORT;
>>
>> Which, I think means, it wants this case to be enclosed in { }
>>
>>> + u8 parent, port_num = ICE_AQC_SET_PHY_REC_CLK_OUT_CURR_PORT;
>>> +
>>> + for (parent = 0; parent < pf->dplls.rclk.num_parents;
>>> + parent++) {
>>> + u8 p = parent;
>>> +
>>> + ret = ice_aq_get_phy_rec_clk_out(&pf->hw, &p,
>>> + &port_num,
>>> + &pin->flags[parent],
>>> + NULL);
>>> + if (ret)
>>> + goto err;
>>> + if (ICE_AQC_GET_PHY_REC_CLK_OUT_OUT_EN &
>>> + pin->flags[parent])
>>> + pin->state[parent] = DPLL_PIN_STATE_CONNECTED;
>>> + else
>>> + pin->state[parent] =
>>> + DPLL_PIN_STATE_DISCONNECTED;
>>> + }
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return 0;
>>> +err:
>>> + if (extack)
>>> + NL_SET_ERR_MSG_FMT(extack,
>>> + "err:%d %s failed to update %s pin:%u\n",
>>> + ret,
>>> + ice_aq_str(pf->hw.adminq.sq_last_status),
>>> + pin_type_name[pin_type], pin->idx);
>>> + else
>>> + dev_err_ratelimited(ice_pf_to_dev(pf),
>>> + "err:%d %s failed to update %s pin:%u\n",
>>> + ret,
>>> + ice_aq_str(pf->hw.adminq.sq_last_status),
>>> + pin_type_name[pin_type], pin->idx);
>>> + return ret;
>>> +}
>>
>> ...
>>
>>> +/**
>>> + * ice_dpll_update_state - update dpll state
>>> + * @pf: pf private structure
>>> + * @d: pointer to queried dpll device
>>> + * @init: if function called on initialization of ice dpll
>>> + *
>>> + * Poll current state of dpll from hw and update ice_dpll struct.
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - AQ failure
>>> + */
>>> +static int
>>> +ice_dpll_update_state(struct ice_pf *pf, struct ice_dpll *d, bool init)
>>> +{
>>> + struct ice_dpll_pin *p = NULL;
>>> + int ret;
>>> +
>>> + ret = ice_get_cgu_state(&pf->hw, d->dpll_idx, d->prev_dpll_state,
>>> + &d->input_idx, &d->ref_state, &d->eec_mode,
>>> + &d->phase_shift, &d->dpll_state, &d->mode);
>>> +
>>> + dev_dbg(ice_pf_to_dev(pf),
>>> + "update dpll=%d, prev_src_idx:%u, src_idx:%u, state:%d, prev:%d
>>>mode:%d\n",
>>> + d->dpll_idx, d->prev_input_idx, d->input_idx,
>>> + d->dpll_state, d->prev_dpll_state, d->mode);
>>> + if (ret) {
>>> + dev_err(ice_pf_to_dev(pf),
>>> + "update dpll=%d state failed, ret=%d %s\n",
>>> + d->dpll_idx, ret,
>>> + ice_aq_str(pf->hw.adminq.sq_last_status));
>>> + return ret;
>>> + }
>>> + if (init) {
>>> + if (d->dpll_state == DPLL_LOCK_STATUS_LOCKED &&
>>> + d->dpll_state == DPLL_LOCK_STATUS_LOCKED_HO_ACQ)
>>
>> Should this be '||' rather than '&&' ?
>>
>> Flagged by a clang-16 W=1 build, Sparse and Smatch.
>>
>>> + d->active_input = pf->dplls.inputs[d->input_idx].pin;
>>> + p = &pf->dplls.inputs[d->input_idx];
>>> + return ice_dpll_pin_state_update(pf, p,
>>> + ICE_DPLL_PIN_TYPE_INPUT, NULL);
>>> + }
>>
>> ...
>>
>>> +/**
>>> + * ice_dpll_init_info_direct_pins - initializes direct pins info
>>> + * @pf: board private structure
>>> + * @pin_type: type of pins being initialized
>>> + *
>>> + * Init information for directly connected pins, cache them in pf's
>>>pins
>>> + * structures.
>>> + *
>>> + * Context: Called under pf->dplls.lock.
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - init failure reason
>>> + */
>>> +static int
>>> +ice_dpll_init_info_direct_pins(struct ice_pf *pf,
>>> + enum ice_dpll_pin_type pin_type)
>>> +{
>>> + struct ice_dpll *de = &pf->dplls.eec, *dp = &pf->dplls.pps;
>>> + struct ice_hw *hw = &pf->hw;
>>> + struct ice_dpll_pin *pins;
>>> + int num_pins, i, ret;
>>> + u8 freq_supp_num;
>>> + bool input;
>>> +
>>> + switch (pin_type) {
>>> + case ICE_DPLL_PIN_TYPE_INPUT:
>>> + pins = pf->dplls.inputs;
>>> + num_pins = pf->dplls.num_inputs;
>>> + input = true;
>>> + break;
>>> + case ICE_DPLL_PIN_TYPE_OUTPUT:
>>> + pins = pf->dplls.outputs;
>>> + num_pins = pf->dplls.num_outputs;
>>> + input = false;
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + for (i = 0; i < num_pins; i++) {
>>> + pins[i].idx = i;
>>> + pins[i].prop.board_label = ice_cgu_get_pin_name(hw, i, input);
>>> + pins[i].prop.type = ice_cgu_get_pin_type(hw, i, input);
>>> + if (input) {
>>> + ret = ice_aq_get_cgu_ref_prio(hw, de->dpll_idx, i,
>>> + &de->input_prio[i]);
>>> + if (ret)
>>> + return ret;
>>> + ret = ice_aq_get_cgu_ref_prio(hw, dp->dpll_idx, i,
>>> + &dp->input_prio[i]);
>>> + if (ret)
>>> + return ret;
>>> + pins[i].prop.capabilities |=
>>> + DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE;
>>> + }
>>> + pins[i].prop.capabilities |= DPLL_PIN_CAPS_STATE_CAN_CHANGE;
>>> + ret = ice_dpll_pin_state_update(pf, &pins[i], pin_type, NULL);
>>> + if (ret)
>>> + return ret;
>>> + pins[i].prop.freq_supported =
>>> + ice_cgu_get_pin_freq_supp(hw, i, input, &freq_supp_num);
>>> + pins[i].prop.freq_supported_num = freq_supp_num;
>>> + pins[i].pf = pf;
>>> + }
>>> +
>>
>> I'm unsure if this can happen,
>> but if the for loop above iterates zero times
>> then ret will be null here.
>>
>> Use of uninitialised variable flagged by Smatch.
>>
>>> + return ret;
>>> +}
>>
>> ...
>>
>>> +/**
>>> + * ice_dpll_init_info - prepare pf's dpll information structure
>>> + * @pf: board private structure
>>> + * @cgu: if cgu is present and controlled by this NIC
>>> + *
>>> + * Acquire (from HW) and set basic dpll information (on pf->dplls
>>>struct).
>>> + *
>>> + * Context: Called under pf->dplls.lock
>>> + * Return:
>>> + * * 0 - success
>>> + * * negative - init failure reason
>>> + */
>>> +static int ice_dpll_init_info(struct ice_pf *pf, bool cgu)
>>> +{
>>> + struct ice_aqc_get_cgu_abilities abilities;
>>> + struct ice_dpll *de = &pf->dplls.eec;
>>> + struct ice_dpll *dp = &pf->dplls.pps;
>>> + struct ice_dplls *d = &pf->dplls;
>>> + struct ice_hw *hw = &pf->hw;
>>> + int ret, alloc_size, i;
>>> +
>>> + d->clock_id = ice_generate_clock_id(pf);
>>> + ret = ice_aq_get_cgu_abilities(hw, &abilities);
>>> + if (ret) {
>>> + dev_err(ice_pf_to_dev(pf),
>>> + "err:%d %s failed to read cgu abilities\n",
>>> + ret, ice_aq_str(hw->adminq.sq_last_status));
>>> + return ret;
>>> + }
>>> +
>>> + de->dpll_idx = abilities.eec_dpll_idx;
>>> + dp->dpll_idx = abilities.pps_dpll_idx;
>>> + d->num_inputs = abilities.num_inputs;
>>> + d->num_outputs = abilities.num_outputs;
>>> + d->input_phase_adj_max = le32_to_cpu(abilities.max_in_phase_adj);
>>> + d->output_phase_adj_max = le32_to_cpu(abilities.max_out_phase_adj);
>>> +
>>> + alloc_size = sizeof(*d->inputs) * d->num_inputs;
>>> + d->inputs = kzalloc(alloc_size, GFP_KERNEL);
>>> + if (!d->inputs)
>>> + return -ENOMEM;
>>> +
>>> + alloc_size = sizeof(*de->input_prio) * d->num_inputs;
>>> + de->input_prio = kzalloc(alloc_size, GFP_KERNEL);
>>> + if (!de->input_prio)
>>> + return -ENOMEM;
>>> +
>>> + dp->input_prio = kzalloc(alloc_size, GFP_KERNEL);
>>> + if (!dp->input_prio)
>>> + return -ENOMEM;
>>> +
>>> + ret = ice_dpll_init_pins_info(pf, ICE_DPLL_PIN_TYPE_INPUT);
>>> + if (ret)
>>> + goto deinit_info;
>>> +
>>> + if (cgu) {
>>> + alloc_size = sizeof(*d->outputs) * d->num_outputs;
>>> + d->outputs = kzalloc(alloc_size, GFP_KERNEL);
>>> + if (!d->outputs)
>>
>> Should ret be set to -ENOMEM here?
>>
>> Flagged by Smatch.
>>
>>> + goto deinit_info;
>>> +
>>> + ret = ice_dpll_init_pins_info(pf, ICE_DPLL_PIN_TYPE_OUTPUT);
>>> + if (ret)
>>> + goto deinit_info;
>>> + }
>>> +
>>> + ret = ice_get_cgu_rclk_pin_info(&pf->hw, &d->base_rclk_idx,
>>> + &pf->dplls.rclk.num_parents);
>>> + if (ret)
>>> + return ret;
>>> + for (i = 0; i < pf->dplls.rclk.num_parents; i++)
>>> + pf->dplls.rclk.parent_idx[i] = d->base_rclk_idx + i;
>>> + ret = ice_dpll_init_pins_info(pf, ICE_DPLL_PIN_TYPE_RCLK_INPUT);
>>> + if (ret)
>>> + return ret;
>>> + de->mode = DPLL_MODE_AUTOMATIC;
>>> + dp->mode = DPLL_MODE_AUTOMATIC;
>>> +
>>> + dev_dbg(ice_pf_to_dev(pf),
>>> + "%s - success, inputs:%u, outputs:%u rclk-parents:%u\n",
>>> + __func__, d->num_inputs, d->num_outputs, d->rclk.num_parents);
>>> +
>>> + return 0;
>>> +
>>> +deinit_info:
>>> + dev_err(ice_pf_to_dev(pf),
>>> + "%s - fail: d->inputs:%p, de->input_prio:%p, dp->input_prio:%p,
>>>d->outputs:%p\n",
>>> + __func__, d->inputs, de->input_prio,
>>> + dp->input_prio, d->outputs);
>>> + ice_dpll_deinit_info(pf);
>>> + return ret;
>>> +}
>>
>> ...
>>
>>> +/**
>>> + * ice_dpll_init - initialize support for dpll subsystem
>>> + * @pf: board private structure
>>> + *
>>> + * Set up the device dplls, register them and pins connected within
>>>Linux dpll
>>> + * subsystem. Allow userpsace to obtain state of DPLL and handling of
>>>DPLL
>>
>> nit: userpsace -> userspace
>>
>>> + * configuration requests.
>>> + *
>>> + * Context: Function initializes and holds pf->dplls.lock mutex.
>>> + */
>>
>> ...
>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h
>b/drivers/net/ethernet/intel/ice/ice_dpll.h
>>> new file mode 100644
>>> index 000000000000..975066b71c5e
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
>>> @@ -0,0 +1,104 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/* Copyright (C) 2022, Intel Corporation. */
>>> +
>>> +#ifndef _ICE_DPLL_H_
>>> +#define _ICE_DPLL_H_
>>> +
>>> +#include "ice.h"
>>> +
>>> +#define ICE_DPLL_PRIO_MAX 0xF
>>> +#define ICE_DPLL_RCLK_NUM_MAX 4
>>> +
>>> +/** ice_dpll_pin - store info about pins
>>> + * @pin: dpll pin structure
>>> + * @pf: pointer to pf, which has registered the dpll_pin
>>> + * @idx: ice pin private idx
>>> + * @num_parents: hols number of parent pins
>>> + * @parent_idx: hold indexes of parent pins
>>> + * @flags: pin flags returned from HW
>>> + * @state: state of a pin
>>> + * @prop: pin properities
>>
>> nit: properities -> properties
>>
>>> + * @freq: current frequency of a pin
>>> + */
>>> +struct ice_dpll_pin {
>>> + struct dpll_pin *pin;
>>> + struct ice_pf *pf;
>>> + u8 idx;
>>> + u8 num_parents;
>>> + u8 parent_idx[ICE_DPLL_RCLK_NUM_MAX];
>>> + u8 flags[ICE_DPLL_RCLK_NUM_MAX];
>>> + u8 state[ICE_DPLL_RCLK_NUM_MAX];
>>> + struct dpll_pin_properties prop;
>>> + u32 freq;
>>> +};
>>
>> ...
Powered by blists - more mailing lists