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

[Alex Williamson]

On Thu, 10 Oct 2024 11:11:36 +0200
Philipp Stanner <pstanner@xxxxxxxxxx> 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@xxxxxxxxxx>  
> > 
> > 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