| Message ID | 20241113124158.22863-2-pstanner@redhat.com |
|---|---|
| Headers | show |
| Series | Remove implicit devres from pci_intx() | expand |
On Wed, Nov 13 2024 at 13:41, Philipp Stanner wrote: > +/** > + * pci_intx_unmanaged - 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 Except that the argument is of type int, which really should be type bool. > + * Enables/disables PCI INTx for device @pdev > + * > + * This function behavios identically to pci_intx(), but is never managed with > + * devres. behavios? > + */ > +void pci_intx_unmanaged(struct pci_dev *pdev, int enable) I find this function name mildy confusing. This _unmanaged suffix is not really telling me anything. And the reference that this behaves identically to pci_intx() makes it even worse. This function is about controlling the PCI INTX_DISABLE bit in the PCI_COMMAND config word, right? So naming it pci_intx_control() would make it entirely clear what this is about, no? Thanks, tglx
On Wed, 2024-11-13 at 17:22 +0100, Thomas Gleixner wrote: > On Wed, Nov 13 2024 at 13:41, 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(). 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 | 43 +-------------- > > ---- > > 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(+), 62 deletions(-) > > Now I'm utterly confused. This undoes the pci_intx_unmanaged() churn > which you carefully split into several patches first. Have you read the email I have linked? There is also the cover-letter (does anyone in the community ever read those?) which explicitly states: "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." P. > > So the net change is that: > > 1) pci_intx() is now always unmanaged > > 2) a couple of drivers use pcim_intx() now instead of pci_intx() > > The obvious ordering is: > > 1) Convert the drivers which need the managed version to use > pcim_intx() > > 2) Remove the managed warning in pci_intx() and clean up the > comment > > 3) Remove __pcim_intx() and invoke pci_intx() in the devres code. > > No? > > Thanks, > > tglx >
On Thu, Nov 14 2024 at 10:05, Philipp Stanner wrote: > On Wed, 2024-11-13 at 17:22 +0100, Thomas Gleixner wrote: >> On Wed, Nov 13 2024 at 13:41, 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(). 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 | 43 +-------------- >> > ---- >> > 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(+), 62 deletions(-) >> >> Now I'm utterly confused. This undoes the pci_intx_unmanaged() churn >> which you carefully split into several patches first. > > Have you read the email I have linked? > > There is also the cover-letter (does anyone in the community ever read > those?) which explicitly states: > > "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." I read it and I assume your intention was to force an eye on every use case of pci_intx() and not just on those which need to be converted to pcim_intx(). I'm not convinced that this is needed, but fair enough.
On Fri, 2024-11-15 at 01:46 +0100, Thomas Gleixner wrote: > On Thu, Nov 14 2024 at 10:05, Philipp Stanner wrote: > > On Wed, 2024-11-13 at 17:22 +0100, Thomas Gleixner wrote: > > > On Wed, Nov 13 2024 at 13:41, 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(). 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 | 43 +---------- > > > > ---- > > > > ---- > > > > 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(+), 62 deletions(-) > > > > > > Now I'm utterly confused. This undoes the pci_intx_unmanaged() > > > churn > > > which you carefully split into several patches first. > > > > Have you read the email I have linked? > > > > There is also the cover-letter (does anyone in the community ever > > read > > those?) which explicitly states: > > > > "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." > > I read it and I assume your intention was to force an eye on every > use > case of pci_intx() and not just on those which need to be converted > to > pcim_intx(). > > I'm not convinced that this is needed, but fair enough. Whether pcim_enable_device() is really not used could have been overlooked, or the driver could move to "managed mode" in parallel for v6.13 for example. Then a bug would be silently introduced into those drivers. Besides, me touching pci_intx() unfortunately caused a few explosions in the past already, in fc8c818e756991f5f50b8dfab07f970a18da2556 and 00f89ae4e759a7eef07e4188e1534af7dd2c7e9c So this time I prefer to be rather safe than sorry. BTW, if you can review the MSI patch and check whether removing devres from there really is fine, that would be helpful. Regards, P.
On Wed, Nov 13 2024 at 13:41, 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. > > 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> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
@Driver-Maintainers: Your driver might be touched by patch "Remove devres from pci_intx()". You might want to take a look. Changes in v2: - Drop pci_intx() deprecation patch. - ata: Add RB from Sergey and Niklas. - wifi: Add AB by Kalle. - Drop INTx deprecation patch - Drop ALSA / hda_intel patch because pci_intx() was removed from there in the meantime. 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 (11): PCI: Prepare removing devres from pci_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() drivers/ata/ahci.c | 2 +- drivers/ata/ata_piix.c | 2 +- drivers/ata/pata_rdc.c | 2 +- drivers/ata/sata_sil24.c | 2 +- drivers/ata/sata_sis.c | 2 +- drivers/ata/sata_uli.c | 2 +- drivers/ata/sata_vsc.c | 2 +- drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 4 ++-- drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 2 +- .../wireless/quantenna/qtnfmac/pcie/pcie.c | 2 +- drivers/pci/devres.c | 24 +++---------------- drivers/pci/pci.c | 16 +++---------- include/linux/pci.h | 1 + 13 files changed, 18 insertions(+), 45 deletions(-)