[PATCH net v3] net: mana: fix reset work race with device removal

[PATCH net v3] net: mana: fix reset work race with device removal

[Fan Wu]

The reset service work is queued on the system workqueue and can
outlive mana_gd_remove(), which frees the GDMA context. It may
then dereference gc through the stale service work.

Embed the service work in gdma_context and make GC_IN_SERVICE describe
whether it may still access gc. Removal and probe unwind close new
admission with GC_REMOVING and wait for an admitted cycle to retire
before clearing drvdata and freeing gc. Service exits retire before
taking the PCI rescan/remove lock, avoiding a lock-cycle with remove.

Do not admit service work while probe is still constructing or
unwinding the device. Latch reset events seen during probe and let
the probe rollback/recovery path handle them; a boundary recheck
preserves events racing probe completion.

The service work stays on the system workqueue because a reset cycle
destroys and re-creates gc->service_wq.

This issue was found by an in-house static analysis tool.

Fixes: fbe346ce9d62 ("net: mana: Handle Reset Request from MANA NIC")
Cc: stable@vger.kernel.org
Reviewed-by: Simon Horman <horms@kernel.org>
Assisted-by: Codex:gpt-5.6
Co-developed-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---

Changes since v2 (<20260905023602.425827-1-fanwu01@zju.edu.cn>,
https://lore.kernel.org/netdev/20260905023602.425827-1-fanwu01@zju.edu.cn):

- Dropped the now-unused cleanup_mana_rdma and cleanup_mana labels in
  the probe error path (Simon Horman).

Changes since v1 (<20260805143812.220509-1-fanwu01@zju.edu.cn>,
https://lore.kernel.org/netdev/20260805143812.220509-1-fanwu01@zju.edu.cn):

- Dropped the device_lock() serialisation: holding the driver-core
  device lock across mana_gd_suspend() + msleep() + mana_gd_resume()
  blocks unbind, reboot, device PM and all PCI hotplug for up to a
  full reset cycle, and can deadlock against the
  flush_workqueue()/destroy_workqueue() of gc->service_wq.

- Replaced it with admission/drain gates: GC_REMOVING closes new
  admission and the freeing paths wait for an admitted cycle to
  retire (clear_bit_unlock/test_bit_acquire pairing) before clearing
  drvdata and calling vfree(); the failed-resume rescan no longer
  reopens admission, and mana_tx_timeout() also skips queue-reset
  work during removal.

- No longer admit service work before the probe completes (the
  reset-event handler no longer overloads GC_PROBE_SUCCEEDED with a
  mid-probe latch); the FPGA reconfig exit and the probe-failure
  recovery path are gated as well.

- Reference series for the HWC lifecycle model:
  https://lore.kernel.org/netdev/20260813174243.3044348-1-longli@microsoft.com
---
 .../net/ethernet/microsoft/mana/gdma_main.c   | 178 +++++++++++++-----
 drivers/net/ethernet/microsoft/mana/mana_en.c |  11 +-
 include/net/mana/gdma.h                       |  14 +-
 3 files changed, 150 insertions(+), 53 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index f92b2d0..28544f0 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -678,10 +678,38 @@ out:
 	pci_unlock_rescan_remove();
 }
 
-static void mana_serv_fpga(struct pci_dev *pdev)
+/* Retire one service cycle: GC_IN_SERVICE is the last state published
+ * by the work, so remove() and the probe unwind may free gc once it
+ * is clear.  Must run before the exit paths take pci_lock_rescan_remove():
+ * removal can be waiting on the bit while holding that lock.
+ */
+static void mana_service_done(struct gdma_context *gc)
+{
+	/* Pairs with test_bit_acquire() on the wait side. */
+	clear_bit_unlock(GC_IN_SERVICE, &gc->flags);
+	/* wake_up_var() requires a full barrier between the update of the
+	 * waited-on variable and the wake.
+	 */
+	smp_mb__after_atomic();
+	wake_up_var(&gc->flags);
+}
+
+/* Retire a cycle whose exit removes the device: close admission
+ * first so no new cycle can be admitted in the retire window.
+ */
+static void mana_service_done_removing(struct gdma_context *gc)
+{
+	set_bit(GC_REMOVING, &gc->flags);
+	mana_service_done(gc);
+}
+
+static void mana_serv_fpga(struct pci_dev *pdev, struct gdma_context *gc)
 {
 	struct pci_bus *bus, *parent;
 
+	if (gc)
+		mana_service_done_removing(gc);
+
 	pci_lock_rescan_remove();
 
 	bus = pdev->bus;
@@ -706,9 +734,8 @@ out:
 	pci_unlock_rescan_remove();
 }
 
-static void mana_serv_reset(struct pci_dev *pdev)
+static void mana_serv_reset(struct pci_dev *pdev, struct gdma_context *gc)
 {
-	struct gdma_context *gc = pci_get_drvdata(pdev);
 	struct hw_channel_context *hwc;
 	int ret;
 
@@ -738,6 +765,7 @@ static void mana_serv_reset(struct pci_dev *pdev)
 	if (ret == -ETIMEDOUT || ret == -EPROTO) {
 		/* Perform PCI rescan on device if we failed on HWC */
 		dev_err(&pdev->dev, "MANA service: resume failed, rescanning\n");
+		mana_service_done_removing(gc);
 		mana_serv_rescan(pdev);
 		return;
 	}
@@ -748,22 +776,25 @@ static void mana_serv_reset(struct pci_dev *pdev)
 		dev_info(&pdev->dev, "MANA reset cycle completed\n");
 
 out:
-	clear_bit(GC_IN_SERVICE, &gc->flags);
+	mana_service_done(gc);
 }
 
-static void mana_do_service(enum gdma_eqe_type type, struct pci_dev *pdev)
+static void mana_do_service(enum gdma_eqe_type type, struct pci_dev *pdev,
+			    struct gdma_context *gc)
 {
 	switch (type) {
 	case GDMA_EQE_HWC_FPGA_RECONFIG:
-		mana_serv_fpga(pdev);
+		mana_serv_fpga(pdev, gc);
 		break;
 
 	case GDMA_EQE_HWC_RESET_REQUEST:
-		mana_serv_reset(pdev);
+		mana_serv_reset(pdev, gc);
 		break;
 
 	default:
 		dev_err(&pdev->dev, "MANA service: unknown type %d\n", type);
+		if (gc)
+			mana_service_done(gc);
 		break;
 	}
 }
@@ -779,12 +810,26 @@ static void mana_recovery_delayed_func(struct work_struct *w)
 	spin_lock_irqsave(&work->lock, flags);
 
 	while (!list_empty(&work->dev_list)) {
+		struct gdma_context *gc;
+
 		dev = list_first_entry(&work->dev_list,
 				       struct mana_dev_recovery, list);
 		list_del(&dev->list);
 		spin_unlock_irqrestore(&work->lock, flags);
 
-		mana_do_service(dev->type, dev->pdev);
+		/* Serialize the drvdata lookup and admission against
+		 * probe/remove.  Do not call sleeping functions while
+		 * holding the device lock.
+		 */
+		device_lock(&dev->pdev->dev);
+		gc = pci_get_drvdata(dev->pdev);
+		if (gc)
+			mana_schedule_serv_work(gc, dev->type);
+		device_unlock(&dev->pdev->dev);
+
+		if (!gc)
+			mana_do_service(dev->type, dev->pdev, NULL);
+
 		pci_dev_put(dev->pdev);
 		kfree(dev);
 
@@ -796,48 +841,49 @@ static void mana_recovery_delayed_func(struct work_struct *w)
 
 static void mana_serv_func(struct work_struct *w)
 {
-	struct mana_serv_work *mns_wk;
-	struct pci_dev *pdev;
-
-	mns_wk = container_of(w, struct mana_serv_work, serv_work);
-	pdev = mns_wk->pdev;
+	struct gdma_context *gc = container_of(w, struct gdma_context, serv_work);
+	struct pci_dev *pdev = to_pci_dev(gc->dev);
 
-	if (pdev)
-		mana_do_service(mns_wk->type, pdev);
+	mana_do_service(gc->serv_type, pdev, gc);
 
+	/* The rescan exits of mana_do_service() remove the device, which
+	 * frees gc before returning.  Only touch the pdev and the module
+	 * reference from here on; both are held until this point drops them.
+	 */
 	pci_dev_put(pdev);
-	kfree(mns_wk);
 	module_put(THIS_MODULE);
 }
 
 int mana_schedule_serv_work(struct gdma_context *gc, enum gdma_eqe_type type)
 {
-	struct mana_serv_work *mns_wk;
-
 	if (test_and_set_bit(GC_IN_SERVICE, &gc->flags)) {
 		dev_info(gc->dev, "Already in service\n");
 		return -EBUSY;
 	}
 
+	/* Pairs with set_bit(GC_REMOVING) + smp_mb__after_atomic() in
+	 * mana_gd_remove(): the test_and_set_bit() above is fully ordered,
+	 * so either this read sees GC_REMOVING and the cycle aborts, or
+	 * remove() observes GC_IN_SERVICE and waits for the cycle to retire
+	 * before it frees gc.
+	 */
+	if (test_bit(GC_REMOVING, &gc->flags)) {
+		dev_info(gc->dev, "Device is being removed\n");
+		mana_service_done(gc);
+		return -EBUSY;
+	}
+
 	if (!try_module_get(THIS_MODULE)) {
 		dev_info(gc->dev, "Module is unloading\n");
-		clear_bit(GC_IN_SERVICE, &gc->flags);
+		mana_service_done(gc);
 		return -ENODEV;
 	}
 
-	mns_wk = kzalloc(sizeof(*mns_wk), GFP_ATOMIC);
-	if (!mns_wk) {
-		module_put(THIS_MODULE);
-		clear_bit(GC_IN_SERVICE, &gc->flags);
-		return -ENOMEM;
-	}
-
 	dev_info(gc->dev, "Start MANA service type:%d\n", type);
-	mns_wk->pdev = to_pci_dev(gc->dev);
-	mns_wk->type = type;
-	pci_dev_get(mns_wk->pdev);
-	INIT_WORK(&mns_wk->serv_work, mana_serv_func);
-	schedule_work(&mns_wk->serv_work);
+
+	gc->serv_type = type;
+	pci_dev_get(to_pci_dev(gc->dev));
+	queue_work(system_wq, &gc->serv_work);
 	return 0;
 }
 
@@ -957,14 +1003,20 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
 	case GDMA_EQE_HWC_RESET_REQUEST:
 		dev_info(gc->dev, "Recv MANA service type:%d\n", type);
 
-		if (!test_and_set_bit(GC_PROBE_SUCCEEDED, &gc->flags)) {
+		if (!test_bit(GC_PROBE_SUCCEEDED, &gc->flags)) {
 			/*
-			 * Device is in probe and we received a hardware reset
-			 * event, the probe function will detect that the flag
-			 * has changed and perform service procedure.
+			 * Probe not completed: latch the event and let the
+			 * probe roll back, the recovery path will rescan.
+			 * Never admit service work before probe success;
+			 * the success re-check below preserves an event
+			 * racing probe completion.
 			 */
-			dev_info(gc->dev,
-				 "Service is to be processed in probe\n");
+			if (!test_and_set_bit(GC_SERVICE_DURING_PROBE,
+					      &gc->flags))
+				dev_info(gc->dev,
+					 "Service is to be processed in probe\n");
+			else if (test_bit(GC_PROBE_SUCCEEDED, &gc->flags))
+				mana_schedule_serv_work(gc, type);
 			break;
 		}
 		mana_schedule_serv_work(gc, type);
@@ -2546,6 +2598,7 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	gc->bar0_va = bar0_va;
 	gc->dev = &pdev->dev;
+	INIT_WORK(&gc->serv_work, mana_serv_func);
 	xa_init(&gc->irq_contexts);
 
 	err = mana_gd_setup(pdev);
@@ -2558,22 +2611,37 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	err = mana_rdma_probe(&gc->mana_ib);
 	if (err)
-		goto cleanup_mana;
+		goto service_quiesce;
 
 	/*
 	 * If a hardware reset event has occurred over HWC during probe,
-	 * rollback and perform hardware reset procedure.
+	 * rollback and perform hardware reset procedure.  Storing the
+	 * success bit before the latch check pairs with the handler's
+	 * latch-then-recheck, so an event racing probe completion is
+	 * admitted rather than lost.
 	 */
-	if (test_and_set_bit(GC_PROBE_SUCCEEDED, &gc->flags)) {
+	set_bit(GC_PROBE_SUCCEEDED, &gc->flags);
+	if (test_and_set_bit(GC_SERVICE_DURING_PROBE, &gc->flags)) {
 		err = -EPROTO;
-		goto cleanup_mana_rdma;
+		goto service_quiesce;
 	}
 
 	return 0;
 
-cleanup_mana_rdma:
+service_quiesce:
+	/* The stats work can admit a service cycle once mana_probe()
+	 * has run: close admission and retire an in-flight cycle
+	 * before any teardown, like mana_gd_remove() does.  Earlier
+	 * failure points cannot have admitted service work.
+	 */
+	set_bit(GC_REMOVING, &gc->flags);
+	/* Pairs with the ordered test_and_set_bit(GC_IN_SERVICE) in
+	 * mana_schedule_serv_work().
+	 */
+	smp_mb__after_atomic();
+	wait_var_event(&gc->flags,
+		       !test_bit_acquire(GC_IN_SERVICE, &gc->flags));
 	mana_rdma_remove(&gc->mana_ib);
-cleanup_mana:
 	mana_remove(&gc->mana, false);
 cleanup_gd:
 	mana_gd_cleanup_device(pdev);
@@ -2581,6 +2649,14 @@ unmap_bar:
 	xa_destroy(&gc->irq_contexts);
 	pci_iounmap(pdev, bar0_va);
 free_gc:
+	/* Backstop: drain before every vfree(). */
+	set_bit(GC_REMOVING, &gc->flags);
+	/* Pairs with the ordered test_and_set_bit(GC_IN_SERVICE) in
+	 * mana_schedule_serv_work().
+	 */
+	smp_mb__after_atomic();
+	wait_var_event(&gc->flags,
+		       !test_bit_acquire(GC_IN_SERVICE, &gc->flags));
 	pci_set_drvdata(pdev, NULL);
 	vfree(gc);
 release_region:
@@ -2624,6 +2700,20 @@ static void mana_gd_remove(struct pci_dev *pdev)
 {
 	struct gdma_context *gc = pci_get_drvdata(pdev);
 
+	/* Close admission and retire an in-flight cycle before any
+	 * teardown: the service work is the only user of gc that
+	 * remove() does not otherwise synchronise with.  The service
+	 * exits retire before taking the PCI rescan/remove lock, so
+	 * this wait cannot deadlock against them.
+	 */
+	set_bit(GC_REMOVING, &gc->flags);
+	/* Pairs with the ordered test_and_set_bit(GC_IN_SERVICE) in
+	 * mana_schedule_serv_work().
+	 */
+	smp_mb__after_atomic();
+	wait_var_event(&gc->flags,
+		       !test_bit_acquire(GC_IN_SERVICE, &gc->flags));
+
 	pci_disable_sriov(pdev);
 
 	mana_rdma_remove(&gc->mana_ib);
@@ -2635,6 +2725,8 @@ static void mana_gd_remove(struct pci_dev *pdev)
 
 	pci_iounmap(pdev, gc->bar0_va);
 
+	/* Prevent late recovery work from using freed gc. */
+	pci_set_drvdata(pdev, NULL);
 	vfree(gc);
 
 	pci_release_regions(pdev);
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 7a1ac85..67c7e7e 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -924,8 +924,8 @@ static void mana_tx_timeout(struct net_device *netdev, unsigned int txqueue)
 		return;
 	}
 
-	/* Already in service, hence tx queue reset is not required.*/
-	if (test_bit(GC_IN_SERVICE, &gc->flags))
+	if (test_bit(GC_IN_SERVICE, &gc->flags) ||
+	    test_bit(GC_REMOVING, &gc->flags))
 		return;
 
 	/* Note: If there are pending queue reset work for this port(apc),
@@ -4056,9 +4056,12 @@ static void mana_gf_stats_work_handler(struct work_struct *work)
 		dev_warn(gc->dev,
 			 "Gf stats wk handler: gf stats query timed out.\n");
 		/* As HWC timed out, indicating a faulty HW state and needs a
-		 * reset.
+		 * reset.  Never admit service work before the probe has
+		 * completed: a probe that is failing unwinds netdevs and the
+		 * HWC channel itself and cannot drain a cycle.
 		 */
-		mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST);
+		if (test_bit(GC_PROBE_SUCCEEDED, &gc->flags))
+			mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST);
 		return;
 	}
 	schedule_delayed_work(&ac->gf_stats_work, MANA_GF_STATS_PERIOD);
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 308950f..fd4b967 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -228,12 +228,6 @@ enum gdma_page_type {
 
 #define GDMA_INVALID_DMA_REGION 0
 
-struct mana_serv_work {
-	struct work_struct serv_work;
-	struct pci_dev *pdev;
-	enum gdma_eqe_type type;
-};
-
 struct gdma_mem_info {
 	struct device *dev;
 
@@ -420,6 +414,8 @@ struct gdma_irq_context {
 enum gdma_context_flags {
 	GC_PROBE_SUCCEEDED	= 0,
 	GC_IN_SERVICE		= 1,
+	GC_REMOVING		= 2,
+	GC_SERVICE_DURING_PROBE	= 3,
 };
 
 struct gdma_context {
@@ -479,6 +475,12 @@ struct gdma_context {
 
 	struct workqueue_struct *service_wq;
 
+	/* The in-flight MANA service cycle, queued on the system workqueue:
+	 * a reset cycle destroys and re-creates @service_wq.
+	 */
+	struct work_struct	serv_work;
+	enum gdma_eqe_type	serv_type;
+
 	unsigned long		flags;
 
 	/* Protect access to GIC context */

Re: [PATCH net v3] net: mana: fix reset work race with device removal

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 1 (1) · Medium: 1 · Low: 1

- [High] GC_REMOVING is a one-way latch that is set on service exits
  which do not necessarily remove the device.
- [Medium] The new `if (test_bit(GC_PROBE_SUCCEEDED, &gc->flags))` gate
  in `mana_gf_stats_work_handler()` (mana_en.c:4068) silently discards…
- [Low] The new service-work submission uses `queue_work(system_wq,
  &gc->serv_work)` (gdma_main.c:886).

Pre-existing issues:
- [High] `mana_gd_shutdown()` (gdma_main.c:2776) tears down the HWC —
  `mana_gd_cleanup_device()` -> `mana_hwc_destroy_channel()` ->…

--

net: mana: fix reset work race with device removal

The service work is moved into struct gdma_context and GC_IN_SERVICE now
describes whether the work may still touch gc.  Removal and the probe
unwind set a new GC_REMOVING bit to close admission and then wait for an
in-flight cycle to retire before clearing drvdata and freeing gc.  Reset
events observed during probe are latched in GC_SERVICE_DURING_PROBE
instead of being admitted immediately.

No Link: tags in the commit header.  Earlier postings referenced in the
changelog:
  https://lore.kernel.org/netdev/20260905023602.425827-1-fanwu01@zju.edu.cn
  https://lore.kernel.org/netdev/20260805143812.220509-1-fanwu01@zju.edu.cn

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index f92b2d0bf926..28544f00e8ce 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -678,10 +678,38 @@ static void mana_serv_rescan(struct pci_dev *pdev)
>  	pci_unlock_rescan_remove();
>  }
>  
> -static void mana_serv_fpga(struct pci_dev *pdev)
> +/* Retire one service cycle: GC_IN_SERVICE is the last state published
> + * by the work, so remove() and the probe unwind may free gc once it
> + * is clear.  Must run before the exit paths take pci_lock_rescan_remove():
> + * removal can be waiting on the bit while holding that lock.
> + */
> +static void mana_service_done(struct gdma_context *gc)
> +{
> +	/* Pairs with test_bit_acquire() on the wait side. */
> +	clear_bit_unlock(GC_IN_SERVICE, &gc->flags);
> +	/* wake_up_var() requires a full barrier between the update of the
> +	 * waited-on variable and the wake.
> +	 */
> +	smp_mb__after_atomic();
> +	wake_up_var(&gc->flags);
> +}
> +
> +/* Retire a cycle whose exit removes the device: close admission
> + * first so no new cycle can be admitted in the retire window.
> + */
> +static void mana_service_done_removing(struct gdma_context *gc)
> +{
> +	set_bit(GC_REMOVING, &gc->flags);
> +	mana_service_done(gc);
> +}
> +
> +static void mana_serv_fpga(struct pci_dev *pdev, struct gdma_context *gc)
>  {
>  	struct pci_bus *bus, *parent;
>  
> +	if (gc)
> +		mana_service_done_removing(gc);
> +
>  	pci_lock_rescan_remove();
>  
>  	bus = pdev->bus;
> @@ -706,9 +734,8 @@ static void mana_serv_fpga(struct pci_dev *pdev)
>  	pci_unlock_rescan_remove();
>  }
>  
> -static void mana_serv_reset(struct pci_dev *pdev)
> +static void mana_serv_reset(struct pci_dev *pdev, struct gdma_context *gc)
>  {
> -	struct gdma_context *gc = pci_get_drvdata(pdev);
>  	struct hw_channel_context *hwc;
>  	int ret;
>  
> @@ -738,6 +765,7 @@ static void mana_serv_reset(struct pci_dev *pdev)
>  	if (ret == -ETIMEDOUT || ret == -EPROTO) {
>  		/* Perform PCI rescan on device if we failed on HWC */
>  		dev_err(&pdev->dev, "MANA service: resume failed, rescanning\n");
> +		mana_service_done_removing(gc);
>  		mana_serv_rescan(pdev);
>  		return;
>  	}

[Severity: High]
Is GC_REMOVING guaranteed to be followed by an actual removal here?  Both
callers of mana_service_done_removing() can return with the device still
bound.

In mana_serv_fpga(), the bit is set before the bus checks:

	if (gc)
		mana_service_done_removing(gc);

	pci_lock_rescan_remove();

	bus = pdev->bus;
	if (!bus) {
		dev_err(&pdev->dev, "MANA service: no bus\n");
		goto out;
	}

	parent = bus->parent;
	if (!parent) {
		dev_err(&pdev->dev, "MANA service: no parent bus\n");
		goto out;
	}

bus->parent is NULL whenever the MANA function sits on a root bus, so
this exits through "goto out" without calling
pci_stop_and_remove_bus_device().

The failed-resume path has the same shape: mana_serv_rescan() has its own
"if (!parent)" bail-out that returns without removing the device, after
GC_REMOVING has already been latched.

I can't find any code that clears GC_REMOVING again.  Once it is set on a
live gc, mana_schedule_serv_work() refuses every subsequent request:

	if (test_bit(GC_REMOVING, &gc->flags)) {
		dev_info(gc->dev, "Device is being removed\n");
		mana_service_done(gc);
		return -EBUSY;
	}

and mana_tx_timeout() in mana_en.c also stops queueing queue-reset work
for the rest of the binding:

	if (test_bit(GC_IN_SERVICE, &gc->flags) ||
	    test_bit(GC_REMOVING, &gc->flags))
		return;

Does this leave a still-bound device permanently without HWC reset
service and without TX queue reset?  Would it be better to set
GC_REMOVING only after the point where removal is known to happen, or to
clear it on the bail-out paths?

> @@ -796,48 +841,49 @@ static void mana_recovery_delayed_func(struct work_struct *w)

[ ... ]

>  int mana_schedule_serv_work(struct gdma_context *gc, enum gdma_eqe_type type)
>  {
> -	struct mana_serv_work *mns_wk;
> -
>  	if (test_and_set_bit(GC_IN_SERVICE, &gc->flags)) {
>  		dev_info(gc->dev, "Already in service\n");
>  		return -EBUSY;
>  	}
>  
> +	/* Pairs with set_bit(GC_REMOVING) + smp_mb__after_atomic() in
> +	 * mana_gd_remove(): the test_and_set_bit() above is fully ordered,
> +	 * so either this read sees GC_REMOVING and the cycle aborts, or
> +	 * remove() observes GC_IN_SERVICE and waits for the cycle to retire
> +	 * before it frees gc.
> +	 */
> +	if (test_bit(GC_REMOVING, &gc->flags)) {
> +		dev_info(gc->dev, "Device is being removed\n");
> +		mana_service_done(gc);
> +		return -EBUSY;
> +	}
> +
>  	if (!try_module_get(THIS_MODULE)) {
>  		dev_info(gc->dev, "Module is unloading\n");
> -		clear_bit(GC_IN_SERVICE, &gc->flags);
> +		mana_service_done(gc);
>  		return -ENODEV;
>  	}
>  
> -	mns_wk = kzalloc(sizeof(*mns_wk), GFP_ATOMIC);
> -	if (!mns_wk) {
> -		module_put(THIS_MODULE);
> -		clear_bit(GC_IN_SERVICE, &gc->flags);
> -		return -ENOMEM;
> -	}
> -
>  	dev_info(gc->dev, "Start MANA service type:%d\n", type);
> -	mns_wk->pdev = to_pci_dev(gc->dev);
> -	mns_wk->type = type;
> -	pci_dev_get(mns_wk->pdev);
> -	INIT_WORK(&mns_wk->serv_work, mana_serv_func);
> -	schedule_work(&mns_wk->serv_work);
> +
> +	gc->serv_type = type;
> +	pci_dev_get(to_pci_dev(gc->dev));
> +	queue_work(system_wq, &gc->serv_work);
>  	return 0;
>  }

[Severity: Low]
This isn't a bug, but was system_wq intended here rather than
system_percpu_wq?  In this tree the declaration is:

include/linux/workqueue.h:
	extern struct workqueue_struct *system_wq; /* use system_percpu_wq, this will be removed */

and system_wq is allocated with __WQ_DEPRECATED, so __queue_work() emits:

kernel/workqueue.c:__queue_work() {
	if (unlikely(wq->flags & __WQ_DEPRECATED))
		pr_warn_once("workqueue: work func %ps enqueued on deprecated workqueue. "
			"Use system_{percpu|dfl}_wq instead.\n",
			work->func);
}

The code being replaced used schedule_work(), which queues on
system_percpu_wq.  The stated reason for staying off gc->service_wq is
satisfied equally by schedule_work(&gc->serv_work).

[ ... ]

> @@ -2624,6 +2700,20 @@ static void mana_gd_remove(struct pci_dev *pdev)
>  {
>  	struct gdma_context *gc = pci_get_drvdata(pdev);
>  
> +	/* Close admission and retire an in-flight cycle before any
> +	 * teardown: the service work is the only user of gc that
> +	 * remove() does not otherwise synchronise with.  The service
> +	 * exits retire before taking the PCI rescan/remove lock, so
> +	 * this wait cannot deadlock against them.
> +	 */
> +	set_bit(GC_REMOVING, &gc->flags);
> +	/* Pairs with the ordered test_and_set_bit(GC_IN_SERVICE) in
> +	 * mana_schedule_serv_work().
> +	 */
> +	smp_mb__after_atomic();
> +	wait_var_event(&gc->flags,
> +		       !test_bit_acquire(GC_IN_SERVICE, &gc->flags));
> +
>  	pci_disable_sriov(pdev);
>  
>  	mana_rdma_remove(&gc->mana_ib);

[Severity: High]
This isn't a bug introduced by this patch, but should mana_gd_shutdown()
get the same admission-close and drain that is added here?  It tears the
HWC down with no synchronisation against an in-flight service cycle:

static void mana_gd_shutdown(struct pci_dev *pdev)
{
	struct gdma_context *gc = pci_get_drvdata(pdev);

	dev_info(&pdev->dev, "Shutdown was called\n");

	mana_rdma_remove(&gc->mana_ib);
	mana_remove(&gc->mana, true);

	mana_gd_cleanup_device(pdev);

	pci_disable_device(pdev);
}

Meanwhile mana_serv_reset() caches the HWC pointer and then sleeps for
MANA_SERVICE_PERIOD seconds across suspend/resume:

	hwc = gc->hwc.driver_data;
	...
	hwc->hwc_timeout = 0;
	...
	mana_gd_suspend(pdev, PMSG_SUSPEND);

	msleep(MANA_SERVICE_PERIOD * 1000);

	ret = mana_gd_resume(pdev);

mana_gd_cleanup_device() -> mana_hwc_destroy_channel() does kfree(hwc),
and both paths call it.  device_shutdown() takes only
device_lock(&pdev->dev), which the service work does not hold, so on
reboot or kexec during a reset cycle can this end up as a use-after-free
of hwc and a second kfree()/destroy_workqueue() on the same objects?

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 591fb4191d90..c297de8bb762 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -4061,9 +4061,12 @@ static void mana_gf_stats_work_handler(struct work_struct *work)
>  		dev_warn(gc->dev,
>  			 "Gf stats wk handler: gf stats query timed out.\n");
>  		/* As HWC timed out, indicating a faulty HW state and needs a
> -		 * reset.
> +		 * reset.  Never admit service work before the probe has
> +		 * completed: a probe that is failing unwinds netdevs and the
> +		 * HWC channel itself and cannot drain a cycle.
>  		 */
> -		mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST);
> +		if (test_bit(GC_PROBE_SUCCEEDED, &gc->flags))
> +			mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST);
>  		return;
>  	}
>  	schedule_delayed_work(&ac->gf_stats_work, MANA_GF_STATS_PERIOD);

[Severity: Medium]
What happens to the reset request when this gate is false but the probe
then succeeds?

mana_probe() arms this work before returning to mana_gd_probe():

	schedule_delayed_work(&ac->gf_stats_work, MANA_GF_STATS_PERIOD);

with MANA_GF_STATS_PERIOD being 2 * HZ, while mana_gd_probe() still has
mana_rdma_probe() to run before it publishes the flag:

	set_bit(GC_PROBE_SUCCEEDED, &gc->flags);
	if (test_and_set_bit(GC_SERVICE_DURING_PROBE, &gc->flags)) {

So a mana_query_gf_stats() timeout in that window takes the new branch,
drops the request, and the bare "return;" also skips the
schedule_delayed_work() re-arm below it.

The EQE handler in gdma_main.c latches and re-checks instead:

	if (!test_bit(GC_PROBE_SUCCEEDED, &gc->flags)) {
		if (!test_and_set_bit(GC_SERVICE_DURING_PROBE, &gc->flags))
			dev_info(gc->dev,
				 "Service is to be processed in probe\n");
		else if (test_bit(GC_PROBE_SUCCEEDED, &gc->flags))
			mana_schedule_serv_work(gc, type);
		break;
	}

Here there is neither a latch nor a re-check, and the plain test_bit()
also races the set_bit() at the probe boundary.  If the probe goes on to
succeed, the device is left with ac->hwc_timeout_occurred set, the stats
poll never re-armed, and no reset cycle requested, where the previous
code called mana_schedule_serv_work() unconditionally.  Should this call
site latch into GC_SERVICE_DURING_PROBE (or re-check after the boundary)
so the request is not lost?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909042529.652301-1-fanwu01%40zju.edu.cn