Message-ID: <1f90d885f0e8dc2e8d9b2b7e88700b4cdb19d84c.camel@redhat.com>
Date: Mon, 21 Oct 2024 08:47:46 +0200
From: Philipp Stanner <pstanner@...hat.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Heiner Kallweit <hkallweit1@...il.com>, Alex Williamson
 <alex.williamson@...hat.com>, Damien Le Moal <dlemoal@...nel.org>, Niklas
 Cassel <cassel@...nel.org>, Sergey Shtylyov <s.shtylyov@....ru>, Basavaraj
 Natikar <basavaraj.natikar@....com>, Jiri Kosina <jikos@...nel.org>,
 Benjamin Tissoires <bentiss@...nel.org>, Arnd Bergmann <arnd@...db.de>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Alex Dubov
 <oakad@...oo.com>, Sudarsana Kalluru <skalluru@...vell.com>, Manish Chopra
 <manishc@...vell.com>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
 Abeni <pabeni@...hat.com>, Rasesh Mody <rmody@...vell.com>,
 GR-Linux-NIC-Dev@...vell.com, Igor Mitsyanko <imitsyanko@...ntenna.com>,
 Sergey Matyukevich <geomatsi@...il.com>, Kalle Valo <kvalo@...nel.org>,
 Sanjay R Mehta <sanju.mehta@....com>, Shyam Sundar S K
 <Shyam-sundar.S-k@....com>, Jon Mason <jdmason@...zu.us>, Dave Jiang
 <dave.jiang@...el.com>, Allen Hubbe <allenbh@...il.com>, Bjorn Helgaas
 <bhelgaas@...gle.com>, Juergen Gross <jgross@...e.com>, Stefano Stabellini
 <sstabellini@...nel.org>, Oleksandr Tyshchenko
 <oleksandr_tyshchenko@...m.com>,  Jaroslav Kysela <perex@...ex.cz>, Takashi
 Iwai <tiwai@...e.com>, Chen Ni <nichen@...as.ac.cn>,  Mario Limonciello
 <mario.limonciello@....com>, Ricky Wu <ricky_wu@...ltek.com>, Al Viro
 <viro@...iv.linux.org.uk>,  Breno Leitao <leitao@...ian.org>, Kevin Tian
 <kevin.tian@...el.com>, Thomas Gleixner <tglx@...utronix.de>,  Ilpo
 Järvinen <ilpo.jarvinen@...ux.intel.com>, Andy
 Shevchenko <andriy.shevchenko@...ux.intel.com>, Mostafa Saleh
 <smostafa@...gle.com>,  Jason Gunthorpe <jgg@...pe.ca>, Yi Liu
 <yi.l.liu@...el.com>, Christian Brauner <brauner@...nel.org>, Ankit Agrawal
 <ankita@...dia.com>, Eric Auger <eric.auger@...hat.com>, Reinette Chatre
 <reinette.chatre@...el.com>, Ye Bin <yebin10@...wei.com>, Marek
 Marczykowski-Górecki <marmarek@...isiblethingslab.com>,
 Pierre-Louis Bossart <pierre-louis.bossart@...ux.dev>, Peter Ujfalusi
 <peter.ujfalusi@...ux.intel.com>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Kai Vehmanen
 <kai.vehmanen@...ux.intel.com>,  Rui Salvaterra <rsalvaterra@...il.com>,
 linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org, 
 linux-input@...r.kernel.org, netdev@...r.kernel.org, 
 linux-wireless@...r.kernel.org, ntb@...ts.linux.dev,
 linux-pci@...r.kernel.org,  kvm@...r.kernel.org,
 xen-devel@...ts.xenproject.org, linux-sound@...r.kernel.org
Subject: Re: [PATCH 13/13] PCI: Deprecate pci_intx(), pcim_intx()

On Fri, 2024-10-18 at 18:45 -0500, Bjorn Helgaas wrote:
> 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@...hat.com> 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?

Yes Sir, that's the idea

Regards,
P.

> 
> > > > > > Signed-off-by: Philipp Stanner <pstanner@...hat.com>
> > > > > > ---
> > > > > >  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)
> > > > > >  {
> > > > > 
> > > > 
> > > > 
> > > 
> > 
> 

Powered by blists - more mailing lists