Hi, On 10/9/23 11:45, 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> > Signed-off-by: Sanket Goswami <Sanket.Goswami@xxxxxxx> > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx> > --- > 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 | 25 +++++++++++++++++-------- > 1 file changed, 17 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c > index e00d69801369..67daa655cc6a 100644 > --- a/drivers/platform/x86/amd/pmc/pmc.c > +++ b/drivers/platform/x86/amd/pmc/pmc.c > @@ -271,18 +271,27 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp) I would prefer to keep the kzalloc as a single alloc call above the if, also no need to zero the mem now: fsize = (num_samples > S2D_TELEMETRY_BYTES_MAX) ? S2D_TELEMETRY_BYTES_MAX : num_samples; flex_arr = kmalloc(struct_size(flex_arr, data, fsize), GFP_KERNEL); if (!flex_arr) return -ENOMEM; flex_arr->size = fsize; And then drop the 2 kzalloc calles in the 2 branches of the if ... else ... > /* 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; > + /* 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; > + > + flex_arr = kzalloc(struct_size(flex_arr, data, S2D_TELEMETRY_BYTES_MAX), > + GFP_KERNEL); > + if (!flex_arr) > + return -ENOMEM; > + > + 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; > - } > + flex_arr = kzalloc(struct_size(flex_arr, data, num_samples), GFP_KERNEL); > + if (!flex_arr) > + return -ENOMEM; > > - flex_arr = kzalloc(struct_size(flex_arr, data, fsize), GFP_KERNEL); > - if (!flex_arr) > - return -ENOMEM; > + memcpy_fromio(flex_arr->data, dev->stb_virt_addr, num_samples); > + } > > - memcpy_fromio(flex_arr->data, dev->stb_virt_addr + stb_rdptr_offset, fsize); > flex_arr->size = fsize; This is wrong for the if (num_samples > S2D_TELEMETRY_BYTES_MAX) code path above since fsize there is used to calculate the size of the first copy, where as flex_arr->size should be set to S2D_TELEMETRY_BYTES_MAX. Dong the alloc + assigning flex_arr->size first as suggested above fixes this. > filp->private_data = flex_arr->data; > Regards, Hans
