[PATCH v2 0/5] Fix static analyser and compiler warnings in int3472

[PATCH v2 0/5] Fix static analyser and compiler warnings in int3472

[Sakari Ailus]

Hi folks,

The two first patches address smatch and compiler warnings in int3472. The
third patch adds __free() support for ACPI objects and the final two
further clean up GPIO parsing in the int3472 driver.

Sakari Ailus (5):
  platform/x86: int3472: Address Coccinelle warning on an error print
  platform/x86: int3472: Fix uninitialised variable warning
  ACPI: Support __free() from cleanup.h for ACPI objects
  platform/x86: int3472: Release ACPI objects using __free()
  platform/x86: int3472: Clean up GPIO parsing

 drivers/platform/x86/intel/int3472/discrete.c | 55 +++++++++----------
 drivers/platform/x86/intel/int3472/tps68470.c |  2 +-
 include/linux/acpi.h                          |  2 +
 3 files changed, 30 insertions(+), 29 deletions(-)

-- 
2.47.3

[PATCH v2 1/5] platform/x86: int3472: Address Coccinelle warning on an error print

[Sakari Ailus]

Fix the following Coccinelle warning:

./tps68470.c:164:58-65: WARNING: Consider using %pe to print PTR_ERR()

by using %pe specifier for printing an error code.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/platform/x86/intel/int3472/tps68470.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index a77ed32abe55..dc777dbac61f 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -161,7 +161,7 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 
 	regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
 	if (IS_ERR(regmap)) {
-		dev_err(&client->dev, "Failed to create regmap: %ld\n", PTR_ERR(regmap));
+		dev_err(&client->dev, "Failed to create regmap: %pe\n", regmap);
 		return PTR_ERR(regmap);
 	}
 
-- 
2.47.3

[PATCH v2 2/5] platform/x86: int3472: Fix uninitialised variable warning

[Sakari Ailus]

Fix a smatch warning about uninitialised err_msg variable, by printing the
error where it is handled.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 21 +++++++++----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 6c729fcfce5d..b4a95b583edb 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -329,7 +329,6 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	unsigned long gpio_flags;
 	union acpi_object *obj;
 	struct gpio_desc *gpio;
-	const char *err_msg;
 	const char *con_id;
 	int ret;
 
@@ -375,7 +374,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	case INT3472_GPIO_TYPE_HOTPLUG_DETECT:
 		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, con_id, gpio_flags);
 		if (ret)
-			err_msg = "Failed to map GPIO pin to sensor\n";
+			dev_err_probe(int3472->dev, ret, "Failed to map GPIO pin to sensor\n");
 
 		break;
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
@@ -387,7 +386,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 		gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, con_id, gpio_flags);
 		if (IS_ERR(gpio)) {
 			ret = PTR_ERR(gpio);
-			err_msg = "Failed to get GPIO\n";
+			dev_err_probe(int3472->dev, ret, "Failed to get GPIO\n");
 			break;
 		}
 
@@ -395,14 +394,14 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 		case INT3472_GPIO_TYPE_CLK_ENABLE:
 			ret = skl_int3472_register_gpio_clock(int3472, gpio);
 			if (ret)
-				err_msg = "Failed to register clock\n";
+				dev_err_probe(int3472->dev, ret, "Failed to register clock\n");
 
 			break;
 		case INT3472_GPIO_TYPE_PRIVACY_LED:
 		case INT3472_GPIO_TYPE_STROBE:
 			ret = skl_int3472_register_led(int3472, gpio, con_id);
 			if (ret)
-				err_msg = "Failed to register LED\n";
+				dev_err_probe(int3472->dev, ret, "Failed to register LED\n");
 
 			break;
 		case INT3472_GPIO_TYPE_POWER_ENABLE:
@@ -413,7 +412,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 			ret = skl_int3472_register_regulator(int3472, gpio, enable_time_us,
 							     con_id, second_sensor);
 			if (ret)
-				err_msg = "Failed to register regulator\n";
+				dev_err_probe(int3472->dev, ret, "Failed to register regulator\n");
 
 			break;
 		default: /* Never reached */
@@ -436,11 +435,11 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	int3472->ngpios++;
 	ACPI_FREE(obj);
 
-	if (ret < 0)
-		return dev_err_probe(int3472->dev, ret, err_msg);
-
-	/* Tell acpi_dev_get_resources() to not make a copy of the resource */
-	return 1;
+	/*
+	 * Either return an error or tell acpi_dev_get_resources() to not make a
+	 * copy of the resource.
+	 */
+	return ret < 0 ? ret : 1;
 }
 
 int int3472_discrete_parse_crs(struct int3472_discrete_device *int3472)
-- 
2.47.3

[PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Sakari Ailus]

Use DEFINE_FREE() to allow ACPI objects to be released automatically.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/linux/acpi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 10d6c6c11bdf..c89390ac6287 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -62,6 +62,8 @@ static inline acpi_handle acpi_device_handle(struct acpi_device *adev)
 #define ACPI_HANDLE_FWNODE(fwnode)	\
 				acpi_device_handle(to_acpi_device_node(fwnode))
 
+DEFINE_FREE(ACPI_FREE, void *, ACPI_FREE(_T))
+
 static inline struct fwnode_handle *acpi_alloc_fwnode_static(void)
 {
 	struct fwnode_handle *fwnode;
-- 
2.47.3

Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Rafael J. Wysocki (Intel)]

On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Use DEFINE_FREE() to allow ACPI objects to be released automatically.

But at least some of them are allocated by ACPICA functions like
acpi_evaluate_object() and so they have no proper constructors.

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/linux/acpi.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 10d6c6c11bdf..c89390ac6287 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -62,6 +62,8 @@ static inline acpi_handle acpi_device_handle(struct acpi_device *adev)
>  #define ACPI_HANDLE_FWNODE(fwnode)     \
>                                 acpi_device_handle(to_acpi_device_node(fwnode))
>
> +DEFINE_FREE(ACPI_FREE, void *, ACPI_FREE(_T))
> +
>  static inline struct fwnode_handle *acpi_alloc_fwnode_static(void)
>  {
>         struct fwnode_handle *fwnode;
> --

Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Sakari Ailus]

Hi Rafael,

On Tue, Aug 25, 2026 at 01:51:18PM +0200, Rafael J. Wysocki (Intel) wrote:
> On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Use DEFINE_FREE() to allow ACPI objects to be released automatically.
> 
> But at least some of them are allocated by ACPICA functions like
> acpi_evaluate_object() and so they have no proper constructors.

You could still assign the return buffer to a local variable. It's not
ideal API-wise though.

I'm not quite sure what was the point you wanted to make but I reckon this
wasn't an ack. :-)

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Rafael J. Wysocki (Intel)]

