On Tue, 29 Aug 2023, Shyam Sundar S K wrote: > > > 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. Heh, I can see the numbers work in that case, however, the above still doesn't really explain why only S2D_TELEMETRY_BYTES_MAX is copied if dram_size - S2D_TELEMETRY_BYTES_MAX > S2D_TELEMETRY_FSIZE_MAX and not S2D_TELEMETRY_FSIZE_MAX as I would expect? -- i.
