[PATCH v1] ACPI: bus: Adjust acpi_osc_handshake() parameter list

[PATCH v1] ACPI: bus: Adjust acpi_osc_handshake() parameter list

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

For the sake of interface cleanliness, it is better to avoid using
ACPICA data types in the parameter lists of helper functions that
don't belong to ACPICA, so adjust the parameter list of recently
introduced acpi_osc_handshake() to take a capabilities buffer pointer
and the size of the buffer (in u32 size units) as parameters directly
instead of a struct acpi_buffer pointer.

This is also somewhat more straightforward on the caller side because
they won't need to create struct acpi_buffer objects themselves to pass
them to the helper function and it guarantees that the size of the
buffer in bytes will always be a multiple of 4 (the size of u32).

Moreover, it addresses a premature cap pointer dereference and
eliminates a sizeof(32) that should have been sizeof(u32) [1].

Fixes: e5322888e6bf ("ACPI: bus: Rework the handling of \_SB._OSC platform features")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/linux-acpi/202512242052.W4GhDauV-lkp@intel.com/
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c |   30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -326,31 +326,33 @@ out:
 EXPORT_SYMBOL(acpi_run_osc);
 
 static int acpi_osc_handshake(acpi_handle handle, const char *uuid_str,
-			      int rev, struct acpi_buffer *cap)
+			      int rev, u32 *capbuf, size_t bufsize)
 {
 	union acpi_object in_params[4], *out_obj;
-	size_t bufsize = cap->length / sizeof(u32);
 	struct acpi_object_list input;
+	struct acpi_buffer cap = {
+		.pointer = capbuf,
+		.length = bufsize * sizeof(32),
+	};
 	struct acpi_buffer output;
-	u32 *capbuf, *retbuf, test;
+	u32 *retbuf, test;
 	guid_t guid;
 	int ret, i;
 
-	if (!cap || cap->length < 2 * sizeof(32) || guid_parse(uuid_str, &guid))
+	if (!capbuf || bufsize < 2 || guid_parse(uuid_str, &guid))
 		return -EINVAL;
 
 	/* First evaluate _OSC with OSC_QUERY_ENABLE set. */
-	capbuf = cap->pointer;
 	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
 
-	ret = acpi_eval_osc(handle, &guid, rev, cap, in_params, &output);
+	ret = acpi_eval_osc(handle, &guid, rev, &cap, in_params, &output);
 	if (ret)
 		return ret;
 
 	out_obj = output.pointer;
 	retbuf = (u32 *)out_obj->buffer.pointer;
 
-	if (acpi_osc_error_check(handle, &guid, rev, cap, retbuf)) {
+	if (acpi_osc_error_check(handle, &guid, rev, &cap, retbuf)) {
 		ret = -ENODATA;
 		goto out;
 	}
@@ -403,7 +405,7 @@ static int acpi_osc_handshake(acpi_handl
 		 */
 		acpi_handle_err(handle, "_OSC: errors while processing control request\n");
 		acpi_handle_err(handle, "_OSC: some features may be missing\n");
-		acpi_osc_error_check(handle, &guid, rev, cap, retbuf);
+		acpi_osc_error_check(handle, &guid, rev, &cap, retbuf);
 	}
 
 out:
@@ -446,10 +448,6 @@ static void acpi_bus_osc_negotiate_platf
 {
 	static const u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
 	u32 capbuf[2], feature_mask;
-	struct acpi_buffer cap = {
-		.pointer = capbuf,
-		.length = sizeof(capbuf),
-	};
 	acpi_handle handle;
 
 	feature_mask = OSC_SB_PR3_SUPPORT | OSC_SB_HOTPLUG_OST_SUPPORT |
@@ -497,7 +495,7 @@ static void acpi_bus_osc_negotiate_platf
 
 	acpi_handle_info(handle, "platform _OSC: OS support mask [%08x]\n", feature_mask);
 
-	if (acpi_osc_handshake(handle, sb_uuid_str, 1, &cap))
+	if (acpi_osc_handshake(handle, sb_uuid_str, 1, capbuf, 2))
 		return;
 
 	feature_mask = capbuf[OSC_SUPPORT_DWORD];
@@ -532,10 +530,6 @@ static void acpi_bus_osc_negotiate_usb_c
 {
 	static const u8 sb_usb_uuid_str[] = "23A0D13A-26AB-486C-9C5F-0FFA525A575A";
 	u32 capbuf[3], control;
-	struct acpi_buffer cap = {
-		.pointer = capbuf,
-		.length = sizeof(capbuf),
-	};
 	acpi_handle handle;
 
 	if (!osc_sb_native_usb4_support_confirmed)
@@ -550,7 +544,7 @@ static void acpi_bus_osc_negotiate_usb_c
 	capbuf[OSC_SUPPORT_DWORD] = 0;
 	capbuf[OSC_CONTROL_DWORD] = control;
 
-	if (acpi_osc_handshake(handle, sb_usb_uuid_str, 1, &cap))
+	if (acpi_osc_handshake(handle, sb_usb_uuid_str, 1, capbuf, 3))
 		return;
 
 	osc_sb_native_usb4_control = capbuf[OSC_CONTROL_DWORD];

Re: [PATCH v1] ACPI: bus: Adjust acpi_osc_handshake() parameter list

[Jonathan Cameron]

On Fri, 26 Dec 2025 14:48:45 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> For the sake of interface cleanliness, it is better to avoid using
> ACPICA data types in the parameter lists of helper functions that
> don't belong to ACPICA, so adjust the parameter list of recently
> introduced acpi_osc_handshake() to take a capabilities buffer pointer
> and the size of the buffer (in u32 size units) as parameters directly
> instead of a struct acpi_buffer pointer.
> 
> This is also somewhat more straightforward on the caller side because
> they won't need to create struct acpi_buffer objects themselves to pass
> them to the helper function and it guarantees that the size of the
> buffer in bytes will always be a multiple of 4 (the size of u32).
> 
> Moreover, it addresses a premature cap pointer dereference and
> eliminates a sizeof(32) that should have been sizeof(u32) [1].
> 
> Fixes: e5322888e6bf ("ACPI: bus: Rework the handling of \_SB._OSC platform features")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/linux-acpi/202512242052.W4GhDauV-lkp@intel.com/
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
A couple of minor comments inline.  I see you have it queued up already, but
FWIW nothing here major enough to warrant reverting that.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  drivers/acpi/bus.c |   30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
> 
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -326,31 +326,33 @@ out:
>  EXPORT_SYMBOL(acpi_run_osc);
>  
>  static int acpi_osc_handshake(acpi_handle handle, const char *uuid_str,
> -			      int rev, struct acpi_buffer *cap)
> +			      int rev, u32 *capbuf, size_t bufsize)

Size parameters in number of u32s is to me a little confusing but
I guess this is only used locally so that's probably fine.
I'd have been tempted to call it dwords or something like that.


>  {
>  	union acpi_object in_params[4], *out_obj;
> -	size_t bufsize = cap->length / sizeof(u32);
>  	struct acpi_object_list input;
> +	struct acpi_buffer cap = {
> +		.pointer = capbuf,
> +		.length = bufsize * sizeof(32),

You fixed this up already but just for completeness.
sizeof(u32)

> +	};
>  	struct acpi_buffer output;
> -	u32 *capbuf, *retbuf, test;
> +	u32 *retbuf, test;
>  	guid_t guid;
>  	int ret, i;
>  
> -	if (!cap || cap->length < 2 * sizeof(32) || guid_parse(uuid_str, &guid))
> +	if (!capbuf || bufsize < 2 || guid_parse(uuid_str, &guid))
>  		return -EINVAL;
>  
>  	/* First evaluate _OSC with OSC_QUERY_ENABLE set. */
> -	capbuf = cap->pointer;
>  	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
>  
> -	ret = acpi_eval_osc(handle, &guid, rev, cap, in_params, &output);
> +	ret = acpi_eval_osc(handle, &guid, rev, &cap, in_params, &output);
>  	if (ret)
>  		return ret;
>  
>  	out_obj = output.pointer;
>  	retbuf = (u32 *)out_obj->buffer.pointer;
>  
> -	if (acpi_osc_error_check(handle, &guid, rev, cap, retbuf)) {
> +	if (acpi_osc_error_check(handle, &guid, rev, &cap, retbuf)) {
>  		ret = -ENODATA;
>  		goto out;
>  	}
> @@ -403,7 +405,7 @@ static int acpi_osc_handshake(acpi_handl
>  		 */
>  		acpi_handle_err(handle, "_OSC: errors while processing control request\n");
>  		acpi_handle_err(handle, "_OSC: some features may be missing\n");
> -		acpi_osc_error_check(handle, &guid, rev, cap, retbuf);
> +		acpi_osc_error_check(handle, &guid, rev, &cap, retbuf);
>  	}
>  
>  out:
> @@ -446,10 +448,6 @@ static void acpi_bus_osc_negotiate_platf
>  {
>  	static const u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
>  	u32 capbuf[2], feature_mask;
> -	struct acpi_buffer cap = {
> -		.pointer = capbuf,
> -		.length = sizeof(capbuf),
> -	};
>  	acpi_handle handle;
>  
>  	feature_mask = OSC_SB_PR3_SUPPORT | OSC_SB_HOTPLUG_OST_SUPPORT |
> @@ -497,7 +495,7 @@ static void acpi_bus_osc_negotiate_platf
>  
>  	acpi_handle_info(handle, "platform _OSC: OS support mask [%08x]\n", feature_mask);
>  
> -	if (acpi_osc_handshake(handle, sb_uuid_str, 1, &cap))
> +	if (acpi_osc_handshake(handle, sb_uuid_str, 1, capbuf, 2))

As below. Maybe ARRAY_SIZE(capbuf) instead of that 2.

>  		return;
>  
>  	feature_mask = capbuf[OSC_SUPPORT_DWORD];
> @@ -532,10 +530,6 @@ static void acpi_bus_osc_negotiate_usb_c
>  {
>  	static const u8 sb_usb_uuid_str[] = "23A0D13A-26AB-486C-9C5F-0FFA525A575A";
>  	u32 capbuf[3], control;
> -	struct acpi_buffer cap = {
> -		.pointer = capbuf,
> -		.length = sizeof(capbuf),
> -	};
>  	acpi_handle handle;
>  
>  	if (!osc_sb_native_usb4_support_confirmed)
> @@ -550,7 +544,7 @@ static void acpi_bus_osc_negotiate_usb_c
>  	capbuf[OSC_SUPPORT_DWORD] = 0;
>  	capbuf[OSC_CONTROL_DWORD] = control;
>  
> -	if (acpi_osc_handshake(handle, sb_usb_uuid_str, 1, &cap))
> +	if (acpi_osc_handshake(handle, sb_usb_uuid_str, 1, capbuf, 3))

Maybe ARRAY_SIZE(capbuf) just to avoid any chance they get out of sync?

>  		return;
>  
>  	osc_sb_native_usb4_control = capbuf[OSC_CONTROL_DWORD];
> 
> 
> 
> 

Re: [PATCH v1] ACPI: bus: Adjust acpi_osc_handshake() parameter list

[Rafael J. Wysocki]

On Fri, Jan 2, 2026 at 12:53 PM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Fri, 26 Dec 2025 14:48:45 +0100
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > For the sake of interface cleanliness, it is better to avoid using
> > ACPICA data types in the parameter lists of helper functions that
> > don't belong to ACPICA, so adjust the parameter list of recently
> > introduced acpi_osc_handshake() to take a capabilities buffer pointer
> > and the size of the buffer (in u32 size units) as parameters directly
> > instead of a struct acpi_buffer pointer.
> >
> > This is also somewhat more straightforward on the caller side because
> > they won't need to create struct acpi_buffer objects themselves to pass
> > them to the helper function and it guarantees that the size of the
> > buffer in bytes will always be a multiple of 4 (the size of u32).
> >
> > Moreover, it addresses a premature cap pointer dereference and
> > eliminates a sizeof(32) that should have been sizeof(u32) [1].
> >
> > Fixes: e5322888e6bf ("ACPI: bus: Rework the handling of \_SB._OSC platform features")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/linux-acpi/202512242052.W4GhDauV-lkp@intel.com/
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> A couple of minor comments inline.  I see you have it queued up already,

Yeah, mostly to address build warnings in linux-next.

> but FWIW nothing here major enough to warrant reverting that.
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>
> > ---
> >  drivers/acpi/bus.c |   30 ++++++++++++------------------
> >  1 file changed, 12 insertions(+), 18 deletions(-)
> >
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -326,31 +326,33 @@ out:
> >  EXPORT_SYMBOL(acpi_run_osc);
> >
> >  static int acpi_osc_handshake(acpi_handle handle, const char *uuid_str,
> > -                           int rev, struct acpi_buffer *cap)
> > +                           int rev, u32 *capbuf, size_t bufsize)
>
> Size parameters in number of u32s is to me a little confusing but
> I guess this is only used locally so that's probably fine.
> I'd have been tempted to call it dwords or something like that.

Well, this technically is the number of elements in the capbuf[]
array, so I guess it could be called capbuf_size or something like
that.

> >  {
> >       union acpi_object in_params[4], *out_obj;
> > -     size_t bufsize = cap->length / sizeof(u32);
> >       struct acpi_object_list input;
> > +     struct acpi_buffer cap = {
> > +             .pointer = capbuf,
> > +             .length = bufsize * sizeof(32),
>
> You fixed this up already but just for completeness.
> sizeof(u32)

Yup.

> > +     };
> >       struct acpi_buffer output;
> > -     u32 *capbuf, *retbuf, test;
> > +     u32 *retbuf, test;
> >       guid_t guid;
> >       int ret, i;
> >
> > -     if (!cap || cap->length < 2 * sizeof(32) || guid_parse(uuid_str, &guid))
> > +     if (!capbuf || bufsize < 2 || guid_parse(uuid_str, &guid))
> >               return -EINVAL;
> >
> >       /* First evaluate _OSC with OSC_QUERY_ENABLE set. */
> > -     capbuf = cap->pointer;
> >       capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
> >
> > -     ret = acpi_eval_osc(handle, &guid, rev, cap, in_params, &output);
> > +     ret = acpi_eval_osc(handle, &guid, rev, &cap, in_params, &output);
> >       if (ret)
> >               return ret;
> >
> >       out_obj = output.pointer;
> >       retbuf = (u32 *)out_obj->buffer.pointer;
> >
> > -     if (acpi_osc_error_check(handle, &guid, rev, cap, retbuf)) {
> > +     if (acpi_osc_error_check(handle, &guid, rev, &cap, retbuf)) {
> >               ret = -ENODATA;
> >               goto out;
> >       }
> > @@ -403,7 +405,7 @@ static int acpi_osc_handshake(acpi_handl
> >                */
> >               acpi_handle_err(handle, "_OSC: errors while processing control request\n");
> >               acpi_handle_err(handle, "_OSC: some features may be missing\n");
> > -             acpi_osc_error_check(handle, &guid, rev, cap, retbuf);
> > +             acpi_osc_error_check(handle, &guid, rev, &cap, retbuf);
> >       }
> >
> >  out:
> > @@ -446,10 +448,6 @@ static void acpi_bus_osc_negotiate_platf
> >  {
> >       static const u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
> >       u32 capbuf[2], feature_mask;
> > -     struct acpi_buffer cap = {
> > -             .pointer = capbuf,
> > -             .length = sizeof(capbuf),
> > -     };
> >       acpi_handle handle;
> >
> >       feature_mask = OSC_SB_PR3_SUPPORT | OSC_SB_HOTPLUG_OST_SUPPORT |
> > @@ -497,7 +495,7 @@ static void acpi_bus_osc_negotiate_platf
> >
> >       acpi_handle_info(handle, "platform _OSC: OS support mask [%08x]\n", feature_mask);
> >
> > -     if (acpi_osc_handshake(handle, sb_uuid_str, 1, &cap))
> > +     if (acpi_osc_handshake(handle, sb_uuid_str, 1, capbuf, 2))
>
> As below. Maybe ARRAY_SIZE(capbuf) instead of that 2.

That's a good idea, I'll change it in the patch.

> >               return;
> >
> >       feature_mask = capbuf[OSC_SUPPORT_DWORD];
> > @@ -532,10 +530,6 @@ static void acpi_bus_osc_negotiate_usb_c
> >  {
> >       static const u8 sb_usb_uuid_str[] = "23A0D13A-26AB-486C-9C5F-0FFA525A575A";
> >       u32 capbuf[3], control;
> > -     struct acpi_buffer cap = {
> > -             .pointer = capbuf,
> > -             .length = sizeof(capbuf),
> > -     };
> >       acpi_handle handle;
> >
> >       if (!osc_sb_native_usb4_support_confirmed)
> > @@ -550,7 +544,7 @@ static void acpi_bus_osc_negotiate_usb_c
> >       capbuf[OSC_SUPPORT_DWORD] = 0;
> >       capbuf[OSC_CONTROL_DWORD] = control;
> >
> > -     if (acpi_osc_handshake(handle, sb_usb_uuid_str, 1, &cap))
> > +     if (acpi_osc_handshake(handle, sb_usb_uuid_str, 1, capbuf, 3))
>
> Maybe ARRAY_SIZE(capbuf) just to avoid any chance they get out of sync?
>
> >               return;
> >
> >       osc_sb_native_usb4_control = capbuf[OSC_CONTROL_DWORD];
> >
> >
> >
> >

Thanks!