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

[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