On Tue, Aug 25, 2026 at 2:07 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Tue, Aug 25, 2026 at 01:51:18PM +0200, Rafael J. Wysocki (Intel) wrote:
> > On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Use DEFINE_FREE() to allow ACPI objects to be released automatically.
> >
> > But at least some of them are allocated by ACPICA functions like
> > acpi_evaluate_object() and so they have no proper constructors.
>
> You could still assign the return buffer to a local variable. It's not
> ideal API-wise though.

Exactly.

> I'm not quite sure what was the point you wanted to make but I reckon this
> wasn't an ack. :-)

Using the _FREE with variables that are not initialized through a
constructor is questionable, so this is generally not particularly
clean.

There is no cleanup.h in ACPICA that is a more traditional C code
base, so mixing up ACPICA code, which ACPI_FREE() is strictly
speaking, with cleanup.h stuff is not particularly straightforward
IMV.  I'd rather not do it.

Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]

On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:

> On Tue, Aug 25, 2026 at 2:07 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > On Tue, Aug 25, 2026 at 01:51:18PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Use DEFINE_FREE() to allow ACPI objects to be released automatically.
> > >
> > > But at least some of them are allocated by ACPICA functions like
> > > acpi_evaluate_object() and so they have no proper constructors.
> >
> > You could still assign the return buffer to a local variable. It's not
> > ideal API-wise though.
> 
> Exactly.
> 
> > I'm not quite sure what was the point you wanted to make but I reckon this
> > wasn't an ack. :-)
> 
> Using the _FREE with variables that are not initialized through a
> constructor is questionable, so this is generally not particularly
> clean.

The driver does call ACPI_FREE() for that pointer so clearly it's already 
using something ending with "_FREE" already. So unless Rafael is 
suggesting ACPI_FREE() should be renamed, I'm a bit lost what that 
even means on concrete terms.

> There is no cleanup.h in ACPICA that is a more traditional C code
> base, so mixing up ACPICA code, which ACPI_FREE() is strictly
> speaking, with cleanup.h stuff is not particularly straightforward
> IMV.  I'd rather not do it.

Perhaps add the DEFINE_FREE() into int3472 driver then, it seems to be 
what intel/pmc is already doing (I don't seem to anymore recall why it was 
added there). Using cleanup.h for that variable it clearly simplifies the 
code flow. It feels a bit stupid to duplicate it there but I guess we'll 
just have to live with that if there's no place in any acpi related 
headers for cleanup.h.

-- 
 i.

Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Rafael J. Wysocki (Intel)]

On Tue, Aug 25, 2026 at 2:32 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:
>
> > On Tue, Aug 25, 2026 at 2:07 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Rafael,
> > >
> > > On Tue, Aug 25, 2026 at 01:51:18PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Use DEFINE_FREE() to allow ACPI objects to be released automatically.
> > > >
> > > > But at least some of them are allocated by ACPICA functions like
> > > > acpi_evaluate_object() and so they have no proper constructors.
> > >
> > > You could still assign the return buffer to a local variable. It's not
> > > ideal API-wise though.
> >
> > Exactly.
> >
> > > I'm not quite sure what was the point you wanted to make but I reckon this
> > > wasn't an ack. :-)
> >
> > Using the _FREE with variables that are not initialized through a
> > constructor is questionable, so this is generally not particularly
> > clean.
>
> The driver does call ACPI_FREE() for that pointer so clearly it's already
> using something ending with "_FREE" already. So unless Rafael is
> suggesting ACPI_FREE() should be renamed, I'm a bit lost what that
> even means on concrete terms.

Sorry for the confusion.

I just don't want people to do things like this:

    struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER };
    union acpi_object *out_obj __free(ACPI_FREE);
    acpi_status status;

    status = acpi_evaluate_object(handle, METHOD_NAME, NULL, &output);
    if (ACPI_FAILURE(status))
        return AN_ERROR;

   out_obj = output.pointer;

> > There is no cleanup.h in ACPICA that is a more traditional C code
> > base, so mixing up ACPICA code, which ACPI_FREE() is strictly
> > speaking, with cleanup.h stuff is not particularly straightforward
> > IMV.  I'd rather not do it.
>
> Perhaps add the DEFINE_FREE() into int3472 driver then, it seems to be
> what intel/pmc is already doing (I don't seem to anymore recall why it was
> added there). Using cleanup.h for that variable it clearly simplifies the
> code flow.

Well, fair enough, but as I said elsewhere, the code flow
simplification can also be achieved in a different way.

> It feels a bit stupid to duplicate it there but I guess we'll
> just have to live with that if there's no place in any acpi related
> headers for cleanup.h.

If there is a cleanup.h "free" that can only be used with objects
returned by acpi_evaluate_dsm_typed(), I'll be fine with that.

Or if everyone agrees that doing

    union acpi_object *out_obj __free(ACPI_FREE) = NULL;

is not confusing and fine, I may just say "Hey, I don't care that much".

Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Rafael J. Wysocki (Intel)]

On Tue, Aug 25, 2026 at 3:02 PM Rafael J. Wysocki (Intel)
<rafael@kernel.org> wrote:
>
> On Tue, Aug 25, 2026 at 2:32 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:
> >
> > > On Tue, Aug 25, 2026 at 2:07 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Rafael,
> > > >
> > > > On Tue, Aug 25, 2026 at 01:51:18PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > > On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > Use DEFINE_FREE() to allow ACPI objects to be released automatically.
> > > > >
> > > > > But at least some of them are allocated by ACPICA functions like
> > > > > acpi_evaluate_object() and so they have no proper constructors.
> > > >
> > > > You could still assign the return buffer to a local variable. It's not
> > > > ideal API-wise though.
> > >
> > > Exactly.
> > >
> > > > I'm not quite sure what was the point you wanted to make but I reckon this
> > > > wasn't an ack. :-)
> > >
> > > Using the _FREE with variables that are not initialized through a
> > > constructor is questionable, so this is generally not particularly
> > > clean.
> >
> > The driver does call ACPI_FREE() for that pointer so clearly it's already
> > using something ending with "_FREE" already. So unless Rafael is
> > suggesting ACPI_FREE() should be renamed, I'm a bit lost what that
> > even means on concrete terms.
>
> Sorry for the confusion.
>
> I just don't want people to do things like this:
>
>     struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER };
>     union acpi_object *out_obj __free(ACPI_FREE);
>     acpi_status status;
>
>     status = acpi_evaluate_object(handle, METHOD_NAME, NULL, &output);
>     if (ACPI_FAILURE(status))
>         return AN_ERROR;
>
>    out_obj = output.pointer;
>
> > > There is no cleanup.h in ACPICA that is a more traditional C code
> > > base, so mixing up ACPICA code, which ACPI_FREE() is strictly
> > > speaking, with cleanup.h stuff is not particularly straightforward
> > > IMV.  I'd rather not do it.
> >
> > Perhaps add the DEFINE_FREE() into int3472 driver then, it seems to be
> > what intel/pmc is already doing (I don't seem to anymore recall why it was
> > added there). Using cleanup.h for that variable it clearly simplifies the
> > code flow.
>
> Well, fair enough, but as I said elsewhere, the code flow
> simplification can also be achieved in a different way.
>
> > It feels a bit stupid to duplicate it there but I guess we'll
> > just have to live with that if there's no place in any acpi related
> > headers for cleanup.h.
>
> If there is a cleanup.h "free" that can only be used with objects
> returned by acpi_evaluate_dsm_typed(), I'll be fine with that.
>
> Or if everyone agrees that doing
>
>     union acpi_object *out_obj __free(ACPI_FREE) = NULL;
>
> is not confusing and fine, I may just say "Hey, I don't care that much".

