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

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

[Philipp Stanner]

@Driver-Maintainers: Your driver might be touched by patch "Remove
devres from pci_intx()". You might want to take a look.

Changes since the RFC [1]:
  - Add a patch deprecating pci{m}_intx(). (Heiner, Andy, Me)
  - Add Acked-by's already given.
  - Export pcim_intx() as a GPL function. (Alex)
  - Drop patch for rts5280, since this driver will be removed quite
    soon. (Philipp Hortmann, Greg)
  - Use early-return in pci_intx_unmanaged() and pci_intx(). (Andy)

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.

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.

Merge plan for this is to enter through the PCI tree.

[1] https://lore.kernel.org/all/20241009083519.10088-1-pstanner@redhat.com/


Regards,
P.

Philipp Stanner (13):
  PCI: Prepare removing devres from pci_intx()
  ALSA: 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()
  wifi: qtnfmac: use always-managed version of pcim_intx()
  HID: amd_sfh: Use always-managed version of pcim_intx()
  Remove devres from pci_intx()
  PCI: Deprecate pci_intx(), pcim_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                          | 29 +++++--------------
 drivers/pci/pci.c                             | 19 ++++--------
 include/linux/pci.h                           |  1 +
 sound/pci/hda/hda_intel.c                     |  2 +-
 14 files changed, 26 insertions(+), 47 deletions(-)

-- 
2.47.0

[PATCH 01/13] PCI: Prepare removing devres from pci_intx()

[Philipp Stanner]

pci_intx() is a hybrid function which sometimes performs devres
operations, depending on whether pcim_enable_device() has been used to
enable the pci_dev. This sometimes-managed nature of the function is
problematic. Notably, it causes the function to allocate under some
circumstances which makes it unusable from interrupt context.

To, ultimately, remove the hybrid nature from pci_intx(), it is first
necessary to provide an always-managed and a never-managed version
of that function. Then, all callers of pci_intx() can be ported to the
version they need, depending whether they use pci_enable_device() or
pcim_enable_device().

An always-managed function exists, namely pcim_intx(), for which
__pcim_intx(), a never-managed version of pci_intx() had been
implemented.

Make __pcim_intx() a public function under the name
pci_intx_unmanaged(). Make pcim_intx() a public function.

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/pci/devres.c | 24 +++---------------------
 drivers/pci/pci.c    | 28 ++++++++++++++++++++++++++++
 include/linux/pci.h  |  2 ++
 3 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index b133967faef8..d32827a1f2f4 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -411,31 +411,12 @@ static inline bool mask_contains_bar(int mask, int bar)
 	return mask & BIT(bar);
 }
 
-/*
- * This is a copy of pci_intx() used to bypass the problem of recursive
- * function calls due to the hybrid nature of pci_intx().
- */
-static void __pcim_intx(struct pci_dev *pdev, int enable)
-{
-	u16 pci_command, new;
-
-	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
-
-	if (enable)
-		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
-	else
-		new = pci_command | PCI_COMMAND_INTX_DISABLE;
-
-	if (new != pci_command)
-		pci_write_config_word(pdev, PCI_COMMAND, new);
-}
-
 static void pcim_intx_restore(struct device *dev, void *data)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct pcim_intx_devres *res = data;
 
-	__pcim_intx(pdev, res->orig_intx);
+	pci_intx_unmanaged(pdev, res->orig_intx);
 }
 
 static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
@@ -472,10 +453,11 @@ int pcim_intx(struct pci_dev *pdev, int enable)
 		return -ENOMEM;
 
 	res->orig_intx = !enable;
-	__pcim_intx(pdev, enable);
+	pci_intx_unmanaged(pdev, enable);
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(pcim_intx);
 
 static void pcim_disable_device(void *pdev_raw)
 {
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7d85c04fbba2..d7fd0772a885 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4476,6 +4476,34 @@ void pci_disable_parity(struct pci_dev *dev)
 	}
 }
 
+/**
+ * pci_intx - enables/disables PCI INTx for device dev, unmanaged version
+ * @pdev: the PCI device to operate on
+ * @enable: boolean: whether to enable or disable PCI INTx
+ *
+ * Enables/disables PCI INTx for device @pdev
+ *
+ * This function behavios identically to pci_intx(), but is never managed with
+ * devres.
+ */
+void pci_intx_unmanaged(struct pci_dev *pdev, int enable)
+{
+	u16 pci_command, new;
+
+	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
+
+	if (enable)
+		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
+	else
+		new = pci_command | PCI_COMMAND_INTX_DISABLE;
+
+	if (new == pci_command)
+		return;
+
+	pci_write_config_word(pdev, PCI_COMMAND, new);
+}
+EXPORT_SYMBOL_GPL(pci_intx_unmanaged);
+
 /**
  * pci_intx - enables/disables PCI INTx for device dev
  * @pdev: the PCI device to operate on
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 573b4c4c2be6..6b8cde76d564 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1353,6 +1353,7 @@ int __must_check pcim_set_mwi(struct pci_dev *dev);
 int pci_try_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 void pci_disable_parity(struct pci_dev *dev);
+void pci_intx_unmanaged(struct pci_dev *pdev, int enable);
 void pci_intx(struct pci_dev *dev, int enable);
 bool pci_check_and_mask_intx(struct pci_dev *dev);
 bool pci_check_and_unmask_intx(struct pci_dev *dev);
@@ -2293,6 +2294,7 @@ static inline void pci_fixup_device(enum pci_fixup_pass pass,
 				    struct pci_dev *dev) { }
 #endif
 
+int pcim_intx(struct pci_dev *pdev, int enabled);
 void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen);
 void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar,
 				const char *name);
-- 
2.47.0

Re: [PATCH 01/13] PCI: Prepare removing devres from pci_intx()

[Thomas Gleixner]

On Tue, Oct 15 2024 at 20:51, Philipp Stanner wrote:
> +/**
> + * pci_intx - enables/disables PCI INTx for device dev, unmanaged version

mismatch vs. actual function name.

> + * @pdev: the PCI device to operate on
> + * @enable: boolean: whether to enable or disable PCI INTx
> + *
> + * Enables/disables PCI INTx for device @pdev
> + *
> + * This function behavios identically to pci_intx(), but is never managed with
> + * devres.
> + */
> +void pci_intx_unmanaged(struct pci_dev *pdev, int enable)

This is a misnomer. The function controls the INTX_DISABLE bit of a
PCI device. Something like this:

void __pci_intx_control()
{
}

static inline void pci_intx_enable(d)
{
        __pci_intx_control(d, true);
}

.....

makes it entirely clear what this is about.

Hmm?

Thanks,

        tglx

Re: [PATCH 01/13] PCI: Prepare removing devres from pci_intx()

[Philipp Stanner]

On Thu, 2024-10-31 at 14:45 +0100, Thomas Gleixner wrote:
> On Tue, Oct 15 2024 at 20:51, Philipp Stanner wrote:
> > +/**
> > + * pci_intx - enables/disables PCI INTx for device dev, unmanaged
> > version
> 
> mismatch vs. actual function name.

ACK, will fix

> 
> > + * @pdev: the PCI device to operate on
> > + * @enable: boolean: whether to enable or disable PCI INTx
> > + *
> > + * Enables/disables PCI INTx for device @pdev
> > + *
> > + * This function behavios identically to pci_intx(), but is never
> > managed with
> > + * devres.
> > + */
> > +void pci_intx_unmanaged(struct pci_dev *pdev, int enable)
> 
> This is a misnomer. The function controls the INTX_DISABLE bit of a
> PCI device. Something like this:
> 
> void __pci_intx_control()
> {
> }
> 
> static inline void pci_intx_enable(d)
> {
>         __pci_intx_control(d, true);
> }
> 
> .....
> 
> makes it entirely clear what this is about.

Well, I would agree if it were about writing a 'real' new function. But
this is actually about creating a _temporary_ function which is added
here and removed again in patch 12 of this same series.

It wouldn't even be needed; the only reason why it exists is to make it
easy for the driver maintainers concerned by patches 2-11 to review the
change and understand what's going on. Hence it is
"pci_intx_unmanaged()" == "Attention, we take automatic management away
from your driver"

pci_intx() is then fully restored after patch 12 and it keeps its old
name.

Grüße,
Philipp


> 
> Hmm?
> 
> Thanks,
> 
>         tglx
> 

[PATCH 02/13] ALSA: hda_intel: Use always-managed version of pcim_intx()

[Philipp Stanner]

pci_intx() is a hybrid function which can sometimes be managed through
devres. To remove this hybrid nature from pci_intx(), it is necessary to
port users to either an always-managed or a never-managed version.

hda_intel enables its PCI-Device with pcim_enable_device(). Thus, it needs
the always-managed version.

Replace pci_intx() with pcim_intx().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 sound/pci/hda/hda_intel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b4540c5cd2a6..b44ca7b6e54f 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx *chip, int do_disconnect)
 	}
 	bus->irq = chip->pci->irq;
 	chip->card->sync_irq = bus->irq;
-	pci_intx(chip->pci, !chip->msi);
+	pcim_intx(chip->pci, !chip->msi);
 	return 0;
 }
 
-- 
2.47.0

Re: [PATCH 02/13] ALSA: hda_intel: Use always-managed version of pcim_intx()

[Takashi Iwai]

On Tue, 15 Oct 2024 20:51:12 +0200,
Philipp Stanner wrote:
> 
> pci_intx() is a hybrid function which can sometimes be managed through
> devres. To remove this hybrid nature from pci_intx(), it is necessary to
> port users to either an always-managed or a never-managed version.
> 
> hda_intel enables its PCI-Device with pcim_enable_device(). Thus, it needs
> the always-managed version.
> 
> Replace pci_intx() with pcim_intx().
> 
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> ---
>  sound/pci/hda/hda_intel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index b4540c5cd2a6..b44ca7b6e54f 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx *chip, int do_disconnect)
>  	}
>  	bus->irq = chip->pci->irq;
>  	chip->card->sync_irq = bus->irq;
> -	pci_intx(chip->pci, !chip->msi);
> +	pcim_intx(chip->pci, !chip->msi);
>  	return 0;
>  }
>  

Hm, it's OK-ish to do this as it's practically same as what pci_intx()
currently does.  But, the current code can be a bit inconsistent about
the original intx value.  pcim_intx() always stores !enable to
res->orig_intx unconditionally, and it means that the orig_intx value
gets overridden at each time pcim_intx() gets called.

Meanwhile, HD-audio driver does release and re-acquire the interrupt
after disabling MSI when something goes wrong, and pci_intx() call
above is a part of that procedure.  So, it can rewrite the
res->orig_intx to another value by retry without MSI.  And after the
driver removal, it'll lead to another state.

In anyway, as it doesn't change the current behavior, feel free to
take my ack for now:

Acked-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH 02/13] ALSA: hda_intel: Use always-managed version of pcim_intx()

[Philipp Stanner]

On Tue, 2024-10-22 at 16:08 +0200, Takashi Iwai wrote:
> On Tue, 15 Oct 2024 20:51:12 +0200,
> Philipp Stanner wrote:
> > 
> > pci_intx() is a hybrid function which can sometimes be managed
> > through
> > devres. To remove this hybrid nature from pci_intx(), it is
> > necessary to
> > port users to either an always-managed or a never-managed version.
> > 
> > hda_intel enables its PCI-Device with pcim_enable_device(). Thus,
> > it needs
> > the always-managed version.
> > 
> > Replace pci_intx() with pcim_intx().
> > 
> > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > ---
> >  sound/pci/hda/hda_intel.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index b4540c5cd2a6..b44ca7b6e54f 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx *chip,
> > int do_disconnect)
> >  	}
> >  	bus->irq = chip->pci->irq;
> >  	chip->card->sync_irq = bus->irq;
> > -	pci_intx(chip->pci, !chip->msi);
> > +	pcim_intx(chip->pci, !chip->msi);
> >  	return 0;
> >  }
> >  
> 
> Hm, it's OK-ish to do this as it's practically same as what
> pci_intx()
> currently does.  But, the current code can be a bit inconsistent
> about
> the original intx value.  pcim_intx() always stores !enable to
> res->orig_intx unconditionally, and it means that the orig_intx value
> gets overridden at each time pcim_intx() gets called.

Yes.

> 
> Meanwhile, HD-audio driver does release and re-acquire the interrupt
> after disabling MSI when something goes wrong, and pci_intx() call
> above is a part of that procedure.  So, it can rewrite the
> res->orig_intx to another value by retry without MSI.  And after the
> driver removal, it'll lead to another state.

I'm not sure that I understand this paragraph completely. Still, could
a solution for the driver on the long-term just be to use pci_intx()?

> 
> In anyway, as it doesn't change the current behavior, feel free to
> take my ack for now:
> 
> Acked-by: Takashi Iwai <tiwai@suse.de>

Thank you,
P.

> 
> 
> thanks,
> 
> Takashi
> 

Re: [PATCH 02/13] ALSA: hda_intel: Use always-managed version of pcim_intx()

[Takashi Iwai]

