Re: [PATCH v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

From: Smita Koralahalli
Date: Thu Jun 22 2023 - 17:11:10 EST


On 6/21/2023 11:31 PM, Lukas Wunner wrote:
On Wed, Jun 21, 2023 at 06:51:52PM +0000, Smita Koralahalli wrote:
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -102,6 +102,10 @@ void pciehp_unconfigure_device(struct controller *ctrl, bool presence)
pci_lock_rescan_remove();
+ pcie_capability_clear_word(ctrl->pcie->port, PCI_EXP_DEVCTL2,
+ (PCI_EXP_DEVCTL2_ARI | PCI_EXP_DEVCTL2_ATOMIC_REQ |
+ PCI_EXP_DEVCTL2_TAG_REQ_EN));
+

Hm, this will clear the bits while the device may still be present.
Note that the subsequent pci_stop_and_remove_bus_device() will unbind
the driver and may thus cause communication with the device.
Can clearing those bits in the hotplug port hamper communication with
the device?

I'd recommend avoiding that issue altogether by clearing the bits at
the end of the function after the call to pci_unlock_rescan_remove(),
so that negotiated state of the hotplug port gets cleared after all
subordinate devices are de-enumerated.

This is a good point. Thanks!


The commit message doesn't point out that PCI_EXP_DEVCTL2_ARI is
already being taken care of on enumeration of future subordinate
devices in pci_configure_ari() and is only cleared here for good
measure. If you intend to configure 10 bit tags and atomic ops
on enumeration in future patches, I'd recommend omitting
PCI_EXP_DEVCTL2_ARI here and clearing each of the other two bits
in the future patches which configure them on enumeration.

Would it be fair to just reuse pci_enable_atomic_ops_to_root() for
Atomic_Ops configuration?


You don't need braces around the "or" operation for the bits.

Sure!

Thanks,
Smita


Thanks,

Lukas

/*
* Stopping an SR-IOV PF device removes all the associated VFs,
* which will update the bus->devices list and confuse the
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index dc2000e0fe3a..6fbc47f23d52 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -668,6 +668,7 @@
#define PCI_EXP_DEVCTL2_IDO_REQ_EN 0x0100 /* Allow IDO for requests */
#define PCI_EXP_DEVCTL2_IDO_CMP_EN 0x0200 /* Allow IDO for completions */
#define PCI_EXP_DEVCTL2_LTR_EN 0x0400 /* Enable LTR mechanism */
+#define PCI_EXP_DEVCTL2_TAG_REQ_EN 0x1000 /* Allow 10 Tags for Requester */
#define PCI_EXP_DEVCTL2_OBFF_MSGA_EN 0x2000 /* Enable OBFF Message type A */
#define PCI_EXP_DEVCTL2_OBFF_MSGB_EN 0x4000 /* Enable OBFF Message type B */
#define PCI_EXP_DEVCTL2_OBFF_WAKE_EN 0x6000 /* OBFF using WAKE# signaling */
--
2.17.1