| Message ID | 20220308224200.2691564-1-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI/ACPI: Add support for `AE_SUPPORT` in _OSC queries | expand |
On Tue, Mar 8, 2022 at 11:44 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > commit a412caea5a2d ("ACPI: bus: Allow negotiating _OSC capabilities") > added support for `acpi_run_osc` to return `AE_SUPPORT` when negotiating > an _OSC. > > This was fixed in other kernel consumers, but `acpi_pci_run_osc` was > missed. Update the function to detect when called with `OSC_QUERY_ENABLE` > set and attempt to negotiate up to 5 times. This is not how it is designed to work, though. acpi_pci_query_osc() is for that. > > Reported-by: Qian Cai <quic_qiancai@quicinc.com> > Fixes: a412caea5a2d ("ACPI: bus: Allow negotiating _OSC capabilities") So I'm seriously thinking about dropping that whole lot at this point. > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/acpi/pci_root.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 6f9e75d14808..2eda355fde57 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -171,7 +171,7 @@ static void decode_osc_control(struct acpi_pci_root *root, char *msg, u32 word) > static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766"; > > static acpi_status acpi_pci_run_osc(acpi_handle handle, > - const u32 *capbuf, u32 *retval) > + u32 *capbuf, u32 *retval) > { > struct acpi_osc_context context = { > .uuid_str = pci_osc_uuid_str, > @@ -180,7 +180,26 @@ static acpi_status acpi_pci_run_osc(acpi_handle handle, > .cap.pointer = (void *)capbuf, > }; > acpi_status status; > + u32 *capbuf_ret; > + int i; > + > + if (!(capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE)) > + goto skip_negotiate; > + for (i = 0; i < 5; i++) { > + status = acpi_run_osc(handle, &context); > + if (status == AE_OK || status == AE_SUPPORT) { > + capbuf_ret = context.ret.pointer; > + capbuf[OSC_SUPPORT_DWORD] = capbuf_ret[OSC_SUPPORT_DWORD]; > + kfree(context.ret.pointer); > + } > + if (status != AE_SUPPORT) > + break; > + } > + if (ACPI_FAILURE(status)) > + return status; > > +skip_negotiate: > + capbuf[OSC_QUERY_DWORD] = 0; > status = acpi_run_osc(handle, &context); > if (ACPI_SUCCESS(status)) { > *retval = *((u32 *)(context.ret.pointer + 8)); > -- > 2.34.1 >
[AMD Official Use Only] > > > > commit a412caea5a2d ("ACPI: bus: Allow negotiating _OSC capabilities") > > added support for `acpi_run_osc` to return `AE_SUPPORT` when negotiating > > an _OSC. > > > > This was fixed in other kernel consumers, but `acpi_pci_run_osc` was > > missed. Update the function to detect when called with > `OSC_QUERY_ENABLE` > > set and attempt to negotiate up to 5 times. > > This is not how it is designed to work, though. > > acpi_pci_query_osc() is for that. > > > > > Reported-by: Qian Cai <quic_qiancai@quicinc.com> > > Fixes: a412caea5a2d ("ACPI: bus: Allow negotiating _OSC capabilities") > > So I'm seriously thinking about dropping that whole lot at this point. Do you want me to send up a series perhaps that reverts those 3 commits and instead moves the logic changes on query handling from acpi_run_osc into acpi_bus_osc_negotiate_platform_control? > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > --- > > drivers/acpi/pci_root.c | 21 ++++++++++++++++++++- > > 1 file changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > > index 6f9e75d14808..2eda355fde57 100644 > > --- a/drivers/acpi/pci_root.c > > +++ b/drivers/acpi/pci_root.c > > @@ -171,7 +171,7 @@ static void decode_osc_control(struct acpi_pci_root > *root, char *msg, u32 word) > > static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766"; > > > > static acpi_status acpi_pci_run_osc(acpi_handle handle, > > - const u32 *capbuf, u32 *retval) > > + u32 *capbuf, u32 *retval) > > { > > struct acpi_osc_context context = { > > .uuid_str = pci_osc_uuid_str, > > @@ -180,7 +180,26 @@ static acpi_status acpi_pci_run_osc(acpi_handle > handle, > > .cap.pointer = (void *)capbuf, > > }; > > acpi_status status; > > + u32 *capbuf_ret; > > + int i; > > + > > + if (!(capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE)) > > + goto skip_negotiate; > > + for (i = 0; i < 5; i++) { > > + status = acpi_run_osc(handle, &context); > > + if (status == AE_OK || status == AE_SUPPORT) { > > + capbuf_ret = context.ret.pointer; > > + capbuf[OSC_SUPPORT_DWORD] = > capbuf_ret[OSC_SUPPORT_DWORD]; > > + kfree(context.ret.pointer); > > + } > > + if (status != AE_SUPPORT) > > + break; > > + } > > + if (ACPI_FAILURE(status)) > > + return status; > > > > +skip_negotiate: > > + capbuf[OSC_QUERY_DWORD] = 0; > > status = acpi_run_osc(handle, &context); > > if (ACPI_SUCCESS(status)) { > > *retval = *((u32 *)(context.ret.pointer + 8)); > > -- > > 2.34.1 > >
On Wed, Mar 9, 2022 at 2:48 PM Limonciello, Mario <Mario.Limonciello@amd.com> wrote: > > [AMD Official Use Only] > > > > > > > > commit a412caea5a2d ("ACPI: bus: Allow negotiating _OSC capabilities") > > > added support for `acpi_run_osc` to return `AE_SUPPORT` when negotiating > > > an _OSC. > > > > > > This was fixed in other kernel consumers, but `acpi_pci_run_osc` was > > > missed. Update the function to detect when called with > > `OSC_QUERY_ENABLE` > > > set and attempt to negotiate up to 5 times. > > > > This is not how it is designed to work, though. > > > > acpi_pci_query_osc() is for that. > > > > > > > > Reported-by: Qian Cai <quic_qiancai@quicinc.com> > > > Fixes: a412caea5a2d ("ACPI: bus: Allow negotiating _OSC capabilities") > > > > So I'm seriously thinking about dropping that whole lot at this point. > > Do you want me to send up a series perhaps that reverts those 3 commits I can just drop them at this point, so no need to revert. > and instead moves the logic changes on query handling from acpi_run_osc into > acpi_bus_osc_negotiate_platform_control? Yes, please!
[Public] > -----Original Message----- > From: Rafael J. Wysocki <rafael@kernel.org> > Sent: Wednesday, March 9, 2022 08:25 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: Rafael J. Wysocki <rafael@kernel.org>; Rafael J . Wysocki > <rjw@rjwysocki.net>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>; > Qian Cai <quic_qiancai@quicinc.com> > Subject: Re: [PATCH] PCI/ACPI: Add support for `AE_SUPPORT` in _OSC > queries > > On Wed, Mar 9, 2022 at 2:48 PM Limonciello, Mario > <Mario.Limonciello@amd.com> wrote: > > > > [AMD Official Use Only] > > > > > > > > > > > > commit a412caea5a2d ("ACPI: bus: Allow negotiating _OSC capabilities") > > > > added support for `acpi_run_osc` to return `AE_SUPPORT` when > negotiating > > > > an _OSC. > > > > > > > > This was fixed in other kernel consumers, but `acpi_pci_run_osc` was > > > > missed. Update the function to detect when called with > > > `OSC_QUERY_ENABLE` > > > > set and attempt to negotiate up to 5 times. > > > > > > This is not how it is designed to work, though. > > > > > > acpi_pci_query_osc() is for that. > > > > > > > > > > > Reported-by: Qian Cai <quic_qiancai@quicinc.com> > > > > Fixes: a412caea5a2d ("ACPI: bus: Allow negotiating _OSC capabilities") > > > > > > So I'm seriously thinking about dropping that whole lot at this point. > > > > Do you want me to send up a series perhaps that reverts those 3 commits > > I can just drop them at this point, so no need to revert. > > > and instead moves the logic changes on query handling from acpi_run_osc > into > > acpi_bus_osc_negotiate_platform_control? > > Yes, please! Here it is. I did it based on linux-pm/bleeding-edge with those 3 commits reverted first (on the assumption they drop). Should have done this from start - it's so many less LOC and easier to follow! https://patchwork.kernel.org/project/linux-acpi/patch/20220309163749.773474-1-mario.limonciello@amd.com/
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 6f9e75d14808..2eda355fde57 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -171,7 +171,7 @@ static void decode_osc_control(struct acpi_pci_root *root, char *msg, u32 word) static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766"; static acpi_status acpi_pci_run_osc(acpi_handle handle, - const u32 *capbuf, u32 *retval) + u32 *capbuf, u32 *retval) { struct acpi_osc_context context = { .uuid_str = pci_osc_uuid_str, @@ -180,7 +180,26 @@ static acpi_status acpi_pci_run_osc(acpi_handle handle, .cap.pointer = (void *)capbuf, }; acpi_status status; + u32 *capbuf_ret; + int i; + + if (!(capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE)) + goto skip_negotiate; + for (i = 0; i < 5; i++) { + status = acpi_run_osc(handle, &context); + if (status == AE_OK || status == AE_SUPPORT) { + capbuf_ret = context.ret.pointer; + capbuf[OSC_SUPPORT_DWORD] = capbuf_ret[OSC_SUPPORT_DWORD]; + kfree(context.ret.pointer); + } + if (status != AE_SUPPORT) + break; + } + if (ACPI_FAILURE(status)) + return status; +skip_negotiate: + capbuf[OSC_QUERY_DWORD] = 0; status = acpi_run_osc(handle, &context); if (ACPI_SUCCESS(status)) { *retval = *((u32 *)(context.ret.pointer + 8));
commit a412caea5a2d ("ACPI: bus: Allow negotiating _OSC capabilities") added support for `acpi_run_osc` to return `AE_SUPPORT` when negotiating an _OSC. This was fixed in other kernel consumers, but `acpi_pci_run_osc` was missed. Update the function to detect when called with `OSC_QUERY_ENABLE` set and attempt to negotiate up to 5 times. Reported-by: Qian Cai <quic_qiancai@quicinc.com> Fixes: a412caea5a2d ("ACPI: bus: Allow negotiating _OSC capabilities") Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/acpi/pci_root.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)