Re: [PATCH v7 3/3] platform/x86/amd/pmc: Add dump_custom_stb module parameter

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4496 bytes --]

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@redhat.com>
> >> Co-developed-by: Harsh Jain <Harsh.Jain@amd.com>
> >> Signed-off-by: Harsh Jain <Harsh.Jain@amd.com>
> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> ---
> >> 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@linux.intel.com>

-- 
 i.