[RESEND PATCH v2 0/4] genirq/irq_sim: misc updates

[RESEND PATCH v2 0/4] genirq/irq_sim: misc updates

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

I'm resending it one last time. As it's been almost 5 months since the
initial submission and there were no reviews for the irq_sim part, I'll
take it through the GPIO tree for the next merge window.

--

Here are a couple of updates to the interrupt simulator. Two are minor:
remove an unused field and reorder includes for readability. The third
one simplifies the error paths by using new cleanup macros. To that end
we also add a cleanup definition for dynamic bitmaps.

v1 -> v2:
- add a NULL-pointer check to the bitmap cleanup macro as advised by
  Peter Zijlstra
- initialize managed pointers when declaring them to create a clear pairing
  between the type and the cleanup action

Bartosz Golaszewski (4):
  bitmap: define a cleanup function for bitmaps
  genirq/irq_sim: remove unused field from struct irq_sim_irq_ctx
  genirq/irq_sim: order headers alphabetically
  genirq/irq_sim: shrink code by using cleanup helpers

 include/linux/bitmap.h |  3 +++
 kernel/irq/irq_sim.c   | 30 ++++++++++++------------------
 2 files changed, 15 insertions(+), 18 deletions(-)

-- 
2.40.1

[RESEND PATCH v2 1/4] bitmap: define a cleanup function for bitmaps

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add support for autopointers for bitmaps allocated with bitmap_alloc()
et al.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Yury Norov <yury.norov@gmail.com>
---
 include/linux/bitmap.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 99451431e4d6..df24c8fb1009 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -6,6 +6,7 @@
 
 #include <linux/align.h>
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/errno.h>
 #include <linux/find.h>
 #include <linux/limits.h>
@@ -127,6 +128,8 @@ unsigned long *bitmap_alloc_node(unsigned int nbits, gfp_t flags, int node);
 unsigned long *bitmap_zalloc_node(unsigned int nbits, gfp_t flags, int node);
 void bitmap_free(const unsigned long *bitmap);
 
+DEFINE_FREE(bitmap, unsigned long *, if (_T) bitmap_free(_T))
+
 /* Managed variants of the above. */
 unsigned long *devm_bitmap_alloc(struct device *dev,
 				 unsigned int nbits, gfp_t flags);
-- 
2.40.1

[RESEND PATCH v2 2/4] genirq/irq_sim: remove unused field from struct irq_sim_irq_ctx

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The irqnum field is unused. Remove it.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 kernel/irq/irq_sim.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index dd76323ea3fd..f5ebb3ba6f9a 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -19,7 +19,6 @@ struct irq_sim_work_ctx {
 };
 
 struct irq_sim_irq_ctx {
-	int			irqnum;
 	bool			enabled;
 	struct irq_sim_work_ctx	*work_ctx;
 };
-- 
2.40.1

[RESEND PATCH v2 3/4] genirq/irq_sim: order headers alphabetically

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

For better readability and maintenance keep headers in alphabetical
order.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 kernel/irq/irq_sim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index f5ebb3ba6f9a..b0d50b48dbd1 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -4,10 +4,10 @@
  * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
  */
 
+#include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
 #include <linux/irq_work.h>
