RE: [PATCH v4 05/13] iommu/vt-d: Prepare intel_iommu_set_dev_pasid() handle replacement

Tian, Kevin
2024-11-06T09:33:52Z

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, November 6, 2024 4:39 PM
>
> On 2024/11/6 16:02, Yi Liu wrote:
> >>> @@ -4329,24 +4368,17 @@ static int intel_iommu_set_dev_pasid(struct
> >>> iommu_domain *domain,
> >>>           ret = intel_pasid_setup_second_level(iommu, dmar_domain,
> >>>                                dev, pasid);
> >>>       if (ret)
> >>> -        goto out_unassign_tag;
> >>> +        goto out_remove_dev_pasid;
> >>>
> >>> -    dev_pasid->dev = dev;
> >>> -    dev_pasid->pasid = pasid;
> >>> -    spin_lock_irqsave(&dmar_domain->lock, flags);
> >>> -    list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
> >>> -    spin_unlock_irqrestore(&dmar_domain->lock, flags);
> >>> +    domain_remove_dev_pasid(old, dev, pasid);
> >>
> >> My preference is moving the check of non-NULL old out here.
> >
> > @Baolu, how about your thought?
>
> If we move the check out of this helper, there will be boilerplate code
> in multiple places. Something like,
>
> if (old)
> domain_remove_dev_pasid(old, dev, pasid);
>

yes, but the logic is slightly clearer to reflect a replace operation
in the caller side instead of pretending it to be a mandatory one.