| Message ID | 20250309083453.900516105@linutronix.de |
|---|---|
| Headers | show |
| Series | genirq/msi: Spring cleaning | expand |
On Sun, Mar 09, 2025 at 09:41:40AM +0100, Thomas Gleixner wrote: > While converting the MSI descriptor locking to a lock guard() I stumbled > over various abuse of MSI descriptors (again). > > The following series cleans up the offending code and converts the MSI > descriptor locking over to lock guard(). > > The series applies on Linus tree and is also available from git: > > git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git irq/msi > > Thanks, > > tglx > --- > drivers/ntb/msi.c | 22 +---- > drivers/pci/controller/pci-hyperv.c | 14 --- > drivers/pci/msi/api.c | 6 - > drivers/pci/msi/msi.c | 77 ++++++++++++++---- > drivers/pci/pci.h | 9 ++ > drivers/pci/tph.c | 44 ---------- > drivers/soc/ti/ti_sci_inta_msi.c | 10 -- > drivers/ufs/host/ufs-qcom.c | 75 +++++++++--------- > include/linux/msi.h | 12 +- > kernel/irq/msi.c | 150 ++++++++++++------------------------ > 10 files changed, 181 insertions(+), 238 deletions(-) For the drivers/pci/ parts: Acked-by: Bjorn Helgaas <bhelgaas@google.com> I assume you'll merge this somewhere, let me know if otherwise.
On Mon, Mar 10 2025 at 11:51, Bjorn Helgaas wrote: > For the drivers/pci/ parts: > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > I assume you'll merge this somewhere, let me know if otherwise. Yes. I intend to get it through tip.
On Sun, 9 Mar 2025 09:41:42 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote: > None of these functions are used outside of the MSI core. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
On Sun, 9 Mar 2025 09:41:46 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote: > Convert the code to use the new guard(msi_descs_lock). > > No functional change intended. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Nishanth Menon <nm@ti.com> > Cc: Tero Kristo <kristo@kernel.org> > Cc: Santosh Shilimkar <ssantosh@kernel.org> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
On Sun, 9 Mar 2025 09:41:49 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote: > Convert the code to use the new guard(msi_descs_lock). > > No functional change intended. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org This one runs into some of the stuff that the docs say to not do. I don't there there are bugs in here as such but it gets harder to reason about and fragile in the long run. Easy enough to avoid though - see inline. Jonathan > --- a/drivers/pci/msi/msi.c > +++ b/drivers/pci/msi/msi.c > @@ -351,7 +351,7 @@ static int msi_verify_entries(struct pci > static int msi_capability_init(struct pci_dev *dev, int nvec, > struct irq_affinity *affd) > { > - struct irq_affinity_desc *masks = NULL; > + struct irq_affinity_desc *masks __free(kfree) = NULL; This is a pattern that the cleanup.h docs talk about that Linus really didn't like in some of the early patches because of wanting constructors and destructors together. Maybe use an inline definition as struct irq_affinity_desc *masks = affd ? irq_create_affinity_masks(nvec, affd) : NULL; > struct msi_desc *entry, desc; > int ret; > > @@ -369,7 +369,7 @@ static int msi_capability_init(struct pc > if (affd) > masks = irq_create_affinity_masks(nvec, affd); > > - msi_lock_descs(&dev->dev); > + guard(msi_descs_lock)(&dev->dev); > ret = msi_setup_msi_desc(dev, nvec, masks); > if (ret) > goto fail; This runs against the advice in cleanup.h. It is probably correct and doesn't hit the undefined paths that motivated that guidance, but still maybe worth avoiding the mix of cleanup and goto handling. Easiest way being to factor out the code after guard to a helper. > @@ -399,16 +399,13 @@ static int msi_capability_init(struct pc > > pcibios_free_irq(dev); > dev->irq = entry->irq; > - goto unlock; > + return 0; > > err: > pci_msi_unmask(&desc, msi_multi_mask(&desc)); > pci_free_msi_irqs(dev); > fail: > dev->msi_enabled = 0; > -unlock: > - msi_unlock_descs(&dev->dev); > - kfree(masks); > return ret; > } > > @@ -669,13 +666,13 @@ static void msix_mask_all(void __iomem * > static int msix_setup_interrupts(struct pci_dev *dev, struct msix_entry *entries, > int nvec, struct irq_affinity *affd) > { > - struct irq_affinity_desc *masks = NULL; > + struct irq_affinity_desc *masks __free(kfree) = NULL; Similar to above. > int ret; > > if (affd) > masks = irq_create_affinity_masks(nvec, affd); > > - msi_lock_descs(&dev->dev); > + guard(msi_descs_lock)(&dev->dev); > ret = msix_setup_msi_descs(dev, entries, nvec, masks); > if (ret) > goto out_free; > @@ -690,13 +687,10 @@ static int msix_setup_interrupts(struct > goto out_free; > > msix_update_entries(dev, entries); > - goto out_unlock; > + return 0; > > out_free: Here as well. > pci_free_msi_irqs(dev); > -out_unlock: > - msi_unlock_descs(&dev->dev); > - kfree(masks); > return ret; > } > > @@ -871,13 +865,13 @@ void __pci_restore_msix_state(struct pci > > write_msg = arch_restore_msi_irqs(dev); > > - msi_lock_descs(&dev->dev); > - msi_for_each_desc(entry, &dev->dev, MSI_DESC_ALL) { > - if (write_msg) > - __pci_write_msi_msg(entry, &entry->msg); > - pci_msix_write_vector_ctrl(entry, entry->pci.msix_ctrl); > + scoped_guard (msi_descs_lock, &dev->dev) { > + msi_for_each_desc(entry, &dev->dev, MSI_DESC_ALL) { > + if (write_msg) > + __pci_write_msi_msg(entry, &entry->msg); > + pci_msix_write_vector_ctrl(entry, entry->pci.msix_ctrl); > + } > } > - msi_unlock_descs(&dev->dev); > > pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0); > } > > >
While converting the MSI descriptor locking to a lock guard() I stumbled over various abuse of MSI descriptors (again). The following series cleans up the offending code and converts the MSI descriptor locking over to lock guard(). The series applies on Linus tree and is also available from git: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git irq/msi Thanks, tglx --- drivers/ntb/msi.c | 22 +---- drivers/pci/controller/pci-hyperv.c | 14 --- drivers/pci/msi/api.c | 6 - drivers/pci/msi/msi.c | 77 ++++++++++++++---- drivers/pci/pci.h | 9 ++ drivers/pci/tph.c | 44 ---------- drivers/soc/ti/ti_sci_inta_msi.c | 10 -- drivers/ufs/host/ufs-qcom.c | 75 +++++++++--------- include/linux/msi.h | 12 +- kernel/irq/msi.c | 150 ++++++++++++------------------------ 10 files changed, 181 insertions(+), 238 deletions(-)