On Wed, Oct 16, 2024 at 10:53:16AM +0200, Philipp Stanner wrote:
> On Wed, 2024-10-16 at 10:43 +0200, Heiner Kallweit wrote:
> > On 16.10.2024 08:57, Philipp Stanner wrote:
> > > On Tue, 2024-10-15 at 13:53 -0600, Alex Williamson wrote:
> > > > On Tue, 15 Oct 2024 20:51:23 +0200
> > > > Philipp Stanner <pstanner@xxxxxxxxxx> wrote:
> > > >
> > > > > pci_intx() and its managed counterpart pcim_intx() only exist
> > > > > for
> > > > > older
> > > > > drivers which have not been ported yet for various reasons.
> > > > > Future
> > > > > drivers should preferably use pci_alloc_irq_vectors().
> > > > >
> > > > > Mark pci_intx() and pcim_intx() as deprecated and encourage
> > > > > usage
> > > > > of
> > > > > pci_alloc_irq_vectors() in its place.
> > > >
> > > > I don't really understand this. As we've discussed previously
> > > > pci_alloc_irq_vectors() is, unsurprisingly, for allocating PCI
> > > > IRQ
> > > > vectors while pci_intx() is for manipulating the INTx disable bit
> > > > on
> > > > PCI devices. The latter is a generic mechanism for preventing
> > > > PCI
> > > > devices from generating INTx, regardless of whether there's a
> > > > vector
> > > > allocated for it. How does the former replace the latter and why
> > > > do
> > > > we
> > > > feel the need to deprecate the latter?
> > > >
> > > > It feels like this fits some narrow narrative and makes all users
> > > > of
> > > > these now deprecated functions second class citizens. Why? At
> > > > it's
> > > > root these are simply providing mask and set or mask and clear
> > > > register
> > > > bit operations. Thanks,
> > >
> > > I got the feeling from the RFC discussion that that was basically
> > > the
> > > consensus: people should use pci_alloc_irq_vectors(). Or did I
> > > misunderstand Andy and Heiner?
> > >
> > I think there are two different use cases for pci_intx().
> > At first there are several drivers where the direct usage of
> > pci_intx()
> > can be eliminated by switching to the pci_alloc_irq_vectors() API.
> >
> > And then there's usage of pci_intx() in
> > drivers/vfio/pci/vfio_pci_intrs.c
> > drivers/xen/xen-pciback/conf_space_header.c
> > There we have to keep the (AFAICS unmanaged) pci_intx() calls.
>
> There is also the usage within PCI itself, in MSI. Patch №8 touches
> that.
>
> It's why I think this series should land before anyone should port
> direct pci_intx() users to the irq vectors function, because the latter
> also uses pci_intx() and its own devres, which sounds explosive to me.
>
> > > I'm perfectly happy with dropping this patch and continue offering
> > > pci{m}_intx() to users, since after removing that hybrid hazzard I
> > > don't see any harm in them anymore.
So is the bottom line that we should drop *this* patch and apply the
rest of the series?
> > > > > Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
> > > > > ---
> > > > > drivers/pci/devres.c | 5 ++++-
> > > > > drivers/pci/pci.c | 5 ++++-
> > > > > 2 files changed, 8 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> > > > > index 6f8f712fe34e..4c76fc063104 100644
> > > > > --- a/drivers/pci/devres.c
> > > > > +++ b/drivers/pci/devres.c
> > > > > @@ -435,7 +435,7 @@ static struct pcim_intx_devres
> > > > > *get_or_create_intx_devres(struct device *dev)
> > > > > }
> > > > >
> > > > > /**
> > > > > - * pcim_intx - managed pci_intx()
> > > > > + * pcim_intx - managed pci_intx() (DEPRECATED)
> > > > > * @pdev: the PCI device to operate on
> > > > > * @enable: boolean: whether to enable or disable PCI INTx
> > > > > *
> > > > > @@ -443,6 +443,9 @@ static struct pcim_intx_devres
> > > > > *get_or_create_intx_devres(struct device *dev)
> > > > > *
> > > > > * Enable/disable PCI INTx for device @pdev.
> > > > > * Restore the original state on driver detach.
> > > > > + *
> > > > > + * This function is DEPRECATED. Do not use it in new code.
> > > > > + * Use pci_alloc_irq_vectors() instead (there is no managed
> > > > > version, currently).
> > > > > */
> > > > > int pcim_intx(struct pci_dev *pdev, int enable)
> > > > > {
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index 7ce1d0e3a1d5..dc69e23b8982 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -4477,11 +4477,14 @@ void pci_disable_parity(struct pci_dev
> > > > > *dev)
> > > > > }
> > > > >
> > > > > /**
> > > > > - * pci_intx - enables/disables PCI INTx for device dev
> > > > > + * pci_intx - enables/disables PCI INTx for device dev
> > > > > (DEPRECATED)
> > > > > * @pdev: the PCI device to operate on
> > > > > * @enable: boolean: whether to enable or disable PCI INTx
> > > > > *
> > > > > * Enables/disables PCI INTx for device @pdev
> > > > > + *
> > > > > + * This function is DEPRECATED. Do not use it in new code.
> > > > > + * Use pci_alloc_irq_vectors() instead.
> > > > > */
> > > > > void pci_intx(struct pci_dev *pdev, int enable)
> > > > > {
> > > >
> > >
> > >
> >
>