On Wed, 23 Oct 2024 15:50:09 +0200,
Philipp Stanner wrote:
> 
> On Tue, 2024-10-22 at 16:08 +0200, Takashi Iwai wrote:
> > On Tue, 15 Oct 2024 20:51:12 +0200,
> > Philipp Stanner wrote:
> > > 
> > > pci_intx() is a hybrid function which can sometimes be managed
> > > through
> > > devres. To remove this hybrid nature from pci_intx(), it is
> > > necessary to
> > > port users to either an always-managed or a never-managed version.
> > > 
> > > hda_intel enables its PCI-Device with pcim_enable_device(). Thus,
> > > it needs
> > > the always-managed version.
> > > 
> > > Replace pci_intx() with pcim_intx().
> > > 
> > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > > ---
> > >  sound/pci/hda/hda_intel.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > index b4540c5cd2a6..b44ca7b6e54f 100644
> > > --- a/sound/pci/hda/hda_intel.c
> > > +++ b/sound/pci/hda/hda_intel.c
> > > @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx *chip,
> > > int do_disconnect)
> > >  	}
> > >  	bus->irq = chip->pci->irq;
> > >  	chip->card->sync_irq = bus->irq;
> > > -	pci_intx(chip->pci, !chip->msi);
> > > +	pcim_intx(chip->pci, !chip->msi);
> > >  	return 0;
> > >  }
> > >  
> > 
> > Hm, it's OK-ish to do this as it's practically same as what
> > pci_intx()
> > currently does.  But, the current code can be a bit inconsistent
> > about
> > the original intx value.  pcim_intx() always stores !enable to
> > res->orig_intx unconditionally, and it means that the orig_intx value
> > gets overridden at each time pcim_intx() gets called.
> 
> Yes.
> 
> > 
> > Meanwhile, HD-audio driver does release and re-acquire the interrupt
> > after disabling MSI when something goes wrong, and pci_intx() call
> > above is a part of that procedure.  So, it can rewrite the
> > res->orig_intx to another value by retry without MSI.  And after the
> > driver removal, it'll lead to another state.
> 
> I'm not sure that I understand this paragraph completely. Still, could
> a solution for the driver on the long-term just be to use pci_intx()?

pci_intx() misses the restore of the original value, so it's no
long-term solution, either.

What I meant is that pcim_intx() blindly assumes the negative of the
passed argument as the original state, which isn't always true.  e.g.
when the driver calls it twice with different values, a wrong value
may be remembered.

That said, I thought of something like below.


thanks,

Takashi

-- 8< --
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -438,8 +438,17 @@ static void pcim_intx_restore(struct device *dev, void *data)
 	__pcim_intx(pdev, res->orig_intx);
 }
 
-static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
+static void save_orig_intx(struct pci_dev *pdev)
 {
+	u16 pci_command;
+
+	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
+	res->orig_intx = !(pci_command & PCI_COMMAND_INTX_DISABLE);
+}
+
+static struct pcim_intx_devres *get_or_create_intx_devres(struct pci_dev *pdev)
+{
+	struct device *dev = &pdev->dev;
 	struct pcim_intx_devres *res;
 
 	res = devres_find(dev, pcim_intx_restore, NULL, NULL);
@@ -447,8 +456,10 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
 		return res;
 
 	res = devres_alloc(pcim_intx_restore, sizeof(*res), GFP_KERNEL);
-	if (res)
+	if (res) {
+		save_orig_intx(pdev);
 		devres_add(dev, res);
+	}
 
 	return res;
 }
@@ -467,11 +478,10 @@ int pcim_intx(struct pci_dev *pdev, int enable)
 {
 	struct pcim_intx_devres *res;
 
-	res = get_or_create_intx_devres(&pdev->dev);
+	res = get_or_create_intx_devres(pdev);
 	if (!res)
 		return -ENOMEM;
 
-	res->orig_intx = !enable;
 	__pcim_intx(pdev, enable);
 
 	return 0;

Re: [PATCH 02/13] ALSA: hda_intel: Use always-managed version of pcim_intx()

[Philipp Stanner]

On Wed, 2024-10-23 at 17:03 +0200, Takashi Iwai wrote:
> On Wed, 23 Oct 2024 15:50:09 +0200,
> Philipp Stanner wrote:
> > 
> > On Tue, 2024-10-22 at 16:08 +0200, Takashi Iwai wrote:
> > > On Tue, 15 Oct 2024 20:51:12 +0200,
> > > Philipp Stanner wrote:
> > > > 
> > > > pci_intx() is a hybrid function which can sometimes be managed
> > > > through
> > > > devres. To remove this hybrid nature from pci_intx(), it is
> > > > necessary to
> > > > port users to either an always-managed or a never-managed
> > > > version.
> > > > 
> > > > hda_intel enables its PCI-Device with pcim_enable_device().
> > > > Thus,
> > > > it needs
> > > > the always-managed version.
> > > > 
> > > > Replace pci_intx() with pcim_intx().
> > > > 
> > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > > > ---
> > > >  sound/pci/hda/hda_intel.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/sound/pci/hda/hda_intel.c
> > > > b/sound/pci/hda/hda_intel.c
> > > > index b4540c5cd2a6..b44ca7b6e54f 100644
> > > > --- a/sound/pci/hda/hda_intel.c
> > > > +++ b/sound/pci/hda/hda_intel.c
> > > > @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx
> > > > *chip,
> > > > int do_disconnect)
> > > >  	}
> > > >  	bus->irq = chip->pci->irq;
> > > >  	chip->card->sync_irq = bus->irq;
> > > > -	pci_intx(chip->pci, !chip->msi);
> > > > +	pcim_intx(chip->pci, !chip->msi);
> > > >  	return 0;
> > > >  }
> > > >  
> > > 
> > > Hm, it's OK-ish to do this as it's practically same as what
> > > pci_intx()
> > > currently does.  But, the current code can be a bit inconsistent
> > > about
> > > the original intx value.  pcim_intx() always stores !enable to
> > > res->orig_intx unconditionally, and it means that the orig_intx
> > > value
> > > gets overridden at each time pcim_intx() gets called.
> > 
> > Yes.
> > 
> > > 
> > > Meanwhile, HD-audio driver does release and re-acquire the
> > > interrupt
> > > after disabling MSI when something goes wrong, and pci_intx()
> > > call
> > > above is a part of that procedure.  So, it can rewrite the
> > > res->orig_intx to another value by retry without MSI.  And after
> > > the
> > > driver removal, it'll lead to another state.
> > 
> > I'm not sure that I understand this paragraph completely. Still,
> > could
> > a solution for the driver on the long-term just be to use
> > pci_intx()?
> 
> pci_intx() misses the restore of the original value, so it's no
> long-term solution, either.

Sure that is missing – I was basically asking whether the driver could
live without that feature.

Consider that point obsolete, see below

> 
> What I meant is that pcim_intx() blindly assumes the negative of the
> passed argument as the original state, which isn't always true.  e.g.
> when the driver calls it twice with different values, a wrong value
> may be remembered.

Ah, I see – thoguh the issue is when it's called several times with the
*same* value, isn't it?

E.g.

pcim_intx(pdev, 1); // 0 is remembered as the old value
pcim_intx(pdev, 1); // 0 is falsely remembered as the old value

Also, it would seem that calling the function for the first time like
that:

pcim_intx(pdev, 0); // old value: 1

is at least incorrect, because INTx should be 0 per default, shouldn't
it? Could then even be a 1st class bug, because INTx would end up being
enabled despite having been disabled all the time.

> 
> That said, I thought of something like below.

At first glance that looks like a good idea to me, thanks for working
this out!

IMO you can submit that as a patch so we can discuss it separately.

Greetings,
Philipp

> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> --- a/drivers/pci/devres.c
> +++ b/drivers/pci/devres.c
> @@ -438,8 +438,17 @@ static void pcim_intx_restore(struct device
> *dev, void *data)
>  	__pcim_intx(pdev, res->orig_intx);
>  }
>  
> -static struct pcim_intx_devres *get_or_create_intx_devres(struct
> device *dev)
> +static void save_orig_intx(struct pci_dev *pdev)
>  {
> +	u16 pci_command;
> +
> +	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
> +	res->orig_intx = !(pci_command & PCI_COMMAND_INTX_DISABLE);
> +}
> +
> +static struct pcim_intx_devres *get_or_create_intx_devres(struct
> pci_dev *pdev)
> +{
> +	struct device *dev = &pdev->dev;
>  	struct pcim_intx_devres *res;
>  
>  	res = devres_find(dev, pcim_intx_restore, NULL, NULL);
> @@ -447,8 +456,10 @@ static struct pcim_intx_devres
> *get_or_create_intx_devres(struct device *dev)
>  		return res;
>  
>  	res = devres_alloc(pcim_intx_restore, sizeof(*res),
> GFP_KERNEL);
> -	if (res)
> +	if (res) {
> +		save_orig_intx(pdev);
>  		devres_add(dev, res);
> +	}
>  
>  	return res;
>  }
> @@ -467,11 +478,10 @@ int pcim_intx(struct pci_dev *pdev, int enable)
>  {
>  	struct pcim_intx_devres *res;
>  
> -	res = get_or_create_intx_devres(&pdev->dev);
> +	res = get_or_create_intx_devres(pdev);
>  	if (!res)
>  		return -ENOMEM;
>  
> -	res->orig_intx = !enable;
>  	__pcim_intx(pdev, enable);
>  
>  	return 0;
> 

Re: [PATCH 02/13] ALSA: hda_intel: Use always-managed version of pcim_intx()

[Takashi Iwai]

On Thu, 24 Oct 2024 10:02:59 +0200,
Philipp Stanner wrote:
> 
> On Wed, 2024-10-23 at 17:03 +0200, Takashi Iwai wrote:
> > On Wed, 23 Oct 2024 15:50:09 +0200,
> > Philipp Stanner wrote:
> > > 
> > > On Tue, 2024-10-22 at 16:08 +0200, Takashi Iwai wrote:
> > > > On Tue, 15 Oct 2024 20:51:12 +0200,
> > > > Philipp Stanner wrote:
> > > > > 
> > > > > pci_intx() is a hybrid function which can sometimes be managed
> > > > > through
> > > > > devres. To remove this hybrid nature from pci_intx(), it is
> > > > > necessary to
> > > > > port users to either an always-managed or a never-managed
> > > > > version.
> > > > > 
> > > > > hda_intel enables its PCI-Device with pcim_enable_device().
> > > > > Thus,
> > > > > it needs
> > > > > the always-managed version.
> > > > > 
> > > > > Replace pci_intx() with pcim_intx().
> > > > > 
> > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > > > > ---
> > > > >  sound/pci/hda/hda_intel.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/sound/pci/hda/hda_intel.c
> > > > > b/sound/pci/hda/hda_intel.c
> > > > > index b4540c5cd2a6..b44ca7b6e54f 100644
> > > > > --- a/sound/pci/hda/hda_intel.c
> > > > > +++ b/sound/pci/hda/hda_intel.c
> > > > > @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx
> > > > > *chip,
> > > > > int do_disconnect)
> > > > >  	}
> > > > >  	bus->irq = chip->pci->irq;
> > > > >  	chip->card->sync_irq = bus->irq;
> > > > > -	pci_intx(chip->pci, !chip->msi);
> > > > > +	pcim_intx(chip->pci, !chip->msi);
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > 
> > > > Hm, it's OK-ish to do this as it's practically same as what
> > > > pci_intx()
> > > > currently does.  But, the current code can be a bit inconsistent
> > > > about
> > > > the original intx value.  pcim_intx() always stores !enable to
> > > > res->orig_intx unconditionally, and it means that the orig_intx
> > > > value
> > > > gets overridden at each time pcim_intx() gets called.
> > > 
> > > Yes.
> > > 
> > > > 
> > > > Meanwhile, HD-audio driver does release and re-acquire the
> > > > interrupt
> > > > after disabling MSI when something goes wrong, and pci_intx()
> > > > call
> > > > above is a part of that procedure.  So, it can rewrite the
> > > > res->orig_intx to another value by retry without MSI.  And after
> > > > the
> > > > driver removal, it'll lead to another state.
> > > 
> > > I'm not sure that I understand this paragraph completely. Still,
> > > could
> > > a solution for the driver on the long-term just be to use
> > > pci_intx()?
> > 
> > pci_intx() misses the restore of the original value, so it's no
> > long-term solution, either.
> 
> Sure that is missing – I was basically asking whether the driver could
> live without that feature.
> 
> Consider that point obsolete, see below
> 
> > 
> > What I meant is that pcim_intx() blindly assumes the negative of the
> > passed argument as the original state, which isn't always true.  e.g.
> > when the driver calls it twice with different values, a wrong value
> > may be remembered.
> 
> Ah, I see – thoguh the issue is when it's called several times with the
> *same* value, isn't it?
> 
> E.g.
> 
> pcim_intx(pdev, 1); // 0 is remembered as the old value
> pcim_intx(pdev, 1); // 0 is falsely remembered as the old value
> 
> Also, it would seem that calling the function for the first time like
> that:
> 
> pcim_intx(pdev, 0); // old value: 1
> 
> is at least incorrect, because INTx should be 0 per default, shouldn't
> it? Could then even be a 1st class bug, because INTx would end up being
> enabled despite having been disabled all the time.

Yeah, and the unexpected restore can happen even with a single call of
pcim_intx(), if the driver calls it unnecessarily.

> > That said, I thought of something like below.
> 
> At first glance that looks like a good idea to me, thanks for working
> this out!
> 
> IMO you can submit that as a patch so we can discuss it separately.

Sure, I'm going to submit later.


thanks,

Takashi

Re: [PATCH 02/13] ALSA: hda_intel: Use always-managed version of pcim_intx()

[Philipp Stanner]

