| Message ID | 20241113124158.22863-3-pstanner@redhat.com (mailing list archive) |
|---|---|
| State | New |
| 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: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 >
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 225a6cd2e9ca..c945811b207a 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4480,6 +4480,35 @@ void pci_disable_parity(struct pci_dev *dev) } } +/** + * 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 + * + * 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);