Re: [PATCH v3 13/13] iommu/arm-smmu-v3: Enable PRI for PCI device in arm_smmu_probe_device()

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, Sep 03, 2026 at 12:18:33PM -0700, Jonathan Cameron wrote:
> > SMMUv3 forbids the Stall model on PCIe streams. Refuse to enable PRI on a
> > PCIe master that came with stall_enabled, so page_response() can dispatch
> > on master state unambiguously.
> 
> As earlier, we have exceptions in tree for Stall mode on PCIe streams
> (lets not reopen that fun arguement) so I'd focus this on PRI not
> making any sense if stall mode is in use as that has alternative
> handling for page faults.

Replaced with:

A stalling master resolves its faults synchronously, so it has no reason at
all to issue a page request. Refuse to enable PRI on such a master, so that
page_response() can keep dispatching on the master state unambiguously.

> > +	/* Same as the eventq drain above, for the hardware priq */
> > +	if (master->pri_enabled) {
> > +		ret |= arm_smmu_drain_queue(smmu, &smmu->priq.q, false);
> > +		/* Ensure pending requests have reached the IOPF queue */
> > +		if (!ret && smmu->priq.q.irq)
> 
> Similar to before, I'd factor out the if (!ret)
> Using |= on return values is never particularly nice though safe
> as used here.  Still I'd use another variable to store that things
> timed out already so we are skipping these.

OK. Replaced with a "bool timed_out" (in both PATCH-4 and here):

@@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
 	struct arm_smmu_master_domain *master_domain = state->old_master_domain;
 	struct arm_smmu_master *master = state->master;
 	struct arm_smmu_device *smmu = master->smmu;
-	int ret = 0;
+	bool timed_out = false;
 
 	lockdep_assert_not_held(&arm_smmu_asid_lock);
 	iommu_group_mutex_assert(master->dev);
@@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
 	 * against the next domain.
 	 */
 	if (master->stall_enabled) {
-		ret = arm_smmu_wait_for_queue_drained(smmu, &smmu->evtq.q, false);
+		timed_out = arm_smmu_wait_for_queue_drained(smmu, &smmu->evtq.q,
+							    false);
 		/*
 		 * Ensure pending events have reached the IOPF queue, unless
 		 * the drain timed out: a stuck consumer would also block an
 		 * unbounded wait_event() inside the synchronize_irq().
 		 */
-		if (!ret) {
+		if (!timed_out) {
 			if (smmu->evtq.q.irq)
 				synchronize_irq(smmu->evtq.q.irq);
 			/* Pending events might be in the combined_irq handler */
@@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
 
 	/* Same as the eventq drain above, for the hardware priq */
 	if (master->pri_enabled) {
-		ret |= arm_smmu_wait_for_queue_drained(smmu, &smmu->priq.q, false);
+		timed_out |= arm_smmu_wait_for_queue_drained(
+			smmu, &smmu->priq.q, false);
 		/* Ensure pending requests have reached the IOPF queue */
-		if (!ret) {
+		if (!timed_out) {
 			if (smmu->priq.q.irq)
 				synchronize_irq(smmu->priq.q.irq);
 			/* Pending requests might be in the combined_irq handler */
@@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
 		 * such work still references. Skip the iopf teardown and
 		 * leak master_domain, rather than risk a UAF.
 		 */
-		if (WARN_ON(ret)) {
+		if (WARN_ON(timed_out)) {
 			state->old_master_domain = NULL;
 			return;
 		}

> > +	if (!pci_pri_supported(pdev))
> > +		return;
> > +
> > +	/* SMMUv3 forbids the Stall model on PCIe streams */
> 
> Again, I'd tweak the wording given we have quite a few examples
> in tree that do stall mode on PCIe smelling streams.

Replaced with:
	/* A stalling master resolves its faults without any page request */

Thanks
Nicolin