-#include <linux/interrupt.h>
 #include <linux/slab.h>
 
 struct irq_sim_work_ctx {
-- 
2.40.1

[RESEND PATCH v2 4/4] genirq/irq_sim: shrink code by using cleanup helpers

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Use the new __free helper from linux/cleanup.h to remove all gotos and
simplify the error paths.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 kernel/irq/irq_sim.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index b0d50b48dbd1..82b7b5051249 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
@@ -163,33 +164,27 @@ static const struct irq_domain_ops irq_sim_domain_ops = {
 struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
 					 unsigned int num_irqs)
 {
-	struct irq_sim_work_ctx *work_ctx;
-
-	work_ctx = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
+	struct irq_sim_work_ctx *work_ctx __free(kfree) =
+				kmalloc(sizeof(*work_ctx), GFP_KERNEL);
 	if (!work_ctx)
-		goto err_out;
+		return ERR_PTR(-ENOMEM);
 
-	work_ctx->pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
-	if (!work_ctx->pending)
-		goto err_free_work_ctx;
+	unsigned long *pending __free(bitmap) = bitmap_zalloc(num_irqs,
+							      GFP_KERNEL);
+	if (!pending)
+		return ERR_PTR(-ENOMEM);
 
 	work_ctx->domain = irq_domain_create_linear(fwnode, num_irqs,
 						    &irq_sim_domain_ops,
 						    work_ctx);
 	if (!work_ctx->domain)
-		goto err_free_bitmap;
+		return ERR_PTR(-ENOMEM);
 
 	work_ctx->irq_count = num_irqs;
 	work_ctx->work = IRQ_WORK_INIT_HARD(irq_sim_handle_irq);
+	work_ctx->pending = no_free_ptr(pending);
 
-	return work_ctx->domain;
-
-err_free_bitmap:
-	bitmap_free(work_ctx->pending);
-err_free_work_ctx:
-	kfree(work_ctx);
-err_out:
-	return ERR_PTR(-ENOMEM);
+	return no_free_ptr(work_ctx)->domain;
 }
 EXPORT_SYMBOL_GPL(irq_domain_create_sim);
 
-- 
2.40.1

[tip: irq/core] genirq/irq_sim: Shrink code by using cleanup helpers

[tip-bot2 for Bartosz Golaszewski]

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     590610d72a790458431cbbebc71ee24521533b5e
Gitweb:        https://git.kernel.org/tip/590610d72a790458431cbbebc71ee24521533b5e
Author:        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
AuthorDate:    Mon, 22 Jan 2024 13:42:43 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Fri, 26 Jan 2024 13:44:48 +01:00

genirq/irq_sim: Shrink code by using cleanup helpers

Use the new __free() mechanism to remove all gotos and simplify the error
paths.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20240122124243.44002-5-brgl@bgdev.pl

---
 kernel/irq/irq_sim.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index b0d50b4..fe8fd30 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
@@ -163,33 +164,27 @@ static const struct irq_domain_ops irq_sim_domain_ops = {
 struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
 					 unsigned int num_irqs)
 {
-	struct irq_sim_work_ctx *work_ctx;
+	struct irq_sim_work_ctx *work_ctx __free(kfree) = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
+	unsigned long *pending;
 
-	work_ctx = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
 	if (!work_ctx)
-		goto err_out;
+		return ERR_PTR(-ENOMEM);
 
-	work_ctx->pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
-	if (!work_ctx->pending)
-		goto err_free_work_ctx;
+	pending = __free(bitmap) = bitmap_zalloc(num_irqs, GFP_KERNEL);
+	if (!pending)
+		return ERR_PTR(-ENOMEM);
 
 	work_ctx->domain = irq_domain_create_linear(fwnode, num_irqs,
 						    &irq_sim_domain_ops,
 						    work_ctx);
 	if (!work_ctx->domain)
-		goto err_free_bitmap;
+		return ERR_PTR(-ENOMEM);
 
 	work_ctx->irq_count = num_irqs;
 	work_ctx->work = IRQ_WORK_INIT_HARD(irq_sim_handle_irq);
+	work_ctx->pending = no_free_ptr(pending);
 
-	return work_ctx->domain;
-
-err_free_bitmap:
-	bitmap_free(work_ctx->pending);
-err_free_work_ctx:
-	kfree(work_ctx);
-err_out:
-	return ERR_PTR(-ENOMEM);
+	return no_free_ptr(work_ctx)->domain;
 }
 EXPORT_SYMBOL_GPL(irq_domain_create_sim);
 

[tip: irq/core] genirq/irq_sim: Remove unused field from struct irq_sim_irq_ctx

[tip-bot2 for Bartosz Golaszewski]

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     3832f390423302e373a1818f6cf8cb29ebf3a195
Gitweb:        https://git.kernel.org/tip/3832f390423302e373a1818f6cf8cb29ebf3a195
Author:        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
AuthorDate:    Mon, 22 Jan 2024 13:42:41 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Fri, 26 Jan 2024 13:44:48 +01:00

genirq/irq_sim: Remove unused field from struct irq_sim_irq_ctx

The irqnum field is unused. Remove it.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20240122124243.44002-3-brgl@bgdev.pl

---
 kernel/irq/irq_sim.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index dd76323..f5ebb3b 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -19,7 +19,6 @@ struct irq_sim_work_ctx {
 };
 
 struct irq_sim_irq_ctx {
-	int			irqnum;
 	bool			enabled;
 	struct irq_sim_work_ctx	*work_ctx;
 };

[tip: irq/core] genirq/irq_sim: Order headers alphabetically

[tip-bot2 for Bartosz Golaszewski]

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     8dab7fd47e53865d37fce73c67bac97b41d5d64a
Gitweb:        https://git.kernel.org/tip/8dab7fd47e53865d37fce73c67bac97b41d5d64a
Author:        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
AuthorDate:    Mon, 22 Jan 2024 13:42:42 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Fri, 26 Jan 2024 13:44:48 +01:00

genirq/irq_sim: Order headers alphabetically

For better readability and maintenance keep headers in alphabetical
order.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20240122124243.44002-4-brgl@bgdev.pl

---
 kernel/irq/irq_sim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index f5ebb3b..b0d50b4 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -4,10 +4,10 @@
  * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
  */
 
+#include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
 #include <linux/irq_work.h>
-#include <linux/interrupt.h>
 #include <linux/slab.h>
 
 struct irq_sim_work_ctx {

[tip: irq/core] bitmap: Define a cleanup function for bitmaps

[tip-bot2 for Bartosz Golaszewski]

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     d12a82848eac28d248e67940378fe4a72b0a8cd3
Gitweb:        https://git.kernel.org/tip/d12a82848eac28d248e67940378fe4a72b0a8cd3
Author:        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
AuthorDate:    Mon, 22 Jan 2024 13:42:40 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Fri, 26 Jan 2024 13:44:48 +01:00

bitmap: Define a cleanup function for bitmaps

Add support for autopointers for bitmaps allocated with bitmap_alloc()
et al.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Yury Norov <yury.norov@gmail.com>
Link: https://lore.kernel.org/r/20240122124243.44002-2-brgl@bgdev.pl

---
 include/linux/bitmap.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 9945143..df24c8f 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -6,6 +6,7 @@
 
 #include <linux/align.h>
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/errno.h>
 #include <linux/find.h>
 #include <linux/limits.h>
@@ -127,6 +128,8 @@ unsigned long *bitmap_alloc_node(unsigned int nbits, gfp_t flags, int node);
 unsigned long *bitmap_zalloc_node(unsigned int nbits, gfp_t flags, int node);
 void bitmap_free(const unsigned long *bitmap);
 
+DEFINE_FREE(bitmap, unsigned long *, if (_T) bitmap_free(_T))
+
 /* Managed variants of the above. */
 unsigned long *devm_bitmap_alloc(struct device *dev,
 				 unsigned int nbits, gfp_t flags);

Re: [tip: irq/core] genirq/irq_sim: Shrink code by using cleanup helpers

[Nathan Chancellor]

On Fri, Jan 26, 2024 at 12:53:36PM -0000, tip-bot2 for Bartosz Golaszewski wrote:
> The following commit has been merged into the irq/core branch of tip:
> 
> Commit-ID:     590610d72a790458431cbbebc71ee24521533b5e
> Gitweb:        https://git.kernel.org/tip/590610d72a790458431cbbebc71ee24521533b5e
> Author:        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> AuthorDate:    Mon, 22 Jan 2024 13:42:43 +01:00
> Committer:     Thomas Gleixner <tglx@linutronix.de>
> CommitterDate: Fri, 26 Jan 2024 13:44:48 +01:00
> 
> genirq/irq_sim: Shrink code by using cleanup helpers
> 
> Use the new __free() mechanism to remove all gotos and simplify the error
> paths.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Link: https://lore.kernel.org/r/20240122124243.44002-5-brgl@bgdev.pl
> 
> ---
>  kernel/irq/irq_sim.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> index b0d50b4..fe8fd30 100644
> --- a/kernel/irq/irq_sim.c
> +++ b/kernel/irq/irq_sim.c
> @@ -4,6 +4,7 @@
>   * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/irq_sim.h>
> @@ -163,33 +164,27 @@ static const struct irq_domain_ops irq_sim_domain_ops = {
>  struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
>  					 unsigned int num_irqs)
>  {
> -	struct irq_sim_work_ctx *work_ctx;
> +	struct irq_sim_work_ctx *work_ctx __free(kfree) = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
> +	unsigned long *pending;
>  
> -	work_ctx = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
>  	if (!work_ctx)
> -		goto err_out;
> +		return ERR_PTR(-ENOMEM);
>  
> -	work_ctx->pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
> -	if (!work_ctx->pending)
> -		goto err_free_work_ctx;
> +	pending = __free(bitmap) = bitmap_zalloc(num_irqs, GFP_KERNEL);

Apologies if this has already been reported elsewhere. This does not
match what was sent and it causes the build to break with both GCC:

  In file included from include/linux/compiler_types.h:89,
                   from <command-line>:
  kernel/irq/irq_sim.c: In function 'irq_domain_create_sim':
  include/linux/compiler_attributes.h:76:41: error: expected expression before '__attribute__'
     76 | #define __cleanup(func)                 __attribute__((__cleanup__(func)))
        |                                         ^~~~~~~~~~~~~
  include/linux/cleanup.h:64:25: note: in expansion of macro '__cleanup'
     64 | #define __free(_name)   __cleanup(__free_##_name)
        |                         ^~~~~~~~~
  kernel/irq/irq_sim.c:173:19: note: in expansion of macro '__free'
    173 |         pending = __free(bitmap) = bitmap_zalloc(num_irqs, GFP_KERNEL);
        |                   ^~~~~~

and Clang:

  kernel/irq/irq_sim.c:173:12: error: expected expression
    173 |         pending = __free(bitmap) = bitmap_zalloc(num_irqs, GFP_KERNEL);
        |                   ^
  include/linux/cleanup.h:64:23: note: expanded from macro '__free'
     64 | #define __free(_name)   __cleanup(__free_##_name)
        |                         ^
  include/linux/compiler-clang.h:15:25: note: expanded from macro '__cleanup'
     15 | #define __cleanup(func) __maybe_unused __attribute__((__cleanup__(func)))
        |                         ^
  include/linux/compiler_attributes.h:344:41: note: expanded from macro '__maybe_unused'
    344 | #define __maybe_unused                  __attribute__((__unused__))
        |                                         ^
  1 error generated.

This was initially noticed by our CI:

https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/7671789235/job/20915505965
https://storage.tuxsuite.com/public/clangbuiltlinux/continuous-integration2/builds/2bVGKZUmat8fRr582Nh8hNA6FXD/build.log

Cheers,
Nathan

> +	if (!pending)
> +		return ERR_PTR(-ENOMEM);
>  
>  	work_ctx->domain = irq_domain_create_linear(fwnode, num_irqs,
>  						    &irq_sim_domain_ops,
>  						    work_ctx);
>  	if (!work_ctx->domain)
> -		goto err_free_bitmap;
> +		return ERR_PTR(-ENOMEM);
>  
>  	work_ctx->irq_count = num_irqs;
>  	work_ctx->work = IRQ_WORK_INIT_HARD(irq_sim_handle_irq);
> +	work_ctx->pending = no_free_ptr(pending);
>  
> -	return work_ctx->domain;
> -
> -err_free_bitmap:
> -	bitmap_free(work_ctx->pending);
> -err_free_work_ctx:
> -	kfree(work_ctx);
> -err_out:
> -	return ERR_PTR(-ENOMEM);
> +	return no_free_ptr(work_ctx)->domain;
>  }
>  EXPORT_SYMBOL_GPL(irq_domain_create_sim);
>  

Re: [tip: irq/core] genirq/irq_sim: Shrink code by using cleanup helpers

[Bartosz Golaszewski]

On Fri, 26 Jan 2024 at 22:05, Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Fri, Jan 26, 2024 at 12:53:36PM -0000, tip-bot2 for Bartosz Golaszewski wrote:
> > The following commit has been merged into the irq/core branch of tip:
> >
> > Commit-ID:     590610d72a790458431cbbebc71ee24521533b5e
> > Gitweb:        https://git.kernel.org/tip/590610d72a790458431cbbebc71ee24521533b5e
> > Author:        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > AuthorDate:    Mon, 22 Jan 2024 13:42:43 +01:00
> > Committer:     Thomas Gleixner <tglx@linutronix.de>
> > CommitterDate: Fri, 26 Jan 2024 13:44:48 +01:00
> >
> > genirq/irq_sim: Shrink code by using cleanup helpers
> >
> > Use the new __free() mechanism to remove all gotos and simplify the error
> > paths.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Link: https://lore.kernel.org/r/20240122124243.44002-5-brgl@bgdev.pl
> >
> > ---
> >  kernel/irq/irq_sim.c | 25 ++++++++++---------------
> >  1 file changed, 10 insertions(+), 15 deletions(-)
> >
> > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> > index b0d50b4..fe8fd30 100644
> > --- a/kernel/irq/irq_sim.c
> > +++ b/kernel/irq/irq_sim.c
> > @@ -4,6 +4,7 @@
> >   * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >   */
> >
> > +#include <linux/cleanup.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/irq.h>
> >  #include <linux/irq_sim.h>
> > @@ -163,33 +164,27 @@ static const struct irq_domain_ops irq_sim_domain_ops = {
> >  struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
> >                                        unsigned int num_irqs)
> >  {
> > -     struct irq_sim_work_ctx *work_ctx;
> > +     struct irq_sim_work_ctx *work_ctx __free(kfree) = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
> > +     unsigned long *pending;
> >
> > -     work_ctx = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
> >       if (!work_ctx)
> > -             goto err_out;
> > +             return ERR_PTR(-ENOMEM);
> >
> > -     work_ctx->pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
> > -     if (!work_ctx->pending)
> > -             goto err_free_work_ctx;
> > +     pending = __free(bitmap) = bitmap_zalloc(num_irqs, GFP_KERNEL);
>
> Apologies if this has already been reported elsewhere. This does not
> match what was sent and it causes the build to break with both GCC:
>

I did not see any other report. I don't know what happened here but
this was a ninja edit as it's not what I sent. If Thomas' intention
was to move the variable declaration and detach it from the assignment
then 'pending' should at least be set to NULL and __free() must
decorate the declaration.

But the coding style of declaring variables when they're first
assigned their auto-cleaned value is what Linus Torvalds explicitly
asked me to do when I first started sending PRs containing uses of
linux/cleanup.h.

Bartosz

>   In file included from include/linux/compiler_types.h:89,
>                    from <command-line>:
>   kernel/irq/irq_sim.c: In function 'irq_domain_create_sim':
>   include/linux/compiler_attributes.h:76:41: error: expected expression before '__attribute__'
>      76 | #define __cleanup(func)                 __attribute__((__cleanup__(func)))
>         |                                         ^~~~~~~~~~~~~
>   include/linux/cleanup.h:64:25: note: in expansion of macro '__cleanup'
>      64 | #define __free(_name)   __cleanup(__free_##_name)
>         |                         ^~~~~~~~~
>   kernel/irq/irq_sim.c:173:19: note: in expansion of macro '__free'
>     173 |         pending = __free(bitmap) = bitmap_zalloc(num_irqs, GFP_KERNEL);
>         |                   ^~~~~~
>
> and Clang:
>
>   kernel/irq/irq_sim.c:173:12: error: expected expression
>     173 |         pending = __free(bitmap) = bitmap_zalloc(num_irqs, GFP_KERNEL);
>         |                   ^
>   include/linux/cleanup.h:64:23: note: expanded from macro '__free'
>      64 | #define __free(_name)   __cleanup(__free_##_name)
>         |                         ^
>   include/linux/compiler-clang.h:15:25: note: expanded from macro '__cleanup'
>      15 | #define __cleanup(func) __maybe_unused __attribute__((__cleanup__(func)))
>         |                         ^
>   include/linux/compiler_attributes.h:344:41: note: expanded from macro '__maybe_unused'
>     344 | #define __maybe_unused                  __attribute__((__unused__))
>         |                                         ^
>   1 error generated.
>
> This was initially noticed by our CI:
>
> https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/7671789235/job/20915505965
> https://storage.tuxsuite.com/public/clangbuiltlinux/continuous-integration2/builds/2bVGKZUmat8fRr582Nh8hNA6FXD/build.log
>
> Cheers,
> Nathan
>
> > +     if (!pending)
> > +             return ERR_PTR(-ENOMEM);
> >
> >       work_ctx->domain = irq_domain_create_linear(fwnode, num_irqs,
> >                                                   &irq_sim_domain_ops,
> >                                                   work_ctx);
> >       if (!work_ctx->domain)
> > -             goto err_free_bitmap;
> > +             return ERR_PTR(-ENOMEM);
> >
> >       work_ctx->irq_count = num_irqs;
> >       work_ctx->work = IRQ_WORK_INIT_HARD(irq_sim_handle_irq);
> > +     work_ctx->pending = no_free_ptr(pending);
> >
> > -     return work_ctx->domain;
> > -
> > -err_free_bitmap:
> > -     bitmap_free(work_ctx->pending);
> > -err_free_work_ctx:
> > -     kfree(work_ctx);
> > -err_out:
> > -     return ERR_PTR(-ENOMEM);
> > +     return no_free_ptr(work_ctx)->domain;
> >  }
> >  EXPORT_SYMBOL_GPL(irq_domain_create_sim);
> >

Re: [tip: irq/core] genirq/irq_sim: Shrink code by using cleanup helpers

[Ingo Molnar]

* Bartosz Golaszewski <bartosz.golaszewski@linaro.org> wrote:

> On Fri, 26 Jan 2024 at 22:05, Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > > Committer:     Thomas Gleixner <tglx@linutronix.de>
> > > CommitterDate: Fri, 26 Jan 2024 13:44:48 +01:00
> > >
> > > genirq/irq_sim: Shrink code by using cleanup helpers
> > >
> > > Use the new __free() mechanism to remove all gotos and simplify the error
> > > paths.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > > Link: https://lore.kernel.org/r/20240122124243.44002-5-brgl@bgdev.pl
> > >
> > > ---
> > >  kernel/irq/irq_sim.c | 25 ++++++++++---------------
> > >  1 file changed, 10 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> > > index b0d50b4..fe8fd30 100644
> > > --- a/kernel/irq/irq_sim.c
> > > +++ b/kernel/irq/irq_sim.c
> > > @@ -4,6 +4,7 @@
> > >   * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >   */
> > >
> > > +#include <linux/cleanup.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/irq.h>
> > >  #include <linux/irq_sim.h>
> > > @@ -163,33 +164,27 @@ static const struct irq_domain_ops irq_sim_domain_ops = {
> > >  struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
> > >                                        unsigned int num_irqs)
> > >  {
> > > -     struct irq_sim_work_ctx *work_ctx;
> > > +     struct irq_sim_work_ctx *work_ctx __free(kfree) = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
> > > +     unsigned long *pending;
> > >
> > > -     work_ctx = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
> > >       if (!work_ctx)
> > > -             goto err_out;
> > > +             return ERR_PTR(-ENOMEM);
> > >
> > > -     work_ctx->pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
> > > -     if (!work_ctx->pending)
> > > -             goto err_free_work_ctx;
> > > +     pending = __free(bitmap) = bitmap_zalloc(num_irqs, GFP_KERNEL);
> >
> > Apologies if this has already been reported elsewhere. This does not
> > match what was sent and it causes the build to break with both GCC:
> >
> 
> I did not see any other report. I don't know what happened here but
> this was a ninja edit as it's not what I sent. If Thomas' intention
> was to move the variable declaration and detach it from the assignment
> then 'pending' should at least be set to NULL and __free() must
> decorate the declaration.
> 
> But the coding style of declaring variables when they're first
> assigned their auto-cleaned value is what Linus Torvalds explicitly
> asked me to do when I first started sending PRs containing uses of
> linux/cleanup.h.

Ok - I've rebased tip:irq/core with the original patch.

Do you have a reference to Linus's mail about C++ style definition
of variables? I can see the validity of the pattern in this context,
but it's explicitly against the kernel coding style AFAICS, which
I suppose prompted Thomas's edit. I'd like to have an URL handy when the
inevitable checkpatch 'fix' gets submitted. ;-)

Thanks,

	Ingo

[tip: irq/core] genirq/irq_sim: Shrink code by using <linux/cleanup.h> helpers

[tip-bot2 for Bartosz Golaszewski]

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     aafd753555c0ecb9c7ce11ff14429a34c8c0a14b
Gitweb:        https://git.kernel.org/tip/aafd753555c0ecb9c7ce11ff14429a34c8c0a14b
Author:        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
AuthorDate:    Mon, 22 Jan 2024 13:42:43 +01:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 29 Jan 2024 11:07:57 +01:00

genirq/irq_sim: Shrink code by using <linux/cleanup.h> helpers

Use the new __free() mechanism to remove all gotos and simplify the error
paths.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Nathan Chancellor <nathan@kernel.org>
Link: https://lore.kernel.org/r/20240122124243.44002-5-brgl@bgdev.pl
---
 kernel/irq/irq_sim.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index b0d50b4..38d6ae6 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
@@ -163,33 +164,27 @@ static const struct irq_domain_ops irq_sim_domain_ops = {
 struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
 					 unsigned int num_irqs)
 {
-	struct irq_sim_work_ctx *work_ctx;
+	struct irq_sim_work_ctx *work_ctx __free(kfree) =
+				kmalloc(sizeof(*work_ctx), GFP_KERNEL);
 
-	work_ctx = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
 	if (!work_ctx)
-		goto err_out;
+		return ERR_PTR(-ENOMEM);
 
-	work_ctx->pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
-	if (!work_ctx->pending)
-		goto err_free_work_ctx;
+	unsigned long *pending __free(bitmap) = bitmap_zalloc(num_irqs, GFP_KERNEL);
+	if (!pending)
+		return ERR_PTR(-ENOMEM);
 
 	work_ctx->domain = irq_domain_create_linear(fwnode, num_irqs,
 						    &irq_sim_domain_ops,
 						    work_ctx);
 	if (!work_ctx->domain)
-		goto err_free_bitmap;
+		return ERR_PTR(-ENOMEM);
 
 	work_ctx->irq_count = num_irqs;
 	work_ctx->work = IRQ_WORK_INIT_HARD(irq_sim_handle_irq);
+	work_ctx->pending = no_free_ptr(pending);
 
-	return work_ctx->domain;
-
-err_free_bitmap:
-	bitmap_free(work_ctx->pending);
-err_free_work_ctx:
-	kfree(work_ctx);
-err_out:
-	return ERR_PTR(-ENOMEM);
+	return no_free_ptr(work_ctx)->domain;
 }
 EXPORT_SYMBOL_GPL(irq_domain_create_sim);
 

Re: [tip: irq/core] genirq/irq_sim: Shrink code by using cleanup helpers

[Bartosz Golaszewski]

On Mon, 29 Jan 2024 at 11:13, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Bartosz Golaszewski <bartosz.golaszewski@linaro.org> wrote:
>
> > On Fri, 26 Jan 2024 at 22:05, Nathan Chancellor <nathan@kernel.org> wrote:
> > >
> > > > Committer:     Thomas Gleixner <tglx@linutronix.de>
> > > > CommitterDate: Fri, 26 Jan 2024 13:44:48 +01:00
> > > >
> > > > genirq/irq_sim: Shrink code by using cleanup helpers
> > > >
> > > > Use the new __free() mechanism to remove all gotos and simplify the error
> > > > paths.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > > > Link: https://lore.kernel.org/r/20240122124243.44002-5-brgl@bgdev.pl
> > > >
> > > > ---
> > > >  kernel/irq/irq_sim.c | 25 ++++++++++---------------
> > > >  1 file changed, 10 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> > > > index b0d50b4..fe8fd30 100644
> > > > --- a/kernel/irq/irq_sim.c
> > > > +++ b/kernel/irq/irq_sim.c
> > > > @@ -4,6 +4,7 @@
> > > >   * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >   */
> > > >
> > > > +#include <linux/cleanup.h>
> > > >  #include <linux/interrupt.h>
> > > >  #include <linux/irq.h>
> > > >  #include <linux/irq_sim.h>
> > > > @@ -163,33 +164,27 @@ static const struct irq_domain_ops irq_sim_domain_ops = {
> > > >  struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
> > > >                                        unsigned int num_irqs)
> > > >  {
> > > > -     struct irq_sim_work_ctx *work_ctx;
> > > > +     struct irq_sim_work_ctx *work_ctx __free(kfree) = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
> > > > +     unsigned long *pending;
> > > >
> > > > -     work_ctx = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
> > > >       if (!work_ctx)
> > > > -             goto err_out;
> > > > +             return ERR_PTR(-ENOMEM);
> > > >
> > > > -     work_ctx->pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
> > > > -     if (!work_ctx->pending)
> > > > -             goto err_free_work_ctx;
> > > > +     pending = __free(bitmap) = bitmap_zalloc(num_irqs, GFP_KERNEL);
> > >
> > > Apologies if this has already been reported elsewhere. This does not
> > > match what was sent and it causes the build to break with both GCC:
> > >
> >
> > I did not see any other report. I don't know what happened here but
> > this was a ninja edit as it's not what I sent. If Thomas' intention
> > was to move the variable declaration and detach it from the assignment
> > then 'pending' should at least be set to NULL and __free() must
> > decorate the declaration.
> >
> > But the coding style of declaring variables when they're first
> > assigned their auto-cleaned value is what Linus Torvalds explicitly
> > asked me to do when I first started sending PRs containing uses of
> > linux/cleanup.h.
>
> Ok - I've rebased tip:irq/core with the original patch.
>
> Do you have a reference to Linus's mail about C++ style definition
> of variables? I can see the validity of the pattern in this context,
> but it's explicitly against the kernel coding style AFAICS, which
> I suppose prompted Thomas's edit. I'd like to have an URL handy when the
> inevitable checkpatch 'fix' gets submitted. ;-)
>

Sure, here's one rant I was the target of:
https://lore.kernel.org/all/CAHk-=wgRHiV5VSxtfXA4S6aLUmcQYEuB67u3BJPJPtuESs1JyA@mail.gmail.com/

Bartosz