Message-ID: <ff7f7358cec4bb03423879a2e4efd16d0a3e8ed7.camel@redhat.com>
Date: Fri, 15 Nov 2024 09:32:16 +0100
From: Philipp Stanner <pstanner@...hat.com>
To: Thomas Gleixner <tglx@...utronix.de>, Damien Le Moal
 <dlemoal@...nel.org>,  Niklas Cassel <cassel@...nel.org>, 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>, Andrew Lunn <andrew+netdev@...n.ch>, "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>, Alex Williamson <alex.williamson@...hat.com>,
 Juergen Gross <jgross@...e.com>, Stefano Stabellini
 <sstabellini@...nel.org>, Oleksandr Tyshchenko
 <oleksandr_tyshchenko@...m.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>, Mostafa Saleh
 <smostafa@...gle.com>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Jason Gunthorpe <jgg@...pe.ca>, Yi Liu <yi.l.liu@...el.com>, Kunwu Chan
 <chentao@...inos.cn>, Ankit Agrawal <ankita@...dia.com>, Christian Brauner
 <brauner@...nel.org>, Reinette Chatre <reinette.chatre@...el.com>, Eric
 Auger <eric.auger@...hat.com>, Ye Bin <yebin10@...wei.com>
Cc: 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
Subject: Re: [PATCH v2 11/11] Remove devres from pci_intx()

On Fri, 2024-11-15 at 01:46 +0100, Thomas Gleixner wrote:
> On Thu, Nov 14 2024 at 10:05, Philipp Stanner wrote:
> > On Wed, 2024-11-13 at 17:22 +0100, Thomas Gleixner wrote:
> > > On Wed, Nov 13 2024 at 13:41, 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(). Have all users
> > > > of
> > > > pci_intx_unmanaged() call pci_intx(). Remove
> > > > pci_intx_unmanaged().
> > > > 
> > > > Signed-off-by: Philipp Stanner <pstanner@...hat.com>
> > > > ---
> > > >  drivers/misc/cardreader/rtsx_pcr.c            |  2 +-
> > > >  drivers/misc/tifm_7xx1.c                      |  6 +--
> > > >  .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  |  2 +-
> > > >  drivers/net/ethernet/brocade/bna/bnad.c       |  2 +-
> > > >  drivers/ntb/hw/amd/ntb_hw_amd.c               |  4 +-
> > > >  drivers/ntb/hw/intel/ntb_hw_gen1.c            |  2 +-
> > > >  drivers/pci/devres.c                          |  4 +-
> > > >  drivers/pci/msi/api.c                         |  2 +-
> > > >  drivers/pci/msi/msi.c                         |  2 +-
> > > >  drivers/pci/pci.c                             | 43 +----------
> > > > ----
> > > > ----
> > > >  drivers/vfio/pci/vfio_pci_core.c              |  2 +-
> > > >  drivers/vfio/pci/vfio_pci_intrs.c             | 10 ++---
> > > >  drivers/xen/xen-pciback/conf_space_header.c   |  2 +-
> > > >  include/linux/pci.h                           |  1 -
> > > >  14 files changed, 22 insertions(+), 62 deletions(-)
> > > 
> > > Now I'm utterly confused. This undoes the pci_intx_unmanaged()
> > > churn
> > > which you carefully split into several patches first.
> > 
> > Have you read the email I have linked?
> > 
> > There is also the cover-letter (does anyone in the community ever
> > read
> > those?) which explicitly states:
> > 
> > "Patch "Remove devres from pci_intx()" obviously reverts the
> > previous
> > patches that made drivers use pci_intx_unmanaged(). But this way
> > it's
> > easier to review and approve. It also makes sure that each checked
> > out
> > commit should provide correct behavior, not just the entire series
> > as a
> > whole."
> 
> I read it and I assume your intention was to force an eye on every
> use
> case of pci_intx() and not just on those which need to be converted
> to
> pcim_intx().
> 
> I'm not convinced that this is needed, but fair enough.

Whether pcim_enable_device() is really not used could have been
overlooked, or the driver could move to "managed mode" in parallel for
v6.13 for example. Then a bug would be silently introduced into those
drivers.

Besides, me touching pci_intx() unfortunately caused a few explosions
in the past already, in

fc8c818e756991f5f50b8dfab07f970a18da2556 and
00f89ae4e759a7eef07e4188e1534af7dd2c7e9c

So this time I prefer to be rather safe than sorry.


BTW, if you can review the MSI patch and check whether removing devres
from there really is fine, that would be helpful.


Regards,
P.

Powered by blists - more mailing lists