<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> [AMD Official Use Only - General] <div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <div id="appendonsend"></div> <div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)"> </div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="divRplyFwdMsg" dir="ltr">From: Lazar, Lijo <Lijo.Lazar@amd.com> Sent: Monday, May 9, 2022 12:50 AM To: Powell, Darren <Darren.Powell@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Cc: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; Feng, Kenneth <Kenneth.Feng@amd.com>; Quan, Evan <Evan.Quan@amd.com>; Ma, Le <Le.Ma@amd.com> Subject: Re: [PATCH v1 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2 <div> </div> </div> <div class="BodyFragment"> <div class="PlainText elementToProof"> 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 <darren.powell@amd.com> > --- > .../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 <div>while accessing the freq_values array. My mistake was that I assumed that meant you intended to access clocks.data array.</div> <div>If aldebaran only uses the curr.min and curr.max values that simplifies this greatly, </div> <div>I'll respin this to initialize display_levels to 2 , and then output from freq_values array.</div> > > - /* > - * 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; > </div> </div> </div> </body> </html>