Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
From:   Jason Andryuk <jandryuk@gmail.com>
Date:   Thu, 17 Nov 2022 14:33:47 -0500
Message-ID: <CAKf6xpuJgj2TPGZsUw7JXvMkpv-wzgPq13ySzMaz0R2Ti9WQ4A@mail.gmail.com>
Subject: iwlwifi write to PCI_CFG_RETRY_TIMEOUT
To:     linux-wireless@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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