Re: [PATCH v4 2/2] ACPICA: Preserve memory opregion mappings
On 30 Jun 2020 17:52, Rafael J. Wysocki wrote: > On Tue, Jun 30, 2020 at 5:31 PM Al Stone wrote: > > > > On 30 Jun 2020 13:44, Rafael J. Wysocki wrote: > > > On Mon, Jun 29, 2020 at 10:57 PM Al Stone wrote: > > > > > > > > On 29 Jun 2020 18:33, Rafael J. Wysocki wrote: > > > > > From: "Rafael J. Wysocki" > > > > > > > > > > The ACPICA's strategy with respect to the handling of memory mappings > > > > > associated with memory operation regions is to avoid mapping the > > > > > entire region at once which may be problematic at least in principle > > > > > (for example, it may lead to conflicts with overlapping mappings > > > > > having different attributes created by drivers). It may also be > > > > > wasteful, because memory opregions on some systems take up vast > > > > > chunks of address space while the fields in those regions actually > > > > > accessed by AML are sparsely distributed. > > > > > > > > > > For this reason, a one-page "window" is mapped for a given opregion > > > > > on the first memory access through it and if that "window" does not > > > > > cover an address range accessed through that opregion subsequently, > > > > > it is unmapped and a new "window" is mapped to replace it. Next, > > > > > if the new "window" is not sufficient to acess memory through the > > > > > opregion in question in the future, it will be replaced with yet > > > > > another "window" and so on. That may lead to a suboptimal sequence > > > > > of memory mapping and unmapping operations, for example if two fields > > > > > in one opregion separated from each other by a sufficiently wide > > > > > chunk of unused address space are accessed in an alternating pattern. > > > > > > > > > > The situation may still be suboptimal if the deferred unmapping > > > > > introduced previously is supported by the OS layer. For instance, > > > > > the alternating memory access pattern mentioned above may produce > > > > > a relatively long list of mappings to release with substantial > > > > > duplication among the entries in it, which could be avoided if > > > > > acpi_ex_system_memory_space_handler() did not release the mapping > > > > > used by it previously as soon as the current access was not covered > > > > > by it. > > > > > > > > > > In order to improve that, modify acpi_ex_system_memory_space_handler() > > > > > to preserve all of the memory mappings created by it until the memory > > > > > regions associated with them go away. > > > > > > > > > > Accordingly, update acpi_ev_system_memory_region_setup() to unmap all > > > > > memory associated with memory opregions that go away. > > > > > > > > > > Reported-by: Dan Williams > > > > > Signed-off-by: Rafael J. Wysocki > > > > > --- > > > > > drivers/acpi/acpica/evrgnini.c | 14 > > > > > drivers/acpi/acpica/exregion.c | 65 > > > > > -- > > > > > include/acpi/actypes.h | 12 +-- > > > > > 3 files changed, 64 insertions(+), 27 deletions(-) > > > > > > > > > > diff --git a/drivers/acpi/acpica/evrgnini.c > > > > > b/drivers/acpi/acpica/evrgnini.c > > > > > index aefc0145e583..89be3ccdad53 100644 > > > > > --- a/drivers/acpi/acpica/evrgnini.c > > > > > +++ b/drivers/acpi/acpica/evrgnini.c > > > > > @@ -38,6 +38,7 @@ acpi_ev_system_memory_region_setup(acpi_handle > > > > > handle, > > > > > union acpi_operand_object *region_desc = > > > > > (union acpi_operand_object *)handle; > > > > > struct acpi_mem_space_context *local_region_context; > > > > > + struct acpi_mem_mapping *mm; > > > > > > > > > > ACPI_FUNCTION_TRACE(ev_system_memory_region_setup); > > > > > > > > > > @@ -46,13 +47,14 @@ acpi_ev_system_memory_region_setup(acpi_handle > > > > > handle, > > > > > local_region_context = > > > > > (struct acpi_mem_space_context > > >
Re: [PATCH v4 2/2] ACPICA: Preserve memory opregion mappings
On 30 Jun 2020 13:44, Rafael J. Wysocki wrote:
> On Mon, Jun 29, 2020 at 10:57 PM Al Stone wrote:
> >
> > On 29 Jun 2020 18:33, Rafael J. Wysocki wrote:
> > > From: "Rafael J. Wysocki"
> > >
> > > The ACPICA's strategy with respect to the handling of memory mappings
> > > associated with memory operation regions is to avoid mapping the
> > > entire region at once which may be problematic at least in principle
> > > (for example, it may lead to conflicts with overlapping mappings
> > > having different attributes created by drivers). It may also be
> > > wasteful, because memory opregions on some systems take up vast
> > > chunks of address space while the fields in those regions actually
> > > accessed by AML are sparsely distributed.
> > >
> > > For this reason, a one-page "window" is mapped for a given opregion
> > > on the first memory access through it and if that "window" does not
> > > cover an address range accessed through that opregion subsequently,
> > > it is unmapped and a new "window" is mapped to replace it. Next,
> > > if the new "window" is not sufficient to acess memory through the
> > > opregion in question in the future, it will be replaced with yet
> > > another "window" and so on. That may lead to a suboptimal sequence
> > > of memory mapping and unmapping operations, for example if two fields
> > > in one opregion separated from each other by a sufficiently wide
> > > chunk of unused address space are accessed in an alternating pattern.
> > >
> > > The situation may still be suboptimal if the deferred unmapping
> > > introduced previously is supported by the OS layer. For instance,
> > > the alternating memory access pattern mentioned above may produce
> > > a relatively long list of mappings to release with substantial
> > > duplication among the entries in it, which could be avoided if
> > > acpi_ex_system_memory_space_handler() did not release the mapping
> > > used by it previously as soon as the current access was not covered
> > > by it.
> > >
> > > In order to improve that, modify acpi_ex_system_memory_space_handler()
> > > to preserve all of the memory mappings created by it until the memory
> > > regions associated with them go away.
> > >
> > > Accordingly, update acpi_ev_system_memory_region_setup() to unmap all
> > > memory associated with memory opregions that go away.
> > >
> > > Reported-by: Dan Williams
> > > Signed-off-by: Rafael J. Wysocki
> > > ---
> > > drivers/acpi/acpica/evrgnini.c | 14
> > > drivers/acpi/acpica/exregion.c | 65 --
> > > include/acpi/actypes.h | 12 +--
> > > 3 files changed, 64 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpica/evrgnini.c
> > > b/drivers/acpi/acpica/evrgnini.c
> > > index aefc0145e583..89be3ccdad53 100644
> > > --- a/drivers/acpi/acpica/evrgnini.c
> > > +++ b/drivers/acpi/acpica/evrgnini.c
> > > @@ -38,6 +38,7 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
> > > union acpi_operand_object *region_desc =
> > > (union acpi_operand_object *)handle;
> > > struct acpi_mem_space_context *local_region_context;
> > > + struct acpi_mem_mapping *mm;
> > >
> > > ACPI_FUNCTION_TRACE(ev_system_memory_region_setup);
> > >
> > > @@ -46,13 +47,14 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
> > > local_region_context =
> > > (struct acpi_mem_space_context
> > > *)*region_context;
> > >
> > > - /* Delete a cached mapping if present */
> > > + /* Delete memory mappings if present */
> > >
> > > - if (local_region_context->mapped_length) {
> > > - acpi_os_unmap_memory(local_region_context->
> > > - mapped_logical_address,
> > > - local_region_context->
> > > - mapped_length);
> > > + while (local_region_context->first_mm) {
> > > + mm = local_region_context->first_mm;
> > > + local_r
Re: [PATCH v4 2/2] ACPICA: Preserve memory opregion mappings
ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> "System-Memory (width %u) R/W %u
> Address=%8.8X%8.8X\n",
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> index aa236b9e6f24..d005e35ab399 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -1201,12 +1201,18 @@ struct acpi_pci_id {
> u16 function;
> };
>
> +struct acpi_mem_mapping {
> + acpi_physical_address physical_address;
> + u8 *logical_address;
> + acpi_size length;
> + struct acpi_mem_mapping *next_mm;
> +};
> +
> struct acpi_mem_space_context {
> u32 length;
> acpi_physical_address address;
> - acpi_physical_address mapped_physical_address;
> - u8 *mapped_logical_address;
> - acpi_size mapped_length;
> + struct acpi_mem_mapping *cur_mm;
> + struct acpi_mem_mapping *first_mm;
> };
>
> /*
> --
> 2.26.2
>
>
>
>
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
[PATCH v3] ACPI / CPPC: do not require the _PSD method when using CPPC
According to the ACPI 6.3 specification, the _PSD method is optional when using CPPC. The underlying assumption is that each CPU can change frequency independently from all other CPUs; _PSD is provided to tell the OS that some processors can NOT do that. However, the acpi_get_psd() function returns ENODEV if there is no _PSD method present, or an ACPI error status if an error occurs when evaluating _PSD, if present. This makes _PSD mandatory when using CPPC, in violation of the specification, and only on Linux. This has forced some firmware writers to provide a dummy _PSD, even though it is irrelevant, but only because Linux requires it; other OSPMs follow the spec. We really do not want to have OS specific ACPI tables, though. So, correct acpi_get_psd() so that it does not return an error if there is no _PSD method present, but does return a failure when the method can not be executed properly. This allows _PSD to be optional as it should be. v3: -- bodged the simplification in v2; simplified it properly this time -- verified all error statuses are handled v2: -- verified simple check for AE_NOT_FOUND was sufficient -- simplified return status check per Rafael's suggestion Signed-off-by: Al Stone --- drivers/acpi/cppc_acpi.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index 15f103d7532b..3b2525908dd8 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -365,8 +365,10 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle) union acpi_object *psd = NULL; struct acpi_psd_package *pdomain; - status = acpi_evaluate_object_typed(handle, "_PSD", NULL, , - ACPI_TYPE_PACKAGE); + status = acpi_evaluate_object_typed(handle, "_PSD", NULL, + , ACPI_TYPE_PACKAGE); + if (status == AE_NOT_FOUND) /* _PSD is optional */ + return 0; if (ACPI_FAILURE(status)) return -ENODEV; -- 2.21.0
Re: [PATCH v2] ACPI / CPPC: do not require the _PSD method when using CPPC
On 8/26/19 5:02 PM, Rafael J. Wysocki wrote:
> On Tue, Aug 27, 2019 at 12:30 AM Al Stone wrote:
>>
>> According to the ACPI 6.3 specification, the _PSD method is optional
>> when using CPPC. The underlying assumption is that each CPU can change
>> frequency independently from all other CPUs; _PSD is provided to tell
>> the OS that some processors can NOT do that.
>>
>> However, the acpi_get_psd() function returns ENODEV if there is no _PSD
>> method present, or an ACPI error status if an error occurs when evaluating
>> _PSD, if present. This makes _PSD mandatory when using CPPC, in violation
>> of the specification, and only on Linux.
>>
>> This has forced some firmware writers to provide a dummy _PSD, even though
>> it is irrelevant, but only because Linux requires it; other OSPMs follow
>> the spec. We really do not want to have OS specific ACPI tables, though.
>>
>> So, correct acpi_get_psd() so that it does not return an error if there
>> is no _PSD method present, but does return a failure when the method can
>> not be executed properly. This allows _PSD to be optional as it should
>> be.
>>
>> v2:
>>-- verified simple check for AE_NOT_FOUND was sufficient
>>-- simplified return status check per Rafael's suggestion
>>
>> Signed-off-by: Al Stone
>> Cc: Rafael J. Wysocki
>> Cc: Len Brown
>> ---
>> drivers/acpi/cppc_acpi.c | 10 ++
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 15f103d7532b..7a946f1944ab 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -365,10 +365,12 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr,
>> acpi_handle handle)
>> union acpi_object *psd = NULL;
>> struct acpi_psd_package *pdomain;
>>
>> - status = acpi_evaluate_object_typed(handle, "_PSD", NULL, ,
>> - ACPI_TYPE_PACKAGE);
>> - if (ACPI_FAILURE(status))
>> - return -ENODEV;
>> + if (acpi_has_method(handle, "_PSD")) {
>
> This doesn't look necessary any more.
Probably true. I'll look back through acpi_evaluate_object_typed().
>> + status = acpi_evaluate_object_typed(handle, "_PSD", NULL,
>> + ,
>> ACPI_TYPE_PACKAGE);
>> + if (status == AE_NOT_FOUND) /* _PSD is optional */
>> + return 0;
>
> And what about the other possible errors?
Argh. My apologies. I was not paying attention. I'll correct
this and send proper code tomorrow. Really sorry for the noise :(...
>> + }
>>
>> psd = buffer.pointer;
>> if (!psd || psd->package.count != 1) {
>> --
>> 2.21.0
>>
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
[PATCH v2] ACPI / CPPC: do not require the _PSD method when using CPPC
According to the ACPI 6.3 specification, the _PSD method is optional
when using CPPC. The underlying assumption is that each CPU can change
frequency independently from all other CPUs; _PSD is provided to tell
the OS that some processors can NOT do that.
However, the acpi_get_psd() function returns ENODEV if there is no _PSD
method present, or an ACPI error status if an error occurs when evaluating
_PSD, if present. This makes _PSD mandatory when using CPPC, in violation
of the specification, and only on Linux.
This has forced some firmware writers to provide a dummy _PSD, even though
it is irrelevant, but only because Linux requires it; other OSPMs follow
the spec. We really do not want to have OS specific ACPI tables, though.
So, correct acpi_get_psd() so that it does not return an error if there
is no _PSD method present, but does return a failure when the method can
not be executed properly. This allows _PSD to be optional as it should
be.
v2:
-- verified simple check for AE_NOT_FOUND was sufficient
-- simplified return status check per Rafael's suggestion
Signed-off-by: Al Stone
Cc: Rafael J. Wysocki
Cc: Len Brown
---
drivers/acpi/cppc_acpi.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 15f103d7532b..7a946f1944ab 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -365,10 +365,12 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr,
acpi_handle handle)
union acpi_object *psd = NULL;
struct acpi_psd_package *pdomain;
- status = acpi_evaluate_object_typed(handle, "_PSD", NULL, ,
- ACPI_TYPE_PACKAGE);
- if (ACPI_FAILURE(status))
- return -ENODEV;
+ if (acpi_has_method(handle, "_PSD")) {
+ status = acpi_evaluate_object_typed(handle, "_PSD", NULL,
+ , ACPI_TYPE_PACKAGE);
+ if (status == AE_NOT_FOUND) /* _PSD is optional */
+ return 0;
+ }
psd = buffer.pointer;
if (!psd || psd->package.count != 1) {
--
2.21.0
Re: [PATCH] ACPI / CPPC: do not require the _PSD method when using CPPC
On 8/13/19 3:59 PM, Rafael J. Wysocki wrote:
> On Monday, August 5, 2019 7:03:38 PM CEST Al Stone wrote:
>> According to the ACPI 6.3 specification, the _PSD method is optional
>> when using CPPC. The underlying assumption appears to be that each CPU
>> can change frequency independently from all other CPUs; _PSD is provided
>> to tell the OS that some processors can NOT do that.
>>
>> However, the acpi_get_psd() function returns -ENODEV if there is no _PSD
>> method present, or an ACPI error status if an error occurs when evaluating
>> _PSD, if present. This essentially makes _PSD mandatory when using CPPC,
>> in violation of the specification, and only on Linux.
>>
>> This has forced some firmware writers to provide a dummy _PSD, even though
>> it is irrelevant, but only because Linux requires it; other OSPMs follow
>> the spec. We really do not want to have OS specific ACPI tables, though.
>>
>> So, correct acpi_get_psd() so that it does not return an error if there
>> is no _PSD method present, but does return a failure when the method can
>> not be executed properly. This allows _PSD to be optional as it should
>> be.
>>
>> Signed-off-by: Al Stone
>> Cc: Rafael J. Wysocki
>> Cc: Len Brown
>> ---
>> drivers/acpi/cppc_acpi.c | 11 +++
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 15f103d7532b..e9ecfa13e997 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -365,10 +365,13 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr,
>> acpi_handle handle)
>> union acpi_object *psd = NULL;
>> struct acpi_psd_package *pdomain;
>>
>> -status = acpi_evaluate_object_typed(handle, "_PSD", NULL, ,
>> -ACPI_TYPE_PACKAGE);
>> -if (ACPI_FAILURE(status))
>> -return -ENODEV;
>> +if (acpi_has_method(handle, "_PSD")) {
>
> It would be better to compare the status below to AE_NOT_FOUND
> and return 0 if that's the case.
>
> A couple of code lines could be saved this way at least.
D'oh. Good point.
Let me dig back through the ACPICA code again; I had some reason for not
relying on AE_NOT_FOUND alone that I apparently did not write down in my
notes. I'll send out a v2 when I figure out what it was, and if it was
of any consequence.
>> +status = acpi_evaluate_object_typed(handle, "_PSD", NULL,
>> +, ACPI_TYPE_PACKAGE);
>> + if (ACPI_FAILURE(status))
>> +return -ENODEV;
>> +} else
>> +return 0; /* _PSD is optional */
>>
>> psd = buffer.pointer;
>> if (!psd || psd->package.count != 1) {
>>
Thanks.
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH] ACPI / CPPC: do not require the _PSD method when using CPPC
On 8/13/19 3:57 PM, Rafael J. Wysocki wrote:
> On Tuesday, August 13, 2019 4:00:56 PM CEST Al Stone wrote:
>> On 8/5/19 11:03 AM, Al Stone wrote:
>>> According to the ACPI 6.3 specification, the _PSD method is optional
>>> when using CPPC. The underlying assumption appears to be that each CPU
>>> can change frequency independently from all other CPUs; _PSD is provided
>>> to tell the OS that some processors can NOT do that.
>>>
>>> However, the acpi_get_psd() function returns -ENODEV if there is no _PSD
>>> method present, or an ACPI error status if an error occurs when evaluating
>>> _PSD, if present. This essentially makes _PSD mandatory when using CPPC,
>>> in violation of the specification, and only on Linux.
>>>
>>> This has forced some firmware writers to provide a dummy _PSD, even though
>>> it is irrelevant, but only because Linux requires it; other OSPMs follow
>>> the spec. We really do not want to have OS specific ACPI tables, though.
>>>
>>> So, correct acpi_get_psd() so that it does not return an error if there
>>> is no _PSD method present, but does return a failure when the method can
>>> not be executed properly. This allows _PSD to be optional as it should
>>> be.
>>>
>>> Signed-off-by: Al Stone
>>> Cc: Rafael J. Wysocki
>>> Cc: Len Brown
>>> ---
>>> drivers/acpi/cppc_acpi.c | 11 +++
>>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>> index 15f103d7532b..e9ecfa13e997 100644
>>> --- a/drivers/acpi/cppc_acpi.c
>>> +++ b/drivers/acpi/cppc_acpi.c
>>> @@ -365,10 +365,13 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr,
>>> acpi_handle handle)
>>> union acpi_object *psd = NULL;
>>> struct acpi_psd_package *pdomain;
>>>
>>> - status = acpi_evaluate_object_typed(handle, "_PSD", NULL, ,
>>> - ACPI_TYPE_PACKAGE);
>>> - if (ACPI_FAILURE(status))
>>> - return -ENODEV;
>>> + if (acpi_has_method(handle, "_PSD")) {
>>> + status = acpi_evaluate_object_typed(handle, "_PSD", NULL,
>>> + , ACPI_TYPE_PACKAGE);
>>> + if (ACPI_FAILURE(status))
>>> + return -ENODEV;
>>> + } else
>>> + return 0; /* _PSD is optional */
>>>
>>> psd = buffer.pointer;
>>> if (!psd || psd->package.count != 1) {
>>>
>>
>> Rafael,
>>
>> Any other comments?
>
> Yes (they will be sent separately).
Thanks, I appreciate it.
>> Would it be possible to pull this into an -rc?
>> I'd really like to avoid anyone else having to ship Linux-specific
>> DSDTs and SSDTs.
>
> You won't achieve that through this patch alone, because they will
> also want older kernels that don't include it to run on their platforms.
My apologies for not mentioning this before, but these are platforms
that are not widely available yet. As far as I know they will not be
able to use older kernels at all, even with this fix. They are very
heavily reliant on the most recent changes to quite a few other things
such as HMAT, PPTT, and CPPC in general. This was just one of the items
their firmware developers ran into, so a 5.3 fix is plenty.
Unless, of course, I missed your point entirely
> Thanks,
> Rafael
>
>
>
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH] ACPI / CPPC: do not require the _PSD method when using CPPC
On 8/5/19 11:03 AM, Al Stone wrote:
> According to the ACPI 6.3 specification, the _PSD method is optional
> when using CPPC. The underlying assumption appears to be that each CPU
> can change frequency independently from all other CPUs; _PSD is provided
> to tell the OS that some processors can NOT do that.
>
> However, the acpi_get_psd() function returns -ENODEV if there is no _PSD
> method present, or an ACPI error status if an error occurs when evaluating
> _PSD, if present. This essentially makes _PSD mandatory when using CPPC,
> in violation of the specification, and only on Linux.
>
> This has forced some firmware writers to provide a dummy _PSD, even though
> it is irrelevant, but only because Linux requires it; other OSPMs follow
> the spec. We really do not want to have OS specific ACPI tables, though.
>
> So, correct acpi_get_psd() so that it does not return an error if there
> is no _PSD method present, but does return a failure when the method can
> not be executed properly. This allows _PSD to be optional as it should
> be.
>
> Signed-off-by: Al Stone
> Cc: Rafael J. Wysocki
> Cc: Len Brown
> ---
> drivers/acpi/cppc_acpi.c | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 15f103d7532b..e9ecfa13e997 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -365,10 +365,13 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr,
> acpi_handle handle)
> union acpi_object *psd = NULL;
> struct acpi_psd_package *pdomain;
>
> - status = acpi_evaluate_object_typed(handle, "_PSD", NULL, ,
> - ACPI_TYPE_PACKAGE);
> - if (ACPI_FAILURE(status))
> - return -ENODEV;
> + if (acpi_has_method(handle, "_PSD")) {
> + status = acpi_evaluate_object_typed(handle, "_PSD", NULL,
> + , ACPI_TYPE_PACKAGE);
> + if (ACPI_FAILURE(status))
> + return -ENODEV;
> + } else
> + return 0; /* _PSD is optional */
>
> psd = buffer.pointer;
> if (!psd || psd->package.count != 1) {
>
Rafael,
Any other comments? Would it be possible to pull this into an -rc?
I'd really like to avoid anyone else having to ship Linux-specific
DSDTs and SSDTs.
Thanks.
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH] ACPI / CPPC: do not require the _PSD method when using CPPC
On 8/7/19 5:41 AM, Sudeep Holla wrote: > On Mon, Aug 05, 2019 at 11:03:38AM -0600, Al Stone wrote: >> According to the ACPI 6.3 specification, the _PSD method is optional >> when using CPPC. The underlying assumption appears to be that each CPU >> can change frequency independently from all other CPUs; _PSD is provided >> to tell the OS that some processors can NOT do that. >> >> However, the acpi_get_psd() function returns -ENODEV if there is no _PSD >> method present, or an ACPI error status if an error occurs when evaluating >> _PSD, if present. This essentially makes _PSD mandatory when using CPPC, >> in violation of the specification, and only on Linux. >> >> This has forced some firmware writers to provide a dummy _PSD, even though >> it is irrelevant, but only because Linux requires it; other OSPMs follow >> the spec. We really do not want to have OS specific ACPI tables, though. >> >> So, correct acpi_get_psd() so that it does not return an error if there >> is no _PSD method present, but does return a failure when the method can >> not be executed properly. This allows _PSD to be optional as it should >> be. >> > > Makes sense to me. FWIW, > > Reviewed-by: Sudeep Holla < sudeep.ho...@arm.com> > > -- > Regards, > Sudeep > Thanks for the review, Sudeep. All the ARM systems I've seen seem to have a _PSD so this hasn't been an issue there. Some newer platforms coming out are starting to use CPPC, though, and took the spec at face value :). -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
[PATCH] ACPI / CPPC: do not require the _PSD method when using CPPC
According to the ACPI 6.3 specification, the _PSD method is optional
when using CPPC. The underlying assumption appears to be that each CPU
can change frequency independently from all other CPUs; _PSD is provided
to tell the OS that some processors can NOT do that.
However, the acpi_get_psd() function returns -ENODEV if there is no _PSD
method present, or an ACPI error status if an error occurs when evaluating
_PSD, if present. This essentially makes _PSD mandatory when using CPPC,
in violation of the specification, and only on Linux.
This has forced some firmware writers to provide a dummy _PSD, even though
it is irrelevant, but only because Linux requires it; other OSPMs follow
the spec. We really do not want to have OS specific ACPI tables, though.
So, correct acpi_get_psd() so that it does not return an error if there
is no _PSD method present, but does return a failure when the method can
not be executed properly. This allows _PSD to be optional as it should
be.
Signed-off-by: Al Stone
Cc: Rafael J. Wysocki
Cc: Len Brown
---
drivers/acpi/cppc_acpi.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 15f103d7532b..e9ecfa13e997 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -365,10 +365,13 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr,
acpi_handle handle)
union acpi_object *psd = NULL;
struct acpi_psd_package *pdomain;
- status = acpi_evaluate_object_typed(handle, "_PSD", NULL, ,
- ACPI_TYPE_PACKAGE);
- if (ACPI_FAILURE(status))
- return -ENODEV;
+ if (acpi_has_method(handle, "_PSD")) {
+ status = acpi_evaluate_object_typed(handle, "_PSD", NULL,
+ , ACPI_TYPE_PACKAGE);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+ } else
+ return 0; /* _PSD is optional */
psd = buffer.pointer;
if (!psd || psd->package.count != 1) {
--
2.21.0
Re: [RFC PATCH] ACPI / processors: allow a processor device _UID to be a string
On 6/11/19 10:11 AM, Sudeep Holla wrote:
> On Tue, Jun 11, 2019 at 10:03:15AM -0600, Al Stone wrote:
>> On 6/11/19 6:53 AM, Sudeep Holla wrote:
>>> On Mon, Jun 10, 2019 at 02:07:34PM -0600, Al Stone wrote:
>>>> In the ACPI specification, section 6.1.12, a _UID may be either an
>>>> integer or a string object. Up until now, when defining processor
>>>> Device()s in ACPI (_HID ACPI0007), only integers were allowed even
>>>> though this ignored the specification. As a practical matter, it
>>>> was not an issue.
>>>>
>>>> Recently, some DSDTs have shown up that look like this:
>>>>
>>>> Device (XX00)
>>>> {
>>>>Name (_HID, "ACPI0007" /* Processor Device */)
>>>> Name (_UID, "XYZZY-XX00")
>>>> .
>>>> }
>>>>
>>>> which is perfectly legal. However, the kernel will report instead:
>>>>
>>>
>>> I am not sure how this can be perfectly legal from specification
>>> perspective. It's legal with respect to AML namespace but then the
>>> other condition of this matching with entries in static tables like
>>> MADT is not possible where there are declared to be simple 4 byte
>>> integer/word. Same is true for even ACPI0010, the processor container
>>> objects which need to match entries in PPTT,
>>>
>>> ACPI Processor UID(in MADT): The OS associates this GICC(applies even
>>> for APIC and family) Structure with a processor device object in
>>> the namespace when the _UID child object of the processor device
>>> evaluates to a numeric value that matches the numeric value in this
>>> field.
>>>
>>> So for me that indicates it can't be string unless you have some ways to
>>> match those _UID entries to ACPI Processor ID in MADT and PPTT.
>>>
>>> Let me know if I am missing to consider something here.
>>>
>>> --
>>> Regards,
>>> Sudeep
>>>
>>
>> Harumph. I think what we have here is a big mess in the spec, but
>> that is exactly why this is an RFC.
>>
>> The MADT can have any of ~16 different subtables, as you note. Of
>> those, only these require a numeric _UID:
>>
>>-- Type 0x0: Processor Local APIC
>>-- Type 0x4: Local APIC NMI [0]
>>-- Type 0x7: Processor Local SAPIC [1]
>>-- Type 0x9: Processor Local x2APIC
>>-- Type 0xa: Local x2APIC NMI [0]
>>-- Type 0xb: GICC
>>
>> Note [0]: a value of !0x0 is also allowed, indicating all processors
>> [1]: this has two fields that could be interpreted as an ID when
>> used together
>>
>> It does not appear that you could build a usable system without any
>> of these subtables -- but perhaps someone knows of incantations that
>> could -- which is why I thought a string _UID might be viable.
>>
>
> I hope no one is shipping such device yet or am I wrong ?
> We can ask them to fix as Linux simply can't boot on such system or
> even if it boots, it may have issues with acpi_processor drivers.
I don't think it's shipping, but even if it is, I'm going to have to
insist they fix their tables, just as a practical matter. I need to
ask if it boots that other OS, too.
>> If we consider the PPTT too, then yeah, _UID must be an integer for
>> some devices.
>>
>> Thanks for the feedback; it forced me to double-check my thinking about
>> the MADT. The root cause of the issue is not the kernel in this case,
>> but a lack of clarity in the spec -- or at least implied requirements
>> that probably need to be explicit. I'll send in a spec change.
>>
>
> Completely agreed. Even little more clarification on this is helpful.
> Thanks for volunteering :) to take up spec change, much appreciated.
No problem, and glad to do it.
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [RFC PATCH] ACPI / processors: allow a processor device _UID to be a string
On 6/11/19 6:53 AM, Sudeep Holla wrote:
> On Mon, Jun 10, 2019 at 02:07:34PM -0600, Al Stone wrote:
>> In the ACPI specification, section 6.1.12, a _UID may be either an
>> integer or a string object. Up until now, when defining processor
>> Device()s in ACPI (_HID ACPI0007), only integers were allowed even
>> though this ignored the specification. As a practical matter, it
>> was not an issue.
>>
>> Recently, some DSDTs have shown up that look like this:
>>
>> Device (XX00)
>> {
>> Name (_HID, "ACPI0007" /* Processor Device */)
>> Name (_UID, "XYZZY-XX00")
>> .
>> }
>>
>> which is perfectly legal. However, the kernel will report instead:
>>
>
> I am not sure how this can be perfectly legal from specification
> perspective. It's legal with respect to AML namespace but then the
> other condition of this matching with entries in static tables like
> MADT is not possible where there are declared to be simple 4 byte
> integer/word. Same is true for even ACPI0010, the processor container
> objects which need to match entries in PPTT,
>
> ACPI Processor UID(in MADT): The OS associates this GICC(applies even
> for APIC and family) Structure with a processor device object in
> the namespace when the _UID child object of the processor device
> evaluates to a numeric value that matches the numeric value in this
> field.
>
> So for me that indicates it can't be string unless you have some ways to
> match those _UID entries to ACPI Processor ID in MADT and PPTT.
>
> Let me know if I am missing to consider something here.
>
> --
> Regards,
> Sudeep
>
Harumph. I think what we have here is a big mess in the spec, but
that is exactly why this is an RFC.
The MADT can have any of ~16 different subtables, as you note. Of
those, only these require a numeric _UID:
-- Type 0x0: Processor Local APIC
-- Type 0x4: Local APIC NMI [0]
-- Type 0x7: Processor Local SAPIC [1]
-- Type 0x9: Processor Local x2APIC
-- Type 0xa: Local x2APIC NMI [0]
-- Type 0xb: GICC
Note [0]: a value of !0x0 is also allowed, indicating all processors
[1]: this has two fields that could be interpreted as an ID when
used together
It does not appear that you could build a usable system without any
of these subtables -- but perhaps someone knows of incantations that
could -- which is why I thought a string _UID might be viable.
If we consider the PPTT too, then yeah, _UID must be an integer for
some devices.
Thanks for the feedback; it forced me to double-check my thinking about
the MADT. The root cause of the issue is not the kernel in this case,
but a lack of clarity in the spec -- or at least implied requirements
that probably need to be explicit. I'll send in a spec change.
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
[RFC PATCH] ACPI / processors: allow a processor device _UID to be a string
In the ACPI specification, section 6.1.12, a _UID may be either an
integer or a string object. Up until now, when defining processor
Device()s in ACPI (_HID ACPI0007), only integers were allowed even
though this ignored the specification. As a practical matter, it
was not an issue.
Recently, some DSDTs have shown up that look like this:
Device (XX00)
{
Name (_HID, "ACPI0007" /* Processor Device */)
Name (_UID, "XYZZY-XX00")
.
}
which is perfectly legal. However, the kernel will report instead:
...
[0.706970] ACPI: \_SB_..XX00: Invalid processor object
...
[0.776998] acpi ACPI0007:xx: Failed to evaluate processor _UID (0xc)
...
The second message comes from acpi_processor_get_info() not being
able to use the string value for _UID that was provided in the DSDT;
the 0xc in the message is actually the buffer size of the returned
value from trying to evaluate _UID.
We correct this by first adding a field called acpi_name to struct
acpi_processor. Then we add the function acpi_processor_evaluate_uid()
to get the returned _UID object and, if it is an integer, behave exactly
as acpi_processor_get_info() did before, but if it is a string, assign
it an arbitrary integer value (required by all the users of this
struct) and capture the actual string used. This results in the
functions acpi_processor_add() and acpi_processor_remove() changing
also so that we do not inadvertently forget to free the space used
for the new acpi_name field.
Why RFC and not a straight patch? I have some concerns that need broader
knowledge than I have at my disposal:
1) Is there a need to expose the CPU name captured from the ACPI
namespace in sysfs (or elsewhere)? I could not think of any
good reason but I probably just missed something.
2) Is making last_used_string_cpuid atomic required? I think it
is, but only because I think acpi_processor_add() could get called
on multiple CPUs at roughly the same time. If that never happens,
an int would be just fine.
3) Is it necessary to check that a _UID string is a duplicate of the
_UID string of some other CPU Device() (_HID ACPI0007)? The code
will ensure unique integer IDs, and the firmware writer should not
have provided duplicate string _UIDs, but things happen. I would
need to add this code to this patch.
4) Testing: I only have one very fragile machine to test this on (the
firmware is really, really unstable right now); more testing would
be greatly appreciated.
Signed-off-by: Al Stone
---
drivers/acpi/acpi_processor.c | 57 ++-
include/acpi/processor.h | 1 +
2 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 449d86d39965..f40163e0edf9 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -14,6 +14,7 @@
*/
#include
+#include
#include
#include
#include
@@ -32,6 +33,8 @@ ACPI_MODULE_NAME("processor");
DEFINE_PER_CPU(struct acpi_processor *, processors);
EXPORT_PER_CPU_SYMBOL(processors);
+static atomic_t last_used_string_cpuid;
+
/* --
Errata Handling
--
*/
@@ -229,6 +232,49 @@ static inline int acpi_processor_hotadd_init(struct
acpi_processor *pr)
}
#endif /* CONFIG_ACPI_HOTPLUG_CPU */
+static acpi_status acpi_processor_evaluate_uid(struct acpi_device *device)
+{
+ acpi_status status = AE_OK;
+ struct acpi_processor *pr;
+ union acpi_object object = { 0 };
+ struct acpi_buffer buffer = { sizeof(union acpi_object), };
+
+ if (!device)
+ return -ENODEV;
+
+ /*
+* Declared with "Device" statement; match _UID.
+*
+* By definition in the ACPI spec, _UID may return either
+* an integer or a string.
+*/
+ pr = acpi_driver_data(device);
+ if (!pr)
+ return -ENODEV;
+
+ status = acpi_evaluate_object(pr->handle, METHOD_NAME__UID,
+ NULL, );
+ if (ACPI_FAILURE(status)) {
+ ACPI_FREE(buffer.pointer);
+ return status;
+ }
+
+ if (object.type == ACPI_TYPE_INTEGER) {
+ pr->acpi_id = object.integer.value;
+ pr->acpi_name = NULL;
+ ACPI_FREE(buffer.pointer); /* no longer needed */
+ } else if (object.type == ACPI_TYPE_STRING) {
+ /* no need to be clever, just pick an unused number */
+ pr->acpi_id = atomic_inc_return(_used_string_cpuid);
+ pr->acpi_name = object.string.pointer; /* still needed */
+ } else {
+ ACPI_F
Re: [PATCH] mailbox: PCC: handle parse error
On 08/27/2018 01:19 PM, David Arcari wrote:
> acpi_pcc_probe calls acpi_table_parse_entries_array but fails to check
> for an error return. This in turn can result in calling kcalloc with
> a negative count as well as emitting the following misleading erorr
> message:
>
> [2.642015] Could not allocate space for PCC mbox channels
>
> Fixes: 8f8027c5f935 ("mailbox: PCC: erroneous error message when parsing ACPI
> PCCT")
>
> Signed-off-by: David Arcari
> Cc: Al Stone
> Cc: Jassi Brar
> ---
> drivers/mailbox/pcc.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 311e91b..256f18b 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -461,8 +461,11 @@ static int __init acpi_pcc_probe(void)
> count = acpi_table_parse_entries_array(ACPI_SIG_PCCT,
> sizeof(struct acpi_table_pcct), proc,
> ACPI_PCCT_TYPE_RESERVED, MAX_PCC_SUBSPACES);
> - if (count == 0 || count > MAX_PCC_SUBSPACES) {
> - pr_warn("Invalid PCCT: %d PCC subspaces\n", count);
> + if (count <= 0 || count > MAX_PCC_SUBSPACES) {
> + if (count < 0)
> + pr_warn("Error parsing PCC subspaces from PCCT\n");
> + else
> + pr_warn("Invalid PCCT: %d PCC subspaces\n", count);
> return -EINVAL;
> }
>
>
Thanks, David. Nice catch.
Reviewed-by: Al Stone
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH] mailbox: PCC: handle parse error
On 08/27/2018 01:19 PM, David Arcari wrote:
> acpi_pcc_probe calls acpi_table_parse_entries_array but fails to check
> for an error return. This in turn can result in calling kcalloc with
> a negative count as well as emitting the following misleading erorr
> message:
>
> [2.642015] Could not allocate space for PCC mbox channels
>
> Fixes: 8f8027c5f935 ("mailbox: PCC: erroneous error message when parsing ACPI
> PCCT")
>
> Signed-off-by: David Arcari
> Cc: Al Stone
> Cc: Jassi Brar
> ---
> drivers/mailbox/pcc.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 311e91b..256f18b 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -461,8 +461,11 @@ static int __init acpi_pcc_probe(void)
> count = acpi_table_parse_entries_array(ACPI_SIG_PCCT,
> sizeof(struct acpi_table_pcct), proc,
> ACPI_PCCT_TYPE_RESERVED, MAX_PCC_SUBSPACES);
> - if (count == 0 || count > MAX_PCC_SUBSPACES) {
> - pr_warn("Invalid PCCT: %d PCC subspaces\n", count);
> + if (count <= 0 || count > MAX_PCC_SUBSPACES) {
> + if (count < 0)
> + pr_warn("Error parsing PCC subspaces from PCCT\n");
> + else
> + pr_warn("Invalid PCCT: %d PCC subspaces\n", count);
> return -EINVAL;
> }
>
>
Thanks, David. Nice catch.
Reviewed-by: Al Stone
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH][next] mailbox: PCC: check for negative count for parse failure checking
On 05/30/2018 12:24 PM, Colin Ian King wrote:
> On 30/05/18 18:59, Al Stone wrote:
>> On 05/30/2018 11:14 AM, Colin King wrote:
>>> From: Colin Ian King
>>>
>>> The function acpi_table_parse_enties_array can potentially return a
>>> negative value if parsing fails. Currently the check on the return
>>> is not checking for errors, so fix this by adding a -ve check too.
>>>
>>> Detected by CoverityScan, CID#1469477 ("Improper use of negative value")
>>>
>>> Fixes: 8f8027c5f935 ("mailbox: PCC: erroneous error message when parsing
>>> ACPI PCCT")
>>> Signed-off-by: Colin Ian King
>>> ---
>>> drivers/mailbox/pcc.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>> index fc3c237daef2..87d67922020d 100644
>>> --- a/drivers/mailbox/pcc.c
>>> +++ b/drivers/mailbox/pcc.c
>>> @@ -461,7 +461,7 @@ static int __init acpi_pcc_probe(void)
>>> count = acpi_table_parse_entries_array(ACPI_SIG_PCCT,
>>> sizeof(struct acpi_table_pcct), proc,
>>> ACPI_PCCT_TYPE_RESERVED, MAX_PCC_SUBSPACES);
>>> - if (count == 0 || count > MAX_PCC_SUBSPACES) {
>>> + if (count <= 0 || count > MAX_PCC_SUBSPACES) {
>>> pr_warn("Invalid PCCT: %d PCC subspaces\n", count);
>>> return -EINVAL;
>>> }
>>>
>>
>> Yup, nice catch. A little paranoid, but we like that in a kernel :).
>> Thanks.
>
> If it can go wrong, it will go wrong, especially with firmware :-)
Amen to that! You are preachin' to the choir, brother ...
>>
>> Reviewed-by: Al Stone
>>
>
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH][next] mailbox: PCC: check for negative count for parse failure checking
On 05/30/2018 12:24 PM, Colin Ian King wrote:
> On 30/05/18 18:59, Al Stone wrote:
>> On 05/30/2018 11:14 AM, Colin King wrote:
>>> From: Colin Ian King
>>>
>>> The function acpi_table_parse_enties_array can potentially return a
>>> negative value if parsing fails. Currently the check on the return
>>> is not checking for errors, so fix this by adding a -ve check too.
>>>
>>> Detected by CoverityScan, CID#1469477 ("Improper use of negative value")
>>>
>>> Fixes: 8f8027c5f935 ("mailbox: PCC: erroneous error message when parsing
>>> ACPI PCCT")
>>> Signed-off-by: Colin Ian King
>>> ---
>>> drivers/mailbox/pcc.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>> index fc3c237daef2..87d67922020d 100644
>>> --- a/drivers/mailbox/pcc.c
>>> +++ b/drivers/mailbox/pcc.c
>>> @@ -461,7 +461,7 @@ static int __init acpi_pcc_probe(void)
>>> count = acpi_table_parse_entries_array(ACPI_SIG_PCCT,
>>> sizeof(struct acpi_table_pcct), proc,
>>> ACPI_PCCT_TYPE_RESERVED, MAX_PCC_SUBSPACES);
>>> - if (count == 0 || count > MAX_PCC_SUBSPACES) {
>>> + if (count <= 0 || count > MAX_PCC_SUBSPACES) {
>>> pr_warn("Invalid PCCT: %d PCC subspaces\n", count);
>>> return -EINVAL;
>>> }
>>>
>>
>> Yup, nice catch. A little paranoid, but we like that in a kernel :).
>> Thanks.
>
> If it can go wrong, it will go wrong, especially with firmware :-)
Amen to that! You are preachin' to the choir, brother ...
>>
>> Reviewed-by: Al Stone
>>
>
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH][next] mailbox: PCC: check for negative count for parse failure checking
On 05/30/2018 11:14 AM, Colin King wrote:
> From: Colin Ian King
>
> The function acpi_table_parse_enties_array can potentially return a
> negative value if parsing fails. Currently the check on the return
> is not checking for errors, so fix this by adding a -ve check too.
>
> Detected by CoverityScan, CID#1469477 ("Improper use of negative value")
>
> Fixes: 8f8027c5f935 ("mailbox: PCC: erroneous error message when parsing ACPI
> PCCT")
> Signed-off-by: Colin Ian King
> ---
> drivers/mailbox/pcc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index fc3c237daef2..87d67922020d 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -461,7 +461,7 @@ static int __init acpi_pcc_probe(void)
> count = acpi_table_parse_entries_array(ACPI_SIG_PCCT,
> sizeof(struct acpi_table_pcct), proc,
> ACPI_PCCT_TYPE_RESERVED, MAX_PCC_SUBSPACES);
> - if (count == 0 || count > MAX_PCC_SUBSPACES) {
> + if (count <= 0 || count > MAX_PCC_SUBSPACES) {
> pr_warn("Invalid PCCT: %d PCC subspaces\n", count);
> return -EINVAL;
> }
>
Yup, nice catch. A little paranoid, but we like that in a kernel :). Thanks.
Reviewed-by: Al Stone
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH][next] mailbox: PCC: check for negative count for parse failure checking
On 05/30/2018 11:14 AM, Colin King wrote:
> From: Colin Ian King
>
> The function acpi_table_parse_enties_array can potentially return a
> negative value if parsing fails. Currently the check on the return
> is not checking for errors, so fix this by adding a -ve check too.
>
> Detected by CoverityScan, CID#1469477 ("Improper use of negative value")
>
> Fixes: 8f8027c5f935 ("mailbox: PCC: erroneous error message when parsing ACPI
> PCCT")
> Signed-off-by: Colin Ian King
> ---
> drivers/mailbox/pcc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index fc3c237daef2..87d67922020d 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -461,7 +461,7 @@ static int __init acpi_pcc_probe(void)
> count = acpi_table_parse_entries_array(ACPI_SIG_PCCT,
> sizeof(struct acpi_table_pcct), proc,
> ACPI_PCCT_TYPE_RESERVED, MAX_PCC_SUBSPACES);
> - if (count == 0 || count > MAX_PCC_SUBSPACES) {
> + if (count <= 0 || count > MAX_PCC_SUBSPACES) {
> pr_warn("Invalid PCCT: %d PCC subspaces\n", count);
> return -EINVAL;
> }
>
Yup, nice catch. A little paranoid, but we like that in a kernel :). Thanks.
Reviewed-by: Al Stone
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH v3 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
On 05/15/2018 02:00 AM, Rafael J. Wysocki wrote: > On Tue, May 15, 2018 at 12:49 AM, Al Stone <a...@redhat.com> wrote: >> On 05/14/2018 03:04 PM, Prakash, Prashanth wrote: >>> >>> >>> On 4/30/2018 6:39 PM, Al Stone wrote: >>>> There have been multiple reports of the following error message: >>>> >>>> [0.068293] Error parsing PCC subspaces from PCCT >>>> >>>> This error message is not correct. In multiple cases examined, the PCCT >>>> (Platform Communications Channel Table) concerned is actually properly >>>> constructed; the problem is that acpi_pcc_probe() which reads the PCCT >>>> is making the assumption that the only valid PCCT is one that contains >>>> subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or >>>> ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these >>>> types are counted and as long as there is at least one of the desired >>>> types, the acpi_pcc_probe() succeeds. When no subtables of these types >>>> are found, regardless of whether or not any other subtable types are >>>> present, the error mentioned above is reported. >>>> >>>> In the cases reported to me personally, the PCCT contains exactly one >>>> subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function >>>> acpi_pcc_probe() does not count it as a valid subtable, so believes >>>> there to be no valid subtables, and hence outputs the error message. >>>> >>>> An example of the PCCT being reported as erroneous yet perfectly fine >>>> is the following: >>>> >>>> Signature : "PCCT" >>>> Table Length : 006E >>>> Revision : 05 >>>> Checksum : A9 >>>>Oem ID : "XX" >>>> Oem Table ID : "X " >>>> Oem Revision : 2280 >>>> Asl Compiler ID : "" >>>> Asl Compiler Revision : 0002 >>>> >>>> Flags (decoded below) : 0001 >>>> Platform : 1 >>>> Reserved : >>>> >>>> Subtable Type : 00 [Generic Communications Subspace] >>>>Length : 3E >>>> >>>> Reserved : >>>> Base Address : DCE43018 >>>>Address Length : 1000 >>>> >>>> Doorbell Register : [Generic Address Structure] >>>> Space ID : 01 [SystemIO] >>>> Bit Width : 08 >>>>Bit Offset : 00 >>>> Encoded Access Width : 01 [Byte Access:8] >>>> Address : 1842 >>>> >>>> Preserve Mask : 00FD >>>>Write Mask : 0002 >>>> Command Latency : 1388 >>>> Maximum Access Rate : >>>> Minimum Turnaround Time : >>>> >>>> To fix this, we count up all of the possible subtable types for the >>>> PCCT, and only report an error when there are none (which could mean >>>> either no subtables, or no valid subtables), or there are too many. >>>> We also change the logic so that if there is a valid subtable, we >>>> do try to initialize it per the PCCT subtable contents. This is a >>>> change in functionality; previously, the probe would have returned >>>> right after the error message and would not have tried to use any >>>> other subtable definition. >>>> >>>> Tested on my personal laptop which showed the error previously; the >>>> error message no longer appears and the laptop appears to operate >>>> normally. >>>> >>>> Signed-off-by: Al Stone <a...@redhat.com> >>>> Cc: Jassi Brar <jassisinghb...@gmail.com> >>>> Cc: Rafael J. Wysocki <r...@rjwysocki.net> >>>> Cc: Len Brown <l...@kernel.org> >>>> --- >>>> drivers/mailbox/pcc.c | 96 >>>> +-- >>>> 1 file changed, 63 insertions(+), 33 deletions(-) >>>>
Re: [PATCH v3 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
On 05/15/2018 02:00 AM, Rafael J. Wysocki wrote: > On Tue, May 15, 2018 at 12:49 AM, Al Stone wrote: >> On 05/14/2018 03:04 PM, Prakash, Prashanth wrote: >>> >>> >>> On 4/30/2018 6:39 PM, Al Stone wrote: >>>> There have been multiple reports of the following error message: >>>> >>>> [0.068293] Error parsing PCC subspaces from PCCT >>>> >>>> This error message is not correct. In multiple cases examined, the PCCT >>>> (Platform Communications Channel Table) concerned is actually properly >>>> constructed; the problem is that acpi_pcc_probe() which reads the PCCT >>>> is making the assumption that the only valid PCCT is one that contains >>>> subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or >>>> ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these >>>> types are counted and as long as there is at least one of the desired >>>> types, the acpi_pcc_probe() succeeds. When no subtables of these types >>>> are found, regardless of whether or not any other subtable types are >>>> present, the error mentioned above is reported. >>>> >>>> In the cases reported to me personally, the PCCT contains exactly one >>>> subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function >>>> acpi_pcc_probe() does not count it as a valid subtable, so believes >>>> there to be no valid subtables, and hence outputs the error message. >>>> >>>> An example of the PCCT being reported as erroneous yet perfectly fine >>>> is the following: >>>> >>>> Signature : "PCCT" >>>> Table Length : 006E >>>> Revision : 05 >>>> Checksum : A9 >>>>Oem ID : "XX" >>>> Oem Table ID : "X " >>>> Oem Revision : 2280 >>>> Asl Compiler ID : "" >>>> Asl Compiler Revision : 0002 >>>> >>>> Flags (decoded below) : 0001 >>>> Platform : 1 >>>> Reserved : >>>> >>>> Subtable Type : 00 [Generic Communications Subspace] >>>>Length : 3E >>>> >>>> Reserved : >>>> Base Address : DCE43018 >>>>Address Length : 1000 >>>> >>>> Doorbell Register : [Generic Address Structure] >>>> Space ID : 01 [SystemIO] >>>> Bit Width : 08 >>>>Bit Offset : 00 >>>> Encoded Access Width : 01 [Byte Access:8] >>>> Address : 1842 >>>> >>>> Preserve Mask : 00FD >>>>Write Mask : 0002 >>>> Command Latency : 1388 >>>> Maximum Access Rate : >>>> Minimum Turnaround Time : >>>> >>>> To fix this, we count up all of the possible subtable types for the >>>> PCCT, and only report an error when there are none (which could mean >>>> either no subtables, or no valid subtables), or there are too many. >>>> We also change the logic so that if there is a valid subtable, we >>>> do try to initialize it per the PCCT subtable contents. This is a >>>> change in functionality; previously, the probe would have returned >>>> right after the error message and would not have tried to use any >>>> other subtable definition. >>>> >>>> Tested on my personal laptop which showed the error previously; the >>>> error message no longer appears and the laptop appears to operate >>>> normally. >>>> >>>> Signed-off-by: Al Stone >>>> Cc: Jassi Brar >>>> Cc: Rafael J. Wysocki >>>> Cc: Len Brown >>>> --- >>>> drivers/mailbox/pcc.c | 96 >>>> +-- >>>> 1 file changed, 63 insertions(+), 33 deletions(-) >>>> >>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c >>>> index 3ef7f036ceea..72af37d7e95e 100644
[PATCH v4 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI, PCCT
There have been multiple reports of the following error message:
[0.068293] Error parsing PCC subspaces from PCCT
This error message is not correct. In multiple cases examined, the PCCT
(Platform Communications Channel Table) concerned is actually properly
constructed; the problem is that acpi_pcc_probe() which reads the PCCT
is making the assumption that the only valid PCCT is one that contains
subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
types are counted and as long as there is at least one of the desired
types, the acpi_pcc_probe() succeeds. When no subtables of these types
are found, regardless of whether or not any other subtable types are
present, the error mentioned above is reported.
In the cases reported to me personally, the PCCT contains exactly one
subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
acpi_pcc_probe() does not count it as a valid subtable, so believes
there to be no valid subtables, and hence outputs the error message.
An example of the PCCT being reported as erroneous yet perfectly fine
is the following:
Signature : "PCCT"
Table Length : 006E
Revision : 05
Checksum : A9
Oem ID : "XX"
Oem Table ID : "X "
Oem Revision : 2280
Asl Compiler ID : ""
Asl Compiler Revision : 0002
Flags (decoded below) : 0001
Platform : 1
Reserved :
Subtable Type : 00 [Generic Communications Subspace]
Length : 3E
Reserved :
Base Address : DCE43018
Address Length : 1000
Doorbell Register : [Generic Address Structure]
Space ID : 01 [SystemIO]
Bit Width : 08
Bit Offset : 00
Encoded Access Width : 01 [Byte Access:8]
Address : 1842
Preserve Mask : 00FD
Write Mask : 0002
Command Latency : 1388
Maximum Access Rate :
Minimum Turnaround Time :
To fix this, we count up all of the possible subtable types for the
PCCT, and only report an error when there are none (which could mean
either no subtables, or no valid subtables), or there are too many.
We also change the logic so that if there is a valid subtable, we
do try to initialize it per the PCCT subtable contents. This is a
change in functionality; previously, the probe would have returned
right after the error message and would not have tried to use any
other subtable definition.
Tested on my personal laptop which showed the error previously; the
error message no longer appears and the laptop appears to operate
normally.
Signed-off-by: Al Stone <a...@redhat.com>
Cc: Jassi Brar <jassisinghb...@gmail.com>
Cc: Rafael J. Wysocki <r...@rjwysocki.net>
Cc: Len Brown <l...@kernel.org>
---
drivers/mailbox/pcc.c | 81 ---
1 file changed, 38 insertions(+), 43 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3ef7f036ceea..fc3c237daef2 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -373,33 +373,24 @@ static const struct mbox_chan_ops pcc_chan_ops = {
};
/**
- * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
- * entries. There should be one entry per PCC client.
+ * parse_pcc_subspaces -- Count PCC subspaces defined
* @header: Pointer to the ACPI subtable header under the PCCT.
* @end: End of subtable entry.
*
- * Return: 0 for Success, else errno.
+ * Return: If we find a PCC subspace entry of a valid type, return 0.
+ * Otherwise, return -EINVAL.
*
* This gets called for each entry in the PCC table.
*/
static int parse_pcc_subspace(struct acpi_subtable_header *header,
const unsigned long end)
{
- struct acpi_pcct_hw_reduced *pcct_ss;
-
- if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) {
- pcct_ss = (struct acpi_pcct_hw_reduced *) header;
+ struct acpi_pcct_subspace *ss = (struct acpi_pcct_subspace *) header;
- if ((pcct_ss->header.type !=
- ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE)
- && (pcct_ss->header.type !=
- ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
- pr_err("Incorrect PCC Subspace type detected\n");
- return -EINVAL;
- }
- }
+ if (ss->header.type < ACPI_PCCT_TYPE_RESERVED)
+ return 0;
- ret
[PATCH v4 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI, PCCT
There have been multiple reports of the following error message:
[0.068293] Error parsing PCC subspaces from PCCT
This error message is not correct. In multiple cases examined, the PCCT
(Platform Communications Channel Table) concerned is actually properly
constructed; the problem is that acpi_pcc_probe() which reads the PCCT
is making the assumption that the only valid PCCT is one that contains
subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
types are counted and as long as there is at least one of the desired
types, the acpi_pcc_probe() succeeds. When no subtables of these types
are found, regardless of whether or not any other subtable types are
present, the error mentioned above is reported.
In the cases reported to me personally, the PCCT contains exactly one
subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
acpi_pcc_probe() does not count it as a valid subtable, so believes
there to be no valid subtables, and hence outputs the error message.
An example of the PCCT being reported as erroneous yet perfectly fine
is the following:
Signature : "PCCT"
Table Length : 006E
Revision : 05
Checksum : A9
Oem ID : "XX"
Oem Table ID : "X "
Oem Revision : 2280
Asl Compiler ID : ""
Asl Compiler Revision : 0002
Flags (decoded below) : 0001
Platform : 1
Reserved :
Subtable Type : 00 [Generic Communications Subspace]
Length : 3E
Reserved :
Base Address : DCE43018
Address Length : 1000
Doorbell Register : [Generic Address Structure]
Space ID : 01 [SystemIO]
Bit Width : 08
Bit Offset : 00
Encoded Access Width : 01 [Byte Access:8]
Address : 1842
Preserve Mask : 00FD
Write Mask : 0002
Command Latency : 1388
Maximum Access Rate :
Minimum Turnaround Time :
To fix this, we count up all of the possible subtable types for the
PCCT, and only report an error when there are none (which could mean
either no subtables, or no valid subtables), or there are too many.
We also change the logic so that if there is a valid subtable, we
do try to initialize it per the PCCT subtable contents. This is a
change in functionality; previously, the probe would have returned
right after the error message and would not have tried to use any
other subtable definition.
Tested on my personal laptop which showed the error previously; the
error message no longer appears and the laptop appears to operate
normally.
Signed-off-by: Al Stone
Cc: Jassi Brar
Cc: Rafael J. Wysocki
Cc: Len Brown
---
drivers/mailbox/pcc.c | 81 ---
1 file changed, 38 insertions(+), 43 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3ef7f036ceea..fc3c237daef2 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -373,33 +373,24 @@ static const struct mbox_chan_ops pcc_chan_ops = {
};
/**
- * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
- * entries. There should be one entry per PCC client.
+ * parse_pcc_subspaces -- Count PCC subspaces defined
* @header: Pointer to the ACPI subtable header under the PCCT.
* @end: End of subtable entry.
*
- * Return: 0 for Success, else errno.
+ * Return: If we find a PCC subspace entry of a valid type, return 0.
+ * Otherwise, return -EINVAL.
*
* This gets called for each entry in the PCC table.
*/
static int parse_pcc_subspace(struct acpi_subtable_header *header,
const unsigned long end)
{
- struct acpi_pcct_hw_reduced *pcct_ss;
-
- if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) {
- pcct_ss = (struct acpi_pcct_hw_reduced *) header;
+ struct acpi_pcct_subspace *ss = (struct acpi_pcct_subspace *) header;
- if ((pcct_ss->header.type !=
- ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE)
- && (pcct_ss->header.type !=
- ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
- pr_err("Incorrect PCC Subspace type detected\n");
- return -EINVAL;
- }
- }
+ if (ss->header.type < ACPI_PCCT_TYPE_RESERVED)
+ return 0;
- return 0;
+ return -EINVAL;
}
/**
@@ -449,8 +440,8 @@ static int __init acpi_pcc_probe(void)
Re: [PATCH v3 2/3] ACPI: ensure acpi_parse_entries_array() does not access non-existent table data
On 05/15/2018 03:53 PM, Al Stone wrote:
> On 05/15/2018 11:19 AM, Rafael J. Wysocki wrote:
>> On Tue, May 1, 2018 at 2:39 AM, Al Stone <a...@redhat.com> wrote:
>>> For ACPI tables that have subtables, acpi_parse_entries_array() gets used
>>> to step through each of the subtables in memory. The primary loop for this
>>> was checking that the beginning location of the subtable being examined
>>> plus the length of struct acpi_subtable_header was not beyond the end of
>>> the complete ACPI table; if it wasn't, the subtable could be examined, but
>>> if it was the loop would terminate as it should.
>>>
>>> In the middle of this subtable loop, a callback is used to examine the
>>> subtable in detail.
>>>
>>> Should the callback function try to examine elements of the subtable that
>>> are located past the subtable header, and the ACPI table containing this
>>> subtable has an incorrect length, it is possible to access either invalid
>>> or protected memory and cause a fault. And, the length of struct
>>> acpi_subtable_header will always be smaller than the length of the actual
>>> subtable.
>>>
>>> To fix this, we make the main loop check that the beginning of the
>>> subtable being examined plus the actual length of the subtable does
>>> not go past the end of the enclosing ACPI table. While this cannot
>>> protect us from malicious callback functions, it can prevent us from
>>> failing because of some poorly constructed ACPI tables.
>>>
>>> Found by inspection. There is no functional change to existing code
>>> that is known to work when calling acpi_parse_entries_array().
>>>
>>> Signed-off-by: Al Stone <a...@redhat.com>
>>> Cc: Rafael J. Wysocki <r...@rjwysocki.net>
>>> Cc: Len Brown <l...@kernel.org>
>>> ---
>>> drivers/acpi/tables.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>>> index 4a3410aa6540..82c3e2c52dd9 100644
>>> --- a/drivers/acpi/tables.c
>>> +++ b/drivers/acpi/tables.c
>>> @@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long
>>> table_size,
>>> entry = (struct acpi_subtable_header *)
>>> ((unsigned long)table_header + table_size);
>>>
>>> - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header)
>>> <
>>> - table_end) {
>>> + while ((unsigned long)entry + entry->length <= table_end) {
>>> if (max_entries && count >= max_entries)
>>> break;
>>>
>>> --
>>
>> This breaks the CPU enumeration on my Dell XPS13 9360 (possibly among
>> other things), so I'm dropping it. I can send you acpidump output
>> from that machine if need be.
>>
>> Thanks,
>> Rafael
Let's just drop this completely -- but please do send the acpidump. It's
going to take me a while to figure why this innocuous little loop does not
behave the way I expect it to. I'll send a separate patch, if needed.
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH v3 2/3] ACPI: ensure acpi_parse_entries_array() does not access non-existent table data
On 05/15/2018 03:53 PM, Al Stone wrote:
> On 05/15/2018 11:19 AM, Rafael J. Wysocki wrote:
>> On Tue, May 1, 2018 at 2:39 AM, Al Stone wrote:
>>> For ACPI tables that have subtables, acpi_parse_entries_array() gets used
>>> to step through each of the subtables in memory. The primary loop for this
>>> was checking that the beginning location of the subtable being examined
>>> plus the length of struct acpi_subtable_header was not beyond the end of
>>> the complete ACPI table; if it wasn't, the subtable could be examined, but
>>> if it was the loop would terminate as it should.
>>>
>>> In the middle of this subtable loop, a callback is used to examine the
>>> subtable in detail.
>>>
>>> Should the callback function try to examine elements of the subtable that
>>> are located past the subtable header, and the ACPI table containing this
>>> subtable has an incorrect length, it is possible to access either invalid
>>> or protected memory and cause a fault. And, the length of struct
>>> acpi_subtable_header will always be smaller than the length of the actual
>>> subtable.
>>>
>>> To fix this, we make the main loop check that the beginning of the
>>> subtable being examined plus the actual length of the subtable does
>>> not go past the end of the enclosing ACPI table. While this cannot
>>> protect us from malicious callback functions, it can prevent us from
>>> failing because of some poorly constructed ACPI tables.
>>>
>>> Found by inspection. There is no functional change to existing code
>>> that is known to work when calling acpi_parse_entries_array().
>>>
>>> Signed-off-by: Al Stone
>>> Cc: Rafael J. Wysocki
>>> Cc: Len Brown
>>> ---
>>> drivers/acpi/tables.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>>> index 4a3410aa6540..82c3e2c52dd9 100644
>>> --- a/drivers/acpi/tables.c
>>> +++ b/drivers/acpi/tables.c
>>> @@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long
>>> table_size,
>>> entry = (struct acpi_subtable_header *)
>>> ((unsigned long)table_header + table_size);
>>>
>>> - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header)
>>> <
>>> - table_end) {
>>> + while ((unsigned long)entry + entry->length <= table_end) {
>>> if (max_entries && count >= max_entries)
>>> break;
>>>
>>> --
>>
>> This breaks the CPU enumeration on my Dell XPS13 9360 (possibly among
>> other things), so I'm dropping it. I can send you acpidump output
>> from that machine if need be.
>>
>> Thanks,
>> Rafael
Let's just drop this completely -- but please do send the acpidump. It's
going to take me a while to figure why this innocuous little loop does not
behave the way I expect it to. I'll send a separate patch, if needed.
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: linux-next 20180515 - ACPI disabled..
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 05/15/2018 04:25 PM, valdis.kletni...@vt.edu wrote: > On Tue, 15 May 2018 15:49:15 -0600, Al Stone said: > >> Not off-hand. Could you please send me a copy of >> /sys/firmware/acpi/tables/APIC > > cat /sys/firmware/acpi/tables/APIC | od -x 000 5041 4349 0072 > 3903 4544 4c4c 2020 020 4243 3358 2020 0020 2009 0107 4d41 2049 040 > 0013 0001 fee0 0001 0800 0001 060 0001 0800 0202 0001 > 0800 0103 100 0001 0800 0304 0001 0c01 0002 120 > fec0 0a02 0002 140 0a02 0900 0009 000d > 0604 05ff 160 0100 162 Thanks. >> on this machine? The commit cd8c65a6442b that I wrote looks like it got >> pulled in on 20180430, which if I'm understanding correctly, seems to >> have fixed the problem. Did this work before 20180415? I assume it >> did. > > 0430 is what I'm running right ow, and it works. Everything before that > too. It fell over sometime between 0430 and 0515, Oh, I see. I had read "0515" as "0415", hence my confusion. - -- ciao, al - --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com - --- -BEGIN PGP SIGNATURE- iQIzBAEBCAAdFiEEiclaKI7ZGQ+4GJLWUwywAtdhsWwFAlr7Yb0ACgkQUwywAtdh sWwshQ//XvOPtjH5kB/OHR0jGP5LQ+9iiyJa40P6UhsW4dTFZbWLWOvhzqrdBJD2 dyA7Xz3ig4zULS1ZAmIpubMixgekGa9VaMtJMumw4CiwqkuphLNnB6gFqZ5QkWS4 YyGwW6zz2RSONf5Lq3K65mr+6cDHMhdtidtEunXzMOC6Ny8ELC52hIr295NpnyXn sv72SVdy9kQYHfxRv7K1IIbMKJZYRBN3TCoWFFJHbJH0stiLnCk+O+57QcsCKNWH ECJ6z8+kELo3BD42PLk7cRYoUKnyPsrGn+fAQvLD9rnknjVRPKhVjRilI9qr/HCo pmj0N9xNcEI0WK2feyN3vzYPeZbzwsxIiiCwd9E+bQIrwq8nI+fbhV6egFZlbwcR 89w3Zhu31s7umcopLLHAEstm4STKOZGS54PdGQFku/Nff4GVes+vvjP1wC3yMEDL kUeOt5tLWgexEMOdXqo9YaMz2AUeog2Duk1PL27EiHjc4eGbSt1Yb9SgRFln03rH Orozfqjjwv6G4yWkz6sjZ/0T6ODWa+CW8qTr2WjFH2l2sj3ZT3DoemN4GkYo3O7x oI+7lKoY31L+GQnKluW6EceliANVmLlgNhlQv5v9i9NYFrgwj8UucJfJVQdJWVKk uJxhGITkkResHoPOfEuHsJ9Ks5wI22L6RiO9OTX2rr7accsA+aY= =Vj8z -END PGP SIGNATURE-
Re: linux-next 20180515 - ACPI disabled..
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 05/15/2018 04:25 PM, valdis.kletni...@vt.edu wrote: > On Tue, 15 May 2018 15:49:15 -0600, Al Stone said: > >> Not off-hand. Could you please send me a copy of >> /sys/firmware/acpi/tables/APIC > > cat /sys/firmware/acpi/tables/APIC | od -x 000 5041 4349 0072 > 3903 4544 4c4c 2020 020 4243 3358 2020 0020 2009 0107 4d41 2049 040 > 0013 0001 fee0 0001 0800 0001 060 0001 0800 0202 0001 > 0800 0103 100 0001 0800 0304 0001 0c01 0002 120 > fec0 0a02 0002 140 0a02 0900 0009 000d > 0604 05ff 160 0100 162 Thanks. >> on this machine? The commit cd8c65a6442b that I wrote looks like it got >> pulled in on 20180430, which if I'm understanding correctly, seems to >> have fixed the problem. Did this work before 20180415? I assume it >> did. > > 0430 is what I'm running right ow, and it works. Everything before that > too. It fell over sometime between 0430 and 0515, Oh, I see. I had read "0515" as "0415", hence my confusion. - -- ciao, al - --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com - --- -BEGIN PGP SIGNATURE- iQIzBAEBCAAdFiEEiclaKI7ZGQ+4GJLWUwywAtdhsWwFAlr7Yb0ACgkQUwywAtdh sWwshQ//XvOPtjH5kB/OHR0jGP5LQ+9iiyJa40P6UhsW4dTFZbWLWOvhzqrdBJD2 dyA7Xz3ig4zULS1ZAmIpubMixgekGa9VaMtJMumw4CiwqkuphLNnB6gFqZ5QkWS4 YyGwW6zz2RSONf5Lq3K65mr+6cDHMhdtidtEunXzMOC6Ny8ELC52hIr295NpnyXn sv72SVdy9kQYHfxRv7K1IIbMKJZYRBN3TCoWFFJHbJH0stiLnCk+O+57QcsCKNWH ECJ6z8+kELo3BD42PLk7cRYoUKnyPsrGn+fAQvLD9rnknjVRPKhVjRilI9qr/HCo pmj0N9xNcEI0WK2feyN3vzYPeZbzwsxIiiCwd9E+bQIrwq8nI+fbhV6egFZlbwcR 89w3Zhu31s7umcopLLHAEstm4STKOZGS54PdGQFku/Nff4GVes+vvjP1wC3yMEDL kUeOt5tLWgexEMOdXqo9YaMz2AUeog2Duk1PL27EiHjc4eGbSt1Yb9SgRFln03rH Orozfqjjwv6G4yWkz6sjZ/0T6ODWa+CW8qTr2WjFH2l2sj3ZT3DoemN4GkYo3O7x oI+7lKoY31L+GQnKluW6EceliANVmLlgNhlQv5v9i9NYFrgwj8UucJfJVQdJWVKk uJxhGITkkResHoPOfEuHsJ9Ks5wI22L6RiO9OTX2rr7accsA+aY= =Vj8z -END PGP SIGNATURE-
Re: [PATCH v3 2/3] ACPI: ensure acpi_parse_entries_array() does not access non-existent table data
On 05/15/2018 11:19 AM, Rafael J. Wysocki wrote:
> On Tue, May 1, 2018 at 2:39 AM, Al Stone <a...@redhat.com> wrote:
>> For ACPI tables that have subtables, acpi_parse_entries_array() gets used
>> to step through each of the subtables in memory. The primary loop for this
>> was checking that the beginning location of the subtable being examined
>> plus the length of struct acpi_subtable_header was not beyond the end of
>> the complete ACPI table; if it wasn't, the subtable could be examined, but
>> if it was the loop would terminate as it should.
>>
>> In the middle of this subtable loop, a callback is used to examine the
>> subtable in detail.
>>
>> Should the callback function try to examine elements of the subtable that
>> are located past the subtable header, and the ACPI table containing this
>> subtable has an incorrect length, it is possible to access either invalid
>> or protected memory and cause a fault. And, the length of struct
>> acpi_subtable_header will always be smaller than the length of the actual
>> subtable.
>>
>> To fix this, we make the main loop check that the beginning of the
>> subtable being examined plus the actual length of the subtable does
>> not go past the end of the enclosing ACPI table. While this cannot
>> protect us from malicious callback functions, it can prevent us from
>> failing because of some poorly constructed ACPI tables.
>>
>> Found by inspection. There is no functional change to existing code
>> that is known to work when calling acpi_parse_entries_array().
>>
>> Signed-off-by: Al Stone <a...@redhat.com>
>> Cc: Rafael J. Wysocki <r...@rjwysocki.net>
>> Cc: Len Brown <l...@kernel.org>
>> ---
>> drivers/acpi/tables.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>> index 4a3410aa6540..82c3e2c52dd9 100644
>> --- a/drivers/acpi/tables.c
>> +++ b/drivers/acpi/tables.c
>> @@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long
>> table_size,
>> entry = (struct acpi_subtable_header *)
>> ((unsigned long)table_header + table_size);
>>
>> - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>> - table_end) {
>> + while ((unsigned long)entry + entry->length <= table_end) {
>> if (max_entries && count >= max_entries)
>> break;
>>
>> --
>
> This breaks the CPU enumeration on my Dell XPS13 9360 (possibly among
> other things), so I'm dropping it. I can send you acpidump output
> from that machine if need be.
>
> Thanks,
> Rafael
>
Yes, please. My fear is that there are a bunch of MADT tables in the real world
that aren't quite right so that while this may be theoretically correct, it may
be wrong as a practical matter.
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH v3 2/3] ACPI: ensure acpi_parse_entries_array() does not access non-existent table data
On 05/15/2018 11:19 AM, Rafael J. Wysocki wrote:
> On Tue, May 1, 2018 at 2:39 AM, Al Stone wrote:
>> For ACPI tables that have subtables, acpi_parse_entries_array() gets used
>> to step through each of the subtables in memory. The primary loop for this
>> was checking that the beginning location of the subtable being examined
>> plus the length of struct acpi_subtable_header was not beyond the end of
>> the complete ACPI table; if it wasn't, the subtable could be examined, but
>> if it was the loop would terminate as it should.
>>
>> In the middle of this subtable loop, a callback is used to examine the
>> subtable in detail.
>>
>> Should the callback function try to examine elements of the subtable that
>> are located past the subtable header, and the ACPI table containing this
>> subtable has an incorrect length, it is possible to access either invalid
>> or protected memory and cause a fault. And, the length of struct
>> acpi_subtable_header will always be smaller than the length of the actual
>> subtable.
>>
>> To fix this, we make the main loop check that the beginning of the
>> subtable being examined plus the actual length of the subtable does
>> not go past the end of the enclosing ACPI table. While this cannot
>> protect us from malicious callback functions, it can prevent us from
>> failing because of some poorly constructed ACPI tables.
>>
>> Found by inspection. There is no functional change to existing code
>> that is known to work when calling acpi_parse_entries_array().
>>
>> Signed-off-by: Al Stone
>> Cc: Rafael J. Wysocki
>> Cc: Len Brown
>> ---
>> drivers/acpi/tables.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>> index 4a3410aa6540..82c3e2c52dd9 100644
>> --- a/drivers/acpi/tables.c
>> +++ b/drivers/acpi/tables.c
>> @@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long
>> table_size,
>> entry = (struct acpi_subtable_header *)
>> ((unsigned long)table_header + table_size);
>>
>> - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>> - table_end) {
>> + while ((unsigned long)entry + entry->length <= table_end) {
>> if (max_entries && count >= max_entries)
>> break;
>>
>> --
>
> This breaks the CPU enumeration on my Dell XPS13 9360 (possibly among
> other things), so I'm dropping it. I can send you acpidump output
> from that machine if need be.
>
> Thanks,
> Rafael
>
Yes, please. My fear is that there are a bunch of MADT tables in the real world
that aren't quite right so that while this may be theoretically correct, it may
be wrong as a practical matter.
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: linux-next 20180515 - ACPI disabled..
On 05/15/2018 03:23 PM, Andy Shevchenko wrote: > +Cc: Rafael > > On Wed, May 16, 2018 at 12:18 AM, <valdis.kletni...@vt.edu> wrote: >> Seeing this at boot with linux-next 20180415. ACPI gets disabled, hilarity >> and hijinks >> result - everything from a lot of stuff can't find an IRQ to the dual-core >> w/ HT CPU >> coming up as just 1 core no HT. 20180430 works just fine... >> >> [0.00] ACPI: Early table checksum verification disabled >> [0.00] ACPI: RSDP 0xCB7F1000 24 (v02 DELL ) >> [0.00] ACPI: XSDT 0xCB7F1088 94 (v01 DELL CBX3 >> 01072009 AMI 00010013) >> [0.00] ACPI: FACP 0xCB7FB720 00010C (v05 DELL CBX3 >> 01072009 AMI 00010013) >> [0.00] ACPI: DSDT 0xCB7F11B0 00A56F (v02 DELL CBX3 >> 0021 INTL 20091112) >> [0.00] ACPI: FACS 0xCCFED080 40 >> [0.00] ACPI: APIC 0xCB7FB830 72 (v03 DELL CBX3 >> 01072009 AMI 00010013) >> [0.00] ACPI: FPDT 0xCB7FB8A8 44 (v01 DELL CBX3 >> 01072009 AMI 00010013) >> [0.00] ACPI: TCPA 0xCB7FB8F0 32 (v02 APTIO4 NAPAASF >> 0001 MSFT 0113) >> [0.00] ACPI: MCFG 0xCB7FB928 3C (v01 DELL CBX3 >> 01072009 MSFT 0097) >> [0.00] ACPI: HPET 0xCB7FB968 38 (v01 DELL CBX3 >> 01072009 AMI. 0005) >> [0.00] ACPI: SSDT 0xCB7FB9A0 000415 (v01 SataRe SataTabl >> 1000 INTL 20091112) >> [0.00] ACPI: SSDT 0xCB7FBDB8 0009B9 (v01 PmRef Cpu0Ist >> 3000 INTL 20051117) >> [0.00] ACPI: SSDT 0xCB7FC778 000B22 (v01 PmRef CpuPm >> 3000 INTL 20051117) >> [0.00] ACPI: DMAR 0xCB7FD2A0 80 (v01 INTEL SNB >> 0001 INTL 0001) >> [0.00] ACPI: ASF! 0xCB7FD320 A5 (v32 INTEL HCG >> 0001 TFSM 000F4240) >> [0.00] ACPI: SSDT 0xCB7FD3C8 000579 (v01 AMITCG PROC >> 0001 INTL 20051117) >> [0.00] ACPI: BGRT 0xCB7FD948 38 (v00 ?? >> 01072009 AMI 00010013) >> [0.00] ACPI: SSDT 0xCB7FD980 00198A (v01 NvdRef NvdTabl >> 1000 INTL 20091112) >> [0.00] ACPI: Local APIC address 0xfee0 >> [0.00] ACPI: [APIC:0x05] Invalid zero length >> [0.00] ACPI: Error parsing LAPIC address override entry >> [0.00] ACPI: Invalid BIOS MADT, disabling ACPI >> [0.00] tsc: Fast TSC calibration using PIT >> >> On next-20180430, the boot goes like this, and continues properly: >> >> [0.00] ACPI: SSDT 0xCB7FD980 00198A (v01 NvdRef NvdTabl >> 1000 INTL 20091112) >> [0.00] ACPI: Local APIC address 0xfee0 >> [0.00] tsc: Fast TSC calibration using PIT >> >> Not seeing any commits to drivers/acpi/tables.c other than Al Stone's. >> >> This ringing any bells before I go bisecting? >> > > > Not off-hand. Could you please send me a copy of /sys/firmware/acpi/tables/APIC on this machine? The commit cd8c65a6442b that I wrote looks like it got pulled in on 20180430, which if I'm understanding correctly, seems to have fixed the problem. Did this work before 20180415? I assume it did. What puzzles me is that this message: ACPI: [APIC:0x05] Invalid zero length should only have shown up if the MADT has a broken subtable, and I think that bit of code has been that way for quite some time (git blame says somewhere around 2012 when the test for this condition was put in). -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: linux-next 20180515 - ACPI disabled..
On 05/15/2018 03:23 PM, Andy Shevchenko wrote: > +Cc: Rafael > > On Wed, May 16, 2018 at 12:18 AM, wrote: >> Seeing this at boot with linux-next 20180415. ACPI gets disabled, hilarity >> and hijinks >> result - everything from a lot of stuff can't find an IRQ to the dual-core >> w/ HT CPU >> coming up as just 1 core no HT. 20180430 works just fine... >> >> [0.00] ACPI: Early table checksum verification disabled >> [0.00] ACPI: RSDP 0xCB7F1000 24 (v02 DELL ) >> [0.00] ACPI: XSDT 0xCB7F1088 94 (v01 DELL CBX3 >> 01072009 AMI 00010013) >> [0.00] ACPI: FACP 0xCB7FB720 00010C (v05 DELL CBX3 >> 01072009 AMI 00010013) >> [0.00] ACPI: DSDT 0xCB7F11B0 00A56F (v02 DELL CBX3 >> 0021 INTL 20091112) >> [0.00] ACPI: FACS 0xCCFED080 40 >> [0.00] ACPI: APIC 0xCB7FB830 72 (v03 DELL CBX3 >> 01072009 AMI 00010013) >> [0.00] ACPI: FPDT 0xCB7FB8A8 44 (v01 DELL CBX3 >> 01072009 AMI 00010013) >> [0.00] ACPI: TCPA 0xCB7FB8F0 32 (v02 APTIO4 NAPAASF >> 0001 MSFT 0113) >> [0.00] ACPI: MCFG 0xCB7FB928 3C (v01 DELL CBX3 >> 01072009 MSFT 0097) >> [0.00] ACPI: HPET 0xCB7FB968 38 (v01 DELL CBX3 >> 01072009 AMI. 0005) >> [0.00] ACPI: SSDT 0xCB7FB9A0 000415 (v01 SataRe SataTabl >> 1000 INTL 20091112) >> [0.00] ACPI: SSDT 0xCB7FBDB8 0009B9 (v01 PmRef Cpu0Ist >> 3000 INTL 20051117) >> [0.00] ACPI: SSDT 0xCB7FC778 000B22 (v01 PmRef CpuPm >> 3000 INTL 20051117) >> [0.00] ACPI: DMAR 0xCB7FD2A0 80 (v01 INTEL SNB >> 0001 INTL 0001) >> [0.00] ACPI: ASF! 0xCB7FD320 A5 (v32 INTEL HCG >> 0001 TFSM 000F4240) >> [0.00] ACPI: SSDT 0xCB7FD3C8 000579 (v01 AMITCG PROC >> 0001 INTL 20051117) >> [0.00] ACPI: BGRT 0xCB7FD948 38 (v00 ?? >> 01072009 AMI 00010013) >> [0.00] ACPI: SSDT 0xCB7FD980 00198A (v01 NvdRef NvdTabl >> 1000 INTL 20091112) >> [0.00] ACPI: Local APIC address 0xfee0 >> [0.00] ACPI: [APIC:0x05] Invalid zero length >> [0.00] ACPI: Error parsing LAPIC address override entry >> [0.00] ACPI: Invalid BIOS MADT, disabling ACPI >> [0.00] tsc: Fast TSC calibration using PIT >> >> On next-20180430, the boot goes like this, and continues properly: >> >> [0.00] ACPI: SSDT 0xCB7FD980 00198A (v01 NvdRef NvdTabl >> 1000 INTL 20091112) >> [0.00] ACPI: Local APIC address 0xfee0 >> [0.00] tsc: Fast TSC calibration using PIT >> >> Not seeing any commits to drivers/acpi/tables.c other than Al Stone's. >> >> This ringing any bells before I go bisecting? >> > > > Not off-hand. Could you please send me a copy of /sys/firmware/acpi/tables/APIC on this machine? The commit cd8c65a6442b that I wrote looks like it got pulled in on 20180430, which if I'm understanding correctly, seems to have fixed the problem. Did this work before 20180415? I assume it did. What puzzles me is that this message: ACPI: [APIC:0x05] Invalid zero length should only have shown up if the MADT has a broken subtable, and I think that bit of code has been that way for quite some time (git blame says somewhere around 2012 when the test for this condition was put in). -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH v3 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
On 05/14/2018 03:04 PM, Prakash, Prashanth wrote:
>
>
> On 4/30/2018 6:39 PM, Al Stone wrote:
>> There have been multiple reports of the following error message:
>>
>> [0.068293] Error parsing PCC subspaces from PCCT
>>
>> This error message is not correct. In multiple cases examined, the PCCT
>> (Platform Communications Channel Table) concerned is actually properly
>> constructed; the problem is that acpi_pcc_probe() which reads the PCCT
>> is making the assumption that the only valid PCCT is one that contains
>> subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
>> ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
>> types are counted and as long as there is at least one of the desired
>> types, the acpi_pcc_probe() succeeds. When no subtables of these types
>> are found, regardless of whether or not any other subtable types are
>> present, the error mentioned above is reported.
>>
>> In the cases reported to me personally, the PCCT contains exactly one
>> subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
>> acpi_pcc_probe() does not count it as a valid subtable, so believes
>> there to be no valid subtables, and hence outputs the error message.
>>
>> An example of the PCCT being reported as erroneous yet perfectly fine
>> is the following:
>>
>> Signature : "PCCT"
>> Table Length : 006E
>> Revision : 05
>> Checksum : A9
>>Oem ID : "XX"
>> Oem Table ID : "X "
>> Oem Revision : 2280
>> Asl Compiler ID : ""
>> Asl Compiler Revision : 0002
>>
>> Flags (decoded below) : 0001
>> Platform : 1
>> Reserved :
>>
>> Subtable Type : 00 [Generic Communications Subspace]
>>Length : 3E
>>
>> Reserved :
>> Base Address : DCE43018
>>Address Length : 1000
>>
>> Doorbell Register : [Generic Address Structure]
>> Space ID : 01 [SystemIO]
>> Bit Width : 08
>>Bit Offset : 00
>> Encoded Access Width : 01 [Byte Access:8]
>> Address : 1842
>>
>> Preserve Mask : 00FD
>>Write Mask : 0002
>> Command Latency : 1388
>> Maximum Access Rate :
>> Minimum Turnaround Time :
>>
>> To fix this, we count up all of the possible subtable types for the
>> PCCT, and only report an error when there are none (which could mean
>> either no subtables, or no valid subtables), or there are too many.
>> We also change the logic so that if there is a valid subtable, we
>> do try to initialize it per the PCCT subtable contents. This is a
>> change in functionality; previously, the probe would have returned
>> right after the error message and would not have tried to use any
>> other subtable definition.
>>
>> Tested on my personal laptop which showed the error previously; the
>> error message no longer appears and the laptop appears to operate
>> normally.
>>
>> Signed-off-by: Al Stone <a...@redhat.com>
>> Cc: Jassi Brar <jassisinghb...@gmail.com>
>> Cc: Rafael J. Wysocki <r...@rjwysocki.net>
>> Cc: Len Brown <l...@kernel.org>
>> ---
>> drivers/mailbox/pcc.c | 96
>> +--
>> 1 file changed, 63 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 3ef7f036ceea..72af37d7e95e 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -372,13 +372,37 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>> .send_data = pcc_send_data,
>> };
>>
>> +/*
>> + *
>> + * count_pcc_subspaces -- Count PCC subspaces not used in reduced HW
>> systems.
>> + * @header: Pointer to the ACPI subtable header under the PCCT.
>> + * @end: End of subtable entry.
>> + *
>> + * Return: If we find a PCC subspace entry that is one of the types used
>> + * in reduced hardware systems, return -EINVAL. Otherwise, return 0.
>> + *
>> + * This gets called for e
Re: [PATCH v3 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
On 05/14/2018 03:04 PM, Prakash, Prashanth wrote:
>
>
> On 4/30/2018 6:39 PM, Al Stone wrote:
>> There have been multiple reports of the following error message:
>>
>> [0.068293] Error parsing PCC subspaces from PCCT
>>
>> This error message is not correct. In multiple cases examined, the PCCT
>> (Platform Communications Channel Table) concerned is actually properly
>> constructed; the problem is that acpi_pcc_probe() which reads the PCCT
>> is making the assumption that the only valid PCCT is one that contains
>> subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
>> ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
>> types are counted and as long as there is at least one of the desired
>> types, the acpi_pcc_probe() succeeds. When no subtables of these types
>> are found, regardless of whether or not any other subtable types are
>> present, the error mentioned above is reported.
>>
>> In the cases reported to me personally, the PCCT contains exactly one
>> subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
>> acpi_pcc_probe() does not count it as a valid subtable, so believes
>> there to be no valid subtables, and hence outputs the error message.
>>
>> An example of the PCCT being reported as erroneous yet perfectly fine
>> is the following:
>>
>> Signature : "PCCT"
>> Table Length : 006E
>> Revision : 05
>> Checksum : A9
>>Oem ID : "XX"
>> Oem Table ID : "X "
>> Oem Revision : 2280
>> Asl Compiler ID : ""
>> Asl Compiler Revision : 0002
>>
>> Flags (decoded below) : 0001
>> Platform : 1
>> Reserved :
>>
>> Subtable Type : 00 [Generic Communications Subspace]
>>Length : 3E
>>
>> Reserved :
>> Base Address : DCE43018
>>Address Length : 1000
>>
>> Doorbell Register : [Generic Address Structure]
>> Space ID : 01 [SystemIO]
>> Bit Width : 08
>>Bit Offset : 00
>> Encoded Access Width : 01 [Byte Access:8]
>> Address : 1842
>>
>> Preserve Mask : 00FD
>>Write Mask : 0002
>> Command Latency : 1388
>> Maximum Access Rate :
>> Minimum Turnaround Time :
>>
>> To fix this, we count up all of the possible subtable types for the
>> PCCT, and only report an error when there are none (which could mean
>> either no subtables, or no valid subtables), or there are too many.
>> We also change the logic so that if there is a valid subtable, we
>> do try to initialize it per the PCCT subtable contents. This is a
>> change in functionality; previously, the probe would have returned
>> right after the error message and would not have tried to use any
>> other subtable definition.
>>
>> Tested on my personal laptop which showed the error previously; the
>> error message no longer appears and the laptop appears to operate
>> normally.
>>
>> Signed-off-by: Al Stone
>> Cc: Jassi Brar
>> Cc: Rafael J. Wysocki
>> Cc: Len Brown
>> ---
>> drivers/mailbox/pcc.c | 96
>> +--
>> 1 file changed, 63 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 3ef7f036ceea..72af37d7e95e 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -372,13 +372,37 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>> .send_data = pcc_send_data,
>> };
>>
>> +/*
>> + *
>> + * count_pcc_subspaces -- Count PCC subspaces not used in reduced HW
>> systems.
>> + * @header: Pointer to the ACPI subtable header under the PCCT.
>> + * @end: End of subtable entry.
>> + *
>> + * Return: If we find a PCC subspace entry that is one of the types used
>> + * in reduced hardware systems, return -EINVAL. Otherwise, return 0.
>> + *
>> + * This gets called for each subtable in the PCC table.
>> + */
>> +static int count_pcc_subspaces(struct acpi_sub
[PATCH v3 1/3] ACPI: improve function documentation for acpi_parse_entries_array()
I found the description of the table_size argument to the function acpi_parse_entries_array() unclear and ambiguous. This is a minor documentation change to improve that description so I don't misuse the argument again in the future, and it is hopefully clearer to other future users. Signed-off-by: Al Stone <a...@redhat.com> Cc: Rafael J. Wysocki <r...@rjwysocki.net> Cc: Len Brown <l...@kernel.org> --- drivers/acpi/tables.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 849c4fb19b03..4a3410aa6540 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -222,7 +222,7 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) * acpi_parse_entries_array - for each proc_num find a suitable subtable * * @id: table id (for debugging purposes) - * @table_size: single entry size + * @table_size: size of the root table * @table_header: where does the table start? * @proc: array of acpi_subtable_proc struct containing entry id *and associated handler with it @@ -233,6 +233,11 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) * on it. Assumption is that there's only single handler for particular * entry id. * + * The table_size is not the size of the complete ACPI table (the length + * field in the header struct), but only the size of the root table; i.e., + * the offset from the very first byte of the complete ACPI table, to the + * first byte of the very first subtable. + * * On success returns sum of all matching entries for all proc handlers. * Otherwise, -ENODEV or -EINVAL is returned. */ @@ -400,7 +405,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler) return -ENODEV; } -/* +/* * The BIOS is supposed to supply a single APIC/MADT, * but some report two. Provide a knob to use either. * (don't you wish instance 0 and 1 were not the same?) -- 2.14.3
[PATCH v3 1/3] ACPI: improve function documentation for acpi_parse_entries_array()
I found the description of the table_size argument to the function acpi_parse_entries_array() unclear and ambiguous. This is a minor documentation change to improve that description so I don't misuse the argument again in the future, and it is hopefully clearer to other future users. Signed-off-by: Al Stone Cc: Rafael J. Wysocki Cc: Len Brown --- drivers/acpi/tables.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 849c4fb19b03..4a3410aa6540 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -222,7 +222,7 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) * acpi_parse_entries_array - for each proc_num find a suitable subtable * * @id: table id (for debugging purposes) - * @table_size: single entry size + * @table_size: size of the root table * @table_header: where does the table start? * @proc: array of acpi_subtable_proc struct containing entry id *and associated handler with it @@ -233,6 +233,11 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) * on it. Assumption is that there's only single handler for particular * entry id. * + * The table_size is not the size of the complete ACPI table (the length + * field in the header struct), but only the size of the root table; i.e., + * the offset from the very first byte of the complete ACPI table, to the + * first byte of the very first subtable. + * * On success returns sum of all matching entries for all proc handlers. * Otherwise, -ENODEV or -EINVAL is returned. */ @@ -400,7 +405,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler) return -ENODEV; } -/* +/* * The BIOS is supposed to supply a single APIC/MADT, * but some report two. Provide a knob to use either. * (don't you wish instance 0 and 1 were not the same?) -- 2.14.3
[PATCH v3 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
There have been multiple reports of the following error message:
[0.068293] Error parsing PCC subspaces from PCCT
This error message is not correct. In multiple cases examined, the PCCT
(Platform Communications Channel Table) concerned is actually properly
constructed; the problem is that acpi_pcc_probe() which reads the PCCT
is making the assumption that the only valid PCCT is one that contains
subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
types are counted and as long as there is at least one of the desired
types, the acpi_pcc_probe() succeeds. When no subtables of these types
are found, regardless of whether or not any other subtable types are
present, the error mentioned above is reported.
In the cases reported to me personally, the PCCT contains exactly one
subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
acpi_pcc_probe() does not count it as a valid subtable, so believes
there to be no valid subtables, and hence outputs the error message.
An example of the PCCT being reported as erroneous yet perfectly fine
is the following:
Signature : "PCCT"
Table Length : 006E
Revision : 05
Checksum : A9
Oem ID : "XX"
Oem Table ID : "X "
Oem Revision : 2280
Asl Compiler ID : ""
Asl Compiler Revision : 0002
Flags (decoded below) : 0001
Platform : 1
Reserved :
Subtable Type : 00 [Generic Communications Subspace]
Length : 3E
Reserved :
Base Address : DCE43018
Address Length : 1000
Doorbell Register : [Generic Address Structure]
Space ID : 01 [SystemIO]
Bit Width : 08
Bit Offset : 00
Encoded Access Width : 01 [Byte Access:8]
Address : 1842
Preserve Mask : 00FD
Write Mask : 0002
Command Latency : 1388
Maximum Access Rate :
Minimum Turnaround Time :
To fix this, we count up all of the possible subtable types for the
PCCT, and only report an error when there are none (which could mean
either no subtables, or no valid subtables), or there are too many.
We also change the logic so that if there is a valid subtable, we
do try to initialize it per the PCCT subtable contents. This is a
change in functionality; previously, the probe would have returned
right after the error message and would not have tried to use any
other subtable definition.
Tested on my personal laptop which showed the error previously; the
error message no longer appears and the laptop appears to operate
normally.
Signed-off-by: Al Stone <a...@redhat.com>
Cc: Jassi Brar <jassisinghb...@gmail.com>
Cc: Rafael J. Wysocki <r...@rjwysocki.net>
Cc: Len Brown <l...@kernel.org>
---
drivers/mailbox/pcc.c | 96 +--
1 file changed, 63 insertions(+), 33 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3ef7f036ceea..72af37d7e95e 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -372,13 +372,37 @@ static const struct mbox_chan_ops pcc_chan_ops = {
.send_data = pcc_send_data,
};
+/*
+ *
+ * count_pcc_subspaces -- Count PCC subspaces not used in reduced HW systems.
+ * @header: Pointer to the ACPI subtable header under the PCCT.
+ * @end: End of subtable entry.
+ *
+ * Return: If we find a PCC subspace entry that is one of the types used
+ * in reduced hardware systems, return -EINVAL. Otherwise, return 0.
+ *
+ * This gets called for each subtable in the PCC table.
+ */
+static int count_pcc_subspaces(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_pcct_subspace *pcct_ss = (struct acpi_pcct_subspace *)
header;
+
+ if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
+ (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
+ (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
+ return 0;
+
+ return -EINVAL;
+}
+
/**
- * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
- * entries. There should be one entry per PCC client.
+ * parse_pcc_subspaces -- Count PCC subspaces used only in reduced HW systems.
* @header: Pointer to the ACPI subtable header under the PCCT.
* @end: End of subtable entry.
*
- * Return: 0 for Success, else errno.
+ * Return: If we find a PCC subspace entry that is one
[PATCH v3 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
There have been multiple reports of the following error message:
[0.068293] Error parsing PCC subspaces from PCCT
This error message is not correct. In multiple cases examined, the PCCT
(Platform Communications Channel Table) concerned is actually properly
constructed; the problem is that acpi_pcc_probe() which reads the PCCT
is making the assumption that the only valid PCCT is one that contains
subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
types are counted and as long as there is at least one of the desired
types, the acpi_pcc_probe() succeeds. When no subtables of these types
are found, regardless of whether or not any other subtable types are
present, the error mentioned above is reported.
In the cases reported to me personally, the PCCT contains exactly one
subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
acpi_pcc_probe() does not count it as a valid subtable, so believes
there to be no valid subtables, and hence outputs the error message.
An example of the PCCT being reported as erroneous yet perfectly fine
is the following:
Signature : "PCCT"
Table Length : 006E
Revision : 05
Checksum : A9
Oem ID : "XX"
Oem Table ID : "X "
Oem Revision : 2280
Asl Compiler ID : ""
Asl Compiler Revision : 0002
Flags (decoded below) : 0001
Platform : 1
Reserved :
Subtable Type : 00 [Generic Communications Subspace]
Length : 3E
Reserved :
Base Address : DCE43018
Address Length : 1000
Doorbell Register : [Generic Address Structure]
Space ID : 01 [SystemIO]
Bit Width : 08
Bit Offset : 00
Encoded Access Width : 01 [Byte Access:8]
Address : 1842
Preserve Mask : 00FD
Write Mask : 0002
Command Latency : 1388
Maximum Access Rate :
Minimum Turnaround Time :
To fix this, we count up all of the possible subtable types for the
PCCT, and only report an error when there are none (which could mean
either no subtables, or no valid subtables), or there are too many.
We also change the logic so that if there is a valid subtable, we
do try to initialize it per the PCCT subtable contents. This is a
change in functionality; previously, the probe would have returned
right after the error message and would not have tried to use any
other subtable definition.
Tested on my personal laptop which showed the error previously; the
error message no longer appears and the laptop appears to operate
normally.
Signed-off-by: Al Stone
Cc: Jassi Brar
Cc: Rafael J. Wysocki
Cc: Len Brown
---
drivers/mailbox/pcc.c | 96 +--
1 file changed, 63 insertions(+), 33 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3ef7f036ceea..72af37d7e95e 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -372,13 +372,37 @@ static const struct mbox_chan_ops pcc_chan_ops = {
.send_data = pcc_send_data,
};
+/*
+ *
+ * count_pcc_subspaces -- Count PCC subspaces not used in reduced HW systems.
+ * @header: Pointer to the ACPI subtable header under the PCCT.
+ * @end: End of subtable entry.
+ *
+ * Return: If we find a PCC subspace entry that is one of the types used
+ * in reduced hardware systems, return -EINVAL. Otherwise, return 0.
+ *
+ * This gets called for each subtable in the PCC table.
+ */
+static int count_pcc_subspaces(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_pcct_subspace *pcct_ss = (struct acpi_pcct_subspace *)
header;
+
+ if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
+ (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
+ (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
+ return 0;
+
+ return -EINVAL;
+}
+
/**
- * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
- * entries. There should be one entry per PCC client.
+ * parse_pcc_subspaces -- Count PCC subspaces used only in reduced HW systems.
* @header: Pointer to the ACPI subtable header under the PCCT.
* @end: End of subtable entry.
*
- * Return: 0 for Success, else errno.
+ * Return: If we find a PCC subspace entry that is one of the types used
+ * in reduced hardware systems, return 0. Otherwise, return -EINVAL.
*
* This
[PATCH v3 0/3] mailbox: ACPI: Remove incorrect error message about parsing PCCT
This set of patches provide some cleanup in ACPI for minor issues found while correcting a bogus error message (the first two patches), and the correction for the error message itself (patch 3/3). Note that patches 1/3 and 2/3 are not required for 3/3 to work: 1/3 only changes a comment and 2/3 makes an ACPI table parsing loop a wee bit more robust. For patch 3/3, many systems on boot have been reporting "Error parsing PCC subspaces from PCCT" which turns out to not be an error at all. The issue is that the probe for ACPI mailboxes defined in the PCCT (Platform Communications Channel Table) makes a faulty assumption about the content of the PCCT. What's more, when the error is reported, no further PCC mailboxes are set up, even when they have been defined in the PCCT. So, in the reported cases, there was no error and the data in the PCCT was being ignored. This is described in more detail in patch 3/3. Since these patches primarily involve ACPI usages, it may make sense for all of them to go through the linux-acpi tree; clearly, this is up to the maintainers, though. v3: -- properly format docbook info in patch 1/3 -- remove extra parens in patch 2/3 -- clean up formatting, remove pr_warn() calls used in debugging but providing no value, clean up docbook info for count_pcc_subspaces() and parse_pcc_subspaces(), all in patch 3/3 v2: -- removed one extraneous '+' in a comment in patch 3/3 -- fixed an if test that had a predicate that kbuild pointed out would always be zero Al Stone (3): ACPI: improve function documentation for acpi_parse_entries_array() ACPI: ensure acpi_parse_entries_array() does not access non-existent table data mailbox: ACPI: erroneous error message when parsing the ACPI PCCT drivers/acpi/tables.c | 12 --- drivers/mailbox/pcc.c | 96 +-- 2 files changed, 71 insertions(+), 37 deletions(-) -- 2.14.3
[PATCH v3 2/3] ACPI: ensure acpi_parse_entries_array() does not access non-existent table data
For ACPI tables that have subtables, acpi_parse_entries_array() gets used
to step through each of the subtables in memory. The primary loop for this
was checking that the beginning location of the subtable being examined
plus the length of struct acpi_subtable_header was not beyond the end of
the complete ACPI table; if it wasn't, the subtable could be examined, but
if it was the loop would terminate as it should.
In the middle of this subtable loop, a callback is used to examine the
subtable in detail.
Should the callback function try to examine elements of the subtable that
are located past the subtable header, and the ACPI table containing this
subtable has an incorrect length, it is possible to access either invalid
or protected memory and cause a fault. And, the length of struct
acpi_subtable_header will always be smaller than the length of the actual
subtable.
To fix this, we make the main loop check that the beginning of the
subtable being examined plus the actual length of the subtable does
not go past the end of the enclosing ACPI table. While this cannot
protect us from malicious callback functions, it can prevent us from
failing because of some poorly constructed ACPI tables.
Found by inspection. There is no functional change to existing code
that is known to work when calling acpi_parse_entries_array().
Signed-off-by: Al Stone <a...@redhat.com>
Cc: Rafael J. Wysocki <r...@rjwysocki.net>
Cc: Len Brown <l...@kernel.org>
---
drivers/acpi/tables.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 4a3410aa6540..82c3e2c52dd9 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
entry = (struct acpi_subtable_header *)
((unsigned long)table_header + table_size);
- while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
- table_end) {
+ while ((unsigned long)entry + entry->length <= table_end) {
if (max_entries && count >= max_entries)
break;
--
2.14.3
[PATCH v3 0/3] mailbox: ACPI: Remove incorrect error message about parsing PCCT
This set of patches provide some cleanup in ACPI for minor issues found while correcting a bogus error message (the first two patches), and the correction for the error message itself (patch 3/3). Note that patches 1/3 and 2/3 are not required for 3/3 to work: 1/3 only changes a comment and 2/3 makes an ACPI table parsing loop a wee bit more robust. For patch 3/3, many systems on boot have been reporting "Error parsing PCC subspaces from PCCT" which turns out to not be an error at all. The issue is that the probe for ACPI mailboxes defined in the PCCT (Platform Communications Channel Table) makes a faulty assumption about the content of the PCCT. What's more, when the error is reported, no further PCC mailboxes are set up, even when they have been defined in the PCCT. So, in the reported cases, there was no error and the data in the PCCT was being ignored. This is described in more detail in patch 3/3. Since these patches primarily involve ACPI usages, it may make sense for all of them to go through the linux-acpi tree; clearly, this is up to the maintainers, though. v3: -- properly format docbook info in patch 1/3 -- remove extra parens in patch 2/3 -- clean up formatting, remove pr_warn() calls used in debugging but providing no value, clean up docbook info for count_pcc_subspaces() and parse_pcc_subspaces(), all in patch 3/3 v2: -- removed one extraneous '+' in a comment in patch 3/3 -- fixed an if test that had a predicate that kbuild pointed out would always be zero Al Stone (3): ACPI: improve function documentation for acpi_parse_entries_array() ACPI: ensure acpi_parse_entries_array() does not access non-existent table data mailbox: ACPI: erroneous error message when parsing the ACPI PCCT drivers/acpi/tables.c | 12 --- drivers/mailbox/pcc.c | 96 +-- 2 files changed, 71 insertions(+), 37 deletions(-) -- 2.14.3
[PATCH v3 2/3] ACPI: ensure acpi_parse_entries_array() does not access non-existent table data
For ACPI tables that have subtables, acpi_parse_entries_array() gets used
to step through each of the subtables in memory. The primary loop for this
was checking that the beginning location of the subtable being examined
plus the length of struct acpi_subtable_header was not beyond the end of
the complete ACPI table; if it wasn't, the subtable could be examined, but
if it was the loop would terminate as it should.
In the middle of this subtable loop, a callback is used to examine the
subtable in detail.
Should the callback function try to examine elements of the subtable that
are located past the subtable header, and the ACPI table containing this
subtable has an incorrect length, it is possible to access either invalid
or protected memory and cause a fault. And, the length of struct
acpi_subtable_header will always be smaller than the length of the actual
subtable.
To fix this, we make the main loop check that the beginning of the
subtable being examined plus the actual length of the subtable does
not go past the end of the enclosing ACPI table. While this cannot
protect us from malicious callback functions, it can prevent us from
failing because of some poorly constructed ACPI tables.
Found by inspection. There is no functional change to existing code
that is known to work when calling acpi_parse_entries_array().
Signed-off-by: Al Stone
Cc: Rafael J. Wysocki
Cc: Len Brown
---
drivers/acpi/tables.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 4a3410aa6540..82c3e2c52dd9 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
entry = (struct acpi_subtable_header *)
((unsigned long)table_header + table_size);
- while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
- table_end) {
+ while ((unsigned long)entry + entry->length <= table_end) {
if (max_entries && count >= max_entries)
break;
--
2.14.3
Re: [PATCH v2 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
On 04/27/2018 05:16 AM, Rafael J. Wysocki wrote:
> On Tue, Apr 24, 2018 at 9:35 PM, Al Stone <a...@redhat.com> wrote:
>> There have been multiple reports of the following error message:
>>
>> [0.068293] Error parsing PCC subspaces from PCCT
>>
>
> [cut]
>
>>
>> Tested on my personal laptop which showed the error previously; the
>> error message no longer appears and the laptop appears to operate
>> normally.
>>
>> Signed-off-by: Al Stone <a...@redhat.com>
>> Cc: Jassi Brar <jassisinghb...@gmail.com>
>> Cc: Rafael J. Wysocki <r...@rjwysocki.net>
>> Cc: Len Brown <l...@kernel.org>
>> ---
>> drivers/mailbox/pcc.c | 99
>> +--
>> 1 file changed, 73 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 3ef7f036ceea..cc77a2f1694f 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -372,6 +372,35 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>> .send_data = pcc_send_data,
>> };
>>
>> +/*
>> + *
>> + * count_pcc_subspaces -- Count the PCC subspaces that are not used in
>> + * reduced hardware systems.
>
> One line here, please.
Ack.
>> + * @header: Pointer to the ACPI subtable header under the PCCT.
>> + * @end: End of subtable entry.
>> + *
>> + * Return: 0 for Success, else errno.
>
> The only errno returned appears to be -EINVAL, so maybe say when that
> is returned and then "or 0 otherwise".
Okay.
>> + *
>> + * This gets called for each entry in the PCC table.
>> + */
>> +static int count_pcc_subspaces(struct acpi_subtable_header *header,
>> + const unsigned long end)
>> +{
>> + struct acpi_pcct_subspace *pcct_ss =
>> + (struct acpi_pcct_subspace *) header;
>
> This can be one line (ignore warnings from checkpatch about that).
Okay. That's always a bit of a judgment call with checkpatch.
>> +
>> + if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
>> + (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
>> + (pcct_ss->header.type !=
>> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
>
> This can be one line too.
Okay.
>> + pr_warn("PCCT count: useful subtype = %d\n",
>> + pcct_ss->header.type);
>
> Why pr_warn()?
>
>> + return 0;
>> + }
>> + pr_warn("PCCT count: unwanted subtype = %d\n", pcct_ss->header.type);
>> + return -EINVAL;
>> +}
>> +
Oh, bugger. I inadvertently left some debug noise in here. Both of the above
pr_warn() calls should go away since they don't actually provide any value.
>> /**
>> * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
>> * entries. There should be one entry per PCC client.
>> @@ -395,10 +424,14 @@ static int parse_pcc_subspace(struct
>> acpi_subtable_header *header,
>> && (pcct_ss->header.type !=
>> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
>> pr_err("Incorrect PCC Subspace type detected\n");
>> + pr_warn("PCCT parse: unwanted subtype = %d\n",
>> + pcct_ss->header.type);
>
> Maybe merge the two messages?
Yup, or remove the pr_warn(); that's really just extra debug info I had put in
to check results.
>
>> return -EINVAL;
>> }
>> }
>>
>> + pcct_ss = (struct acpi_pcct_hw_reduced *) header;
>> + pr_warn("PCCT parse: useful subtype = %d\n", pcct_ss->header.type);
...and same here -- the pr_warn() is extraneous, really.
>> return 0;
>> }
>>
>> @@ -449,8 +482,8 @@ static int __init acpi_pcc_probe(void)
>> struct acpi_table_header *pcct_tbl;
>> struct acpi_subtable_header *pcct_entry;
>> struct acpi_table_pcct *acpi_pcct_tbl;
>> + struct acpi_subtable_proc proc[ACPI_PCCT_TYPE_RESERVED];
>> int count, i, rc;
>> - int sum = 0;
>> acpi_status status = AE_OK;
>>
>> /* Search for PCCT */
>> @@ -459,43 +492,50 @@ static int __init acpi_pcc_probe(void)
>> if (ACPI_FAILURE(status) || !pcct_tbl)
>> return -
Re: [PATCH v2 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
On 04/27/2018 05:16 AM, Rafael J. Wysocki wrote:
> On Tue, Apr 24, 2018 at 9:35 PM, Al Stone wrote:
>> There have been multiple reports of the following error message:
>>
>> [0.068293] Error parsing PCC subspaces from PCCT
>>
>
> [cut]
>
>>
>> Tested on my personal laptop which showed the error previously; the
>> error message no longer appears and the laptop appears to operate
>> normally.
>>
>> Signed-off-by: Al Stone
>> Cc: Jassi Brar
>> Cc: Rafael J. Wysocki
>> Cc: Len Brown
>> ---
>> drivers/mailbox/pcc.c | 99
>> +--
>> 1 file changed, 73 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 3ef7f036ceea..cc77a2f1694f 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -372,6 +372,35 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>> .send_data = pcc_send_data,
>> };
>>
>> +/*
>> + *
>> + * count_pcc_subspaces -- Count the PCC subspaces that are not used in
>> + * reduced hardware systems.
>
> One line here, please.
Ack.
>> + * @header: Pointer to the ACPI subtable header under the PCCT.
>> + * @end: End of subtable entry.
>> + *
>> + * Return: 0 for Success, else errno.
>
> The only errno returned appears to be -EINVAL, so maybe say when that
> is returned and then "or 0 otherwise".
Okay.
>> + *
>> + * This gets called for each entry in the PCC table.
>> + */
>> +static int count_pcc_subspaces(struct acpi_subtable_header *header,
>> + const unsigned long end)
>> +{
>> + struct acpi_pcct_subspace *pcct_ss =
>> + (struct acpi_pcct_subspace *) header;
>
> This can be one line (ignore warnings from checkpatch about that).
Okay. That's always a bit of a judgment call with checkpatch.
>> +
>> + if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
>> + (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
>> + (pcct_ss->header.type !=
>> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
>
> This can be one line too.
Okay.
>> + pr_warn("PCCT count: useful subtype = %d\n",
>> + pcct_ss->header.type);
>
> Why pr_warn()?
>
>> + return 0;
>> + }
>> + pr_warn("PCCT count: unwanted subtype = %d\n", pcct_ss->header.type);
>> + return -EINVAL;
>> +}
>> +
Oh, bugger. I inadvertently left some debug noise in here. Both of the above
pr_warn() calls should go away since they don't actually provide any value.
>> /**
>> * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
>> * entries. There should be one entry per PCC client.
>> @@ -395,10 +424,14 @@ static int parse_pcc_subspace(struct
>> acpi_subtable_header *header,
>> && (pcct_ss->header.type !=
>> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
>> pr_err("Incorrect PCC Subspace type detected\n");
>> + pr_warn("PCCT parse: unwanted subtype = %d\n",
>> + pcct_ss->header.type);
>
> Maybe merge the two messages?
Yup, or remove the pr_warn(); that's really just extra debug info I had put in
to check results.
>
>> return -EINVAL;
>> }
>> }
>>
>> + pcct_ss = (struct acpi_pcct_hw_reduced *) header;
>> + pr_warn("PCCT parse: useful subtype = %d\n", pcct_ss->header.type);
...and same here -- the pr_warn() is extraneous, really.
>> return 0;
>> }
>>
>> @@ -449,8 +482,8 @@ static int __init acpi_pcc_probe(void)
>> struct acpi_table_header *pcct_tbl;
>> struct acpi_subtable_header *pcct_entry;
>> struct acpi_table_pcct *acpi_pcct_tbl;
>> + struct acpi_subtable_proc proc[ACPI_PCCT_TYPE_RESERVED];
>> int count, i, rc;
>> - int sum = 0;
>> acpi_status status = AE_OK;
>>
>> /* Search for PCCT */
>> @@ -459,43 +492,50 @@ static int __init acpi_pcc_probe(void)
>> if (ACPI_FAILURE(status) || !pcct_tbl)
>> return -ENODEV;
>>
>> - count = acpi_table_parse_entries(ACPI_SIG_PCCT,
>> -
Re: [PATCH v2 2/3] ACPI: ensure acpi_parse_entries_array() does not access non-existent table data
On 04/27/2018 05:05 AM, Rafael J. Wysocki wrote:
> On Tue, Apr 24, 2018 at 9:35 PM, Al Stone <a...@redhat.com> wrote:
>> For ACPI tables that have subtables, acpi_parse_entries_array() gets used
>> to step through each of the subtables in memory. The primary loop for this
>> was checking that the beginning location of the subtable being examined
>> plus the length of struct acpi_subtable_header was not beyond the end of
>> the complete ACPI table; if it wasn't, the subtable could be examined, but
>> if it was the loop would terminate as it should.
>>
>> In the middle of this subtable loop, a callback is used to examine the
>> subtable in detail.
>>
>> Should the callback function try to examine elements of the subtable that
>> are located past the subtable header, and the ACPI table containing this
>> subtable has an incorrect length, it is possible to access either invalid
>> or protected memory and cause a fault. And, the length of struct
>> acpi_subtable_header will always be smaller than the length of the actual
>> subtable.
>>
>> To fix this, we make the main loop check that the beginning of the
>> subtable being examined plus the actual length of the subtable does
>> not go past the end of the enclosing ACPI table. While this cannot
>> protect us from malicious callback functions, it can prevent us from
>> failing because of some poorly constructed ACPI tables.
>>
>> Found by inspection. There is no functional change to existing code
>> that is known to work when calling acpi_parse_entries_array().
>>
>> Signed-off-by: Al Stone <a...@redhat.com>
>> Cc: Rafael J. Wysocki <r...@rjwysocki.net>
>> Cc: Len Brown <l...@kernel.org>
>> ---
>> drivers/acpi/tables.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>> index 21535762b890..c7b028f231a6 100644
>> --- a/drivers/acpi/tables.c
>> +++ b/drivers/acpi/tables.c
>> @@ -271,8 +271,7 @@ acpi_parse_entries_array(char *id, unsigned long
>> table_size,
>> entry = (struct acpi_subtable_header *)
>> ((unsigned long)table_header + table_size);
>>
>> - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>> - table_end) {
>> + while (((unsigned long)entry + entry->length) <= table_end) {
>
> The inner parens are not needed.
Pure paranoia on my part. Will fix.
>> if (max_entries && count >= max_entries)
>> break;
>>
>> --
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH v2 2/3] ACPI: ensure acpi_parse_entries_array() does not access non-existent table data
On 04/27/2018 05:05 AM, Rafael J. Wysocki wrote:
> On Tue, Apr 24, 2018 at 9:35 PM, Al Stone wrote:
>> For ACPI tables that have subtables, acpi_parse_entries_array() gets used
>> to step through each of the subtables in memory. The primary loop for this
>> was checking that the beginning location of the subtable being examined
>> plus the length of struct acpi_subtable_header was not beyond the end of
>> the complete ACPI table; if it wasn't, the subtable could be examined, but
>> if it was the loop would terminate as it should.
>>
>> In the middle of this subtable loop, a callback is used to examine the
>> subtable in detail.
>>
>> Should the callback function try to examine elements of the subtable that
>> are located past the subtable header, and the ACPI table containing this
>> subtable has an incorrect length, it is possible to access either invalid
>> or protected memory and cause a fault. And, the length of struct
>> acpi_subtable_header will always be smaller than the length of the actual
>> subtable.
>>
>> To fix this, we make the main loop check that the beginning of the
>> subtable being examined plus the actual length of the subtable does
>> not go past the end of the enclosing ACPI table. While this cannot
>> protect us from malicious callback functions, it can prevent us from
>> failing because of some poorly constructed ACPI tables.
>>
>> Found by inspection. There is no functional change to existing code
>> that is known to work when calling acpi_parse_entries_array().
>>
>> Signed-off-by: Al Stone
>> Cc: Rafael J. Wysocki
>> Cc: Len Brown
>> ---
>> drivers/acpi/tables.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>> index 21535762b890..c7b028f231a6 100644
>> --- a/drivers/acpi/tables.c
>> +++ b/drivers/acpi/tables.c
>> @@ -271,8 +271,7 @@ acpi_parse_entries_array(char *id, unsigned long
>> table_size,
>> entry = (struct acpi_subtable_header *)
>> ((unsigned long)table_header + table_size);
>>
>> - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>> - table_end) {
>> + while (((unsigned long)entry + entry->length) <= table_end) {
>
> The inner parens are not needed.
Pure paranoia on my part. Will fix.
>> if (max_entries && count >= max_entries)
>> break;
>>
>> --
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH v2 1/3] ACPI: improve function documentation for acpi_parse_entries_array()
On 04/27/2018 05:04 AM, Rafael J. Wysocki wrote: > On Tue, Apr 24, 2018 at 9:35 PM, Al Stone <a...@redhat.com> wrote: >> I found the description of the table_size argument to the function >> acpi_parse_entries_array() unclear and ambiguous. This is a minor >> documentation change to improve that description so I don't misuse >> the argument again in the future, and it is hopefully clearer to >> other future users. >> >> Signed-off-by: Al Stone <a...@redhat.com> >> Cc: Rafael J. Wysocki <r...@rjwysocki.net> >> Cc: Len Brown <l...@kernel.org> >> --- >> drivers/acpi/tables.c | 6 -- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c >> index 849c4fb19b03..21535762b890 100644 >> --- a/drivers/acpi/tables.c >> +++ b/drivers/acpi/tables.c >> @@ -222,7 +222,9 @@ void acpi_table_print_madt_entry(struct >> acpi_subtable_header *header) >> * acpi_parse_entries_array - for each proc_num find a suitable subtable >> * >> * @id: table id (for debugging purposes) >> - * @table_size: single entry size >> + * @table_size: size of the root table; i.e., the offset from the very >> + * first byte of the complete ACPI table, to the first byte >> + * of the first subtable > > But alas this needs to be one line. > > You can add more details in the comment body below. Whups. Right. Will do. >> * @table_header: where does the table start? >> * @proc: array of acpi_subtable_proc struct containing entry id >> *and associated handler with it >> @@ -400,7 +402,7 @@ int __init acpi_table_parse(char *id, >> acpi_tbl_table_handler handler) >> return -ENODEV; >> } > > Thanks, > Rafael > -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH v2 1/3] ACPI: improve function documentation for acpi_parse_entries_array()
On 04/27/2018 05:04 AM, Rafael J. Wysocki wrote: > On Tue, Apr 24, 2018 at 9:35 PM, Al Stone wrote: >> I found the description of the table_size argument to the function >> acpi_parse_entries_array() unclear and ambiguous. This is a minor >> documentation change to improve that description so I don't misuse >> the argument again in the future, and it is hopefully clearer to >> other future users. >> >> Signed-off-by: Al Stone >> Cc: Rafael J. Wysocki >> Cc: Len Brown >> --- >> drivers/acpi/tables.c | 6 -- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c >> index 849c4fb19b03..21535762b890 100644 >> --- a/drivers/acpi/tables.c >> +++ b/drivers/acpi/tables.c >> @@ -222,7 +222,9 @@ void acpi_table_print_madt_entry(struct >> acpi_subtable_header *header) >> * acpi_parse_entries_array - for each proc_num find a suitable subtable >> * >> * @id: table id (for debugging purposes) >> - * @table_size: single entry size >> + * @table_size: size of the root table; i.e., the offset from the very >> + * first byte of the complete ACPI table, to the first byte >> + * of the first subtable > > But alas this needs to be one line. > > You can add more details in the comment body below. Whups. Right. Will do. >> * @table_header: where does the table start? >> * @proc: array of acpi_subtable_proc struct containing entry id >> *and associated handler with it >> @@ -400,7 +402,7 @@ int __init acpi_table_parse(char *id, >> acpi_tbl_table_handler handler) >> return -ENODEV; >> } > > Thanks, > Rafael > -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
[PATCH v2 1/3] ACPI: improve function documentation for acpi_parse_entries_array()
I found the description of the table_size argument to the function acpi_parse_entries_array() unclear and ambiguous. This is a minor documentation change to improve that description so I don't misuse the argument again in the future, and it is hopefully clearer to other future users. Signed-off-by: Al Stone <a...@redhat.com> Cc: Rafael J. Wysocki <r...@rjwysocki.net> Cc: Len Brown <l...@kernel.org> --- drivers/acpi/tables.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 849c4fb19b03..21535762b890 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -222,7 +222,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) * acpi_parse_entries_array - for each proc_num find a suitable subtable * * @id: table id (for debugging purposes) - * @table_size: single entry size + * @table_size: size of the root table; i.e., the offset from the very + * first byte of the complete ACPI table, to the first byte + * of the first subtable * @table_header: where does the table start? * @proc: array of acpi_subtable_proc struct containing entry id *and associated handler with it @@ -400,7 +402,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler) return -ENODEV; } -/* +/* * The BIOS is supposed to supply a single APIC/MADT, * but some report two. Provide a knob to use either. * (don't you wish instance 0 and 1 were not the same?) -- 2.14.3
[PATCH v2 1/3] ACPI: improve function documentation for acpi_parse_entries_array()
I found the description of the table_size argument to the function acpi_parse_entries_array() unclear and ambiguous. This is a minor documentation change to improve that description so I don't misuse the argument again in the future, and it is hopefully clearer to other future users. Signed-off-by: Al Stone Cc: Rafael J. Wysocki Cc: Len Brown --- drivers/acpi/tables.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 849c4fb19b03..21535762b890 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -222,7 +222,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) * acpi_parse_entries_array - for each proc_num find a suitable subtable * * @id: table id (for debugging purposes) - * @table_size: single entry size + * @table_size: size of the root table; i.e., the offset from the very + * first byte of the complete ACPI table, to the first byte + * of the first subtable * @table_header: where does the table start? * @proc: array of acpi_subtable_proc struct containing entry id *and associated handler with it @@ -400,7 +402,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler) return -ENODEV; } -/* +/* * The BIOS is supposed to supply a single APIC/MADT, * but some report two. Provide a knob to use either. * (don't you wish instance 0 and 1 were not the same?) -- 2.14.3
[PATCH v2 2/3] ACPI: ensure acpi_parse_entries_array() does not access non-existent table data
For ACPI tables that have subtables, acpi_parse_entries_array() gets used
to step through each of the subtables in memory. The primary loop for this
was checking that the beginning location of the subtable being examined
plus the length of struct acpi_subtable_header was not beyond the end of
the complete ACPI table; if it wasn't, the subtable could be examined, but
if it was the loop would terminate as it should.
In the middle of this subtable loop, a callback is used to examine the
subtable in detail.
Should the callback function try to examine elements of the subtable that
are located past the subtable header, and the ACPI table containing this
subtable has an incorrect length, it is possible to access either invalid
or protected memory and cause a fault. And, the length of struct
acpi_subtable_header will always be smaller than the length of the actual
subtable.
To fix this, we make the main loop check that the beginning of the
subtable being examined plus the actual length of the subtable does
not go past the end of the enclosing ACPI table. While this cannot
protect us from malicious callback functions, it can prevent us from
failing because of some poorly constructed ACPI tables.
Found by inspection. There is no functional change to existing code
that is known to work when calling acpi_parse_entries_array().
Signed-off-by: Al Stone <a...@redhat.com>
Cc: Rafael J. Wysocki <r...@rjwysocki.net>
Cc: Len Brown <l...@kernel.org>
---
drivers/acpi/tables.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 21535762b890..c7b028f231a6 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -271,8 +271,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
entry = (struct acpi_subtable_header *)
((unsigned long)table_header + table_size);
- while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
- table_end) {
+ while (((unsigned long)entry + entry->length) <= table_end) {
if (max_entries && count >= max_entries)
break;
--
2.14.3
[PATCH v2 2/3] ACPI: ensure acpi_parse_entries_array() does not access non-existent table data
For ACPI tables that have subtables, acpi_parse_entries_array() gets used
to step through each of the subtables in memory. The primary loop for this
was checking that the beginning location of the subtable being examined
plus the length of struct acpi_subtable_header was not beyond the end of
the complete ACPI table; if it wasn't, the subtable could be examined, but
if it was the loop would terminate as it should.
In the middle of this subtable loop, a callback is used to examine the
subtable in detail.
Should the callback function try to examine elements of the subtable that
are located past the subtable header, and the ACPI table containing this
subtable has an incorrect length, it is possible to access either invalid
or protected memory and cause a fault. And, the length of struct
acpi_subtable_header will always be smaller than the length of the actual
subtable.
To fix this, we make the main loop check that the beginning of the
subtable being examined plus the actual length of the subtable does
not go past the end of the enclosing ACPI table. While this cannot
protect us from malicious callback functions, it can prevent us from
failing because of some poorly constructed ACPI tables.
Found by inspection. There is no functional change to existing code
that is known to work when calling acpi_parse_entries_array().
Signed-off-by: Al Stone
Cc: Rafael J. Wysocki
Cc: Len Brown
---
drivers/acpi/tables.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 21535762b890..c7b028f231a6 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -271,8 +271,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
entry = (struct acpi_subtable_header *)
((unsigned long)table_header + table_size);
- while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
- table_end) {
+ while (((unsigned long)entry + entry->length) <= table_end) {
if (max_entries && count >= max_entries)
break;
--
2.14.3
[PATCH v2 0/3] mailbox: ACPI: Remove incorrect error message about parsing PCCT
This set of patches provide some cleanup in ACPI for minor issues found while correcting a bogus error message (the first two patches), and the correction for the error message itself (patch 3/3). Note that patches 1/3 and 2/3 are not required for 3/3 to work: 1/3 only changes a comment and 2/3 makes an ACPI table parsing loop a wee bit more robust. For patch 3/3, many systems on boot have been reporting "Error parsing PCC subspaces from PCCT" which turns out to not be an error at all. The issue is that the probe for ACPI mailboxes defined in the PCCT (Platform Communications Channel Table) makes a faulty assumption about the content of the PCCT. What's more, when the error is reported, no further PCC mailboxes are set up, even when they have been defined in the PCCT. So, in the reported cases, there was no error and the data in the PCCT was being ignored. This is described in more detail in patch 3/3. Since these patches primarily involve ACPI usages, it may make sense for all of them to go through the linux-acpi tree; clearly, this is up to the maintainers, though. v2: -- removed one extraneous '+' in a comment in patch 3/3 -- fixed an if test that had a predicate that kbuild pointed out would always be zero Al Stone (3): ACPI: improve function documentation for acpi_parse_entries_array() ACPI: ensure acpi_parse_entries_array() does not access non-existent table data mailbox: ACPI: erroneous error message when parsing the ACPI PCCT drivers/acpi/tables.c | 9 ++--- drivers/mailbox/pcc.c | 99 +-- 2 files changed, 78 insertions(+), 30 deletions(-) -- 2.14.3
[PATCH v2 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
There have been multiple reports of the following error message:
[0.068293] Error parsing PCC subspaces from PCCT
This error message is not correct. In multiple cases examined, the PCCT
(Platform Communications Channel Table) concerned is actually properly
constructed; the problem is that acpi_pcc_probe() which reads the PCCT
is making the assumption that the only valid PCCT is one that contains
subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
types are counted and as long as there is at least one of the desired
types, the acpi_pcc_probe() succeeds. When no subtables of these types
are found, regardless of whether or not any other subtable types are
present, the error mentioned above is reported.
In the cases reported to me personally, the PCCT contains exactly one
subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
acpi_pcc_probe() does not count it as a valid subtable, so believes
there to be no valid subtables, and hence outputs the error message.
An example of the PCCT being reported as erroneous yet perfectly fine
is the following:
Signature : "PCCT"
Table Length : 006E
Revision : 05
Checksum : A9
Oem ID : "XX"
Oem Table ID : "X "
Oem Revision : 2280
Asl Compiler ID : ""
Asl Compiler Revision : 0002
Flags (decoded below) : 0001
Platform : 1
Reserved :
Subtable Type : 00 [Generic Communications Subspace]
Length : 3E
Reserved :
Base Address : DCE43018
Address Length : 1000
Doorbell Register : [Generic Address Structure]
Space ID : 01 [SystemIO]
Bit Width : 08
Bit Offset : 00
Encoded Access Width : 01 [Byte Access:8]
Address : 1842
Preserve Mask : 00FD
Write Mask : 0002
Command Latency : 1388
Maximum Access Rate :
Minimum Turnaround Time :
To fix this, we count up all of the possible subtable types for the
PCCT, and only report an error when there are none (which could mean
either no subtables, or no valid subtables), or there are too many.
We also change the logic so that if there is a valid subtable, we
do try to initialize it per the PCCT subtable contents. This is a
change in functionality; previously, the probe would have returned
right after the error message and would not have tried to use any
other subtable definition.
Tested on my personal laptop which showed the error previously; the
error message no longer appears and the laptop appears to operate
normally.
Signed-off-by: Al Stone <a...@redhat.com>
Cc: Jassi Brar <jassisinghb...@gmail.com>
Cc: Rafael J. Wysocki <r...@rjwysocki.net>
Cc: Len Brown <l...@kernel.org>
---
drivers/mailbox/pcc.c | 99 +--
1 file changed, 73 insertions(+), 26 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3ef7f036ceea..cc77a2f1694f 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -372,6 +372,35 @@ static const struct mbox_chan_ops pcc_chan_ops = {
.send_data = pcc_send_data,
};
+/*
+ *
+ * count_pcc_subspaces -- Count the PCC subspaces that are not used in
+ * reduced hardware systems.
+ * @header: Pointer to the ACPI subtable header under the PCCT.
+ * @end: End of subtable entry.
+ *
+ * Return: 0 for Success, else errno.
+ *
+ * This gets called for each entry in the PCC table.
+ */
+static int count_pcc_subspaces(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_pcct_subspace *pcct_ss =
+ (struct acpi_pcct_subspace *) header;
+
+ if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
+ (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
+ (pcct_ss->header.type !=
+ ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
+ pr_warn("PCCT count: useful subtype = %d\n",
+ pcct_ss->header.type);
+ return 0;
+ }
+ pr_warn("PCCT count: unwanted subtype = %d\n", pcct_ss->header.type);
+ return -EINVAL;
+}
+
/**
* parse_pcc_subspace - Parse the PCC table and verify PCC subspace
* entries. There should be one entry per PCC client.
@@ -395,10 +424,
[PATCH v2 0/3] mailbox: ACPI: Remove incorrect error message about parsing PCCT
This set of patches provide some cleanup in ACPI for minor issues found while correcting a bogus error message (the first two patches), and the correction for the error message itself (patch 3/3). Note that patches 1/3 and 2/3 are not required for 3/3 to work: 1/3 only changes a comment and 2/3 makes an ACPI table parsing loop a wee bit more robust. For patch 3/3, many systems on boot have been reporting "Error parsing PCC subspaces from PCCT" which turns out to not be an error at all. The issue is that the probe for ACPI mailboxes defined in the PCCT (Platform Communications Channel Table) makes a faulty assumption about the content of the PCCT. What's more, when the error is reported, no further PCC mailboxes are set up, even when they have been defined in the PCCT. So, in the reported cases, there was no error and the data in the PCCT was being ignored. This is described in more detail in patch 3/3. Since these patches primarily involve ACPI usages, it may make sense for all of them to go through the linux-acpi tree; clearly, this is up to the maintainers, though. v2: -- removed one extraneous '+' in a comment in patch 3/3 -- fixed an if test that had a predicate that kbuild pointed out would always be zero Al Stone (3): ACPI: improve function documentation for acpi_parse_entries_array() ACPI: ensure acpi_parse_entries_array() does not access non-existent table data mailbox: ACPI: erroneous error message when parsing the ACPI PCCT drivers/acpi/tables.c | 9 ++--- drivers/mailbox/pcc.c | 99 +-- 2 files changed, 78 insertions(+), 30 deletions(-) -- 2.14.3
[PATCH v2 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
There have been multiple reports of the following error message:
[0.068293] Error parsing PCC subspaces from PCCT
This error message is not correct. In multiple cases examined, the PCCT
(Platform Communications Channel Table) concerned is actually properly
constructed; the problem is that acpi_pcc_probe() which reads the PCCT
is making the assumption that the only valid PCCT is one that contains
subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
types are counted and as long as there is at least one of the desired
types, the acpi_pcc_probe() succeeds. When no subtables of these types
are found, regardless of whether or not any other subtable types are
present, the error mentioned above is reported.
In the cases reported to me personally, the PCCT contains exactly one
subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
acpi_pcc_probe() does not count it as a valid subtable, so believes
there to be no valid subtables, and hence outputs the error message.
An example of the PCCT being reported as erroneous yet perfectly fine
is the following:
Signature : "PCCT"
Table Length : 006E
Revision : 05
Checksum : A9
Oem ID : "XX"
Oem Table ID : "X "
Oem Revision : 2280
Asl Compiler ID : ""
Asl Compiler Revision : 0002
Flags (decoded below) : 0001
Platform : 1
Reserved :
Subtable Type : 00 [Generic Communications Subspace]
Length : 3E
Reserved :
Base Address : DCE43018
Address Length : 1000
Doorbell Register : [Generic Address Structure]
Space ID : 01 [SystemIO]
Bit Width : 08
Bit Offset : 00
Encoded Access Width : 01 [Byte Access:8]
Address : 1842
Preserve Mask : 00FD
Write Mask : 0002
Command Latency : 1388
Maximum Access Rate :
Minimum Turnaround Time :
To fix this, we count up all of the possible subtable types for the
PCCT, and only report an error when there are none (which could mean
either no subtables, or no valid subtables), or there are too many.
We also change the logic so that if there is a valid subtable, we
do try to initialize it per the PCCT subtable contents. This is a
change in functionality; previously, the probe would have returned
right after the error message and would not have tried to use any
other subtable definition.
Tested on my personal laptop which showed the error previously; the
error message no longer appears and the laptop appears to operate
normally.
Signed-off-by: Al Stone
Cc: Jassi Brar
Cc: Rafael J. Wysocki
Cc: Len Brown
---
drivers/mailbox/pcc.c | 99 +--
1 file changed, 73 insertions(+), 26 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3ef7f036ceea..cc77a2f1694f 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -372,6 +372,35 @@ static const struct mbox_chan_ops pcc_chan_ops = {
.send_data = pcc_send_data,
};
+/*
+ *
+ * count_pcc_subspaces -- Count the PCC subspaces that are not used in
+ * reduced hardware systems.
+ * @header: Pointer to the ACPI subtable header under the PCCT.
+ * @end: End of subtable entry.
+ *
+ * Return: 0 for Success, else errno.
+ *
+ * This gets called for each entry in the PCC table.
+ */
+static int count_pcc_subspaces(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_pcct_subspace *pcct_ss =
+ (struct acpi_pcct_subspace *) header;
+
+ if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
+ (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
+ (pcct_ss->header.type !=
+ ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
+ pr_warn("PCCT count: useful subtype = %d\n",
+ pcct_ss->header.type);
+ return 0;
+ }
+ pr_warn("PCCT count: unwanted subtype = %d\n", pcct_ss->header.type);
+ return -EINVAL;
+}
+
/**
* parse_pcc_subspace - Parse the PCC table and verify PCC subspace
* entries. There should be one entry per PCC client.
@@ -395,10 +424,14 @@ static int parse_pcc_subspace(struct acpi_subtable_header
*header,
&& (pcct_ss->header.type !=
Re: [PATCH 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
On 04/04/2018 02:39 PM, kbuild test robot wrote:
> Hi Al,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on pm/linux-next]
> [also build test WARNING on v4.16 next-20180404]
> [if your patch is applied to the wrong git tree, please drop us a note to
> help improve the system]
>
> url:
> https://github.com/0day-ci/linux/commits/Al-Stone/ACPI-improve-function-documentation-for-acpi_parse_entries_array/20180404-151910
> base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
> linux-next
>
> smatch warnings:
> drivers/mailbox/pcc.c:392 count_pcc_subspaces() warn: always true condition
> '(pcct_ss->header.type >= 0) => (0-255 >= 0)'
>
> vim +392 drivers/mailbox/pcc.c
>
>374
>375/*+
>376 *
>377 * count_pcc_subspaces -- Count the PCC subspaces that are not
> used in
>378 * reduced hardware systems.
>379 * @header: Pointer to the ACPI subtable header under the PCCT.
>380 * @end: End of subtable entry.
>381 *
>382 * Return: 0 for Success, else errno.
>383 *
>384 * This gets called for each entry in the PCC table.
>385 */
>386static int count_pcc_subspaces(struct acpi_subtable_header
> *header,
>387const unsigned long end)
>388{
>389struct acpi_pcct_subspace *pcct_ss =
>390(struct acpi_pcct_subspace *)
> header;
>391
> > 392if ((pcct_ss->header.type >=
> ACPI_PCCT_TYPE_GENERIC_SUBSPACE) &&
>393(pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
>394(pcct_ss->header.type !=
> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
>395(pcct_ss->header.type !=
>396
> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
Aha. Yup, can be rewritten as:
if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
(pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
(pcct_ss->header.type !=
ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
I will let this patch sit for a little while to see if there are any further
comments before sending out a v2.
>397pr_warn("PCCT count: useful subtype = %d\n",
>398pcct_ss->header.type);
>399return 0;
>400}
>401pr_warn("PCCT count: unwanted subtype = %d\n",
> pcct_ss->header.type);
>402 return -EINVAL;
>403}
>404
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
On 04/04/2018 02:39 PM, kbuild test robot wrote:
> Hi Al,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on pm/linux-next]
> [also build test WARNING on v4.16 next-20180404]
> [if your patch is applied to the wrong git tree, please drop us a note to
> help improve the system]
>
> url:
> https://github.com/0day-ci/linux/commits/Al-Stone/ACPI-improve-function-documentation-for-acpi_parse_entries_array/20180404-151910
> base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
> linux-next
>
> smatch warnings:
> drivers/mailbox/pcc.c:392 count_pcc_subspaces() warn: always true condition
> '(pcct_ss->header.type >= 0) => (0-255 >= 0)'
>
> vim +392 drivers/mailbox/pcc.c
>
>374
>375/*+
>376 *
>377 * count_pcc_subspaces -- Count the PCC subspaces that are not
> used in
>378 * reduced hardware systems.
>379 * @header: Pointer to the ACPI subtable header under the PCCT.
>380 * @end: End of subtable entry.
>381 *
>382 * Return: 0 for Success, else errno.
>383 *
>384 * This gets called for each entry in the PCC table.
>385 */
>386static int count_pcc_subspaces(struct acpi_subtable_header
> *header,
>387const unsigned long end)
>388{
>389struct acpi_pcct_subspace *pcct_ss =
>390(struct acpi_pcct_subspace *)
> header;
>391
> > 392if ((pcct_ss->header.type >=
> ACPI_PCCT_TYPE_GENERIC_SUBSPACE) &&
>393(pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
>394(pcct_ss->header.type !=
> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
>395(pcct_ss->header.type !=
>396
> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
Aha. Yup, can be rewritten as:
if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
(pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
(pcct_ss->header.type !=
ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
I will let this patch sit for a little while to see if there are any further
comments before sending out a v2.
>397pr_warn("PCCT count: useful subtype = %d\n",
>398pcct_ss->header.type);
>399return 0;
>400}
>401pr_warn("PCCT count: unwanted subtype = %d\n",
> pcct_ss->header.type);
>402 return -EINVAL;
>403}
>404
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
[PATCH 2/3] ACPI: ensure acpi_parse_entries_array() does not access non-existent table data
For ACPI tables that have subtables, acpi_parse_entries_array() gets used
to step through each of the subtables in memory. The primary loop for this
was checking that the beginning location of the subtable being examined
plus the length of struct acpi_subtable_header was not beyond the end of
the complete ACPI table; if it wasn't, the subtable could be examined, but
if it was the loop would terminate as it should.
In the middle of this subtable loop, a callback is used to examine the
subtable in detail.
Should the callback function try to examine elements of the subtable that
are located past the subtable header, and the ACPI table containing this
subtable has an incorrect length, it is possible to access either invalid
or protected memory and cause a fault. And, the length of struct
acpi_subtable_header will always be smaller than the length of the actual
subtable.
To fix this, we make the main loop check that the beginning of the
subtable being examined plus the actual length of the subtable does
not go past the end of the enclosing ACPI table. While this cannot
protect us from malicious callback functions, it can prevent us from
failing because of some poorly constructed ACPI tables.
Found by inspection. There is no functional change to existing code
that is known to work when calling acpi_parse_entries_array().
Signed-off-by: Al Stone <a...@redhat.com>
Cc: Rafael J. Wysocki <r...@rjwysocki.net>
Cc: Len Brown <l...@kernel.org>
---
drivers/acpi/tables.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index b6ac8162a2c0..672a1f22c3a5 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -271,8 +271,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
entry = (struct acpi_subtable_header *)
((unsigned long)table_header + table_size);
- while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
- table_end) {
+ while (((unsigned long)entry + entry->length) <= table_end) {
if (max_entries && count >= max_entries)
break;
--
2.14.3
[PATCH 2/3] ACPI: ensure acpi_parse_entries_array() does not access non-existent table data
For ACPI tables that have subtables, acpi_parse_entries_array() gets used
to step through each of the subtables in memory. The primary loop for this
was checking that the beginning location of the subtable being examined
plus the length of struct acpi_subtable_header was not beyond the end of
the complete ACPI table; if it wasn't, the subtable could be examined, but
if it was the loop would terminate as it should.
In the middle of this subtable loop, a callback is used to examine the
subtable in detail.
Should the callback function try to examine elements of the subtable that
are located past the subtable header, and the ACPI table containing this
subtable has an incorrect length, it is possible to access either invalid
or protected memory and cause a fault. And, the length of struct
acpi_subtable_header will always be smaller than the length of the actual
subtable.
To fix this, we make the main loop check that the beginning of the
subtable being examined plus the actual length of the subtable does
not go past the end of the enclosing ACPI table. While this cannot
protect us from malicious callback functions, it can prevent us from
failing because of some poorly constructed ACPI tables.
Found by inspection. There is no functional change to existing code
that is known to work when calling acpi_parse_entries_array().
Signed-off-by: Al Stone
Cc: Rafael J. Wysocki
Cc: Len Brown
---
drivers/acpi/tables.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index b6ac8162a2c0..672a1f22c3a5 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -271,8 +271,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
entry = (struct acpi_subtable_header *)
((unsigned long)table_header + table_size);
- while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
- table_end) {
+ while (((unsigned long)entry + entry->length) <= table_end) {
if (max_entries && count >= max_entries)
break;
--
2.14.3
[PATCH 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
There have been multiple reports of the following error message:
[0.068293] Error parsing PCC subspaces from PCCT
This error message is not correct. In multiple cases examined, the PCCT
(Platform Communications Channel Table) concerned is actually properly
constructed; the problem is that acpi_pcc_probe() which reads the PCCT
is making the assumption that the only valid PCCT is one that contains
subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
types are counted and as long as there is at least one of the desired
types, the acpi_pcc_probe() succeeds. When no subtables of these types
are found, regardless of whether or not any other subtable types are
present, the error mentioned above is reported.
In the cases reported to me personally, the PCCT contains exactly one
subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
acpi_pcc_probe() does not count it as a valid subtable, so believes
there to be no valid subtables, and hence outputs the error message.
An example of the PCCT being reported as erroneous yet perfectly fine
is the following:
Signature : "PCCT"
Table Length : 006E
Revision : 05
Checksum : A9
Oem ID : "XX"
Oem Table ID : "X "
Oem Revision : 2280
Asl Compiler ID : ""
Asl Compiler Revision : 0002
Flags (decoded below) : 0001
Platform : 1
Reserved :
Subtable Type : 00 [Generic Communications Subspace]
Length : 3E
Reserved :
Base Address : DCE43018
Address Length : 1000
Doorbell Register : [Generic Address Structure]
Space ID : 01 [SystemIO]
Bit Width : 08
Bit Offset : 00
Encoded Access Width : 01 [Byte Access:8]
Address : 1842
Preserve Mask : 00FD
Write Mask : 0002
Command Latency : 1388
Maximum Access Rate :
Minimum Turnaround Time :
To fix this, we count up all of the possible subtable types for the
PCCT, and only report an error when there are none (which could mean
either no subtables, or no valid subtables), or there are too many.
We also change the logic so that if there is a valid subtable, we
do try to initialize it per the PCCT subtable contents. This is a
change in functionality; previously, the probe would have returned
right after the error message and would not have tried to use any
other subtable definition.
Tested on my personal laptop which showed the error previously; the
error message no longer appears and the laptop appears to operate
normally.
Signed-off-by: Al Stone <a...@redhat.com>
Cc: Jassi Brar <jassisinghb...@gmail.com>
Cc: Rafael J. Wysocki <r...@rjwysocki.net>
Cc: Len Brown <l...@kernel.org>
---
drivers/mailbox/pcc.c | 99 +--
1 file changed, 73 insertions(+), 26 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3ef7f036ceea..c6294476acc1 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -372,6 +372,36 @@ static const struct mbox_chan_ops pcc_chan_ops = {
.send_data = pcc_send_data,
};
+/*+
+ *
+ * count_pcc_subspaces -- Count the PCC subspaces that are not used in
+ * reduced hardware systems.
+ * @header: Pointer to the ACPI subtable header under the PCCT.
+ * @end: End of subtable entry.
+ *
+ * Return: 0 for Success, else errno.
+ *
+ * This gets called for each entry in the PCC table.
+ */
+static int count_pcc_subspaces(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_pcct_subspace *pcct_ss =
+ (struct acpi_pcct_subspace *) header;
+
+ if ((pcct_ss->header.type >= ACPI_PCCT_TYPE_GENERIC_SUBSPACE) &&
+ (pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
+ (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
+ (pcct_ss->header.type !=
+ ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
+ pr_warn("PCCT count: useful subtype = %d\n",
+ pcct_ss->header.type);
+ return 0;
+ }
+ pr_warn("PCCT count: unwanted subtype = %d\n", pcct_ss->header.type);
+ return -EINVAL;
+}
+
/**
* parse_pcc_subspace - Parse the PCC table and verify PCC subspace
* entries. There should be one entry per P
[PATCH 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
There have been multiple reports of the following error message:
[0.068293] Error parsing PCC subspaces from PCCT
This error message is not correct. In multiple cases examined, the PCCT
(Platform Communications Channel Table) concerned is actually properly
constructed; the problem is that acpi_pcc_probe() which reads the PCCT
is making the assumption that the only valid PCCT is one that contains
subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
types are counted and as long as there is at least one of the desired
types, the acpi_pcc_probe() succeeds. When no subtables of these types
are found, regardless of whether or not any other subtable types are
present, the error mentioned above is reported.
In the cases reported to me personally, the PCCT contains exactly one
subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
acpi_pcc_probe() does not count it as a valid subtable, so believes
there to be no valid subtables, and hence outputs the error message.
An example of the PCCT being reported as erroneous yet perfectly fine
is the following:
Signature : "PCCT"
Table Length : 006E
Revision : 05
Checksum : A9
Oem ID : "XX"
Oem Table ID : "X "
Oem Revision : 2280
Asl Compiler ID : ""
Asl Compiler Revision : 0002
Flags (decoded below) : 0001
Platform : 1
Reserved :
Subtable Type : 00 [Generic Communications Subspace]
Length : 3E
Reserved :
Base Address : DCE43018
Address Length : 1000
Doorbell Register : [Generic Address Structure]
Space ID : 01 [SystemIO]
Bit Width : 08
Bit Offset : 00
Encoded Access Width : 01 [Byte Access:8]
Address : 1842
Preserve Mask : 00FD
Write Mask : 0002
Command Latency : 1388
Maximum Access Rate :
Minimum Turnaround Time :
To fix this, we count up all of the possible subtable types for the
PCCT, and only report an error when there are none (which could mean
either no subtables, or no valid subtables), or there are too many.
We also change the logic so that if there is a valid subtable, we
do try to initialize it per the PCCT subtable contents. This is a
change in functionality; previously, the probe would have returned
right after the error message and would not have tried to use any
other subtable definition.
Tested on my personal laptop which showed the error previously; the
error message no longer appears and the laptop appears to operate
normally.
Signed-off-by: Al Stone
Cc: Jassi Brar
Cc: Rafael J. Wysocki
Cc: Len Brown
---
drivers/mailbox/pcc.c | 99 +--
1 file changed, 73 insertions(+), 26 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3ef7f036ceea..c6294476acc1 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -372,6 +372,36 @@ static const struct mbox_chan_ops pcc_chan_ops = {
.send_data = pcc_send_data,
};
+/*+
+ *
+ * count_pcc_subspaces -- Count the PCC subspaces that are not used in
+ * reduced hardware systems.
+ * @header: Pointer to the ACPI subtable header under the PCCT.
+ * @end: End of subtable entry.
+ *
+ * Return: 0 for Success, else errno.
+ *
+ * This gets called for each entry in the PCC table.
+ */
+static int count_pcc_subspaces(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_pcct_subspace *pcct_ss =
+ (struct acpi_pcct_subspace *) header;
+
+ if ((pcct_ss->header.type >= ACPI_PCCT_TYPE_GENERIC_SUBSPACE) &&
+ (pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
+ (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
+ (pcct_ss->header.type !=
+ ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
+ pr_warn("PCCT count: useful subtype = %d\n",
+ pcct_ss->header.type);
+ return 0;
+ }
+ pr_warn("PCCT count: unwanted subtype = %d\n", pcct_ss->header.type);
+ return -EINVAL;
+}
+
/**
* parse_pcc_subspace - Parse the PCC table and verify PCC subspace
* entries. There should be one entry per PCC client.
@@ -395,10 +424,14 @@ stat
[PATCH 1/3] ACPI: improve function documentation for acpi_parse_entries_array()
I found the description of the table_size argument to the function acpi_parse_entries_array() unclear and ambiguous. This is a minor documentation change to improve that description so I don't misuse the argument again in the future, and it is hopefully clearer to other future users. Signed-off-by: Al Stone <a...@redhat.com> Cc: Rafael J. Wysocki <r...@rjwysocki.net> Cc: Len Brown <l...@kernel.org> --- drivers/acpi/tables.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 6c5ee7e66842..b6ac8162a2c0 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -222,7 +222,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) * acpi_parse_entries_array - for each proc_num find a suitable subtable * * @id: table id (for debugging purposes) - * @table_size: single entry size + * @table_size: size of the root table; i.e., the offset from the very + * first byte of the complete ACPI table, to the first byte + * of the first subtable * @table_header: where does the table start? * @proc: array of acpi_subtable_proc struct containing entry id *and associated handler with it @@ -400,7 +402,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler) return -ENODEV; } -/* +/* * The BIOS is supposed to supply a single APIC/MADT, * but some report two. Provide a knob to use either. * (don't you wish instance 0 and 1 were not the same?) -- 2.14.3
[PATCH 0/3] mailbox: ACPI: Remove incorrect error message about parsing PCCT
This set of patches provide some cleanup in ACPI for minor issues found while correcting a bogus error message (the first two patches), and the correction for the error message itself (patch 3/3). Note that patches 1/3 and 2/3 are not required for 3/3 to work: 1/3 only changes a comment and 2/3 makes an ACPI table parsing loop a wee bit more robust. For patch 3/3, many systems on boot have been reporting "Error parsing PCC subspaces from PCCT" which turns out to not be an error at all. The issue is that the probe for ACPI mailboxes defined in the PCCT (Platform Communications Channel Table) makes a faulty assumption about the content of the PCCT. What's more, when the error is reported, no further PCC mailboxes are set up, even when they have been defined in the PCCT. So, in the reported cases, there was no error and the data in the PCCT was being ignored. This is described in more detail in patch 3/3. Since these patches primarily involve ACPI usages, it may make sense for all of them to go through the linux-acpi tree; clearly, this is up to the maintainers, though. Al Stone (3): ACPI: improve function documentation for acpi_parse_entries_array() ACPI: ensure acpi_parse_entries_array() does not access non-existent table data mailbox: ACPI: erroneous error message when parsing the ACPI PCCT drivers/acpi/tables.c | 9 ++--- drivers/mailbox/pcc.c | 99 +-- 2 files changed, 78 insertions(+), 30 deletions(-) -- 2.14.3
[PATCH 0/3] mailbox: ACPI: Remove incorrect error message about parsing PCCT
This set of patches provide some cleanup in ACPI for minor issues found while correcting a bogus error message (the first two patches), and the correction for the error message itself (patch 3/3). Note that patches 1/3 and 2/3 are not required for 3/3 to work: 1/3 only changes a comment and 2/3 makes an ACPI table parsing loop a wee bit more robust. For patch 3/3, many systems on boot have been reporting "Error parsing PCC subspaces from PCCT" which turns out to not be an error at all. The issue is that the probe for ACPI mailboxes defined in the PCCT (Platform Communications Channel Table) makes a faulty assumption about the content of the PCCT. What's more, when the error is reported, no further PCC mailboxes are set up, even when they have been defined in the PCCT. So, in the reported cases, there was no error and the data in the PCCT was being ignored. This is described in more detail in patch 3/3. Since these patches primarily involve ACPI usages, it may make sense for all of them to go through the linux-acpi tree; clearly, this is up to the maintainers, though. Al Stone (3): ACPI: improve function documentation for acpi_parse_entries_array() ACPI: ensure acpi_parse_entries_array() does not access non-existent table data mailbox: ACPI: erroneous error message when parsing the ACPI PCCT drivers/acpi/tables.c | 9 ++--- drivers/mailbox/pcc.c | 99 +-- 2 files changed, 78 insertions(+), 30 deletions(-) -- 2.14.3
[PATCH 1/3] ACPI: improve function documentation for acpi_parse_entries_array()
I found the description of the table_size argument to the function acpi_parse_entries_array() unclear and ambiguous. This is a minor documentation change to improve that description so I don't misuse the argument again in the future, and it is hopefully clearer to other future users. Signed-off-by: Al Stone Cc: Rafael J. Wysocki Cc: Len Brown --- drivers/acpi/tables.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 6c5ee7e66842..b6ac8162a2c0 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -222,7 +222,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) * acpi_parse_entries_array - for each proc_num find a suitable subtable * * @id: table id (for debugging purposes) - * @table_size: single entry size + * @table_size: size of the root table; i.e., the offset from the very + * first byte of the complete ACPI table, to the first byte + * of the first subtable * @table_header: where does the table start? * @proc: array of acpi_subtable_proc struct containing entry id *and associated handler with it @@ -400,7 +402,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler) return -ENODEV; } -/* +/* * The BIOS is supposed to supply a single APIC/MADT, * but some report two. Provide a knob to use either. * (don't you wish instance 0 and 1 were not the same?) -- 2.14.3
Re: [PATCH v3 6/7] arm64: topology: Enable ACPI/PPTT based CPU topology.
On 10/20/2017 03:22 AM, Lorenzo Pieralisi wrote: > On Thu, Oct 19, 2017 at 11:54:22AM -0500, Jeremy Linton wrote: > > [...] > >>>> + cpu_topology[cpu].core_id = topology_id; >>>> + topology_id = setup_acpi_cpu_topology(cpu, 2); >>>> + cpu_topology[cpu].cluster_id = topology_id; >>>> + topology_id = setup_acpi_cpu_topology(cpu, max_topo); >>> >>> If you want a package id (that's just a package tag to group cores), you >>> should not use a large level because you know how >>> setup_acpi_cpu_topology()works, you should add an API that allows you to >>> retrieve the package id >>> (so that you can use th ACPI_PPTT_PHYSICAL_PACKAGE flag consistenly, >>> whatever it represents). >> >> I don't think the spec requires the use of PHYSICAL_PACKAGE... Am I >> misreading it? Which means we need to "pick" a node level to >> represent the physical package if one doesn't exist... > > The specs define a means to detect if a given PPTT node corresponds to a > package (I am refraining from stating again that to me that's not clean > cut what a package is _architecturally_, I think you know my POV by now) > and that's what you need to use to retrieve a packageid for a given cpu, > if I understand the aim of the physical package flag. > > Either that or that flag is completely useless. > > Lorenzo > > ACPI 6.2 - Table 5-151 (page 248) > Physical package > - > Set to 1 if this node of the processor topology represents the boundary > of a physical package, whether socketed or surface mounted. Set to 0 if > this instance of the processor topology does not represent the boundary > of a physical package. > I've been following the discussion and I'm not sure I understand what the confusion is around having a physical package ID. Since I'm the one that insisted it be in the spec, I'd be glad to clarify anything. My apologies for not saying anything sooner but things IRL have been very complicated of late. What was intended was a simple flag that was meant to tell me if a CPU ID (this could be a CPU, a cluster, a processor container -- I don't really care which) is *also* an actual physical device on a motherboard. That is the only intent; there was no architectural meaning intended at all -- that is what the PPTT structures are for, in conjunction with any DSDT information uncovered later in the boot process. However, in the broader server ecosystem, this can be incredibly useful. There are a significant number of software products sold or supported that base their fees on the number of physical sockets in use. There have been in the past (and may be in the near future) machines where the cost of the lease on the machine is determined by how many physical sockets (or even CPUs) are in use, even if the machine has many more available. Some vendors also include FRU (Field Replaceable Unit) location info in their ACPI tables. So, for example, one or more CPUs or caches might fail in one physical package, which is then reported to a maintenance system of some sort that tells some human which of the physical sockets on what motherboard needs a replacement device, or it's simply noted and shut off until it's time to replace the entire server, or perhaps it's logged and used in an algorithm to predict when the server might fail completely. So, that's why the flag exists in the spec. It seems to make sense to me to have a package ID as part of struct cpu_topology -- it might even be really handy for CPU hotplug. If you don't, it seems to me a whole separate struct would be needed with more cpumasks to show who belongs to what physical package; that might be okay but seems unnecessarily complicated to me. You can also tell me that I have completely missed the point of the discussion so far :-). But if you do, you have to tell me what I missed. Hope this helps clarify... -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH v3 6/7] arm64: topology: Enable ACPI/PPTT based CPU topology.
On 10/20/2017 03:22 AM, Lorenzo Pieralisi wrote: > On Thu, Oct 19, 2017 at 11:54:22AM -0500, Jeremy Linton wrote: > > [...] > >>>> + cpu_topology[cpu].core_id = topology_id; >>>> + topology_id = setup_acpi_cpu_topology(cpu, 2); >>>> + cpu_topology[cpu].cluster_id = topology_id; >>>> + topology_id = setup_acpi_cpu_topology(cpu, max_topo); >>> >>> If you want a package id (that's just a package tag to group cores), you >>> should not use a large level because you know how >>> setup_acpi_cpu_topology()works, you should add an API that allows you to >>> retrieve the package id >>> (so that you can use th ACPI_PPTT_PHYSICAL_PACKAGE flag consistenly, >>> whatever it represents). >> >> I don't think the spec requires the use of PHYSICAL_PACKAGE... Am I >> misreading it? Which means we need to "pick" a node level to >> represent the physical package if one doesn't exist... > > The specs define a means to detect if a given PPTT node corresponds to a > package (I am refraining from stating again that to me that's not clean > cut what a package is _architecturally_, I think you know my POV by now) > and that's what you need to use to retrieve a packageid for a given cpu, > if I understand the aim of the physical package flag. > > Either that or that flag is completely useless. > > Lorenzo > > ACPI 6.2 - Table 5-151 (page 248) > Physical package > - > Set to 1 if this node of the processor topology represents the boundary > of a physical package, whether socketed or surface mounted. Set to 0 if > this instance of the processor topology does not represent the boundary > of a physical package. > I've been following the discussion and I'm not sure I understand what the confusion is around having a physical package ID. Since I'm the one that insisted it be in the spec, I'd be glad to clarify anything. My apologies for not saying anything sooner but things IRL have been very complicated of late. What was intended was a simple flag that was meant to tell me if a CPU ID (this could be a CPU, a cluster, a processor container -- I don't really care which) is *also* an actual physical device on a motherboard. That is the only intent; there was no architectural meaning intended at all -- that is what the PPTT structures are for, in conjunction with any DSDT information uncovered later in the boot process. However, in the broader server ecosystem, this can be incredibly useful. There are a significant number of software products sold or supported that base their fees on the number of physical sockets in use. There have been in the past (and may be in the near future) machines where the cost of the lease on the machine is determined by how many physical sockets (or even CPUs) are in use, even if the machine has many more available. Some vendors also include FRU (Field Replaceable Unit) location info in their ACPI tables. So, for example, one or more CPUs or caches might fail in one physical package, which is then reported to a maintenance system of some sort that tells some human which of the physical sockets on what motherboard needs a replacement device, or it's simply noted and shut off until it's time to replace the entire server, or perhaps it's logged and used in an algorithm to predict when the server might fail completely. So, that's why the flag exists in the spec. It seems to make sense to me to have a package ID as part of struct cpu_topology -- it might even be really handy for CPU hotplug. If you don't, it seems to me a whole separate struct would be needed with more cpumasks to show who belongs to what physical package; that might be okay but seems unnecessarily complicated to me. You can also tell me that I have completely missed the point of the discussion so far :-). But if you do, you have to tell me what I missed. Hope this helps clarify... -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable
On 10/20/2017 10:10 AM, Mark Rutland wrote: > Hi Al, > > On Mon, Oct 16, 2017 at 05:43:19PM -0600, Al Stone wrote: >> On 10/13/2017 08:27 AM, Mark Rutland wrote: >>> I certainly agree that exposing the information that we have is useful, >>> as I have stated several times. I'm not NAKing exposing this information >>> elsewhere. >>> >>> If you want a consistent cross-architecture interface for this >>> information, then you need to propose a new one. That was we can >>> actually solve the underlying issues, for all architectures, without >>> breaking ABI. >>> >>> I would be *very* interested in such an interface, and would be more >>> than happy to help. >> >> I'm playing with some patches that do very similar things in sysfs, vs >> proc. Is that better :)? > > Exposing data under sysfs is certainly better, yes. :) > >> Obviously, you'll have to see the patches to >> properly answer that, but what I'm playing with at present is placing >> this info in new entries in /sys/devices/cpu and/or /sys/devices/system, >> and generating some of the content based on what's already in header files >> (e.g., in cputype.h). > > My opposition to MIDR -> string mapping applies regardless of > location... Harumph. This is the one thing I get asked for most often, however (second most is frequency). It turns out humans are not nearly as good at indexed lookups as computers, hence the requests. Whatever the root of the opposition is, it needs to get fixed. My fear is that if it doesn't get fixed in the firmware or the kernel, it will get fixed in some far messier, less controllable way somewhere else (more likely several somewhere elses) and just exacerbate the problem. >> The idea of course is to keep this new info from touching any existing >> info so we don't break compatibility -- does that feel like a better >> direction, at least? > > ... but otherwise this sounds good to me! > > Thanks, > Mark. > -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable
On 10/20/2017 10:10 AM, Mark Rutland wrote: > Hi Al, > > On Mon, Oct 16, 2017 at 05:43:19PM -0600, Al Stone wrote: >> On 10/13/2017 08:27 AM, Mark Rutland wrote: >>> I certainly agree that exposing the information that we have is useful, >>> as I have stated several times. I'm not NAKing exposing this information >>> elsewhere. >>> >>> If you want a consistent cross-architecture interface for this >>> information, then you need to propose a new one. That was we can >>> actually solve the underlying issues, for all architectures, without >>> breaking ABI. >>> >>> I would be *very* interested in such an interface, and would be more >>> than happy to help. >> >> I'm playing with some patches that do very similar things in sysfs, vs >> proc. Is that better :)? > > Exposing data under sysfs is certainly better, yes. :) > >> Obviously, you'll have to see the patches to >> properly answer that, but what I'm playing with at present is placing >> this info in new entries in /sys/devices/cpu and/or /sys/devices/system, >> and generating some of the content based on what's already in header files >> (e.g., in cputype.h). > > My opposition to MIDR -> string mapping applies regardless of > location... Harumph. This is the one thing I get asked for most often, however (second most is frequency). It turns out humans are not nearly as good at indexed lookups as computers, hence the requests. Whatever the root of the opposition is, it needs to get fixed. My fear is that if it doesn't get fixed in the firmware or the kernel, it will get fixed in some far messier, less controllable way somewhere else (more likely several somewhere elses) and just exacerbate the problem. >> The idea of course is to keep this new info from touching any existing >> info so we don't break compatibility -- does that feel like a better >> direction, at least? > > ... but otherwise this sounds good to me! > > Thanks, > Mark. > -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable
On 10/13/2017 08:27 AM, Mark Rutland wrote: > Hi Timur, > > On Fri, Oct 13, 2017 at 08:39:09AM -0500, Timur Tabi wrote: >> On Wed, Sep 27, 2017 at 5:34 AM, Mark Rutland <mark.rutl...@arm.com> wrote: >>> On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote: >>>> As ARMv8 servers get deployed, I keep getting the same set of questions >>>> from end-users of those systems: what do all the hex numbers mean in >>>> /proc/cpuinfo and could you make them so I don't have to carry a cheat >>>> sheet with me all the time? >>> >>> I appreciate that /proc/cpuinfo can be opaque to end users, but I do not >>> believe that this is the right solution to that problem. >>> >>> There are a number of issues stemming from the face that /proc/cpuinfo >>> is ill-defined and overused for a number of cases. Changes to it almost >>> certainly violate brittle de-facto ABI details people are relying on, >>> and there's a very long tail on fallout resulting from this. In >>> addition, many niceties come at an excessive maintenance cost, and are >>> simply unreliable as an ABI. >>> >>> So, as with all other patches modifying /proc/cpuinfo, I must NAK this >>> series. >> >> Qualcomm Datacenter Technologies is very interested in seeing these >> patches (or some variant of them) accepted. Updates to /proc/cpuinfo >> are long overdue, and I'm asking you to reconsider your objections. > > I more than appreciate that there is information that people want access > to, and I'd really like to expose that in a consistent and structured > manner. > > So please propose exposing the information elsewhere, which I would be > more than happy with for information that the kernel already has access > to. > > I object to /proc/cpuinfo changes because they impose an ABI break, and > because some of the proposed changes impose new and requirements on the > kernel that cannot be maintained in a forwards-compatible manner (which > is liable to result in other ABI breaks). > > For better or worse, we're stuck with the existing arch-specific format > of /proc/cpuinfo, as this is parsed by a wide variety of applications, > and any change risks breaking these. > >> We're willing to work with distro vendors to get this information >> added to their products while upstream is left behind, but I hope that >> won't be necessary. > > Please understand that this is an ABI break, and that is why it is being > NAK'd. > > I don't have the power to stop Red Hat or other vendors from > deliberately breaking ABI, but I would very strongly recommend that they > do not. > >> I would even go so far as to say that we should be making >> /proc/cpuinfo for ARM match the x86 output as closely as possible, > > As has been stated before, /proc/cpuinfo is an existing arch-specific ABI. > > For better or worse, we're stuck with it as-is, regardless of what could > be nicer had we done something different from the outset. > > It has never been portable across architectures, and applications > relying upon it have never been portable except by chance. > >> even using their terminology. We should be providing information like >> frequencies and product names. > > As has been stated before, the problem with exposing these is that we > don't have the relevant information. > > We have no consistent mechanism for acquiring product names, and for > those cases where it is truly necessary to identify an implementation, > the MIDR+REVIDR provide the exact same information. > > For frequencies, I'd be more than happy to expose this information when > we have it, in new files. In practice today, we do not have this > information most of the time. > >> Having a human-readable /proc/cpuinfo with extensive details of the >> CPU spelled out is very useful, and Al's reasoning is valid. The fact >> that it's "fragile" is not as important as the fact that on x86, >> /proc/cpuinfo is much more useful. > > I must disagree. > > We care about that fragility so that we do not break userspace, which is > the most important upstream rule. > > I certainly agree that exposing the information that we have is useful, > as I have stated several times. I'm not NAKing exposing this information > elsewhere. > > If you want a consistent cross-architecture interface for this > information, then you need to propose a new one. That was we can > actually solve the underlying issues, for all architectures, without > breaking ABI. > > I would be *very* interested in such an interface, and would be more > than happy
Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable
On 10/13/2017 08:27 AM, Mark Rutland wrote: > Hi Timur, > > On Fri, Oct 13, 2017 at 08:39:09AM -0500, Timur Tabi wrote: >> On Wed, Sep 27, 2017 at 5:34 AM, Mark Rutland wrote: >>> On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote: >>>> As ARMv8 servers get deployed, I keep getting the same set of questions >>>> from end-users of those systems: what do all the hex numbers mean in >>>> /proc/cpuinfo and could you make them so I don't have to carry a cheat >>>> sheet with me all the time? >>> >>> I appreciate that /proc/cpuinfo can be opaque to end users, but I do not >>> believe that this is the right solution to that problem. >>> >>> There are a number of issues stemming from the face that /proc/cpuinfo >>> is ill-defined and overused for a number of cases. Changes to it almost >>> certainly violate brittle de-facto ABI details people are relying on, >>> and there's a very long tail on fallout resulting from this. In >>> addition, many niceties come at an excessive maintenance cost, and are >>> simply unreliable as an ABI. >>> >>> So, as with all other patches modifying /proc/cpuinfo, I must NAK this >>> series. >> >> Qualcomm Datacenter Technologies is very interested in seeing these >> patches (or some variant of them) accepted. Updates to /proc/cpuinfo >> are long overdue, and I'm asking you to reconsider your objections. > > I more than appreciate that there is information that people want access > to, and I'd really like to expose that in a consistent and structured > manner. > > So please propose exposing the information elsewhere, which I would be > more than happy with for information that the kernel already has access > to. > > I object to /proc/cpuinfo changes because they impose an ABI break, and > because some of the proposed changes impose new and requirements on the > kernel that cannot be maintained in a forwards-compatible manner (which > is liable to result in other ABI breaks). > > For better or worse, we're stuck with the existing arch-specific format > of /proc/cpuinfo, as this is parsed by a wide variety of applications, > and any change risks breaking these. > >> We're willing to work with distro vendors to get this information >> added to their products while upstream is left behind, but I hope that >> won't be necessary. > > Please understand that this is an ABI break, and that is why it is being > NAK'd. > > I don't have the power to stop Red Hat or other vendors from > deliberately breaking ABI, but I would very strongly recommend that they > do not. > >> I would even go so far as to say that we should be making >> /proc/cpuinfo for ARM match the x86 output as closely as possible, > > As has been stated before, /proc/cpuinfo is an existing arch-specific ABI. > > For better or worse, we're stuck with it as-is, regardless of what could > be nicer had we done something different from the outset. > > It has never been portable across architectures, and applications > relying upon it have never been portable except by chance. > >> even using their terminology. We should be providing information like >> frequencies and product names. > > As has been stated before, the problem with exposing these is that we > don't have the relevant information. > > We have no consistent mechanism for acquiring product names, and for > those cases where it is truly necessary to identify an implementation, > the MIDR+REVIDR provide the exact same information. > > For frequencies, I'd be more than happy to expose this information when > we have it, in new files. In practice today, we do not have this > information most of the time. > >> Having a human-readable /proc/cpuinfo with extensive details of the >> CPU spelled out is very useful, and Al's reasoning is valid. The fact >> that it's "fragile" is not as important as the fact that on x86, >> /proc/cpuinfo is much more useful. > > I must disagree. > > We care about that fragility so that we do not break userspace, which is > the most important upstream rule. > > I certainly agree that exposing the information that we have is useful, > as I have stated several times. I'm not NAKing exposing this information > elsewhere. > > If you want a consistent cross-architecture interface for this > information, then you need to propose a new one. That was we can > actually solve the underlying issues, for all architectures, without > breaking ABI. > > I would be *very* interested in such an interface, and would be more > than happy to help. I'm playing with s
Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable
On 09/27/2017 04:34 AM, Mark Rutland wrote: > On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote: >> As ARMv8 servers get deployed, I keep getting the same set of questions >> from end-users of those systems: what do all the hex numbers mean in >> /proc/cpuinfo and could you make them so I don't have to carry a cheat >> sheet with me all the time? > > I appreciate that /proc/cpuinfo can be opaque to end users, but I do not > believe that this is the right solution to that problem. > > There are a number of issues stemming from the face that /proc/cpuinfo > is ill-defined and overused for a number of cases. Changes to it almost > certainly violate brittle de-facto ABI details people are relying on, > and there's a very long tail on fallout resulting from this. In > addition, many niceties come at an excessive maintenance cost, and are > simply unreliable as an ABI. Instead of dealing with the replies to the specific patches, I'd like to deal with the real gist of the matter first -- details can be worked out later. While I don't disagree -- cpuinfo is enormously fragile -- what _is_ a good solution, then? Right now, things are not terribly useful. Yes, it is true /sys/firmware/dmi has info in it, but again it is all numeric (or in a file named 'raw'). Any one who wishes to read the values will need to put some interpretation on them somehow. My personal preference is that the kernel control that interpretation instead of random admin tools scattered over a raft of different companies. > So, as with all other patches modifying /proc/cpuinfo, I must NAK this > series. Hrm. I suggest that policy needs to be rethought. I understand the reasoning, but it puts my end-users in a very difficult position; they now have to justify modifying management software that is already in operation in order to purchase and integrate ARM servers. And while I really don't want distro-specific code, I'd have to consider it as a possibility. To be completely fair, though, I'm not completely fixated on cpuinfo as the only possible answer. It has been the most requested, however. > For the MPIDR and product info, I think we can expose this in a more > structured way (e.g. under sysfs). IIUC the product info is all derived > from DMI -- do we not expose that already? Agreed; the MPIDR I think could just as easily be in /sys/devices/cpu/* as a hex value. It doesn't have the value to end-users that the other info does, either -- for debugging, definitely, but not necessarily sysadmin. I'll put something together for this. The DMI info is in sysfs but it is not in a fashion normally consumed, hence the questions from end-users, in a way. They're not expecting to have to do the interpretation, and they are expecting the platform to be able to tell them what "Model: 2" means. > For the human-readable names I must NAK such patches. This is an > extremely brittle ABI that cannot be forwards compatible, and comes with > hilarious political problems. This should be managed in userspace alone. > > I thought tools like lscpu and lshw were intended to gather such > information for users. What are these missing today? All of the above human-readable parts -- lscpu reads model information from /proc/cpuinfo, for example, and would require new code to read this info from somewhere else, and then figure out how to interpret it. All I get is a lovely '2' for 'model' :). We could obviously change the userspace tools, but I'm guessing it just moves the political hilarity from the kernel to userspace, correct? It also runs the risk of even greater silliness, like perhaps lscpu gets a product name right, but lshw calls it something different, and so on. The other approach that occurs to me is to blend what x86 does and sysfs; i.e., would it be acceptable to modify cputype.h to include comments (as x86 does) that are then munged by a script somewhat like arch/x86/kernel/cpu/mkcapflags.sh that then produces a header file with proper human-readable content that can then be exposed in sysfs? I don't think we can ever remove the brittleness completely, but perhaps this would at least mitigate it. This would still require userspace modifications, too, but they could at least rely on a uniform source of data -- and perhaps a single place to focus when hilarity ensues :). I'll bodge some patches together and maybe start over as an RFC, if there's a chance this would be accepted. -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable
On 09/27/2017 04:34 AM, Mark Rutland wrote: > On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote: >> As ARMv8 servers get deployed, I keep getting the same set of questions >> from end-users of those systems: what do all the hex numbers mean in >> /proc/cpuinfo and could you make them so I don't have to carry a cheat >> sheet with me all the time? > > I appreciate that /proc/cpuinfo can be opaque to end users, but I do not > believe that this is the right solution to that problem. > > There are a number of issues stemming from the face that /proc/cpuinfo > is ill-defined and overused for a number of cases. Changes to it almost > certainly violate brittle de-facto ABI details people are relying on, > and there's a very long tail on fallout resulting from this. In > addition, many niceties come at an excessive maintenance cost, and are > simply unreliable as an ABI. Instead of dealing with the replies to the specific patches, I'd like to deal with the real gist of the matter first -- details can be worked out later. While I don't disagree -- cpuinfo is enormously fragile -- what _is_ a good solution, then? Right now, things are not terribly useful. Yes, it is true /sys/firmware/dmi has info in it, but again it is all numeric (or in a file named 'raw'). Any one who wishes to read the values will need to put some interpretation on them somehow. My personal preference is that the kernel control that interpretation instead of random admin tools scattered over a raft of different companies. > So, as with all other patches modifying /proc/cpuinfo, I must NAK this > series. Hrm. I suggest that policy needs to be rethought. I understand the reasoning, but it puts my end-users in a very difficult position; they now have to justify modifying management software that is already in operation in order to purchase and integrate ARM servers. And while I really don't want distro-specific code, I'd have to consider it as a possibility. To be completely fair, though, I'm not completely fixated on cpuinfo as the only possible answer. It has been the most requested, however. > For the MPIDR and product info, I think we can expose this in a more > structured way (e.g. under sysfs). IIUC the product info is all derived > from DMI -- do we not expose that already? Agreed; the MPIDR I think could just as easily be in /sys/devices/cpu/* as a hex value. It doesn't have the value to end-users that the other info does, either -- for debugging, definitely, but not necessarily sysadmin. I'll put something together for this. The DMI info is in sysfs but it is not in a fashion normally consumed, hence the questions from end-users, in a way. They're not expecting to have to do the interpretation, and they are expecting the platform to be able to tell them what "Model: 2" means. > For the human-readable names I must NAK such patches. This is an > extremely brittle ABI that cannot be forwards compatible, and comes with > hilarious political problems. This should be managed in userspace alone. > > I thought tools like lscpu and lshw were intended to gather such > information for users. What are these missing today? All of the above human-readable parts -- lscpu reads model information from /proc/cpuinfo, for example, and would require new code to read this info from somewhere else, and then figure out how to interpret it. All I get is a lovely '2' for 'model' :). We could obviously change the userspace tools, but I'm guessing it just moves the political hilarity from the kernel to userspace, correct? It also runs the risk of even greater silliness, like perhaps lscpu gets a product name right, but lshw calls it something different, and so on. The other approach that occurs to me is to blend what x86 does and sysfs; i.e., would it be acceptable to modify cputype.h to include comments (as x86 does) that are then munged by a script somewhat like arch/x86/kernel/cpu/mkcapflags.sh that then produces a header file with proper human-readable content that can then be exposed in sysfs? I don't think we can ever remove the brittleness completely, but perhaps this would at least mitigate it. This would still require userspace modifications, too, but they could at least rely on a uniform source of data -- and perhaps a single place to focus when hilarity ensues :). I'll bodge some patches together and maybe start over as an RFC, if there's a chance this would be accepted. -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable
As ARMv8 servers get deployed, I keep getting the same set of questions from end-users of those systems: what do all the hex numbers mean in /proc/cpuinfo and could you make them so I don't have to carry a cheat sheet with me all the time? These patches respond to those questions. For good or ill, some of the automation used to manage systems in data centers (as well as many of the humans involved) need to have text; this helps them simply slide into place and become usable quickly. Patch 1/2 provides the MPIDR as basic topology info in /proc/cpuinfo when using ACPI, perhaps until such time as the more robust ACPI implementation is available [0]; this is helpful in automating the selection of multi-CPU systems when many choices are available (for example, in automated testing systems). While it is yet another hex value, it does provide some topology information without interfering with what [0] will ultimately provide, and is helpful in sorting out ACPI table issues that use the MPIDR for identifying CPUs. Patches 2/3 and 3/3 are similar in that they provide a more human- readable version of the info already available; this allows admin tools to provide proper strings to display in inventory systems, for example, or when a human is using a CI system and needs to be provided a list of possible systems to test on. In all of the patches, I have avoided replacing or interfering with any existing output so as not to affect systems already in use. Tested on AMD Seattle, APM Mustang and Cavium ThunderX systems. [0] https://marc.info/?l=linux-pm=150584702021552=2 Al Stone (3): arm64: cpuinfo: add MPIDR value to /proc/cpuinfo arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo arm64: cpuinfo: display product info in /proc/cpuinfo arch/arm64/include/asm/cpu.h | 1 + arch/arm64/kernel/cpuinfo.c | 225 +++ 2 files changed, 226 insertions(+) -- 2.13.5
[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable
As ARMv8 servers get deployed, I keep getting the same set of questions from end-users of those systems: what do all the hex numbers mean in /proc/cpuinfo and could you make them so I don't have to carry a cheat sheet with me all the time? These patches respond to those questions. For good or ill, some of the automation used to manage systems in data centers (as well as many of the humans involved) need to have text; this helps them simply slide into place and become usable quickly. Patch 1/2 provides the MPIDR as basic topology info in /proc/cpuinfo when using ACPI, perhaps until such time as the more robust ACPI implementation is available [0]; this is helpful in automating the selection of multi-CPU systems when many choices are available (for example, in automated testing systems). While it is yet another hex value, it does provide some topology information without interfering with what [0] will ultimately provide, and is helpful in sorting out ACPI table issues that use the MPIDR for identifying CPUs. Patches 2/3 and 3/3 are similar in that they provide a more human- readable version of the info already available; this allows admin tools to provide proper strings to display in inventory systems, for example, or when a human is using a CI system and needs to be provided a list of possible systems to test on. In all of the patches, I have avoided replacing or interfering with any existing output so as not to affect systems already in use. Tested on AMD Seattle, APM Mustang and Cavium ThunderX systems. [0] https://marc.info/?l=linux-pm=150584702021552=2 Al Stone (3): arm64: cpuinfo: add MPIDR value to /proc/cpuinfo arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo arm64: cpuinfo: display product info in /proc/cpuinfo arch/arm64/include/asm/cpu.h | 1 + arch/arm64/kernel/cpuinfo.c | 225 +++ 2 files changed, 226 insertions(+) -- 2.13.5
[PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo
While it is very useful to know what CPU is being used, it is also
useful to know who made the platform being used. On servers, this
can point to the right person to contact when a server is having
trouble.
Go get the product info -- manufacturer, product name and version --
from DMI (aka SMBIOS) and display it in /proc/cpuinfo. To look more
like other server platforms, include the CPU type and frequency when
displaying the product info, too.
Signed-off-by: Al Stone <a...@redhat.com>
Cc: Catalin Marinas <catalin.mari...@arm.com>
Cc: Will Deacon <will.dea...@arm.com>
Cc: Suzuki K Poulose <suzuki.poul...@arm.com>
Cc: Mark Rutland <mark.rutl...@arm.com>
---
arch/arm64/kernel/cpuinfo.c | 135 +++-
1 file changed, 134 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 0b4261884862..6a9dbad5ee3f 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -19,10 +19,12 @@
#include
#include
#include
+#include
#include
#include
#include
+#include
#include
#include
#include
@@ -31,6 +33,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -167,6 +170,111 @@ static const char *const compat_hwcap2_str[] = {
};
#endif /* CONFIG_COMPAT */
+/* Details needed when extracting fields from DMI info */
+#define DMI_ENTRY_BASEBOARD_MIN_LENGTH 8
+#define DMI_ENTRY_PROCESSOR_MIN_LENGTH 48
+
+#define DMI_BASEBOARD_MANUFACTURER 0x04
+#define DMI_BASEBOARD_PRODUCT 0x05
+#define DMI_BASEBOARD_VERSION 0x06
+#define DMI_PROCESSOR_MAX_SPEED0x14
+
+#define DMI_MAX_STRLEN 80
+
+/* Values captured from DMI info */
+static u64 dmi_max_mhz;
+static char *dmi_product_info;
+
+/* Callback function used to retrieve the max frequency from DMI */
+static void find_dmi_mhz(const struct dmi_header *dm, void *private)
+{
+ const u8 *dmi_data = (const u8 *)dm;
+ u16 *mhz = (u16 *)private;
+
+ if (dm->type == DMI_ENTRY_PROCESSOR &&
+ dm->length >= DMI_ENTRY_PROCESSOR_MIN_LENGTH) {
+ u16 val = (u16)get_unaligned((const u16 *)
+ (dmi_data + DMI_PROCESSOR_MAX_SPEED));
+ *mhz = val > *mhz ? val : *mhz;
+ }
+}
+
+/* Look up the max frequency in DMI */
+static u64 get_dmi_max_mhz(void)
+{
+ u16 mhz = 0;
+
+ dmi_walk(find_dmi_mhz, );
+
+ /*
+* Real stupid fallback value, just in case there is no
+* actual value set.
+*/
+ mhz = mhz ? mhz : 1;
+
+ return (u64)mhz;
+}
+
+/* Helper function for the product info callback */
+static char *copy_string_n(char *dst, char *table, int idx)
+{
+ char *d = dst;
+ char *ptr = table;
+ int ii;
+
+ /* skip the first idx-1 strings */
+ for (ii = 1; ii < idx; ii++) {
+ while (*ptr)
+ ptr++;
+ ptr++;
+ }
+
+ /* copy in the string we need */
+ while (*ptr && (d - dst) < (DMI_MAX_STRLEN - 2))
+ *d++ = *ptr++;
+
+ return d;
+}
+
+/* Callback function used to retrieve the product info DMI */
+static void find_dmi_product_info(const struct dmi_header *dm, void *private)
+{
+ const u8 *dmi_data = (const u8 *)dm;
+ char *ptr = (char *)private;
+
+ if (dm->type == DMI_ENTRY_BASEBOARD &&
+ dm->length >= DMI_ENTRY_BASEBOARD_MIN_LENGTH) {
+ int idx;
+
+ idx = (int)get_unaligned((const u8 *)
+ (dmi_data + DMI_BASEBOARD_MANUFACTURER));
+ ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
+ *ptr++ = ' ';
+
+ idx = (int)get_unaligned((const u8 *)
+ (dmi_data + DMI_BASEBOARD_PRODUCT));
+ ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
+ *ptr++ = ' ';
+
+ idx = (int)get_unaligned((const u8 *)
+ (dmi_data + DMI_BASEBOARD_VERSION));
+ ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
+ }
+}
+
+/* Look up the baseboard info in DMI */
+static void get_dmi_product_info(void)
+{
+ if (!dmi_product_info) {
+ dmi_product_info = kcalloc(DMI_MAX_STRLEN,
+ sizeof(char), GFP_KERNEL);
+ if (!dmi_product_info)
+ return;
+ }
+
+ dmi_walk(find_dmi_product_info, dmi_product_info);
+}
+
static int c_show(struct seq_file *m, void *v)
{
int i, j;
@@ -190,6 +298,31 @@ static int c_show(struct seq_file *m, void *v)
seq_printf(m, "model name\t: ARMv8 Processor rev %d
(%s)\n",
MIDR_REVISION(midr), COMPAT_ELF_PLATFORM
[PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo
While it is very useful to know what CPU is being used, it is also
useful to know who made the platform being used. On servers, this
can point to the right person to contact when a server is having
trouble.
Go get the product info -- manufacturer, product name and version --
from DMI (aka SMBIOS) and display it in /proc/cpuinfo. To look more
like other server platforms, include the CPU type and frequency when
displaying the product info, too.
Signed-off-by: Al Stone
Cc: Catalin Marinas
Cc: Will Deacon
Cc: Suzuki K Poulose
Cc: Mark Rutland
---
arch/arm64/kernel/cpuinfo.c | 135 +++-
1 file changed, 134 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 0b4261884862..6a9dbad5ee3f 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -19,10 +19,12 @@
#include
#include
#include
+#include
#include
#include
#include
+#include
#include
#include
#include
@@ -31,6 +33,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -167,6 +170,111 @@ static const char *const compat_hwcap2_str[] = {
};
#endif /* CONFIG_COMPAT */
+/* Details needed when extracting fields from DMI info */
+#define DMI_ENTRY_BASEBOARD_MIN_LENGTH 8
+#define DMI_ENTRY_PROCESSOR_MIN_LENGTH 48
+
+#define DMI_BASEBOARD_MANUFACTURER 0x04
+#define DMI_BASEBOARD_PRODUCT 0x05
+#define DMI_BASEBOARD_VERSION 0x06
+#define DMI_PROCESSOR_MAX_SPEED0x14
+
+#define DMI_MAX_STRLEN 80
+
+/* Values captured from DMI info */
+static u64 dmi_max_mhz;
+static char *dmi_product_info;
+
+/* Callback function used to retrieve the max frequency from DMI */
+static void find_dmi_mhz(const struct dmi_header *dm, void *private)
+{
+ const u8 *dmi_data = (const u8 *)dm;
+ u16 *mhz = (u16 *)private;
+
+ if (dm->type == DMI_ENTRY_PROCESSOR &&
+ dm->length >= DMI_ENTRY_PROCESSOR_MIN_LENGTH) {
+ u16 val = (u16)get_unaligned((const u16 *)
+ (dmi_data + DMI_PROCESSOR_MAX_SPEED));
+ *mhz = val > *mhz ? val : *mhz;
+ }
+}
+
+/* Look up the max frequency in DMI */
+static u64 get_dmi_max_mhz(void)
+{
+ u16 mhz = 0;
+
+ dmi_walk(find_dmi_mhz, );
+
+ /*
+* Real stupid fallback value, just in case there is no
+* actual value set.
+*/
+ mhz = mhz ? mhz : 1;
+
+ return (u64)mhz;
+}
+
+/* Helper function for the product info callback */
+static char *copy_string_n(char *dst, char *table, int idx)
+{
+ char *d = dst;
+ char *ptr = table;
+ int ii;
+
+ /* skip the first idx-1 strings */
+ for (ii = 1; ii < idx; ii++) {
+ while (*ptr)
+ ptr++;
+ ptr++;
+ }
+
+ /* copy in the string we need */
+ while (*ptr && (d - dst) < (DMI_MAX_STRLEN - 2))
+ *d++ = *ptr++;
+
+ return d;
+}
+
+/* Callback function used to retrieve the product info DMI */
+static void find_dmi_product_info(const struct dmi_header *dm, void *private)
+{
+ const u8 *dmi_data = (const u8 *)dm;
+ char *ptr = (char *)private;
+
+ if (dm->type == DMI_ENTRY_BASEBOARD &&
+ dm->length >= DMI_ENTRY_BASEBOARD_MIN_LENGTH) {
+ int idx;
+
+ idx = (int)get_unaligned((const u8 *)
+ (dmi_data + DMI_BASEBOARD_MANUFACTURER));
+ ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
+ *ptr++ = ' ';
+
+ idx = (int)get_unaligned((const u8 *)
+ (dmi_data + DMI_BASEBOARD_PRODUCT));
+ ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
+ *ptr++ = ' ';
+
+ idx = (int)get_unaligned((const u8 *)
+ (dmi_data + DMI_BASEBOARD_VERSION));
+ ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
+ }
+}
+
+/* Look up the baseboard info in DMI */
+static void get_dmi_product_info(void)
+{
+ if (!dmi_product_info) {
+ dmi_product_info = kcalloc(DMI_MAX_STRLEN,
+ sizeof(char), GFP_KERNEL);
+ if (!dmi_product_info)
+ return;
+ }
+
+ dmi_walk(find_dmi_product_info, dmi_product_info);
+}
+
static int c_show(struct seq_file *m, void *v)
{
int i, j;
@@ -190,6 +298,31 @@ static int c_show(struct seq_file *m, void *v)
seq_printf(m, "model name\t: ARMv8 Processor rev %d
(%s)\n",
MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
+ if (IS_ENABLED(CONFIG_DMI)) {
+ seq_puts(m, "product name\t: ");
+
+
[PATCH 2/3] arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo
In the interest of making things easier for humans to use, add a
"CPU name" line to /proc/cpuinfo for each CPU that uses plain old
words instead of hex values. For example, instead of printing only
CPU implementer 0x43 and CPU part 0x0A1, print also "CPU name :
Cavium ThunderX".
Note that this is not meant to be an exhaustive list of all possible
implementers or CPUs (I'm not even sure that is knowable); this patch
is intentionally limited to only those willing to provide info in
arch/arm64/include/asm/cputype.h
Signed-off-by: Al Stone <a...@redhat.com>
Cc: Catalin Marinas <catalin.mari...@arm.com>
Cc: Will Deacon <will.dea...@arm.com>
Cc: Suzuki K Poulose <suzuki.poul...@arm.com>
Cc: Mark Rutland <mark.rutl...@arm.com>
---
arch/arm64/kernel/cpuinfo.c | 84 +
1 file changed, 84 insertions(+)
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index e505007138eb..0b4261884862 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -75,6 +75,61 @@ static const char *const hwcap_str[] = {
NULL
};
+struct hw_part {
+ u16 id;
+ char*name;
+};
+
+static const struct hw_part arm_hw_part[] = {
+ { ARM_CPU_PART_AEM_V8, "AEMv8 Model" },
+ { ARM_CPU_PART_FOUNDATION, "Foundation Model" },
+ { ARM_CPU_PART_CORTEX_A57, "Cortex A57" },
+ { ARM_CPU_PART_CORTEX_A53, "Cortex A53" },
+ { ARM_CPU_PART_CORTEX_A73, "Cortex A73" },
+ { (-1), "unknown" } /* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part apm_hw_part[] = {
+ { APM_CPU_PART_POTENZA, "Potenza" },
+ { (-1), "unknown" } /* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part brcm_hw_part[] = {
+ { BRCM_CPU_PART_VULCAN, "Vulcan" },
+ { (-1), "unknown" } /* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part cavium_hw_part[] = {
+ { CAVIUM_CPU_PART_THUNDERX, "ThunderX" },
+ { CAVIUM_CPU_PART_THUNDERX_81XX, "ThunderX 81XX" },
+ { CAVIUM_CPU_PART_THUNDERX_83XX, "ThunderX 83XX" },
+ { (-1), "unknown" } /* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part qcom_hw_part[] = {
+ { QCOM_CPU_PART_FALKOR_V1, "Falkor v1" },
+ { (-1), "unknown" } /* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part unknown_hw_part[] = {
+ { (-1), "unknown" } /* Potenza == 0, unfortunately */
+};
+
+struct hw_impl {
+ u8 id;
+ const struct hw_part*parts;
+ char*name;
+};
+
+static const struct hw_impl hw_implementer[] = {
+ { ARM_CPU_IMP_ARM, arm_hw_part,"ARM Ltd." },
+ { ARM_CPU_IMP_APM, apm_hw_part,"Applied Micro" },
+ { ARM_CPU_IMP_CAVIUM, cavium_hw_part, "Cavium" },
+ { ARM_CPU_IMP_BRCM, brcm_hw_part, "Broadcom" },
+ { ARM_CPU_IMP_QCOM, qcom_hw_part, "Qualcomm" },
+ { 0, unknown_hw_part, "unknown" }
+};
+
#ifdef CONFIG_COMPAT
static const char *const compat_hwcap_str[] = {
"swp",
@@ -116,6 +171,9 @@ static int c_show(struct seq_file *m, void *v)
{
int i, j;
bool compat = personality(current->personality) == PER_LINUX32;
+ u8 impl;
+ u16 part;
+ const struct hw_part *parts;
for_each_online_cpu(i) {
struct cpuinfo_arm64 *cpuinfo = _cpu(cpu_data, i);
@@ -132,6 +190,32 @@ static int c_show(struct seq_file *m, void *v)
seq_printf(m, "model name\t: ARMv8 Processor rev %d
(%s)\n",
MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
+ impl = (u8) MIDR_IMPLEMENTOR(midr);
+ for (j = 0; hw_implementer[j].id != 0; j++) {
+ if (hw_implementer[j].id == impl) {
+ seq_printf(m, "CPU name\t: %s ",
+ hw_implementer[j].name);
+ parts = hw_implementer[j].parts;
+ break;
+ }
+ }
+ if (hw_implementer[j].id == 0) {
+ seq_printf(m, "CPU name\t: %s ",
+ hw_implementer[j].name);
+ parts = hw_implementer[j].parts;
+ }
+
+ part = (u16) MIDR_PARTNUM(midr);
+ for (j = 0; parts[j].id != (-1); j++) {
+ if (parts[j].id == part) {
+
[PATCH 2/3] arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo
In the interest of making things easier for humans to use, add a
"CPU name" line to /proc/cpuinfo for each CPU that uses plain old
words instead of hex values. For example, instead of printing only
CPU implementer 0x43 and CPU part 0x0A1, print also "CPU name :
Cavium ThunderX".
Note that this is not meant to be an exhaustive list of all possible
implementers or CPUs (I'm not even sure that is knowable); this patch
is intentionally limited to only those willing to provide info in
arch/arm64/include/asm/cputype.h
Signed-off-by: Al Stone
Cc: Catalin Marinas
Cc: Will Deacon
Cc: Suzuki K Poulose
Cc: Mark Rutland
---
arch/arm64/kernel/cpuinfo.c | 84 +
1 file changed, 84 insertions(+)
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index e505007138eb..0b4261884862 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -75,6 +75,61 @@ static const char *const hwcap_str[] = {
NULL
};
+struct hw_part {
+ u16 id;
+ char*name;
+};
+
+static const struct hw_part arm_hw_part[] = {
+ { ARM_CPU_PART_AEM_V8, "AEMv8 Model" },
+ { ARM_CPU_PART_FOUNDATION, "Foundation Model" },
+ { ARM_CPU_PART_CORTEX_A57, "Cortex A57" },
+ { ARM_CPU_PART_CORTEX_A53, "Cortex A53" },
+ { ARM_CPU_PART_CORTEX_A73, "Cortex A73" },
+ { (-1), "unknown" } /* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part apm_hw_part[] = {
+ { APM_CPU_PART_POTENZA, "Potenza" },
+ { (-1), "unknown" } /* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part brcm_hw_part[] = {
+ { BRCM_CPU_PART_VULCAN, "Vulcan" },
+ { (-1), "unknown" } /* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part cavium_hw_part[] = {
+ { CAVIUM_CPU_PART_THUNDERX, "ThunderX" },
+ { CAVIUM_CPU_PART_THUNDERX_81XX, "ThunderX 81XX" },
+ { CAVIUM_CPU_PART_THUNDERX_83XX, "ThunderX 83XX" },
+ { (-1), "unknown" } /* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part qcom_hw_part[] = {
+ { QCOM_CPU_PART_FALKOR_V1, "Falkor v1" },
+ { (-1), "unknown" } /* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part unknown_hw_part[] = {
+ { (-1), "unknown" } /* Potenza == 0, unfortunately */
+};
+
+struct hw_impl {
+ u8 id;
+ const struct hw_part*parts;
+ char*name;
+};
+
+static const struct hw_impl hw_implementer[] = {
+ { ARM_CPU_IMP_ARM, arm_hw_part,"ARM Ltd." },
+ { ARM_CPU_IMP_APM, apm_hw_part,"Applied Micro" },
+ { ARM_CPU_IMP_CAVIUM, cavium_hw_part, "Cavium" },
+ { ARM_CPU_IMP_BRCM, brcm_hw_part, "Broadcom" },
+ { ARM_CPU_IMP_QCOM, qcom_hw_part, "Qualcomm" },
+ { 0, unknown_hw_part, "unknown" }
+};
+
#ifdef CONFIG_COMPAT
static const char *const compat_hwcap_str[] = {
"swp",
@@ -116,6 +171,9 @@ static int c_show(struct seq_file *m, void *v)
{
int i, j;
bool compat = personality(current->personality) == PER_LINUX32;
+ u8 impl;
+ u16 part;
+ const struct hw_part *parts;
for_each_online_cpu(i) {
struct cpuinfo_arm64 *cpuinfo = _cpu(cpu_data, i);
@@ -132,6 +190,32 @@ static int c_show(struct seq_file *m, void *v)
seq_printf(m, "model name\t: ARMv8 Processor rev %d
(%s)\n",
MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
+ impl = (u8) MIDR_IMPLEMENTOR(midr);
+ for (j = 0; hw_implementer[j].id != 0; j++) {
+ if (hw_implementer[j].id == impl) {
+ seq_printf(m, "CPU name\t: %s ",
+ hw_implementer[j].name);
+ parts = hw_implementer[j].parts;
+ break;
+ }
+ }
+ if (hw_implementer[j].id == 0) {
+ seq_printf(m, "CPU name\t: %s ",
+ hw_implementer[j].name);
+ parts = hw_implementer[j].parts;
+ }
+
+ part = (u16) MIDR_PARTNUM(midr);
+ for (j = 0; parts[j].id != (-1); j++) {
+ if (parts[j].id == part) {
+ seq_printf(m, "%s\n", parts[j].name);
+ break;
+ }
+ }
+
[PATCH 1/3] arm64: cpuinfo: add MPIDR value to /proc/cpuinfo
When displaying cpuinfo on an arm64 system, include the MPIDR register
value which can be used to understand the CPU topology on a platform.
This can serve as a cross-check to the information provided in sysfs,
and has been useful in understanding CPU and NUMA allocation issues.
Signed-off-by: Al Stone <a...@redhat.com>
Cc: Catalin Marinas <catalin.mari...@arm.com>
Cc: Will Deacon <will.dea...@arm.com>
Cc: Suzuki K Poulose <suzuki.poul...@arm.com>
Cc: Mark Rutland <mark.rutl...@arm.com>
---
arch/arm64/include/asm/cpu.h | 1 +
arch/arm64/kernel/cpuinfo.c | 8
2 files changed, 9 insertions(+)
diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
index 889226b4c6e1..ac40894df247 100644
--- a/arch/arm64/include/asm/cpu.h
+++ b/arch/arm64/include/asm/cpu.h
@@ -32,6 +32,7 @@ struct cpuinfo_arm64 {
u32 reg_midr;
u32 reg_revidr;
+ u64 reg_id_aa64mpidr;
u64 reg_id_aa64dfr0;
u64 reg_id_aa64dfr1;
u64 reg_id_aa64isar0;
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 4a6f875ac854..e505007138eb 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -120,6 +120,7 @@ static int c_show(struct seq_file *m, void *v)
for_each_online_cpu(i) {
struct cpuinfo_arm64 *cpuinfo = _cpu(cpu_data, i);
u32 midr = cpuinfo->reg_midr;
+ u64 mpidr = cpuinfo->reg_id_aa64mpidr;
/*
* glibc reads /proc/cpuinfo to determine the number of
@@ -159,6 +160,12 @@ static int c_show(struct seq_file *m, void *v)
}
seq_puts(m, "\n");
+ seq_printf(m, "CPU MPIDR\t: 0x%016llx ", mpidr);
+ seq_printf(m, "(Aff3 %d Aff2 %d Aff1 %d Aff0 %d)\n",
+ (u8) MPIDR_AFFINITY_LEVEL(mpidr, 3),
+ (u8) MPIDR_AFFINITY_LEVEL(mpidr, 2),
+ (u8) MPIDR_AFFINITY_LEVEL(mpidr, 1),
+ (u8) MPIDR_AFFINITY_LEVEL(mpidr, 0));
seq_printf(m, "CPU implementer\t: 0x%02x\n",
MIDR_IMPLEMENTOR(midr));
seq_printf(m, "CPU architecture: 8\n");
@@ -372,6 +379,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
info->reg_midr = read_cpuid_id();
info->reg_revidr = read_cpuid(REVIDR_EL1);
+ info->reg_id_aa64mpidr = read_cpuid_mpidr();
info->reg_id_aa64dfr0 = read_cpuid(ID_AA64DFR0_EL1);
info->reg_id_aa64dfr1 = read_cpuid(ID_AA64DFR1_EL1);
info->reg_id_aa64isar0 = read_cpuid(ID_AA64ISAR0_EL1);
--
2.13.5
[PATCH 1/3] arm64: cpuinfo: add MPIDR value to /proc/cpuinfo
When displaying cpuinfo on an arm64 system, include the MPIDR register
value which can be used to understand the CPU topology on a platform.
This can serve as a cross-check to the information provided in sysfs,
and has been useful in understanding CPU and NUMA allocation issues.
Signed-off-by: Al Stone
Cc: Catalin Marinas
Cc: Will Deacon
Cc: Suzuki K Poulose
Cc: Mark Rutland
---
arch/arm64/include/asm/cpu.h | 1 +
arch/arm64/kernel/cpuinfo.c | 8
2 files changed, 9 insertions(+)
diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
index 889226b4c6e1..ac40894df247 100644
--- a/arch/arm64/include/asm/cpu.h
+++ b/arch/arm64/include/asm/cpu.h
@@ -32,6 +32,7 @@ struct cpuinfo_arm64 {
u32 reg_midr;
u32 reg_revidr;
+ u64 reg_id_aa64mpidr;
u64 reg_id_aa64dfr0;
u64 reg_id_aa64dfr1;
u64 reg_id_aa64isar0;
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 4a6f875ac854..e505007138eb 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -120,6 +120,7 @@ static int c_show(struct seq_file *m, void *v)
for_each_online_cpu(i) {
struct cpuinfo_arm64 *cpuinfo = _cpu(cpu_data, i);
u32 midr = cpuinfo->reg_midr;
+ u64 mpidr = cpuinfo->reg_id_aa64mpidr;
/*
* glibc reads /proc/cpuinfo to determine the number of
@@ -159,6 +160,12 @@ static int c_show(struct seq_file *m, void *v)
}
seq_puts(m, "\n");
+ seq_printf(m, "CPU MPIDR\t: 0x%016llx ", mpidr);
+ seq_printf(m, "(Aff3 %d Aff2 %d Aff1 %d Aff0 %d)\n",
+ (u8) MPIDR_AFFINITY_LEVEL(mpidr, 3),
+ (u8) MPIDR_AFFINITY_LEVEL(mpidr, 2),
+ (u8) MPIDR_AFFINITY_LEVEL(mpidr, 1),
+ (u8) MPIDR_AFFINITY_LEVEL(mpidr, 0));
seq_printf(m, "CPU implementer\t: 0x%02x\n",
MIDR_IMPLEMENTOR(midr));
seq_printf(m, "CPU architecture: 8\n");
@@ -372,6 +379,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
info->reg_midr = read_cpuid_id();
info->reg_revidr = read_cpuid(REVIDR_EL1);
+ info->reg_id_aa64mpidr = read_cpuid_mpidr();
info->reg_id_aa64dfr0 = read_cpuid(ID_AA64DFR0_EL1);
info->reg_id_aa64dfr1 = read_cpuid(ID_AA64DFR1_EL1);
info->reg_id_aa64isar0 = read_cpuid(ID_AA64ISAR0_EL1);
--
2.13.5
Re: [PATCH] acpi: apei: check for pending errors when probing HED type GHES entries
On 03/28/2017 02:14 PM, Tyler Baicar wrote: > If a HED type error occurs prior to GHES probing, the kernel will > never report the error. The HED driver will see that no notifiers > are registers, and clear the interrupt. ..."registers" or "registered"? > This becomes a more serious problem with firmware that supports > GHESv2 acknowledgements from the kernel. The firmware will populate > the error and wait for the kernel ack. But since the kernel will > never process the error we get into a state that the firmware will > not send any more errors and the kernel will never see or ack the > original error. > > Check for pending errors when probing HED type GHES entries to > avoid the above situation. > > This patch is based on Shiju's patch that adds support for GSIV > and GPIO notification types: > https://patchwork.kernel.org/patch/9628817/ > > Signed-off-by: Tyler Baicar <tbai...@codeaurora.org> > --- > drivers/acpi/apei/ghes.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index fd39929..cf5e938 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -1035,6 +1035,7 @@ static int ghes_probe(struct platform_device *ghes_dev) > register_acpi_hed_notifier(_notifier_hed); > list_add_rcu(>list, _hed); > mutex_unlock(_list_mutex); > + ghes_proc(ghes); > break; > case ACPI_HEST_NOTIFY_NMI: > ghes_nmi_add(ghes); > -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH] acpi: apei: check for pending errors when probing HED type GHES entries
On 03/28/2017 02:14 PM, Tyler Baicar wrote: > If a HED type error occurs prior to GHES probing, the kernel will > never report the error. The HED driver will see that no notifiers > are registers, and clear the interrupt. ..."registers" or "registered"? > This becomes a more serious problem with firmware that supports > GHESv2 acknowledgements from the kernel. The firmware will populate > the error and wait for the kernel ack. But since the kernel will > never process the error we get into a state that the firmware will > not send any more errors and the kernel will never see or ack the > original error. > > Check for pending errors when probing HED type GHES entries to > avoid the above situation. > > This patch is based on Shiju's patch that adds support for GSIV > and GPIO notification types: > https://patchwork.kernel.org/patch/9628817/ > > Signed-off-by: Tyler Baicar > --- > drivers/acpi/apei/ghes.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index fd39929..cf5e938 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -1035,6 +1035,7 @@ static int ghes_probe(struct platform_device *ghes_dev) > register_acpi_hed_notifier(_notifier_hed); > list_add_rcu(>list, _hed); > mutex_unlock(_list_mutex); > + ghes_proc(ghes); > break; > case ACPI_HEST_NOTIFY_NMI: > ghes_nmi_add(ghes); > -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [Update][PATCH v9.1 15/15] irqchip: mbigen: Add ACPI support
On 03/28/2017 06:21 AM, Hanjun Guo wrote:
> With the preparation of platform msi support and interrupt producer
> in commit d44fa3d46079 ("ACPI: Add support for ResourceSource/IRQ
> domain mapping"), we can add mbigen ACPI support now.
>
> Now that the major framework changes are ready, we just need to add
> the ACPI probe code which creates the irqdomain for devices connecting
> to it.
>
> In order to create the irqdomain, we need to know the number of hw
> irqs as input which is provided by mbigen. In DT case, we are using
> "num-pins" property to describe it, and we will take advantage of
> that too using _DSD in ACPI as there is no standard way of describe
> it in ACPI way, also according to the _DSD rule described in
> Documentation/acpi/DSD-properties-rules.txt, it doesn't break
> the rules.
>
> The DSDT is represented as below:
>
> For mbigen,
> Device(MBI0) {
> Name(_HID, "HISI0152")
> Name(_UID, Zero)
> Name(_CRS, ResourceTemplate() {
> Memory32Fixed(ReadWrite, 0xa008, 0x1)
> })
>
> Name(_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"num-pins", 378}
> }
> })
> }
>
> For devices,
> Device(SAS0) {
> Name(_HID, "HISI")
> Name(_UID, Zero)
> Name(_CRS, ResourceTemplate() {
> Memory32Fixed(ReadWrite, 0xb003, 0x1)
>Interrupt(ResourceConsumer,..., "\_SB.MBI0") {12, ...}
> })
> }
>
> So for the devices connected to the mbigen, as we clearly say that
> it refers to a specific interrupt controller (mbigen), we can get
> the virq from mbigen's irqdomain once it's created successfully.
>
> Signed-off-by: Hanjun Guo <hanjun@linaro.org>
> Signed-off-by: MaJun <majun...@huawei.com>
> Cc: Al Stone <a...@redhat.com>
> Cc: Darren Hart <dvh...@infradead.org>
> Cc: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
> Cc: Marc Zyngier <marc.zyng...@arm.com>
> ---
> drivers/irqchip/irq-mbigen.c | 75
> ++--
> 1 file changed, 72 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> index 3756408..061cdb8 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -16,6 +16,7 @@
> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include
> #include
> #include
> #include
> @@ -180,7 +181,7 @@ static int mbigen_domain_translate(struct irq_domain *d,
> unsigned long *hwirq,
> unsigned int *type)
> {
> - if (is_of_node(fwspec->fwnode)) {
> + if (is_of_node(fwspec->fwnode) || is_acpi_device_node(fwspec->fwnode)) {
> if (fwspec->param_count != 2)
> return -EINVAL;
>
> @@ -271,6 +272,58 @@ static int mbigen_of_create_domain(struct
> platform_device *pdev,
> return 0;
> }
>
> +#ifdef CONFIG_ACPI
> +static int mbigen_acpi_create_domain(struct platform_device *pdev,
> + struct mbigen_device *mgn_chip)
> +{
> + struct irq_domain *domain;
> + u32 num_pins = 0;
> + int ret;
> +
> + /*
> + * "num-pins" is the total number of interrupt pins implemented in
> + * this mbigen instance, and mbigen is an interrupt controller
> + * connected to ITS converting wired interrupts into MSI, so we
> + * use "num-pins" to alloc MSI vectors which are needed by client
> + * devices connected to it.
> + *
> + * Here is the DSDT device node used for mbigen in firmware:
> + * Device(MBI0) {
> + * Name(_HID, "HISI0152")
> + * Name(_UID, Zero)
> + * Name(_CRS, ResourceTemplate() {
> + * Memory32Fixed(ReadWrite, 0xa008, 0x1)
> + * })
> + *
> + * Name(_DSD, Package () {
> + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + * Package () {
> + * Package () {"num-pins", 378}
> + * }
> + * })
> + * }
> + */
Excellent. These comments I really like. Thanks for doing this!
> + ret =
Re: [Update][PATCH v9.1 15/15] irqchip: mbigen: Add ACPI support
On 03/28/2017 06:21 AM, Hanjun Guo wrote:
> With the preparation of platform msi support and interrupt producer
> in commit d44fa3d46079 ("ACPI: Add support for ResourceSource/IRQ
> domain mapping"), we can add mbigen ACPI support now.
>
> Now that the major framework changes are ready, we just need to add
> the ACPI probe code which creates the irqdomain for devices connecting
> to it.
>
> In order to create the irqdomain, we need to know the number of hw
> irqs as input which is provided by mbigen. In DT case, we are using
> "num-pins" property to describe it, and we will take advantage of
> that too using _DSD in ACPI as there is no standard way of describe
> it in ACPI way, also according to the _DSD rule described in
> Documentation/acpi/DSD-properties-rules.txt, it doesn't break
> the rules.
>
> The DSDT is represented as below:
>
> For mbigen,
> Device(MBI0) {
> Name(_HID, "HISI0152")
> Name(_UID, Zero)
> Name(_CRS, ResourceTemplate() {
> Memory32Fixed(ReadWrite, 0xa008, 0x1)
> })
>
> Name(_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"num-pins", 378}
> }
> })
> }
>
> For devices,
> Device(SAS0) {
> Name(_HID, "HISI")
> Name(_UID, Zero)
> Name(_CRS, ResourceTemplate() {
> Memory32Fixed(ReadWrite, 0xb003, 0x1)
>Interrupt(ResourceConsumer,..., "\_SB.MBI0") {12, ...}
> })
> }
>
> So for the devices connected to the mbigen, as we clearly say that
> it refers to a specific interrupt controller (mbigen), we can get
> the virq from mbigen's irqdomain once it's created successfully.
>
> Signed-off-by: Hanjun Guo
> Signed-off-by: MaJun
> Cc: Al Stone
> Cc: Darren Hart
> Cc: Lorenzo Pieralisi
> Cc: Marc Zyngier
> ---
> drivers/irqchip/irq-mbigen.c | 75
> ++--
> 1 file changed, 72 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> index 3756408..061cdb8 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -16,6 +16,7 @@
> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include
> #include
> #include
> #include
> @@ -180,7 +181,7 @@ static int mbigen_domain_translate(struct irq_domain *d,
> unsigned long *hwirq,
> unsigned int *type)
> {
> - if (is_of_node(fwspec->fwnode)) {
> + if (is_of_node(fwspec->fwnode) || is_acpi_device_node(fwspec->fwnode)) {
> if (fwspec->param_count != 2)
> return -EINVAL;
>
> @@ -271,6 +272,58 @@ static int mbigen_of_create_domain(struct
> platform_device *pdev,
> return 0;
> }
>
> +#ifdef CONFIG_ACPI
> +static int mbigen_acpi_create_domain(struct platform_device *pdev,
> + struct mbigen_device *mgn_chip)
> +{
> + struct irq_domain *domain;
> + u32 num_pins = 0;
> + int ret;
> +
> + /*
> + * "num-pins" is the total number of interrupt pins implemented in
> + * this mbigen instance, and mbigen is an interrupt controller
> + * connected to ITS converting wired interrupts into MSI, so we
> + * use "num-pins" to alloc MSI vectors which are needed by client
> + * devices connected to it.
> + *
> + * Here is the DSDT device node used for mbigen in firmware:
> + * Device(MBI0) {
> + * Name(_HID, "HISI0152")
> + * Name(_UID, Zero)
> + * Name(_CRS, ResourceTemplate() {
> + * Memory32Fixed(ReadWrite, 0xa008, 0x1)
> + * })
> + *
> + * Name(_DSD, Package () {
> + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + * Package () {
> + * Package () {"num-pins", 378}
> + * }
> + * })
> + * }
> + */
Excellent. These comments I really like. Thanks for doing this!
> + ret = device_property_read_u32(>dev, "num-pins", _pins);
> + if (ret || num_pins == 0)
> + return -EINVAL;
> +
> + domain =
Re: [PATCH v9 15/15] irqchip: mbigen: Add ACPI support
Package ()
>> {
>> Package () {"num-pins", xxx}
>> }
>> })
>> }
Please fix the indentation here; the ASL parser doesn't care, but I don't
see '}'s as clearly as it does :). In particular, _DSD usage *must* be for
a device and it does not look like that if one doesn't carefully count all
the parentheses and braces.
>>
>> For devices,
>>Device(COM0) {
>> Name(_HID, "ACPIIDxx")
>> Name(_UID, Zero)
>> Name(_CRS, ResourceTemplate() {
>> Memory32Fixed(ReadWrite, 0xb003, 0x1)
>> Interrupt(ResourceConsumer,..., "\_SB.MBI0") {12}
>> })
>> }
>>
>>
>> Marc, Lorenzo if you are ok with the above we will submit v10 based on
>> this...
>
> I am ok with it. I am not 100% up-to-date on what's the status on _DSD
> bindings/review/guidelines but it would be certainly a good idea to
> kickstart the process for MBIgen which basically means following this
> as far as I know (and post to the relevant mailing list):
>
> https://github.com/ahs3/dsd/blob/master/documentation/process_rules.txt
So, let's correct this bit: consider that URL deprecated, please, and refer
instead to the text that was agreed upon and in the kernel tree:
Documentation/acpi/DSD-properties-rules.txt
> Al and Darren may add to that as they have more insights.
Since this use of _DSD is very specific to this device, and this device
only, I don't have any real objections. I will say that "num-pins" is
not terribly descriptive so it would be really good to either use a much
more descriptive name or add plenty of commentary in the code -- and
preferably both. I would recommend including the ASL in the code comments,
with a description of how the property is to be used and what it means.
> I would like to send IORT patches to Catalin as soon as possible so
> as Marc pointed out the sooner we sort this out the better.
>
> Thanks,
> Lorenzo
>
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH v9 15/15] irqchip: mbigen: Add ACPI support
{
>> Package () {"num-pins", xxx}
>> }
>> })
>> }
Please fix the indentation here; the ASL parser doesn't care, but I don't
see '}'s as clearly as it does :). In particular, _DSD usage *must* be for
a device and it does not look like that if one doesn't carefully count all
the parentheses and braces.
>>
>> For devices,
>>Device(COM0) {
>> Name(_HID, "ACPIIDxx")
>> Name(_UID, Zero)
>> Name(_CRS, ResourceTemplate() {
>> Memory32Fixed(ReadWrite, 0xb003, 0x1)
>> Interrupt(ResourceConsumer,..., "\_SB.MBI0") {12}
>> })
>> }
>>
>>
>> Marc, Lorenzo if you are ok with the above we will submit v10 based on
>> this...
>
> I am ok with it. I am not 100% up-to-date on what's the status on _DSD
> bindings/review/guidelines but it would be certainly a good idea to
> kickstart the process for MBIgen which basically means following this
> as far as I know (and post to the relevant mailing list):
>
> https://github.com/ahs3/dsd/blob/master/documentation/process_rules.txt
So, let's correct this bit: consider that URL deprecated, please, and refer
instead to the text that was agreed upon and in the kernel tree:
Documentation/acpi/DSD-properties-rules.txt
> Al and Darren may add to that as they have more insights.
Since this use of _DSD is very specific to this device, and this device
only, I don't have any real objections. I will say that "num-pins" is
not terribly descriptive so it would be really good to either use a much
more descriptive name or add plenty of commentary in the code -- and
preferably both. I would recommend including the ASL in the code comments,
with a description of how the property is to be used and what it means.
> I would like to send IORT patches to Catalin as soon as possible so
> as Marc pointed out the sooner we sort this out the better.
>
> Thanks,
> Lorenzo
>
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF
On 01/25/2017 04:27 PM, Dmitry Torokhov wrote: > On Wed, Jan 25, 2017 at 02:44:10PM -0700, Al Stone wrote: >> >> But, to the point of some of the other discussion on the thread, this ACPI >> sort >> of power management is a very, very different model than DT so that >> intertwining >> the two models is highly unlikely to work, IMHO. > > And yet this is something that is sorely needed. If you look, for > example, at drivers in drivers/input/*, then all non-SOC-specific > devices can easily find their way onto both ACPI-based and DT-based > systems (not mentioning legacy-style boards). Having two distinct power > schemes implemented in drivers will lead to many problems. I really can't speak to those sorts of systems; where I deal with ACPI is on enterprise-class server systems which seldom have a graphics card, much less input devices other than a keyboard. And in general, those systems are required to use only ACPI. If a vendor wants their device to work on such a system, they need to provide a driver that works with ACPI. It may also work with DT, but in this environment it doesn't matter. Whether or not there are two power schemes is a moot point. We have DT and we have ACPI, and they have very different schemes for power management, so we're already there. And so far, my experience has been that as long as the ACPI and DT parts of the driver are kept disjoint when the models diverge, and share code when they are semantically absolutely identical, things work pretty well. > Having unified way of describing hardware is how _DSD came about, right? > Nobody wanted to write and maintain and test two separate ways of > describing properties when one was already implemented and working. I can't speak for those that proposed _DSD to be part of the ACPI spec, but no, it was not meant as a unified way of describing hardware, as far as I can remember from the ASWG discussions I was part of. The intent, as I recall it, was to provide some of the same flexibility to ASL that was available in DT. At the time, power managment was even discussed as one of the areas where the DT model and the ACPI model clashed. -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF
On 01/25/2017 04:27 PM, Dmitry Torokhov wrote: > On Wed, Jan 25, 2017 at 02:44:10PM -0700, Al Stone wrote: >> >> But, to the point of some of the other discussion on the thread, this ACPI >> sort >> of power management is a very, very different model than DT so that >> intertwining >> the two models is highly unlikely to work, IMHO. > > And yet this is something that is sorely needed. If you look, for > example, at drivers in drivers/input/*, then all non-SOC-specific > devices can easily find their way onto both ACPI-based and DT-based > systems (not mentioning legacy-style boards). Having two distinct power > schemes implemented in drivers will lead to many problems. I really can't speak to those sorts of systems; where I deal with ACPI is on enterprise-class server systems which seldom have a graphics card, much less input devices other than a keyboard. And in general, those systems are required to use only ACPI. If a vendor wants their device to work on such a system, they need to provide a driver that works with ACPI. It may also work with DT, but in this environment it doesn't matter. Whether or not there are two power schemes is a moot point. We have DT and we have ACPI, and they have very different schemes for power management, so we're already there. And so far, my experience has been that as long as the ACPI and DT parts of the driver are kept disjoint when the models diverge, and share code when they are semantically absolutely identical, things work pretty well. > Having unified way of describing hardware is how _DSD came about, right? > Nobody wanted to write and maintain and test two separate ways of > describing properties when one was already implemented and working. I can't speak for those that proposed _DSD to be part of the ACPI spec, but no, it was not meant as a unified way of describing hardware, as far as I can remember from the ASWG discussions I was part of. The intent, as I recall it, was to provide some of the same flexibility to ASL that was available in DT. At the time, power managment was even discussed as one of the areas where the DT model and the ACPI model clashed. -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF
On 01/25/2017 12:27 PM, Dmitry Torokhov wrote:
> On Wed, Jan 25, 2017 at 10:44:32AM -0800, Dmitry Torokhov wrote:
>> On Wed, Jan 25, 2017 at 06:29:55PM +, Mark Brown wrote:
>>> On Wed, Jan 25, 2017 at 06:23:20PM +, Mark Rutland wrote:
>>>> On Wed, Jan 25, 2017 at 08:56:42AM -0800, Furquan Shaikh wrote:
>>>
>>>>> That is the reason why the recent change to add ACPI support to fixed
>>>>> regulators was done
>>>>> (https://github.com/torvalds/linux/blob/master/drivers/regulator/fixed.c#L100).
>>>
>>>> To be honest, I'm surprised this got merged.
>>>
>>> My understanding was that it was instantiated from another device as an
>>> implementation detail of that device, letting it say "this GPIO should
>>> be handled as a regulator".
>>>
>>>> Mark, this was added in this cycle; can we please rip that out for now?
>>>
>>> If it's instantiated directly we probably should.
>>>
>>>> We can certainly come up with something that allows drivers to support
>>>> both, but trying to do this without updating drivers opens a huge set of
>>>> problems.
>>>
>>> I think there's a reasonable chance that any ACPI specs could be written
>>> in such a way as to allow transparent support in Linux, the main thing
>>> I'd worry about is naming issues.
>>
>> So if I am reading this correctly, currently ACPI does not expose power
>> supplies directly, but rather ties them to the device power state (D0,
>> D3cold, etc). Linux drivers do not usually follow that state model and
>> expect to have all their power supplies be given to them and then
>> figures out what to do with them itself. Given that, what do we do? Do
>> we map only entries from _PR3 so they are available to drivers via
>> regulator_get()? Or we ask the standard to add method enumerating all
>> supplies?
>
> For the record, the main issue for the drivers, which is being solved by
> exposing power supplies to the driver, is the following:
>
> 1. We suspend the device. Since there is no regulators the driver
> assumes that it will retain it's state upon resume
> 2. System goes into some sleep state
> 3. System wakes up
> 4. Device goes through resume, normally disabling wakeup interrupt and
> enabling normal processing
> 5. We end up with non functioning device because the firmware actually
> cut the power off without the driver knowing anything about it.
>
> I would really hate to go through _every_ driver and add the following
> code to the resume path:
>
> #if IS_ENABLED(CONFIG_ACPI)
> if (acpi_device_was_powered_off_between_suspend_and_now(dev)) {
> completely_reinitialize_device(dev);
> }
> #endif
>
> Thanks.
>
Please see Sections 3.2-3.5 (3.6, too, for a broader picture) in the ACPI spec
[0] for an overview of ACPI power management. Section 7 of the spec [0] adds
the details on how the firmware and OS are to cooperate in managing power.
In those sections, ACPI defines a PowerResource object, aka a power supply of
some flavor. ACPI devices are then connected to that power resource. So, the
spec may already have what you need defined. Further, the code in drivers/acpi
/device_pm.c and drivers/acpi/power.c may handle the situations described -- I
believe that's their intent, at any rate. If the ACPI ASL is written
properly, I don't think the device driver would have to add the code shown
above; it would just be handled via the acpi driver and ASL. The ACPI model is
specifically designed so that drivers don't have to know these sorts of things,
so that the hardware and firmware underneath the OS can change over time
without having to change the OS -- the OS just needs to know how to talk to
ACPI.
But, to the point of some of the other discussion on the thread, this ACPI sort
of power management is a very, very different model than DT so that intertwining
the two models is highly unlikely to work, IMHO.
[0] http://www.uefi.org/specifications -- version 6.1, specifically.
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF
On 01/25/2017 12:27 PM, Dmitry Torokhov wrote:
> On Wed, Jan 25, 2017 at 10:44:32AM -0800, Dmitry Torokhov wrote:
>> On Wed, Jan 25, 2017 at 06:29:55PM +, Mark Brown wrote:
>>> On Wed, Jan 25, 2017 at 06:23:20PM +, Mark Rutland wrote:
>>>> On Wed, Jan 25, 2017 at 08:56:42AM -0800, Furquan Shaikh wrote:
>>>
>>>>> That is the reason why the recent change to add ACPI support to fixed
>>>>> regulators was done
>>>>> (https://github.com/torvalds/linux/blob/master/drivers/regulator/fixed.c#L100).
>>>
>>>> To be honest, I'm surprised this got merged.
>>>
>>> My understanding was that it was instantiated from another device as an
>>> implementation detail of that device, letting it say "this GPIO should
>>> be handled as a regulator".
>>>
>>>> Mark, this was added in this cycle; can we please rip that out for now?
>>>
>>> If it's instantiated directly we probably should.
>>>
>>>> We can certainly come up with something that allows drivers to support
>>>> both, but trying to do this without updating drivers opens a huge set of
>>>> problems.
>>>
>>> I think there's a reasonable chance that any ACPI specs could be written
>>> in such a way as to allow transparent support in Linux, the main thing
>>> I'd worry about is naming issues.
>>
>> So if I am reading this correctly, currently ACPI does not expose power
>> supplies directly, but rather ties them to the device power state (D0,
>> D3cold, etc). Linux drivers do not usually follow that state model and
>> expect to have all their power supplies be given to them and then
>> figures out what to do with them itself. Given that, what do we do? Do
>> we map only entries from _PR3 so they are available to drivers via
>> regulator_get()? Or we ask the standard to add method enumerating all
>> supplies?
>
> For the record, the main issue for the drivers, which is being solved by
> exposing power supplies to the driver, is the following:
>
> 1. We suspend the device. Since there is no regulators the driver
> assumes that it will retain it's state upon resume
> 2. System goes into some sleep state
> 3. System wakes up
> 4. Device goes through resume, normally disabling wakeup interrupt and
> enabling normal processing
> 5. We end up with non functioning device because the firmware actually
> cut the power off without the driver knowing anything about it.
>
> I would really hate to go through _every_ driver and add the following
> code to the resume path:
>
> #if IS_ENABLED(CONFIG_ACPI)
> if (acpi_device_was_powered_off_between_suspend_and_now(dev)) {
> completely_reinitialize_device(dev);
> }
> #endif
>
> Thanks.
>
Please see Sections 3.2-3.5 (3.6, too, for a broader picture) in the ACPI spec
[0] for an overview of ACPI power management. Section 7 of the spec [0] adds
the details on how the firmware and OS are to cooperate in managing power.
In those sections, ACPI defines a PowerResource object, aka a power supply of
some flavor. ACPI devices are then connected to that power resource. So, the
spec may already have what you need defined. Further, the code in drivers/acpi
/device_pm.c and drivers/acpi/power.c may handle the situations described -- I
believe that's their intent, at any rate. If the ACPI ASL is written
properly, I don't think the device driver would have to add the code shown
above; it would just be handled via the acpi driver and ASL. The ACPI model is
specifically designed so that drivers don't have to know these sorts of things,
so that the hardware and firmware underneath the OS can change over time
without having to change the OS -- the OS just needs to know how to talk to
ACPI.
But, to the point of some of the other discussion on the thread, this ACPI sort
of power management is a very, very different model than DT so that intertwining
the two models is highly unlikely to work, IMHO.
[0] http://www.uefi.org/specifications -- version 6.1, specifically.
--
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---
Re: [PATCH] ACPI: allow compilation with bare metal compilers
On 11/15/2016 08:43 AM, Moore, Robert wrote: > The design for all of this is as follows: > > 1) OS-dependent includes > 2) Compiler-specific includes > 3) acenv.h is the master file that pulls in the correct headers (one > compiler, and one OS) Sure, that's understood from the current structure. The issue is #3 -- there is no OS, in the sense of a run-time environment, so that assumption is incorrect in this case. All we know is that we're compiling the kernel and it needs to be able to run in complete isolation. > So, I think I see a couple of possible solutions for you: > > 1) If you are using GCC, the __GNUC__ symbol should already be defined. Right, which is what this patch is relying on. > 2) If "aclinux.h" works for you, we can either add a conditional case that > would apply to your environment, or: I'm confused; I thought that was what this patch was doing. What is being suggested instead? > 2a) You could define _LINUX in your gcc invocations. This option might force changes in the kernel build which is otherwise not affected; this is only an issue with drivers/acpi/acpica/*. It may also cause ACPICA to build incorrectly for other OSs, but I'll have to look at that. Off hand, it seems like this would cause more problems with acenv.h. > > Because ACPICA supports many different environments, we don't want to have a > "default" case which in a sense would only be an attempt to guess what the > user intended. We want to have a clear error that tells the user that > something important needs to be done before the code can be compiled. Understood, which is why I left the #error case; the intent is only to have a default when compiling Linux for bare-metal environments using GCC. Perhaps this is just a Linux-only modification... > Bob > > >> -Original Message- >> From: Al Stone [mailto:a...@redhat.com] >> Sent: Monday, November 14, 2016 3:09 PM >> To: linux-a...@vger.kernel.org; de...@acpica.org; linux- >> ker...@vger.kernel.org >> Cc: Al Stone <a...@redhat.com>; Rafael J . Wysocki <r...@rjwysocki.net>; >> Len Brown <l...@kernel.org>; Moore, Robert <robert.mo...@intel.com>; >> Zheng, Lv <lv.zh...@intel.com> >> Subject: [PATCH] ACPI: allow compilation with bare metal compilers >> >> The ACPICA subsystem of the ACPI driver sets up a compilation >> environment for itself, adding in multiple typedefs unique to ACPICA >> that depend on where ACPICA will be used. >> >> The vast majority of such environments (Linux, QNX, ...) have an >> environment defined by the acenv.h header file. When using a Linaro >> compiler [1] specifically built to be used in an embedded environment >> with perhaps a kernel and an init process as the only things running, >> there is no environment defined for ACPICA so the typedefs it needs are >> not set up, causing compilation to fail badly unless ACPI is completely >> disabled. >> Since ACPI is enabled in the default config for the kernel, the >> compilation failure is fairly obvious. >> >> This may not be the optimal solution, but add in to the ACPI header file >> include/acpi/platform/acenv.h a default so that if GCC is being used, >> and all else fails, assume that we are going to be in a Linux-like >> environment and re-use the environment definition for Linux. This >> allows us to build a kernel using this compiler [1] with or without >> ACPI. >> >> [1] >> https://releases.linaro.org/components/toolchain/binaries/latest/aarch64 >> -elff/gcc-linaro-6.1.1-2016-08-x86_64_aarch64-elf.tar.xz >> >> Signed-off-by: Al Stone <a...@redhat.com> >> Cc: Rafael J. Wysocki <r...@rjwysocki.net> >> Cc: Len Brown <l...@kernel.org> >> Cc: Robert Moore <robert.mo...@intel.com> >> Cc: Lv Zheng <lv.zh...@intel.com> >> --- >> include/acpi/platform/acenv.h | 15 +++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/include/acpi/platform/acenv.h >> b/include/acpi/platform/acenv.h index 34cce72..cdd1cd6 100644 >> --- a/include/acpi/platform/acenv.h >> +++ b/include/acpi/platform/acenv.h >> @@ -234,6 +234,21 @@ >> #elif defined(_AED_EFI) || defined(_GNU_EFI) || defined(_EDK2_EFI) >> #include "acefi.h" >> >> +/* >> + * Up to this point, we've been looking for specific environments. In >> + * some cases, there is no environment, and we're just working on bare >> + * metal. However, since we're compiling the Linux kernel, let's just >> + * pretend we're in a Linux environment. >> + */ >> +#elif defined(__GNUC__) && !defined(__INTEL_COMPILER) #if >> +!defined(_LINUX) #define _LINUX #endif #if !defined(__linux__) #define >> +__linux__ #endif #include >> + >> #else >> >> /* Unknown environment */ >> -- >> 2.10.2 > -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH] ACPI: allow compilation with bare metal compilers
On 11/15/2016 08:43 AM, Moore, Robert wrote: > The design for all of this is as follows: > > 1) OS-dependent includes > 2) Compiler-specific includes > 3) acenv.h is the master file that pulls in the correct headers (one > compiler, and one OS) Sure, that's understood from the current structure. The issue is #3 -- there is no OS, in the sense of a run-time environment, so that assumption is incorrect in this case. All we know is that we're compiling the kernel and it needs to be able to run in complete isolation. > So, I think I see a couple of possible solutions for you: > > 1) If you are using GCC, the __GNUC__ symbol should already be defined. Right, which is what this patch is relying on. > 2) If "aclinux.h" works for you, we can either add a conditional case that > would apply to your environment, or: I'm confused; I thought that was what this patch was doing. What is being suggested instead? > 2a) You could define _LINUX in your gcc invocations. This option might force changes in the kernel build which is otherwise not affected; this is only an issue with drivers/acpi/acpica/*. It may also cause ACPICA to build incorrectly for other OSs, but I'll have to look at that. Off hand, it seems like this would cause more problems with acenv.h. > > Because ACPICA supports many different environments, we don't want to have a > "default" case which in a sense would only be an attempt to guess what the > user intended. We want to have a clear error that tells the user that > something important needs to be done before the code can be compiled. Understood, which is why I left the #error case; the intent is only to have a default when compiling Linux for bare-metal environments using GCC. Perhaps this is just a Linux-only modification... > Bob > > >> -Original Message- >> From: Al Stone [mailto:a...@redhat.com] >> Sent: Monday, November 14, 2016 3:09 PM >> To: linux-a...@vger.kernel.org; de...@acpica.org; linux- >> ker...@vger.kernel.org >> Cc: Al Stone ; Rafael J . Wysocki ; >> Len Brown ; Moore, Robert ; >> Zheng, Lv >> Subject: [PATCH] ACPI: allow compilation with bare metal compilers >> >> The ACPICA subsystem of the ACPI driver sets up a compilation >> environment for itself, adding in multiple typedefs unique to ACPICA >> that depend on where ACPICA will be used. >> >> The vast majority of such environments (Linux, QNX, ...) have an >> environment defined by the acenv.h header file. When using a Linaro >> compiler [1] specifically built to be used in an embedded environment >> with perhaps a kernel and an init process as the only things running, >> there is no environment defined for ACPICA so the typedefs it needs are >> not set up, causing compilation to fail badly unless ACPI is completely >> disabled. >> Since ACPI is enabled in the default config for the kernel, the >> compilation failure is fairly obvious. >> >> This may not be the optimal solution, but add in to the ACPI header file >> include/acpi/platform/acenv.h a default so that if GCC is being used, >> and all else fails, assume that we are going to be in a Linux-like >> environment and re-use the environment definition for Linux. This >> allows us to build a kernel using this compiler [1] with or without >> ACPI. >> >> [1] >> https://releases.linaro.org/components/toolchain/binaries/latest/aarch64 >> -elff/gcc-linaro-6.1.1-2016-08-x86_64_aarch64-elf.tar.xz >> >> Signed-off-by: Al Stone >> Cc: Rafael J. Wysocki >> Cc: Len Brown >> Cc: Robert Moore >> Cc: Lv Zheng >> --- >> include/acpi/platform/acenv.h | 15 +++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/include/acpi/platform/acenv.h >> b/include/acpi/platform/acenv.h index 34cce72..cdd1cd6 100644 >> --- a/include/acpi/platform/acenv.h >> +++ b/include/acpi/platform/acenv.h >> @@ -234,6 +234,21 @@ >> #elif defined(_AED_EFI) || defined(_GNU_EFI) || defined(_EDK2_EFI) >> #include "acefi.h" >> >> +/* >> + * Up to this point, we've been looking for specific environments. In >> + * some cases, there is no environment, and we're just working on bare >> + * metal. However, since we're compiling the Linux kernel, let's just >> + * pretend we're in a Linux environment. >> + */ >> +#elif defined(__GNUC__) && !defined(__INTEL_COMPILER) #if >> +!defined(_LINUX) #define _LINUX #endif #if !defined(__linux__) #define >> +__linux__ #endif #include >> + >> #else >> >> /* Unknown environment */ >> -- >> 2.10.2 > -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH] ACPI: allow compilation with bare metal compilers
My bad. Adding Will Deacon who originally reported the error. On 11/14/2016 04:08 PM, Al Stone wrote: > The ACPICA subsystem of the ACPI driver sets up a compilation environment > for itself, adding in multiple typedefs unique to ACPICA that depend on > where ACPICA will be used. > > The vast majority of such environments (Linux, QNX, ...) have an environment > defined by the acenv.h header file. When using a Linaro compiler [1] > specifically built to be used in an embedded environment with perhaps a > kernel and an init process as the only things running, there is no > environment defined for ACPICA so the typedefs it needs are not set up, > causing compilation to fail badly unless ACPI is completely disabled. > Since ACPI is enabled in the default config for the kernel, the compilation > failure is fairly obvious. > > This may not be the optimal solution, but add in to the ACPI header file > include/acpi/platform/acenv.h a default so that if GCC is being used, and > all else fails, assume that we are going to be in a Linux-like environment > and re-use the environment definition for Linux. This allows us to build > a kernel using this compiler [1] with or without ACPI. > > [1] > https://releases.linaro.org/components/toolchain/binaries/latest/aarch64-elff/gcc-linaro-6.1.1-2016-08-x86_64_aarch64-elf.tar.xz > > Signed-off-by: Al Stone <a...@redhat.com> > Cc: Rafael J. Wysocki <r...@rjwysocki.net> > Cc: Len Brown <l...@kernel.org> > Cc: Robert Moore <robert.mo...@intel.com> > Cc: Lv Zheng <lv.zh...@intel.com> > --- > include/acpi/platform/acenv.h | 15 +++ > 1 file changed, 15 insertions(+) > > diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h > index 34cce72..cdd1cd6 100644 > --- a/include/acpi/platform/acenv.h > +++ b/include/acpi/platform/acenv.h > @@ -234,6 +234,21 @@ > #elif defined(_AED_EFI) || defined(_GNU_EFI) || defined(_EDK2_EFI) > #include "acefi.h" > > +/* > + * Up to this point, we've been looking for specific environments. In > + * some cases, there is no environment, and we're just working on bare > + * metal. However, since we're compiling the Linux kernel, let's just > + * pretend we're in a Linux environment. > + */ > +#elif defined(__GNUC__) && !defined(__INTEL_COMPILER) > +#if !defined(_LINUX) > +#define _LINUX > +#endif > +#if !defined(__linux__) > +#define __linux__ > +#endif > +#include > + > #else > > /* Unknown environment */ > -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH] ACPI: allow compilation with bare metal compilers
My bad. Adding Will Deacon who originally reported the error. On 11/14/2016 04:08 PM, Al Stone wrote: > The ACPICA subsystem of the ACPI driver sets up a compilation environment > for itself, adding in multiple typedefs unique to ACPICA that depend on > where ACPICA will be used. > > The vast majority of such environments (Linux, QNX, ...) have an environment > defined by the acenv.h header file. When using a Linaro compiler [1] > specifically built to be used in an embedded environment with perhaps a > kernel and an init process as the only things running, there is no > environment defined for ACPICA so the typedefs it needs are not set up, > causing compilation to fail badly unless ACPI is completely disabled. > Since ACPI is enabled in the default config for the kernel, the compilation > failure is fairly obvious. > > This may not be the optimal solution, but add in to the ACPI header file > include/acpi/platform/acenv.h a default so that if GCC is being used, and > all else fails, assume that we are going to be in a Linux-like environment > and re-use the environment definition for Linux. This allows us to build > a kernel using this compiler [1] with or without ACPI. > > [1] > https://releases.linaro.org/components/toolchain/binaries/latest/aarch64-elff/gcc-linaro-6.1.1-2016-08-x86_64_aarch64-elf.tar.xz > > Signed-off-by: Al Stone > Cc: Rafael J. Wysocki > Cc: Len Brown > Cc: Robert Moore > Cc: Lv Zheng > --- > include/acpi/platform/acenv.h | 15 +++ > 1 file changed, 15 insertions(+) > > diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h > index 34cce72..cdd1cd6 100644 > --- a/include/acpi/platform/acenv.h > +++ b/include/acpi/platform/acenv.h > @@ -234,6 +234,21 @@ > #elif defined(_AED_EFI) || defined(_GNU_EFI) || defined(_EDK2_EFI) > #include "acefi.h" > > +/* > + * Up to this point, we've been looking for specific environments. In > + * some cases, there is no environment, and we're just working on bare > + * metal. However, since we're compiling the Linux kernel, let's just > + * pretend we're in a Linux environment. > + */ > +#elif defined(__GNUC__) && !defined(__INTEL_COMPILER) > +#if !defined(_LINUX) > +#define _LINUX > +#endif > +#if !defined(__linux__) > +#define __linux__ > +#endif > +#include > + > #else > > /* Unknown environment */ > -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
[PATCH] ACPI: allow compilation with bare metal compilers
The ACPICA subsystem of the ACPI driver sets up a compilation environment for itself, adding in multiple typedefs unique to ACPICA that depend on where ACPICA will be used. The vast majority of such environments (Linux, QNX, ...) have an environment defined by the acenv.h header file. When using a Linaro compiler [1] specifically built to be used in an embedded environment with perhaps a kernel and an init process as the only things running, there is no environment defined for ACPICA so the typedefs it needs are not set up, causing compilation to fail badly unless ACPI is completely disabled. Since ACPI is enabled in the default config for the kernel, the compilation failure is fairly obvious. This may not be the optimal solution, but add in to the ACPI header file include/acpi/platform/acenv.h a default so that if GCC is being used, and all else fails, assume that we are going to be in a Linux-like environment and re-use the environment definition for Linux. This allows us to build a kernel using this compiler [1] with or without ACPI. [1] https://releases.linaro.org/components/toolchain/binaries/latest/aarch64-elff/gcc-linaro-6.1.1-2016-08-x86_64_aarch64-elf.tar.xz Signed-off-by: Al Stone <a...@redhat.com> Cc: Rafael J. Wysocki <r...@rjwysocki.net> Cc: Len Brown <l...@kernel.org> Cc: Robert Moore <robert.mo...@intel.com> Cc: Lv Zheng <lv.zh...@intel.com> --- include/acpi/platform/acenv.h | 15 +++ 1 file changed, 15 insertions(+) diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h index 34cce72..cdd1cd6 100644 --- a/include/acpi/platform/acenv.h +++ b/include/acpi/platform/acenv.h @@ -234,6 +234,21 @@ #elif defined(_AED_EFI) || defined(_GNU_EFI) || defined(_EDK2_EFI) #include "acefi.h" +/* + * Up to this point, we've been looking for specific environments. In + * some cases, there is no environment, and we're just working on bare + * metal. However, since we're compiling the Linux kernel, let's just + * pretend we're in a Linux environment. + */ +#elif defined(__GNUC__) && !defined(__INTEL_COMPILER) +#if !defined(_LINUX) +#define _LINUX +#endif +#if !defined(__linux__) +#define __linux__ +#endif +#include + #else /* Unknown environment */ -- 2.10.2
[PATCH] ACPI: allow compilation with bare metal compilers
The ACPICA subsystem of the ACPI driver sets up a compilation environment for itself, adding in multiple typedefs unique to ACPICA that depend on where ACPICA will be used. The vast majority of such environments (Linux, QNX, ...) have an environment defined by the acenv.h header file. When using a Linaro compiler [1] specifically built to be used in an embedded environment with perhaps a kernel and an init process as the only things running, there is no environment defined for ACPICA so the typedefs it needs are not set up, causing compilation to fail badly unless ACPI is completely disabled. Since ACPI is enabled in the default config for the kernel, the compilation failure is fairly obvious. This may not be the optimal solution, but add in to the ACPI header file include/acpi/platform/acenv.h a default so that if GCC is being used, and all else fails, assume that we are going to be in a Linux-like environment and re-use the environment definition for Linux. This allows us to build a kernel using this compiler [1] with or without ACPI. [1] https://releases.linaro.org/components/toolchain/binaries/latest/aarch64-elff/gcc-linaro-6.1.1-2016-08-x86_64_aarch64-elf.tar.xz Signed-off-by: Al Stone Cc: Rafael J. Wysocki Cc: Len Brown Cc: Robert Moore Cc: Lv Zheng --- include/acpi/platform/acenv.h | 15 +++ 1 file changed, 15 insertions(+) diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h index 34cce72..cdd1cd6 100644 --- a/include/acpi/platform/acenv.h +++ b/include/acpi/platform/acenv.h @@ -234,6 +234,21 @@ #elif defined(_AED_EFI) || defined(_GNU_EFI) || defined(_EDK2_EFI) #include "acefi.h" +/* + * Up to this point, we've been looking for specific environments. In + * some cases, there is no environment, and we're just working on bare + * metal. However, since we're compiling the Linux kernel, let's just + * pretend we're in a Linux environment. + */ +#elif defined(__GNUC__) && !defined(__INTEL_COMPILER) +#if !defined(_LINUX) +#define _LINUX +#endif +#if !defined(__linux__) +#define __linux__ +#endif +#include + #else /* Unknown environment */ -- 2.10.2
Re: [PATCH] cpufreq: CPPC: Avoid overflow when calculating desired_perf
On 09/14/2016 09:06 PM, Hoan Tran wrote: > Hi Rafael, > > On Wed, Sep 14, 2016 at 5:50 PM, Rafael J. Wysocki <r...@rjwysocki.net> wrote: >> On Wednesday, September 14, 2016 04:08:28 PM Hoan Tran wrote: >>> This patch fixes overflow issue when calculating the desired_perf. >>> >>> Signed-off-by: Hoan Tran <hot...@apm.com> >>> --- >>> drivers/cpufreq/cppc_cpufreq.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c >>> index 894e465..3e0961e 100644 >>> --- a/drivers/cpufreq/cppc_cpufreq.c >>> +++ b/drivers/cpufreq/cppc_cpufreq.c >>> @@ -84,7 +84,8 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy >>> *policy, >>> >>> cpu = all_cpu_data[policy->cpu]; >>> >>> - cpu->perf_ctrls.desired_perf = target_freq * policy->max / >>> cppc_dmi_max_khz; >>> + cpu->perf_ctrls.desired_perf = >>> + (u64)target_freq * policy->max / cppc_dmi_max_khz; >>> freqs.old = policy->cur; >>> freqs.new = target_freq; >> >> That's on top of the CPPC material in linux-next I gather? > > Yes, it's on TOP of linux-next. > >> >> Which commit does it fix? > > This is a fix for ad38677df44b67e0f5b6c4d31e9c2734abde8ed9 (cpufreq: > CPPC: Force reporting values in KHz to fix user space interface) > http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/cpufreq/cppc_cpufreq.c?id=ad38677df44b67e0f5b6c4d31e9c2734abde8ed9 > > Thanks > Hoan > >> >> Thanks, >> Rafael >> Nice catch, Hoan. Thanks! -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
Re: [PATCH] cpufreq: CPPC: Avoid overflow when calculating desired_perf
On 09/14/2016 09:06 PM, Hoan Tran wrote: > Hi Rafael, > > On Wed, Sep 14, 2016 at 5:50 PM, Rafael J. Wysocki wrote: >> On Wednesday, September 14, 2016 04:08:28 PM Hoan Tran wrote: >>> This patch fixes overflow issue when calculating the desired_perf. >>> >>> Signed-off-by: Hoan Tran >>> --- >>> drivers/cpufreq/cppc_cpufreq.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c >>> index 894e465..3e0961e 100644 >>> --- a/drivers/cpufreq/cppc_cpufreq.c >>> +++ b/drivers/cpufreq/cppc_cpufreq.c >>> @@ -84,7 +84,8 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy >>> *policy, >>> >>> cpu = all_cpu_data[policy->cpu]; >>> >>> - cpu->perf_ctrls.desired_perf = target_freq * policy->max / >>> cppc_dmi_max_khz; >>> + cpu->perf_ctrls.desired_perf = >>> + (u64)target_freq * policy->max / cppc_dmi_max_khz; >>> freqs.old = policy->cur; >>> freqs.new = target_freq; >> >> That's on top of the CPPC material in linux-next I gather? > > Yes, it's on TOP of linux-next. > >> >> Which commit does it fix? > > This is a fix for ad38677df44b67e0f5b6c4d31e9c2734abde8ed9 (cpufreq: > CPPC: Force reporting values in KHz to fix user space interface) > http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/cpufreq/cppc_cpufreq.c?id=ad38677df44b67e0f5b6c4d31e9c2734abde8ed9 > > Thanks > Hoan > >> >> Thanks, >> Rafael >> Nice catch, Hoan. Thanks! -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com ---
