| Message ID | 20241009083519.10088-14-pstanner@redhat.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Remove implicit devres from pci_intx() | expand |
On Wed, Oct 09, 2024 at 10:35:19AM +0200, Philipp Stanner wrote: > pci_intx() is a hybrid function which can sometimes be managed through > devres. This hybrid nature is undesirable. > > Since all users of pci_intx() have by now been ported either to > always-managed pcim_intx() or never-managed pci_intx_unmanaged(), the > devres functionality can be removed from pci_intx(). > > Consequently, pci_intx_unmanaged() is now redundant, because pci_intx() > itself is now unmanaged. > > Remove the devres functionality from pci_intx(). Remove pci_intx_unmanaged(). > Have all users of pci_intx_unmanaged() call pci_intx(). > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> I don't like when we change a function like this but it still compiles fine. If someone is working on a driver and hasn't pushed it yet, then it's probably supposed to be using the new pcim_intx() but they won't discover that until they detect the leaks at runtime. Why not leave the pci_intx_unmanaged() name. It's ugly and that will discorage people from introducing new uses. regards, dan carpenter
On Thu, 2024-10-10 at 11:50 +0300, Dan Carpenter wrote: > On Wed, Oct 09, 2024 at 10:35:19AM +0200, Philipp Stanner wrote: > > pci_intx() is a hybrid function which can sometimes be managed > > through > > devres. This hybrid nature is undesirable. > > > > Since all users of pci_intx() have by now been ported either to > > always-managed pcim_intx() or never-managed pci_intx_unmanaged(), > > the > > devres functionality can be removed from pci_intx(). > > > > Consequently, pci_intx_unmanaged() is now redundant, because > > pci_intx() > > itself is now unmanaged. > > > > Remove the devres functionality from pci_intx(). Remove > > pci_intx_unmanaged(). > > Have all users of pci_intx_unmanaged() call pci_intx(). > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > I don't like when we change a function like this but it still > compiles fine. > If someone is working on a driver and hasn't pushed it yet, then it's > probably > supposed to be using the new pcim_intx() but they won't discover that > until they > detect the leaks at runtime. There wouldn't be any *leaks*, it's just that the INTx state would not automatically be restored. BTW the official documentation in its current state does not hint at pci_intx() doing anything automatically, but rather actively marks it as deprecated. But you are right that a hypothetical new driver and OOT drivers could experience bugs through this change. > > Why not leave the pci_intx_unmanaged() name. It's ugly and that will > discorage > people from introducing new uses. I'd be OK with that. Then we'd have to remove pci_intx() as it has new users anymore. Either way should be fine and keep the behavior for existing drivers identical. I think Bjorn should express a preference P. > > regards, > dan carpenter >
On Thu, 10 Oct 2024 11:11:36 +0200 Philipp Stanner <pstanner@redhat.com> wrote: > On Thu, 2024-10-10 at 11:50 +0300, Dan Carpenter wrote: > > On Wed, Oct 09, 2024 at 10:35:19AM +0200, Philipp Stanner wrote: > > > pci_intx() is a hybrid function which can sometimes be managed > > > through > > > devres. This hybrid nature is undesirable. > > > > > > Since all users of pci_intx() have by now been ported either to > > > always-managed pcim_intx() or never-managed pci_intx_unmanaged(), > > > the > > > devres functionality can be removed from pci_intx(). > > > > > > Consequently, pci_intx_unmanaged() is now redundant, because > > > pci_intx() > > > itself is now unmanaged. > > > > > > Remove the devres functionality from pci_intx(). Remove > > > pci_intx_unmanaged(). > > > Have all users of pci_intx_unmanaged() call pci_intx(). > > > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > > > I don't like when we change a function like this but it still > > compiles fine. > > If someone is working on a driver and hasn't pushed it yet, then it's > > probably > > supposed to be using the new pcim_intx() but they won't discover that > > until they > > detect the leaks at runtime. > > There wouldn't be any *leaks*, it's just that the INTx state would not > automatically be restored. BTW the official documentation in its > current state does not hint at pci_intx() doing anything automatically, > but rather actively marks it as deprecated. > > But you are right that a hypothetical new driver and OOT drivers could > experience bugs through this change. > > > > > Why not leave the pci_intx_unmanaged() name. It's ugly and that will > > discorage > > people from introducing new uses. > > I'd be OK with that. Then we'd have to remove pci_intx() as it has new > users anymore. > > Either way should be fine and keep the behavior for existing drivers > identical. > > I think Bjorn should express a preference FWIW, I think pcim_intx() and pci_intx() align better to our naming convention for devres interfaces. Would it be sufficient if pci_intx() triggered a WARN_ON if called for a pci_is_managed() device? Thanks, Alex
On Thu, Oct 10, 2024 at 11:43:14AM -0600, Alex Williamson wrote: > FWIW, I think pcim_intx() and pci_intx() align better to our naming > convention for devres interfaces. Would it be sufficient if pci_intx() > triggered a WARN_ON if called for a pci_is_managed() device? Thanks, > To be honest, I also don't mind if you also just merge the patchset as-is. I was mostly just throwing out ideas. regards, dan carpenter
On Thu, 2024-10-10 at 11:43 -0600, Alex Williamson wrote: > On Thu, 10 Oct 2024 11:11:36 +0200 > Philipp Stanner <pstanner@redhat.com> wrote: > > > On Thu, 2024-10-10 at 11:50 +0300, Dan Carpenter wrote: > > > On Wed, Oct 09, 2024 at 10:35:19AM +0200, Philipp Stanner wrote: > > > > pci_intx() is a hybrid function which can sometimes be managed > > > > through > > > > devres. This hybrid nature is undesirable. > > > > > > > > Since all users of pci_intx() have by now been ported either to > > > > always-managed pcim_intx() or never-managed > > > > pci_intx_unmanaged(), > > > > the > > > > devres functionality can be removed from pci_intx(). > > > > > > > > Consequently, pci_intx_unmanaged() is now redundant, because > > > > pci_intx() > > > > itself is now unmanaged. > > > > > > > > Remove the devres functionality from pci_intx(). Remove > > > > pci_intx_unmanaged(). > > > > Have all users of pci_intx_unmanaged() call pci_intx(). > > > > > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > > > > > I don't like when we change a function like this but it still > > > compiles fine. > > > If someone is working on a driver and hasn't pushed it yet, then > > > it's > > > probably > > > supposed to be using the new pcim_intx() but they won't discover > > > that > > > until they > > > detect the leaks at runtime. > > > > There wouldn't be any *leaks*, it's just that the INTx state would > > not > > automatically be restored. BTW the official documentation in its > > current state does not hint at pci_intx() doing anything > > automatically, > > but rather actively marks it as deprecated. > > > > But you are right that a hypothetical new driver and OOT drivers > > could > > experience bugs through this change. > > > > > > > > Why not leave the pci_intx_unmanaged() name. It's ugly and that > > > will > > > discorage > > > people from introducing new uses. > > > > I'd be OK with that. Then we'd have to remove pci_intx() as it has > > new > > users anymore. > > > > Either way should be fine and keep the behavior for existing > > drivers > > identical. > > > > I think Bjorn should express a preference > > FWIW, I think pcim_intx() and pci_intx() align better to our naming > convention for devres interfaces. Yup, also my personal preference. But we can mark those functions as deprecated via docstring-comment. That should fullfill Damien's goal. > Would it be sufficient if pci_intx() > triggered a WARN_ON if called for a pci_is_managed() device? No, I don't think that's a good idea; reason being that pci_is_managed() just checks that global boolean which we inherited from the old implementation and which should not be necessary with proper devres. The boolean is used for making functions such as pci_intx() and __pci_request_region() hybrid. So with our non-hybrid version we never need it. P. > Thanks, > > Alex >
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 475a3ae5c33f..402558fd2436 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 318cfb5b5e15..2d5992fbd413 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4476,43 +4476,12 @@ 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) - 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) { @@ -4525,15 +4494,8 @@ void pci_intx(struct pci_dev *pdev, int enable) 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; - } - + if (new != pci_command) 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);
pci_intx() is a hybrid function which can sometimes be managed through devres. This hybrid nature is undesirable. Since all users of pci_intx() have by now been ported either to always-managed pcim_intx() or never-managed pci_intx_unmanaged(), the devres functionality can be removed from pci_intx(). Consequently, pci_intx_unmanaged() is now redundant, because pci_intx() itself is now unmanaged. Remove the devres functionality from pci_intx(). Remove pci_intx_unmanaged(). Have all users of pci_intx_unmanaged() call pci_intx(). Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- 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 | 40 +------------------ 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, 21 insertions(+), 60 deletions(-)