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

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] mana_gf_stats_work_handler()
  (drivers/net/ethernet/microsoft/mana/mana_en.c:4056-4072) now drops…
- [Low] The comment added at the new service_quiesce label in
  mana_gd_probe() ('The stats work can admit service once mana_probe()…
- [Low] mana_schedule_serv_work() replaces schedule_work() with
  queue_work(system_wq, &gc->serv_work).

Pre-existing issues:
- [High] The new admission/quiesce protocol is applied to
  mana_gd_remove(), mana_gd_shutdown() and the probe unwind, but not to…

--

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?

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