Re: [PATCH v1 2/2] iommu/arm-smmu-v3: Recover ATC invalidate timeouts

[Nicolin Chen]

On Tue, Mar 10, 2026 at 07:16:02PM +0000, Pranjal Shrivastava wrote:
> On Wed, Mar 04, 2026 at 09:21:42PM -0800, Nicolin Chen wrote:
> > +	/*
> > +	 * ATC timeout indicates the device has stopped responding to coherence
> > +	 * protocol requests. The only safe recovery is a reset to flush stale
> > +	 * cached translations. Note that pci_reset_function() internally calls
> > +	 * pci_dev_reset_iommu_prepare/done() as well and ensures to block ATS
> > +	 * if PCI-level reset fails.
> > +	 */
> > +	if (!pci_reset_function(pdev)) {
> 
> I'm a little uncomfortable with this, why is an IOMMU driver poking into
> the PCI mechanics? I agree that a reset might be the right thing to do
> here but we wouldn't want the IOMMU driver to trigger it.. Ideally, we'd
> need a mechanism that bubbles up fatal IOMMU faults to the PCI core and
> let it decide/perform the reset. Maybe this could mean adding another op
> to struct pci_error_handlers or something like that?

Robin/Jason already had similar remarks (to most of your other
comments as well). I have acked their comments, and am already
reworking on these.

> > +		/*
> > +		 * If reset succeeds, set BME back. Otherwise, fence the system
> > +		 * from a faulty device, in which case user will have to replug
> > +		 * the device to invoke pci_set_master().
> > +		 */
> > +		pci_dev_lock(pdev);
> 
> Why are we using spinlock_irqsave across the worker? Also, why does
> atc_recovery.lock have to be a spinlock? The workers run in process
> context, and I also don't see anyone else take the atc_recovery.lock?

I guess mutex would be okay here, since there is no other place
access the linked list. Pairing a linked list with a spinlock is
just a common practice..

> Why does it need to be irq-safe? If this can somehow run in irq context,
> we also seem to be using pci_dev_lock and streams_mutex across the
> worker?

pci_dev_lock was to fence race on the PCI level. Yet, the entire
BME call is probably not a good idea. So, dropping that means we
won't need pci_dev_lock.

> Mixing mutexes with spinlocks is brittle and invites 
> "sleep-while-atomic" bugs in future refactors..

Either streams_mutex or atc_recovery.lock was scoped for only a
few lines each section. Each was released before the other one
was taken. Where is the "mixing" or "sleep-while-atomic" case?

Nicolin