[PATCH v7 1/3] platform/x86/amd/pmc: Use flex array when calling amd_pmc_stb_debugfs_open_v2()

[PATCH v7 1/3] platform/x86/amd/pmc: Use flex array when calling amd_pmc_stb_debugfs_open_v2()

[Shyam Sundar S K]

Currently in amd_pmc_stb_debugfs_open_v2() the buffer size is assumed to
be fixed and a second call to amd_pmc_stb_debugfs_open_v2() may race with
a process holding open another fd. This could change "fsize" to a
bigger size causing an out of bounds read.

Instead create a struct with a flexarray to solve this.

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

v6:
- Handle release buffer case as per Hans remarks
- based on review-ilpo branch

v5:
- new patch based on comments in v4 from Hans.
- based on review-ilpo branch

 drivers/platform/x86/amd/pmc/pmc.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index c92dd5077a16..fdc1e104c437 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -122,6 +122,11 @@ enum s2d_arg {
 	S2D_DRAM_SIZE,
 };
 
+struct amd_pmc_stb_v2_data {
+	size_t size;
+	u8 data[] __counted_by(size);
+};
+
 struct amd_pmc_bit_map {
 	const char *name;
 	u32 bit_mask;
@@ -239,7 +244,8 @@ static const struct file_operations amd_pmc_stb_debugfs_fops = {
 static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 {
 	struct amd_pmc_dev *dev = filp->f_inode->i_private;
-	u32 *buf, fsize, num_samples, val, stb_rdptr_offset = 0;
+	u32 fsize, num_samples, val, stb_rdptr_offset = 0;
+	struct amd_pmc_stb_v2_data *flex_arr;
 	int ret;
 
 	/* Write dummy postcode while reading the STB buffer */
@@ -247,10 +253,6 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 	if (ret)
 		dev_err(dev->dev, "error writing to STB: %d\n", ret);
 
-	buf = kzalloc(S2D_TELEMETRY_BYTES_MAX, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
 	/* Spill to DRAM num_samples uses separate SMU message port */
 	dev->msg_port = 1;
 
@@ -264,10 +266,16 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 	dev->msg_port = 0;
 	if (ret) {
 		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
-		kfree(buf);
 		return ret;
 	}
 
+	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;
+
 	/* Start capturing data from the last push location */
 	if (num_samples > S2D_TELEMETRY_BYTES_MAX) {
 		fsize  = S2D_TELEMETRY_BYTES_MAX;
@@ -277,8 +285,8 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 		stb_rdptr_offset = 0;
 	}
 
-	memcpy_fromio(buf, dev->stb_virt_addr + stb_rdptr_offset, fsize);
-	filp->private_data = buf;
+	memcpy_fromio(flex_arr->data, dev->stb_virt_addr + stb_rdptr_offset, fsize);
+	filp->private_data = flex_arr;
 
 	return 0;
 }
@@ -286,11 +294,9 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
 					   loff_t *pos)
 {
-	if (!filp->private_data)
-		return -EINVAL;
+	struct amd_pmc_stb_v2_data *data = filp->private_data;
 
-	return simple_read_from_buffer(buf, size, pos, filp->private_data,
-					S2D_TELEMETRY_BYTES_MAX);
+	return simple_read_from_buffer(buf, size, pos, data->data, data->size);
 }
 
 static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
-- 
2.25.1

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

[Shyam Sundar S K]

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);
 	}
 
-	memcpy_fromio(flex_arr->data, dev->stb_virt_addr + stb_rdptr_offset, fsize);
 	filp->private_data = flex_arr;
 
 	return 0;
-- 
2.25.1

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

[Ilpo Järvinen]

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?

-- 
 i.

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

[Shyam Sundar S K]

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

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

