The acpi parser can set the DISABLED flag on a resource. In
setup_res() we clear all flags except MEM, so we ignore the DISABLED
flag. Add proper checks and preserve the flags.
Signed-off-by: Thomas Gleixner <[email protected]>
---
drivers/acpi/ioapic.c | 38 +++++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 17 deletions(-)
Index: tip/drivers/acpi/ioapic.c
===================================================================
--- tip.orig/drivers/acpi/ioapic.c
+++ tip/drivers/acpi/ioapic.c
@@ -40,29 +40,33 @@ struct acpi_pci_ioapic {
static LIST_HEAD(ioapic_list);
static DEFINE_MUTEX(ioapic_list_lock);
+static inline bool is_valid_mem_resource(struct resource *res)
+{
+ return !(res->flags & IORESOURCE_DISABLED) &&
+ (res->flags & IORESOURCE_MEM);
+}
+
static acpi_status setup_res(struct acpi_resource *acpi_res, void *data)
{
+ struct acpi_resource_address64 addr;
struct resource *res = data;
memset(res, 0, sizeof(*res));
- if (acpi_dev_resource_memory(acpi_res, res)) {
- res->flags &= IORESOURCE_MEM;
- if (res->flags)
- return AE_OK;
- } else if (acpi_dev_resource_address_space(acpi_res, res)) {
- struct acpi_resource_address64 addr;
+ if (acpi_dev_resource_memory(acpi_res, res))
+ return AE_OK;
- res->flags &= IORESOURCE_MEM;
- if (res->flags &&
- ACPI_SUCCESS(acpi_resource_to_address64(acpi_res, &addr)) &&
- addr.info.mem.caching != ACPI_PREFETCHABLE_MEMORY) {
- res->start += addr.translation_offset;
- res->end += addr.translation_offset;
- return AE_OK;
- }
+ if (!acpi_dev_resource_address_space(acpi_res, res) ||
+ !is_valid_mem_resource(res))
+ return AE_OK;
+ /*
+ * FIXME: This lacks a proper comment, why the resource
+ * address needs to be translated.
+ */
+ if (ACPI_SUCCESS(acpi_resource_to_address64(acpi_res, &addr)) &&
+ addr.info.mem.caching != ACPI_PREFETCHABLE_MEMORY) {
+ res->start += addr.translation_offset;
+ res->end += addr.translation_offset;
}
- res->flags = 0;
-
return AE_OK;
}
@@ -150,7 +154,7 @@ static acpi_status handle_ioapic_add(acp
res = &ioapic->res;
acpi_walk_resources(handle, METHOD_NAME__CRS, setup_res, res);
- if (res->flags == IORESOURCE_UNSET) {
+ if (!is_valid_mem_resource(res)) {
acpi_handle_warn(handle, "failed to get resource\n");
goto exit_free;
} else if (request_resource(&iomem_resource, res)) {
On Thu, Dec 11, 2014 at 11:48 AM, Thomas Gleixner <[email protected]> wrote:
> The acpi parser can set the DISABLED flag on a resource. In
> setup_res() we clear all flags except MEM, so we ignore the DISABLED
> flag. Add proper checks and preserve the flags.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> drivers/acpi/ioapic.c | 38 +++++++++++++++++++++-----------------
> 1 file changed, 21 insertions(+), 17 deletions(-)
>
> Index: tip/drivers/acpi/ioapic.c
> ===================================================================
> --- tip.orig/drivers/acpi/ioapic.c
> +++ tip/drivers/acpi/ioapic.c
> @@ -40,29 +40,33 @@ struct acpi_pci_ioapic {
> static LIST_HEAD(ioapic_list);
> static DEFINE_MUTEX(ioapic_list_lock);
>
> +static inline bool is_valid_mem_resource(struct resource *res)
> +{
> + return !(res->flags & IORESOURCE_DISABLED) &&
> + (res->flags & IORESOURCE_MEM);
> +}
> +
> static acpi_status setup_res(struct acpi_resource *acpi_res, void *data)
> {
> + struct acpi_resource_address64 addr;
> struct resource *res = data;
>
> memset(res, 0, sizeof(*res));
> - if (acpi_dev_resource_memory(acpi_res, res)) {
> - res->flags &= IORESOURCE_MEM;
> - if (res->flags)
> - return AE_OK;
> - } else if (acpi_dev_resource_address_space(acpi_res, res)) {
> - struct acpi_resource_address64 addr;
> + if (acpi_dev_resource_memory(acpi_res, res))
> + return AE_OK;
>
> - res->flags &= IORESOURCE_MEM;
> - if (res->flags &&
> - ACPI_SUCCESS(acpi_resource_to_address64(acpi_res, &addr)) &&
> - addr.info.mem.caching != ACPI_PREFETCHABLE_MEMORY) {
> - res->start += addr.translation_offset;
> - res->end += addr.translation_offset;
> - return AE_OK;
> - }
> + if (!acpi_dev_resource_address_space(acpi_res, res) ||
> + !is_valid_mem_resource(res))
> + return AE_OK;
> + /*
> + * FIXME: This lacks a proper comment, why the resource
> + * address needs to be translated.
> + */
> + if (ACPI_SUCCESS(acpi_resource_to_address64(acpi_res, &addr)) &&
> + addr.info.mem.caching != ACPI_PREFETCHABLE_MEMORY) {
> + res->start += addr.translation_offset;
> + res->end += addr.translation_offset;
> }
> - res->flags = 0;
> -
> return AE_OK;
> }
>
> @@ -150,7 +154,7 @@ static acpi_status handle_ioapic_add(acp
>
> res = &ioapic->res;
> acpi_walk_resources(handle, METHOD_NAME__CRS, setup_res, res);
> - if (res->flags == IORESOURCE_UNSET) {
> + if (!is_valid_mem_resource(res)) {
> acpi_handle_warn(handle, "failed to get resource\n");
> goto exit_free;
> } else if (request_resource(&iomem_resource, res)) {
>
>
There is minor problem about mem pref handling, original code will ignore them.
with this patch will let it follow through.
should change is_valid_mem_resource to is_valid_mem_nonpref_resource()...
+static inline bool is_valid_mem_nonpref_resource(struct resource *res)
{
return !(res->flags & IORESOURCE_DISABLED) &&
- (res->flags & IORESOURCE_MEM);
+ (res->flags & IORESOURCE_MEM) &&
+ !(res->flags & IORESOURCE_PREFETCH);
}
other than that for whole patch set:
Acked-by: Yinghai Lu <[email protected]>
Also please check attached following patch that will move code to acpi core.
Thanks
Yinghai
On Thu, 11 Dec 2014, Yinghai Lu wrote:
> On Thu, Dec 11, 2014 at 11:48 AM, Thomas Gleixner <[email protected]> wrote:
> > +static inline bool is_valid_mem_resource(struct resource *res)
> > +{
> > + return !(res->flags & IORESOURCE_DISABLED) &&
> > + (res->flags & IORESOURCE_MEM);
> > +}
> > +
> There is minor problem about mem pref handling, original code will ignore them.
Bah. I missed that in that well documented function...
> with this patch will let it follow through.
>
> should change is_valid_mem_resource to is_valid_mem_nonpref_resource()...
>
> +static inline bool is_valid_mem_nonpref_resource(struct resource *res)
> {
> return !(res->flags & IORESOURCE_DISABLED) &&
> - (res->flags & IORESOURCE_MEM);
> + (res->flags & IORESOURCE_MEM) &&
> + !(res->flags & IORESOURCE_PREFETCH);
> }
Unfortunately that does not help, because nothing sets the
IORESOURCE_PREFETCH flag. Will fix it proper.
I still have no explanation why the translation offset needs to be
applied here.
Thanks,
tglx
On Thu, Dec 11, 2014 at 11:53 PM, Thomas Gleixner <[email protected]> wrote:
>
> I still have no explanation why the translation offset needs to be
> applied here.
That should be copied from pci root bus resource setup.
But looks like only ia64 use those translation_offset?
Thanks
Yinghai
On 2014/12/12 15:53, Thomas Gleixner wrote:
> On Thu, 11 Dec 2014, Yinghai Lu wrote:
>> On Thu, Dec 11, 2014 at 11:48 AM, Thomas Gleixner <[email protected]> wrote:
>>> +static inline bool is_valid_mem_resource(struct resource *res)
>>> +{
>>> + return !(res->flags & IORESOURCE_DISABLED) &&
>>> + (res->flags & IORESOURCE_MEM);
>>> +}
>>> +
>> There is minor problem about mem pref handling, original code will ignore them.
>
> Bah. I missed that in that well documented function...
>
>> with this patch will let it follow through.
>>
>> should change is_valid_mem_resource to is_valid_mem_nonpref_resource()...
>>
>> +static inline bool is_valid_mem_nonpref_resource(struct resource *res)
>> {
>> return !(res->flags & IORESOURCE_DISABLED) &&
>> - (res->flags & IORESOURCE_MEM);
>> + (res->flags & IORESOURCE_MEM) &&
>> + !(res->flags & IORESOURCE_PREFETCH);
>> }
>
> Unfortunately that does not help, because nothing sets the
> IORESOURCE_PREFETCH flag. Will fix it proper.
>
> I still have no explanation why the translation offset needs to be
> applied here.
Hi Thomas,
I have read related section in ACPI spec, seems the addition
of translation_offset is redundant here.
Quotation from ACPI spec 5a, 6.4.3.5.1
For bridges that translate addresses across the bridge, this is the
offset that must be added to the address on the secondary side to
obtain the address on the primary side. Non-bridge devices must list
0 for all Address Translation offset bits.
Quotation from ACPI spec 5, 9.17 I/O APIC Device:
It must also contain a _CRS object that reports the base address of the
I/O APIC device. The _CRS object is required to contain only one
resource, a memory resource pointing to the I/O APIC register base.
IO APIC is not a bridge, so translation_offset should always be zero.
Thanks!
Gerry
>
> Thanks,
>
> tglx
>
On Thu, 11 Dec 2014, Yinghai Lu wrote:
> On Thu, Dec 11, 2014 at 11:48 AM, Thomas Gleixner <[email protected]> wrote:
>
> Also please check attached following patch that will move code to acpi core.
Fun. I came up with more or less the same patch (properly split into
seperate patches) yesterday morning, but decided to keep it out of the
series because it looked ugly.
We really should have something like this:
struct translated_rescource {
struct resource res;
u64 offset;
};
and a proper acpi function for it. Adding the offset could be done
there as well. That will simplify the magic in x86/pci/acpi as you
only have to deal with a single storage place.
Thanks,
tglx
On Fri, 12 Dec 2014, Jiang Liu wrote:
> On 2014/12/12 15:53, Thomas Gleixner wrote:
> > On Thu, 11 Dec 2014, Yinghai Lu wrote:
> >> On Thu, Dec 11, 2014 at 11:48 AM, Thomas Gleixner <[email protected]> wrote:
> >>> +static inline bool is_valid_mem_resource(struct resource *res)
> >>> +{
> >>> + return !(res->flags & IORESOURCE_DISABLED) &&
> >>> + (res->flags & IORESOURCE_MEM);
> >>> +}
> >>> +
> >> There is minor problem about mem pref handling, original code will ignore them.
> >
> > Bah. I missed that in that well documented function...
> >
> >> with this patch will let it follow through.
> >>
> >> should change is_valid_mem_resource to is_valid_mem_nonpref_resource()...
> >>
> >> +static inline bool is_valid_mem_nonpref_resource(struct resource *res)
> >> {
> >> return !(res->flags & IORESOURCE_DISABLED) &&
> >> - (res->flags & IORESOURCE_MEM);
> >> + (res->flags & IORESOURCE_MEM) &&
> >> + !(res->flags & IORESOURCE_PREFETCH);
> >> }
> >
> > Unfortunately that does not help, because nothing sets the
> > IORESOURCE_PREFETCH flag. Will fix it proper.
> >
> > I still have no explanation why the translation offset needs to be
> > applied here.
> Hi Thomas,
> I have read related section in ACPI spec, seems the addition
> of translation_offset is redundant here.
>
> Quotation from ACPI spec 5a, 6.4.3.5.1
> For bridges that translate addresses across the bridge, this is the
> offset that must be added to the address on the secondary side to
> obtain the address on the primary side. Non-bridge devices must list
> 0 for all Address Translation offset bits.
>
> Quotation from ACPI spec 5, 9.17 I/O APIC Device:
> It must also contain a _CRS object that reports the base address of the
> I/O APIC device. The _CRS object is required to contain only one
> resource, a memory resource pointing to the I/O APIC register base.
>
> IO APIC is not a bridge, so translation_offset should always be zero.
Right and we really need a proper interface for this on the acpi side,
so we can avoid all that dance in the usage sites.
Thanks,
tglx
Hi Thomas,
Should I keep the development history or start from scratch
for this ACPI resource patch set?
Thanks!
Gerry
On 2014/12/12 19:43, Thomas Gleixner wrote:
> On Fri, 12 Dec 2014, Jiang Liu wrote:
>> On 2014/12/12 15:53, Thomas Gleixner wrote:
>>> On Thu, 11 Dec 2014, Yinghai Lu wrote:
>>>> On Thu, Dec 11, 2014 at 11:48 AM, Thomas Gleixner <[email protected]> wrote:
>>>>> +static inline bool is_valid_mem_resource(struct resource *res)
>>>>> +{
>>>>> + return !(res->flags & IORESOURCE_DISABLED) &&
>>>>> + (res->flags & IORESOURCE_MEM);
>>>>> +}
>>>>> +
>>>> There is minor problem about mem pref handling, original code will ignore them.
>>>
>>> Bah. I missed that in that well documented function...
>>>
>>>> with this patch will let it follow through.
>>>>
>>>> should change is_valid_mem_resource to is_valid_mem_nonpref_resource()...
>>>>
>>>> +static inline bool is_valid_mem_nonpref_resource(struct resource *res)
>>>> {
>>>> return !(res->flags & IORESOURCE_DISABLED) &&
>>>> - (res->flags & IORESOURCE_MEM);
>>>> + (res->flags & IORESOURCE_MEM) &&
>>>> + !(res->flags & IORESOURCE_PREFETCH);
>>>> }
>>>
>>> Unfortunately that does not help, because nothing sets the
>>> IORESOURCE_PREFETCH flag. Will fix it proper.
>>>
>>> I still have no explanation why the translation offset needs to be
>>> applied here.
>> Hi Thomas,
>> I have read related section in ACPI spec, seems the addition
>> of translation_offset is redundant here.
>>
>> Quotation from ACPI spec 5a, 6.4.3.5.1
>> For bridges that translate addresses across the bridge, this is the
>> offset that must be added to the address on the secondary side to
>> obtain the address on the primary side. Non-bridge devices must list
>> 0 for all Address Translation offset bits.
>>
>> Quotation from ACPI spec 5, 9.17 I/O APIC Device:
>> It must also contain a _CRS object that reports the base address of the
>> I/O APIC device. The _CRS object is required to contain only one
>> resource, a memory resource pointing to the I/O APIC register base.
>>
>> IO APIC is not a bridge, so translation_offset should always be zero.
>
> Right and we really need a proper interface for this on the acpi side,
> so we can avoid all that dance in the usage sites.
>
> Thanks,
>
> tglx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Wed, 17 Dec 2014, Jiang Liu wrote:
> Hi Thomas,
> Should I keep the development history or start from scratch
> for this ACPI resource patch set?
We better start from scratch.