Messages in this thread | | | Date | Thu, 22 Jun 2023 14:02:12 -0700 | Subject | Re: [PATCH v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug | From | Smita Koralahalli <> |
| |
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
| |