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

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922070728.309280-1-fanwu01@zju.edu.cn?part=1
---
net: mana: fix reset work race with device removal

The service work item is moved into struct gdma_context and its lifecycle
is guarded by a new serv_lock plus a waitqueue, so removal, shutdown and
the probe unwind can close admission and wait for an in-flight reset cycle
before gc is freed.  The probe boundary is handled by latching reset events
that arrive while the probe is still running.

A few questions below about the new admission gates and the paths that were
not converted.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 8e9bfc1d6a2ae..b91d2542cfe56 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c

[ ... ]

> @@ -796,51 +899,94 @@ static void mana_recovery_delayed_func(struct work_struct *w)

[ ... ]

> +/* Serialize admission with retirement.  Called in hard IRQ context, so
> + * serv_lock is taken with interrupts disabled and nothing that may
> + * sleep runs under it.
> + */
>  int mana_schedule_serv_work(struct gdma_context *gc, enum gdma_eqe_type type)
>  {
> -	struct mana_serv_work *mns_wk;
> +	unsigned long flags;
> +	bool busy;

[ ... ]

>  	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]
Is system_wq the intended target here?  include/linux/workqueue.h carries:

    extern struct workqueue_struct *system_wq; /* use system_percpu_wq, this will be removed */

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

    workqueue: work func %ps enqueued on deprecated workqueue. Use system_{percpu|dfl}_wq instead.

The schedule_work() that was removed expanded to queue_work(system_percpu_wq,
...), so this hunk seems to newly pick the deprecated alias and a warning on
every service cycle.

Given that mana_serv_reset()/mana_serv_fpga() do msleep(MANA_SERVICE_PERIOD *
1000) and PCI stop/rescan, would system_long_wq (or system_dfl_long_wq) be a
better fit than the per-CPU queue?

[ ... ]

> @@ -2558,22 +2697,26 @@ 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.
>  	 */
> -	if (test_and_set_bit(GC_PROBE_SUCCEEDED, &gc->flags)) {
> +	rollback = mana_service_probe_complete(gc);
> +	if (rollback) {
>  		err = -EPROTO;
> -		goto cleanup_mana_rdma;
> +		goto service_quiesce;
>  	}
>  
>  	return 0;
>  
> -cleanup_mana_rdma:
> +service_quiesce:
> +	/* The stats work can admit service once mana_probe() has run:
> +	 * retire an in-flight cycle before any teardown.
> +	 */
> +	mana_service_quiesce(gc);

[Severity: Low]
Does this comment still describe the code after the change in mana_en.c?

The stats work handler now only admits service when
mana_service_probe_done(gc) is true, and serv_probe_done is set solely by
mana_service_probe_complete(), which runs after mana_rdma_probe() has
succeeded.  On the path that jumps here because mana_rdma_probe() failed,
serv_probe_done is still false, so the stats work cannot have admitted a
cycle at all, and the EQ producer is gated on the same flag.

Could the comment be reworded to match the gate, or is a producer intended
that can still admit during probe?

[ ... ]

> @@ -2687,6 +2837,9 @@ static void mana_gd_shutdown(struct pci_dev *pdev)
>  
>  	dev_info(&pdev->dev, "Shutdown was called\n");
>  
> +	/* Shutdown tears down the same HW paths as remove(). */
> +	mana_service_quiesce(gc);
> +
>  	mana_rdma_remove(&gc->mana_ib);
>  	mana_remove(&gc->mana, true);

[Severity: High]
This isn't a bug introduced by this patch, since the old GC_IN_SERVICE bit
was ignored by the PM callbacks too, but should mana_gd_suspend() and
mana_gd_resume() (installed as mana_driver.suspend/.resume) get the same
treatment as remove() and shutdown()?

An admitted cycle runs:

mana_serv_reset()
    mana_gd_suspend(pdev, PMSG_SUSPEND);
    msleep(MANA_SERVICE_PERIOD * 1000);
    ret = mana_gd_resume(pdev);

with serv_in_flight set for that whole 10+ second window, and the work sits
on system_wq, which is not WQ_FREEZABLE.  If the guest enters system suspend
or hibernate during that window, pci_pm_suspend() -> pci_legacy_suspend()
calls mana_gd_suspend() a second time on the same gc, and neither PM callback
consults serv_in_flight/serv_removing or calls mana_service_quiesce().

Both threads then reach mana_hwc_destroy_channel(), whose guard is not atomic
with the clearing store:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_destroy_channel() {
	struct hw_channel_context *hwc = gc->hwc.driver_data;

	if (!hwc)
		return;
	...
	kfree(hwc);
	gc->hwc.driver_data = NULL;
	gc->hwc.gdma_context = NULL;

	vfree(gc->cq_table);
	gc->cq_table = NULL;
}

mana_gd_cleanup_device() has the same shape around gc->service_wq:

	if (gc->service_wq) {
		destroy_workqueue(gc->service_wq);
		gc->service_wq = NULL;
	}

Can this interleaving double-free hwc and gc->cq_table, or call
destroy_workqueue() twice on gc->service_wq?  And can mana_gd_resume() from
the service cycle re-arm DMA/MMIO on a device the PM core has already
suspended?

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 591fb4191d90d..0eb23aea2e139 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -4060,10 +4060,13 @@ static void mana_gf_stats_work_handler(struct work_struct *work)
>  		memset(&ac->hc_stats, 0, sizeof(ac->hc_stats));
>  		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.
> +		/* As HWC timed out, indicating a faulty HW state and
> +		 * needs a 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 (mana_service_probe_done(gc))
> +			mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST);
>  		return;
>  	}
>  	schedule_delayed_work(&ac->gf_stats_work, MANA_GF_STATS_PERIOD);

[Severity: Medium]
When mana_service_probe_done(gc) is false here, is the reset request lost for
good?

mana_probe() arms this work with a MANA_GF_STATS_PERIOD delay, while
serv_probe_done is only set later, at the tail of mana_gd_probe():

	err = mana_rdma_probe(&gc->mana_ib);
	if (err)
		goto service_quiesce;
	...
	rollback = mana_service_probe_complete(gc);

So the handler can run with serv_probe_done still false if mana_rdma_probe()
(register device plus the synchronous mana_ib adev probe and its HWC
requests) takes longer than that delay.

In that window an -ETIMEDOUT from mana_query_gf_stats() takes the new branch,
no cycle is admitted, serv_during_probe is not set so
mana_service_probe_complete() reports no rollback, and the return also skips
the schedule_delayed_work() re-arm.  If the probe then succeeds, is there any
remaining path that retries?  The recovery list in mana_gd_probe() is only
populated when the probe itself fails with -ETIMEDOUT/-EPROTO, and
hwc_timeout_occurred only feeds a warning, so hc_stats would stay zeroed for
the life of the binding and a wedged HWC would never be reset.  Before this
patch the request was unconditional and the resulting reset cycle re-armed
the stats work through mana_probe(resuming=true).

The EQ producer solves the same race by latching under serv_lock:

	admit = gc->serv_probe_done;
	first = !gc->serv_during_probe;
	if (!admit)
		gc->serv_during_probe = true;

Would routing this producer through the same latch-or-admit helper work, or
alternatively keeping the delayed work re-armed so the request is retried
once serv_probe_done is set?