Re: [PATCH v7 2/3] platform/x86/amd/pmc: Handle overflow cases where the num_samples range is higher

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

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@redhat.com>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.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:
>> - 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