Re: [patch 32/39] PCI/MSI: Reorder functions in msi.c
On Wed, Nov 16 2022 at 10:28, Bjorn Helgaas wrote: > On Fri, Nov 11, 2022 at 02:55:06PM +0100, Thomas Gleixner wrote: >> From: Ahmed S. Darwish >> >> There is no way to navigate msi.c without banging the head against the wall >> every now and then because MSI and MSI-X specific functions are >> intermingled and the code flow is completely non-obvious. >> >> Reorder everthing so common helpers, MSI and MSI-X specific functions are >> grouped together. > > s/everthing/everything/ > >> Suggested-by: Thomas Gleixner >> Signed-off-by: Ahmed S. Darwish >> Signed-off-by: Thomas Gleixner > > Acked-by: Bjorn Helgaas > > I assume this is pure code movement, so I didn't even look at the > text below. It is.
Re: [patch 32/39] PCI/MSI: Reorder functions in msi.c
On Fri, Nov 11, 2022 at 02:55:06PM +0100, Thomas Gleixner wrote: > From: Ahmed S. Darwish > > There is no way to navigate msi.c without banging the head against the wall > every now and then because MSI and MSI-X specific functions are > intermingled and the code flow is completely non-obvious. > > Reorder everthing so common helpers, MSI and MSI-X specific functions are > grouped together. s/everthing/everything/ > Suggested-by: Thomas Gleixner > Signed-off-by: Ahmed S. Darwish > Signed-off-by: Thomas Gleixner Acked-by: Bjorn Helgaas I assume this is pure code movement, so I didn't even look at the text below. > --- > drivers/pci/msi/msi.c | 577 > +- > 1 file changed, 295 insertions(+), 282 deletions(-) > > --- a/drivers/pci/msi/msi.c > +++ b/drivers/pci/msi/msi.c > @@ -16,6 +16,97 @@ > int pci_msi_enable = 1; > int pci_msi_ignore_mask; > > +/** > + * pci_msi_supported - check whether MSI may be enabled on a device > + * @dev: pointer to the pci_dev data structure of MSI device function > + * @nvec: how many MSIs have been requested? > + * > + * Look at global flags, the device itself, and its parent buses > + * to determine if MSI/-X are supported for the device. If MSI/-X is > + * supported return 1, else return 0. > + **/ > +static int pci_msi_supported(struct pci_dev *dev, int nvec) > +{ > + struct pci_bus *bus; > + > + /* MSI must be globally enabled and supported by the device */ > + if (!pci_msi_enable) > + return 0; > + > + if (!dev || dev->no_msi) > + return 0; > + > + /* > + * You can't ask to have 0 or less MSIs configured. > + * a) it's stupid .. > + * b) the list manipulation code assumes nvec >= 1. > + */ > + if (nvec < 1) > + return 0; > + > + /* > + * Any bridge which does NOT route MSI transactions from its > + * secondary bus to its primary bus must set NO_MSI flag on > + * the secondary pci_bus. > + * > + * The NO_MSI flag can either be set directly by: > + * - arch-specific PCI host bus controller drivers (deprecated) > + * - quirks for specific PCI bridges > + * > + * or indirectly by platform-specific PCI host bridge drivers by > + * advertising the 'msi_domain' property, which results in > + * the NO_MSI flag when no MSI domain is found for this bridge > + * at probe time. > + */ > + for (bus = dev->bus; bus; bus = bus->parent) > + if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI) > + return 0; > + > + return 1; > +} > + > +static void pcim_msi_release(void *pcidev) > +{ > + struct pci_dev *dev = pcidev; > + > + dev->is_msi_managed = false; > + pci_free_irq_vectors(dev); > +} > + > +/* > + * Needs to be separate from pcim_release to prevent an ordering problem > + * vs. msi_device_data_release() in the MSI core code. > + */ > +static int pcim_setup_msi_release(struct pci_dev *dev) > +{ > + int ret; > + > + if (!pci_is_managed(dev) || dev->is_msi_managed) > + return 0; > + > + ret = devm_add_action(>dev, pcim_msi_release, dev); > + if (!ret) > + dev->is_msi_managed = true; > + return ret; > +} > + > +/* > + * Ordering vs. devres: msi device data has to be installed first so that > + * pcim_msi_release() is invoked before it on device release. > + */ > +static int pci_setup_msi_context(struct pci_dev *dev) > +{ > + int ret = msi_setup_device_data(>dev); > + > + if (!ret) > + ret = pcim_setup_msi_release(dev); > + return ret; > +} > + > +/* > + * Helper functions for mask/unmask and MSI message handling > + */ > + > void pci_msi_update_mask(struct msi_desc *desc, u32 clear, u32 set) > { > raw_spinlock_t *lock = _pci_dev(desc->dev)->msi_lock; > @@ -163,15 +254,8 @@ void pci_write_msi_msg(unsigned int irq, > } > EXPORT_SYMBOL_GPL(pci_write_msi_msg); > > -void pci_free_msi_irqs(struct pci_dev *dev) > -{ > - pci_msi_teardown_msi_irqs(dev); > > - if (dev->msix_base) { > - iounmap(dev->msix_base); > - dev->msix_base = NULL; > - } > -} > +/* PCI/MSI specific functionality */ > > static void pci_intx_for_msi(struct pci_dev *dev, int enable) > { > @@ -190,111 +274,6 @@ static void pci_msi_set_enable(struct pc > pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, control); > } > > -/* > - * Architecture override returns true when the PCI MSI message should be > - * written by the generic restore function. > - */ > -bool __weak arch_restore_msi_irqs(struct pci_dev *dev) > -{ > - return true; > -} > - > -void __pci_restore_msi_state(struct pci_dev *dev) > -{ > - struct msi_desc *entry; > - u16 control; > - > - if (!dev->msi_enabled) > - return; > - > - entry = irq_get_msi_desc(dev->irq); > - > - pci_intx_for_msi(dev, 0); > - pci_msi_set_enable(dev, 0); > - if
[patch 32/39] PCI/MSI: Reorder functions in msi.c
From: Ahmed S. Darwish There is no way to navigate msi.c without banging the head against the wall every now and then because MSI and MSI-X specific functions are intermingled and the code flow is completely non-obvious. Reorder everthing so common helpers, MSI and MSI-X specific functions are grouped together. Suggested-by: Thomas Gleixner Signed-off-by: Ahmed S. Darwish Signed-off-by: Thomas Gleixner --- drivers/pci/msi/msi.c | 577 +- 1 file changed, 295 insertions(+), 282 deletions(-) --- a/drivers/pci/msi/msi.c +++ b/drivers/pci/msi/msi.c @@ -16,6 +16,97 @@ int pci_msi_enable = 1; int pci_msi_ignore_mask; +/** + * pci_msi_supported - check whether MSI may be enabled on a device + * @dev: pointer to the pci_dev data structure of MSI device function + * @nvec: how many MSIs have been requested? + * + * Look at global flags, the device itself, and its parent buses + * to determine if MSI/-X are supported for the device. If MSI/-X is + * supported return 1, else return 0. + **/ +static int pci_msi_supported(struct pci_dev *dev, int nvec) +{ + struct pci_bus *bus; + + /* MSI must be globally enabled and supported by the device */ + if (!pci_msi_enable) + return 0; + + if (!dev || dev->no_msi) + return 0; + + /* +* You can't ask to have 0 or less MSIs configured. +* a) it's stupid .. +* b) the list manipulation code assumes nvec >= 1. +*/ + if (nvec < 1) + return 0; + + /* +* Any bridge which does NOT route MSI transactions from its +* secondary bus to its primary bus must set NO_MSI flag on +* the secondary pci_bus. +* +* The NO_MSI flag can either be set directly by: +* - arch-specific PCI host bus controller drivers (deprecated) +* - quirks for specific PCI bridges +* +* or indirectly by platform-specific PCI host bridge drivers by +* advertising the 'msi_domain' property, which results in +* the NO_MSI flag when no MSI domain is found for this bridge +* at probe time. +*/ + for (bus = dev->bus; bus; bus = bus->parent) + if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI) + return 0; + + return 1; +} + +static void pcim_msi_release(void *pcidev) +{ + struct pci_dev *dev = pcidev; + + dev->is_msi_managed = false; + pci_free_irq_vectors(dev); +} + +/* + * Needs to be separate from pcim_release to prevent an ordering problem + * vs. msi_device_data_release() in the MSI core code. + */ +static int pcim_setup_msi_release(struct pci_dev *dev) +{ + int ret; + + if (!pci_is_managed(dev) || dev->is_msi_managed) + return 0; + + ret = devm_add_action(>dev, pcim_msi_release, dev); + if (!ret) + dev->is_msi_managed = true; + return ret; +} + +/* + * Ordering vs. devres: msi device data has to be installed first so that + * pcim_msi_release() is invoked before it on device release. + */ +static int pci_setup_msi_context(struct pci_dev *dev) +{ + int ret = msi_setup_device_data(>dev); + + if (!ret) + ret = pcim_setup_msi_release(dev); + return ret; +} + +/* + * Helper functions for mask/unmask and MSI message handling + */ + void pci_msi_update_mask(struct msi_desc *desc, u32 clear, u32 set) { raw_spinlock_t *lock = _pci_dev(desc->dev)->msi_lock; @@ -163,15 +254,8 @@ void pci_write_msi_msg(unsigned int irq, } EXPORT_SYMBOL_GPL(pci_write_msi_msg); -void pci_free_msi_irqs(struct pci_dev *dev) -{ - pci_msi_teardown_msi_irqs(dev); - if (dev->msix_base) { - iounmap(dev->msix_base); - dev->msix_base = NULL; - } -} +/* PCI/MSI specific functionality */ static void pci_intx_for_msi(struct pci_dev *dev, int enable) { @@ -190,111 +274,6 @@ static void pci_msi_set_enable(struct pc pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, control); } -/* - * Architecture override returns true when the PCI MSI message should be - * written by the generic restore function. - */ -bool __weak arch_restore_msi_irqs(struct pci_dev *dev) -{ - return true; -} - -void __pci_restore_msi_state(struct pci_dev *dev) -{ - struct msi_desc *entry; - u16 control; - - if (!dev->msi_enabled) - return; - - entry = irq_get_msi_desc(dev->irq); - - pci_intx_for_msi(dev, 0); - pci_msi_set_enable(dev, 0); - if (arch_restore_msi_irqs(dev)) - __pci_write_msi_msg(entry, >msg); - - pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, ); - pci_msi_update_mask(entry, 0, 0); - control &= ~PCI_MSI_FLAGS_QSIZE; - control |= (entry->pci.msi_attrib.multiple << 4) | PCI_MSI_FLAGS_ENABLE; - pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, control); -} -