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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Mar 16, 2022 at 04:15:48PM -0500, Bjorn Helgaas wrote:
> 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 guess it's wrong wording in the last paragraph. It's not safe, but it's
_required_ since HW doesn't follow PCI specification that clearly says:
"MSI, MSI-X, and INTx are mutually exclusive".

This is also supported by the other devices on the same chipset doing
that and by the reported of the issue.

-- 
With Best Regards,
Andy Shevchenko