And particularly there is this paragraph in a comment in cleanup.h:

 * Given that the "__free(...) = NULL" pattern for variables defined at
 * the top of the function poses this potential interdependency problem
 * the recommendation is to always define and assign variables in one
 * statement and not group variable definitions at the top of the
 * function when __free() is used.

regarding a broken code example, so I would think that this is not a
made-up concern.

Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4394 bytes --]

On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:

> On Tue, Aug 25, 2026 at 3:02 PM Rafael J. Wysocki (Intel)
> <rafael@kernel.org> wrote:
> >
> > On Tue, Aug 25, 2026 at 2:32 PM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > >
> > > On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:
> > >
> > > > On Tue, Aug 25, 2026 at 2:07 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Rafael,
> > > > >
> > > > > On Tue, Aug 25, 2026 at 01:51:18PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > > > On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
> > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > >
> > > > > > > Use DEFINE_FREE() to allow ACPI objects to be released automatically.
> > > > > >
> > > > > > But at least some of them are allocated by ACPICA functions like
> > > > > > acpi_evaluate_object() and so they have no proper constructors.
> > > > >
> > > > > You could still assign the return buffer to a local variable. It's not
> > > > > ideal API-wise though.
> > > >
> > > > Exactly.
> > > >
> > > > > I'm not quite sure what was the point you wanted to make but I reckon this
> > > > > wasn't an ack. :-)
> > > >
> > > > Using the _FREE with variables that are not initialized through a
> > > > constructor is questionable, so this is generally not particularly
> > > > clean.
> > >
> > > The driver does call ACPI_FREE() for that pointer so clearly it's already
> > > using something ending with "_FREE" already. So unless Rafael is
> > > suggesting ACPI_FREE() should be renamed, I'm a bit lost what that
> > > even means on concrete terms.
> >
> > Sorry for the confusion.
> >
> > I just don't want people to do things like this:
> >
> >     struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER };
> >     union acpi_object *out_obj __free(ACPI_FREE);
> >     acpi_status status;
> >
> >     status = acpi_evaluate_object(handle, METHOD_NAME, NULL, &output);
> >     if (ACPI_FAILURE(status))
> >         return AN_ERROR;
> >
> >    out_obj = output.pointer;
> >
> > > > There is no cleanup.h in ACPICA that is a more traditional C code
> > > > base, so mixing up ACPICA code, which ACPI_FREE() is strictly
> > > > speaking, with cleanup.h stuff is not particularly straightforward
> > > > IMV.  I'd rather not do it.
> > >
> > > Perhaps add the DEFINE_FREE() into int3472 driver then, it seems to be
> > > what intel/pmc is already doing (I don't seem to anymore recall why it was
> > > added there). Using cleanup.h for that variable it clearly simplifies the
> > > code flow.
> >
> > Well, fair enough, but as I said elsewhere, the code flow
> > simplification can also be achieved in a different way.
> >
> > > It feels a bit stupid to duplicate it there but I guess we'll
> > > just have to live with that if there's no place in any acpi related
> > > headers for cleanup.h.
> >
> > If there is a cleanup.h "free" that can only be used with objects
> > returned by acpi_evaluate_dsm_typed(), I'll be fine with that.
> >
> > Or if everyone agrees that doing
> >
> >     union acpi_object *out_obj __free(ACPI_FREE) = NULL;
> >
> > is not confusing and fine, I may just say "Hey, I don't care that much".
> 
> And particularly there is this paragraph in a comment in cleanup.h:
> 
>  * Given that the "__free(...) = NULL" pattern for variables defined at
>  * the top of the function poses this potential interdependency problem
>  * the recommendation is to always define and assign variables in one
>  * statement and not group variable definitions at the top of the
>  * function when __free() is used.
> 
> regarding a broken code example, so I would think that this is not a
> made-up concern.

That comment relates to how defining the variables at the start of 
functions may result in wrong/unexpected cleanup order. More imporantly, 
the comment is not an argument for not using __free() but an instruction 
on what is the correct pattern to use it so those ordering issues do not 
occur.

The cleanups will execute in reverse order the variables where defined so 
a variable defined at start may be cleaned up only after releasing a lock 
that was taken mid-function (which often is safe but one can easily 
envision cases where the lock should be still held when the cleanup runs).

-- 
 i.

Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Rafael J. Wysocki (Intel)]

