| Message ID | 20250313130212.450198939@linutronix.de |
|---|---|
| Headers | show |
| Series | genirq/msi: Spring cleaning | expand |
On Thu, 13 Mar 2025 14:03:38 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote: > In cases where an allocation is consumed by another function, the > allocation needs to be retained on success or freed on failure. The code > pattern is usually: > > struct foo *f = kzalloc(sizeof(*f), GFP_KERNEL); > struct bar *b; > > ,,, > // Initialize f > ... > if (ret) > goto free; > ... > bar = bar_create(f); > if (!bar) { > ret = -ENOMEM; > goto free; > } > ... > return 0; > free: > kfree(f); > return ret; > > This prevents using __free(kfree) on @f because there is no canonical way > to tell the cleanup code that the allocation should not be freed. > > Abusing no_free_ptr() by force ignoring the return value is not really a > sensible option either. > > Provide an explicit macro retain_ptr(), which NULLs the cleanup > pointer. That makes it easy to analyze and reason about. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Peter Zijlstra <peterz@infradead.org> Seems sensible to me and the resulting code is reasonably easy to follow / contained in a small region. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > include/linux/cleanup.h | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > --- a/include/linux/cleanup.h > +++ b/include/linux/cleanup.h > @@ -216,6 +216,23 @@ const volatile void * __must_check_fn(co > > #define return_ptr(p) return no_free_ptr(p) > > +/* > + * Only for situations where an allocation is handed in to another function > + * and consumed by that function on success. > + * > + * struct foo *f __free(kfree) = kzalloc(sizeof(*f), GFP_KERNEL); > + * > + * setup(f); > + * if (some_condition) > + * return -EINVAL; > + * .... > + * ret = bar(f); > + * if (!ret) > + * retain_ptr(f); > + * return ret; > + */ > +#define retain_ptr(p) \ > + __get_and_null(p, NULL) > > /* > * DEFINE_CLASS(name, type, exit, init, init_args...): >
On Thu, 13 Mar 2025 14:03:44 +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 > --- > V2: Remove the gotos - Jonathan Hi Thomas, There is a bit of the original code that is carried forwards here that superficially seemed overly complex. However as far as I can tell this is functionally the same as you intended. So with that in mind if my question isn't complete garbage, maybe a readability issue for another day. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- a/drivers/pci/msi/msi.c > +++ b/drivers/pci/msi/msi.c > +static int msix_setup_interrupts(struct pci_dev *dev, struct msix_entry *entries, > + int nvec, struct irq_affinity *affd) > +{ > + struct irq_affinity_desc *masks __free(kfree) = > + affd ? irq_create_affinity_masks(nvec, affd) : NULL; > + > + guard(msi_descs_lock)(&dev->dev); > + int ret = __msix_setup_interrupts(dev, entries, nvec, masks); > + if (ret) > + pci_free_msi_irqs(dev); It's not immediately obvious what this is undoing (i.e. where the alloc is). I think it is at least mostly the pci_msi_setup_msi_irqs in __msix_setup_interrupts Why not handle the error in __msix_setup_interrupts and make that function side effect free. Does require gotos but in a function that isn't doing any cleanup magic so should be fine. Mind you I'm not following the logic in msix_setup_interrupts() before this series either. i.e. why doesn't msix_setup_msi_descs() clean up after itself on failure (i.e. undo loop iterations that weren't failures) as that at least superficially looks like it would give more readable code. So this is the same as current and as such the patch is fine I think. > return ret; > } >
On Thu, Mar 13, 2025 at 02:03:38PM +0100, Thomas Gleixner wrote: > In cases where an allocation is consumed by another function, the > allocation needs to be retained on success or freed on failure. The code > pattern is usually: > > struct foo *f = kzalloc(sizeof(*f), GFP_KERNEL); > struct bar *b; > > ,,, > // Initialize f > ... > if (ret) > goto free; > ... > bar = bar_create(f); > if (!bar) { > ret = -ENOMEM; > goto free; > } > ... > return 0; > free: > kfree(f); > return ret; > > This prevents using __free(kfree) on @f because there is no canonical way > to tell the cleanup code that the allocation should not be freed. > > Abusing no_free_ptr() by force ignoring the return value is not really a > sensible option either. > > Provide an explicit macro retain_ptr(), which NULLs the cleanup > pointer. That makes it easy to analyze and reason about. So no objection per se, but one way to solve this is by handing ownership to bar_create(), such that it is responsible for freeing f on failure. Anyway, I suspect the __must_check came from Linus, OTOH take_fd(), the equivalent for file descriptors also don't have that __must_check. So clearly we have precedent here.
On Thu, Mar 13, 2025 at 06:55:12PM +0100, Thomas Gleixner wrote: > On Thu, Mar 13 2025 at 15:50, Jonathan Cameron wrote: > >> + guard(msi_descs_lock)(&dev->dev); > >> + int ret = __msix_setup_interrupts(dev, entries, nvec, masks); > >> + if (ret) > >> + pci_free_msi_irqs(dev); > > > > It's not immediately obvious what this is undoing (i.e. where the alloc > > is). I think it is at least mostly the pci_msi_setup_msi_irqs in > > __msix_setup_interrupts > > It's a universal cleanup for all possible error cases. > > > Why not handle the error in __msix_setup_interrupts and make that function > > side effect free. Does require gotos but in a function that isn't > > doing any cleanup magic so should be fine. > > I had the gotos first and then hated them. But you are right, it's better > to have them than having the magic clean up at the call site. > > I'll fold the delta patch below. > > Thanks, > > tglx > --- > --- a/drivers/pci/msi/msi.c > +++ b/drivers/pci/msi/msi.c > @@ -671,19 +671,23 @@ static int __msix_setup_interrupts(struc > int ret = msix_setup_msi_descs(dev, entries, nvec, masks); > > if (ret) > - return ret; > + goto fail; > > ret = pci_msi_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX); > if (ret) > - return ret; > + goto fail; > > /* Check if all MSI entries honor device restrictions */ > ret = msi_verify_entries(dev); > if (ret) > - return ret; > + goto fail; > > msix_update_entries(dev, entries); > return 0; > + > +fail: > + pci_free_msi_irqs(dev); > + return ret; > } How about something like: int __msix_setup_interrupts(struct device *_dev,... ) { struct device *dev __free(msi_irqs) = _dev; int ret = msix_setup_msi_descs(dev, entries, nvec, masks); if (ret) return ret; ret = pci_msi_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX); if (ret) return ret; /* Check if all MSI entries honor device restrictions */ ret = msi_verify_entries(dev); if (ret) return ret; retain_ptr(dev); msix_update_entries(dev, entries); return 0; } ?
On Thu, Mar 13, 2025 at 02:03:38PM +0100, Thomas Gleixner wrote: > In cases where an allocation is consumed by another function, the > allocation needs to be retained on success or freed on failure. The code > pattern is usually: > > struct foo *f = kzalloc(sizeof(*f), GFP_KERNEL); > struct bar *b; > > ,,, > // Initialize f > ... > if (ret) > goto free; > ... > bar = bar_create(f); > if (!bar) { > ret = -ENOMEM; > goto free; > } > ... > return 0; > free: > kfree(f); > return ret; > > This prevents using __free(kfree) on @f because there is no canonical way > to tell the cleanup code that the allocation should not be freed. > > Abusing no_free_ptr() by force ignoring the return value is not really a > sensible option either. > > Provide an explicit macro retain_ptr(), which NULLs the cleanup > pointer. That makes it easy to analyze and reason about. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Peter Zijlstra <peterz@infradead.org> > --- > include/linux/cleanup.h | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > --- a/include/linux/cleanup.h > +++ b/include/linux/cleanup.h > @@ -216,6 +216,23 @@ const volatile void * __must_check_fn(co > > #define return_ptr(p) return no_free_ptr(p) > > +/* > + * Only for situations where an allocation is handed in to another function > + * and consumed by that function on success. > + * > + * struct foo *f __free(kfree) = kzalloc(sizeof(*f), GFP_KERNEL); > + * > + * setup(f); > + * if (some_condition) > + * return -EINVAL; > + * .... > + * ret = bar(f); > + * if (!ret) > + * retain_ptr(f); > + * return ret; Is it better like ret = bar(f); if (ret) return ret; retain_ptr(f); return 0; If there are more than one f, like f1, f2, f3.... ret= bar(f1, f2, ....) if (ret) return ret; retain_ptr(f1); retain_ptr(f2); ... return 0; Or define a macro like #defne no_free_ptr_on_ok(ret, p) ret ? ret : __get_and_null(p, NULL), 0 ret = bar (f); return no_free_ptr_on_ok(ret, f); Frank > + */ > +#define retain_ptr(p) \ > + __get_and_null(p, NULL) > > /* > * DEFINE_CLASS(name, type, exit, init, init_args...): >
This is version 2 of the cleanup work. The previous version can be found here: https://lore.kernel.org/all/20250309083453.900516105@linutronix.de 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 guards. Changes vs. V1: - Introduce retain_ptr() to allow using __free() when the allocation is consumed by a called function (on success) and therefore no_free_ptr() can't be used. - Rework the PCI/MSI changes to avoid gotos in guard sections - Drop patch 1 as it's already applied - Collect Reviewed/Tested/Acked-by tags where appropriate Patches 3,4,6-10 are unmodifed. The series applies on: git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/msi and is 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 | 168 ++++++++++++++++++++++-------------- 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/cleanup.h | 17 +++ include/linux/irqdomain.h | 2 include/linux/msi.h | 7 + kernel/irq/msi.c | 125 ++++++++++---------------- 12 files changed, 247 insertions(+), 252 deletions(-)