On Thu, 2024-10-24 at 17:43 +0200, Takashi Iwai wrote:
> On Thu, 24 Oct 2024 10:02:59 +0200,
> Philipp Stanner wrote:
> > 
> > On Wed, 2024-10-23 at 17:03 +0200, Takashi Iwai wrote:
> > > On Wed, 23 Oct 2024 15:50:09 +0200,
> > > Philipp Stanner wrote:
> > > > 
> > > > On Tue, 2024-10-22 at 16:08 +0200, Takashi Iwai wrote:
> > > > > On Tue, 15 Oct 2024 20:51:12 +0200,
> > > > > Philipp Stanner wrote:
> > > > > > 
> > > > > > pci_intx() is a hybrid function which can sometimes be
> > > > > > managed
> > > > > > through
> > > > > > devres. To remove this hybrid nature from pci_intx(), it is
> > > > > > necessary to
> > > > > > port users to either an always-managed or a never-managed
> > > > > > version.
> > > > > > 
> > > > > > hda_intel enables its PCI-Device with pcim_enable_device().
> > > > > > Thus,
> > > > > > it needs
> > > > > > the always-managed version.
> > > > > > 
> > > > > > Replace pci_intx() with pcim_intx().
> > > > > > 
> > > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > > > > > ---
> > > > > >  sound/pci/hda/hda_intel.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/sound/pci/hda/hda_intel.c
> > > > > > b/sound/pci/hda/hda_intel.c
> > > > > > index b4540c5cd2a6..b44ca7b6e54f 100644
> > > > > > --- a/sound/pci/hda/hda_intel.c
> > > > > > +++ b/sound/pci/hda/hda_intel.c
> > > > > > @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx
> > > > > > *chip,
> > > > > > int do_disconnect)
> > > > > >  	}
> > > > > >  	bus->irq = chip->pci->irq;
> > > > > >  	chip->card->sync_irq = bus->irq;
> > > > > > -	pci_intx(chip->pci, !chip->msi);
> > > > > > +	pcim_intx(chip->pci, !chip->msi);
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  
> > > > > 
> > > > > Hm, it's OK-ish to do this as it's practically same as what
> > > > > pci_intx()
> > > > > currently does.  But, the current code can be a bit
> > > > > inconsistent
> > > > > about
> > > > > the original intx value.  pcim_intx() always stores !enable
> > > > > to
> > > > > res->orig_intx unconditionally, and it means that the
> > > > > orig_intx
> > > > > value
> > > > > gets overridden at each time pcim_intx() gets called.
> > > > 
> > > > Yes.
> > > > 
> > > > > 
> > > > > Meanwhile, HD-audio driver does release and re-acquire the
> > > > > interrupt
> > > > > after disabling MSI when something goes wrong, and pci_intx()
> > > > > call
> > > > > above is a part of that procedure.  So, it can rewrite the
> > > > > res->orig_intx to another value by retry without MSI.  And
> > > > > after
> > > > > the
> > > > > driver removal, it'll lead to another state.
> > > > 
> > > > I'm not sure that I understand this paragraph completely.
> > > > Still,
> > > > could
> > > > a solution for the driver on the long-term just be to use
> > > > pci_intx()?
> > > 
> > > pci_intx() misses the restore of the original value, so it's no
> > > long-term solution, either.
> > 
> > Sure that is missing – I was basically asking whether the driver
> > could
> > live without that feature.
> > 
> > Consider that point obsolete, see below
> > 
> > > 
> > > What I meant is that pcim_intx() blindly assumes the negative of
> > > the
> > > passed argument as the original state, which isn't always true. 
> > > e.g.
> > > when the driver calls it twice with different values, a wrong
> > > value
> > > may be remembered.
> > 
> > Ah, I see – thoguh the issue is when it's called several times with
> > the
> > *same* value, isn't it?
> > 
> > E.g.
> > 
> > pcim_intx(pdev, 1); // 0 is remembered as the old value
> > pcim_intx(pdev, 1); // 0 is falsely remembered as the old value
> > 
> > Also, it would seem that calling the function for the first time
> > like
> > that:
> > 
> > pcim_intx(pdev, 0); // old value: 1
> > 
> > is at least incorrect, because INTx should be 0 per default,
> > shouldn't
> > it? Could then even be a 1st class bug, because INTx would end up
> > being
> > enabled despite having been disabled all the time.
> 
> Yeah, and the unexpected restore can happen even with a single call
> of
> pcim_intx(), if the driver calls it unnecessarily.
> 
> > > That said, I thought of something like below.
> > 
> > At first glance that looks like a good idea to me, thanks for
> > working
> > this out!
> > 
> > IMO you can submit that as a patch so we can discuss it separately.
> 
> Sure, I'm going to submit later.

I just took a look into the old implementation of pci_intx() (there was
no pcim_intx() back then), before I started cleaning up PCI's devres.
This what it looked like before
25216afc9db53d85dc648aba8fb7f6d31f2c8731:

void pci_intx(struct pci_dev *pdev, int enable)
{
	u16 pci_command, new;

	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);

	if (enable)
		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
	else
		new = pci_command | PCI_COMMAND_INTX_DISABLE;

	if (new != pci_command) {
		struct pci_devres *dr;

		pci_write_config_word(pdev, PCI_COMMAND, new);

		dr = find_pci_dr(pdev);
		if (dr && !dr->restore_intx) {
			dr->restore_intx = 1;
			dr->orig_intx = !enable;
		}
	}
}
EXPORT_SYMBOL_GPL(pci_intx);

If I'm not mistaken the old version did not have the problem because
the value to be restored only changed if new != pci_command.

That should always be correct, what do you think?

If so, only my commit 25216afc9db53d85dc648aba8fb7f6d31f2c8731 needs to
be fixed.

Thanks,
P.


> 
> 
> thanks,
> 
> Takashi
> 

Re: [PATCH 02/13] ALSA: hda_intel: Use always-managed version of pcim_intx()

[Takashi Iwai]

On Fri, 25 Oct 2024 10:37:57 +0200,
Philipp Stanner wrote:
> 
> On Thu, 2024-10-24 at 17:43 +0200, Takashi Iwai wrote:
> > On Thu, 24 Oct 2024 10:02:59 +0200,
> > Philipp Stanner wrote:
> > > 
> > > On Wed, 2024-10-23 at 17:03 +0200, Takashi Iwai wrote:
> > > > On Wed, 23 Oct 2024 15:50:09 +0200,
> > > > Philipp Stanner wrote:
> > > > > 
> > > > > On Tue, 2024-10-22 at 16:08 +0200, Takashi Iwai wrote:
> > > > > > On Tue, 15 Oct 2024 20:51:12 +0200,
> > > > > > Philipp Stanner wrote:
> > > > > > > 
> > > > > > > pci_intx() is a hybrid function which can sometimes be
> > > > > > > managed
> > > > > > > through
> > > > > > > devres. To remove this hybrid nature from pci_intx(), it is
> > > > > > > necessary to
> > > > > > > port users to either an always-managed or a never-managed
> > > > > > > version.
> > > > > > > 
> > > > > > > hda_intel enables its PCI-Device with pcim_enable_device().
> > > > > > > Thus,
> > > > > > > it needs
> > > > > > > the always-managed version.
> > > > > > > 
> > > > > > > Replace pci_intx() with pcim_intx().
> > > > > > > 
> > > > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > > > > > > ---
> > > > > > >  sound/pci/hda/hda_intel.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/sound/pci/hda/hda_intel.c
> > > > > > > b/sound/pci/hda/hda_intel.c
> > > > > > > index b4540c5cd2a6..b44ca7b6e54f 100644
> > > > > > > --- a/sound/pci/hda/hda_intel.c
> > > > > > > +++ b/sound/pci/hda/hda_intel.c
> > > > > > > @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx
> > > > > > > *chip,
> > > > > > > int do_disconnect)
> > > > > > >  	}
> > > > > > >  	bus->irq = chip->pci->irq;
> > > > > > >  	chip->card->sync_irq = bus->irq;
> > > > > > > -	pci_intx(chip->pci, !chip->msi);
> > > > > > > +	pcim_intx(chip->pci, !chip->msi);
> > > > > > >  	return 0;
> > > > > > >  }
> > > > > > >  
> > > > > > 
> > > > > > Hm, it's OK-ish to do this as it's practically same as what
> > > > > > pci_intx()
> > > > > > currently does.  But, the current code can be a bit
> > > > > > inconsistent
> > > > > > about
> > > > > > the original intx value.  pcim_intx() always stores !enable
> > > > > > to
> > > > > > res->orig_intx unconditionally, and it means that the
> > > > > > orig_intx
> > > > > > value
> > > > > > gets overridden at each time pcim_intx() gets called.
> > > > > 
> > > > > Yes.
> > > > > 
> > > > > > 
> > > > > > Meanwhile, HD-audio driver does release and re-acquire the
> > > > > > interrupt
> > > > > > after disabling MSI when something goes wrong, and pci_intx()
> > > > > > call
> > > > > > above is a part of that procedure.  So, it can rewrite the
> > > > > > res->orig_intx to another value by retry without MSI.  And
> > > > > > after
> > > > > > the
> > > > > > driver removal, it'll lead to another state.
> > > > > 
> > > > > I'm not sure that I understand this paragraph completely.
> > > > > Still,
> > > > > could
> > > > > a solution for the driver on the long-term just be to use
> > > > > pci_intx()?
> > > > 
> > > > pci_intx() misses the restore of the original value, so it's no
> > > > long-term solution, either.
> > > 
> > > Sure that is missing – I was basically asking whether the driver
> > > could
> > > live without that feature.
> > > 
> > > Consider that point obsolete, see below
> > > 
> > > > 
> > > > What I meant is that pcim_intx() blindly assumes the negative of
> > > > the
> > > > passed argument as the original state, which isn't always true. 
> > > > e.g.
> > > > when the driver calls it twice with different values, a wrong
> > > > value
> > > > may be remembered.
> > > 
> > > Ah, I see – thoguh the issue is when it's called several times with
> > > the
> > > *same* value, isn't it?
> > > 
> > > E.g.
> > > 
> > > pcim_intx(pdev, 1); // 0 is remembered as the old value
> > > pcim_intx(pdev, 1); // 0 is falsely remembered as the old value
> > > 
> > > Also, it would seem that calling the function for the first time
> > > like
> > > that:
> > > 
> > > pcim_intx(pdev, 0); // old value: 1
> > > 
> > > is at least incorrect, because INTx should be 0 per default,
> > > shouldn't
> > > it? Could then even be a 1st class bug, because INTx would end up
> > > being
> > > enabled despite having been disabled all the time.
> > 
> > Yeah, and the unexpected restore can happen even with a single call
> > of
> > pcim_intx(), if the driver calls it unnecessarily.
> > 
> > > > That said, I thought of something like below.
> > > 
> > > At first glance that looks like a good idea to me, thanks for
> > > working
> > > this out!
> > > 
> > > IMO you can submit that as a patch so we can discuss it separately.
> > 
> > Sure, I'm going to submit later.
> 
> I just took a look into the old implementation of pci_intx() (there was
> no pcim_intx() back then), before I started cleaning up PCI's devres.
> This what it looked like before
> 25216afc9db53d85dc648aba8fb7f6d31f2c8731:
> 
> void pci_intx(struct pci_dev *pdev, int enable)
> {
> 	u16 pci_command, new;
> 
> 	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
> 
> 	if (enable)
> 		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
> 	else
> 		new = pci_command | PCI_COMMAND_INTX_DISABLE;
> 
> 	if (new != pci_command) {
> 		struct pci_devres *dr;
> 
> 		pci_write_config_word(pdev, PCI_COMMAND, new);
> 
> 		dr = find_pci_dr(pdev);
> 		if (dr && !dr->restore_intx) {
> 			dr->restore_intx = 1;
> 			dr->orig_intx = !enable;
> 		}
> 	}
> }
> EXPORT_SYMBOL_GPL(pci_intx);
> 
> If I'm not mistaken the old version did not have the problem because
> the value to be restored only changed if new != pci_command.
> 
> That should always be correct, what do you think?
> 
> If so, only my commit 25216afc9db53d85dc648aba8fb7f6d31f2c8731 needs to
> be fixed.

Yes, it looks so.  Fortunately my submitted patch pointed to the right
Fixes tag :)


thanks,

Takashi

[PATCH 03/13] drivers/xen: Use never-managed version of pci_intx()

[Philipp Stanner]

pci_intx() is a hybrid function which can sometimes be managed through
devres. To remove this hybrid nature from pci_intx(), it is necessary to
port users to either an always-managed or a never-managed version.

xen enables its PCI-Device with pci_enable_device(). Thus, it
needs the never-managed version.

Replace pci_intx() with pci_intx_unmanaged().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Acked-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xen-pciback/conf_space_header.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/xen-pciback/conf_space_header.c b/drivers/xen/xen-pciback/conf_space_header.c
index fc0332645966..8d26d64232e8 100644
--- a/drivers/xen/xen-pciback/conf_space_header.c
+++ b/drivers/xen/xen-pciback/conf_space_header.c
@@ -106,7 +106,7 @@ static int command_write(struct pci_dev *dev, int offset, u16 value, void *data)
 
 	if (dev_data && dev_data->allow_interrupt_control &&
 	    ((cmd->val ^ value) & PCI_COMMAND_INTX_DISABLE))
-		pci_intx(dev, !(value & PCI_COMMAND_INTX_DISABLE));
+		pci_intx_unmanaged(dev, !(value & PCI_COMMAND_INTX_DISABLE));
 
 	cmd->val = value;
 
-- 
2.47.0

[PATCH 04/13] net/ethernet: Use never-managed version of pci_intx()

[Philipp Stanner]

pci_intx() is a hybrid function which can sometimes be managed through
devres. To remove this hybrid nature from pci_intx(), it is necessary to
port users to either an always-managed or a never-managed version.

broadcom/bnx2x and brocade/bna enable their PCI-Device with
pci_enable_device(). Thus, they need the never-managed version.

Replace pci_intx() with pci_intx_unmanaged().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 2 +-
 drivers/net/ethernet/brocade/bna/bnad.c          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 678829646cec..2ae63d6e6792 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -1669,7 +1669,7 @@ static void bnx2x_igu_int_enable(struct bnx2x *bp)
 	REG_WR(bp, IGU_REG_PF_CONFIGURATION, val);
 
 	if (val & IGU_PF_CONF_INT_LINE_EN)
-		pci_intx(bp->pdev, true);
+		pci_intx_unmanaged(bp->pdev, true);
 
 	barrier();
 
