| Message ID | 20221215163649.386750-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/pm: avoid large variable on kernel stack | expand |
Applied. Thanks! Alex On Thu, Dec 15, 2022 at 11:37 AM Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > The activity_monitor_external[] array is too big to fit on the > kernel stack, resulting in this warning with clang: > > drivers/gpu/drm/amd/amdgpu/../pm/swsmu/smu13/smu_v13_0_7_ppt.c:1438:12: error: stack frame size (1040) exceeds limit (1024) in 'smu_v13_0_7_get_power_profile_mode' [-Werror,-Wframe-larger-than] > > Use dynamic allocation instead. It should also be possible to > have single element here instead of the array, but this seems > easier. > > Fixes: 334682ae8151 ("drm/amd/pm: enable workload type change on smu_v13_0_7") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > .../drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c | 21 ++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c > index c270f94a1b86..7eba854e09ec 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c > @@ -1439,7 +1439,7 @@ static int smu_v13_0_7_get_power_limit(struct smu_context *smu, > > static int smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf) > { > - DpmActivityMonitorCoeffIntExternal_t activity_monitor_external[PP_SMC_POWER_PROFILE_COUNT]; > + DpmActivityMonitorCoeffIntExternal_t *activity_monitor_external; > uint32_t i, j, size = 0; > int16_t workload_type = 0; > int result = 0; > @@ -1447,6 +1447,12 @@ static int smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf > if (!buf) > return -EINVAL; > > + activity_monitor_external = kcalloc(sizeof(activity_monitor_external), > + PP_SMC_POWER_PROFILE_COUNT, > + GFP_KERNEL); > + if (!activity_monitor_external) > + return -ENOMEM; > + > size += sysfs_emit_at(buf, size, " "); > for (i = 0; i <= PP_SMC_POWER_PROFILE_WINDOW3D; i++) > size += sysfs_emit_at(buf, size, "%-14s%s", amdgpu_pp_profile_name[i], > @@ -1459,15 +1465,17 @@ static int smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf > workload_type = smu_cmn_to_asic_specific_index(smu, > CMN2ASIC_MAPPING_WORKLOAD, > i); > - if (workload_type < 0) > - return -EINVAL; > + if (workload_type < 0) { > + result = -EINVAL; > + goto out; > + } > > result = smu_cmn_update_table(smu, > SMU_TABLE_ACTIVITY_MONITOR_COEFF, workload_type, > (void *)(&activity_monitor_external[i]), false); > if (result) { > dev_err(smu->adev->dev, "[%s] Failed to get activity monitor!", __func__); > - return result; > + goto out; > } > } > > @@ -1495,7 +1503,10 @@ do { \ > PRINT_DPM_MONITOR(Fclk_BoosterFreq); > #undef PRINT_DPM_MONITOR > > - return size; > + result = size; > +out: > + kfree(activity_monitor_external); > + return result; > } > > static int smu_v13_0_7_set_power_profile_mode(struct smu_context *smu, long *input, uint32_t size) > -- > 2.35.1 >
Le 15/12/2022 à 17:36, Arnd Bergmann a écrit : > From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> > > The activity_monitor_external[] array is too big to fit on the > kernel stack, resulting in this warning with clang: > > drivers/gpu/drm/amd/amdgpu/../pm/swsmu/smu13/smu_v13_0_7_ppt.c:1438:12: error: stack frame size (1040) exceeds limit (1024) in 'smu_v13_0_7_get_power_profile_mode' [-Werror,-Wframe-larger-than] > > Use dynamic allocation instead. It should also be possible to > have single element here instead of the array, but this seems > easier. > > Fixes: 334682ae8151 ("drm/amd/pm: enable workload type change on smu_v13_0_7") > Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> > --- > .../drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c | 21 ++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c > index c270f94a1b86..7eba854e09ec 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c > @@ -1439,7 +1439,7 @@ static int smu_v13_0_7_get_power_limit(struct smu_context *smu, > > static int smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf) > { > - DpmActivityMonitorCoeffIntExternal_t activity_monitor_external[PP_SMC_POWER_PROFILE_COUNT]; > + DpmActivityMonitorCoeffIntExternal_t *activity_monitor_external; > uint32_t i, j, size = 0; > int16_t workload_type = 0; > int result = 0; > @@ -1447,6 +1447,12 @@ static int smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf > if (!buf) > return -EINVAL; > > + activity_monitor_external = kcalloc(sizeof(activity_monitor_external), Hi, Before, 'activity_monitor_external' was not initialized. Maybe kcalloc() is enough? sizeof(*activity_monitor_external)? ~~~~ > + PP_SMC_POWER_PROFILE_COUNT, > + GFP_KERNEL); > + if (!activity_monitor_external) > + return -ENOMEM; > + > size += sysfs_emit_at(buf, size, " "); > for (i = 0; i <= PP_SMC_POWER_PROFILE_WINDOW3D; i++) Unrelated, but wouldn't it be more straightforward with "< PP_SMC_POWER_PROFILE_COUNT"? > size += sysfs_emit_at(buf, size, "%-14s%s", amdgpu_pp_profile_name[i], > @@ -1459,15 +1465,17 @@ static int smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf > workload_type = smu_cmn_to_asic_specific_index(smu, > CMN2ASIC_MAPPING_WORKLOAD, > i); > - if (workload_type < 0) > - return -EINVAL; > + if (workload_type < 0) { > + result = -EINVAL; > + goto out; > + } > > result = smu_cmn_update_table(smu, > SMU_TABLE_ACTIVITY_MONITOR_COEFF, workload_type, > (void *)(&activity_monitor_external[i]), false); > if (result) { > dev_err(smu->adev->dev, "[%s] Failed to get activity monitor!", __func__); > - return result; > + goto out; > } > } > > @@ -1495,7 +1503,10 @@ do { \ > PRINT_DPM_MONITOR(Fclk_BoosterFreq); > #undef PRINT_DPM_MONITOR > > - return size; > + result = size; > +out: > + kfree(activity_monitor_external); > + return result; > } > > static int smu_v13_0_7_set_power_profile_mode(struct smu_context *smu, long *input, uint32_t size)
Le 15/12/2022 à 20:46, Christophe JAILLET a écrit : > Le 15/12/2022 à 17:36, Arnd Bergmann a écrit : >> From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> >> >> The activity_monitor_external[] array is too big to fit on the >> kernel stack, resulting in this warning with clang: >> >> drivers/gpu/drm/amd/amdgpu/../pm/swsmu/smu13/smu_v13_0_7_ppt.c:1438:12: error: stack frame size (1040) exceeds limit (1024) in 'smu_v13_0_7_get_power_profile_mode' [-Werror,-Wframe-larger-than] >> >> Use dynamic allocation instead. It should also be possible to >> have single element here instead of the array, but this seems >> easier. >> >> Fixes: 334682ae8151 ("drm/amd/pm: enable workload type change on >> smu_v13_0_7") >> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> >> --- >> .../drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c | 21 ++++++++++++++----- >> 1 file changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c >> b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c >> index c270f94a1b86..7eba854e09ec 100644 >> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c >> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c >> @@ -1439,7 +1439,7 @@ static int smu_v13_0_7_get_power_limit(struct >> smu_context *smu, >> static int smu_v13_0_7_get_power_profile_mode(struct smu_context >> *smu, char *buf) >> { >> - DpmActivityMonitorCoeffIntExternal_t >> activity_monitor_external[PP_SMC_POWER_PROFILE_COUNT]; >> + DpmActivityMonitorCoeffIntExternal_t *activity_monitor_external; >> uint32_t i, j, size = 0; >> int16_t workload_type = 0; >> int result = 0; >> @@ -1447,6 +1447,12 @@ static int >> smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf >> if (!buf) >> return -EINVAL; >> + activity_monitor_external = >> kcalloc(sizeof(activity_monitor_external), > > Hi, > > Before, 'activity_monitor_external' was not initialized. > Maybe kcalloc() is enough? Read kmalloc_array() > > sizeof(*activity_monitor_external)? > ~~~~ > >> + PP_SMC_POWER_PROFILE_COUNT, >> + GFP_KERNEL); >> + if (!activity_monitor_external) >> + return -ENOMEM; >> + >> size += sysfs_emit_at(buf, size, " "); >> for (i = 0; i <= PP_SMC_POWER_PROFILE_WINDOW3D; i++) > > Unrelated, but wouldn't it be more straightforward with "< > PP_SMC_POWER_PROFILE_COUNT"? > >> size += sysfs_emit_at(buf, size, "%-14s%s", >> amdgpu_pp_profile_name[i], >> @@ -1459,15 +1465,17 @@ static int >> smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf >> workload_type = smu_cmn_to_asic_specific_index(smu, >> CMN2ASIC_MAPPING_WORKLOAD, >> i); >> - if (workload_type < 0) >> - return -EINVAL; >> + if (workload_type < 0) { >> + result = -EINVAL; >> + goto out; >> + } >> result = smu_cmn_update_table(smu, >> SMU_TABLE_ACTIVITY_MONITOR_COEFF, workload_type, >> (void *)(&activity_monitor_external[i]), false); >> if (result) { >> dev_err(smu->adev->dev, "[%s] Failed to get activity >> monitor!", __func__); >> - return result; >> + goto out; >> } >> } >> @@ -1495,7 +1503,10 @@ do >> { \ >> PRINT_DPM_MONITOR(Fclk_BoosterFreq); >> #undef PRINT_DPM_MONITOR >> - return size; >> + result = size; >> +out: >> + kfree(activity_monitor_external); >> + return result; >> } >> static int smu_v13_0_7_set_power_profile_mode(struct smu_context >> *smu, long *input, uint32_t size) > >
On Thu, Dec 15, 2022 at 2:46 PM Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > Le 15/12/2022 à 17:36, Arnd Bergmann a écrit : > > From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> > > > > The activity_monitor_external[] array is too big to fit on the > > kernel stack, resulting in this warning with clang: > > > > drivers/gpu/drm/amd/amdgpu/../pm/swsmu/smu13/smu_v13_0_7_ppt.c:1438:12: error: stack frame size (1040) exceeds limit (1024) in 'smu_v13_0_7_get_power_profile_mode' [-Werror,-Wframe-larger-than] > > > > Use dynamic allocation instead. It should also be possible to > > have single element here instead of the array, but this seems > > easier. > > > > Fixes: 334682ae8151 ("drm/amd/pm: enable workload type change on smu_v13_0_7") > > Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> > > --- > > .../drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c | 21 ++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c > > index c270f94a1b86..7eba854e09ec 100644 > > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c > > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c > > @@ -1439,7 +1439,7 @@ static int smu_v13_0_7_get_power_limit(struct smu_context *smu, > > > > static int smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf) > > { > > - DpmActivityMonitorCoeffIntExternal_t activity_monitor_external[PP_SMC_POWER_PROFILE_COUNT]; > > + DpmActivityMonitorCoeffIntExternal_t *activity_monitor_external; > > uint32_t i, j, size = 0; > > int16_t workload_type = 0; > > int result = 0; > > @@ -1447,6 +1447,12 @@ static int smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf > > if (!buf) > > return -EINVAL; > > > > + activity_monitor_external = kcalloc(sizeof(activity_monitor_external), > > Hi, > > Before, 'activity_monitor_external' was not initialized. > Maybe kcalloc() is enough? > > sizeof(*activity_monitor_external)? > ~~~~ I've fixed this up when applying. Alex > > > + PP_SMC_POWER_PROFILE_COUNT, > > + GFP_KERNEL); > > + if (!activity_monitor_external) > > + return -ENOMEM; > > + > > size += sysfs_emit_at(buf, size, " "); > > for (i = 0; i <= PP_SMC_POWER_PROFILE_WINDOW3D; i++) > > Unrelated, but wouldn't it be more straightforward with "< > PP_SMC_POWER_PROFILE_COUNT"? > > > size += sysfs_emit_at(buf, size, "%-14s%s", amdgpu_pp_profile_name[i], > > @@ -1459,15 +1465,17 @@ static int smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf > > workload_type = smu_cmn_to_asic_specific_index(smu, > > CMN2ASIC_MAPPING_WORKLOAD, > > i); > > - if (workload_type < 0) > > - return -EINVAL; > > + if (workload_type < 0) { > > + result = -EINVAL; > > + goto out; > > + } > > > > result = smu_cmn_update_table(smu, > > SMU_TABLE_ACTIVITY_MONITOR_COEFF, workload_type, > > (void *)(&activity_monitor_external[i]), false); > > if (result) { > > dev_err(smu->adev->dev, "[%s] Failed to get activity monitor!", __func__); > > - return result; > > + goto out; > > } > > } > > > > @@ -1495,7 +1503,10 @@ do { \ > > PRINT_DPM_MONITOR(Fclk_BoosterFreq); > > #undef PRINT_DPM_MONITOR > > > > - return size; > > + result = size; > > +out: > > + kfree(activity_monitor_external); > > + return result; > > } > > > > static int smu_v13_0_7_set_power_profile_mode(struct smu_context *smu, long *input, uint32_t size) >
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c index c270f94a1b86..7eba854e09ec 100644 --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c @@ -1439,7 +1439,7 @@ static int smu_v13_0_7_get_power_limit(struct smu_context *smu, static int smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf) { - DpmActivityMonitorCoeffIntExternal_t activity_monitor_external[PP_SMC_POWER_PROFILE_COUNT]; + DpmActivityMonitorCoeffIntExternal_t *activity_monitor_external; uint32_t i, j, size = 0; int16_t workload_type = 0; int result = 0; @@ -1447,6 +1447,12 @@ static int smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf if (!buf) return -EINVAL; + activity_monitor_external = kcalloc(sizeof(activity_monitor_external), + PP_SMC_POWER_PROFILE_COUNT, + GFP_KERNEL); + if (!activity_monitor_external) + return -ENOMEM; + size += sysfs_emit_at(buf, size, " "); for (i = 0; i <= PP_SMC_POWER_PROFILE_WINDOW3D; i++) size += sysfs_emit_at(buf, size, "%-14s%s", amdgpu_pp_profile_name[i], @@ -1459,15 +1465,17 @@ static int smu_v13_0_7_get_power_profile_mode(struct smu_context *smu, char *buf workload_type = smu_cmn_to_asic_specific_index(smu, CMN2ASIC_MAPPING_WORKLOAD, i); - if (workload_type < 0) - return -EINVAL; + if (workload_type < 0) { + result = -EINVAL; + goto out; + } result = smu_cmn_update_table(smu, SMU_TABLE_ACTIVITY_MONITOR_COEFF, workload_type, (void *)(&activity_monitor_external[i]), false); if (result) { dev_err(smu->adev->dev, "[%s] Failed to get activity monitor!", __func__); - return result; + goto out; } } @@ -1495,7 +1503,10 @@ do { \ PRINT_DPM_MONITOR(Fclk_BoosterFreq); #undef PRINT_DPM_MONITOR - return size; + result = size; +out: + kfree(activity_monitor_external); + return result; } static int smu_v13_0_7_set_power_profile_mode(struct smu_context *smu, long *input, uint32_t size)