On Wed, 16 Mar 2022 16:15:48 -0500
Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> [+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.
I don't have an authoritative answer, but in the conventional PCI 2.3
spec the bit definitions for DisINTx and MSI Enable cross reference each
other from which one might infer a symmetry that we disable one to use
the other. In PCIe INTx becomes a virtual wire interrupt and there the
DisINTx bit definition states that setting this bit requires the device
to issue the de-assert message for that virtual wire, so there might be
some FUD relative to enabling MSI/X while the device has already
asserted INTx.
At best though, I don't know of anything to support that setting
DisINTx should have any effect on MSI/X, that much seems like it is
certainly a hardware bug. Thanks,
Alex