diff --git a/drivers/net/ethernet/brocade/bna/bnad.c b/drivers/net/ethernet/brocade/bna/bnad.c
index ece6f3b48327..2b37462d406e 100644
--- a/drivers/net/ethernet/brocade/bna/bnad.c
+++ b/drivers/net/ethernet/brocade/bna/bnad.c
@@ -2669,7 +2669,7 @@ bnad_enable_msix(struct bnad *bnad)
 		}
 	}
 
-	pci_intx(bnad->pcidev, 0);
+	pci_intx_unmanaged(bnad->pcidev, 0);
 
 	return;
 
-- 
2.47.0

[PATCH 05/13] net/ntb: Use never-managed version of pci_intx()

[Philipp Stanner]

pci_intx() is a hybrid function which can sometimes be managed through
devres. To remove this hybrid nature from pci_intx(), it is necessary to
port users to either an always-managed or a never-managed version.

hw/amd and how/intel enable their PCI-Device with pci_enable_device().
Thus, they need the never-managed version.

Replace pci_intx() with pci_intx_unmanaged().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> #for ntb_hw_amd.c
---
 drivers/ntb/hw/amd/ntb_hw_amd.c    | 4 ++--
 drivers/ntb/hw/intel/ntb_hw_gen1.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c
index d687e8c2cc78..b146f170e839 100644
--- a/drivers/ntb/hw/amd/ntb_hw_amd.c
+++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
@@ -791,7 +791,7 @@ static int ndev_init_isr(struct amd_ntb_dev *ndev,
 err_msi_enable:
 
 	/* Try to set up intx irq */
-	pci_intx(pdev, 1);
+	pci_intx_unmanaged(pdev, 1);
 
 	rc = request_irq(pdev->irq, ndev_irq_isr, IRQF_SHARED,
 			 "ndev_irq_isr", ndev);
@@ -831,7 +831,7 @@ static void ndev_deinit_isr(struct amd_ntb_dev *ndev)
 		if (pci_dev_msi_enabled(pdev))
 			pci_disable_msi(pdev);
 		else
-			pci_intx(pdev, 0);
+			pci_intx_unmanaged(pdev, 0);
 	}
 }
 
diff --git a/drivers/ntb/hw/intel/ntb_hw_gen1.c b/drivers/ntb/hw/intel/ntb_hw_gen1.c
index 079b8cd79785..9ad9d7fe227e 100644
--- a/drivers/ntb/hw/intel/ntb_hw_gen1.c
+++ b/drivers/ntb/hw/intel/ntb_hw_gen1.c
@@ -445,7 +445,7 @@ int ndev_init_isr(struct intel_ntb_dev *ndev,
 
 	/* Try to set up intx irq */
 
-	pci_intx(pdev, 1);
+	pci_intx_unmanaged(pdev, 1);
 
 	rc = request_irq(pdev->irq, ndev_irq_isr, IRQF_SHARED,
 			 "ndev_irq_isr", ndev);
-- 
2.47.0

[PATCH 06/13] misc: Use never-managed version of pci_intx()

[Philipp Stanner]

pci_intx() is a hybrid function which can sometimes be managed through
devres. To remove this hybrid nature from pci_intx(), it is necessary to
port users to either an always-managed or a never-managed version.

cardreader/rtsx_pcr.c and tifm_7xx1.c enable their PCI-Device with
pci_enable_device(). Thus, they need the never-managed version.

Replace pci_intx() with pci_intx_unmanaged().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/misc/cardreader/rtsx_pcr.c | 2 +-
 drivers/misc/tifm_7xx1.c           | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/cardreader/rtsx_pcr.c b/drivers/misc/cardreader/rtsx_pcr.c
index be3d4e0e50cc..e25e6d560dd7 100644
--- a/drivers/misc/cardreader/rtsx_pcr.c
+++ b/drivers/misc/cardreader/rtsx_pcr.c
@@ -1057,7 +1057,7 @@ static int rtsx_pci_acquire_irq(struct rtsx_pcr *pcr)
 	}
 
 	pcr->irq = pcr->pci->irq;
-	pci_intx(pcr->pci, !pcr->msi_en);
+	pci_intx_unmanaged(pcr->pci, !pcr->msi_en);
 
 	return 0;
 }
diff --git a/drivers/misc/tifm_7xx1.c b/drivers/misc/tifm_7xx1.c
index 1d54680d6ed2..5f9c7ccae8d2 100644
--- a/drivers/misc/tifm_7xx1.c
+++ b/drivers/misc/tifm_7xx1.c
@@ -327,7 +327,7 @@ static int tifm_7xx1_probe(struct pci_dev *dev,
 		goto err_out;
 	}
 
-	pci_intx(dev, 1);
+	pci_intx_unmanaged(dev, 1);
 
 	fm = tifm_alloc_adapter(dev->device == PCI_DEVICE_ID_TI_XX21_XX11_FM
 				? 4 : 2, &dev->dev);
@@ -368,7 +368,7 @@ static int tifm_7xx1_probe(struct pci_dev *dev,
 err_out_free:
 	tifm_free_adapter(fm);
 err_out_int:
-	pci_intx(dev, 0);
+	pci_intx_unmanaged(dev, 0);
 	pci_release_regions(dev);
 err_out:
 	if (!pci_dev_busy)
@@ -392,7 +392,7 @@ static void tifm_7xx1_remove(struct pci_dev *dev)
 		tifm_7xx1_sock_power_off(tifm_7xx1_sock_addr(fm->addr, cnt));
 
 	iounmap(fm->addr);
-	pci_intx(dev, 0);
+	pci_intx_unmanaged(dev, 0);
 	pci_release_regions(dev);
 
 	pci_disable_device(dev);
-- 
2.47.0

[PATCH 07/13] vfio/pci: Use never-managed version of pci_intx()

[Philipp Stanner]

pci_intx() is a hybrid function which can sometimes be managed through
devres. To remove this hybrid nature from pci_intx(), it is necessary to
port users to either an always-managed or a never-managed version.

vfio enables its PCI-Device with pci_enable_device(). Thus, it
needs the never-managed version.

Replace pci_intx() with pci_intx_unmanaged().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/vfio/pci/vfio_pci_core.c  |  2 +-
 drivers/vfio/pci/vfio_pci_intrs.c | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 1ab58da9f38a..90240c8d51aa 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -498,7 +498,7 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
 		if (vfio_pci_nointx(pdev)) {
 			pci_info(pdev, "Masking broken INTx support\n");
 			vdev->nointx = true;
-			pci_intx(pdev, 0);
+			pci_intx_unmanaged(pdev, 0);
 		} else
 			vdev->pci_2_3 = pci_intx_mask_supported(pdev);
 	}
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 8382c5834335..40abb0b937a2 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -118,7 +118,7 @@ static bool __vfio_pci_intx_mask(struct vfio_pci_core_device *vdev)
 	 */
 	if (unlikely(!is_intx(vdev))) {
 		if (vdev->pci_2_3)
-			pci_intx(pdev, 0);
+			pci_intx_unmanaged(pdev, 0);
 		goto out_unlock;
 	}
 
@@ -132,7 +132,7 @@ static bool __vfio_pci_intx_mask(struct vfio_pci_core_device *vdev)
 		 * mask, not just when something is pending.
 		 */
 		if (vdev->pci_2_3)
-			pci_intx(pdev, 0);
+			pci_intx_unmanaged(pdev, 0);
 		else
 			disable_irq_nosync(pdev->irq);
 
@@ -178,7 +178,7 @@ static int vfio_pci_intx_unmask_handler(void *opaque, void *data)
 	 */
 	if (unlikely(!is_intx(vdev))) {
 		if (vdev->pci_2_3)
-			pci_intx(pdev, 1);
+			pci_intx_unmanaged(pdev, 1);
 		goto out_unlock;
 	}
 
@@ -296,7 +296,7 @@ static int vfio_intx_enable(struct vfio_pci_core_device *vdev,
 	 */
 	ctx->masked = vdev->virq_disabled;
 	if (vdev->pci_2_3) {
-		pci_intx(pdev, !ctx->masked);
+		pci_intx_unmanaged(pdev, !ctx->masked);
 		irqflags = IRQF_SHARED;
 	} else {
 		irqflags = ctx->masked ? IRQF_NO_AUTOEN : 0;
@@ -569,7 +569,7 @@ static void vfio_msi_disable(struct vfio_pci_core_device *vdev, bool msix)
 	 * via their shutdown paths.  Restore for NoINTx devices.
 	 */
 	if (vdev->nointx)
-		pci_intx(pdev, 0);
+		pci_intx_unmanaged(pdev, 0);
 
 	vdev->irq_type = VFIO_PCI_NUM_IRQS;
 }
-- 
2.47.0

[PATCH 08/13] PCI: MSI: Use never-managed version of pci_intx()

[Philipp Stanner]

pci_intx() is a hybrid function which can sometimes be managed through
devres. To remove this hybrid nature from pci_intx(), it is necessary to
port users to either an always-managed or a never-managed version.

MSI sets up its own separate devres callback implicitly in
pcim_setup_msi_release(). This callback ultimately uses pci_intx(),
which is problematic since the callback of course runs on driver-detach.

That problem has last been described here:
https://lore.kernel.org/all/ee44ea7ac760e73edad3f20b30b4d2fff66c1a85.camel@redhat.com/

Replace the call to pci_intx() with one to the never-managed version
pci_intx_unmanaged().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/pci/msi/api.c | 2 +-
 drivers/pci/msi/msi.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/msi/api.c b/drivers/pci/msi/api.c
index b956ce591f96..c95e2e7dc9ab 100644
--- a/drivers/pci/msi/api.c
+++ b/drivers/pci/msi/api.c
@@ -289,7 +289,7 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
 			 */
 			if (affd)
 				irq_create_affinity_masks(1, affd);
-			pci_intx(dev, 1);
+			pci_intx_unmanaged(dev, 1);
 			return 1;
 		}
 	}
diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
index 3a45879d85db..53f13b09db50 100644
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -268,7 +268,7 @@ EXPORT_SYMBOL_GPL(pci_write_msi_msg);
 static void pci_intx_for_msi(struct pci_dev *dev, int enable)
 {
 	if (!(dev->dev_flags & PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG))
-		pci_intx(dev, enable);
+		pci_intx_unmanaged(dev, enable);
 }
 
 static void pci_msi_set_enable(struct pci_dev *dev, int enable)
-- 
2.47.0

[PATCH 09/13] ata: Use always-managed version of pci_intx()

[Philipp Stanner]

pci_intx() is a hybrid function which can sometimes be managed through
devres. To remove this hybrid nature from pci_intx(), it is necessary to
port users to either an always-managed or a never-managed version.

All users in ata enable their PCI-Device with pcim_enable_device(). Thus,
they need the always-managed version.

Replace pci_intx() with pcim_intx().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 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 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 45f63b09828a..9273ff3d4732 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1985,7 +1985,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	if (ahci_init_msi(pdev, n_ports, hpriv) < 0) {
 		/* legacy intx interrupts */
-		pci_intx(pdev, 1);
+		pcim_intx(pdev, 1);
 	}
 	hpriv->irq = pci_irq_vector(pdev, 0);
 
diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 093b940bc953..d441246fa357 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -1725,7 +1725,7 @@ static int piix_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 * message-signalled interrupts currently).
 	 */
 	if (port_flags & PIIX_FLAG_CHECKINTR)
