| Message ID | 20241113124158.22863-2-pstanner@redhat.com |
|---|---|
| Headers | show |
| Series | Remove implicit devres from pci_intx() | expand |
On Wed, 2024-11-13 at 17:04 +0100, Thomas Gleixner wrote: > 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. True, but this is a *temporary* copy of pci_intx(), a ~16 year old function. Older C programmers had the habit of for some reason using 32-bit integers for a true/false boolean all the time. We _could_ think of changing pci_intx()'s parameter to a boolean, but I think it wouldn't really improve things very much see also below > > > + * Enables/disables PCI INTx for device @pdev > > + * > > + * This function behavios identically to pci_intx(), but is never > > managed with > > + * devres. > > behavios? -> behaves. Will fix. > > > + */ > > +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? We had this conversation last week. I answered on that already, maybe you have overlooked it: https://lore.kernel.org/all/a8d9f32f60f55c58d79943c4409b8b94535ff853.camel@redhat.com/ Please also take a look at patch 11, then you'll see the full picture Danke, Philipp > > Thanks, > > tglx >
@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(-)