Re: [PATCH v1 1/1] PCI: Enable INTx quirk for ATI PCIe-USB adapter

[Bjorn Helgaas]

[+cc Alex]

On Wed, Mar 16, 2022 at 06:12:19PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 16, 2022 at 06:52:09AM -0500, Bjorn Helgaas wrote:
> > On Wed, Mar 16, 2022 at 12:27:57PM +0200, Andy Shevchenko wrote:
> > > On Tue, Mar 15, 2022 at 03:22:31PM -0500, Bjorn Helgaas wrote:
> > > > On Tue, Mar 15, 2022 at 12:09:08PM +0200, Andy Shevchenko wrote:
> > > > > On Mon, Mar 14, 2022 at 02:42:53PM -0500, Bjorn Helgaas wrote:
> > > > > > On Mon, Mar 14, 2022 at 12:14:48PM +0200, Andy Shevchenko wrote:
> > > > > > > ATI PCIe-USB adapter advertises MSI, but it doesn't work
> > > > > > > if INTx is disabled.  Enable the respective quirk as
> > > > > > > it's done for other ATI devices on this chipset,
> > > > > > > 
> > > > > > > Fixes: 306c54d0edb6 ("usb: hcd: Try MSI interrupts on
> > > > > > > PCI devices")

> > > > Anyway, I applied this to pci/msi for v5.18 with the following
> > > > commit log:
> > > > 
> > > >     PCI: Disable broken MSI on ATI SB600 USB adapters
> > > > 
> > > >     Some ATI SB600 USB adapters advertise MSI, but MSI doesn't
> > > >     work if INTx is disabled.  Disable MSI on these adapters.
> > > 
> > > But IIUC MSI is _not_ disabled. That's why I have issued this
> > > version of the patch with different commit message. Did I
> > > misunderstand something?
> > 
> > Oh, right, of course.  Sorry, I was asleep at the wheel.
> 
> Are you going to fix that?

Yes, of course, I'll do something with the commit message after we
figure out how to handle PCI_COMMAND_INTX_DISABLE.

> > I guess it's just that for these devices, we don't disable INTx
> > when enabling MSI.  I can't remember why we disable INTx when
> > enabling MSI, but it raises the question of whether it's better to
> > leave INTx enabled or to just disable use of MSI completely.
> 
> It's required by specification to disable INTx if I read 6.1.4.3
> Enabling Operation correctly.

Thanks for the reference; I was looking for something like that.  But
I don't think this section requires us to set
PCI_COMMAND_INTX_DISABLE.  For the benefit of folks without the spec,
PCIe r6.0, sec 6.1.4.3 says:

  To maintain backward compatibility, the MSI Enable bit in the
  Message Control Register for MSI and the MSI-X Enable bit in the
  Message Control Register for MSI-X are each Clear by default (MSI
  and MSI-X are both disabled). System configuration software Sets one
  of these bits to enable either MSI or MSI-X, but never both
  simultaneously. Behavior is undefined if both MSI and MSI-X are
  enabled simultaneously. Software disabling either mechanism during
  active operation may result in the Function dropping pending
  interrupt conditions or failing to recognize new interrupt
  conditions. While enabled for MSI or MSI-X operation, a Function is
  prohibited from using INTx interrupts (if implemented) to request
  service (MSI, MSI-X, and INTx are mutually exclusive).

The only *software* constraints I see are (1) software must never
enable both MSI and MSI-X simultaneously, and (2) if software disables
MSI or MSI-X during active operation, the Function may fail to
generate an interrupt when it should.

I read the last sentence as a constraint on the *hardware*: if either
MSI or MSI-X is enabled, the Function is not allowed to use INTx,
regardless of the state of PCI_COMMAND_INTX_DISABLE.

I searched the spec for "Interrupt Disable", looking for situations
where software might be *required* to set it, but I didn't see
anything.

I suspect "Interrupt Disable" was intended to help the OS stop all
activity from a device during hot-plug or reconfiguration, as hinted
at in sec 6.4, "Device Synchronization":

  The ability of the driver and/or system software to block new
  Requests from the device is supported by the Bus Master Enable,
  SERR# Enable, and Interrupt Disable bits in the Command register
  (Section 7.5.1.1.3) of each device Function, and other such control
  bits.

So I'm trying to figure out why when enabling MSI we need to set
PCI_COMMAND_INTX_DISABLE for most devices, but it's safe to skip that
for these quirked devices.

Bjorn