On Tue, Aug 25, 2026 at 3:51 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:
>
> > On Tue, Aug 25, 2026 at 3:02 PM Rafael J. Wysocki (Intel)
> > <rafael@kernel.org> wrote:
> > >
> > > On Tue, Aug 25, 2026 at 2:32 PM Ilpo Järvinen
> > > <ilpo.jarvinen@linux.intel.com> wrote:
> > > >
> > > > On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:
> > > >
> > > > > On Tue, Aug 25, 2026 at 2:07 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > Hi Rafael,
> > > > > >
> > > > > > On Tue, Aug 25, 2026 at 01:51:18PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > > > > On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
> > > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > > >
> > > > > > > > Use DEFINE_FREE() to allow ACPI objects to be released automatically.
> > > > > > >
> > > > > > > But at least some of them are allocated by ACPICA functions like
> > > > > > > acpi_evaluate_object() and so they have no proper constructors.
> > > > > >
> > > > > > You could still assign the return buffer to a local variable. It's not
> > > > > > ideal API-wise though.
> > > > >
> > > > > Exactly.
> > > > >
> > > > > > I'm not quite sure what was the point you wanted to make but I reckon this
> > > > > > wasn't an ack. :-)
> > > > >
> > > > > Using the _FREE with variables that are not initialized through a
> > > > > constructor is questionable, so this is generally not particularly
> > > > > clean.
> > > >
> > > > The driver does call ACPI_FREE() for that pointer so clearly it's already
> > > > using something ending with "_FREE" already. So unless Rafael is
> > > > suggesting ACPI_FREE() should be renamed, I'm a bit lost what that
> > > > even means on concrete terms.
> > >
> > > Sorry for the confusion.
> > >
> > > I just don't want people to do things like this:
> > >
> > >     struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER };
> > >     union acpi_object *out_obj __free(ACPI_FREE);
> > >     acpi_status status;
> > >
> > >     status = acpi_evaluate_object(handle, METHOD_NAME, NULL, &output);
> > >     if (ACPI_FAILURE(status))
> > >         return AN_ERROR;
> > >
> > >    out_obj = output.pointer;
> > >
> > > > > There is no cleanup.h in ACPICA that is a more traditional C code
> > > > > base, so mixing up ACPICA code, which ACPI_FREE() is strictly
> > > > > speaking, with cleanup.h stuff is not particularly straightforward
> > > > > IMV.  I'd rather not do it.
> > > >
> > > > Perhaps add the DEFINE_FREE() into int3472 driver then, it seems to be
> > > > what intel/pmc is already doing (I don't seem to anymore recall why it was
> > > > added there). Using cleanup.h for that variable it clearly simplifies the
> > > > code flow.
> > >
> > > Well, fair enough, but as I said elsewhere, the code flow
> > > simplification can also be achieved in a different way.
> > >
> > > > It feels a bit stupid to duplicate it there but I guess we'll
> > > > just have to live with that if there's no place in any acpi related
> > > > headers for cleanup.h.
> > >
> > > If there is a cleanup.h "free" that can only be used with objects
> > > returned by acpi_evaluate_dsm_typed(), I'll be fine with that.
> > >
> > > Or if everyone agrees that doing
> > >
> > >     union acpi_object *out_obj __free(ACPI_FREE) = NULL;
> > >
> > > is not confusing and fine, I may just say "Hey, I don't care that much".
> >
> > And particularly there is this paragraph in a comment in cleanup.h:
> >
> >  * Given that the "__free(...) = NULL" pattern for variables defined at
> >  * the top of the function poses this potential interdependency problem
> >  * the recommendation is to always define and assign variables in one
> >  * statement and not group variable definitions at the top of the
> >  * function when __free() is used.
> >
> > regarding a broken code example, so I would think that this is not a
> > made-up concern.
>
> That comment relates to how defining the variables at the start of
> functions may result in wrong/unexpected cleanup order. More imporantly,
> the comment is not an argument for not using __free() but an instruction
> on what is the correct pattern to use it so those ordering issues do not
> occur.
>
> The cleanups will execute in reverse order the variables where defined so
> a variable defined at start may be cleaned up only after releasing a lock
> that was taken mid-function (which often is safe but one can easily
> envision cases where the lock should be still held when the cleanup runs).

I agree with all of the above.

Though I think that cleanup ordering issues are easier to overlook
when the "__free(...) = NULL" pattern is used, which is why I wouldn't
like to encourage using it.

Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Sakari Ailus]

Hi Rafael, Ilpo,

On Tue, Aug 25, 2026 at 03:08:07PM +0200, Rafael J. Wysocki (Intel) wrote:
> > If there is a cleanup.h "free" that can only be used with objects
> > returned by acpi_evaluate_dsm_typed(), I'll be fine with that.
> >
> > Or if everyone agrees that doing
> >
> >     union acpi_object *out_obj __free(ACPI_FREE) = NULL;
> >
> > is not confusing and fine, I may just say "Hey, I don't care that much".
> 
> And particularly there is this paragraph in a comment in cleanup.h:
> 
>  * Given that the "__free(...) = NULL" pattern for variables defined at
>  * the top of the function poses this potential interdependency problem
>  * the recommendation is to always define and assign variables in one
>  * statement and not group variable definitions at the top of the
>  * function when __free() is used.
> 
> regarding a broken code example, so I would think that this is not a
> made-up concern.

That's indeed a valid concern, still quite unlikely in practice but
probably hard to find when it happens, so avoiding that is definitely
preferred. That being said, the biggest trap in cleanup.h is probably in
scoped_guard(), and doing __free() = NULL somewhere doesn't matter much in
the end.

In this case I'll just call ACPI_FREE() sooner.

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Ilpo Järvinen]

On Tue, 25 Aug 2026, Sakari Ailus wrote:

> Hi Rafael, Ilpo,
> 
> On Tue, Aug 25, 2026 at 03:08:07PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > If there is a cleanup.h "free" that can only be used with objects
> > > returned by acpi_evaluate_dsm_typed(), I'll be fine with that.
> > >
> > > Or if everyone agrees that doing
> > >
> > >     union acpi_object *out_obj __free(ACPI_FREE) = NULL;
> > >
> > > is not confusing and fine, I may just say "Hey, I don't care that much".
> > 
> > And particularly there is this paragraph in a comment in cleanup.h:
> > 
> >  * Given that the "__free(...) = NULL" pattern for variables defined at
> >  * the top of the function poses this potential interdependency problem
> >  * the recommendation is to always define and assign variables in one
> >  * statement and not group variable definitions at the top of the
> >  * function when __free() is used.
> > 
> > regarding a broken code example, so I would think that this is not a
> > made-up concern.
> 
> That's indeed a valid concern, still quite unlikely in practice but
> probably hard to find when it happens, so avoiding that is definitely
> preferred. That being said, the biggest trap in cleanup.h is probably in
> scoped_guard(), and doing __free() = NULL somewhere doesn't matter much in
> the end.

__free() = NULL is not that hard to catch during review (or even in
code already in-tree), have done that dozens of times myself by now.
Checkpatch, too, should be able to catch that easily, if it doesn't 
already.

None of those cases I've commented on had a bug, so it was just for 
teaching submitters & readers of that code the correct __free() pattern.
I think the concern is largely overblown given how rare actual bugs are 
even if the wrong pattern is used. Put that to contrast to memleaks found 
on our rollback paths, __free() looks a clear win despite very rare to 
occur caveats.

Given what I've seen, I'd say on dangerous level __free() = NULL is 
somewhere around using MAGIC_SIZE_DEFINE instead sizeof(*obj) when doing 
mem allocs. It usually isn't buggy even if we don't want to teach people 
to use it.

Besides, it was actually Rafael himself who brought the unsafe pattern 
into this discussion (I immediately noticed the problem but since it was 
not an actual patch, I didn't raise a concern). Sakari's patch did use the 
correct pattern (and if it wouldn't have done so, there would have been a 
review comment from me ;-)). In my reply to Rafael, I intentionally left 
the right side open with "= ..." to not place that NULL there.

> In this case I'll just call ACPI_FREE() sooner.

That works too in this case, yes.

-- 
 i.

Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Rafael J. Wysocki (Intel)]

