| Message ID | 20241015185124.64726-2-pstanner@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove implicit devres from pci_intx() | expand |
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
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 >
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);