[RFC PATCH 00/13] Remove implicit devres from pci_intx()

Philipp Stanner posted 13 patches 1 month, 2 weeks ago
There is a newer version of this series
drivers/ata/ahci.c                            |  2 +-
drivers/ata/ata_piix.c                        |  2 +-
drivers/ata/pata_rdc.c                        |  2 +-
drivers/ata/sata_sil24.c                      |  2 +-
drivers/ata/sata_sis.c                        |  2 +-
drivers/ata/sata_uli.c                        |  2 +-
drivers/ata/sata_vsc.c                        |  2 +-
drivers/hid/amd-sfh-hid/amd_sfh_pcie.c        |  4 ++--
drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c |  2 +-
.../wireless/quantenna/qtnfmac/pcie/pcie.c    |  2 +-
drivers/pci/devres.c                          | 24 +++----------------
drivers/pci/pci.c                             | 14 +----------
drivers/staging/rts5208/rtsx.c                |  2 +-
include/linux/pci.h                           |  1 +
sound/pci/hda/hda_intel.c                     |  2 +-
15 files changed, 18 insertions(+), 47 deletions(-)
[RFC PATCH 00/13] Remove implicit devres from pci_intx()
Posted by Philipp Stanner 1 month, 2 weeks ago
Hi all,

this series removes a problematic feature from pci_intx(). That function
sometimes implicitly uses devres for automatic cleanup. We should get
rid of this implicit behavior.

To do so, a pci_intx() version that is always-managed, and one that is
never-managed are provided. Then, all pci_intx() users are ported to the
version they need. Afterwards, pci_intx() can be cleaned up and the
users of the never-managed version be ported back to pci_intx().

This way we'd get this PCI API consistent again.

The last patch 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.

Merge plan for this would be to enter through the PCI tree.

Please say so if you've got concerns with the general idea behind the
RFC.

Regards,
P.

Philipp Stanner (13):
  PCI: Prepare removing devres from pci_intx()
  ALSA: hda: hda_intel: Use always-managed version of pcim_intx()
  drivers/xen: Use never-managed version of pci_intx()
  net/ethernet: Use never-managed version of pci_intx()
  net/ntb: Use never-managed version of pci_intx()
  misc: Use never-managed version of pci_intx()
  vfio/pci: Use never-managed version of pci_intx()
  PCI: MSI: Use never-managed version of pci_intx()
  ata: Use always-managed version of pci_intx()
  staging: rts5280: Use always-managed version of pci_intx()
  wifi: qtnfmac: use always-managed version of pcim_intx()
  HID: amd_sfh: Use always-managed version of pcim_intx()
  Remove devres from pci_intx()

 drivers/ata/ahci.c                            |  2 +-
 drivers/ata/ata_piix.c                        |  2 +-
 drivers/ata/pata_rdc.c                        |  2 +-
 drivers/ata/sata_sil24.c                      |  2 +-
 drivers/ata/sata_sis.c                        |  2 +-
 drivers/ata/sata_uli.c                        |  2 +-
 drivers/ata/sata_vsc.c                        |  2 +-
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c        |  4 ++--
 drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c |  2 +-
 .../wireless/quantenna/qtnfmac/pcie/pcie.c    |  2 +-
 drivers/pci/devres.c                          | 24 +++----------------
 drivers/pci/pci.c                             | 14 +----------
 drivers/staging/rts5208/rtsx.c                |  2 +-
 include/linux/pci.h                           |  1 +
 sound/pci/hda/hda_intel.c                     |  2 +-
 15 files changed, 18 insertions(+), 47 deletions(-)

