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

[Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>]

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