| Message ID | 20220401172840.1252-1-james.liu@hpe.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ACPI: OSL: Fix the memory mapping of an ACPI GAS that addresses a data structure | expand |
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on v5.17 next-20220401]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/james-liu-hpe-com/ACPI-OSL-Fix-the-memory-mapping-of-an-ACPI-GAS-that-addresses-a-data-structure/20220402-013755
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220402/202204021406.lxRmdIm5-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c4a1b07d0979e7ff20d7d541af666d822d66b566)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/594496ff7d62c6d42b3c8a436fca46cc289aea67
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review james-liu-hpe-com/ACPI-OSL-Fix-the-memory-mapping-of-an-ACPI-GAS-that-addresses-a-data-structure/20220402-013755
git checkout 594496ff7d62c6d42b3c8a436fca46cc289aea67
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/acpi/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/acpi/osl.c:463:29: warning: variable 'addr' is uninitialized when used here [-Wuninitialized]
return acpi_os_map_iomem(addr, PAGE_SIZE);
^~~~
drivers/acpi/osl.c:455:10: note: initialize the variable 'addr' to silence this warning
u64 addr;
^
= 0
drivers/acpi/osl.c:487:25: warning: variable 'addr' is uninitialized when used here [-Wuninitialized]
map = acpi_map_lookup(addr, PAGE_SIZE);
^~~~
drivers/acpi/osl.c:477:10: note: initialize the variable 'addr' to silence this warning
u64 addr;
^
= 0
2 warnings generated.
vim +/addr +463 drivers/acpi/osl.c
452
453 void __iomem *acpi_os_map_generic_address(struct acpi_generic_address *gas)
454 {
455 u64 addr;
456
457 if (gas->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
458 return NULL;
459
460 /* Handle a case that GAS is used to address an ACPI data structure */
461 if (!gas->bit_width) {
462 pr_info("An ACPI data structure at 0x%llx is mapped\n", addr);
> 463 return acpi_os_map_iomem(addr, PAGE_SIZE);
464 }
465
466 /* Handle possible alignment issues */
467 memcpy(&addr, &gas->address, sizeof(addr));
468 if (!addr)
469 return NULL;
470
471 return acpi_os_map_iomem(addr, gas->bit_width / 8);
472 }
473 EXPORT_SYMBOL(acpi_os_map_generic_address);
474
Hi Rafael and all, Could you take a look at this patch? The mentioned bug blocks EINJ testing when a firmware can correctly support ACPI 6.4 spec. On Fri, Apr 01, 2022 at 05:28:40PM +0000, james.liu@hpe.com wrote: > From: James Liu <james.liu@hpe.com> > > Modify acpi_os_map_generic_address and acpi_os_unmap_generic_address > to handle a case that a GAS table (i.e., Table 5.1 in ACPI 6.4) is used > to address a data structure; in this case, the GAS has the field of > "Register Bit Width" equal to 0. > > For example, "Injection Instruction Entry" (Table 18.25 in ACPI 6.4) > has a RegisterRegion field that is a GAS that points to a data > structure SET_ERROR_TYPE_WITH_ADDRESS (Table 18.30), which is required > when using EINJ (Error Injection module). > > This fix preserves a fairly sufficient memory space (i.e., page size) > to store the data structure so as to prevent EINJ module from loading > failure if platform firmware can support Injection Instruction Entry in > an EINJ table. > > Signed-off-by: James Liu <james.liu@hpe.com> > --- > drivers/acpi/osl.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c > index 45c5c0e45..ab2f584b1 100644 > --- a/drivers/acpi/osl.c > +++ b/drivers/acpi/osl.c > @@ -457,9 +457,15 @@ void __iomem *acpi_os_map_generic_address(struct acpi_generic_address *gas) > if (gas->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) > return NULL; > > + /* Handle a case that GAS is used to address an ACPI data structure */ > + if (!gas->bit_width) { > + pr_info("An ACPI data structure at 0x%llx is mapped\n", addr); > + return acpi_os_map_iomem(addr, PAGE_SIZE); > + } > + > /* Handle possible alignment issues */ > memcpy(&addr, &gas->address, sizeof(addr)); > - if (!addr || !gas->bit_width) > + if (!addr) > return NULL; > > return acpi_os_map_iomem(addr, gas->bit_width / 8); > @@ -474,9 +480,22 @@ void acpi_os_unmap_generic_address(struct acpi_generic_address *gas) > if (gas->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) > return; > > + /* Handle a case that GAS is used to address an ACPI data structure */ > + if (!gas->bit_width) { > + pr_info("An ACPI data structure at 0x%llx is unmapped\n", addr); > + mutex_lock(&acpi_ioremap_lock); > + map = acpi_map_lookup(addr, PAGE_SIZE); > + if (!map) { > + mutex_unlock(&acpi_ioremap_lock); > + return; > + } > + acpi_os_drop_map_ref(map); > + mutex_unlock(&acpi_ioremap_lock); > + } > + > /* Handle possible alignment issues */ > memcpy(&addr, &gas->address, sizeof(addr)); > - if (!addr || !gas->bit_width) > + if (!addr) > return; > > mutex_lock(&acpi_ioremap_lock); > -- > 2.25.1 >
On Tue, May 17, 2022 at 7:56 PM James Liu <james.liu@hpe.com> wrote: > > Hi Rafael and all, > Could you take a look into this patches? The mentioned bug blocks EINJ > testing whenever a system firmware can correctly support GAS according > to ACPI 6.4. The kernel test robot reported an issue with it. Have you seen that report? > On Fri, Apr 01, 2022 at 05:28:40PM +0000, james.liu@hpe.com wrote: > > From: James Liu <james.liu@hpe.com> > > > > Modify acpi_os_map_generic_address and acpi_os_unmap_generic_address > > to handle a case that a GAS table (i.e., Table 5.1 in ACPI 6.4) is used > > to address a data structure; in this case, the GAS has the field of > > "Register Bit Width" equal to 0. > > > > For example, "Injection Instruction Entry" (Table 18.25 in ACPI 6.4) > > has a RegisterRegion field that is a GAS that points to a data > > structure SET_ERROR_TYPE_WITH_ADDRESS (Table 18.30), which is required > > when using EINJ (Error Injection module). > > > > This fix preserves a fairly sufficient memory space (i.e., page size) > > to store the data structure so as to prevent EINJ module from loading > > failure if platform firmware can support Injection Instruction Entry in > > an EINJ table. > > > > Signed-off-by: James Liu <james.liu@hpe.com> > > --- > > drivers/acpi/osl.c | 23 +++++++++++++++++++++-- > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c > > index 45c5c0e45..ab2f584b1 100644 > > --- a/drivers/acpi/osl.c > > +++ b/drivers/acpi/osl.c > > @@ -457,9 +457,15 @@ void __iomem *acpi_os_map_generic_address(struct acpi_generic_address *gas) > > if (gas->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) > > return NULL; > > > > + /* Handle a case that GAS is used to address an ACPI data structure */ > > + if (!gas->bit_width) { > > + pr_info("An ACPI data structure at 0x%llx is mapped\n", addr); > > + return acpi_os_map_iomem(addr, PAGE_SIZE); > > + } > > + > > /* Handle possible alignment issues */ > > memcpy(&addr, &gas->address, sizeof(addr)); > > - if (!addr || !gas->bit_width) > > + if (!addr) > > return NULL; > > > > return acpi_os_map_iomem(addr, gas->bit_width / 8); > > @@ -474,9 +480,22 @@ void acpi_os_unmap_generic_address(struct acpi_generic_address *gas) > > if (gas->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) > > return; > > > > + /* Handle a case that GAS is used to address an ACPI data structure */ > > + if (!gas->bit_width) { > > + pr_info("An ACPI data structure at 0x%llx is unmapped\n", addr); > > + mutex_lock(&acpi_ioremap_lock); > > + map = acpi_map_lookup(addr, PAGE_SIZE); > > + if (!map) { > > + mutex_unlock(&acpi_ioremap_lock); > > + return; > > + } > > + acpi_os_drop_map_ref(map); > > + mutex_unlock(&acpi_ioremap_lock); > > + } > > + > > /* Handle possible alignment issues */ > > memcpy(&addr, &gas->address, sizeof(addr)); > > - if (!addr || !gas->bit_width) > > + if (!addr) > > return; > > > > mutex_lock(&acpi_ioremap_lock); > > -- > > 2.25.1 > >
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index 45c5c0e45..ab2f584b1 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -457,9 +457,15 @@ void __iomem *acpi_os_map_generic_address(struct acpi_generic_address *gas) if (gas->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) return NULL; + /* Handle a case that GAS is used to address an ACPI data structure */ + if (!gas->bit_width) { + pr_info("An ACPI data structure at 0x%llx is mapped\n", addr); + return acpi_os_map_iomem(addr, PAGE_SIZE); + } + /* Handle possible alignment issues */ memcpy(&addr, &gas->address, sizeof(addr)); - if (!addr || !gas->bit_width) + if (!addr) return NULL; return acpi_os_map_iomem(addr, gas->bit_width / 8); @@ -474,9 +480,22 @@ void acpi_os_unmap_generic_address(struct acpi_generic_address *gas) if (gas->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) return; + /* Handle a case that GAS is used to address an ACPI data structure */ + if (!gas->bit_width) { + pr_info("An ACPI data structure at 0x%llx is unmapped\n", addr); + mutex_lock(&acpi_ioremap_lock); + map = acpi_map_lookup(addr, PAGE_SIZE); + if (!map) { + mutex_unlock(&acpi_ioremap_lock); + return; + } + acpi_os_drop_map_ref(map); + mutex_unlock(&acpi_ioremap_lock); + } + /* Handle possible alignment issues */ memcpy(&addr, &gas->address, sizeof(addr)); - if (!addr || !gas->bit_width) + if (!addr) return; mutex_lock(&acpi_ioremap_lock);