| Message ID | 20230124123450.321852-1-pierre.gondois@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next] cacheinfo: Correctly handle new acpi_get_cache_info() prototype | expand |
Hey! On Tue, Jan 24, 2023 at 01:34:46PM +0100, Pierre Gondois wrote: > commit bd500361a937 ("ACPI: PPTT: Update acpi_find_last_cache_level() > to acpi_get_cache_info()") > updates the function acpi_get_cache_info(). > > If CONFIG_ACPI_PPTT is not defined, acpi_get_cache_info() doesn't > update its *levels and *split_levels parameters and returns 0. > This can lead to a faulty behaviour. > > Make acpi_get_cache_info() return an error code if CONFIG_ACPI_PPTT > is not defined. Initialize levels and split_levels before passing > their address to acpi_get_cache_info(). > > Also, in init_cache_level(): Hmm... > - commit e75d18cecbb3 ("arm64: cacheinfo: Fix incorrect > assignment of signed error value to unsigned fw_level") > checks the fw_level value in init_cache_level() in case > the value is negative. Remove this check as the error code > is not returned through fw_level anymore. > - if no PPTT is present or CONFIG_ACPI_PPTT is not defined, > it is still possible to use the cache information from clidr_el1. > Instead of aborting if acpi_get_cache_info() returns an error > code, just continue. To be honest, these feel like entirely separate things that should be in different patches. You've got: - Dan's smatch fixes - a redundant check being removed - a behaviour change for if acpi_get_cache_info() returns an error > Reported-by: Dan Carpenter <error27@gmail.com> > Reported-by: kernel test robot <lkp@intel.com> How about Link: to the LKP/Dan's report? Link: https://lore.kernel.org/all/Y86iruJPuwNN7rZw@kili/ I did a quick check but didn't don't see the LKP report... Also a Fixes: tag too, no? Thanks, Conor.
On Tue, Jan 24, 2023 at 01:31:06PM +0000, Conor Dooley wrote: > Hey! > > On Tue, Jan 24, 2023 at 01:34:46PM +0100, Pierre Gondois wrote: > > commit bd500361a937 ("ACPI: PPTT: Update acpi_find_last_cache_level() > > to acpi_get_cache_info()") > > updates the function acpi_get_cache_info(). > > > > If CONFIG_ACPI_PPTT is not defined, acpi_get_cache_info() doesn't > > update its *levels and *split_levels parameters and returns 0. > > This can lead to a faulty behaviour. > > > > Make acpi_get_cache_info() return an error code if CONFIG_ACPI_PPTT > > is not defined. Initialize levels and split_levels before passing > > their address to acpi_get_cache_info(). > > > > Also, in init_cache_level(): > > Hmm... > > > - commit e75d18cecbb3 ("arm64: cacheinfo: Fix incorrect > > assignment of signed error value to unsigned fw_level") > > checks the fw_level value in init_cache_level() in case > > the value is negative. Remove this check as the error code > > is not returned through fw_level anymore. > > - if no PPTT is present or CONFIG_ACPI_PPTT is not defined, > > it is still possible to use the cache information from clidr_el1. > > Instead of aborting if acpi_get_cache_info() returns an error > > code, just continue. > > To be honest, these feel like entirely separate things that should be > in different patches. You've got: > - Dan's smatch fixes > - a redundant check being removed > - a behaviour change for if acpi_get_cache_info() returns an error > I am not too fussy about it, but for sure it would be cleaner for sure. > > Reported-by: Dan Carpenter <error27@gmail.com> > > Reported-by: kernel test robot <lkp@intel.com> > > How about Link: to the LKP/Dan's report? > Link: https://lore.kernel.org/all/Y86iruJPuwNN7rZw@kili/ > > I did a quick check but didn't don't see the LKP report... > Yes, LKP dropped all the cc when reported, even I saw after merging the changes. I think this is the one: https://lore.kernel.org/all/202301052307.JYt1GWaJ-lkp@intel.com/ > Also a Fixes: tag too, no? > +1, if you split make sure you tag fixes to the right one(mainly one that changes return from acpi_get_cache_info()) > Thanks, > Conor.
diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c index 36c3b07cdf2d..3ba70985e3a2 100644 --- a/arch/arm64/kernel/cacheinfo.c +++ b/arch/arm64/kernel/cacheinfo.c @@ -64,12 +64,9 @@ int init_cache_level(unsigned int cpu) } else { ret = acpi_get_cache_info(cpu, &fw_level, NULL); if (ret < 0) - return ret; + fw_level = 0; } - if (fw_level < 0) - return fw_level; - if (level < fw_level) { /* * some external caches not specified in CLIDR_EL1 diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c index b57fbd0d7114..f184ef7dc1d2 100644 --- a/drivers/base/cacheinfo.c +++ b/drivers/base/cacheinfo.c @@ -414,7 +414,7 @@ int allocate_cache_info(int cpu) int fetch_cache_info(unsigned int cpu) { struct cpu_cacheinfo *this_cpu_ci; - unsigned int levels, split_levels; + unsigned int levels = 0, split_levels = 0; int ret; if (acpi_disabled) { diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h index dfef57077cd0..908e19d17f49 100644 --- a/include/linux/cacheinfo.h +++ b/include/linux/cacheinfo.h @@ -100,7 +100,7 @@ static inline int acpi_get_cache_info(unsigned int cpu, unsigned int *levels, unsigned int *split_levels) { - return 0; + return -ENOENT; } #else int acpi_get_cache_info(unsigned int cpu,
commit bd500361a937 ("ACPI: PPTT: Update acpi_find_last_cache_level() to acpi_get_cache_info()") updates the function acpi_get_cache_info(). If CONFIG_ACPI_PPTT is not defined, acpi_get_cache_info() doesn't update its *levels and *split_levels parameters and returns 0. This can lead to a faulty behaviour. Make acpi_get_cache_info() return an error code if CONFIG_ACPI_PPTT is not defined. Initialize levels and split_levels before passing their address to acpi_get_cache_info(). Also, in init_cache_level(): - commit e75d18cecbb3 ("arm64: cacheinfo: Fix incorrect assignment of signed error value to unsigned fw_level") checks the fw_level value in init_cache_level() in case the value is negative. Remove this check as the error code is not returned through fw_level anymore. - if no PPTT is present or CONFIG_ACPI_PPTT is not defined, it is still possible to use the cache information from clidr_el1. Instead of aborting if acpi_get_cache_info() returns an error code, just continue. Reported-by: Dan Carpenter <error27@gmail.com> Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> --- arch/arm64/kernel/cacheinfo.c | 5 +---- drivers/base/cacheinfo.c | 2 +- include/linux/cacheinfo.h | 2 +- 3 files changed, 3 insertions(+), 6 deletions(-)