-		pci_intx(pdev, 1);
+		pcim_intx(pdev, 1);
 
 	if (piix_check_450nx_errata(pdev)) {
 		/* This writes into the master table but it does not
diff --git a/drivers/ata/pata_rdc.c b/drivers/ata/pata_rdc.c
index 0a9689862f71..09792aac7f9d 100644
--- a/drivers/ata/pata_rdc.c
+++ b/drivers/ata/pata_rdc.c
@@ -340,7 +340,7 @@ static int rdc_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		return rc;
 	host->private_data = hpriv;
 
-	pci_intx(pdev, 1);
+	pcim_intx(pdev, 1);
 
 	host->flags |= ATA_HOST_PARALLEL_SCAN;
 
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index 72c03cbdaff4..b771ebd41252 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -1317,7 +1317,7 @@ static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	if (sata_sil24_msi && !pci_enable_msi(pdev)) {
 		dev_info(&pdev->dev, "Using MSI\n");
-		pci_intx(pdev, 0);
+		pcim_intx(pdev, 0);
 	}
 
 	pci_set_master(pdev);
diff --git a/drivers/ata/sata_sis.c b/drivers/ata/sata_sis.c
index ef8724986de3..b8b6d9eff3b8 100644
--- a/drivers/ata/sata_sis.c
+++ b/drivers/ata/sata_sis.c
@@ -290,7 +290,7 @@ static int sis_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	}
 
 	pci_set_master(pdev);
-	pci_intx(pdev, 1);
+	pcim_intx(pdev, 1);
 	return ata_host_activate(host, pdev->irq, ata_bmdma_interrupt,
 				 IRQF_SHARED, &sis_sht);
 }
diff --git a/drivers/ata/sata_uli.c b/drivers/ata/sata_uli.c
index 60ea45926cd1..52894ff49dcb 100644
--- a/drivers/ata/sata_uli.c
+++ b/drivers/ata/sata_uli.c
@@ -221,7 +221,7 @@ static int uli_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	}
 
 	pci_set_master(pdev);
-	pci_intx(pdev, 1);
+	pcim_intx(pdev, 1);
 	return ata_host_activate(host, pdev->irq, ata_bmdma_interrupt,
 				 IRQF_SHARED, &uli_sht);
 }
diff --git a/drivers/ata/sata_vsc.c b/drivers/ata/sata_vsc.c
index d39b87537168..a53a2dfc1e17 100644
--- a/drivers/ata/sata_vsc.c
+++ b/drivers/ata/sata_vsc.c
@@ -384,7 +384,7 @@ static int vsc_sata_init_one(struct pci_dev *pdev,
 		pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
 
 	if (pci_enable_msi(pdev) == 0)
-		pci_intx(pdev, 0);
+		pcim_intx(pdev, 0);
 
 	/*
 	 * Config offset 0x98 is "Extended Control and Status Register 0"
-- 
2.47.0

Re: [PATCH 09/13] ata: Use always-managed version of pci_intx()

[Sergey Shtylyov]

On 10/15/24 9:51 PM, Philipp Stanner wrote:

> pci_intx() is a hybrid function which can sometimes be managed through
> devres. To remove this hybrid nature from pci_intx(), it is necessary to
> port users to either an always-managed or a never-managed version.
> 
> All users in ata enable their PCI-Device with pcim_enable_device(). Thus,
> they need the always-managed version.
> 
> Replace pci_intx() with pcim_intx().
> 
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

Re: [PATCH 09/13] ata: Use always-managed version of pci_intx()

[Niklas Cassel]

On Tue, Oct 15, 2024 at 08:51:19PM +0200, Philipp Stanner wrote:
> pci_intx() is a hybrid function which can sometimes be managed through
> devres. To remove this hybrid nature from pci_intx(), it is necessary to
> port users to either an always-managed or a never-managed version.
> 
> All users in ata enable their PCI-Device with pcim_enable_device(). Thus,
> they need the always-managed version.
> 
> Replace pci_intx() with pcim_intx().
> 
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> ---

Acked-by: Niklas Cassel <cassel@kernel.org>

[PATCH 10/13] wifi: qtnfmac: use always-managed version of pcim_intx()

[Philipp Stanner]

pci_intx() is a hybrid function which can sometimes be managed through
devres. To remove this hybrid nature from pci_intx(), it is necessary to
port users to either an always-managed or a never-managed version.

qtnfmac enables its PCI-Device with pcim_enable_device(). Thus, it needs
the always-managed version.

Replace pci_intx() with pcim_intx().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/net/wireless/quantenna/qtnfmac/pcie/pcie.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/quantenna/qtnfmac/pcie/pcie.c b/drivers/net/wireless/quantenna/qtnfmac/pcie/pcie.c
index f66eb43094d4..3adcfac2886f 100644
--- a/drivers/net/wireless/quantenna/qtnfmac/pcie/pcie.c
+++ b/drivers/net/wireless/quantenna/qtnfmac/pcie/pcie.c
@@ -204,7 +204,7 @@ static void qtnf_pcie_init_irq(struct qtnf_pcie_bus_priv *priv, bool use_msi)
 
 	if (!priv->msi_enabled) {
 		pr_warn("legacy PCIE interrupts enabled\n");
-		pci_intx(pdev, 1);
+		pcim_intx(pdev, 1);
 	}
 }
 
-- 
2.47.0

Re: [PATCH 10/13] wifi: qtnfmac: use always-managed version of pcim_intx()

[Kalle Valo]

Philipp Stanner <pstanner@redhat.com> writes:

> pci_intx() is a hybrid function which can sometimes be managed through
> devres. To remove this hybrid nature from pci_intx(), it is necessary to
> port users to either an always-managed or a never-managed version.
>
> qtnfmac enables its PCI-Device with pcim_enable_device(). Thus, it needs
> the always-managed version.
>
> Replace pci_intx() with pcim_intx().
>
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>

Feel free to take this via the PCI tree:

Acked-by: Kalle Valo <kvalo@kernel.org>

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH 11/13] HID: amd_sfh: Use always-managed version of pcim_intx()

[Philipp Stanner]

pci_intx() is a hybrid function which can sometimes be managed through
devres. To remove this hybrid nature from pci_intx(), it is necessary to
port users to either an always-managed or a never-managed version.

All users of amd_mp2_pci_remove(), where pci_intx() is used, call
pcim_enable_device(), which is why the driver needs the always-managed
version.

Replace pci_intx() with pcim_intx().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Acked-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c        | 4 ++--
 drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index 0c28ca349bcd..48cfd0c58241 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
@@ -122,7 +122,7 @@ int amd_sfh_irq_init_v2(struct amd_mp2_dev *privdata)
 {
 	int rc;
 
-	pci_intx(privdata->pdev, true);
+	pcim_intx(privdata->pdev, true);
 
 	rc = devm_request_irq(&privdata->pdev->dev, privdata->pdev->irq,
 			      amd_sfh_irq_handler, 0, DRIVER_NAME, privdata);
@@ -248,7 +248,7 @@ static void amd_mp2_pci_remove(void *privdata)
 	struct amd_mp2_dev *mp2 = privdata;
 	amd_sfh_hid_client_deinit(privdata);
 	mp2->mp2_ops->stop_all(mp2);
-	pci_intx(mp2->pdev, false);
+	pcim_intx(mp2->pdev, false);
 	amd_sfh_clear_intr(mp2);
 }
 
diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
index db36d87d5634..ec9feb8e023b 100644
--- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
+++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
@@ -289,7 +289,7 @@ static void amd_mp2_pci_remove(void *privdata)
 	sfh_deinit_emp2();
 	amd_sfh_hid_client_deinit(privdata);
 	mp2->mp2_ops->stop_all(mp2);
-	pci_intx(mp2->pdev, false);
+	pcim_intx(mp2->pdev, false);
 	amd_sfh_clear_intr(mp2);
 }
 
-- 
2.47.0

[PATCH 12/13] Remove devres from pci_intx()

[Philipp Stanner]

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@redhat.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                             | 42 +------------------
 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(+), 61 deletions(-)

diff --git a/drivers/misc/cardreader/rtsx_pcr.c b/drivers/misc/cardreader/rtsx_pcr.c
index e25e6d560dd7..be3d4e0e50cc 100644
--- a/drivers/misc/cardreader/rtsx_pcr.c
+++ b/drivers/misc/cardreader/rtsx_pcr.c
@@ -1057,7 +1057,7 @@ static int rtsx_pci_acquire_irq(struct rtsx_pcr *pcr)
 	}
 
 	pcr->irq = pcr->pci->irq;
-	pci_intx_unmanaged(pcr->pci, !pcr->msi_en);
+	pci_intx(pcr->pci, !pcr->msi_en);
 
 	return 0;
 }
diff --git a/drivers/misc/tifm_7xx1.c b/drivers/misc/tifm_7xx1.c
index 5f9c7ccae8d2..1d54680d6ed2 100644
--- a/drivers/misc/tifm_7xx1.c
+++ b/drivers/misc/tifm_7xx1.c
@@ -327,7 +327,7 @@ static int tifm_7xx1_probe(struct pci_dev *dev,
 		goto err_out;
 	}
 
-	pci_intx_unmanaged(dev, 1);
+	pci_intx(dev, 1);
 
 	fm = tifm_alloc_adapter(dev->device == PCI_DEVICE_ID_TI_XX21_XX11_FM
 				? 4 : 2, &dev->dev);
@@ -368,7 +368,7 @@ static int tifm_7xx1_probe(struct pci_dev *dev,
 err_out_free:
 	tifm_free_adapter(fm);
 err_out_int:
-	pci_intx_unmanaged(dev, 0);
+	pci_intx(dev, 0);
 	pci_release_regions(dev);
 err_out:
 	if (!pci_dev_busy)
@@ -392,7 +392,7 @@ static void tifm_7xx1_remove(struct pci_dev *dev)
 		tifm_7xx1_sock_power_off(tifm_7xx1_sock_addr(fm->addr, cnt));
 
 	iounmap(fm->addr);
-	pci_intx_unmanaged(dev, 0);
+	pci_intx(dev, 0);
 	pci_release_regions(dev);
 
 	pci_disable_device(dev);
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 2ae63d6e6792..678829646cec 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -1669,7 +1669,7 @@ static void bnx2x_igu_int_enable(struct bnx2x *bp)
 	REG_WR(bp, IGU_REG_PF_CONFIGURATION, val);
 
 	if (val & IGU_PF_CONF_INT_LINE_EN)
-		pci_intx_unmanaged(bp->pdev, true);
+		pci_intx(bp->pdev, true);
 
 	barrier();
 
diff --git a/drivers/net/ethernet/brocade/bna/bnad.c b/drivers/net/ethernet/brocade/bna/bnad.c
index 2b37462d406e..ece6f3b48327 100644
--- a/drivers/net/ethernet/brocade/bna/bnad.c
+++ b/drivers/net/ethernet/brocade/bna/bnad.c
@@ -2669,7 +2669,7 @@ bnad_enable_msix(struct bnad *bnad)
 		}
 	}
 
-	pci_intx_unmanaged(bnad->pcidev, 0);
+	pci_intx(bnad->pcidev, 0);
 
 	return;
 
diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c
index b146f170e839..d687e8c2cc78 100644
--- a/drivers/ntb/hw/amd/ntb_hw_amd.c
+++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
@@ -791,7 +791,7 @@ static int ndev_init_isr(struct amd_ntb_dev *ndev,
 err_msi_enable:
 
 	/* Try to set up intx irq */
-	pci_intx_unmanaged(pdev, 1);
+	pci_intx(pdev, 1);
 
 	rc = request_irq(pdev->irq, ndev_irq_isr, IRQF_SHARED,
 			 "ndev_irq_isr", ndev);
@@ -831,7 +831,7 @@ static void ndev_deinit_isr(struct amd_ntb_dev *ndev)
 		if (pci_dev_msi_enabled(pdev))
 			pci_disable_msi(pdev);
 		else
-			pci_intx_unmanaged(pdev, 0);
+			pci_intx(pdev, 0);
 	}
 }
 
diff --git a/drivers/ntb/hw/intel/ntb_hw_gen1.c b/drivers/ntb/hw/intel/ntb_hw_gen1.c
index 9ad9d7fe227e..079b8cd79785 100644
--- a/drivers/ntb/hw/intel/ntb_hw_gen1.c
+++ b/drivers/ntb/hw/intel/ntb_hw_gen1.c
@@ -445,7 +445,7 @@ int ndev_init_isr(struct intel_ntb_dev *ndev,
 
 	/* Try to set up intx irq */
 
-	pci_intx_unmanaged(pdev, 1);
+	pci_intx(pdev, 1);
 
 	rc = request_irq(pdev->irq, ndev_irq_isr, IRQF_SHARED,
 			 "ndev_irq_isr", ndev);
diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index d32827a1f2f4..6f8f712fe34e 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -416,7 +416,7 @@ static void pcim_intx_restore(struct device *dev, void *data)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct pcim_intx_devres *res = data;
 
-	pci_intx_unmanaged(pdev, res->orig_intx);
+	pci_intx(pdev, res->orig_intx);
 }
 
 static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
@@ -453,7 +453,7 @@ int pcim_intx(struct pci_dev *pdev, int enable)
 		return -ENOMEM;
 
 	res->orig_intx = !enable;
-	pci_intx_unmanaged(pdev, enable);
+	pci_intx(pdev, enable);
 
 	return 0;
 }
diff --git a/drivers/pci/msi/api.c b/drivers/pci/msi/api.c
index c95e2e7dc9ab..b956ce591f96 100644
--- a/drivers/pci/msi/api.c
+++ b/drivers/pci/msi/api.c
@@ -289,7 +289,7 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
 			 */
 			if (affd)
 				irq_create_affinity_masks(1, affd);
-			pci_intx_unmanaged(dev, 1);
+			pci_intx(dev, 1);
 			return 1;
 		}
 	}
diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
index 53f13b09db50..3a45879d85db 100644
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -268,7 +268,7 @@ EXPORT_SYMBOL_GPL(pci_write_msi_msg);
 static void pci_intx_for_msi(struct pci_dev *dev, int enable)
 {
 	if (!(dev->dev_flags & PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG))
-		pci_intx_unmanaged(dev, enable);
+		pci_intx(dev, enable);
 }
 
 static void pci_msi_set_enable(struct pci_dev *dev, int enable)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d7fd0772a885..7ce1d0e3a1d5 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4477,16 +4477,13 @@ void pci_disable_parity(struct pci_dev *dev)
 }
 
 /**
- * pci_intx - enables/disables PCI INTx for device dev, unmanaged version
+ * pci_intx - enables/disables PCI INTx for device dev
  * @pdev: the PCI device to operate on
  * @enable: boolean: whether to enable or disable PCI INTx
  *
  * Enables/disables PCI INTx for device @pdev
- *
- * This function behavios identically to pci_intx(), but is never managed with
- * devres.
  */
-void pci_intx_unmanaged(struct pci_dev *pdev, int enable)
+void pci_intx(struct pci_dev *pdev, int enable)
 {
 	u16 pci_command, new;
 
@@ -4502,41 +4499,6 @@ void pci_intx_unmanaged(struct pci_dev *pdev, int enable)
 
 	pci_write_config_word(pdev, PCI_COMMAND, new);
 }
