On Fri, Dec 22, 2023 at 09:56:39AM +0800, Ethan Zhao wrote: > I don't know if the polling along sleeping for completion of meanningless > devTLB invalidation request blindly sent to (removed/powered down/link down) > device makes sense or not. If you have a way to get to the struct pci_dev * which you're waiting for in qi_submit_sync() then I guess you could check for its presence and bail out if it's gone, instead of issuing a cpu_relax(). > > Again, the proposed patch is not a proper solution. It will paper over > > the issue most of the time but every once in a while someone will still > > get a hard lockup splat and it will then be more difficult to reproduce > > and fix if the proposed patch is accepted. > > Could you point out why is not proper ? Is there any other window > the hard lockup still could happen with the ATS capable devcie > supprise_removal case if we checked the connection state first ? > Please help to elaberate it. Even though user space may have initiated orderly removal via sysfs, the device may be yanked from the slot (surprise removed) while the orderly removal is happening. > Yes, this is the old kernel stack trace, but customer also tried lasted > 6.7rc4 If you could provide a stacktrace for a contemporary kernel, I think that would be preferred. > (doesn't work) and the patched 6.7rc4 (fixed). Why is it fixed in v6.7-rc4? Is the present patch thus unnecessary? > > Finally, it is common to adhere to terms > > used in the PCIe Base Spec in commit messages, so "ATC Invalidate Request" > > might be preferable to "devTLB flush request". > > ATS Invalidate Request ? devTLB flush request has the same meaning, > > I thought all iommu/PCIe guys could understand. I'm just pointing out the preferred way to write commit messages in the PCI subsystem (as I've perceived it over the years) so that you can reduce the number of iterations you have to go through due to maintainer feedback. I'm just trying to be helpful. > How to define the point "some" msec to timeout while software > break out the waiting loop ? or polling if the target is gone ? I'd say adhere to the 1 min + 50% number provided in the spec. If you know the device is gone before that then you can break out of the loop in qi_submit_sync() of course. The question is, does the Intel IOMMU have a timeout at all for Invalidate Requests? I guess we don't really know that because in the stack trace you've provided, the watchdog stops the machine before a timeout occurs. So it's at least 12 sec. Or there's no timeout at all. If the Intel IOMMU doesn't enforce a timeout, you should probably amend qi_submit_sync() to break out of the loop once the 1 min + 50% limit is exceeded. And you need to amend the function to sleep instead of polling in interrupt context. Can you check with hardware engineers whether there's a timeout? Thanks, Lukas
