Commit e9ce7f49 authored by Jakub Kicinski's avatar Jakub Kicinski
Browse files
Tony Nguyen says:

====================
ice: postpone service task disabling

Przemek Kitszel says:

Move service task shutdown to the very end of driver teardown procedure.
This is needed (or at least beneficial) for all unwinding functions that
talk to FW/HW via Admin Queue (so, most of top-level functions, like
ice_deinit_hw()).

Most of the patches move stuff around (I believe it makes it much easier
to review/proof when kept separate) in preparation to defer stopping the
service task to the very end of ice_remove() (and other unwinding flows).
Then last patch fixes duplicate call to ice_init_hw() (actual, but
unlikely to encounter, so -next, given the size of the changes).

First patch is not much related, only by that it was developed together

* '100GbE' of git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue:
  ice: remove duplicate call to ice_deinit_hw() on error paths
  ice: move ice_deinit_dev() to the end of deinit paths
  ice: extract ice_init_dev() from ice_init()
  ice: move ice_init_pf() out of ice_init_dev()
  ice: move udp_tunnel_nic and misc IRQ setup into ice_init_pf()
  ice: ice_init_pf: destroy mutexes and xarrays on memory alloc failure
  ice: move ice_init_interrupt_scheme() prior ice_init_pf()
  ice: move service task start out of ice_init_pf()
  ice: enforce RTNL assumption of queue NAPI manipulation
====================

Link: https://patch.msgid.link/20251024204746.3092277-1-anthony.l.nguyen@intel.com


Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
parents 5c00da85 1390b8b3
Loading
Loading
Loading
Loading
+17 −4
Original line number Diff line number Diff line
@@ -459,6 +459,7 @@ static void ice_devlink_reinit_down(struct ice_pf *pf)
	rtnl_lock();
	ice_vsi_decfg(ice_get_main_vsi(pf));
	rtnl_unlock();
	ice_deinit_pf(pf);
	ice_deinit_dev(pf);
}

@@ -1231,11 +1232,13 @@ static void ice_set_min_max_msix(struct ice_pf *pf)
static int ice_devlink_reinit_up(struct ice_pf *pf)
{
	struct ice_vsi *vsi = ice_get_main_vsi(pf);
	struct device *dev = ice_pf_to_dev(pf);
	bool need_dev_deinit = false;
	int err;

	err = ice_init_hw(&pf->hw);
	if (err) {
		dev_err(ice_pf_to_dev(pf), "ice_init_hw failed: %d\n", err);
		dev_err(dev, "ice_init_hw failed: %d\n", err);
		return err;
	}

@@ -1246,13 +1249,19 @@ static int ice_devlink_reinit_up(struct ice_pf *pf)
	if (err)
		goto unroll_hw_init;

	err = ice_init_pf(pf);
	if (err) {
		dev_err(dev, "ice_init_pf failed: %d\n", err);
		goto unroll_dev_init;
	}

	vsi->flags = ICE_VSI_FLAG_INIT;

	rtnl_lock();
	err = ice_vsi_cfg(vsi);
	rtnl_unlock();
	if (err)
		goto err_vsi_cfg;
		goto unroll_pf_init;

	/* No need to take devl_lock, it's already taken by devlink API */
	err = ice_load(pf);
@@ -1265,10 +1274,14 @@ static int ice_devlink_reinit_up(struct ice_pf *pf)
	rtnl_lock();
	ice_vsi_decfg(vsi);
	rtnl_unlock();
err_vsi_cfg:
	ice_deinit_dev(pf);
unroll_pf_init:
	ice_deinit_pf(pf);
unroll_dev_init:
	need_dev_deinit = true;
unroll_hw_init:
	ice_deinit_hw(&pf->hw);
	if (need_dev_deinit)
		ice_deinit_dev(pf);
	return err;
}

+4 −0
Original line number Diff line number Diff line
@@ -1029,11 +1029,15 @@ int ice_open(struct net_device *netdev);
int ice_open_internal(struct net_device *netdev);
int ice_stop(struct net_device *netdev);
void ice_service_task_schedule(struct ice_pf *pf);
void ice_start_service_task(struct ice_pf *pf);
int ice_load(struct ice_pf *pf);
void ice_unload(struct ice_pf *pf);
void ice_adv_lnk_speed_maps_init(void);
void ice_init_dev_hw(struct ice_pf *pf);
int ice_init_dev(struct ice_pf *pf);
void ice_deinit_dev(struct ice_pf *pf);
int ice_init_pf(struct ice_pf *pf);
void ice_deinit_pf(struct ice_pf *pf);
int ice_change_mtu(struct net_device *netdev, int new_mtu);
void ice_tx_timeout(struct net_device *netdev, unsigned int txqueue);
int ice_xdp(struct net_device *dev, struct netdev_bpf *xdp);
+3 −0
Original line number Diff line number Diff line
@@ -1161,6 +1161,9 @@ int ice_init_hw(struct ice_hw *hw)
	status = ice_init_hw_tbls(hw);
	if (status)
		goto err_unroll_fltr_mgmt_struct;

	ice_init_dev_hw(hw->back);

	mutex_init(&hw->tnl_lock);
	ice_init_chk_recipe_reuse_support(hw);

