Re: [PATCH v2 3/8] iommu/amd: Add debugfs support to dump IOMMU Capability registers

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Jan 07, 2025 at 11:33:16AM +0530, Srivastava, Dheeraj Kumar wrote:
> On 11/27/2024 2:29 AM, Bjorn Helgaas wrote:
> > On Wed, Nov 06, 2024 at 01:16:34PM +0530, Dheeraj Kumar Srivastava wrote:
> > > IOMMU Capability registers defines capabilities of IOMMU and information
> > > needed for initialising MMIO registers and device table. This is useful
> > > to dump these registers for debugging IOMMU related issues.
> > > 
> > > e.g.To get capability registers value for iommu<x>
> > >    # echo "0x10" > /sys/kernel/debug/iommu/amd/iommu00/capability
> > >    # cat /sys/kernel/debug/iommu/amd/iommu00/capability_dump
> > 
> > Same comment here.  Why does this need to be so complicated to use?
> > Can't you make a single read-only file that contains all the registers
> > of interest?
> 
> Please do let me know your concerns and views on my comments in the
> previous patch.
> 
> With the implemented approach we do need separate files for mmio
> registers and capability registers input/output files as to
> understand if user input is mmio's offset or capability register's
> offset.

My comment is not about the difference between MMIO and config space
registers.  My concern is using two separate files to read the same
register.  That's inherently racy:

  UserA# echo "0x10" > /sys/kernel/debug/iommu/amd/iommu00/capability
  UserB# echo "0x20" > /sys/kernel/debug/iommu/amd/iommu00/capability
  UserA# cat /sys/kernel/debug/iommu/amd/iommu00/capability_dump

UserA expected to see the register at 0x10, but sees the one at 0x20
instead.

I think there's value in using a strategy similar to other IOMMU
drivers, e.g., intel.  But I'm not an IOMMU maintainer, so I'm just
kibbitzing here, and maybe your strategy is better.

Bjorn