-- 
2.46.1
Re: [RFC PATCH 00/13] Remove implicit devres from pci_intx()
Posted by Heiner Kallweit 1 month, 2 weeks ago
On 09.10.2024 10:35, Philipp Stanner wrote:
> Hi all,
> 
> this series removes a problematic feature from pci_intx(). That function
> sometimes implicitly uses devres for automatic cleanup. We should get
> rid of this implicit behavior.
> 
> To do so, a pci_intx() version that is always-managed, and one that is
> never-managed are provided. Then, all pci_intx() users are ported to the
> version they need. Afterwards, pci_intx() can be cleaned up and the
> users of the never-managed version be ported back to pci_intx().
> 
> This way we'd get this PCI API consistent again.
> 
AFAICS pci_intx() is used only by drivers which haven't been converted
to the pci_alloc_irq_vectors() API yet. Wouldn't it be better to do this
instead of trying to improve pci_intx()?
Eventually pci_intx() would have to be used in PCI core only.

> The last patch 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.
> 
> Merge plan for this would be to enter through the PCI tree.
> 
> Please say so if you've got concerns with the general idea behind the
> RFC.
> 
> Regards,
> P.
> 
> Philipp Stanner (13):
>   PCI: Prepare removing devres from pci_intx()
>   ALSA: hda: hda_intel: Use always-managed version of pcim_intx()
>   drivers/xen: Use never-managed version of pci_intx()
>   net/ethernet: Use never-managed version of pci_intx()
>   net/ntb: Use never-managed version of pci_intx()
>   misc: Use never-managed version of pci_intx()
>   vfio/pci: Use never-managed version of pci_intx()
>   PCI: MSI: Use never-managed version of pci_intx()
>   ata: Use always-managed version of pci_intx()
>   staging: rts5280: Use always-managed version of pci_intx()
>   wifi: qtnfmac: use always-managed version of pcim_intx()
>   HID: amd_sfh: Use always-managed version of pcim_intx()
>   Remove devres from pci_intx()
> 
>  drivers/ata/ahci.c                            |  2 +-
>  drivers/ata/ata_piix.c                        |  2 +-
>  drivers/ata/pata_rdc.c                        |  2 +-
>  drivers/ata/sata_sil24.c                      |  2 +-
>  drivers/ata/sata_sis.c                        |  2 +-
>  drivers/ata/sata_uli.c                        |  2 +-
>  drivers/ata/sata_vsc.c                        |  2 +-
>  drivers/hid/amd-sfh-hid/amd_sfh_pcie.c        |  4 ++--
>  drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c |  2 +-
>  .../wireless/quantenna/qtnfmac/pcie/pcie.c    |  2 +-
>  drivers/pci/devres.c                          | 24 +++----------------
>  drivers/pci/pci.c                             | 14 +----------
>  drivers/staging/rts5208/rtsx.c                |  2 +-
>  include/linux/pci.h                           |  1 +
>  sound/pci/hda/hda_intel.c                     |  2 +-
>  15 files changed, 18 insertions(+), 47 deletions(-)
>
Re: [RFC PATCH 00/13] Remove implicit devres from pci_intx()
Posted by Philipp Stanner 1 month, 2 weeks ago
On Wed, 2024-10-09 at 20:32 +0200, Heiner Kallweit wrote:
> On 09.10.2024 10:35, Philipp Stanner wrote:
> > Hi all,
> > 
> > this series removes a problematic feature from pci_intx(). That
> > function
> > sometimes implicitly uses devres for automatic cleanup. We should
> > get
> > rid of this implicit behavior.
> > 
> > To do so, a pci_intx() version that is always-managed, and one that
> > is
> > never-managed are provided. Then, all pci_intx() users are ported
> > to the
> > version they need. Afterwards, pci_intx() can be cleaned up and the
> > users of the never-managed version be ported back to pci_intx().
> > 
> > This way we'd get this PCI API consistent again.
> > 
> AFAICS pci_intx() is used only by drivers which haven't been
> converted
> to the pci_alloc_irq_vectors() API yet. Wouldn't it be better to do
> this
> instead of trying to improve pci_intx()?

This would be the créme-de-la-créme-solution, yes.

But such a portation would require more detailed knowledge of the old
drivers.

In this discussion, Alex points out that at least in some drivers, you
can't replace pci_intx() without further ado:
https://lore.kernel.org/all/20240904151020.486f599e.alex.williamson@redhat.com/


