On Mon, 9 Oct 2023, Shyam Sundar S K wrote: > On 10/9/2023 8:36 PM, Ilpo Järvinen wrote: > > On Mon, 9 Oct 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. > >> > >> With this change, there will be two cases on how the driver fetches the > >> stb data: > >> 1) A special case (proposed now) - which is required only for certain > >> platforms. Here, a new module param will be supplied to the driver that > >> will have a special PMFW supporting enhanced dram sizes for getting > >> the stb data. Without the special PMFW support, just setting the module > >> param will not help to get the enhanced stb data. > >> > >> To adapt to this change, we will have a new amd_pmc_stb_handle_efr() to > >> handle enhanced firmware reporting mechanism. Note that, since num_samples > >> based r/w pointer offset calculation is not required for enhanced firmware > >> reporting we will have this mailbox command sent only in case of regular > >> STB cases. > >> > >> 2) Current code branch which fetches the stb data based on the parameters > >> like the num_samples, fsize and the r/w pointer. > >> > >> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx> > >> 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> > >> --- > >> v6->v7: > >> - Code simplication > >> > >> v5->v6: > >> - No change > >> > >> v4->v5: > >> - create a new function amd_pmc_stb_handle_efr() to handle enhanced firmware reporting mechanism > >> - based on review-ilpo branch > >> > >> v3->v4: > >> - Update code branches and commit-msg as per Ilpo's remark. > >> > >> 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 | 32 ++++++++++++++++++++++++++++++ > >> 1 file changed, 32 insertions(+) > >> > >> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c > >> index e0b5d9de473a..af6d400193ff 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_RSVD_RAM_SPACE 0x100000 > >> #define S2D_TELEMETRY_DRAMBYTES_MAX 0x1000000 > >> > >> /* STB Spill to DRAM Message Definition */ > >> @@ -165,6 +166,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); > >> @@ -241,6 +246,25 @@ static const struct file_operations amd_pmc_stb_debugfs_fops = { > >> .release = amd_pmc_stb_debugfs_release, > >> }; > >> > >> +/* Enhanced STB Firmware Reporting Mechanism */ > >> +static int amd_pmc_stb_handle_efr(struct file *filp) > >> +{ > >> + struct amd_pmc_dev *dev = filp->f_inode->i_private; > >> + struct amd_pmc_stb_v2_data *flex_arr; > >> + u32 fsize; > >> + > >> + fsize = dev->dram_size - S2D_RSVD_RAM_SPACE; > >> + flex_arr = kmalloc(struct_size(flex_arr, data, fsize), GFP_KERNEL); > >> + if (!flex_arr) > >> + return -ENOMEM; > >> + > >> + flex_arr->size = fsize; > >> + memcpy_fromio(flex_arr->data, dev->stb_virt_addr, fsize); > >> + filp->private_data = flex_arr; > >> + > >> + return 0; > > > > Thanks, this make much more sense than the early versions! > > > > Just one confirmation, is dev->dram_size >= S2D_RSVD_RAM_SPACE always > > guaranteed so that the fsize never underflows? > > > > Yes, that's right. Okay thanks, Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> -- i.