On Wed, Aug 26, 2026 at 11:04 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Tue, 25 Aug 2026, Sakari Ailus wrote:
>
> > Hi Rafael, Ilpo,
> >
> > On Tue, Aug 25, 2026 at 03:08:07PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > If there is a cleanup.h "free" that can only be used with objects
> > > > returned by acpi_evaluate_dsm_typed(), I'll be fine with that.
> > > >
> > > > Or if everyone agrees that doing
> > > >
> > > >     union acpi_object *out_obj __free(ACPI_FREE) = NULL;
> > > >
> > > > is not confusing and fine, I may just say "Hey, I don't care that much".
> > >
> > > And particularly there is this paragraph in a comment in cleanup.h:
> > >
> > >  * Given that the "__free(...) = NULL" pattern for variables defined at
> > >  * the top of the function poses this potential interdependency problem
> > >  * the recommendation is to always define and assign variables in one
> > >  * statement and not group variable definitions at the top of the
> > >  * function when __free() is used.
> > >
> > > regarding a broken code example, so I would think that this is not a
> > > made-up concern.
> >
> > That's indeed a valid concern, still quite unlikely in practice but
> > probably hard to find when it happens, so avoiding that is definitely
> > preferred. That being said, the biggest trap in cleanup.h is probably in
> > scoped_guard(), and doing __free() = NULL somewhere doesn't matter much in
> > the end.
>
> __free() = NULL is not that hard to catch during review (or even in
> code already in-tree), have done that dozens of times myself by now.
> Checkpatch, too, should be able to catch that easily, if it doesn't
> already.
>
> None of those cases I've commented on had a bug, so it was just for
> teaching submitters & readers of that code the correct __free() pattern.
> I think the concern is largely overblown given how rare actual bugs are
> even if the wrong pattern is used. Put that to contrast to memleaks found
> on our rollback paths, __free() looks a clear win despite very rare to
> occur caveats.
>
> Given what I've seen, I'd say on dangerous level __free() = NULL is
> somewhere around using MAGIC_SIZE_DEFINE instead sizeof(*obj) when doing
> mem allocs. It usually isn't buggy even if we don't want to teach people
> to use it.
>
> Besides, it was actually Rafael himself who brought the unsafe pattern
> into this discussion (I immediately noticed the problem but since it was
> not an actual patch, I didn't raise a concern). Sakari's patch did use the
> correct pattern (and if it wouldn't have done so, there would have been a
> review comment from me ;-)). In my reply to Rafael, I intentionally left
> the right side open with "= ..." to not place that NULL there.
>
> > In this case I'll just call ACPI_FREE() sooner.
>
> That works too in this case, yes.

So overall, something like this can be defined

static inline void free_acpi_object(union acpi_object *obj)
{
        ACPI_FREE(obj);
}

DEFINE_FREE(acpi_object_free, union acpi_object *, free_acpi_object(_T))

and used along with acpi_evaluate_dsm*().

Moreover, a wrapper around acpi_evaluate_object() returning a pointer
to union acpi_object on success and NULL on failure can be defined and
used along with the above.

IMV that would be way cleaner than wrapping a raw cleanup macro around
ACPI_FREE().

I generally think that there needs to be a wrapper layer around the
ACPICA API, so drivers don't need to call functions returning
acpi_status directly, for instance.

Someday we'll get there.

Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3619 bytes --]

On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:

> On Tue, Aug 25, 2026 at 2:32 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:
> >
> > > On Tue, Aug 25, 2026 at 2:07 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Rafael,
> > > >
> > > > On Tue, Aug 25, 2026 at 01:51:18PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > > On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > Use DEFINE_FREE() to allow ACPI objects to be released automatically.
> > > > >
> > > > > But at least some of them are allocated by ACPICA functions like
> > > > > acpi_evaluate_object() and so they have no proper constructors.
> > > >
> > > > You could still assign the return buffer to a local variable. It's not
> > > > ideal API-wise though.
> > >
> > > Exactly.
> > >
> > > > I'm not quite sure what was the point you wanted to make but I reckon this
> > > > wasn't an ack. :-)
> > >
> > > Using the _FREE with variables that are not initialized through a
> > > constructor is questionable, so this is generally not particularly
> > > clean.
> >
> > The driver does call ACPI_FREE() for that pointer so clearly it's already
> > using something ending with "_FREE" already. So unless Rafael is
> > suggesting ACPI_FREE() should be renamed, I'm a bit lost what that
> > even means on concrete terms.
> 
> Sorry for the confusion.
> 
> I just don't want people to do things like this:
> 
>     struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER };
>     union acpi_object *out_obj __free(ACPI_FREE);
>     acpi_status status;
> 
>     status = acpi_evaluate_object(handle, METHOD_NAME, NULL, &output);
>     if (ACPI_FAILURE(status))
>         return AN_ERROR;
> 
>    out_obj = output.pointer;
> 
> > > There is no cleanup.h in ACPICA that is a more traditional C code
> > > base, so mixing up ACPICA code, which ACPI_FREE() is strictly
> > > speaking, with cleanup.h stuff is not particularly straightforward
> > > IMV.  I'd rather not do it.
> >
> > Perhaps add the DEFINE_FREE() into int3472 driver then, it seems to be
> > what intel/pmc is already doing (I don't seem to anymore recall why it was
> > added there). Using cleanup.h for that variable it clearly simplifies the
> > code flow.
> 
> Well, fair enough, but as I said elsewhere, the code flow
> simplification can also be achieved in a different way.
> 
> > It feels a bit stupid to duplicate it there but I guess we'll
> > just have to live with that if there's no place in any acpi related
> > headers for cleanup.h.
> 
> If there is a cleanup.h "free" that can only be used with objects
> returned by acpi_evaluate_dsm_typed(), I'll be fine with that.

So you'd be fine with something like this:

      union acpi_object *out_obj __free(some_other_name_than_ACPI_FREE) = ...

?

> Or if everyone agrees that doing
> 
>     union acpi_object *out_obj __free(ACPI_FREE) = NULL;
>
> is not confusing and fine, I may just say "Hey, I don't care that much".

I personally don't hang myself into "free" or "constructor" terminology 
but look it more pragmatically, if ACPI_FREE() should be called before 
the object goes out of scope, __free() is a tool for that.

Have Rust people tried to wrap this interface already (there seems to 
acpi.rs but it seems very limited in scope)? I'd expect them to be just as 
interested in knowing what to do when something is at the end of its
lifetime.

-- 
 i.

Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

[Rafael J. Wysocki (Intel)]

