| 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: <DM6PR11MB46571657F0DF87765DAB32FE9B06A@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Fri, 28 Jul 2023 23:03:59 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>, Vadim Fedorenko <vadim.fedorenko@...ux.dev>
CC: Jakub Kicinski <kuba@...nel.org>, 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: Jiri Pirko <jiri@...nulli.us>
>Sent: Friday, July 21, 2023 1:39 PM
>
>Thu, Jul 20, 2023 at 11:19:01AM CEST, vadim.fedorenko@...ux.dev wrote:
>>From: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
[...]
>>+
>>+/**
>>+ * ice_dpll_cb_lock - lock dplls mutex in callback context
>>+ * @pf: private board structure
>>+ * @extack: error reporting
>>+ *
>>+ * Lock the mutex from the callback operations invoked by dpll subsystem.
>>+ * Prevent dead lock caused by `rmmod ice` when dpll callbacks are under
>stress
>>+ * tests.
>
>I don't know, I will probably need to beg you here. Why exactly are you
>ignoring my comments? It's not nice, I thought we are way past it...
>
>There is no "dead lock". Could you please describe how exactly
>the case you mention can happen? It can't.
>Could you please remove the trylock iteration below?
>It's completely pointless.
>
Yep, dead lock cannot happen now, will fix docs.
Will remove trylock.
>
>
>>+ *
>>+ * Return:
>>+ * 0 - if lock acquired
>>+ * negative - lock not acquired or dpll is not initialized
>>+ */
>>+static int ice_dpll_cb_lock(struct ice_pf *pf, struct netlink_ext_ack
>>*extack)
>>+{
>>+ int i;
>>+
>>+ for (i = 0; i < ICE_DPLL_LOCK_TRIES; i++) {
>>+ if (!test_bit(ICE_FLAG_DPLL, pf->flags)) {
>
>And again, as I already told you, this flag checking is totally
>pointless. See below my comment to ice_dpll_init()/ice_dpll_deinit().
>
This is not pointless, will explain below.
>
>
[...]
>>+/**
>>+ * ice_dpll_release_pins - release pins resources from dpll subsystem
>>+ * @pins: pointer to pins array
>>+ * @count: number of pins
>>+ *
>>+ * Release resources of given pins array in the dpll subsystem.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ */
>>+static void ice_dpll_release_pins(struct ice_dpll_pin *pins, int count)
>>+{
>>+ int i;
>>+
>>+ for (i = 0; i < count; i++)
>>+ dpll_pin_put(pins[i].pin);
>>+}
>>+
>>+/**
>>+ * ice_dpll_get_pins - get pins from dpll subsystem
>>+ * @pf: board private structure
>>+ * @pins: pointer to pins array
>>+ * @start_idx: get starts from this pin idx value
>>+ * @count: number of pins
>>+ * @clock_id: clock_id of dpll device
>>+ *
>>+ * Get pins - allocate - in dpll subsystem, store them in pin field of
>>given
>>+ * pins array.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - allocation failure reason
>>+ */
>>+static int
>>+ice_dpll_get_pins(struct ice_pf *pf, struct ice_dpll_pin *pins,
>>+ int start_idx, int count, u64 clock_id)
>>+{
>>+ int i, ret;
>>+
>>+ for (i = 0; i < count; i++) {
>>+ pins[i].pin = dpll_pin_get(clock_id, i + start_idx,
>>THIS_MODULE,
>>+ &pins[i].prop);
>>+ if (IS_ERR(pins[i].pin)) {
>>+ ret = PTR_ERR(pins[i].pin);
>>+ goto release_pins;
>>+ }
>>+ }
>>+
>>+ return 0;
>>+
>>+release_pins:
>>+ while (--i >= 0)
>>+ dpll_pin_put(pins[i].pin);
>>+ return ret;
>>+}
>>+
>>+/**
>>+ * ice_dpll_unregister_pins - unregister pins from a dpll
>>+ * @dpll: dpll device pointer
>>+ * @pins: pointer to pins array
>>+ * @ops: callback ops registered with the pins
>>+ * @count: number of pins
>>+ *
>>+ * Unregister pins of a given array of pins from given dpll device
>>registered in
>>+ * dpll subsystem.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ */
>>+static void
>>+ice_dpll_unregister_pins(struct dpll_device *dpll, struct ice_dpll_pin
>>*pins,
>>+ const struct dpll_pin_ops *ops, int count)
>>+{
>>+ int i;
>>+
>>+ for (i = 0; i < count; i++)
>>+ dpll_pin_unregister(dpll, pins[i].pin, ops, &pins[i]);
>>+}
>>+
>>+/**
>>+ * ice_dpll_register_pins - register pins with a dpll
>>+ * @dpll: dpll pointer to register pins with
>>+ * @pins: pointer to pins array
>>+ * @ops: callback ops registered with the pins
>>+ * @count: number of pins
>>+ *
>>+ * Register pins of a given array with given dpll in dpll subsystem.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - registration failure reason
>>+ */
>>+static int
>>+ice_dpll_register_pins(struct dpll_device *dpll, struct ice_dpll_pin
>>*pins,
>>+ const struct dpll_pin_ops *ops, int count)
>>+{
>>+ int ret, i;
>>+
>>+ for (i = 0; i < count; i++) {
>>+ ret = dpll_pin_register(dpll, pins[i].pin, ops, &pins[i]);
>>+ if (ret)
>>+ goto unregister_pins;
>>+ }
>>+
>>+ return 0;
>>+
>>+unregister_pins:
>>+ while (--i >= 0)
>>+ dpll_pin_unregister(dpll, pins[i].pin, ops, &pins[i]);
>>+ return ret;
>>+}
>>+
>>+/**
>>+ * ice_dpll_deinit_direct_pins - deinitialize direct pins
>>+ * @cgu: if cgu is present and controlled by this NIC
>>+ * @pins: pointer to pins array
>>+ * @count: number of pins
>>+ * @ops: callback ops registered with the pins
>>+ * @first: dpll device pointer
>>+ * @second: dpll device pointer
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ * If cgu is owned unregister pins from given dplls.
>>+ * Release pins resources to the dpll subsystem.
>>+ */
>>+static void
>>+ice_dpll_deinit_direct_pins(bool cgu, struct ice_dpll_pin *pins, int
>>count,
>>+ const struct dpll_pin_ops *ops,
>>+ struct dpll_device *first,
>>+ struct dpll_device *second)
>>+{
>>+ if (cgu) {
>>+ ice_dpll_unregister_pins(first, pins, ops, count);
>>+ ice_dpll_unregister_pins(second, pins, ops, count);
>>+ }
>>+ ice_dpll_release_pins(pins, count);
>>+}
>>+
>>+/**
>>+ * ice_dpll_init_direct_pins - initialize direct pins
>>+ * @pf: board private structure
>>+ * @cgu: if cgu is present and controlled by this NIC
>>+ * @pins: pointer to pins array
>>+ * @start_idx: on which index shall allocation start in dpll subsystem
>>+ * @count: number of pins
>>+ * @ops: callback ops registered with the pins
>>+ * @first: dpll device pointer
>>+ * @second: dpll device pointer
>>+ *
>>+ * Allocate directly connected pins of a given array in dpll subsystem.
>>+ * If cgu is owned register allocated pins with given dplls.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - registration failure reason
>>+ */
>>+static int
>>+ice_dpll_init_direct_pins(struct ice_pf *pf, bool cgu,
>>+ struct ice_dpll_pin *pins, int start_idx, int count,
>>+ const struct dpll_pin_ops *ops,
>>+ struct dpll_device *first, struct dpll_device *second)
>>+{
>>+ int ret;
>>+
>>+ ret = ice_dpll_get_pins(pf, pins, start_idx, count, pf-
>>dplls.clock_id);
>>+ if (ret)
>>+ return ret;
>>+ if (cgu) {
>>+ ret = ice_dpll_register_pins(first, pins, ops, count);
>>+ if (ret)
>>+ goto release_pins;
>>+ ret = ice_dpll_register_pins(second, pins, ops, count);
>>+ if (ret)
>>+ goto unregister_first;
>>+ }
>>+
>>+ return 0;
>>+
>>+unregister_first:
>>+ ice_dpll_unregister_pins(first, pins, ops, count);
>>+release_pins:
>>+ ice_dpll_release_pins(pins, count);
>>+ return ret;
>>+}
>>+
>>+/**
>>+ * ice_dpll_deinit_rclk_pin - release rclk pin resources
>>+ * @pf: board private structure
>>+ *
>>+ * Deregister rclk pin from parent pins and release resources in dpll
>>subsystem.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ */
>>+static void ice_dpll_deinit_rclk_pin(struct ice_pf *pf)
>>+{
>>+ struct ice_dpll_pin *rclk = &pf->dplls.rclk;
>>+ struct ice_vsi *vsi = ice_get_main_vsi(pf);
>>+ struct dpll_pin *parent;
>>+ int i;
>>+
>>+ for (i = 0; i < rclk->num_parents; i++) {
>>+ parent = pf->dplls.inputs[rclk->parent_idx[i]].pin;
>>+ if (!parent)
>>+ continue;
>>+ dpll_pin_on_pin_unregister(parent, rclk->pin,
>>+ &ice_dpll_rclk_ops, rclk);
>>+ }
>>+ if (WARN_ON_ONCE(!vsi || !vsi->netdev))
>>+ return;
>>+ netdev_dpll_pin_clear(vsi->netdev);
>>+ dpll_pin_put(rclk->pin);
>>+}
>>+
>>+/**
>>+ * ice_dpll_init_rclk_pins - initialize recovered clock pin
>>+ * @pf: board private structure
>>+ * @pin: pin to register
>>+ * @start_idx: on which index shall allocation start in dpll subsystem
>>+ * @ops: callback ops registered with the pins
>>+ *
>>+ * Allocate resource for recovered clock pin in dpll subsystem. Register
>>the
>>+ * pin with the parents it has in the info. Register pin with the pf's
>>main vsi
>>+ * netdev.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - registration failure reason
>>+ */
>>+static int
>>+ice_dpll_init_rclk_pins(struct ice_pf *pf, struct ice_dpll_pin *pin,
>>+ int start_idx, const struct dpll_pin_ops *ops)
>>+{
>>+ struct ice_vsi *vsi = ice_get_main_vsi(pf);
>>+ struct dpll_pin *parent;
>>+ int ret, i;
>>+
>>+ ret = ice_dpll_get_pins(pf, pin, start_idx, ICE_DPLL_RCLK_NUM_PER_PF,
>>+ pf->dplls.clock_id);
>>+ if (ret)
>>+ return ret;
>>+ for (i = 0; i < pf->dplls.rclk.num_parents; i++) {
>>+ parent = pf->dplls.inputs[pf->dplls.rclk.parent_idx[i]].pin;
>>+ if (!parent) {
>>+ ret = -ENODEV;
>>+ goto unregister_pins;
>>+ }
>>+ ret = dpll_pin_on_pin_register(parent, pf->dplls.rclk.pin,
>>+ ops, &pf->dplls.rclk);
>>+ if (ret)
>>+ goto unregister_pins;
>>+ }
>>+ if (WARN_ON((!vsi || !vsi->netdev)))
>>+ return -EINVAL;
>>+ netdev_dpll_pin_set(vsi->netdev, pf->dplls.rclk.pin);
>>+
>>+ return 0;
>>+
>>+unregister_pins:
>>+ while (i) {
>>+ parent = pf->dplls.inputs[pf->dplls.rclk.parent_idx[--i]].pin;
>>+ dpll_pin_on_pin_unregister(parent, pf->dplls.rclk.pin,
>>+ &ice_dpll_rclk_ops, &pf->dplls.rclk);
>>+ }
>>+ ice_dpll_release_pins(pin, ICE_DPLL_RCLK_NUM_PER_PF);
>>+ return ret;
>>+}
>>+
>>+/**
>>+ * ice_dpll_deinit_pins - deinitialize direct pins
>>+ * @pf: board private structure
>>+ * @cgu: if cgu is controlled by this pf
>>+ *
>>+ * If cgu is owned unregister directly connected pins from the dplls.
>>+ * Release resources of directly connected pins from the dpll subsystem.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ */
>>+static void ice_dpll_deinit_pins(struct ice_pf *pf, bool cgu)
>>+{
>>+ struct ice_dpll_pin *outputs = pf->dplls.outputs;
>>+ struct ice_dpll_pin *inputs = pf->dplls.inputs;
>>+ int num_outputs = pf->dplls.num_outputs;
>>+ int num_inputs = pf->dplls.num_inputs;
>>+ struct ice_dplls *d = &pf->dplls;
>>+ struct ice_dpll *de = &d->eec;
>>+ struct ice_dpll *dp = &d->pps;
>>+
>>+ ice_dpll_deinit_rclk_pin(pf);
>>+ if (cgu) {
>>+ ice_dpll_unregister_pins(dp->dpll, inputs, &ice_dpll_input_ops,
>>+ num_inputs);
>>+ ice_dpll_unregister_pins(de->dpll, inputs, &ice_dpll_input_ops,
>>+ num_inputs);
>>+ }
>>+ ice_dpll_release_pins(inputs, num_inputs);
>>+ if (cgu) {
>>+ ice_dpll_unregister_pins(dp->dpll, outputs,
>>+ &ice_dpll_output_ops, num_outputs);
>>+ ice_dpll_unregister_pins(de->dpll, outputs,
>>+ &ice_dpll_output_ops, num_outputs);
>>+ ice_dpll_release_pins(outputs, num_outputs);
>>+ }
>>+}
>>+
>>+/**
>>+ * ice_dpll_init_pins - init pins and register pins with a dplls
>>+ * @pf: board private structure
>>+ * @cgu: if cgu is present and controlled by this NIC
>>+ *
>>+ * Initialize directly connected pf's pins within pf's dplls in a Linux
>>dpll
>>+ * subsystem.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - initialization failure reason
>>+ */
>>+static int ice_dpll_init_pins(struct ice_pf *pf, bool cgu)
>>+{
>>+ u32 rclk_idx;
>>+ int ret;
>>+
>>+ ret = ice_dpll_init_direct_pins(pf, cgu, pf->dplls.inputs, 0,
>>+ pf->dplls.num_inputs,
>>+ &ice_dpll_input_ops,
>>+ pf->dplls.eec.dpll, pf->dplls.pps.dpll);
>>+ if (ret)
>>+ return ret;
>>+ if (cgu) {
>>+ ret = ice_dpll_init_direct_pins(pf, cgu, pf->dplls.outputs,
>>+ pf->dplls.num_inputs,
>>+ pf->dplls.num_outputs,
>>+ &ice_dpll_output_ops,
>>+ pf->dplls.eec.dpll,
>>+ pf->dplls.pps.dpll);
>>+ if (ret)
>>+ goto deinit_inputs;
>>+ }
>>+ rclk_idx = pf->dplls.num_inputs + pf->dplls.num_outputs + pf-
>>hw.pf_id;
>>+ ret = ice_dpll_init_rclk_pins(pf, &pf->dplls.rclk, rclk_idx,
>>+ &ice_dpll_rclk_ops);
>>+ if (ret)
>>+ goto deinit_outputs;
>>+
>>+ return 0;
>>+deinit_outputs:
>>+ ice_dpll_deinit_direct_pins(cgu, pf->dplls.outputs,
>>+ pf->dplls.num_outputs,
>>+ &ice_dpll_output_ops, pf->dplls.pps.dpll,
>>+ pf->dplls.eec.dpll);
>>+deinit_inputs:
>>+ ice_dpll_deinit_direct_pins(cgu, pf->dplls.inputs, pf-
>>dplls.num_inputs,
>>+ &ice_dpll_input_ops, pf->dplls.pps.dpll,
>>+ pf->dplls.eec.dpll);
>>+ return ret;
>>+}
>>+
>>+/**
>>+ * ice_dpll_deinit_dpll - deinitialize dpll device
>>+ * @pf: board private structure
>>+ * @d: pointer to ice_dpll
>>+ * @cgu: if cgu is present and controlled by this NIC
>>+ *
>>+ * If cgu is owned unregister the dpll from dpll subsystem.
>>+ * Release resources of dpll device from dpll subsystem.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ */
>>+static void
>>+ice_dpll_deinit_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
>>+{
>>+ if (cgu)
>>+ dpll_device_unregister(d->dpll, &ice_dpll_ops, d);
>>+ dpll_device_put(d->dpll);
>>+}
>>+
>>+/**
>>+ * ice_dpll_init_dpll - initialize dpll device in dpll subsystem
>>+ * @pf: board private structure
>>+ * @d: dpll to be initialized
>>+ * @cgu: if cgu is present and controlled by this NIC
>>+ * @type: type of dpll being initialized
>>+ *
>>+ * Allocate dpll instance for this board in dpll subsystem, if cgu is
>>controlled
>>+ * by this NIC, register dpll with the callback ops.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - initialization failure reason
>>+ */
>>+static int
>>+ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu,
>>+ enum dpll_type type)
>>+{
>>+ u64 clock_id = pf->dplls.clock_id;
>>+ int ret;
>>+
>>+ d->dpll = dpll_device_get(clock_id, d->dpll_idx, THIS_MODULE);
>>+ if (IS_ERR(d->dpll)) {
>>+ ret = PTR_ERR(d->dpll);
>>+ dev_err(ice_pf_to_dev(pf),
>>+ "dpll_device_get failed (%p) err=%d\n", d, ret);
>>+ return ret;
>>+ }
>>+ d->pf = pf;
>>+ if (cgu) {
>>+ ret = dpll_device_register(d->dpll, type, &ice_dpll_ops, d);
>>+ if (ret) {
>>+ dpll_device_put(d->dpll);
>>+ return ret;
>>+ }
>>+ }
>>+
>>+ return 0;
>>+}
>>+
>>+/**
>>+ * ice_dpll_deinit_worker - deinitialize dpll kworker
>>+ * @pf: board private structure
>>+ *
>>+ * Stop dpll's kworker, release it's resources.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ */
>>+static void ice_dpll_deinit_worker(struct ice_pf *pf)
>>+{
>>+ struct ice_dplls *d = &pf->dplls;
>>+
>>+ kthread_cancel_delayed_work_sync(&d->work);
>>+ kthread_destroy_worker(d->kworker);
>>+}
>>+
>>+/**
>>+ * ice_dpll_init_worker - Initialize DPLLs periodic worker
>>+ * @pf: board private structure
>>+ *
>>+ * Create and start DPLLs periodic worker.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - create worker failure
>>+ */
>>+static int ice_dpll_init_worker(struct ice_pf *pf)
>>+{
>>+ struct ice_dplls *d = &pf->dplls;
>>+ struct kthread_worker *kworker;
>>+
>>+ ice_dpll_update_state(pf, &d->eec, true);
>>+ ice_dpll_update_state(pf, &d->pps, true);
>>+ kthread_init_delayed_work(&d->work, ice_dpll_periodic_work);
>>+ kworker = kthread_create_worker(0, "ice-dplls-%s",
>>+ dev_name(ice_pf_to_dev(pf)));
>>+ if (IS_ERR(kworker))
>>+ return PTR_ERR(kworker);
>>+ d->kworker = kworker;
>>+ d->cgu_state_acq_err_num = 0;
>>+ kthread_queue_delayed_work(d->kworker, &d->work, 0);
>>+
>>+ return 0;
>>+}
>>+
>>+/**
>>+ * 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.
>
>No, it is not.
>
Yes, will fix.
>
>>+ * 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;
>>+ }
>>+
>>+ return ret;
>>+}
>>+
>>+/**
>>+ * ice_dpll_init_info_rclk_pin - initializes rclk pin information
>>+ * @pf: board private structure
>>+ *
>>+ * Init information for rclk pin, cache them in pf->dplls.rclk.
>>+ *
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - init failure reason
>>+ */
>>+static int ice_dpll_init_info_rclk_pin(struct ice_pf *pf)
>>+{
>>+ struct ice_dpll_pin *pin = &pf->dplls.rclk;
>>+
>>+ pin->prop.type = DPLL_PIN_TYPE_SYNCE_ETH_PORT;
>>+ pin->prop.capabilities |= DPLL_PIN_CAPS_STATE_CAN_CHANGE;
>>+ pin->pf = pf;
>>+
>>+ return ice_dpll_pin_state_update(pf, pin,
>>+ ICE_DPLL_PIN_TYPE_RCLK_INPUT, NULL);
>>+}
>>+
>>+/**
>>+ * ice_dpll_init_pins_info - init pins info wrapper
>>+ * @pf: board private structure
>>+ * @pin_type: type of pins being initialized
>>+ *
>>+ * Wraps functions for pin initialization.
>>+ *
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - init failure reason
>>+ */
>>+static int
>>+ice_dpll_init_pins_info(struct ice_pf *pf, enum ice_dpll_pin_type
>>pin_type)
>>+{
>>+ switch (pin_type) {
>>+ case ICE_DPLL_PIN_TYPE_INPUT:
>>+ case ICE_DPLL_PIN_TYPE_OUTPUT:
>>+ return ice_dpll_init_info_direct_pins(pf, pin_type);
>>+ case ICE_DPLL_PIN_TYPE_RCLK_INPUT:
>>+ return ice_dpll_init_info_rclk_pin(pf);
>>+ default:
>>+ return -EINVAL;
>>+ }
>>+}
>>+
>>+/**
>>+ * ice_dpll_deinit_info - release memory allocated for pins info
>>+ * @pf: board private structure
>>+ *
>>+ * Release memory allocated for pins by ice_dpll_init_info function.
>>+ *
>>+ * Context: Called under pf->dplls.lock
>
>No, it is not.
>
Yes, will fix.
>
>>+ */
>>+static void ice_dpll_deinit_info(struct ice_pf *pf)
>>+{
>>+ kfree(pf->dplls.inputs);
>>+ kfree(pf->dplls.outputs);
>>+ kfree(pf->dplls.eec.input_prio);
>>+ kfree(pf->dplls.pps.input_prio);
>>+}
>>+
>>+/**
>>+ * 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
>
>No, it is not.
>
Yes, will fix.
>
>>+ * 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)
>>+ 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_deinit - Disable the driver/HW support for dpll subsystem
>>+ * the dpll device.
>>+ * @pf: board private structure
>>+ *
>>+ * Handles the cleanup work required after dpll initialization,freeing
>>resources
>>+ * and unregistering the dpll, pin and all resources used for handling
>>them.
>>+ *
>>+ * Context: Function holds pf->dplls.lock mutex.
>
>No it does not. Update your comments. Or better, remove them,
>they are totally useless anyway :/
>
Yes, will fix.
>
>>+ */
>>+void ice_dpll_deinit(struct ice_pf *pf)
>>+{
>>+ bool cgu = ice_is_feature_supported(pf, ICE_F_CGU);
>>+
>>+ if (!test_bit(ICE_FLAG_DPLL, pf->flags))
>>+ return;
>>+ clear_bit(ICE_FLAG_DPLL, pf->flags);
>>+
>>+ ice_dpll_deinit_pins(pf, cgu);
>>+ ice_dpll_deinit_dpll(pf, &pf->dplls.pps, cgu);
>>+ ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu);
>>+ ice_dpll_deinit_info(pf);
>>+ if (cgu)
>>+ ice_dpll_deinit_worker(pf);
>
>Could you please order the ice_dpll_deinit() to be symmetrical to
>ice_dpll_init()? Then, you can drop ICE_FLAG_DPLL flag entirely, as the
>ice_dpll_periodic_work() function is the only reason why you need it
>currently.
>
Not true.
The feature flag is common approach in ice. If the feature was successfully
initialized the flag is set. It allows to determine if deinit of the feature
is required on driver unload.
Right now the check for the flag is not only in kworker but also in each
callback, if the flag were cleared the data shall be not accessed by callbacks.
I know this is not required, but it helps on loading and unloading the driver,
thanks to that, spam of pin-get dump is not slowing the driver load/unload.
>
>>+ mutex_destroy(&pf->dplls.lock);
>>+}
>>+
>>+/**
>>+ * 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
>>+ * configuration requests.
>>+ *
>>+ * Context: Function initializes and holds pf->dplls.lock mutex.
>
>No, it does not hold it.
>
Yes, will fix.
>
>>+ */
>>+void ice_dpll_init(struct ice_pf *pf)
>>+{
>>+ bool cgu = ice_is_feature_supported(pf, ICE_F_CGU);
>>+ struct ice_dplls *d = &pf->dplls;
>>+ int err = 0;
>>+
>>+ err = ice_dpll_init_info(pf, cgu);
>>+ if (err)
>>+ goto err_exit;
>>+ err = ice_dpll_init_dpll(pf, &pf->dplls.eec, cgu, DPLL_TYPE_EEC);
>>+ if (err)
>>+ goto deinit_info;
>>+ err = ice_dpll_init_dpll(pf, &pf->dplls.pps, cgu, DPLL_TYPE_PPS);
>>+ if (err)
>>+ goto deinit_eec;
>>+ err = ice_dpll_init_pins(pf, cgu);
>>+ if (err)
>>+ goto deinit_pps;
>>+ set_bit(ICE_FLAG_DPLL, pf->flags);
>>+ if (cgu) {
>>+ err = ice_dpll_init_worker(pf);
>>+ if (err)
>>+ goto deinit_pins;
>>+ }
>>+
>>+ return;
>>+
>>+deinit_pins:
>>+ ice_dpll_deinit_pins(pf, cgu);
>>+deinit_pps:
>>+ ice_dpll_deinit_dpll(pf, &pf->dplls.pps, cgu);
>>+deinit_eec:
>>+ ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu);
>>+deinit_info:
>>+ ice_dpll_deinit_info(pf);
>>+err_exit:
>>+ clear_bit(ICE_FLAG_DPLL, pf->flags);
>>+ mutex_unlock(&d->lock);
>
>Leftover, please remove.
>
Yes, will fix.
>
>>+ mutex_destroy(&d->lock);
>>+ dev_warn(ice_pf_to_dev(pf), "DPLLs init failure err:%d\n", err);
>>+}
>>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
>>+ * @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;
>>+};
>>+
>>+/** ice_dpll - store info required for DPLL control
>>+ * @dpll: pointer to dpll dev
>>+ * @pf: pointer to pf, which has registered the dpll_device
>>+ * @dpll_idx: index of dpll on the NIC
>>+ * @input_idx: currently selected input index
>>+ * @prev_input_idx: previously selected input index
>>+ * @ref_state: state of dpll reference signals
>>+ * @eec_mode: eec_mode dpll is configured for
>>+ * @phase_shift: phase shift delay of a dpll
>>+ * @input_prio: priorities of each input
>>+ * @dpll_state: current dpll sync state
>>+ * @prev_dpll_state: last dpll sync state
>>+ * @active_input: pointer to active input pin
>>+ * @prev_input: pointer to previous active input pin
>>+ */
>>+struct ice_dpll {
>>+ struct dpll_device *dpll;
>>+ struct ice_pf *pf;
>>+ u8 dpll_idx;
>>+ u8 input_idx;
>>+ u8 prev_input_idx;
>>+ u8 ref_state;
>>+ u8 eec_mode;
>>+ s64 phase_shift;
>>+ u8 *input_prio;
>>+ enum dpll_lock_status dpll_state;
>>+ enum dpll_lock_status prev_dpll_state;
>>+ enum dpll_mode mode;
>>+ struct dpll_pin *active_input;
>>+ struct dpll_pin *prev_input;
>>+};
>>+
>>+/** ice_dplls - store info required for CCU (clock controlling unit)
>>+ * @kworker: periodic worker
>>+ * @work: periodic work
>>+ * @lock: locks access to configuration of a dpll
>>+ * @eec: pointer to EEC dpll dev
>>+ * @pps: pointer to PPS dpll dev
>>+ * @inputs: input pins pointer
>>+ * @outputs: output pins pointer
>>+ * @rclk: recovered pins pointer
>>+ * @num_inputs: number of input pins available on dpll
>>+ * @num_outputs: number of output pins available on dpll
>>+ * @cgu_state_acq_err_num: number of errors returned during periodic work
>>+ * @base_rclk_idx: idx of first pin used for clock revocery pins
>>+ * @clock_id: clock_id of dplls
>>+ */
>>+struct ice_dplls {
>>+ struct kthread_worker *kworker;
>>+ struct kthread_delayed_work work;
>>+ struct mutex lock;
>>+ struct ice_dpll eec;
>>+ struct ice_dpll pps;
>>+ struct ice_dpll_pin *inputs;
>>+ struct ice_dpll_pin *outputs;
>>+ struct ice_dpll_pin rclk;
>>+ u8 num_inputs;
>>+ u8 num_outputs;
>>+ int cgu_state_acq_err_num;
>>+ u8 base_rclk_idx;
>>+ u64 clock_id;
>>+ s32 input_phase_adj_max;
>>+ s32 output_phase_adj_max;
>>+};
>>+
>>+void ice_dpll_init(struct ice_pf *pf);
>>+
>>+void ice_dpll_deinit(struct ice_pf *pf);
>>+
>>+#endif
>>diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
>>b/drivers/net/ethernet/intel/ice/ice_main.c
>>index 19a5e7f3a075..0a94daaf3d20 100644
>>--- a/drivers/net/ethernet/intel/ice/ice_main.c
>>+++ b/drivers/net/ethernet/intel/ice/ice_main.c
>>@@ -4613,6 +4613,10 @@ static void ice_init_features(struct ice_pf *pf)
>> if (ice_is_feature_supported(pf, ICE_F_GNSS))
>> ice_gnss_init(pf);
>>
>>+ if (ice_is_feature_supported(pf, ICE_F_CGU) ||
>>+ ice_is_feature_supported(pf, ICE_F_PHY_RCLK))
>>+ ice_dpll_init(pf);
>>+
>> /* Note: Flow director init failure is non-fatal to load */
>> if (ice_init_fdir(pf))
>> dev_err(dev, "could not initialize flow director\n");
>>@@ -4639,6 +4643,9 @@ static void ice_deinit_features(struct ice_pf *pf)
>> ice_gnss_exit(pf);
>> if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags))
>> ice_ptp_release(pf);
>>+ if (ice_is_feature_supported(pf, ICE_F_PHY_RCLK) ||
>>+ ice_is_feature_supported(pf, ICE_F_CGU))
>>+ ice_dpll_deinit(pf);
>> }
>>
>> static void ice_init_wakeup(struct ice_pf *pf)
>>--
>>2.27.0
>>
Powered by blists - more mailing lists