Message-ID: <a688a4109e292d7510ebd7206bd3296e23ca1e3b.camel@redhat.com>
Date: Fri, 11 Oct 2024 14:07:48 +0200
From: Philipp Stanner <pstanner@...hat.com>
To: Alex Williamson <alex.williamson@...hat.com>
Cc: Dan Carpenter <dan.carpenter@...aro.org>, 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>, Mario Limonciello <mario.limonciello@....com>, Chen
 Ni <nichen@...as.ac.cn>, 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>, Mostafa Saleh
 <smostafa@...gle.com>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Hannes Reinecke <hare@...e.de>, John Garry <john.g.garry@...cle.com>,
 Soumya Negi <soumya.negi97@...il.com>, Jason Gunthorpe <jgg@...pe.ca>, Yi
 Liu <yi.l.liu@...el.com>, "Dr. David Alan Gilbert" <linux@...blig.org>, 
 Christian Brauner <brauner@...nel.org>, Ankit Agrawal <ankita@...dia.com>,
 Reinette Chatre <reinette.chatre@...el.com>, Eric Auger
 <eric.auger@...hat.com>, Ye Bin <yebin10@...wei.com>, Marek
 Marczykowski-Górecki <marmarek@...isiblethingslab.com>,
 Pierre-Louis Bossart <pierre-louis.bossart@...ux.dev>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Kai Vehmanen
 <kai.vehmanen@...ux.intel.com>,  Peter Ujfalusi
 <peter.ujfalusi@...ux.intel.com>, Rui Salvaterra <rsalvaterra@...il.com>,
 Marc Zyngier <maz@...nel.org>, 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, 
 linux-staging@...ts.linux.dev, kvm@...r.kernel.org, 
 xen-devel@...ts.xenproject.org, linux-sound@...r.kernel.org
Subject: Re: [RFC PATCH 13/13] Remove devres from pci_intx()

On Thu, 2024-10-10 at 11:43 -0600, Alex Williamson wrote:
> On Thu, 10 Oct 2024 11:11:36 +0200
> Philipp Stanner <pstanner@...hat.com> wrote:
> 
> > On Thu, 2024-10-10 at 11:50 +0300, Dan Carpenter wrote:
> > > On Wed, Oct 09, 2024 at 10:35:19AM +0200, Philipp Stanner wrote: 
> > > > pci_intx() is a hybrid function which can sometimes be managed
> > > > through
> > > > devres. This hybrid nature is undesirable.
> > > > 
> > > > Since all users of pci_intx() have by now been ported either to
> > > > always-managed pcim_intx() or never-managed
> > > > pci_intx_unmanaged(),
> > > > the
> > > > devres functionality can be removed from pci_intx().
> > > > 
> > > > Consequently, pci_intx_unmanaged() is now redundant, because
> > > > pci_intx()
> > > > itself is now unmanaged.
> > > > 
> > > > Remove the devres functionality from pci_intx(). Remove
> > > > pci_intx_unmanaged().
> > > > Have all users of pci_intx_unmanaged() call pci_intx().
> > > > 
> > > > Signed-off-by: Philipp Stanner <pstanner@...hat.com>  
> > > 
> > > I don't like when we change a function like this but it still
> > > compiles fine.
> > > If someone is working on a driver and hasn't pushed it yet, then
> > > it's
> > > probably
> > > supposed to be using the new pcim_intx() but they won't discover
> > > that
> > > until they
> > > detect the leaks at runtime.  
> > 
> > There wouldn't be any *leaks*, it's just that the INTx state would
> > not
> > automatically be restored. BTW the official documentation in its
> > current state does not hint at pci_intx() doing anything
> > automatically,
> > but rather actively marks it as deprecated.
> > 
> > But you are right that a hypothetical new driver and OOT drivers
> > could
> > experience bugs through this change.
> > 
> > > 
> > > Why not leave the pci_intx_unmanaged() name.  It's ugly and that
> > > will
> > > discorage
> > > people from introducing new uses.  
> > 
> > I'd be OK with that. Then we'd have to remove pci_intx() as it has
> > new
> > users anymore.
> > 
> > Either way should be fine and keep the behavior for existing
> > drivers
> > identical.
> > 
> > I think Bjorn should express a preference
> 
> FWIW, I think pcim_intx() and pci_intx() align better to our naming
> convention for devres interfaces.

Yup, also my personal preference. But we can mark those functions as
deprecated via docstring-comment. That should fullfill Damien's goal.

>   Would it be sufficient if pci_intx()
> triggered a WARN_ON if called for a pci_is_managed() device?

No, I don't think that's a good idea; reason being that
pci_is_managed() just checks that global boolean which we inherited
from the old implementation and which should not be necessary with
proper devres.
The boolean is used for making functions such as pci_intx() and
__pci_request_region() hybrid. So with our non-hybrid version we never
need it.

P.

>   Thanks,
> 
> Alex
> 

Powered by blists - more mailing lists