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
