AMD General >> Alternatively, we could add a single stop condition in >> amdgpu_get_pp_od_clk_voltage() instead of per-emit checks.
I prefer this way. Thanks. Best Regards, Kevin > -----Original Message----- > From: Kamal, Asad <[email protected]> > Sent: Friday, May 29, 2026 16:00 > To: Wang, Yang(Kevin) <[email protected]>; amd- > [email protected]; Koenig, Christian <[email protected]> > Cc: Lazar, Lijo <[email protected]>; Zhang, Hawking > <[email protected]>; Ma, Le <[email protected]>; Zhang, Morris > <[email protected]>; Deucher, Alexander <[email protected]> > Subject: RE: [PATCH 1/4] drm/amd/pm: Handle truncation in common clk/pcie > printers > > AMD General > > Thanks for the review. > > I agree that size += sysfs_emit_at() is the common pattern for simple sysfs > show > handlers, and for many SMU paths the output is small enough that PAGE_SIZE is > never exhausted. > > The checks were added specifically for helpers that append many sections into > one > PAGE_SIZE buffer, in particular pp_od_clk_voltage, which calls > amdgpu_dpm_emit_clock_levels() repeatedly in a single show(). On SMU13 OD > paths the combined OD/fan output can approach the sysfs page limit. Once the > buffer is full, further sysfs_emit_at(buf, size, ...) calls hit the at >= > PAGE_SIZE > WARN path in sysfs_emit_at() and return 0, continuing the loop only generates > warnings and does not improve userspace output. > > I'm happy to narrow the series, keep the return check only in multi-emit/loop > paths > (e.g. smu_cmn_print_dpm_clk_levels, SMU13 OD emit_clk_levels), and revert it > for > small single-shot emits such as smu_v13_0_6 where truncation is not realistic. > Alternatively, we could add a single stop condition in > amdgpu_get_pp_od_clk_voltage() instead of per-emit checks. > > Let me know your thoughts. > > Regards > Asad > > -----Original Message----- > From: Wang, Yang(Kevin) <[email protected]> > Sent: Friday, May 29, 2026 12:46 PM > To: Kamal, Asad <[email protected]>; [email protected]; Koenig, > Christian <[email protected]> > Cc: Lazar, Lijo <[email protected]>; Zhang, Hawking > <[email protected]>; Ma, Le <[email protected]>; Zhang, Morris > <[email protected]>; Deucher, Alexander <[email protected]> > Subject: RE: [PATCH 1/4] drm/amd/pm: Handle truncation in common clk/pcie > printers > > AMD General > > The "len += sysfs_emit_at(...)" is a common practice in the Linux kernel. > Its return value has no adverse impact on subsequent calls, so repeatedly > checking > the return value serves no practical purpose. > You may refer to other usages of sysfs_emit_at() in the kernel source; none > of the > existing examples check its return value. > (this may be a widely adopted convention). > > Cc @Koenig, Christian > > Best Regards, > Kevin > > -----Original Message----- > > From: Kamal, Asad <[email protected]> > > Sent: Friday, May 29, 2026 14:19 > > To: [email protected] > > Cc: Lazar, Lijo <[email protected]>; Zhang, Hawking > > <[email protected]>; Ma, Le <[email protected]>; Zhang, Morris > > <[email protected]>; Deucher, Alexander > <[email protected]>; > > Wang, Yang(Kevin) <[email protected]>; Kamal, Asad > > <[email protected]> > > Subject: [PATCH 1/4] drm/amd/pm: Handle truncation in common clk/pcie > > printers > > > > In smu_cmn_print_dpm_clk_levels() and smu_cmn_print_pcie_levels(), use > > the > > sysfs_emit_at() return value, break out of per-level loops when n == > > 0, and use a shared out label before updating offset. > > > > Signed-off-by: Asad Kamal <[email protected]> > > --- > > drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c | 77 > > +++++++++++++++----------- > > 1 file changed, 44 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c > > b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c > > index 546e64e3ba9c..0a745afa8552 100644 > > --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c > > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c > > @@ -1384,7 +1384,7 @@ int smu_cmn_print_dpm_clk_levels(struct > > smu_context *smu, { > > uint32_t min_clk, max_clk, level_index, count; > > uint32_t freq_values[3]; > > - int size, lvl, i; > > + int size, lvl, i, n; > > bool is_fine_grained; > > bool is_deep_sleep; > > bool freq_match; > > @@ -1402,7 +1402,10 @@ int smu_cmn_print_dpm_clk_levels(struct > > smu_context *smu, > > /* Deep sleep - current clock < min_clock/2, TBD: cur_clk = 0 as > > GFXOFF */ > > is_deep_sleep = cur_clk < min_clk / 2; > > if (is_deep_sleep) { > > - size += sysfs_emit_at(buf, size, "S: %uMhz *\n", cur_clk); > > + n = sysfs_emit_at(buf, size, "S: %uMhz *\n", cur_clk); > > + if (!n) > > + goto out; > > + size += n; > > level_index = 1; > > } > > > > @@ -1412,10 +1415,13 @@ int smu_cmn_print_dpm_clk_levels(struct > > smu_context *smu, > > smu_cmn_freqs_match( > > cur_clk, > > dpm_table->dpm_levels[i].value); > > - size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", > > - level_index + i, > > - dpm_table->dpm_levels[i].value, > > - freq_match ? "*" : ""); > > + n = sysfs_emit_at(buf, size, "%d: %uMhz %s\n", > > + level_index + i, > > + dpm_table->dpm_levels[i].value, > > + freq_match ? "*" : ""); > > + if (!n) > > + break; > > + size += n; > > } > > } else { > > count = 2; > > @@ -1437,13 +1443,16 @@ int smu_cmn_print_dpm_clk_levels(struct > > smu_context *smu, > > } > > > > for (i = 0; i < count; i++) { > > - size += sysfs_emit_at( > > - buf, size, "%d: %uMhz %s\n", level_index + i, > > - freq_values[i], > > - (!is_deep_sleep && i == lvl) ? "*" : ""); > > + n = sysfs_emit_at(buf, size, "%d: %uMhz %s\n", > > + level_index + i, freq_values[i], > > + (!is_deep_sleep && i == lvl) ? "*" > > : ""); > > + if (!n) > > + break; > > + size += n; > > } > > } > > > > +out: > > *offset = size; > > > > return 0; > > @@ -1454,7 +1463,7 @@ int smu_cmn_print_pcie_levels(struct smu_context > *smu, > > uint32_t cur_gen, uint32_t cur_lane, char *buf, > > int *offset) { > > - int size, i; > > + int size, i, n; > > > > if (!pcie_table || !buf) > > return -EINVAL; > > @@ -1462,28 +1471,30 @@ int smu_cmn_print_pcie_levels(struct > > smu_context *smu, > > size = *offset; > > > > for (i = 0; i < pcie_table->lclk_levels; i++) { > > - size += sysfs_emit_at( > > - buf, size, "%d: %s %s %dMhz %s\n", i, > > - (pcie_table->pcie_gen[i] == 0) ? "2.5GT/s," : > > - (pcie_table->pcie_gen[i] == 1) ? "5.0GT/s," : > > - (pcie_table->pcie_gen[i] == 2) ? "8.0GT/s," : > > - (pcie_table->pcie_gen[i] == 3) ? "16.0GT/s," : > > - (pcie_table->pcie_gen[i] == 4) ? "32.0GT/s," : > > - (pcie_table->pcie_gen[i] == 5) ? "64.0GT/s," : > > - "", > > - (pcie_table->pcie_lane[i] == 1) ? "x1" : > > - (pcie_table->pcie_lane[i] == 2) ? "x2" : > > - (pcie_table->pcie_lane[i] == 3) ? "x4" : > > - (pcie_table->pcie_lane[i] == 4) ? "x8" : > > - (pcie_table->pcie_lane[i] == 5) ? "x12" : > > - (pcie_table->pcie_lane[i] == 6) ? "x16" : > > - (pcie_table->pcie_lane[i] == 7) ? "x32" : > > - "", > > - pcie_table->lclk_freq[i], > > - (cur_gen == pcie_table->pcie_gen[i]) && > > - (cur_lane == > > pcie_table->pcie_lane[i]) ? > > - "*" : > > - ""); > > + n = sysfs_emit_at(buf, size, "%d: %s %s %dMhz %s\n", i, > > + (pcie_table->pcie_gen[i] == 0) ? "2.5GT/s," > > : > > + (pcie_table->pcie_gen[i] == 1) ? "5.0GT/s," > > : > > + (pcie_table->pcie_gen[i] == 2) ? "8.0GT/s," > > : > > + (pcie_table->pcie_gen[i] == 3) ? > > "16.0GT/s," : > > + (pcie_table->pcie_gen[i] == 4) ? > > "32.0GT/s," : > > + (pcie_table->pcie_gen[i] == 5) ? > > "64.0GT/s," : > > + "", > > + (pcie_table->pcie_lane[i] == 1) ? "x1" : > > + (pcie_table->pcie_lane[i] == 2) ? "x2" : > > + (pcie_table->pcie_lane[i] == 3) ? "x4" : > > + (pcie_table->pcie_lane[i] == 4) ? "x8" : > > + (pcie_table->pcie_lane[i] == 5) ? "x12" : > > + (pcie_table->pcie_lane[i] == 6) ? "x16" : > > + (pcie_table->pcie_lane[i] == 7) ? "x32" : > > + "", > > + pcie_table->lclk_freq[i], > > + (cur_gen == pcie_table->pcie_gen[i]) && > > + (cur_lane == pcie_table->pcie_lane[i]) ? > > + "*" : > > + ""); > > + if (!n) > > + break; > > + size += n; > > } > > > > *offset = size; > > -- > > 2.46.0 > >
