| Message ID | 20250309084110.267883135@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | genirq/msi: Spring cleaning | expand |
On Sun, 9 Mar 2025 09:41:44 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote: > Provide a lock guard for MSI descriptor locking and update the core code > accordingly. > > No functional change intended. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> One case in here made me go back to the helpful docs Dan wrote after early confusion on how to use this stuff. We might want to consider updating as you've found a case that I at least didn't consider when reviewing those. +CC Dan. Anyhow, this seems fine to me. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- a/kernel/irq/msi.c > +++ b/kernel/irq/msi.c > @@ -443,7 +443,6 @@ EXPORT_SYMBOL_GPL(msi_next_desc); > > @@ -1037,25 +1032,23 @@ bool msi_create_device_irq_domain(struct > if (msi_setup_device_data(dev)) Hmm. We might want to make the docs in cleanup.h more nuanced. They specifically say to not mix goto and auto cleanup, but in the case of scoped_guard() unlikely almost any other case it should be fine. > goto free_fwnode; > > - msi_lock_descs(dev); > - > - if (WARN_ON_ONCE(msi_get_device_domain(dev, domid))) > - goto fail; > - > - if (!pops->init_dev_msi_info(dev, parent, parent, &bundle->info)) > - goto fail; > - > - domain = __msi_create_irq_domain(fwnode, &bundle->info, IRQ_DOMAIN_FLAG_MSI_DEVICE, parent); > - if (!domain) > - goto fail; > - > - domain->dev = dev; > - dev->msi.data->__domains[domid].domain = domain; > - msi_unlock_descs(dev); > - return true; > + scoped_guard(msi_descs_lock, dev) { > + if (WARN_ON_ONCE(msi_get_device_domain(dev, domid))) > + goto free_fwnode; > + > + if (!pops->init_dev_msi_info(dev, parent, parent, &bundle->info)) > + goto free_fwnode; > + > + domain = __msi_create_irq_domain(fwnode, &bundle->info, IRQ_DOMAIN_FLAG_MSI_DEVICE, > + parent); > + if (!domain) > + goto free_fwnode; > + > + domain->dev = dev; > + dev->msi.data->__domains[domid].domain = domain; > + return true; > + } > > -fail: > - msi_unlock_descs(dev); > free_fwnode: > irq_domain_free_fwnode(fwnalloced); > free_bundle:
On Tue, Mar 11 2025 at 18:00, Jonathan Cameron wrote: > On Sun, 9 Mar 2025 09:41:44 +0100 (CET) > Thomas Gleixner <tglx@linutronix.de> wrote: >> >> @@ -1037,25 +1032,23 @@ bool msi_create_device_irq_domain(struct >> if (msi_setup_device_data(dev)) > > Hmm. We might want to make the docs in cleanup.h more nuanced. > They specifically say to not mix goto and auto cleanup, but > in the case of scoped_guard() unlikely almost any other case > it should be fine. > >> goto free_fwnode; I got rid of the gotos. It requires __free() for the two allocations. Thanks, tglx --- --- a/include/linux/cleanup.h +++ b/include/linux/cleanup.h @@ -216,6 +216,8 @@ const volatile void * __must_check_fn(co #define return_ptr(p) return no_free_ptr(p) +#define retain_ptr(p) \ + __get_and_null(p, NULL) /* * DEFINE_CLASS(name, type, exit, init, init_args...): --- a/include/linux/irqdomain.h +++ b/include/linux/irqdomain.h @@ -281,6 +281,8 @@ static inline struct fwnode_handle *irq_ void irq_domain_free_fwnode(struct fwnode_handle *fwnode); +DEFINE_FREE(irq_domain_free_fwnode, struct fwnode_handle *, if (_T) irq_domain_free_fwnode(_T)) + struct irq_domain_chip_generic_info; /** --- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -227,6 +227,9 @@ int msi_setup_device_data(struct device void msi_lock_descs(struct device *dev); void msi_unlock_descs(struct device *dev); +DEFINE_LOCK_GUARD_1(msi_descs_lock, struct device, msi_lock_descs(_T->lock), + msi_unlock_descs(_T->lock)); + struct msi_desc *msi_domain_first_desc(struct device *dev, unsigned int domid, enum msi_desc_filter filter); --- a/kernel/irq/msi.c +++ b/kernel/irq/msi.c @@ -442,7 +442,6 @@ EXPORT_SYMBOL_GPL(msi_next_desc); unsigned int msi_domain_get_virq(struct device *dev, unsigned int domid, unsigned int index) { struct msi_desc *desc; - unsigned int ret = 0; bool pcimsi = false; struct xarray *xa; @@ -456,7 +455,7 @@ unsigned int msi_domain_get_virq(struct if (dev_is_pci(dev) && domid == MSI_DEFAULT_DOMAIN) pcimsi = to_pci_dev(dev)->msi_enabled; - msi_lock_descs(dev); + guard(msi_descs_lock)(dev); xa = &dev->msi.data->__domains[domid].store; desc = xa_load(xa, pcimsi ? 0 : index); if (desc && desc->irq) { @@ -465,16 +464,12 @@ unsigned int msi_domain_get_virq(struct * PCI-MSIX and platform MSI use a descriptor per * interrupt. */ - if (pcimsi) { - if (index < desc->nvec_used) - ret = desc->irq + index; - } else { - ret = desc->irq; - } + if (!pcimsi) + return desc->irq; + if (index < desc->nvec_used) + return desc->irq + index; } - - msi_unlock_descs(dev); - return ret; + return 0; } EXPORT_SYMBOL_GPL(msi_domain_get_virq); @@ -974,9 +969,8 @@ bool msi_create_device_irq_domain(struct void *chip_data) { struct irq_domain *domain, *parent = dev->msi.domain; - struct fwnode_handle *fwnode, *fwnalloced = NULL; - struct msi_domain_template *bundle; const struct msi_parent_ops *pops; + struct fwnode_handle *fwnode; if (!irq_domain_is_msi_parent(parent)) return false; @@ -984,7 +978,8 @@ bool msi_create_device_irq_domain(struct if (domid >= MSI_MAX_DEVICE_IRQDOMAINS) return false; - bundle = kmemdup(template, sizeof(*bundle), GFP_KERNEL); + struct msi_domain_template *bundle __free(kfree) = + bundle = kmemdup(template, sizeof(*bundle), GFP_KERNEL); if (!bundle) return false; @@ -1007,41 +1002,36 @@ bool msi_create_device_irq_domain(struct * node as they are not guaranteed to have a fwnode. They are never * looked up and always handled in the context of the device. */ - if (bundle->info.flags & MSI_FLAG_USE_DEV_FWNODE) - fwnode = dev->fwnode; + struct fwnode_handle *fwnode_alloced __free(irq_domain_free_fwnode) = NULL; + + if (!(bundle->info.flags & MSI_FLAG_USE_DEV_FWNODE)) + fwnode = fwnode_alloced = irq_domain_alloc_named_fwnode(bundle->name); else - fwnode = fwnalloced = irq_domain_alloc_named_fwnode(bundle->name); + fwnode = dev->fwnode; if (!fwnode) - goto free_bundle; + return false; if (msi_setup_device_data(dev)) - goto free_fwnode; - - msi_lock_descs(dev); + return false; + guard(msi_descs_lock)(dev); if (WARN_ON_ONCE(msi_get_device_domain(dev, domid))) - goto fail; + return false; if (!pops->init_dev_msi_info(dev, parent, parent, &bundle->info)) - goto fail; + return false; domain = __msi_create_irq_domain(fwnode, &bundle->info, IRQ_DOMAIN_FLAG_MSI_DEVICE, parent); if (!domain) - goto fail; + return false; + /* @bundle and @fwnode_alloced are now in use. Prevent cleanup */ + retain_ptr(bundle); + retain_ptr(fwnode_alloced); domain->dev = dev; dev->msi.data->__domains[domid].domain = domain; - msi_unlock_descs(dev); return true; - -fail: - msi_unlock_descs(dev); -free_fwnode: - irq_domain_free_fwnode(fwnalloced); -free_bundle: - kfree(bundle); - return false; } /** @@ -1055,12 +1045,10 @@ void msi_remove_device_irq_domain(struct struct msi_domain_info *info; struct irq_domain *domain; - msi_lock_descs(dev); - + guard(msi_descs_lock)(dev); domain = msi_get_device_domain(dev, domid); - if (!domain || !irq_domain_is_msi_device(domain)) - goto unlock; + return; dev->msi.data->__domains[domid].domain = NULL; info = domain->host_data; @@ -1069,9 +1057,6 @@ void msi_remove_device_irq_domain(struct irq_domain_remove(domain); irq_domain_free_fwnode(fwnode); kfree(container_of(info, struct msi_domain_template, info)); - -unlock: - msi_unlock_descs(dev); } /** @@ -1087,16 +1072,14 @@ bool msi_match_device_irq_domain(struct { struct msi_domain_info *info; struct irq_domain *domain; - bool ret = false; - msi_lock_descs(dev); + guard(msi_descs_lock)(dev); domain = msi_get_device_domain(dev, domid); if (domain && irq_domain_is_msi_device(domain)) { info = domain->host_data; - ret = info->bus_token == bus_token; + return info->bus_token == bus_token; } - msi_unlock_descs(dev); - return ret; + return false; } static int msi_domain_prepare_irqs(struct irq_domain *domain, struct device *dev, @@ -1346,12 +1329,9 @@ int msi_domain_alloc_irqs_range(struct d .last = last, .nirqs = last + 1 - first, }; - int ret; - msi_lock_descs(dev); - ret = msi_domain_alloc_locked(dev, &ctrl); - msi_unlock_descs(dev); - return ret; + guard(msi_descs_lock)(dev); + return msi_domain_alloc_locked(dev, &ctrl); } EXPORT_SYMBOL_GPL(msi_domain_alloc_irqs_range); @@ -1455,12 +1435,8 @@ struct msi_map msi_domain_alloc_irq_at(s const struct irq_affinity_desc *affdesc, union msi_instance_cookie *icookie) { - struct msi_map map; - - msi_lock_descs(dev); - map = __msi_domain_alloc_irq_at(dev, domid, index, affdesc, icookie); - msi_unlock_descs(dev); - return map; + guard(msi_descs_lock)(dev); + return __msi_domain_alloc_irq_at(dev, domid, index, affdesc, icookie); } /** @@ -1497,13 +1473,11 @@ int msi_device_domain_alloc_wired(struct icookie.value = ((u64)type << 32) | hwirq; - msi_lock_descs(dev); + guard(msi_descs_lock)(dev); if (WARN_ON_ONCE(msi_get_device_domain(dev, domid) != domain)) map.index = -EINVAL; else map = __msi_domain_alloc_irq_at(dev, domid, MSI_ANY_INDEX, NULL, &icookie); - msi_unlock_descs(dev); - return map.index >= 0 ? map.virq : map.index; } @@ -1596,9 +1570,8 @@ static void msi_domain_free_irqs_range_l void msi_domain_free_irqs_range(struct device *dev, unsigned int domid, unsigned int first, unsigned int last) { - msi_lock_descs(dev); + guard(msi_descs_lock)(dev); msi_domain_free_irqs_range_locked(dev, domid, first, last); - msi_unlock_descs(dev); } EXPORT_SYMBOL_GPL(msi_domain_free_irqs_all); @@ -1628,9 +1601,8 @@ void msi_domain_free_irqs_all_locked(str */ void msi_domain_free_irqs_all(struct device *dev, unsigned int domid) { - msi_lock_descs(dev); + guard(msi_descs_lock)(dev); msi_domain_free_irqs_all_locked(dev, domid); - msi_unlock_descs(dev); } /** @@ -1649,12 +1621,11 @@ void msi_device_domain_free_wired(struct if (WARN_ON_ONCE(!dev || !desc || domain->bus_token != DOMAIN_BUS_WIRED_TO_MSI)) return; - msi_lock_descs(dev); - if (!WARN_ON_ONCE(msi_get_device_domain(dev, MSI_DEFAULT_DOMAIN) != domain)) { - msi_domain_free_irqs_range_locked(dev, MSI_DEFAULT_DOMAIN, desc->msi_index, - desc->msi_index); - } - msi_unlock_descs(dev); + guard(msi_descs_lock)(dev); + if (WARN_ON_ONCE(msi_get_device_domain(dev, MSI_DEFAULT_DOMAIN) != domain)) + return; + msi_domain_free_irqs_range_locked(dev, MSI_DEFAULT_DOMAIN, desc->msi_index, + desc->msi_index); } /**
On Tue, 11 Mar 2025 22:26:52 +0100 Thomas Gleixner <tglx@linutronix.de> wrote: > On Tue, Mar 11 2025 at 18:00, Jonathan Cameron wrote: > > On Sun, 9 Mar 2025 09:41:44 +0100 (CET) > > Thomas Gleixner <tglx@linutronix.de> wrote: > > >> > >> @@ -1037,25 +1032,23 @@ bool msi_create_device_irq_domain(struct > >> if (msi_setup_device_data(dev)) > > > > Hmm. We might want to make the docs in cleanup.h more nuanced. > > They specifically say to not mix goto and auto cleanup, but > > in the case of scoped_guard() unlikely almost any other case > > it should be fine. > > > >> goto free_fwnode; > > I got rid of the gotos. It requires __free() for the two allocations. That works. > > Thanks, > > tglx > --- > --- a/include/linux/cleanup.h > +++ b/include/linux/cleanup.h > @@ -216,6 +216,8 @@ const volatile void * __must_check_fn(co > > #define return_ptr(p) return no_free_ptr(p) > > +#define retain_ptr(p) \ > + __get_and_null(p, NULL) Single line? This sort of thing got discussed in the past though I doubt I can find the thread. There was some push back but maybe now is it's time! Probably worth shouting about it a bit to attract attention. Maybe a separate patch. Jonathan
On Wed, Mar 12 2025 at 15:08, Jonathan Cameron wrote: > On Tue, 11 Mar 2025 22:26:52 +0100 > Thomas Gleixner <tglx@linutronix.de> wrote: >> >> #define return_ptr(p) return no_free_ptr(p) >> >> +#define retain_ptr(p) \ >> + __get_and_null(p, NULL) > Single line? > > This sort of thing got discussed in the past though I doubt I can find > the thread. There was some push back but maybe now is it's time! > > Probably worth shouting about it a bit to attract attention. Maybe > a separate patch. Yes of course.
--- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -227,6 +227,9 @@ int msi_setup_device_data(struct device void msi_lock_descs(struct device *dev); void msi_unlock_descs(struct device *dev); +DEFINE_LOCK_GUARD_1(msi_descs_lock, struct device, msi_lock_descs(_T->lock), + msi_unlock_descs(_T->lock)); + struct msi_desc *msi_domain_first_desc(struct device *dev, unsigned int domid, enum msi_desc_filter filter); --- a/kernel/irq/msi.c +++ b/kernel/irq/msi.c @@ -443,7 +443,6 @@ EXPORT_SYMBOL_GPL(msi_next_desc); unsigned int msi_domain_get_virq(struct device *dev, unsigned int domid, unsigned int index) { struct msi_desc *desc; - unsigned int ret = 0; bool pcimsi = false; struct xarray *xa; @@ -457,7 +456,7 @@ unsigned int msi_domain_get_virq(struct if (dev_is_pci(dev) && domid == MSI_DEFAULT_DOMAIN) pcimsi = to_pci_dev(dev)->msi_enabled; - msi_lock_descs(dev); + guard(msi_descs_lock)(dev); xa = &dev->msi.data->__domains[domid].store; desc = xa_load(xa, pcimsi ? 0 : index); if (desc && desc->irq) { @@ -466,16 +465,12 @@ unsigned int msi_domain_get_virq(struct * PCI-MSIX and platform MSI use a descriptor per * interrupt. */ - if (pcimsi) { - if (index < desc->nvec_used) - ret = desc->irq + index; - } else { - ret = desc->irq; - } + if (!pcimsi) + return desc->irq; + if (index < desc->nvec_used) + return desc->irq + index; } - - msi_unlock_descs(dev); - return ret; + return 0; } EXPORT_SYMBOL_GPL(msi_domain_get_virq); @@ -1037,25 +1032,23 @@ bool msi_create_device_irq_domain(struct if (msi_setup_device_data(dev)) goto free_fwnode; - msi_lock_descs(dev); - - if (WARN_ON_ONCE(msi_get_device_domain(dev, domid))) - goto fail; - - if (!pops->init_dev_msi_info(dev, parent, parent, &bundle->info)) - goto fail; - - domain = __msi_create_irq_domain(fwnode, &bundle->info, IRQ_DOMAIN_FLAG_MSI_DEVICE, parent); - if (!domain) - goto fail; - - domain->dev = dev; - dev->msi.data->__domains[domid].domain = domain; - msi_unlock_descs(dev); - return true; + scoped_guard(msi_descs_lock, dev) { + if (WARN_ON_ONCE(msi_get_device_domain(dev, domid))) + goto free_fwnode; + + if (!pops->init_dev_msi_info(dev, parent, parent, &bundle->info)) + goto free_fwnode; + + domain = __msi_create_irq_domain(fwnode, &bundle->info, IRQ_DOMAIN_FLAG_MSI_DEVICE, + parent); + if (!domain) + goto free_fwnode; + + domain->dev = dev; + dev->msi.data->__domains[domid].domain = domain; + return true; + } -fail: - msi_unlock_descs(dev); free_fwnode: irq_domain_free_fwnode(fwnalloced); free_bundle: @@ -1074,12 +1067,10 @@ void msi_remove_device_irq_domain(struct struct msi_domain_info *info; struct irq_domain *domain; - msi_lock_descs(dev); - + guard(msi_descs_lock)(dev); domain = msi_get_device_domain(dev, domid); - if (!domain || !irq_domain_is_msi_device(domain)) - goto unlock; + return; dev->msi.data->__domains[domid].domain = NULL; info = domain->host_data; @@ -1088,9 +1079,6 @@ void msi_remove_device_irq_domain(struct irq_domain_remove(domain); irq_domain_free_fwnode(fwnode); kfree(container_of(info, struct msi_domain_template, info)); - -unlock: - msi_unlock_descs(dev); } /** @@ -1106,16 +1094,14 @@ bool msi_match_device_irq_domain(struct { struct msi_domain_info *info; struct irq_domain *domain; - bool ret = false; - msi_lock_descs(dev); + guard(msi_descs_lock)(dev); domain = msi_get_device_domain(dev, domid); if (domain && irq_domain_is_msi_device(domain)) { info = domain->host_data; - ret = info->bus_token == bus_token; + return info->bus_token == bus_token; } - msi_unlock_descs(dev); - return ret; + return false; } static int msi_domain_prepare_irqs(struct irq_domain *domain, struct device *dev, @@ -1365,12 +1351,9 @@ int msi_domain_alloc_irqs_range(struct d .last = last, .nirqs = last + 1 - first, }; - int ret; - msi_lock_descs(dev); - ret = msi_domain_alloc_locked(dev, &ctrl); - msi_unlock_descs(dev); - return ret; + guard(msi_descs_lock)(dev); + return msi_domain_alloc_locked(dev, &ctrl); } EXPORT_SYMBOL_GPL(msi_domain_alloc_irqs_range); @@ -1474,12 +1457,8 @@ struct msi_map msi_domain_alloc_irq_at(s const struct irq_affinity_desc *affdesc, union msi_instance_cookie *icookie) { - struct msi_map map; - - msi_lock_descs(dev); - map = __msi_domain_alloc_irq_at(dev, domid, index, affdesc, icookie); - msi_unlock_descs(dev); - return map; + guard(msi_descs_lock)(dev); + return __msi_domain_alloc_irq_at(dev, domid, index, affdesc, icookie); } /** @@ -1516,13 +1495,11 @@ int msi_device_domain_alloc_wired(struct icookie.value = ((u64)type << 32) | hwirq; - msi_lock_descs(dev); + guard(msi_descs_lock)(dev); if (WARN_ON_ONCE(msi_get_device_domain(dev, domid) != domain)) map.index = -EINVAL; else map = __msi_domain_alloc_irq_at(dev, domid, MSI_ANY_INDEX, NULL, &icookie); - msi_unlock_descs(dev); - return map.index >= 0 ? map.virq : map.index; } @@ -1615,9 +1592,8 @@ static void msi_domain_free_irqs_range_l void msi_domain_free_irqs_range(struct device *dev, unsigned int domid, unsigned int first, unsigned int last) { - msi_lock_descs(dev); + guard(msi_descs_lock)(dev); msi_domain_free_irqs_range_locked(dev, domid, first, last); - msi_unlock_descs(dev); } EXPORT_SYMBOL_GPL(msi_domain_free_irqs_all); @@ -1647,9 +1623,8 @@ void msi_domain_free_irqs_all_locked(str */ void msi_domain_free_irqs_all(struct device *dev, unsigned int domid) { - msi_lock_descs(dev); + guard(msi_descs_lock)(dev); msi_domain_free_irqs_all_locked(dev, domid); - msi_unlock_descs(dev); } /** @@ -1668,12 +1643,11 @@ void msi_device_domain_free_wired(struct if (WARN_ON_ONCE(!dev || !desc || domain->bus_token != DOMAIN_BUS_WIRED_TO_MSI)) return; - msi_lock_descs(dev); - if (!WARN_ON_ONCE(msi_get_device_domain(dev, MSI_DEFAULT_DOMAIN) != domain)) { - msi_domain_free_irqs_range_locked(dev, MSI_DEFAULT_DOMAIN, desc->msi_index, - desc->msi_index); - } - msi_unlock_descs(dev); + guard(msi_descs_lock)(dev); + if (WARN_ON_ONCE(msi_get_device_domain(dev, MSI_DEFAULT_DOMAIN) != domain)) + return; + msi_domain_free_irqs_range_locked(dev, MSI_DEFAULT_DOMAIN, desc->msi_index, + desc->msi_index); } /**
Provide a lock guard for MSI descriptor locking and update the core code accordingly. No functional change intended. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- include/linux/msi.h | 3 + kernel/irq/msi.c | 104 +++++++++++++++++++--------------------------------- 2 files changed, 42 insertions(+), 65 deletions(-)