Re: [RFC PATCH 13/13] Remove devres from pci_intx()

Alex Williamson
2024-10-10T17:43:20Z

On Thu, 10 Oct 2024 11:11:36 +0200
Philipp Stanner <pstanner@redhat.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@redhat.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. Would it be sufficient if pci_intx()
triggered a WARN_ON if called for a pci_is_managed() device? Thanks,

Alex