> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx> > 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.