-EXPORT_SYMBOL_GPL(pci_intx_unmanaged);
-
-/**
- * pci_intx - enables/disables PCI INTx for device dev
- * @pdev: the PCI device to operate on
- * @enable: boolean: whether to enable or disable PCI INTx
- *
- * Enables/disables PCI INTx for device @pdev
- *
- * NOTE:
- * This is a "hybrid" function: It's normally unmanaged, but becomes managed
- * when pcim_enable_device() has been called in advance. This hybrid feature is
- * DEPRECATED! If you want managed cleanup, use pcim_intx() instead.
- */
-void pci_intx(struct pci_dev *pdev, int enable)
-{
-	u16 pci_command, new;
-
-	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
-
-	if (enable)
-		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
-	else
-		new = pci_command | PCI_COMMAND_INTX_DISABLE;
-
-	if (new != pci_command) {
-		/* Preserve the "hybrid" behavior for backwards compatibility */
-		if (pci_is_managed(pdev)) {
-			WARN_ON_ONCE(pcim_intx(pdev, enable) != 0);
-			return;
-		}
-
-		pci_write_config_word(pdev, PCI_COMMAND, new);
-	}
-}
 EXPORT_SYMBOL_GPL(pci_intx);
 
 /**
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 90240c8d51aa..1ab58da9f38a 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -498,7 +498,7 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
 		if (vfio_pci_nointx(pdev)) {
 			pci_info(pdev, "Masking broken INTx support\n");
 			vdev->nointx = true;
-			pci_intx_unmanaged(pdev, 0);
+			pci_intx(pdev, 0);
 		} else
 			vdev->pci_2_3 = pci_intx_mask_supported(pdev);
 	}
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 40abb0b937a2..8382c5834335 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -118,7 +118,7 @@ static bool __vfio_pci_intx_mask(struct vfio_pci_core_device *vdev)
 	 */
 	if (unlikely(!is_intx(vdev))) {
 		if (vdev->pci_2_3)
-			pci_intx_unmanaged(pdev, 0);
+			pci_intx(pdev, 0);
 		goto out_unlock;
 	}
 
@@ -132,7 +132,7 @@ static bool __vfio_pci_intx_mask(struct vfio_pci_core_device *vdev)
 		 * mask, not just when something is pending.
 		 */
 		if (vdev->pci_2_3)
-			pci_intx_unmanaged(pdev, 0);
+			pci_intx(pdev, 0);
 		else
 			disable_irq_nosync(pdev->irq);
 
@@ -178,7 +178,7 @@ static int vfio_pci_intx_unmask_handler(void *opaque, void *data)
 	 */
 	if (unlikely(!is_intx(vdev))) {
 		if (vdev->pci_2_3)
-			pci_intx_unmanaged(pdev, 1);
+			pci_intx(pdev, 1);
 		goto out_unlock;
 	}
 