On Tue, Aug 25, 2026 at 3:37 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:
>
> > On Tue, Aug 25, 2026 at 2:32 PM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > >
> > > On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:
> > >
> > > > On Tue, Aug 25, 2026 at 2:07 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Rafael,
> > > > >
> > > > > On Tue, Aug 25, 2026 at 01:51:18PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > > > On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
> > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > >
> > > > > > > Use DEFINE_FREE() to allow ACPI objects to be released automatically.
> > > > > >
> > > > > > But at least some of them are allocated by ACPICA functions like
> > > > > > acpi_evaluate_object() and so they have no proper constructors.
> > > > >
> > > > > You could still assign the return buffer to a local variable. It's not
> > > > > ideal API-wise though.
> > > >
> > > > Exactly.
> > > >
> > > > > I'm not quite sure what was the point you wanted to make but I reckon this
> > > > > wasn't an ack. :-)
> > > >
> > > > Using the _FREE with variables that are not initialized through a
> > > > constructor is questionable, so this is generally not particularly
> > > > clean.
> > >
> > > The driver does call ACPI_FREE() for that pointer so clearly it's already
> > > using something ending with "_FREE" already. So unless Rafael is
> > > suggesting ACPI_FREE() should be renamed, I'm a bit lost what that
> > > even means on concrete terms.
> >
> > Sorry for the confusion.
> >
> > I just don't want people to do things like this:
> >
> >     struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER };
> >     union acpi_object *out_obj __free(ACPI_FREE);
> >     acpi_status status;
> >
> >     status = acpi_evaluate_object(handle, METHOD_NAME, NULL, &output);
> >     if (ACPI_FAILURE(status))
> >         return AN_ERROR;
> >
> >    out_obj = output.pointer;
> >
> > > > There is no cleanup.h in ACPICA that is a more traditional C code
> > > > base, so mixing up ACPICA code, which ACPI_FREE() is strictly
> > > > speaking, with cleanup.h stuff is not particularly straightforward
> > > > IMV.  I'd rather not do it.
> > >
> > > Perhaps add the DEFINE_FREE() into int3472 driver then, it seems to be
> > > what intel/pmc is already doing (I don't seem to anymore recall why it was
> > > added there). Using cleanup.h for that variable it clearly simplifies the
> > > code flow.
> >
> > Well, fair enough, but as I said elsewhere, the code flow
> > simplification can also be achieved in a different way.
> >
> > > It feels a bit stupid to duplicate it there but I guess we'll
> > > just have to live with that if there's no place in any acpi related
> > > headers for cleanup.h.
> >
> > If there is a cleanup.h "free" that can only be used with objects
> > returned by acpi_evaluate_dsm_typed(), I'll be fine with that.
>
> So you'd be fine with something like this:
>
>       union acpi_object *out_obj __free(some_other_name_than_ACPI_FREE) = ...
>
> ?

Basically, yes.

> > Or if everyone agrees that doing
> >
> >     union acpi_object *out_obj __free(ACPI_FREE) = NULL;
> >
> > is not confusing and fine, I may just say "Hey, I don't care that much".
>
> I personally don't hang myself into "free" or "constructor" terminology
> but look it more pragmatically, if ACPI_FREE() should be called before
> the object goes out of scope, __free() is a tool for that.

It is also good to consider the example given in cleanup.h though, in
which there is the

            struct object *obj __free(remove_free) = NULL;

pattern leading to a real bug due to a cleanup ordering issue.

I would prefer to avoid encouraging people to create code patterns
that are potentially problematic.  The "free" or "constructor"
terminology mentioned above boils down to avoiding those code patterns
which also matters in practice.

> Have Rust people tried to wrap this interface already (there seems to
> acpi.rs but it seems very limited in scope)? I'd expect them to be just as
> interested in knowing what to do when something is at the end of its
> lifetime.

No, they haven't, AFAICS.  At least I'm not aware of any attempts to do so.

[PATCH v2 4/5] platform/x86: int3472: Release ACPI objects using __free()

[Sakari Ailus]

Use __free() to release ACPI objects received from
acpi_evaluate_dsm_typed() without explicit ACPI_FREE().

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index b4a95b583edb..2024eaa09033 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -4,6 +4,7 @@
 #include <linux/acpi.h>
 #include <linux/array_size.h>
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
 #include <linux/gpio/consumer.h>
@@ -327,7 +328,6 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	unsigned int enable_time_us;
 	u8 active_value, pin, type;
 	unsigned long gpio_flags;
-	union acpi_object *obj;
 	struct gpio_desc *gpio;
 	const char *con_id;
 	int ret;
@@ -339,10 +339,11 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	 * ngpios + 2 because the index of this _DSM function is 1-based and
 	 * the first function is just a count.
 	 */
