On 8/29/2023 3:09 PM, Ilpo Järvinen wrote: > On Tue, 29 Aug 2023, Shyam Sundar S K wrote: > >> There have been instances when the default size (1M) of the STB is not >> sufficient to get the complete traces of the failure. In such scenarios >> we can use a module_param to enable full trace that shall contain more >> debugging data. This is not a regular case and hence not enabling this >> capability by default. >> >> Co-developed-by: Harsh Jain <Harsh.Jain@xxxxxxx> >> Signed-off-by: Harsh Jain <Harsh.Jain@xxxxxxx> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@xxxxxxx> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx> >> --- >> v2->v3: >> - no change >> >> v1->v2: >> - rebase to 'review-hans' branch >> - drop 2/4 of v1 >> (https://patchwork.kernel.org/project/platform-driver-x86/list/?series=775324&state=%2A&archive=both) >> >> drivers/platform/x86/amd/pmc/pmc.c | 34 +++++++++++++++++++----------- >> drivers/platform/x86/amd/pmc/pmc.h | 1 + >> 2 files changed, 23 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c >> index 443bb78ea5f4..0e61ae74f1a9 100644 >> --- a/drivers/platform/x86/amd/pmc/pmc.c >> +++ b/drivers/platform/x86/amd/pmc/pmc.c >> @@ -53,6 +53,7 @@ >> >> /* STB Spill to DRAM Parameters */ >> #define S2D_TELEMETRY_BYTES_MAX 0x100000 >> +#define S2D_TELEMETRY_FSIZE_MAX 0x200000 >> #define S2D_TELEMETRY_DRAMBYTES_MAX 0x1000000 >> >> /* STB Spill to DRAM Message Definition */ >> @@ -160,6 +161,10 @@ static bool disable_workarounds; >> module_param(disable_workarounds, bool, 0644); >> MODULE_PARM_DESC(disable_workarounds, "Disable workarounds for platform bugs"); >> >> +static bool dump_custom_stb; >> +module_param(dump_custom_stb, bool, 0644); >> +MODULE_PARM_DESC(dump_custom_stb, "Enable to dump full STB buffer"); >> + >> static struct amd_pmc_dev pmc; >> static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret); >> static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf); >> @@ -239,7 +244,7 @@ static const struct file_operations amd_pmc_stb_debugfs_fops = { >> static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp) >> { >> struct amd_pmc_dev *dev = filp->f_inode->i_private; >> - u32 *buf, fsize, num_samples, val, stb_rdptr_offset = 0; >> + u32 *buf, num_samples, val, stb_rdptr_offset = 0; >> int ret; >> >> /* Write dummy postcode while reading the STB buffer */ >> @@ -247,10 +252,6 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp) >> if (ret) >> dev_err(dev->dev, "error writing to STB: %d\n", ret); >> >> - buf = kzalloc(S2D_TELEMETRY_BYTES_MAX, GFP_KERNEL); >> - if (!buf) >> - return -ENOMEM; >> - >> /* Spill to DRAM num_samples uses separate SMU message port */ >> dev->msg_port = 1; >> >> @@ -264,20 +265,27 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp) >> dev->msg_port = 0; >> if (ret) { >> dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret); >> - kfree(buf); >> return ret; >> } >> >> /* Start capturing data from the last push location */ >> - if (num_samples > S2D_TELEMETRY_BYTES_MAX) { >> - fsize = S2D_TELEMETRY_BYTES_MAX; >> - stb_rdptr_offset = num_samples - fsize; >> + if (dump_custom_stb && >> + (dev->dram_size - S2D_TELEMETRY_BYTES_MAX <= S2D_TELEMETRY_FSIZE_MAX)) { >> + dev->fsize = dev->dram_size - S2D_TELEMETRY_BYTES_MAX; >> + stb_rdptr_offset = 0; >> + } else if (num_samples > S2D_TELEMETRY_BYTES_MAX) { > > I find the logic here somewhat suspicious in the first if check (I fail > to follow the reasoning here despite spending considerable amount of time > staring at it). > > Lets assume dump_custom_stb is set. If dev->dram_size is large, the first > if condition will be false because LHS of <= is large. Thus, we go to the > second branch and dump exactly as many samples as before this patch. > ...And that is opposite of what this patch claims to do? > Let me explain: We have cases where the S2D (Spill to DRAM) STB (Smart Trace Buffer - which is meant to have debug data that gives information about the system conditions, be it success or failure.) may not fit in the current stb buffer size of 1M (i.e. S2D_TELEMETRY_BYTES_MAX). The stb buffer size of 1M is sufficient for most of the platforms, but there could be exceptions. In those cases, the dram_size would be updated by the PMFW (Power Management Firmware) coupled with "dump_custom_stb" to fit into a custom requirement. As an example (with this proposed change): when dump_custom_stb is SET and dev->dram_size is 3M (received from PMFW) the first if() will be successful and it takes the corresponding code path. >> + dev->fsize = S2D_TELEMETRY_BYTES_MAX; > > Please remove also the extra space here since you're touching this line. Ah! thanks, will change this in v4. Thanks, Shyam >