@@ -296,7 +296,7 @@ static int vfio_intx_enable(struct vfio_pci_core_device *vdev,
 	 */
 	ctx->masked = vdev->virq_disabled;
 	if (vdev->pci_2_3) {
-		pci_intx_unmanaged(pdev, !ctx->masked);
+		pci_intx(pdev, !ctx->masked);
 		irqflags = IRQF_SHARED;
 	} else {
 		irqflags = ctx->masked ? IRQF_NO_AUTOEN : 0;
@@ -569,7 +569,7 @@ static void vfio_msi_disable(struct vfio_pci_core_device *vdev, bool msix)
 	 * via their shutdown paths.  Restore for NoINTx devices.
 	 */
 	if (vdev->nointx)
-		pci_intx_unmanaged(pdev, 0);
+		pci_intx(pdev, 0);
 
 	vdev->irq_type = VFIO_PCI_NUM_IRQS;
 }
diff --git a/drivers/xen/xen-pciback/conf_space_header.c b/drivers/xen/xen-pciback/conf_space_header.c
index 8d26d64232e8..fc0332645966 100644
--- a/drivers/xen/xen-pciback/conf_space_header.c
+++ b/drivers/xen/xen-pciback/conf_space_header.c
@@ -106,7 +106,7 @@ static int command_write(struct pci_dev *dev, int offset, u16 value, void *data)
 
 	if (dev_data && dev_data->allow_interrupt_control &&
 	    ((cmd->val ^ value) & PCI_COMMAND_INTX_DISABLE))
-		pci_intx_unmanaged(dev, !(value & PCI_COMMAND_INTX_DISABLE));
+		pci_intx(dev, !(value & PCI_COMMAND_INTX_DISABLE));
 
 	cmd->val = value;
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6b8cde76d564..1b2a6dd1dfed 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1353,7 +1353,6 @@ int __must_check pcim_set_mwi(struct pci_dev *dev);
 int pci_try_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 void pci_disable_parity(struct pci_dev *dev);
-void pci_intx_unmanaged(struct pci_dev *pdev, int enable);
 void pci_intx(struct pci_dev *dev, int enable);
 bool pci_check_and_mask_intx(struct pci_dev *dev);
 bool pci_check_and_unmask_intx(struct pci_dev *dev);
-- 
2.47.0

[PATCH 13/13] PCI: Deprecate pci_intx(), pcim_intx()

[Philipp Stanner]

pci_intx() and its managed counterpart pcim_intx() only exist for older
drivers which have not been ported yet for various reasons. Future
drivers should preferably use pci_alloc_irq_vectors().

Mark pci_intx() and pcim_intx() as deprecated and encourage usage of
pci_alloc_irq_vectors() in its place.

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/pci/devres.c | 5 ++++-
 drivers/pci/pci.c    | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 6f8f712fe34e..4c76fc063104 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -435,7 +435,7 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
 }
 
 /**
- * pcim_intx - managed pci_intx()
+ * pcim_intx - managed pci_intx() (DEPRECATED)
  * @pdev: the PCI device to operate on
  * @enable: boolean: whether to enable or disable PCI INTx
  *
@@ -443,6 +443,9 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
  *
  * Enable/disable PCI INTx for device @pdev.
  * Restore the original state on driver detach.
+ *
+ * This function is DEPRECATED. Do not use it in new code.
+ * Use pci_alloc_irq_vectors() instead (there is no managed version, currently).
  */
 int pcim_intx(struct pci_dev *pdev, int enable)
 {
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7ce1d0e3a1d5..dc69e23b8982 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4477,11 +4477,14 @@ void pci_disable_parity(struct pci_dev *dev)
 }
 
 /**
- * pci_intx - enables/disables PCI INTx for device dev
+ * pci_intx - enables/disables PCI INTx for device dev (DEPRECATED)
  * @pdev: the PCI device to operate on
  * @enable: boolean: whether to enable or disable PCI INTx
  *
  * Enables/disables PCI INTx for device @pdev
+ *
+ * This function is DEPRECATED. Do not use it in new code.
+ * Use pci_alloc_irq_vectors() instead.
  */
 void pci_intx(struct pci_dev *pdev, int enable)
 {
-- 
2.47.0

Re: [PATCH 13/13] PCI: Deprecate pci_intx(), pcim_intx()

[Alex Williamson]

On Tue, 15 Oct 2024 20:51:23 +0200
Philipp Stanner <pstanner@redhat.com> wrote:

> pci_intx() and its managed counterpart pcim_intx() only exist for older
> drivers which have not been ported yet for various reasons. Future
> drivers should preferably use pci_alloc_irq_vectors().
> 
> Mark pci_intx() and pcim_intx() as deprecated and encourage usage of
> pci_alloc_irq_vectors() in its place.

I don't really understand this.  As we've discussed previously
pci_alloc_irq_vectors() is, unsurprisingly, for allocating PCI IRQ
vectors while pci_intx() is for manipulating the INTx disable bit on
PCI devices.  The latter is a generic mechanism for preventing PCI
devices from generating INTx, regardless of whether there's a vector
allocated for it.  How does the former replace the latter and why do we
feel the need to deprecate the latter?

It feels like this fits some narrow narrative and makes all users of
these now deprecated functions second class citizens.  Why?  At it's
root these are simply providing mask and set or mask and clear register
bit operations.  Thanks,

Alex
 
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> ---
>  drivers/pci/devres.c | 5 ++++-
>  drivers/pci/pci.c    | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> index 6f8f712fe34e..4c76fc063104 100644
> --- a/drivers/pci/devres.c
> +++ b/drivers/pci/devres.c
> @@ -435,7 +435,7 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
>  }
>  
>  /**
> - * pcim_intx - managed pci_intx()
> + * pcim_intx - managed pci_intx() (DEPRECATED)
>   * @pdev: the PCI device to operate on
>   * @enable: boolean: whether to enable or disable PCI INTx
>   *
> @@ -443,6 +443,9 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
>   *
>   * Enable/disable PCI INTx for device @pdev.
>   * Restore the original state on driver detach.
> + *
> + * This function is DEPRECATED. Do not use it in new code.
> + * Use pci_alloc_irq_vectors() instead (there is no managed version, currently).
>   */
>  int pcim_intx(struct pci_dev *pdev, int enable)
>  {
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7ce1d0e3a1d5..dc69e23b8982 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4477,11 +4477,14 @@ void pci_disable_parity(struct pci_dev *dev)
>  }
>  
>  /**
> - * pci_intx - enables/disables PCI INTx for device dev
> + * pci_intx - enables/disables PCI INTx for device dev (DEPRECATED)
>   * @pdev: the PCI device to operate on
>   * @enable: boolean: whether to enable or disable PCI INTx
>   *
>   * Enables/disables PCI INTx for device @pdev
> + *
> + * This function is DEPRECATED. Do not use it in new code.
> + * Use pci_alloc_irq_vectors() instead.
>   */
>  void pci_intx(struct pci_dev *pdev, int enable)
>  {

Re: [PATCH 13/13] PCI: Deprecate pci_intx(), pcim_intx()

[Philipp Stanner]

On Tue, 2024-10-15 at 13:53 -0600, Alex Williamson wrote:
> On Tue, 15 Oct 2024 20:51:23 +0200
> Philipp Stanner <pstanner@redhat.com> wrote:
> 
> > pci_intx() and its managed counterpart pcim_intx() only exist for
> > older
> > drivers which have not been ported yet for various reasons. Future
> > drivers should preferably use pci_alloc_irq_vectors().
> > 
> > Mark pci_intx() and pcim_intx() as deprecated and encourage usage
> > of
> > pci_alloc_irq_vectors() in its place.
> 
> I don't really understand this.  As we've discussed previously
> pci_alloc_irq_vectors() is, unsurprisingly, for allocating PCI IRQ
> vectors while pci_intx() is for manipulating the INTx disable bit on
> PCI devices.  The latter is a generic mechanism for preventing PCI
> devices from generating INTx, regardless of whether there's a vector
> allocated for it.  How does the former replace the latter and why do
> we
> feel the need to deprecate the latter?
> 
> It feels like this fits some narrow narrative and makes all users of
> these now deprecated functions second class citizens.  Why?  At it's
> root these are simply providing mask and set or mask and clear
> register
> bit operations.  Thanks,

I got the feeling from the RFC discussion that that was basically the
consensus: people should use pci_alloc_irq_vectors(). Or did I
misunderstand Andy and Heiner?

I'm perfectly happy with dropping this patch and continue offering
pci{m}_intx() to users, since after removing that hybrid hazzard I
don't see any harm in them anymore.


P.

> 
> Alex
>  
> > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > ---
> >  drivers/pci/devres.c | 5 ++++-
> >  drivers/pci/pci.c    | 5 ++++-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> > index 6f8f712fe34e..4c76fc063104 100644
> > --- a/drivers/pci/devres.c
> > +++ b/drivers/pci/devres.c
> > @@ -435,7 +435,7 @@ static struct pcim_intx_devres
> > *get_or_create_intx_devres(struct device *dev)
> >  }
> >  
> >  /**
> > - * pcim_intx - managed pci_intx()
> > + * pcim_intx - managed pci_intx() (DEPRECATED)
> >   * @pdev: the PCI device to operate on
> >   * @enable: boolean: whether to enable or disable PCI INTx
> >   *
> > @@ -443,6 +443,9 @@ static struct pcim_intx_devres
> > *get_or_create_intx_devres(struct device *dev)
> >   *
> >   * Enable/disable PCI INTx for device @pdev.
> >   * Restore the original state on driver detach.
> > + *
> > + * This function is DEPRECATED. Do not use it in new code.
> > + * Use pci_alloc_irq_vectors() instead (there is no managed
> > version, currently).
> >   */
> >  int pcim_intx(struct pci_dev *pdev, int enable)
> >  {
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 7ce1d0e3a1d5..dc69e23b8982 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -4477,11 +4477,14 @@ void pci_disable_parity(struct pci_dev
> > *dev)
> >  }
> >  
> >  /**
> > - * pci_intx - enables/disables PCI INTx for device dev
> > + * pci_intx - enables/disables PCI INTx for device dev
> > (DEPRECATED)
> >   * @pdev: the PCI device to operate on
> >   * @enable: boolean: whether to enable or disable PCI INTx
> >   *
> >   * Enables/disables PCI INTx for device @pdev
> > + *
> > + * This function is DEPRECATED. Do not use it in new code.
> > + * Use pci_alloc_irq_vectors() instead.
> >   */
> >  void pci_intx(struct pci_dev *pdev, int enable)
> >  {
> 

Re: [PATCH 13/13] PCI: Deprecate pci_intx(), pcim_intx()

[Heiner Kallweit]

On 16.10.2024 08:57, Philipp Stanner wrote:
> On Tue, 2024-10-15 at 13:53 -0600, Alex Williamson wrote:
>> On Tue, 15 Oct 2024 20:51:23 +0200
>> Philipp Stanner <pstanner@redhat.com> wrote:
>>
>>> pci_intx() and its managed counterpart pcim_intx() only exist for
>>> older
>>> drivers which have not been ported yet for various reasons. Future
>>> drivers should preferably use pci_alloc_irq_vectors().
>>>
>>> Mark pci_intx() and pcim_intx() as deprecated and encourage usage
>>> of
>>> pci_alloc_irq_vectors() in its place.
>>
>> I don't really understand this.  As we've discussed previously
>> pci_alloc_irq_vectors() is, unsurprisingly, for allocating PCI IRQ
>> vectors while pci_intx() is for manipulating the INTx disable bit on
>> PCI devices.  The latter is a generic mechanism for preventing PCI
>> devices from generating INTx, regardless of whether there's a vector
>> allocated for it.  How does the former replace the latter and why do
>> we
>> feel the need to deprecate the latter?
>>
>> It feels like this fits some narrow narrative and makes all users of
>> these now deprecated functions second class citizens.  Why?  At it's
>> root these are simply providing mask and set or mask and clear
>> register
>> bit operations.  Thanks,
> 
> I got the feeling from the RFC discussion that that was basically the
> consensus: people should use pci_alloc_irq_vectors(). Or did I
> misunderstand Andy and Heiner?
> 
I think there are two different use cases for pci_intx().
At first there are several drivers where the direct usage of pci_intx()
can be eliminated by switching to the pci_alloc_irq_vectors() API.

And then there's usage of pci_intx() in
drivers/vfio/pci/vfio_pci_intrs.c
drivers/xen/xen-pciback/conf_space_header.c
There we have to keep the (AFAICS unmanaged) pci_intx() calls.

> I'm perfectly happy with dropping this patch and continue offering
> pci{m}_intx() to users, since after removing that hybrid hazzard I
> don't see any harm in them anymore.
> 
> 
> P.
> 
>>
>> Alex
>>  
>>> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
>>> ---
>>>  drivers/pci/devres.c | 5 ++++-
>>>  drivers/pci/pci.c    | 5 ++++-
>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
>>> index 6f8f712fe34e..4c76fc063104 100644
>>> --- a/drivers/pci/devres.c
>>> +++ b/drivers/pci/devres.c
>>> @@ -435,7 +435,7 @@ static struct pcim_intx_devres
>>> *get_or_create_intx_devres(struct device *dev)
>>>  }
>>>  
>>>  /**
>>> - * pcim_intx - managed pci_intx()
>>> + * pcim_intx - managed pci_intx() (DEPRECATED)
>>>   * @pdev: the PCI device to operate on
>>>   * @enable: boolean: whether to enable or disable PCI INTx
>>>   *
>>> @@ -443,6 +443,9 @@ static struct pcim_intx_devres
>>> *get_or_create_intx_devres(struct device *dev)
>>>   *
>>>   * Enable/disable PCI INTx for device @pdev.
>>>   * Restore the original state on driver detach.
>>> + *
>>> + * This function is DEPRECATED. Do not use it in new code.
>>> + * Use pci_alloc_irq_vectors() instead (there is no managed
>>> version, currently).
>>>   */
>>>  int pcim_intx(struct pci_dev *pdev, int enable)
>>>  {
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index 7ce1d0e3a1d5..dc69e23b8982 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -4477,11 +4477,14 @@ void pci_disable_parity(struct pci_dev
>>> *dev)
>>>  }
>>>  
>>>  /**
>>> - * pci_intx - enables/disables PCI INTx for device dev
>>> + * pci_intx - enables/disables PCI INTx for device dev
>>> (DEPRECATED)
>>>   * @pdev: the PCI device to operate on
>>>   * @enable: boolean: whether to enable or disable PCI INTx
>>>   *
>>>   * Enables/disables PCI INTx for device @pdev
>>> + *
>>> + * This function is DEPRECATED. Do not use it in new code.
>>> + * Use pci_alloc_irq_vectors() instead.
>>>   */
>>>  void pci_intx(struct pci_dev *pdev, int enable)
>>>  {
>>
> 
> 

Re: [PATCH 13/13] PCI: Deprecate pci_intx(), pcim_intx()

[Philipp Stanner]

On Wed, 2024-10-16 at 10:43 +0200, Heiner Kallweit wrote:
> On 16.10.2024 08:57, Philipp Stanner wrote:
> > On Tue, 2024-10-15 at 13:53 -0600, Alex Williamson wrote:
> > > On Tue, 15 Oct 2024 20:51:23 +0200
> > > Philipp Stanner <pstanner@redhat.com> wrote:
> > > 
> > > > pci_intx() and its managed counterpart pcim_intx() only exist
> > > > for
> > > > older
> > > > drivers which have not been ported yet for various reasons.
> > > > Future
> > > > drivers should preferably use pci_alloc_irq_vectors().
> > > > 
> > > > Mark pci_intx() and pcim_intx() as deprecated and encourage
> > > > usage
> > > > of
> > > > pci_alloc_irq_vectors() in its place.
> > > 
> > > I don't really understand this.  As we've discussed previously
> > > pci_alloc_irq_vectors() is, unsurprisingly, for allocating PCI
> > > IRQ
> > > vectors while pci_intx() is for manipulating the INTx disable bit
> > > on
> > > PCI devices.  The latter is a generic mechanism for preventing
> > > PCI
> > > devices from generating INTx, regardless of whether there's a
> > > vector
> > > allocated for it.  How does the former replace the latter and why
> > > do
> > > we
> > > feel the need to deprecate the latter?
> > > 
> > > It feels like this fits some narrow narrative and makes all users
> > > of
> > > these now deprecated functions second class citizens.  Why?  At
> > > it's
> > > root these are simply providing mask and set or mask and clear
> > > register
> > > bit operations.  Thanks,
> > 
> > I got the feeling from the RFC discussion that that was basically
> > the
> > consensus: people should use pci_alloc_irq_vectors(). Or did I
> > misunderstand Andy and Heiner?
> > 
> I think there are two different use cases for pci_intx().
> At first there are several drivers where the direct usage of
> pci_intx()
> can be eliminated by switching to the pci_alloc_irq_vectors() API.
> 
> And then there's usage of pci_intx() in
> drivers/vfio/pci/vfio_pci_intrs.c
> drivers/xen/xen-pciback/conf_space_header.c
> There we have to keep the (AFAICS unmanaged) pci_intx() calls.

There is also the usage within PCI itself, in MSI. Patch №8 touches
that.

It's why I think this series should land before anyone should port
direct pci_intx() users to the irq vectors function, because the latter
also uses pci_intx() and its own devres, which sounds explosive to me.


P.

> 
> > I'm perfectly happy with dropping this patch and continue offering
> > pci{m}_intx() to users, since after removing that hybrid hazzard I
> > don't see any harm in them anymore.
> > 
> > 
> > P.
> > 
> > > 
> > > Alex
> > >  
> > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > > > ---
> > > >  drivers/pci/devres.c | 5 ++++-
> > > >  drivers/pci/pci.c    | 5 ++++-
> > > >  2 files changed, 8 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> > > > index 6f8f712fe34e..4c76fc063104 100644
> > > > --- a/drivers/pci/devres.c
> > > > +++ b/drivers/pci/devres.c
> > > > @@ -435,7 +435,7 @@ static struct pcim_intx_devres
> > > > *get_or_create_intx_devres(struct device *dev)
> > > >  }
> > > >  
> > > >  /**
> > > > - * pcim_intx - managed pci_intx()
> > > > + * pcim_intx - managed pci_intx() (DEPRECATED)
> > > >   * @pdev: the PCI device to operate on
> > > >   * @enable: boolean: whether to enable or disable PCI INTx
> > > >   *
> > > > @@ -443,6 +443,9 @@ static struct pcim_intx_devres
> > > > *get_or_create_intx_devres(struct device *dev)
> > > >   *
> > > >   * Enable/disable PCI INTx for device @pdev.
> > > >   * Restore the original state on driver detach.
> > > > + *
> > > > + * This function is DEPRECATED. Do not use it in new code.
> > > > + * Use pci_alloc_irq_vectors() instead (there is no managed
> > > > version, currently).
> > > >   */
> > > >  int pcim_intx(struct pci_dev *pdev, int enable)
> > > >  {
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index 7ce1d0e3a1d5..dc69e23b8982 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -4477,11 +4477,14 @@ void pci_disable_parity(struct pci_dev
> > > > *dev)
> > > >  }
> > > >  
> > > >  /**
> > > > - * pci_intx - enables/disables PCI INTx for device dev
> > > > + * pci_intx - enables/disables PCI INTx for device dev
> > > > (DEPRECATED)
> > > >   * @pdev: the PCI device to operate on
> > > >   * @enable: boolean: whether to enable or disable PCI INTx
> > > >   *
> > > >   * Enables/disables PCI INTx for device @pdev
> > > > + *
> > > > + * This function is DEPRECATED. Do not use it in new code.
> > > > + * Use pci_alloc_irq_vectors() instead.
> > > >   */
> > > >  void pci_intx(struct pci_dev *pdev, int enable)
> > > >  {
> > > 
> > 
> > 
> 

Re: [PATCH 13/13] PCI: Deprecate pci_intx(), pcim_intx()

[Bjorn Helgaas]

On Wed, Oct 16, 2024 at 10:53:16AM +0200, Philipp Stanner wrote:
> On Wed, 2024-10-16 at 10:43 +0200, Heiner Kallweit wrote:
> > On 16.10.2024 08:57, Philipp Stanner wrote:
> > > On Tue, 2024-10-15 at 13:53 -0600, Alex Williamson wrote:
> > > > On Tue, 15 Oct 2024 20:51:23 +0200
> > > > Philipp Stanner <pstanner@redhat.com> wrote:
> > > > 
> > > > > pci_intx() and its managed counterpart pcim_intx() only exist
> > > > > for
> > > > > older
> > > > > drivers which have not been ported yet for various reasons.
> > > > > Future
> > > > > drivers should preferably use pci_alloc_irq_vectors().
> > > > > 
> > > > > Mark pci_intx() and pcim_intx() as deprecated and encourage
> > > > > usage
> > > > > of
> > > > > pci_alloc_irq_vectors() in its place.
> > > > 
> > > > I don't really understand this.  As we've discussed previously
> > > > pci_alloc_irq_vectors() is, unsurprisingly, for allocating PCI
> > > > IRQ
> > > > vectors while pci_intx() is for manipulating the INTx disable bit
> > > > on
> > > > PCI devices.  The latter is a generic mechanism for preventing
> > > > PCI
> > > > devices from generating INTx, regardless of whether there's a
> > > > vector
> > > > allocated for it.  How does the former replace the latter and why
> > > > do
> > > > we
> > > > feel the need to deprecate the latter?
> > > > 
> > > > It feels like this fits some narrow narrative and makes all users
> > > > of
> > > > these now deprecated functions second class citizens.  Why?  At
> > > > it's
> > > > root these are simply providing mask and set or mask and clear
> > > > register
> > > > bit operations.  Thanks,
> > > 
> > > I got the feeling from the RFC discussion that that was basically
> > > the
> > > consensus: people should use pci_alloc_irq_vectors(). Or did I
> > > misunderstand Andy and Heiner?
> > > 
> > I think there are two different use cases for pci_intx().
> > At first there are several drivers where the direct usage of
> > pci_intx()
> > can be eliminated by switching to the pci_alloc_irq_vectors() API.
> > 
> > And then there's usage of pci_intx() in
> > drivers/vfio/pci/vfio_pci_intrs.c
> > drivers/xen/xen-pciback/conf_space_header.c
> > There we have to keep the (AFAICS unmanaged) pci_intx() calls.
> 
> There is also the usage within PCI itself, in MSI. Patch №8 touches
> that.
> 
> It's why I think this series should land before anyone should port
> direct pci_intx() users to the irq vectors function, because the latter
> also uses pci_intx() and its own devres, which sounds explosive to me.
>
> > > I'm perfectly happy with dropping this patch and continue offering
> > > pci{m}_intx() to users, since after removing that hybrid hazzard I
> > > don't see any harm in them anymore.

So is the bottom line that we should drop *this* patch and apply the
rest of the series?

> > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > > > > ---
> > > > >  drivers/pci/devres.c | 5 ++++-
> > > > >  drivers/pci/pci.c    | 5 ++++-
> > > > >  2 files changed, 8 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> > > > > index 6f8f712fe34e..4c76fc063104 100644
> > > > > --- a/drivers/pci/devres.c
> > > > > +++ b/drivers/pci/devres.c
> > > > > @@ -435,7 +435,7 @@ static struct pcim_intx_devres
> > > > > *get_or_create_intx_devres(struct device *dev)
> > > > >  }
> > > > >  
> > > > >  /**
> > > > > - * pcim_intx - managed pci_intx()
> > > > > + * pcim_intx - managed pci_intx() (DEPRECATED)
> > > > >   * @pdev: the PCI device to operate on
> > > > >   * @enable: boolean: whether to enable or disable PCI INTx
> > > > >   *
> > > > > @@ -443,6 +443,9 @@ static struct pcim_intx_devres
> > > > > *get_or_create_intx_devres(struct device *dev)
> > > > >   *
> > > > >   * Enable/disable PCI INTx for device @pdev.
> > > > >   * Restore the original state on driver detach.
> > > > > + *
> > > > > + * This function is DEPRECATED. Do not use it in new code.
> > > > > + * Use pci_alloc_irq_vectors() instead (there is no managed
> > > > > version, currently).
> > > > >   */
> > > > >  int pcim_intx(struct pci_dev *pdev, int enable)
> > > > >  {
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index 7ce1d0e3a1d5..dc69e23b8982 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -4477,11 +4477,14 @@ void pci_disable_parity(struct pci_dev
> > > > > *dev)
> > > > >  }
> > > > >  
> > > > >  /**
> > > > > - * pci_intx - enables/disables PCI INTx for device dev
> > > > > + * pci_intx - enables/disables PCI INTx for device dev
> > > > > (DEPRECATED)
> > > > >   * @pdev: the PCI device to operate on
> > > > >   * @enable: boolean: whether to enable or disable PCI INTx
> > > > >   *
> > > > >   * Enables/disables PCI INTx for device @pdev
> > > > > + *
> > > > > + * This function is DEPRECATED. Do not use it in new code.
> > > > > + * Use pci_alloc_irq_vectors() instead.
> > > > >   */
> > > > >  void pci_intx(struct pci_dev *pdev, int enable)
> > > > >  {
> > > > 
> > > 
> > > 
> > 
> 

Re: [PATCH 13/13] PCI: Deprecate pci_intx(), pcim_intx()

[Philipp Stanner]

On Fri, 2024-10-18 at 18:45 -0500, Bjorn Helgaas wrote:
> On Wed, Oct 16, 2024 at 10:53:16AM +0200, Philipp Stanner wrote:
> > On Wed, 2024-10-16 at 10:43 +0200, Heiner Kallweit wrote:
> > > On 16.10.2024 08:57, Philipp Stanner wrote:
> > > > On Tue, 2024-10-15 at 13:53 -0600, Alex Williamson wrote:
> > > > > On Tue, 15 Oct 2024 20:51:23 +0200
> > > > > Philipp Stanner <pstanner@redhat.com> wrote:
> > > > > 
> > > > > > pci_intx() and its managed counterpart pcim_intx() only
> > > > > > exist
> > > > > > for
> > > > > > older
> > > > > > drivers which have not been ported yet for various reasons.
> > > > > > Future
> > > > > > drivers should preferably use pci_alloc_irq_vectors().
> > > > > > 
> > > > > > Mark pci_intx() and pcim_intx() as deprecated and encourage
> > > > > > usage
> > > > > > of
> > > > > > pci_alloc_irq_vectors() in its place.
> > > > > 
> > > > > I don't really understand this.  As we've discussed
> > > > > previously
> > > > > pci_alloc_irq_vectors() is, unsurprisingly, for allocating
> > > > > PCI
> > > > > IRQ
> > > > > vectors while pci_intx() is for manipulating the INTx disable
> > > > > bit
> > > > > on
> > > > > PCI devices.  The latter is a generic mechanism for
> > > > > preventing
> > > > > PCI
> > > > > devices from generating INTx, regardless of whether there's a
> > > > > vector
> > > > > allocated for it.  How does the former replace the latter and
> > > > > why
> > > > > do
> > > > > we
> > > > > feel the need to deprecate the latter?
> > > > > 
> > > > > It feels like this fits some narrow narrative and makes all
> > > > > users
> > > > > of
> > > > > these now deprecated functions second class citizens.  Why? 
> > > > > At
> > > > > it's
> > > > > root these are simply providing mask and set or mask and
> > > > > clear
> > > > > register
> > > > > bit operations.  Thanks,
> > > > 
> > > > I got the feeling from the RFC discussion that that was
> > > > basically
> > > > the
> > > > consensus: people should use pci_alloc_irq_vectors(). Or did I
> > > > misunderstand Andy and Heiner?
> > > > 
> > > I think there are two different use cases for pci_intx().
> > > At first there are several drivers where the direct usage of
> > > pci_intx()
> > > can be eliminated by switching to the pci_alloc_irq_vectors()
> > > API.
> > > 
> > > And then there's usage of pci_intx() in
> > > drivers/vfio/pci/vfio_pci_intrs.c
> > > drivers/xen/xen-pciback/conf_space_header.c
> > > There we have to keep the (AFAICS unmanaged) pci_intx() calls.
> > 
> > There is also the usage within PCI itself, in MSI. Patch №8 touches
> > that.
> > 
> > It's why I think this series should land before anyone should port
> > direct pci_intx() users to the irq vectors function, because the
> > latter
> > also uses pci_intx() and its own devres, which sounds explosive to
> > me.
> > 
> > > > I'm perfectly happy with dropping this patch and continue
> > > > offering
> > > > pci{m}_intx() to users, since after removing that hybrid
> > > > hazzard I
> > > > don't see any harm in them anymore.
> 
> So is the bottom line that we should drop *this* patch and apply the
> rest of the series?

Yes Sir, that's the idea

Regards,
P.

> 
> > > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > > > > > ---
> > > > > >  drivers/pci/devres.c | 5 ++++-
> > > > > >  drivers/pci/pci.c    | 5 ++++-
> > > > > >  2 files changed, 8 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> > > > > > index 6f8f712fe34e..4c76fc063104 100644
> > > > > > --- a/drivers/pci/devres.c
> > > > > > +++ b/drivers/pci/devres.c
> > > > > > @@ -435,7 +435,7 @@ static struct pcim_intx_devres
> > > > > > *get_or_create_intx_devres(struct device *dev)
> > > > > >  }
> > > > > >  
> > > > > >  /**
> > > > > > - * pcim_intx - managed pci_intx()
> > > > > > + * pcim_intx - managed pci_intx() (DEPRECATED)
> > > > > >   * @pdev: the PCI device to operate on
> > > > > >   * @enable: boolean: whether to enable or disable PCI INTx
> > > > > >   *
> > > > > > @@ -443,6 +443,9 @@ static struct pcim_intx_devres
> > > > > > *get_or_create_intx_devres(struct device *dev)
> > > > > >   *
> > > > > >   * Enable/disable PCI INTx for device @pdev.
> > > > > >   * Restore the original state on driver detach.
> > > > > > + *
> > > > > > + * This function is DEPRECATED. Do not use it in new code.
> > > > > > + * Use pci_alloc_irq_vectors() instead (there is no
> > > > > > managed
> > > > > > version, currently).
> > > > > >   */
> > > > > >  int pcim_intx(struct pci_dev *pdev, int enable)
> > > > > >  {
> > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > > index 7ce1d0e3a1d5..dc69e23b8982 100644
> > > > > > --- a/drivers/pci/pci.c
> > > > > > +++ b/drivers/pci/pci.c
> > > > > > @@ -4477,11 +4477,14 @@ void pci_disable_parity(struct
> > > > > > pci_dev
> > > > > > *dev)
> > > > > >  }
> > > > > >  
> > > > > >  /**
> > > > > > - * pci_intx - enables/disables PCI INTx for device dev
> > > > > > + * pci_intx - enables/disables PCI INTx for device dev
> > > > > > (DEPRECATED)
> > > > > >   * @pdev: the PCI device to operate on
> > > > > >   * @enable: boolean: whether to enable or disable PCI INTx
> > > > > >   *
> > > > > >   * Enables/disables PCI INTx for device @pdev
> > > > > > + *
> > > > > > + * This function is DEPRECATED. Do not use it in new code.
> > > > > > + * Use pci_alloc_irq_vectors() instead.
> > > > > >   */
> > > > > >  void pci_intx(struct pci_dev *pdev, int enable)
> > > > > >  {
> > > > > 
> > > > 
> > > > 
> > > 
> > 
> 

Re: [PATCH 13/13] PCI: Deprecate pci_intx(), pcim_intx()

[Greg Kroah-Hartman]

On Tue, Oct 15, 2024 at 08:51:23PM +0200, Philipp Stanner wrote:
> pci_intx() and its managed counterpart pcim_intx() only exist for older
> drivers which have not been ported yet for various reasons. Future
> drivers should preferably use pci_alloc_irq_vectors().
> 
> Mark pci_intx() and pcim_intx() as deprecated and encourage usage of
> pci_alloc_irq_vectors() in its place.

No one is going to notice these comments, so please, if this really does
need to be removed, just remove it from all callers and delete the
function from the tree.

thanks,

greg k-h

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

[Bjorn Helgaas]

On Tue, Oct 15, 2024 at 08:51:10PM +0200, Philipp Stanner wrote:
> @Driver-Maintainers: Your driver might be touched by patch "Remove
> devres from pci_intx()". You might want to take a look.
> 
> Changes since the RFC [1]:
>   - Add a patch deprecating pci{m}_intx(). (Heiner, Andy, Me)
>   - Add Acked-by's already given.
>   - Export pcim_intx() as a GPL function. (Alex)
>   - Drop patch for rts5280, since this driver will be removed quite
>     soon. (Philipp Hortmann, Greg)
>   - Use early-return in pci_intx_unmanaged() and pci_intx(). (Andy)
> 
> 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.
> 
> 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.
> 
> Merge plan for this is to enter through the PCI tree.
> 
> [1] https://lore.kernel.org/all/20241009083519.10088-1-pstanner@redhat.com/

I *think* this series depends on resolution of Takashi's "Restore the
original INTX_DISABLE bit by pcim_intx()" patch [2], right?

For now I'm postponing this series, but let me know if that's not the
right thing.

[2] https://lore.kernel.org/r/20241024155539.19416-1-tiwai@suse.de

> Philipp Stanner (13):
>   PCI: Prepare removing devres from pci_intx()
>   ALSA: 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()
>   wifi: qtnfmac: use always-managed version of pcim_intx()
>   HID: amd_sfh: Use always-managed version of pcim_intx()
>   Remove devres from pci_intx()
>   PCI: Deprecate pci_intx(), pcim_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                          | 29 +++++--------------
>  drivers/pci/pci.c                             | 19 ++++--------
>  include/linux/pci.h                           |  1 +
>  sound/pci/hda/hda_intel.c                     |  2 +-
>  14 files changed, 26 insertions(+), 47 deletions(-)
> 
> -- 
> 2.47.0
> 

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

[Takashi Iwai]

On Wed, 30 Oct 2024 23:17:37 +0100,
Bjorn Helgaas wrote:
> 
> On Tue, Oct 15, 2024 at 08:51:10PM +0200, Philipp Stanner wrote:
> > @Driver-Maintainers: Your driver might be touched by patch "Remove
> > devres from pci_intx()". You might want to take a look.
> > 
> > Changes since the RFC [1]:
> >   - Add a patch deprecating pci{m}_intx(). (Heiner, Andy, Me)
> >   - Add Acked-by's already given.
> >   - Export pcim_intx() as a GPL function. (Alex)
> >   - Drop patch for rts5280, since this driver will be removed quite
> >     soon. (Philipp Hortmann, Greg)
> >   - Use early-return in pci_intx_unmanaged() and pci_intx(). (Andy)
> > 
> > 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.
> > 
> > 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.
> > 
> > Merge plan for this is to enter through the PCI tree.
> > 
> > [1] https://lore.kernel.org/all/20241009083519.10088-1-pstanner@redhat.com/
> 
> I *think* this series depends on resolution of Takashi's "Restore the
> original INTX_DISABLE bit by pcim_intx()" patch [2], right?

IIUC, it's not really dependent, as pcim_intx() has been used in
pci_intx() internally when the PCI device is already managed.
My patch is to correct the already existing behavior.  So I guess you
can take this series, and I'll post the revised patch later (sorry, I
was too busy for other tasks).


