| Message ID | 20220301124908.1931221-2-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/3] ACPI: APEI: Adjust for acpi_run_osc logic changes | expand |
On Tue, Mar 01, 2022 at 06:49:07AM -0600, Mario Limonciello wrote: > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > index 07f604832fd6..302619ad9d31 100644 > --- a/drivers/acpi/bus.c > +++ b/drivers/acpi/bus.c > @@ -184,6 +184,18 @@ static void acpi_print_osc_error(acpi_handle handle, > pr_debug("\n"); > } > > +/** > + * acpi_run_osc - Evaluate the _OSC method for a given ACPI handle > + * @handle: ACPI handle containing _OSC to evaluate > + * @context: A structure specifying UUID, revision, and buffer for data > + * > + * Used for negotiating with firmware the capabilities that will be used > + * by the OSPM. Although the return type is acpi_status, the ACPI_SUCCESS > + * macro can not be used to determine whether to free the buffer of > + * returned data. > + * > + * The buffer must be freed when this function returns AE_OK or AE_SUPPORT. > + */ > acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context) > { > acpi_status status; > @@ -243,16 +255,19 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context) > acpi_print_osc_error(handle, context, > "_OSC invalid revision"); > if (errors & OSC_CAPABILITIES_MASK_ERROR) { > + acpi_print_osc_error(handle, context, "_OSC capabilities masked"); > if (((u32 *)context->cap.pointer)[OSC_QUERY_DWORD] > - & OSC_QUERY_ENABLE) > - goto out_success; > - status = AE_SUPPORT; > - goto out_kfree; > + & OSC_QUERY_ENABLE) { > + status = AE_SUPPORT; > + goto out_masked; > + } > } > status = AE_ERROR; > goto out_kfree; > } > -out_success: > + > + status = AE_OK; > +out_masked: > context->ret.length = out_obj->buffer.length; > context->ret.pointer = kmemdup(out_obj->buffer.pointer, > context->ret.length, GFP_KERNEL); > @@ -260,7 +275,6 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context) > status = AE_NO_MEMORY; > goto out_kfree; > } > - status = AE_OK; > > out_kfree: > kfree(output.pointer); Do we forget to free "context.ret.pointer" in acpi_pci_run_osc() as well when acpi_run_osc() starts to return AE_SUPPORT? I saw memory leaks on the latest linux-next which include this series. # cat /sys/kernel/debug/kmemleak unreferenced object 0xffff07ff968c6e80 (size 128): comm "swapper/0", pid 1, jiffies 4294894996 (age 1785.652s) hex dump (first 32 bytes): 11 00 00 00 1f 01 00 00 18 00 00 00 6b 6b 6b 6b ............kkkk 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk backtrace: [<ffffad5bd9d79adc>] __kmalloc_track_caller [<ffffad5bd9c439f4>] kmemdup [<ffffad5bda97c710>] acpi_run_osc acpi_run_osc at drivers/acpi/bus.c:272 [<ffffad5bdc52f8a8>] acpi_pci_run_osc acpi_pci_run_osc at drivers/acpi/pci_root.c:184 [<ffffad5bdc52fe30>] negotiate_os_control.constprop.0 acpi_pci_query_osc at drivers/acpi/pci_root.c:205 (inlined by) acpi_pci_osc_control_set at drivers/acpi/pci_root.c:353 (inlined by) negotiate_os_control at drivers/acpi/pci_root.c:482 [<ffffad5bda995748>] acpi_pci_root_add [<ffffad5bda982558>] acpi_bus_attach [<ffffad5bda9823c8>] acpi_bus_attach [<ffffad5bda9823c8>] acpi_bus_attach [<ffffad5bda98808c>] acpi_bus_scan [<ffffad5bdd9624a0>] acpi_scan_init [<ffffad5bdd961e74>] acpi_init [<ffffad5bd9595438>] do_one_initcall [<ffffad5bdd90201c>] kernel_init_freeable [<ffffad5bdc5684e0>] kernel_init [<ffffad5bd959841c>] ret_from_fork
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 07f604832fd6..302619ad9d31 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -184,6 +184,18 @@ static void acpi_print_osc_error(acpi_handle handle, pr_debug("\n"); } +/** + * acpi_run_osc - Evaluate the _OSC method for a given ACPI handle + * @handle: ACPI handle containing _OSC to evaluate + * @context: A structure specifying UUID, revision, and buffer for data + * + * Used for negotiating with firmware the capabilities that will be used + * by the OSPM. Although the return type is acpi_status, the ACPI_SUCCESS + * macro can not be used to determine whether to free the buffer of + * returned data. + * + * The buffer must be freed when this function returns AE_OK or AE_SUPPORT. + */ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context) { acpi_status status; @@ -243,16 +255,19 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context) acpi_print_osc_error(handle, context, "_OSC invalid revision"); if (errors & OSC_CAPABILITIES_MASK_ERROR) { + acpi_print_osc_error(handle, context, "_OSC capabilities masked"); if (((u32 *)context->cap.pointer)[OSC_QUERY_DWORD] - & OSC_QUERY_ENABLE) - goto out_success; - status = AE_SUPPORT; - goto out_kfree; + & OSC_QUERY_ENABLE) { + status = AE_SUPPORT; + goto out_masked; + } } status = AE_ERROR; goto out_kfree; } -out_success: + + status = AE_OK; +out_masked: context->ret.length = out_obj->buffer.length; context->ret.pointer = kmemdup(out_obj->buffer.pointer, context->ret.length, GFP_KERNEL); @@ -260,7 +275,6 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context) status = AE_NO_MEMORY; goto out_kfree; } - status = AE_OK; out_kfree: kfree(output.pointer);
Currently when capabilities have been masked by firmware during a negotiation with OSC_QUERY_ENABLE set they're silently ignored by the caller. If the caller calls `acpi_run_osc` again without query set and the same capabilities, then they instead get a failure possibly leading to downstream problems. So instead when query is set return AE_SUPPORT which callers can then use for determining that capabilities were masked. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v3->v4: * Correct kernel doc for AE_OK/AE_SUPPORT not AE_OK/AE_SUCCESS drivers/acpi/bus.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)