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