-	obj = acpi_evaluate_dsm_typed(int3472->adev->handle,
-				      &int3472_gpio_guid, 0x00,
-				      int3472->ngpios + 2,
-				      NULL, ACPI_TYPE_INTEGER);
+	union acpi_object *obj __free(ACPI_FREE) =
+		acpi_evaluate_dsm_typed(int3472->adev->handle,
+					&int3472_gpio_guid, 0x00,
+					int3472->ngpios + 2,
+					NULL, ACPI_TYPE_INTEGER);
 
 	if (!obj) {
 		dev_warn(int3472->dev, "No _DSM entry for GPIO pin %u\n",
@@ -433,7 +434,6 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	}
 
 	int3472->ngpios++;
-	ACPI_FREE(obj);
 
 	/*
 	 * Either return an error or tell acpi_dev_get_resources() to not make a
-- 
2.47.3

Re: [PATCH v2 4/5] platform/x86: int3472: Release ACPI objects using __free()

[Rafael J. Wysocki (Intel)]

On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Use __free() to release ACPI objects received from
> acpi_evaluate_dsm_typed() without explicit ACPI_FREE().

Well, the benefit of doing so is not entirely clear to me.

At least there is more overhead in the new code, or am I mistaken?

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/platform/x86/intel/int3472/discrete.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index b4a95b583edb..2024eaa09033 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -4,6 +4,7 @@
>  #include <linux/acpi.h>
>  #include <linux/array_size.h>
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/device.h>
>  #include <linux/dmi.h>
>  #include <linux/gpio/consumer.h>
> @@ -327,7 +328,6 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>         unsigned int enable_time_us;
>         u8 active_value, pin, type;
>         unsigned long gpio_flags;
> -       union acpi_object *obj;
>         struct gpio_desc *gpio;
>         const char *con_id;
>         int ret;
> @@ -339,10 +339,11 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>          * ngpios + 2 because the index of this _DSM function is 1-based and
>          * the first function is just a count.
>          */
> -       obj = acpi_evaluate_dsm_typed(int3472->adev->handle,
> -                                     &int3472_gpio_guid, 0x00,
> -                                     int3472->ngpios + 2,
> -                                     NULL, ACPI_TYPE_INTEGER);
> +       union acpi_object *obj __free(ACPI_FREE) =
> +               acpi_evaluate_dsm_typed(int3472->adev->handle,
> +                                       &int3472_gpio_guid, 0x00,
> +                                       int3472->ngpios + 2,
> +                                       NULL, ACPI_TYPE_INTEGER);
>
>         if (!obj) {
>                 dev_warn(int3472->dev, "No _DSM entry for GPIO pin %u\n",
> @@ -433,7 +434,6 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>         }
>
>         int3472->ngpios++;
> -       ACPI_FREE(obj);
>
>         /*
>          * Either return an error or tell acpi_dev_get_resources() to not make a
> --
> 2.47.3
>

Re: [PATCH v2 4/5] platform/x86: int3472: Release ACPI objects using __free()

[Sakari Ailus]

Hi Rafael,

Thank you for the review.

On Tue, Aug 25, 2026 at 01:54:28PM +0200, Rafael J. Wysocki (Intel) wrote:
> On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Use __free() to release ACPI objects received from
> > acpi_evaluate_dsm_typed() without explicit ACPI_FREE().
> 
> Well, the benefit of doing so is not entirely clear to me.
> 
> At least there is more overhead in the new code, or am I mistaken?

It becomes possible to simplify error handling mid-function. See the 5th
patch -- perhaps squashing this with that would make this more visible?

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 4/5] platform/x86: int3472: Release ACPI objects using __free()

[Rafael J. Wysocki (Intel)]

On Tue, Aug 25, 2026 at 2:04 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> Thank you for the review.
>
> On Tue, Aug 25, 2026 at 01:54:28PM +0200, Rafael J. Wysocki (Intel) wrote:
> > On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Use __free() to release ACPI objects received from
> > > acpi_evaluate_dsm_typed() without explicit ACPI_FREE().
> >
> > Well, the benefit of doing so is not entirely clear to me.
> >
> > At least there is more overhead in the new code, or am I mistaken?
>
> It becomes possible to simplify error handling mid-function. See the 5th
> patch -- perhaps squashing this with that would make this more visible?

Possibly, but this may also be achieved by processing the object
coming from the ACPICA code in a separate function.  For example

obj = acpi_evaluate_dsm_typed(args);
ret = do_stuff_with_obj(obj);
ACPI_FREE(obj);
return ret;

and the error handling in do_stuff_with_obj() can be just like in the
case when __free() is used in the caller.

Of course, you may argue that acpi_evaluate_dsm_typed() can be used as
a proper constructor, so this is all fine, but IMV it is a special
case and I'm not quite comfortable with defining a _FREE that looks
general enough for this particular special case.

[PATCH v2 5/5] platform/x86: int3472: Clean up GPIO parsing

[Sakari Ailus]

In skl_int3472_handle_gpio_resources(), return an error where it happens,
except when we're holding a reference to a GPIO. This involves
incrementing ngpios earlier on but that does not introduce a functional
change.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 42 +++++++++----------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 2024eaa09033..400bee9e4cb8 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -369,15 +369,23 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 		agpio->resource_source.string_ptr, agpio->pin_table[0],
 		str_high_low(gpio_flags == GPIO_ACTIVE_HIGH));
 
+	/*
+	 * int3472->ngpios can be incremented here as it is an argument to the
+	 * _DSM, not e.g. an index to an array in C. Additionally, in case of an
+	 * error the value won't be used.
+	 */
+	int3472->ngpios++;
+
 	switch (type) {
 	case INT3472_GPIO_TYPE_RESET:
 	case INT3472_GPIO_TYPE_POWERDOWN:
 	case INT3472_GPIO_TYPE_HOTPLUG_DETECT:
 		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, con_id, gpio_flags);
 		if (ret)
-			dev_err_probe(int3472->dev, ret, "Failed to map GPIO pin to sensor\n");
+			return dev_err_probe(int3472->dev, ret,
+					     "Failed to map GPIO pin to sensor\n");
 
-		break;
+		return 0;
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
 	case INT3472_GPIO_TYPE_PRIVACY_LED:
 	case INT3472_GPIO_TYPE_STROBE:
@@ -385,24 +393,24 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	case INT3472_GPIO_TYPE_DOVDD:
 	case INT3472_GPIO_TYPE_HANDSHAKE:
 		gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, con_id, gpio_flags);
-		if (IS_ERR(gpio)) {
-			ret = PTR_ERR(gpio);
-			dev_err_probe(int3472->dev, ret, "Failed to get GPIO\n");
-			break;
-		}
+		if (IS_ERR(gpio))
+			return dev_err_probe(int3472->dev, PTR_ERR(gpio),
+					     "Failed to get GPIO\n");
 
 		switch (type) {
 		case INT3472_GPIO_TYPE_CLK_ENABLE:
 			ret = skl_int3472_register_gpio_clock(int3472, gpio);
 			if (ret)
-				dev_err_probe(int3472->dev, ret, "Failed to register clock\n");
+				dev_err_probe(int3472->dev, ret,
+					      "Failed to register clock\n");
 
 			break;
 		case INT3472_GPIO_TYPE_PRIVACY_LED:
 		case INT3472_GPIO_TYPE_STROBE:
 			ret = skl_int3472_register_led(int3472, gpio, con_id);
 			if (ret)
-				dev_err_probe(int3472->dev, ret, "Failed to register LED\n");
+				dev_err_probe(int3472->dev, ret,
+					      "Failed to register LED\n");
 
 			break;
 		case INT3472_GPIO_TYPE_POWER_ENABLE:
@@ -413,7 +421,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 			ret = skl_int3472_register_regulator(int3472, gpio, enable_time_us,
 							     con_id, second_sensor);
 			if (ret)
-				dev_err_probe(int3472->dev, ret, "Failed to register regulator\n");
+				dev_err_probe(int3472->dev, ret,
+					      "Failed to register regulator\n");
 
 			break;
 		default: /* Never reached */
@@ -424,22 +433,13 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 		if (ret)
 			gpiod_put(gpio);
 
-		break;
+		return ret;
 	default:
 		dev_warn(int3472->dev,
 			 "GPIO type 0x%02x unknown; the sensor may not work\n",
 			 type);
-		ret = 1;
-		break;
+		return 1;
 	}
-
-	int3472->ngpios++;
-
-	/*
-	 * Either return an error or tell acpi_dev_get_resources() to not make a
-	 * copy of the resource.
-	 */
-	return ret < 0 ? ret : 1;
 }
 
 int int3472_discrete_parse_crs(struct int3472_discrete_device *int3472)
-- 
2.47.3

Re: [PATCH v2 5/5] platform/x86: int3472: Clean up GPIO parsing

[Ilpo Järvinen]

On Tue, 25 Aug 2026, Sakari Ailus wrote:

> In skl_int3472_handle_gpio_resources(), return an error where it happens,
> except when we're holding a reference to a GPIO. This involves
> incrementing ngpios earlier on but that does not introduce a functional
> change.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/platform/x86/intel/int3472/discrete.c | 42 +++++++++----------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index 2024eaa09033..400bee9e4cb8 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -369,15 +369,23 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  		agpio->resource_source.string_ptr, agpio->pin_table[0],
>  		str_high_low(gpio_flags == GPIO_ACTIVE_HIGH));
>  
> +	/*
> +	 * int3472->ngpios can be incremented here as it is an argument to the
> +	 * _DSM, not e.g. an index to an array in C. Additionally, in case of an
> +	 * error the value won't be used.
> +	 */

I wasn't expecting comment for this. You might have added it because of 
my questions but IMO it's not needed.

I think just the old way/place was confusing, especially given how 
some/part of the error cases did increment it. But after moving it here, 
it makes much more sense even without that comment.

And one can determine it's not being used for C arrays by using grep so 
that too seems just unnecessary detail.

