On Fri, 2024-05-17 at 10:56 +0200, Alexandre Ratchov wrote:
> On Thu, May 16, 2024 at 09:31:01PM +0200, Mark Kettenis wrote:
> > 
> > You're on to something.
> > 
> > The main difference between legacy INTx interrupts and MSIs is that
> > the former are level-triggered, where as the latter are
> > edge-triggered.  Level-triggered interrupts are a bit more forgiving
> > in the sense that if you don't handle all interrupts, the interrupt
> > will just fire again.  But edge-triggered interrupts will only trigger
> > if the interrupt condition is properly cleared and a new interrupt
> > happens.
> > 
> > At first sight the driver seems to do the right thing for
> > edge-triggered interrupts.  The handler reads the INTSTS register and
> > then writes to it to clear the interrupt condition.  It is reasonable
> > to expect that if further interrupts are triggered, we will get a new
> > MSI message.  However, if you look at the spec:
> > 
> > https://www.intel.com/content/www/us/en/standards/high-definition-audio-specification.html
> > 
> > you'll see the INTSTS is a RO (read-only) registers; not a W1C
> > (write-one to clear) register.  So on spec-compliant hardware, this
> > write doesn't do anything!  Which means that any new interrupts coming
> > in while we're processing the initial ones will cause the interrupt to
> > remain asserted without a new MSI to happen.
> > 
> > So instead the interrupt handler has to continue to process interrupts
> > as long as bits for the enabled interrupts are set.  Only when none of
> > the bits are set can we return and be sure a new MSI to happen.
> > 
> > Below is a new diff that does this and gets rid of all the bogus
> > INTSTS writes.
> > 
> > This seems to work fine for me on Intel hardware.
> > 
> > 
> 
> Thank you for looking at this.
> 
> The "clear interrupt" logic you describe is already present in the
> rirb and stream handlers and seems correct. Despite writing on a RO
> register being wrong, there's no need to clear the interrupt source at
> the controller level.
> 
> Does it fix the AMD 17h/1xh problem?

It does on my hardware:
 19:0:6: AMD 17h/1xh HD Audio
        0x0000: Vendor ID: 1022, Product ID: 15e3
        0x0004: Command: 0006, Status: 0010
        0x0008: Class: 04 Multimedia, Subclass: 03 HD Audio,
                Interface: 00, Revision: 00
        0x000c: BIST: 00, Header Type: 80, Latency Timer: 00,
                Cache Line Size: 10
        0x0010: BAR mem 32bit addr: 0xfca00000/0x00008000
        0x0014: BAR empty (00000000)
        0x0018: BAR empty (00000000)
        0x001c: BAR empty (00000000)
        0x0020: BAR empty (00000000)
        0x0024: BAR empty (00000000)
        0x0028: Cardbus CIS: 00000000
        0x002c: Subsystem Vendor ID: 1043 Product ID: 886d
        0x0030: Expansion ROM Base Address: 00000000
        0x0038: 00000000
        0x003c: Interrupt Pin: 03 Line: ff Min Gnt: 00 Max Lat: 00
        0x0048: Capability 0x09: Vendor Specific
        0x0050: Capability 0x01: Power Management
                State: D0
        0x0064: Capability 0x10: PCI Express
                Max Payload Size: 256 / 256 bytes
                Max Read Request Size: 512 bytes
                Link Speed: 16.0 / 16.0 GT/s
                Link Width: x16 / x16
        0x0100: Enhanced Capability 0x0b: Vendor-Specific
        0x02a0: Enhanced Capability 0x0d: Access Control Services
        0x00a0: Capability 0x05: Message Signalled Interrupts (MSI)
                Enabled: yes; 1 vectors (1 enabled)