Hi,
I was looking at iwlwifi under Xen PCI passthrough and I noticed a
curious PCI config space write:
https://github.com/torvalds/linux/blob/master/drivers/net/wireless/intel/iwlwifi/pcie/drv.c#L1721
/*
* We disable the RETRY_TIMEOUT register (0x41) to keep
* PCI Tx retries from interfering with C3 CPU state.
*/
pci_write_config_byte(pdev, PCI_CFG_RETRY_TIMEOUT, 0x00);
With 00:14.3 Network controller: Intel Corporation Cannon Point-LP
CNVi [Wireless-AC] (rev 30)
register 0x41 in the PCI config space is the next cap pointer for
"Capabilities: [40] Express Root Complex Integrated Endpoint, MSI 00".
On baremetal, the write seems to be dropped since `hexdump -C
/sys/bus/pci/devices/0000\:00\:14.3/config` still shows 0x80 (in my
case). Though I suppose the device could be acting on it even if the
value doesn't change.
With Xen PCI passthrough, QEMU seems to honor emulating the write and
it breaks lspci traversing the capabilities so MSI-X is no longer
shown.
Is the write to RETRY_TIMEOUT at 0x41 correct? It seems to be really
old. Here it references being copied from ipw2100:
commit b572b24c578ab1be9d1fcb11d2d8244878757a66
Author: Luis R. Rodriguez <[email protected]>
Date: Thu Mar 12 18:18:51 2009 -0400
ath9k: remove dummy PCI "retry timeout" fix
Remove the PCI retry timeout code as that was just taken from ipw2100
due to historical reasons but in reality its a no-op, additionally its
simply incorrect as each PCI devices has its own custom PCI configuration
space on PCI config space >= 0x40. Not to mention we were trying to write
0 to a place that already has 0 on it.
That was applied, but then reverted in:
commit f0214843ba23d9bf6dc6b8ad2c6ee27b60f0322e
Author: Jouni Malinen <[email protected]>
Date: Tue Jun 16 11:59:23 2009 +0300
ath9k: Fix PCI FATAL interrupts by restoring RETRY_TIMEOUT disabling
An earlier commit, 'ath9k: remove dummy PCI "retry timeout" fix', removed
code that was documented to disable RETRY_TIMEOUT register (PCI reg
0x41) since it was claimed to be a no-op. However, it turns out that
there are some combinations of hosts and ath9k-supported cards for
which this is not a no-op (reg 0x41 has value 0x80, not 0) and this
code (or something similar) is needed. In such cases, the driver may
be next to unusable due to very frequent PCI FATAL interrupts from the
card.
Reverting the earlier commit, i.e., restoring the RETRY_TIMEOUT
disabling, seems to resolve the issue. Since the removal of this code
was not based on any known issue and was purely a cleanup change, the
safest option here is to just revert that commit. Should there be
desire to clean this up in the future, the change will need to be
tested with a more complete coverage of cards and host systems.
At least with newer devices, it seems incorrect since it is writing to
the next capability pointer.
Thanks,
Jason
On Thu, Nov 17, 2022 at 2:33 PM Jason Andryuk <[email protected]> wrote:
>
> Hi,
>
> I was looking at iwlwifi under Xen PCI passthrough and I noticed a
> curious PCI config space write:
>
> https://github.com/torvalds/linux/blob/master/drivers/net/wireless/intel/iwlwifi/pcie/drv.c#L1721
> /*
> * We disable the RETRY_TIMEOUT register (0x41) to keep
> * PCI Tx retries from interfering with C3 CPU state.
> */
> pci_write_config_byte(pdev, PCI_CFG_RETRY_TIMEOUT, 0x00);
>
> With 00:14.3 Network controller: Intel Corporation Cannon Point-LP
> CNVi [Wireless-AC] (rev 30)
> register 0x41 in the PCI config space is the next cap pointer for
> "Capabilities: [40] Express Root Complex Integrated Endpoint, MSI 00".
>
> On baremetal, the write seems to be dropped since `hexdump -C
> /sys/bus/pci/devices/0000\:00\:14.3/config` still shows 0x80 (in my
> case). Though I suppose the device could be acting on it even if the
> value doesn't change.
>
> With Xen PCI passthrough, QEMU seems to honor emulating the write and
> it breaks lspci traversing the capabilities so MSI-X is no longer
> shown.
>
> Is the write to RETRY_TIMEOUT at 0x41 correct? It seems to be really
> old. Here it references being copied from ipw2100:
It seems like lots of drivers copied the write from ipw2100. And it
seems like no one knows exactly why it is there. But it does do
something for some devices which is why ath9k kept it.
These are some interesting and relevant emails:
https://lore.kernel.org/linux-wireless/[email protected]/
https://lore.kernel.org/linux-wireless/[email protected]/
>> I seem to remember the provenance of this code was copy-paste from
>> an intel driver, so while it does "something," the comment may not
>> match the code, 0x41 being vendor-defined.
>
> The exact story behind this has been a bit more than trivial task to
> figure out ;-). I would assume this comment is referring to a madwifi
> changeset: http://madwifi-project.org/changeset/584
Unfortunately, that link seems to be dead and archive.org doesn't have it.
Regards,
Jason
On Fri, Nov 18, 2022 at 11:13 PM Jason Andryuk <[email protected]> wrote:
>
> On Thu, Nov 17, 2022 at 2:33 PM Jason Andryuk <[email protected]> wrote:
> >
> > Hi,
> >
> > I was looking at iwlwifi under Xen PCI passthrough and I noticed a
> > curious PCI config space write:
> >
> > https://github.com/torvalds/linux/blob/master/drivers/net/wireless/intel/iwlwifi/pcie/drv.c#L1721
> > /*
> > * We disable the RETRY_TIMEOUT register (0x41) to keep
> > * PCI Tx retries from interfering with C3 CPU state.
> > */
> > pci_write_config_byte(pdev, PCI_CFG_RETRY_TIMEOUT, 0x00);
> >
> > With 00:14.3 Network controller: Intel Corporation Cannon Point-LP
> > CNVi [Wireless-AC] (rev 30)
> > register 0x41 in the PCI config space is the next cap pointer for
> > "Capabilities: [40] Express Root Complex Integrated Endpoint, MSI 00".
> >
> > On baremetal, the write seems to be dropped since `hexdump -C
> > /sys/bus/pci/devices/0000\:00\:14.3/config` still shows 0x80 (in my
> > case). Though I suppose the device could be acting on it even if the
> > value doesn't change.
> >
> > With Xen PCI passthrough, QEMU seems to honor emulating the write and
> > it breaks lspci traversing the capabilities so MSI-X is no longer
> > shown.
> >
> > Is the write to RETRY_TIMEOUT at 0x41 correct? It seems to be really
> > old. Here it references being copied from ipw2100:
>
> It seems like lots of drivers copied the write from ipw2100. And it
> seems like no one knows exactly why it is there. But it does do
> something for some devices which is why ath9k kept it.
>
> These are some interesting and relevant emails:
> https://lore.kernel.org/linux-wireless/[email protected]/
>
> https://lore.kernel.org/linux-wireless/[email protected]/
>
> >> I seem to remember the provenance of this code was copy-paste from
> >> an intel driver, so while it does "something," the comment may not
> >> match the code, 0x41 being vendor-defined.
> >
> > The exact story behind this has been a bit more than trivial task to
> > figure out ;-). I would assume this comment is referring to a madwifi
> > changeset: http://madwifi-project.org/changeset/584
>
> Unfortunately, that link seems to be dead and archive.org doesn't have it.
>
I asked internally and unfortunately I couldn't get any clear answer.
Looks like this setting
has been there for many many years (~15) and nobody really understands
why. It has been
removed for our newest device in another (close source) driver and I
guess we'll do the
same in Linux. I am not sure what we'll do with legacy devices though...
We could remove it and risk a regression, or just ... leave it..
On Sun, Nov 20, 2022 at 2:50 PM Emmanuel Grumbach <[email protected]> wrote:
>
> On Fri, Nov 18, 2022 at 11:13 PM Jason Andryuk <[email protected]> wrote:
> >
> > On Thu, Nov 17, 2022 at 2:33 PM Jason Andryuk <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > I was looking at iwlwifi under Xen PCI passthrough and I noticed a
> > > curious PCI config space write:
> > >
> > > https://github.com/torvalds/linux/blob/master/drivers/net/wireless/intel/iwlwifi/pcie/drv.c#L1721
> > > /*
> > > * We disable the RETRY_TIMEOUT register (0x41) to keep
> > > * PCI Tx retries from interfering with C3 CPU state.
> > > */
> > > pci_write_config_byte(pdev, PCI_CFG_RETRY_TIMEOUT, 0x00);
> > >
> > > With 00:14.3 Network controller: Intel Corporation Cannon Point-LP
> > > CNVi [Wireless-AC] (rev 30)
> > > register 0x41 in the PCI config space is the next cap pointer for
> > > "Capabilities: [40] Express Root Complex Integrated Endpoint, MSI 00".
> > >
> > > On baremetal, the write seems to be dropped since `hexdump -C
> > > /sys/bus/pci/devices/0000\:00\:14.3/config` still shows 0x80 (in my
> > > case). Though I suppose the device could be acting on it even if the
> > > value doesn't change.
> > >
> > > With Xen PCI passthrough, QEMU seems to honor emulating the write and
> > > it breaks lspci traversing the capabilities so MSI-X is no longer
> > > shown.
> > >
> > > Is the write to RETRY_TIMEOUT at 0x41 correct? It seems to be really
> > > old. Here it references being copied from ipw2100:
> >
> > It seems like lots of drivers copied the write from ipw2100. And it
> > seems like no one knows exactly why it is there. But it does do
> > something for some devices which is why ath9k kept it.
> >
> > These are some interesting and relevant emails:
> > https://lore.kernel.org/linux-wireless/[email protected]/
> >
> > https://lore.kernel.org/linux-wireless/[email protected]/
> >
> > >> I seem to remember the provenance of this code was copy-paste from
> > >> an intel driver, so while it does "something," the comment may not
> > >> match the code, 0x41 being vendor-defined.
> > >
> > > The exact story behind this has been a bit more than trivial task to
> > > figure out ;-). I would assume this comment is referring to a madwifi
> > > changeset: http://madwifi-project.org/changeset/584
> >
> > Unfortunately, that link seems to be dead and archive.org doesn't have it.
> >
>
> I asked internally and unfortunately I couldn't get any clear answer.
> Looks like this setting
> has been there for many many years (~15) and nobody really understands
> why. It has been
> removed for our newest device in another (close source) driver and I
> guess we'll do the
> same in Linux. I am not sure what we'll do with legacy devices though...
> We could remove it and risk a regression, or just ... leave it..
Thanks, for checking, Emmanuel.
Yes, it seems safest to just leave the write and avoid the risk of
regression. It is curious how this "mystery" write made its way into
so many drivers.
Regards,
Jason