> +	int3472->ngpios++;
> +
>  	switch (type) {
>  	case INT3472_GPIO_TYPE_RESET:
>  	case INT3472_GPIO_TYPE_POWERDOWN:
>  	case INT3472_GPIO_TYPE_HOTPLUG_DETECT:
>  		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, con_id, gpio_flags);
>  		if (ret)
> -			dev_err_probe(int3472->dev, ret, "Failed to map GPIO pin to sensor\n");
> +			return dev_err_probe(int3472->dev, ret,
> +					     "Failed to map GPIO pin to sensor\n");
>  
> -		break;
> +		return 0;
>  	case INT3472_GPIO_TYPE_CLK_ENABLE:
>  	case INT3472_GPIO_TYPE_PRIVACY_LED:
>  	case INT3472_GPIO_TYPE_STROBE:
> @@ -385,24 +393,24 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  	case INT3472_GPIO_TYPE_DOVDD:
>  	case INT3472_GPIO_TYPE_HANDSHAKE:
>  		gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, con_id, gpio_flags);
> -		if (IS_ERR(gpio)) {
> -			ret = PTR_ERR(gpio);
> -			dev_err_probe(int3472->dev, ret, "Failed to get GPIO\n");
> -			break;
> -		}
> +		if (IS_ERR(gpio))
> +			return dev_err_probe(int3472->dev, PTR_ERR(gpio),
> +					     "Failed to get GPIO\n");
>  
>  		switch (type) {
>  		case INT3472_GPIO_TYPE_CLK_ENABLE:
>  			ret = skl_int3472_register_gpio_clock(int3472, gpio);
>  			if (ret)
> -				dev_err_probe(int3472->dev, ret, "Failed to register clock\n");
> +				dev_err_probe(int3472->dev, ret,
> +					      "Failed to register clock\n");

Don't know why changed linesplits in this instead doing them directly in 
the 2nd patch.

>  
>  			break;
>  		case INT3472_GPIO_TYPE_PRIVACY_LED:
>  		case INT3472_GPIO_TYPE_STROBE:
>  			ret = skl_int3472_register_led(int3472, gpio, con_id);
>  			if (ret)
> -				dev_err_probe(int3472->dev, ret, "Failed to register LED\n");
> +				dev_err_probe(int3472->dev, ret,
> +					      "Failed to register LED\n");
>  
>  			break;
>  		case INT3472_GPIO_TYPE_POWER_ENABLE:
> @@ -413,7 +421,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  			ret = skl_int3472_register_regulator(int3472, gpio, enable_time_us,
>  							     con_id, second_sensor);
>  			if (ret)
> -				dev_err_probe(int3472->dev, ret, "Failed to register regulator\n");
> +				dev_err_probe(int3472->dev, ret,
> +					      "Failed to register regulator\n");
>  
>  			break;
>  		default: /* Never reached */
> @@ -424,22 +433,13 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  		if (ret)
>  			gpiod_put(gpio);
>  
> -		break;
> +		return ret;
>  	default:
>  		dev_warn(int3472->dev,
>  			 "GPIO type 0x%02x unknown; the sensor may not work\n",
>  			 type);
> -		ret = 1;
> -		break;
> +		return 1;
>  	}
> -
> -	int3472->ngpios++;
> -
> -	/*
> -	 * Either return an error or tell acpi_dev_get_resources() to not make a
> -	 * copy of the resource.
> -	 */
> -	return ret < 0 ? ret : 1;
>  }
>  
>  int int3472_discrete_parse_crs(struct int3472_discrete_device *int3472)
> 

-- 
 i.

Re: [PATCH v2 5/5] platform/x86: int3472: Clean up GPIO parsing

[Sakari Ailus]

Moi,

On Tue, Aug 25, 2026 at 01:40:25PM +0300, Ilpo Järvinen wrote:
> On Tue, 25 Aug 2026, Sakari Ailus wrote:
> 
> > In skl_int3472_handle_gpio_resources(), return an error where it happens,
> > except when we're holding a reference to a GPIO. This involves
> > incrementing ngpios earlier on but that does not introduce a functional
> > change.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/platform/x86/intel/int3472/discrete.c | 42 +++++++++----------
> >  1 file changed, 21 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> > index 2024eaa09033..400bee9e4cb8 100644
> > --- a/drivers/platform/x86/intel/int3472/discrete.c
> > +++ b/drivers/platform/x86/intel/int3472/discrete.c
> > @@ -369,15 +369,23 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
> >  		agpio->resource_source.string_ptr, agpio->pin_table[0],
> >  		str_high_low(gpio_flags == GPIO_ACTIVE_HIGH));
> >  
> > +	/*
> > +	 * int3472->ngpios can be incremented here as it is an argument to the
> > +	 * _DSM, not e.g. an index to an array in C. Additionally, in case of an
> > +	 * error the value won't be used.
> > +	 */
> 
> I wasn't expecting comment for this. You might have added it because of 
> my questions but IMO it's not needed.
> 
> I think just the old way/place was confusing, especially given how 
> some/part of the error cases did increment it. But after moving it here, 
> it makes much more sense even without that comment.
> 
> And one can determine it's not being used for C arrays by using grep so 
> that too seems just unnecessary detail.

Ok, I'll drop it.

> 
> > +	int3472->ngpios++;
> > +
> >  	switch (type) {
> >  	case INT3472_GPIO_TYPE_RESET:
> >  	case INT3472_GPIO_TYPE_POWERDOWN:
> >  	case INT3472_GPIO_TYPE_HOTPLUG_DETECT:
> >  		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, con_id, gpio_flags);
> >  		if (ret)
> > -			dev_err_probe(int3472->dev, ret, "Failed to map GPIO pin to sensor\n");
> > +			return dev_err_probe(int3472->dev, ret,
> > +					     "Failed to map GPIO pin to sensor\n");
> >  
> > -		break;
> > +		return 0;
> >  	case INT3472_GPIO_TYPE_CLK_ENABLE:
> >  	case INT3472_GPIO_TYPE_PRIVACY_LED:
> >  	case INT3472_GPIO_TYPE_STROBE:
> > @@ -385,24 +393,24 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
> >  	case INT3472_GPIO_TYPE_DOVDD:
> >  	case INT3472_GPIO_TYPE_HANDSHAKE:
> >  		gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, con_id, gpio_flags);
> > -		if (IS_ERR(gpio)) {
> > -			ret = PTR_ERR(gpio);
> > -			dev_err_probe(int3472->dev, ret, "Failed to get GPIO\n");
> > -			break;
> > -		}
> > +		if (IS_ERR(gpio))
> > +			return dev_err_probe(int3472->dev, PTR_ERR(gpio),
> > +					     "Failed to get GPIO\n");
> >  
> >  		switch (type) {
> >  		case INT3472_GPIO_TYPE_CLK_ENABLE:
> >  			ret = skl_int3472_register_gpio_clock(int3472, gpio);
> >  			if (ret)
> > -				dev_err_probe(int3472->dev, ret, "Failed to register clock\n");
> > +				dev_err_probe(int3472->dev, ret,
> > +					      "Failed to register clock\n");
> 
> Don't know why changed linesplits in this instead doing them directly in 
> the 2nd patch.

Oops, that's where I intended to make them. I'll fix that for v3.

-- 
Sakari Ailus