On 10/9/2023 8:51 PM, Ilpo Järvinen wrote: > On Mon, 9 Oct 2023, Shyam Sundar S K wrote: > >> In amd_pmc_stb_debugfs_open_v2(), the stb buffer is created based on the >> num_samples and the read/write pointer offset. This holds good when the >> num_samples reported by PMFW is less than S2D_TELEMETRY_BYTES_MAX; where >> the stb buffer gets filled from 0th position until >> S2D_TELEMETRY_BYTES_MAX - 1 based on the read/write pointer offset. >> >> But when the num_samples exceeds the S2D_TELEMETRY_BYTES_MAX, the current >> code does not handle it well as it does not account for the cases where >> the stb buffer has to filled up as a circular buffer. >> >> Handle this scenario into two cases, where first memcpy will have the >> samples from location: >> (num_samples % S2D_TELEMETRY_BYTES_MAX) - (S2D_TELEMETRY_BYTES_MAX - 1) >> and next memcpy will have the newest ones i.e. >> 0 - (num_samples % S2D_TELEMETRY_BYTES_MAX - 1) >> >> Suggested-by: Hans de Goede <hdegoede@xxxxxxxxxx> >> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@xxxxxxx> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx> >> --- >> v6->v7: >> - No change >> >> v5->v6: >> - Make changes as per Hans on v5 >> - based on review-ilpo branch >> >> v4->v5: >> - Fix exisiting code problems when reading stb buffer as a circular data >> - 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 | 19 +++++++++++++------ >> 1 file changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c >> index fdc1e104c437..e0b5d9de473a 100644 >> --- a/drivers/platform/x86/amd/pmc/pmc.c >> +++ b/drivers/platform/x86/amd/pmc/pmc.c >> @@ -276,16 +276,23 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp) >> >> flex_arr->size = fsize; >> >> - /* Start capturing data from the last push location */ >> + /* >> + * Start capturing data from the last push location. >> + * This is for general cases, where the stb limits >> + * are meant for standard usage. >> + */ >> if (num_samples > S2D_TELEMETRY_BYTES_MAX) { >> - fsize = S2D_TELEMETRY_BYTES_MAX; >> - stb_rdptr_offset = num_samples - fsize; >> + /* First read oldest data starting 1 behind last write till end of ringbuffer */ >> + stb_rdptr_offset = num_samples % S2D_TELEMETRY_BYTES_MAX; >> + fsize = S2D_TELEMETRY_BYTES_MAX - stb_rdptr_offset; >> + >> + memcpy_fromio(flex_arr->data, dev->stb_virt_addr + stb_rdptr_offset, fsize); >> + /* Second copy the newer samples from offset 0 - last write */ >> + memcpy_fromio(flex_arr->data + fsize, dev->stb_virt_addr, stb_rdptr_offset); >> } else { >> - fsize = num_samples; >> - stb_rdptr_offset = 0; >> + memcpy_fromio(flex_arr->data, dev->stb_virt_addr, fsize); > > Is this actually correct if less than S2D_TELEMETRY_BYTES_MAX are read > first time, and then the second call will use zero offset if num_samples > is still less than S2D_TELEMETRY_BYTES_MAX? It seems to return duplicated > entries and not the latest entries at all until num_samples wraps? That's right. If there are duplicate entries in the STB dump, there is an internal tool that parses all the STB data (remove duplicates, if any). Thanks, Shyam