+2 −2
Original line number Diff line number Diff line
@@ -2769,7 +2769,6 @@ void ice_dis_vsi(struct ice_vsi *vsi, bool locked)
 * @vsi: VSI pointer
 *
 * Associate queue[s] with napi for all vectors.
 * The caller must hold rtnl_lock.
 */
void ice_vsi_set_napi_queues(struct ice_vsi *vsi)
{
@@ -2779,6 +2778,7 @@ void ice_vsi_set_napi_queues(struct ice_vsi *vsi)
	if (!netdev)
		return;

	ASSERT_RTNL();
	ice_for_each_rxq(vsi, q_idx)
		netif_queue_set_napi(netdev, q_idx, NETDEV_QUEUE_TYPE_RX,
				     &vsi->rx_rings[q_idx]->q_vector->napi);
@@ -2799,7 +2799,6 @@ void ice_vsi_set_napi_queues(struct ice_vsi *vsi)
 * @vsi: VSI pointer
 *
 * Clear the association between all VSI queues queue[s] and napi.
 * The caller must hold rtnl_lock.
 */
void ice_vsi_clear_napi_queues(struct ice_vsi *vsi)
{
@@ -2809,6 +2808,7 @@ void ice_vsi_clear_napi_queues(struct ice_vsi *vsi)
	if (!netdev)
		return;

	ASSERT_RTNL();
	/* Clear the NAPI's interrupt number */
	ice_for_each_q_vector(vsi, v_idx) {
		struct ice_q_vector *q_vector = vsi->q_vectors[v_idx];
+84 −85
Original line number Diff line number Diff line
@@ -3949,9 +3949,10 @@ u16 ice_get_avail_rxq_count(struct ice_pf *pf)
 * ice_deinit_pf - Unrolls initialziations done by ice_init_pf
 * @pf: board private structure to initialize
 */
static void ice_deinit_pf(struct ice_pf *pf)
void ice_deinit_pf(struct ice_pf *pf)
{
	ice_service_task_stop(pf);
	/* note that we unroll also on ice_init_pf() failure here */

	mutex_destroy(&pf->lag_mutex);
	mutex_destroy(&pf->adev_mutex);
	mutex_destroy(&pf->sw_mutex);
@@ -3977,6 +3978,9 @@ static void ice_deinit_pf(struct ice_pf *pf)
	if (pf->ptp.clock)
		ptp_clock_unregister(pf->ptp.clock);

	if (!xa_empty(&pf->irq_tracker.entries))
		ice_free_irq_msix_misc(pf);

	xa_destroy(&pf->dyn_ports);
	xa_destroy(&pf->sf_nums);
}
@@ -4030,13 +4034,25 @@ static void ice_set_pf_caps(struct ice_pf *pf)
	pf->max_pf_rxqs = func_caps->common_cap.num_rxq;
}

void ice_start_service_task(struct ice_pf *pf)
{
	timer_setup(&pf->serv_tmr, ice_service_timer, 0);
	pf->serv_tmr_period = HZ;
	INIT_WORK(&pf->serv_task, ice_service_task);
	clear_bit(ICE_SERVICE_SCHED, pf->state);
}

/**
 * ice_init_pf - Initialize general software structures (struct ice_pf)
 * @pf: board private structure to initialize
 * Return: 0 on success, negative errno otherwise.
 */
static int ice_init_pf(struct ice_pf *pf)
int ice_init_pf(struct ice_pf *pf)
{
	ice_set_pf_caps(pf);
	struct udp_tunnel_nic_info *udp_tunnel_nic = &pf->hw.udp_tunnel_nic;
	struct device *dev = ice_pf_to_dev(pf);
	struct ice_hw *hw = &pf->hw;
	int err = -ENOMEM;

	mutex_init(&pf->sw_mutex);
	mutex_init(&pf->tc_mutex);
@@ -4049,32 +4065,7 @@ static int ice_init_pf(struct ice_pf *pf)

	init_waitqueue_head(&pf->reset_wait_queue);

	/* setup service timer and periodic service task */
	timer_setup(&pf->serv_tmr, ice_service_timer, 0);
	pf->serv_tmr_period = HZ;
	INIT_WORK(&pf->serv_task, ice_service_task);
	clear_bit(ICE_SERVICE_SCHED, pf->state);

	mutex_init(&pf->avail_q_mutex);
	pf->avail_txqs = bitmap_zalloc(pf->max_pf_txqs, GFP_KERNEL);
	if (!pf->avail_txqs)
		return -ENOMEM;

	pf->avail_rxqs = bitmap_zalloc(pf->max_pf_rxqs, GFP_KERNEL);
	if (!pf->avail_rxqs) {
		bitmap_free(pf->avail_txqs);
		pf->avail_txqs = NULL;
		return -ENOMEM;
	}

	pf->txtime_txqs = bitmap_zalloc(pf->max_pf_txqs, GFP_KERNEL);
	if (!pf->txtime_txqs) {
		bitmap_free(pf->avail_txqs);
		pf->avail_txqs = NULL;
		bitmap_free(pf->avail_rxqs);
		pf->avail_rxqs = NULL;
		return -ENOMEM;
	}

	mutex_init(&pf->vfs.table_lock);
	hash_init(pf->vfs.table);
@@ -4087,7 +4078,36 @@ static int ice_init_pf(struct ice_pf *pf)
	xa_init(&pf->dyn_ports);
	xa_init(&pf->sf_nums);

	pf->avail_txqs = bitmap_zalloc(pf->max_pf_txqs, GFP_KERNEL);
	pf->avail_rxqs = bitmap_zalloc(pf->max_pf_rxqs, GFP_KERNEL);
	pf->txtime_txqs = bitmap_zalloc(pf->max_pf_txqs, GFP_KERNEL);
	if (!pf->avail_txqs || !pf->avail_rxqs || !pf->txtime_txqs)
		goto undo_init;

	udp_tunnel_nic->set_port = ice_udp_tunnel_set_port;
	udp_tunnel_nic->unset_port = ice_udp_tunnel_unset_port;
	udp_tunnel_nic->shared = &hw->udp_tunnel_shared;
	udp_tunnel_nic->tables[0].n_entries = hw->tnl.valid_count[TNL_VXLAN];
	udp_tunnel_nic->tables[0].tunnel_types = UDP_TUNNEL_TYPE_VXLAN;
	udp_tunnel_nic->tables[1].n_entries = hw->tnl.valid_count[TNL_GENEVE];
	udp_tunnel_nic->tables[1].tunnel_types = UDP_TUNNEL_TYPE_GENEVE;

	/* In case of MSIX we are going to setup the misc vector right here
	 * to handle admin queue events etc. In case of legacy and MSI
	 * the misc functionality and queue processing is combined in
	 * the same vector and that gets setup at open.
	 */
	err = ice_req_irq_msix_misc(pf);
	if (err) {
		dev_err(dev, "setup of misc vector failed: %d\n", err);
		goto undo_init;
	}

	return 0;
undo_init:
	/* deinit handles half-initialized pf just fine */
	ice_deinit_pf(pf);
	return err;
}

/**
@@ -4722,9 +4742,8 @@ static void ice_decfg_netdev(struct ice_vsi *vsi)
	vsi->netdev = NULL;
}

int ice_init_dev(struct ice_pf *pf)
void ice_init_dev_hw(struct ice_pf *pf)
{
	struct device *dev = ice_pf_to_dev(pf);
	struct ice_hw *hw = &pf->hw;
	int err;

@@ -4744,61 +4763,28 @@ int ice_init_dev(struct ice_pf *pf)
		 */
		ice_set_safe_mode_caps(hw);
	}

	err = ice_init_pf(pf);
	if (err) {
		dev_err(dev, "ice_init_pf failed: %d\n", err);
		return err;
}

	pf->hw.udp_tunnel_nic.set_port = ice_udp_tunnel_set_port;
	pf->hw.udp_tunnel_nic.unset_port = ice_udp_tunnel_unset_port;
	pf->hw.udp_tunnel_nic.shared = &pf->hw.udp_tunnel_shared;
	if (pf->hw.tnl.valid_count[TNL_VXLAN]) {
		pf->hw.udp_tunnel_nic.tables[0].n_entries =
			pf->hw.tnl.valid_count[TNL_VXLAN];
		pf->hw.udp_tunnel_nic.tables[0].tunnel_types =
			UDP_TUNNEL_TYPE_VXLAN;
	}
	if (pf->hw.tnl.valid_count[TNL_GENEVE]) {
		pf->hw.udp_tunnel_nic.tables[1].n_entries =
			pf->hw.tnl.valid_count[TNL_GENEVE];
		pf->hw.udp_tunnel_nic.tables[1].tunnel_types =
			UDP_TUNNEL_TYPE_GENEVE;
	}
int ice_init_dev(struct ice_pf *pf)
{
	struct device *dev = ice_pf_to_dev(pf);
	int err;

	ice_set_pf_caps(pf);
	err = ice_init_interrupt_scheme(pf);
	if (err) {
		dev_err(dev, "ice_init_interrupt_scheme failed: %d\n", err);
		err = -EIO;
		goto unroll_pf_init;
		return -EIO;
	}

	/* In case of MSIX we are going to setup the misc vector right here
	 * to handle admin queue events etc. In case of legacy and MSI
	 * the misc functionality and queue processing is combined in
	 * the same vector and that gets setup at open.
	 */
	err = ice_req_irq_msix_misc(pf);
	if (err) {
		dev_err(dev, "setup of misc vector failed: %d\n", err);
		goto unroll_irq_scheme_init;
	}
	ice_start_service_task(pf);

	return 0;

unroll_irq_scheme_init:
	ice_clear_interrupt_scheme(pf);
unroll_pf_init:
	ice_deinit_pf(pf);
	return err;
}

void ice_deinit_dev(struct ice_pf *pf)
{
	ice_free_irq_msix_misc(pf);
	ice_deinit_pf(pf);
	ice_deinit_hw(&pf->hw);
	ice_service_task_stop(pf);

	/* Service task is already stopped, so call reset directly. */
	ice_reset(&pf->hw, ICE_RESET_PFR);
@@ -5038,21 +5024,24 @@ static void ice_deinit_devlink(struct ice_pf *pf)

static int ice_init(struct ice_pf *pf)
{
	struct device *dev = ice_pf_to_dev(pf);
	int err;

	err = ice_init_dev(pf);
	if (err)
	err = ice_init_pf(pf);
	if (err) {
		dev_err(dev, "ice_init_pf failed: %d\n", err);
		return err;
	}

	if (pf->hw.mac_type == ICE_MAC_E830) {
		err = pci_enable_ptm(pf->pdev, NULL);
		if (err)
			dev_dbg(ice_pf_to_dev(pf), "PCIe PTM not supported by PCIe bus/controller\n");
			dev_dbg(dev, "PCIe PTM not supported by PCIe bus/controller\n");
	}

	err = ice_alloc_vsis(pf);
	if (err)
		goto err_alloc_vsis;
		goto unroll_pf_init;

	err = ice_init_pf_sw(pf);
	if (err)
@@ -5089,8 +5078,8 @@ static int ice_init(struct ice_pf *pf)
	ice_deinit_pf_sw(pf);
err_init_pf_sw:
	ice_dealloc_vsis(pf);
err_alloc_vsis:
	ice_deinit_dev(pf);
unroll_pf_init:
	ice_deinit_pf(pf);
	return err;
}

@@ -5101,7 +5090,7 @@ static void ice_deinit(struct ice_pf *pf)

	ice_deinit_pf_sw(pf);
	ice_dealloc_vsis(pf);
	ice_deinit_dev(pf);
	ice_deinit_pf(pf);
}

/**
@@ -5235,6 +5224,7 @@ static int
ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
{
	struct device *dev = &pdev->dev;
	bool need_dev_deinit = false;
	struct ice_adapter *adapter;
	struct ice_pf *pf;
	struct ice_hw *hw;
@@ -5331,10 +5321,14 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
	}
	pf->adapter = adapter;

	err = ice_init(pf);
	err = ice_init_dev(pf);
	if (err)
		goto unroll_adapter;

	err = ice_init(pf);
	if (err)
		goto unroll_dev_init;

	devl_lock(priv_to_devlink(pf));
	err = ice_load(pf);
	if (err)
@@ -5352,10 +5346,14 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
unroll_init:
	devl_unlock(priv_to_devlink(pf));
	ice_deinit(pf);
unroll_dev_init:
	need_dev_deinit = true;
unroll_adapter:
	ice_adapter_put(pdev);
unroll_hw_init:
	ice_deinit_hw(hw);
	if (need_dev_deinit)
		ice_deinit_dev(pf);
	return err;
}

@@ -5450,10 +5448,6 @@ static void ice_remove(struct pci_dev *pdev)

	ice_hwmon_exit(pf);

	ice_service_task_stop(pf);
	ice_aq_cancel_waiting_tasks(pf);
	set_bit(ICE_DOWN, pf->state);

	if (!ice_is_safe_mode(pf))
		ice_remove_arfs(pf);

@@ -5471,6 +5465,11 @@ static void ice_remove(struct pci_dev *pdev)
	ice_set_wake(pf);

	ice_adapter_put(pdev);
	ice_deinit_hw(&pf->hw);

	ice_deinit_dev(pf);
	ice_aq_cancel_waiting_tasks(pf);
	set_bit(ICE_DOWN, pf->state);
}

/**