What we could do is mark pci_intx() and pcim_intx() as deprecated and
point everyone to pci_alloc_irq_vectors(). Then someone can look into
porting the old drivers at some point in the future.


P.


> Eventually pci_intx() would have to be used in PCI core only.
> 
> > The last patch 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.
> > 
> > Merge plan for this would be to enter through the PCI tree.
> > 
> > Please say so if you've got concerns with the general idea behind
> > the
> > RFC.
> > 
> > Regards,
> > P.
> > 
> > Philipp Stanner (13):
> >   PCI: Prepare removing devres from pci_intx()
> >   ALSA: hda: hda_intel: Use always-managed version of pcim_intx()
> >   drivers/xen: Use never-managed version of pci_intx()
> >   net/ethernet: Use never-managed version of pci_intx()
> >   net/ntb: Use never-managed version of pci_intx()
> >   misc: Use never-managed version of pci_intx()
> >   vfio/pci: Use never-managed version of pci_intx()
> >   PCI: MSI: Use never-managed version of pci_intx()
> >   ata: Use always-managed version of pci_intx()
> >   staging: rts5280: Use always-managed version of pci_intx()
> >   wifi: qtnfmac: use always-managed version of pcim_intx()
> >   HID: amd_sfh: Use always-managed version of pcim_intx()
> >   Remove devres from pci_intx()
> > 
> >  drivers/ata/ahci.c                            |  2 +-
> >  drivers/ata/ata_piix.c                        |  2 +-
> >  drivers/ata/pata_rdc.c                        |  2 +-
> >  drivers/ata/sata_sil24.c                      |  2 +-
> >  drivers/ata/sata_sis.c                        |  2 +-
> >  drivers/ata/sata_uli.c                        |  2 +-
> >  drivers/ata/sata_vsc.c                        |  2 +-
> >  drivers/hid/amd-sfh-hid/amd_sfh_pcie.c        |  4 ++--
> >  drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c |  2 +-
> >  .../wireless/quantenna/qtnfmac/pcie/pcie.c    |  2 +-
> >  drivers/pci/devres.c                          | 24 +++------------
> > ----
> >  drivers/pci/pci.c                             | 14 +----------
> >  drivers/staging/rts5208/rtsx.c                |  2 +-
> >  include/linux/pci.h                           |  1 +
> >  sound/pci/hda/hda_intel.c                     |  2 +-
> >  15 files changed, 18 insertions(+), 47 deletions(-)
> > 
> 
Re: [RFC PATCH 00/13] Remove implicit devres from pci_intx()
Posted by Andy Shevchenko 1 month, 2 weeks ago
On Thu, Oct 10, 2024 at 10:09:12AM +0200, Philipp Stanner wrote:
> On Wed, 2024-10-09 at 20:32 +0200, Heiner Kallweit wrote:
> > On 09.10.2024 10:35, Philipp Stanner wrote:

...

> > > To do so, a pci_intx() version that is always-managed, and one that
> > > is
> > > never-managed are provided. Then, all pci_intx() users are ported
> > > to the
> > > version they need. Afterwards, pci_intx() can be cleaned up and the
> > > users of the never-managed version be ported back to pci_intx().
> > > 
> > > This way we'd get this PCI API consistent again.
> > > 
> > AFAICS pci_intx() is used only by drivers which haven't been
> > converted
> > to the pci_alloc_irq_vectors() API yet. Wouldn't it be better to do
> > this
> > instead of trying to improve pci_intx()?

My first impression was the same...

> This would be the créme-de-la-créme-solution, yes.
> 
> But such a portation would require more detailed knowledge of the old
> drivers.
> 
> In this discussion, Alex points out that at least in some drivers, you
> can't replace pci_intx() without further ado:
> https://lore.kernel.org/all/20240904151020.486f599e.alex.williamson@redhat.com/
> 
> What we could do is mark pci_intx() and pcim_intx() as deprecated and
> point everyone to pci_alloc_irq_vectors(). Then someone can look into
> porting the old drivers at some point in the future.

...but here I got the point by Philipp.

-- 
With Best Regards,
Andy Shevchenko