[AMD Official Use Only - General] ________________________________ From: Lazar, Lijo Sent: Monday, May 9, 2022 12:50 AM To: Powell, Darren ; amd-gfx@lists.freedesktop.org Cc: Wang, Yang(Kevin) ; Feng, Kenneth ; Quan, Evan ; Ma, Le Subject: Re: [PATCH v1 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2 On 5/9/2022 9:28 AM, Darren Powell wrote: > added a check to populate and use SCLK shim table freq_values only > if using dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL or > AMD_DPM_FORCED_LEVEL_PERF_DETERMINISM > removed clocks.num_levels from calculation of shim table size > removed unsafe accesses to shim table freq_values > output gfx_table values if using other dpm levels > added check for freq_match when using freq_values for when now == min_clk > > == Test == > LOGFILE=aldebaran-sclk.test.log > AMDGPU_PCI_ADDR=`lspci -nn | grep "VGA\|Display" | cut -d " " -f 1` > AMDGPU_HWMON=`ls -la /sys/class/hwmon | grep $AMDGPU_PCI_ADDR | awk '{print $9}'` > HWMON_DIR=/sys/class/hwmon/${AMDGPU_HWMON} > > lspci -nn | grep "VGA\|Display" > $LOGFILE > FILES="pp_od_clk_voltage > pp_dpm_sclk" > > for f in $FILES > do > echo === $f === >> $LOGFILE > cat $HWMON_DIR/device/$f >> $LOGFILE > done > cat $LOGFILE > > Signed-off-by: Darren Powell > --- > .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 60 +++++++++---------- > 1 file changed, 29 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c > index 6a4fca47ae53..a653668e8402 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c > @@ -740,9 +740,8 @@ static int aldebaran_print_clk_levels(struct smu_context *smu, > struct smu_13_0_dpm_table *single_dpm_table; > struct smu_dpm_context *smu_dpm = &smu->smu_dpm; > struct smu_13_0_dpm_context *dpm_context = NULL; > - uint32_t display_levels; > uint32_t freq_values[3] = {0}; > - uint32_t min_clk, max_clk; > + uint32_t min_clk, max_clk, display_levels = 0; > > smu_cmn_get_sysfs_buf(&buf, &size); > > @@ -765,46 +764,45 @@ static int aldebaran_print_clk_levels(struct smu_context *smu, > return ret; > } > > - single_dpm_table = &(dpm_context->dpm_tables.gfx_table); > - ret = aldebaran_get_clk_table(smu, &clocks, single_dpm_table); > - if (ret) { > - dev_err(smu->adev->dev, "Attempt to get gfx clk levels Failed!"); > - return ret; > - } > - > - display_levels = clocks.num_levels; > - > - min_clk = pstate_table->gfxclk_pstate.curr.min; > - max_clk = pstate_table->gfxclk_pstate.curr.max; > - > - freq_values[0] = min_clk; > - freq_values[1] = max_clk; > - > - /* fine-grained dpm has only 2 levels */ > - if (now > min_clk && now < max_clk) { > - display_levels = clocks.num_levels + 1; > - freq_values[2] = max_clk; > - freq_values[1] = now; > - } > + if ((smu_dpm->dpm_level != AMD_DPM_FORCED_LEVEL_MANUAL && > + smu_dpm->dpm_level != AMD_DPM_FORCED_LEVEL_PERF_DETERMINISM)) { > + single_dpm_table = &(dpm_context->dpm_tables.gfx_table); > + ret = aldebaran_get_clk_table(smu, &clocks, single_dpm_table); > + if (ret) { > + dev_err(smu->adev->dev, "Attempt to get gfx clk levels Failed!"); > + return ret; > + } There are only two levels for GFX clock in aldebaran - min and max. Regardless of the mode, gfxclk_pstate.curr.min/max should reflect the current min/max level. Could you explain the issue you are seeing? It's not so clear from the commit message. Thanks, Lijo [DP] I was concerned by the initialization of display_levels from clocks.num_levels and how it is used as the loop iterator while accessing the freq_values array. My mistake was that I assumed that meant you intended to access clocks.data array. If aldebaran only uses the curr.min and curr.max values that simplifies this greatly, I'll respin this to initialize display_levels to 2 , and then output from freq_values array. > > - /* > - * For DPM disabled case, there will be only one clock level. > - * And it's safe to assume that is always the current clock. > - */ > - if (display_levels == clocks.num_levels) { > for (i = 0; i < clocks.num_levels; i++) > size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i, > - freq_values[i], > + clocks.data[i].clocks_in_khz / 1000, > (clocks.num_levels == 1) ? > "*" : > (aldebaran_freqs_in_same_level( > - freq_values[i], now) ? > + clocks.data[i].clocks_in_khz / 1000, now) ? > "*" : > "")); > } else { > + /* fine-grained dpm has only 2 levels */ > + display_levels = 2; > + > + min_clk = pstate_table->gfxclk_pstate.curr.min; > + max_clk = pstate_table->gfxclk_pstate.curr.max; > + > + freq_values[0] = min_clk; > + freq_values[1] = max_clk; > + > + if (now > min_clk && now < max_clk) { > + display_levels++; > + freq_values[2] = max_clk; > + freq_values[1] = now; > + } > + > for (i = 0; i < display_levels; i++) > size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i, > - freq_values[i], i == 1 ? "*" : ""); > + freq_values[i], > + aldebaran_freqs_in_same_level(freq_values[i], now) ? > + "*" : ""); > } > > break; >