Re: [PATCH v5 3/3] iommu/arm-smmu-v3: Avoid redundant master lookup in events

[Nicolin Chen <nicolinc@nvidia.com>]

On Tue, Nov 12, 2024 at 08:30:18AM +0000, Pranjal Shrivastava wrote:
> Remove the call to `arm_smmu_find_master` in `arm_smmu_handle_evt`.
> 
> The call was primarily to retrieve `master->dev` ptr for reporting iommu
> device faults. Since this info is already available in the `event->dev`
> field, we no longer need to lookup the master to fetch the dev pointer.

We only protected the "dev" for dev_name() while the streams_mutex
is still required against arm_smmu_remove_master?

> @@ -1857,17 +1855,14 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu,
>                 flt->prm.pasid = event->ssid;
>         }
> 
> -       mutex_lock(&smmu->streams_mutex);
> -       master = arm_smmu_find_master(smmu, event->sid);
> -       if (!master) {
> -               ret = -EINVAL;
> -               goto out_unlock;
> -       }
> +       /*
> +        * If the master wasn't found while reading the event or
> +        * get_device() returned NULL, we shouldn't report a fault.
> +        */
> +       if (!event->dev)
> +               return -EINVAL;
> 
> -       ret = iommu_report_device_fault(master->dev, &fault_evt);
> -out_unlock:
> -       mutex_unlock(&smmu->streams_mutex);
> -       return ret;
> +       return iommu_report_device_fault(event->dev, &fault_evt);

Apart from the question that I asked above, it'd be convenient for
my vIRQ series to add on top if we can keep the master here:
https://github.com/nicolinc/iommufd/blob/66b3f9cdb39920880fb052b53f11ceb16af007fe/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c#L1782

Given that your flow is:
	arm_smmu_decode_event(...);		// get event->dev
	if (arm_smmu_handle_evt(...))		// validate event->dev
		arm_smmu_dump_event(...);	// use event->dev 

Could we just keep the driver as it is, so your flow will be:
	arm_smmu_decode_event(...);		// no event->dev
	if (arm_smmu_handle_evt(...))		// get event->dev 
		arm_smmu_dump_event(...);	// use event->dev
?

>  static void arm_smmu_dump_raw_event(struct arm_smmu_device *smmu, u64 *raw,
> @@ -1927,6 +1922,7 @@ static void arm_smmu_dump_event(struct arm_smmu_device *smmu, u64 *raw,
>         }
>  }
> 
> +
>  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)

Should drop this.

Thanks
Nicolin