On 09/06/2021 10:27, Jan Beulich wrote: > It appears unhelpful to me for flush_command_buffer() to block all > progress elsewhere for the given IOMMU by holding its lock while > waiting for command completion. Unless the lock is already held, > acquire it in send_iommu_command(). Release it in all cases in > flush_command_buffer(), before actually starting the wait loop. > > Some of the involved functions did/do get called with the lock already > held: For amd_iommu_flush_intremap() we can simply move the locking > inside. For amd_iommu_flush_device() and amd_iommu_flush_all_caches() > the lock now gets dropped in the course of the function's operation. > > Where touching function headers anyway, also adjust types used to be > better in line with our coding style and, where applicable, the > functions' callers. > > Signed-off-by: Jan Beulich <jbeul...@suse.com> > Reviewed-by: Paul Durrant <p...@xen.org>
Honestly, I'm -2 to this. It is horrible obfuscation of the logic. I agree with the premise of not holding the lock when we don't need to, but moving the lock/unlocks into different functions makes it impossible to follow. (Also, the static analysers are going to scream at this patch, and rightfully so IMO.) send_iommu_command() is static, as is flush_command_buffer(), so there is no need to split the locking like this AFAICT. Instead, each amd_iommu_flush_* external accessor knows exactly what it is doing, and whether a wait descriptor is wanted. flush_command_buffer() wants merging into send_iommu_command() as a "bool wait" parameter, at which point the locking and unlocking moves entirely into send_iommu_command() with no pointer games. ~Andrew
