Re: [PATCH RFC v1 1/2] iommu: Introduce iommu_dev_reset_prepare() and iommu_dev_reset_done()

[Nicolin Chen]

On Tue, Jun 10, 2025 at 12:26:07PM +0800, Baolu Lu wrote:
> On 6/10/25 02:45, Nicolin Chen wrote:
> > +	ops = dev_iommu_ops(dev);
> 
> Should this be protected by group->mutext?

Not seemingly, but should require the iommu_probe_device_lock I
think.

> > +	/*
> > +	 * group->mutex starts
> > +	 *
> > +	 * This has to hold the group mutex until the reset is done, to prevent
> > +	 * any RID or PASID domain attachment/replacement, which otherwise might
> > +	 * re-enable the ATS during the reset cycle.
> > +	 */
> > +	mutex_lock(&group->mutex);
> 
> Is it possible that group has been freed when it reaches here?

It doesn't look very obvious to me which lock we need here. But,
it's a good point that dev->iommu_group is unsafe here. Will dig
a bit later.

> > +void iommu_dev_reset_done(struct device *dev)
> > +{
> > +	struct iommu_group *group = dev->iommu_group;
> > +	const struct iommu_ops *ops;
> > +	unsigned long pasid;
> > +	void *entry;
> > +
> > +	/* Previously unlocked */
> > +	if (!dev_has_iommu(dev))
> > +		return;
> > +	ops = dev_iommu_ops(dev);
> > +	if (!ops->blocked_domain)
> > +		return;
> 
> Should it be a WARN_ON()? As proposed, reset_prepare and reset_done must
> be paired. So if reset_prepare returns failure, reset_done should not be
> called. Or not?

Yea, I agree. Should work like that.

> > +	/* group->mutex held in iommu_dev_reset_prepare() continues from here */
> > +	WARN_ON(!lockdep_is_held(&group->mutex));
> 
> Probably iommu_group_mutex_assert() and move it up?

Yes and not sure (will take another look).

> How about combining these two helpers? Something like,
> 
> int iommu_dev_block_dma_and_action(struct device *dev,
> 		int (*action)(struct pci_dev *dev))
> {
> 	prepare();
> 	action();
> 	done();
> }

That's an interesting idea! So, we wouldn't need to worry about
the pairing.

Thanks!
Nicolin