[Shyam Sundar S K]

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;
+}
+
 static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 {
 	struct amd_pmc_dev *dev = filp->f_inode->i_private;
@@ -260,6 +284,14 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 	if (ret)
 		dev_warn_once(dev->dev, "S2D force flush not supported\n");
 
+	/*
+	 * We have a custom stb size and the PMFW is supposed to give
+	 * the enhanced dram size. Note that we land here only for the
+	 * platforms that support enhanced dram size reporting.
+	 */
+	if (dump_custom_stb)
+		return amd_pmc_stb_handle_efr(filp);
+
 	/* Get the num_samples to calculate the last push location */
 	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
 	/* Clear msg_port for other SMU operation */
-- 
2.25.1

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

[Ilpo Järvinen]

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?

-- 
 i.

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

[Shyam Sundar S K]

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.

Thanks,
Shyam

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

[Ilpo Järvinen]

[-- 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.

Re: [PATCH v7 1/3] platform/x86/amd/pmc: Use flex array when calling amd_pmc_stb_debugfs_open_v2()

[Ilpo Järvinen]

On Mon, 9 Oct 2023, Shyam Sundar S K wrote:

> Currently in amd_pmc_stb_debugfs_open_v2() the buffer size is assumed to
> be fixed and a second call to amd_pmc_stb_debugfs_open_v2() may race with
> a process holding open another fd. This could change "fsize" to a
> bigger size causing an out of bounds read.
> 
> Instead create a struct with a flexarray to solve this.
> 
> 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
> 
> v6:
> - Handle release buffer case as per Hans remarks
> - based on review-ilpo branch
> 
> v5:
> - new patch based on comments in v4 from Hans.
> - based on review-ilpo branch
> 
>  drivers/platform/x86/amd/pmc/pmc.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index c92dd5077a16..fdc1e104c437 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -122,6 +122,11 @@ enum s2d_arg {
>  	S2D_DRAM_SIZE,
>  };
>  
> +struct amd_pmc_stb_v2_data {
> +	size_t size;
> +	u8 data[] __counted_by(size);
> +};
> +
>  struct amd_pmc_bit_map {
>  	const char *name;
>  	u32 bit_mask;
> @@ -239,7 +244,8 @@ static const struct file_operations amd_pmc_stb_debugfs_fops = {
>  static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  {
>  	struct amd_pmc_dev *dev = filp->f_inode->i_private;
> -	u32 *buf, fsize, num_samples, val, stb_rdptr_offset = 0;
> +	u32 fsize, num_samples, val, stb_rdptr_offset = 0;
> +	struct amd_pmc_stb_v2_data *flex_arr;
>  	int ret;
>  
>  	/* Write dummy postcode while reading the STB buffer */
> @@ -247,10 +253,6 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  	if (ret)
>  		dev_err(dev->dev, "error writing to STB: %d\n", ret);
>  
> -	buf = kzalloc(S2D_TELEMETRY_BYTES_MAX, GFP_KERNEL);
> -	if (!buf)
> -		return -ENOMEM;
> -
>  	/* Spill to DRAM num_samples uses separate SMU message port */
>  	dev->msg_port = 1;
>  
> @@ -264,10 +266,16 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  	dev->msg_port = 0;
>  	if (ret) {
>  		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
> -		kfree(buf);
>  		return ret;
>  	}
>  
> +	fsize = (num_samples > S2D_TELEMETRY_BYTES_MAX) ? S2D_TELEMETRY_BYTES_MAX : num_samples;

min() but that will only work when you add U postfix to 
S2D_TELEMETRY_BYTES_MAX (I see no reason why it couldn't make it 
unsigned).

Make sure to add the include for it too.

-- 
 i.


> +	flex_arr = kmalloc(struct_size(flex_arr, data, fsize), GFP_KERNEL);
> +	if (!flex_arr)
> +		return -ENOMEM;
> +
> +	flex_arr->size = fsize;
> +
>  	/* Start capturing data from the last push location */
>  	if (num_samples > S2D_TELEMETRY_BYTES_MAX) {
>  		fsize  = S2D_TELEMETRY_BYTES_MAX;
> @@ -277,8 +285,8 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  		stb_rdptr_offset = 0;
>  	}
>  
> -	memcpy_fromio(buf, dev->stb_virt_addr + stb_rdptr_offset, fsize);
> -	filp->private_data = buf;
> +	memcpy_fromio(flex_arr->data, dev->stb_virt_addr + stb_rdptr_offset, fsize);
> +	filp->private_data = flex_arr;
>  
>  	return 0;
>  }
> @@ -286,11 +294,9 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
>  					   loff_t *pos)
>  {
> -	if (!filp->private_data)
> -		return -EINVAL;
> +	struct amd_pmc_stb_v2_data *data = filp->private_data;
>  
> -	return simple_read_from_buffer(buf, size, pos, filp->private_data,
> -					S2D_TELEMETRY_BYTES_MAX);
> +	return simple_read_from_buffer(buf, size, pos, data->data, data->size);
>  }
>  
>  static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
> 

Re: [PATCH v7 1/3] platform/x86/amd/pmc: Use flex array when calling amd_pmc_stb_debugfs_open_v2()

[Shyam Sundar S K]

On 10/9/2023 8:42 PM, Ilpo Järvinen wrote:
> On Mon, 9 Oct 2023, Shyam Sundar S K wrote:
> 
>> Currently in amd_pmc_stb_debugfs_open_v2() the buffer size is assumed to
>> be fixed and a second call to amd_pmc_stb_debugfs_open_v2() may race with
>> a process holding open another fd. This could change "fsize" to a
>> bigger size causing an out of bounds read.
>>
>> Instead create a struct with a flexarray to solve this.
>>
>> 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
>>
>> v6:
>> - Handle release buffer case as per Hans remarks
>> - based on review-ilpo branch
>>
>> v5:
>> - new patch based on comments in v4 from Hans.
>> - based on review-ilpo branch
>>
>>  drivers/platform/x86/amd/pmc/pmc.c | 30 ++++++++++++++++++------------
>>  1 file changed, 18 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>> index c92dd5077a16..fdc1e104c437 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -122,6 +122,11 @@ enum s2d_arg {
>>  	S2D_DRAM_SIZE,
>>  };
>>  
>> +struct amd_pmc_stb_v2_data {
>> +	size_t size;
>> +	u8 data[] __counted_by(size);
>> +};
>> +
>>  struct amd_pmc_bit_map {
>>  	const char *name;
>>  	u32 bit_mask;
>> @@ -239,7 +244,8 @@ static const struct file_operations amd_pmc_stb_debugfs_fops = {
>>  static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>>  {
>>  	struct amd_pmc_dev *dev = filp->f_inode->i_private;
>> -	u32 *buf, fsize, num_samples, val, stb_rdptr_offset = 0;
>> +	u32 fsize, num_samples, val, stb_rdptr_offset = 0;
>> +	struct amd_pmc_stb_v2_data *flex_arr;
>>  	int ret;
>>  
>>  	/* Write dummy postcode while reading the STB buffer */
>> @@ -247,10 +253,6 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>>  	if (ret)
>>  		dev_err(dev->dev, "error writing to STB: %d\n", ret);
>>  
>> -	buf = kzalloc(S2D_TELEMETRY_BYTES_MAX, GFP_KERNEL);
>> -	if (!buf)
>> -		return -ENOMEM;
>> -
>>  	/* Spill to DRAM num_samples uses separate SMU message port */
>>  	dev->msg_port = 1;
>>  
>> @@ -264,10 +266,16 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>>  	dev->msg_port = 0;
>>  	if (ret) {
>>  		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
>> -		kfree(buf);
>>  		return ret;
>>  	}
>>  
>> +	fsize = (num_samples > S2D_TELEMETRY_BYTES_MAX) ? S2D_TELEMETRY_BYTES_MAX : num_samples;
> 
> min() but that will only work when you add U postfix to 
> S2D_TELEMETRY_BYTES_MAX (I see no reason why it couldn't make it 
> unsigned).
> 
> Make sure to add the include for it too.
> 

Sure, I will address this.

Thanks,
Shyam