iwlwifi write to PCI_CFG_RETRY_TIMEOUT

iwlwifi write to PCI_CFG_RETRY_TIMEOUT

[Jason Andryuk]

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 <lrodriguez@atheros.com>
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 <jouni.malinen@atheros.com>
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

Re: iwlwifi write to PCI_CFG_RETRY_TIMEOUT

[Jason Andryuk]

On Thu, Nov 17, 2022 at 2:33 PM Jason Andryuk <jandryuk@gmail.com> 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/43e72e890912192251r4de4a3c3idb5e4c3723ef87aa@mail.gmail.com/

https://lore.kernel.org/linux-wireless/20090616151258.GA22849@jm.kir.nu/

>> 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

Re: iwlwifi write to PCI_CFG_RETRY_TIMEOUT

[Emmanuel Grumbach]

On Fri, Nov 18, 2022 at 11:13 PM Jason Andryuk <jandryuk@gmail.com> wrote:
>
> On Thu, Nov 17, 2022 at 2:33 PM Jason Andryuk <jandryuk@gmail.com> 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/43e72e890912192251r4de4a3c3idb5e4c3723ef87aa@mail.gmail.com/
>
> https://lore.kernel.org/linux-wireless/20090616151258.GA22849@jm.kir.nu/
>
> >> 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..

Re: iwlwifi write to PCI_CFG_RETRY_TIMEOUT

[Jason Andryuk]

On Sun, Nov 20, 2022 at 2:50 PM Emmanuel Grumbach <egrumbach@gmail.com> wrote:
>
> On Fri, Nov 18, 2022 at 11:13 PM Jason Andryuk <jandryuk@gmail.com> wrote:
> >
> > On Thu, Nov 17, 2022 at 2:33 PM Jason Andryuk <jandryuk@gmail.com> 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/43e72e890912192251r4de4a3c3idb5e4c3723ef87aa@mail.gmail.com/
> >
> > https://lore.kernel.org/linux-wireless/20090616151258.GA22849@jm.kir.nu/
> >
> > >> 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