lkml.org 
[lkml]   [2023]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug
From
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

\
 
 \ /
  Last update: 2023-06-22 23:12    [W:1.525 / U:0.232 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site