[PATCH 0/3] thermal: int340x: Misc acpi_buffer handling updates

[PATCH 0/3] thermal: int340x: Misc acpi_buffer handling updates

[Davidlohr Bueso]

Hello,

The following is a fallout of eyeballing _OSC handling in the driver.
These changes have only been compile-tested. Patch 1 is a fix and the
other two are cleanups.

Thanks!

Davidlohr Bueso (3):
  thermal: int340x: Fix bogus acpi_buffer pointer freeing
  thermal: int340x: Consolidate freeing of acpi_buffer pointer
  thermal: int340x: Cleanup osc context init

 .../intel/int340x_thermal/int3400_thermal.c   | 24 +++++++------------
 1 file changed, 9 insertions(+), 15 deletions(-)

--
2.26.2

[PATCH 1/3] thermal: int340x: Fix bogus acpi_buffer pointer freeing

[Davidlohr Bueso]

It is the caller's responsibility to free only upon ACPI_SUCCESS.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/thermal/intel/int340x_thermal/int3400_thermal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
index 4954800b9850..0e7931c286ec 100644
--- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
+++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
@@ -186,11 +186,11 @@ static int int3400_thermal_run_osc(acpi_handle handle, char *uuid_str, int *enab
 		ret = *((u32 *)(context.ret.pointer + 4));
 		if (ret != *enable)
 			result = -EPERM;
+
+		kfree(context.ret.pointer);
 	} else
 		result = -EPERM;
 
-	kfree(context.ret.pointer);
-
 	return result;
 }
 
-- 
2.26.2

Re: [PATCH 1/3] thermal: int340x: Fix bogus acpi_buffer pointer freeing

[srinivas pandruvada]

On Thu, 2022-03-24 at 12:09 -0700, Davidlohr Bueso wrote:
> It is the caller's responsibility to free only upon ACPI_SUCCESS.
> 

Since context->ret.pointer will be NULL on failure so calling kfree
will just return.

Although we can avoid this call to kfree.

Thanks,
Srinivas

> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/thermal/intel/int340x_thermal/int3400_thermal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> index 4954800b9850..0e7931c286ec 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> @@ -186,11 +186,11 @@ static int int3400_thermal_run_osc(acpi_handle
> handle, char *uuid_str, int *enab
>                 ret = *((u32 *)(context.ret.pointer + 4));
>                 if (ret != *enable)
>                         result = -EPERM;
> +
> +               kfree(context.ret.pointer);
>         } else
>                 result = -EPERM;
>  
> -       kfree(context.ret.pointer);
> -
>         return result;
>  }
>  

Re: [PATCH 1/3] thermal: int340x: Fix bogus acpi_buffer pointer freeing

[Rafael J. Wysocki]

On Thu, Mar 24, 2022 at 8:10 PM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> It is the caller's responsibility to free only upon ACPI_SUCCESS.
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/thermal/intel/int340x_thermal/int3400_thermal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> index 4954800b9850..0e7931c286ec 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> @@ -186,11 +186,11 @@ static int int3400_thermal_run_osc(acpi_handle handle, char *uuid_str, int *enab
>                 ret = *((u32 *)(context.ret.pointer + 4));
>                 if (ret != *enable)
>                         result = -EPERM;
> +
> +               kfree(context.ret.pointer);
>         } else
>                 result = -EPERM;
>
> -       kfree(context.ret.pointer);
> -
>         return result;
>  }
>
> --

Because the code before the change is not incorrect, this is a cleanup
rather than a fix, so I've adjusted the subject a bit and applied this
along with the [2-3/3] as 5.19 material.

Thanks!

[PATCH 2/3] thermal: int340x: Consolidate freeing of acpi_buffer pointer

[Davidlohr Bueso]

Introduce a single point of freeing/exit after ensuring no error in
int3400_setup_gddv().

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 .../thermal/intel/int340x_thermal/int3400_thermal.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
index 0e7931c286ec..e299873d50b8 100644
--- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
+++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
@@ -508,21 +508,18 @@ static void int3400_setup_gddv(struct int3400_thermal_priv *priv)
 
 	obj = buffer.pointer;
 	if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 1
-	    || obj->package.elements[0].type != ACPI_TYPE_BUFFER) {
-		kfree(buffer.pointer);
-		return;
-	}
+	    || obj->package.elements[0].type != ACPI_TYPE_BUFFER)
+		goto out_free;
 
 	priv->data_vault = kmemdup(obj->package.elements[0].buffer.pointer,
 				   obj->package.elements[0].buffer.length,
 				   GFP_KERNEL);
-	if (!priv->data_vault) {
-		kfree(buffer.pointer);
-		return;
-	}
+	if (!priv->data_vault)
+		goto out_free;
 
 	bin_attr_data_vault.private = priv->data_vault;
 	bin_attr_data_vault.size = obj->package.elements[0].buffer.length;
+out_free:
 	kfree(buffer.pointer);
 }
 
-- 
2.26.2

[PATCH 3/3] thermal: int340x: Cleanup osc context init

[Davidlohr Bueso]

Now that the UUID is already sanitized by the caller,
lets trivially cleanup some of the context arming.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/thermal/intel/int340x_thermal/int3400_thermal.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
index e299873d50b8..4c49a6c9d4df 100644
--- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
+++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
@@ -169,18 +169,15 @@ static int int3400_thermal_run_osc(acpi_handle handle, char *uuid_str, int *enab
 	acpi_status status;
 	int result = 0;
 	struct acpi_osc_context context = {
-		.uuid_str = NULL,
+		.uuid_str = uuid_str,
 		.rev = 1,
 		.cap.length = 8,
+		.cap.pointer = buf,
 	};
 
-	context.uuid_str = uuid_str;
-
 	buf[OSC_QUERY_DWORD] = 0;
 	buf[OSC_SUPPORT_DWORD] = *enable;
 
-	context.cap.pointer = buf;
-
 	status = acpi_run_osc(handle, &context);
 	if (ACPI_SUCCESS(status)) {
 		ret = *((u32 *)(context.ret.pointer + 4));
-- 
2.26.2

Re: [PATCH 0/3] thermal: int340x: Misc acpi_buffer handling updates

[Zhang Rui]

Acked-by: Zhang Rui <rui.zhang@intel.com>

for the whole patch series.

thanks,
rui

On Thu, 2022-03-24 at 12:09 -0700, Davidlohr Bueso wrote:
> Hello,
> 
> The following is a fallout of eyeballing _OSC handling in the driver.
> These changes have only been compile-tested. Patch 1 is a fix and the
> other two are cleanups.
> 
> Thanks!
> 
> Davidlohr Bueso (3):
>   thermal: int340x: Fix bogus acpi_buffer pointer freeing
>   thermal: int340x: Consolidate freeing of acpi_buffer pointer
>   thermal: int340x: Cleanup osc context init
> 
>  .../intel/int340x_thermal/int3400_thermal.c   | 24 +++++++--------
> ----
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> --
> 2.26.2
>