From: Aili Yao <yaoaili@xxxxxxxxxxxx> Hi, Thanks for your comments! > -----Original Message----- > From: James Morse [mailto:james.morse@xxxxxxx] > Sent: Thursday, October 29, 2020 3:19 AM > To: yaoaili126@xxxxxxx > Cc: rjw@xxxxxxxxxxxxx; lenb@xxxxxxxxxx; tony.luck@xxxxxxxxx; bp@xxxxxxxxx; > linux-acpi@xxxxxxxxxxxxxxx; YANGFENG1 [杨峰] > <YANGFENG1@xxxxxxxxxxxx>; yaoaili [么爱利] <yaoaili@xxxxxxxxxxxx> > Subject: Re: [PATCH] Fix incorrect return value of pre_map_gar_callback > > Hi! > > On 26/10/2020 06:15, yaoaili126@xxxxxxx wrote: > > From: Aili Yao <yaoaili@xxxxxxxxxxxx> > > > > From commit 6915564dc5a8 ("ACPI: OSL: Change the type of > > acpi_os_map_generic_address() return > > value"),acpi_os_map_generic_address > > will return logical address or NULL for error, but > > pre_map_gar_callback,for ACPI_ADR_SPACE_SYSTEM_IO case, it should > > 'it should' refers to pre_map_gar_callback(), not > acpi_os_map_generic_address()? > yes > > > be also return 0,as it's a > > normal case, but now it will return -ENXIO. so check it out for such > > case to avoid einj module initialization fail. > > apei_map_generic_address() calls acpi_os_map_generic_address() which > returns NULL for any address space that isn't > ACPI_ADR_SPACE_SYSTEM_MEMORY. > That commit now maps this to an error code, where-as before: this code was > getting away with it. > > The bug is it tries to map a GAR that doesn't need mapping. > > > Could we avoid this problem more clearly by returning 0 from > apei_map_generic_address() for address spaces that don't need mapping? > (e.g. IO) > > This would also fix any other callers lurking in apei. > yes, you are right, I think we can move the ACPI_ADR_SPACE_SYSTEM_MEMORY check to: 626 int apei_map_generic_address(struct acpi_generic_address *reg) 627 { 628 int rc; 629 u32 access_bit_width; 630 u64 address; 631 632 rc = apei_check_gar(reg, &address, &access_bit_width); 633 if (rc) 634 return rc; 635 636 if (!acpi_os_map_generic_address(reg) && 637 reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) 638 return -ENXIO; 639 640 return 0; 641 } 642 EXPORT_SYMBOL_GPL(apei_map_generic_address); > > Thanks, > > James > > > > diff --git a/drivers/acpi/apei/apei-base.c > > b/drivers/acpi/apei/apei-base.c index 552fd9ffaca4..042d2dbdb855 > > 100644 > > --- a/drivers/acpi/apei/apei-base.c > > +++ b/drivers/acpi/apei/apei-base.c > > @@ -230,7 +230,8 @@ static int pre_map_gar_callback(struct > > apei_exec_context *ctx, { > > u8 ins = entry->instruction; > > > > - if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER) > > + if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER && > > + entry->register_region.space_id == > ACPI_ADR_SPACE_SYSTEM_MEMORY) > > return apei_map_generic_address(&entry->register_region); > > > > return 0; > Thanks Aili Yao
