Re: [PATCH net-next v10 1/4] net: mana: prepare HWC ownership for safe reinitialization

[Wei Hu]

Thanks for the review.

Below are my responses, including the small follow-ups planned for v11.

> mana_hwc_establish_channel() passes &hwc->setup_active as submitted. What
> happens when the PF explicitly rejects ESTABLISH_HWC?

setup_active records that the PF may own the queues, not that ESTABLISH
succeeded. A nonzero status indicates failure, but I do not have a protocol
guarantee that a rejected ESTABLISH leaves no partially installed mappings.
Also, -EPROTO covers malformed replies as well as a reported failure.

I plan to leave this unchanged. Clearing ownership on that error would
allow freeing or reusing DMA without an acknowledged teardown or another
proven quiescence point. Distinguishing a validated rejection would only
justify that change if the PF guarantees transactional rollback.

> Is there anything that bounds this leak? On the PCI probe path, a failure
> with -ETIMEDOUT or -EPROTO frees gc via mana_gd_free_context() at free_gc.

The leak is real. Final disposal loses the allocation metadata, and it is
not bounded to one bundle across repeated fresh failed probes.

Retention is intentional when DESTROY_HWC has not been acknowledged:
freeing the allocations could expose reused pages to outstanding PF DMA.
I do not have a confirmed PF-side quiescence guarantee for these paths
merely from the calls that disable bus mastering.

I do not plan to add unconditional DMA freeing in this respin. A bounded
recovery or reclamation policy is a worthwhile separate improvement, but
it needs a verified quiescence contract. This is a safety trade-off, not a
claim that the allocation loss is harmless.

> What happens to a retained context across a device reset? Only an
> acknowledged DESTROY_HWC in mana_smc_teardown_hwc_locked() clears
> setup_active, but a reset discards the PF-side HWC state anyway.

The final premise is not established by the cited mana_serv_reset() path.
That path suspends, waits, and resumes; it does not itself execute or
confirm a hardware reset on every invocation. An all-ones aperture or the
service delay also does not acknowledge that all old PF mappings are gone.

I therefore plan to keep ownership tracking unchanged rather than clear
setup_active or switch reset_vf on this assumption. A confirmed reset that
invalidates those mappings could provide a reclamation point, but that
requires the corresponding PF/SMC reset contract.

> The commit is described as preparation for dynamic HWC sizing. Several of
> the changes, though, look like fixes for bugs that can be hit in the current
> driver:

Agreed that the existing failure-path and malformed-init bugs should be
identified explicitly. I will clarify that in the commit description and
include applicable, verified Fixes metadata. A stable backport needs the
dependency-complete fix subset, not an automatic backport of the whole
feature stack. I do not plan to restructure the series solely for this
Low finding.

> Two cases that used to WARN are now dropped silently: a completion for a CQ
> id at or above max_num_cqs, and an empty cq_table slot. The commit message
> does not mention this.

I will document the intended quiet drops and correct the stale WARN
reference in mana_destroy_txq(). That is a small description/comment
correction; I will not restore WARNs for these cases during teardown.

> Does this comment match the teardown order? In mana_hwc_destroy_channel()
> the table is withdrawn and freed before the HWC EQ is fenced:

The comment is misleading. Pointer withdrawal and the RCU grace period
protect the table before the later EQ fencing. I will correct the comment
to describe that ordering, without changing the teardown sequence.

> This is a pre-existing issue and was not introduced by this patch. Now that
> the RCU protection of cq_table is documented here, though, mana_ib does not
> seem to follow it.

The independent RDMA CQ callback publication/lifetime issue is
pre-existing. We will address it separately. The comment clarification
above is not a claim that the separate RDMA lifetime issue is fixed.

Regards,
Wei