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

[Lukas Wunner]

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.

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.

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

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