thanks,

Takashi

> 
> For now I'm postponing this series, but let me know if that's not the
> right thing.
> 
> [2] https://lore.kernel.org/r/20241024155539.19416-1-tiwai@suse.de
> 
> > Philipp Stanner (13):
> >   PCI: Prepare removing devres from pci_intx()
> >   ALSA: 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()
> >   wifi: qtnfmac: use always-managed version of pcim_intx()
> >   HID: amd_sfh: Use always-managed version of pcim_intx()
> >   Remove devres from pci_intx()
> >   PCI: Deprecate pci_intx(), pcim_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                          | 29 +++++--------------
> >  drivers/pci/pci.c                             | 19 ++++--------
> >  include/linux/pci.h                           |  1 +
> >  sound/pci/hda/hda_intel.c                     |  2 +-
> >  14 files changed, 26 insertions(+), 47 deletions(-)
> > 
> > -- 
> > 2.47.0
> > 

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

[Philipp Stanner]

On Thu, 2024-10-31 at 10:19 +0100, Takashi Iwai wrote:
> On Wed, 30 Oct 2024 23:17:37 +0100,
> Bjorn Helgaas wrote:
> > 
> > On Tue, Oct 15, 2024 at 08:51:10PM +0200, Philipp Stanner wrote:
> > > @Driver-Maintainers: Your driver might be touched by patch
> > > "Remove
> > > devres from pci_intx()". You might want to take a look.
> > > 
> > > Changes since the RFC [1]:
> > >   - Add a patch deprecating pci{m}_intx(). (Heiner, Andy, Me)
> > >   - Add Acked-by's already given.
> > >   - Export pcim_intx() as a GPL function. (Alex)
> > >   - Drop patch for rts5280, since this driver will be removed
> > > quite
> > >     soon. (Philipp Hortmann, Greg)
> > >   - Use early-return in pci_intx_unmanaged() and pci_intx().
> > > (Andy)
> > > 
> > > 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.
> > > 
> > > 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.
> > > 
> > > Merge plan for this is to enter through the PCI tree.
> > > 
> > > [1]
> > > https://lore.kernel.org/all/20241009083519.10088-1-pstanner@redhat.com/
> > 
> > I *think* this series depends on resolution of Takashi's "Restore
> > the
> > original INTX_DISABLE bit by pcim_intx()" patch [2], right?
> 
> IIUC, it's not really dependent, as pcim_intx() has been used in
> pci_intx() internally when the PCI device is already managed.
> My patch is to correct the already existing behavior.

IOW, pcim_intx() does not behave correctly, independently from removing
the call to it in pci_intx().

>   So I guess you
> can take this series, and I'll post the revised patch later (sorry, I
> was too busy for other tasks).
> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > For now I'm postponing this series, but let me know if that's not
> > the
> > right thing.

There are still several reviews / acks missing from the respective
driver maintainers, so there's no hurry with this series regarding your
side ;)

Regards
P.


> > 
> > [2] https://lore.kernel.org/r/20241024155539.19416-1-tiwai@suse.de
> > 
> > > Philipp Stanner (13):
> > >   PCI: Prepare removing devres from pci_intx()
> > >   ALSA: 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()
> > >   wifi: qtnfmac: use always-managed version of pcim_intx()
> > >   HID: amd_sfh: Use always-managed version of pcim_intx()
> > >   Remove devres from pci_intx()
> > >   PCI: Deprecate pci_intx(), pcim_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                          | 29 +++++--------
> > > ------
> > >  drivers/pci/pci.c                             | 19 ++++--------
> > >  include/linux/pci.h                           |  1 +
> > >  sound/pci/hda/hda_intel.c                     |  2 +-
> > >  14 files changed, 26 insertions(+), 47 deletions(-)
> > > 
> > > -- 
> > > 2.47.0
> > > 
>