[PATCH] platform/x86/amd/pmc: Detect when STB is not available

[PATCH] platform/x86/amd/pmc: Detect when STB is not available

[Corey Hickey]

From: Corey Hickey <bugfood-c@fatooh.org>

Loading the amd_pmc module as:

    amd_pmc enable_stb=1

...can result in the following messages in the kernel ring buffer:

    amd_pmc AMDI0009:00: SMU cmd failed. err: 0xff
    ioremap on RAM at 0x0000000000000000 - 0x0000000000ffffff
    WARNING: CPU: 10 PID: 2151 at arch/x86/mm/ioremap.c:217 __ioremap_caller+0x2cd/0x340

Additional debug shows that this happens when the calls to obtain
S2D_PHYS_ADDR_LOW and S2D_PHYS_ADDR_HIGH return 0.

Per discussion on platform-driver-x86@vger.kernel.org, this condition
indicates that the STB is not available.

In order to avoid the ioremap warning, and to help the user understand
the situation, catch the invalid address and print an error.

Signed-off-by: Corey Hickey <bugfood-c@fatooh.org>
---
 drivers/platform/x86/amd/pmc/pmc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index bbb8edb62e00..72b1dfc64bf1 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -998,6 +998,11 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
 	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
 	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
 
+	if (!phys_addr_hi && !phys_addr_low) {
+		dev_err(dev->dev, "amd_pmc: STB is not enabled on the system; disable enable_stb or contact system vendor\n");
+		return -EINVAL;
+	}
+
 	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
 
 	/* Clear msg_port for other SMU operation */
-- 
2.45.2

Re: [PATCH] platform/x86/amd/pmc: Detect when STB is not available

[Ilpo Järvinen]

On Thu, 24 Oct 2024, Corey Hickey wrote:

> From: Corey Hickey <bugfood-c@fatooh.org>
> 
> Loading the amd_pmc module as:
> 
>     amd_pmc enable_stb=1
> 
> ...can result in the following messages in the kernel ring buffer:
> 
>     amd_pmc AMDI0009:00: SMU cmd failed. err: 0xff
>     ioremap on RAM at 0x0000000000000000 - 0x0000000000ffffff
>     WARNING: CPU: 10 PID: 2151 at arch/x86/mm/ioremap.c:217 __ioremap_caller+0x2cd/0x340
> 
> Additional debug shows that this happens when the calls to obtain
> S2D_PHYS_ADDR_LOW and S2D_PHYS_ADDR_HIGH return 0.
> 
> Per discussion on platform-driver-x86@vger.kernel.org, this condition
> indicates that the STB is not available.

If you want to refer to discussion, add it into a Link: tag. Only write 
the conclusion into the commit message (the part you have there after 
comma).

> In order to avoid the ioremap warning, and to help the user understand
> the situation, catch the invalid address and print an error.
> 
> Signed-off-by: Corey Hickey <bugfood-c@fatooh.org>

Isn't Fixes tag appropriate for this change?

> ---
>  drivers/platform/x86/amd/pmc/pmc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index bbb8edb62e00..72b1dfc64bf1 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -998,6 +998,11 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>  	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
>  	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
>  
> +	if (!phys_addr_hi && !phys_addr_low) {
> +		dev_err(dev->dev, "amd_pmc: STB is not enabled on the system; disable enable_stb or contact system vendor\n");

Won't that end up duplicating the prefix if you put one into the string? 
The prefix is handled for you by pr_fmt() which is already provided in 
this file.

-- 
 i.

> +		return -EINVAL;
> +	}
> +
>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>  
>  	/* Clear msg_port for other SMU operation */
> 

Re: [PATCH] platform/x86/amd/pmc: Detect when STB is not available

[Corey Hickey]

On 2024-10-25 07:55, Ilpo Järvinen wrote:
>> Per discussion on platform-driver-x86@vger.kernel.org, this condition
>> indicates that the STB is not available.
> 
> If you want to refer to discussion, add it into a Link: tag. Only write
> the conclusion into the commit message (the part you have there after
> comma).

Thank you, I have added a Link tag to the mailing list archives.

I wanted to avoid taking credit for the claim, though--I don't have the 
background to claim that on my own. I have changed the wording to refer 
to Shyam Sundar S K, hoping that's an ok thing to do.

>> In order to avoid the ioremap warning, and to help the user understand
>> the situation, catch the invalid address and print an error.
>>
>> Signed-off-by: Corey Hickey <bugfood-c@fatooh.org>
> 
> Isn't Fixes tag appropriate for this change?

Hmm... I didn't think so at first, but I guess so? I didn't do a git 
bisect, but I can identify that the code in question was added by 
3d7d407dfb05, so I added that as a Fixes tag.

>> ---
>>   drivers/platform/x86/amd/pmc/pmc.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>> index bbb8edb62e00..72b1dfc64bf1 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -998,6 +998,11 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>>   	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
>>   	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
>>   
>> +	if (!phys_addr_hi && !phys_addr_low) {
>> +		dev_err(dev->dev, "amd_pmc: STB is not enabled on the system; disable enable_stb or contact system vendor\n");
> 
> Won't that end up duplicating the prefix if you put one into the string?
> The prefix is handled for you by pr_fmt() which is already provided in
> this file.

Ah, yes, I missed that.

Thank you for your review. I will submit a patch v2 momentarily.

-Corey

[PATCH v2] platform/x86/amd/pmc: Detect when STB is not available

[Corey Hickey]

From: Corey Hickey <bugfood-c@fatooh.org>

Loading the amd_pmc module as:

    amd_pmc enable_stb=1

...can result in the following messages in the kernel ring buffer:

    amd_pmc AMDI0009:00: SMU cmd failed. err: 0xff
    ioremap on RAM at 0x0000000000000000 - 0x0000000000ffffff
    WARNING: CPU: 10 PID: 2151 at arch/x86/mm/ioremap.c:217 __ioremap_caller+0x2cd/0x340

Additional debug shows that this happens when the calls to obtain
S2D_PHYS_ADDR_LOW and S2D_PHYS_ADDR_HIGH return 0.

According to Shyam Sundar S K <Shyam-sundar.S-k@amd.com>, this condition
indicates that the STB is not available.

In order to avoid the ioremap warning, and to help the user understand
the situation, catch the invalid address and print an error.

Link: https://lore.kernel.org/platform-driver-x86/c588ff5d-3e04-4549-9a86-284b9b4419ba@amd.com
Fixes: 3d7d407dfb05 ("platform/x86: amd-pmc: Add support for AMD Spill to DRAM STB feature")
Signed-off-by: Corey Hickey <bugfood-c@fatooh.org>
---
 drivers/platform/x86/amd/pmc/pmc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index bbb8edb62e00..5669f94c3d06 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -998,6 +998,11 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
 	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
 	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
 
+	if (!phys_addr_hi && !phys_addr_low) {
+		dev_err(dev->dev, "STB is not enabled on the system; disable enable_stb or contact system vendor\n");
+		return -EINVAL;
+	}
+
 	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
 
 	/* Clear msg_port for other SMU operation */
-- 
2.45.2

Re: [PATCH v2] platform/x86/amd/pmc: Detect when STB is not available

[Shyam Sundar S K]

On 10/27/2024 08:04, Corey Hickey wrote:
> From: Corey Hickey <bugfood-c@fatooh.org>
> 
> Loading the amd_pmc module as:
> 
>     amd_pmc enable_stb=1
> 
> ...can result in the following messages in the kernel ring buffer:
> 
>     amd_pmc AMDI0009:00: SMU cmd failed. err: 0xff
>     ioremap on RAM at 0x0000000000000000 - 0x0000000000ffffff
>     WARNING: CPU: 10 PID: 2151 at arch/x86/mm/ioremap.c:217 __ioremap_caller+0x2cd/0x340
> 
> Additional debug shows that this happens when the calls to obtain
> S2D_PHYS_ADDR_LOW and S2D_PHYS_ADDR_HIGH return 0.
> 
> According to Shyam Sundar S K <Shyam-sundar.S-k@amd.com>, this condition
> indicates that the STB is not available.

No need to put my name in the commit message.

You may simply the commit message to something like this:

Further debugging reveals that this occurs when the requests for
S2D_PHYS_ADDR_LOW and S2D_PHYS_ADDR_HIGH return a value of 0,
indicating that the STB is inaccessible. To prevent the ioremap
warning and provide clarity to the user, handle the invalid address
and display an error message.

With this addressed

Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

Thanks,
Shyam

> 
> In order to avoid the ioremap warning, and to help the user understand
> the situation, catch the invalid address and print an error.
> 
> Link: https://lore.kernel.org/platform-driver-x86/c588ff5d-3e04-4549-9a86-284b9b4419ba@amd.com
> Fixes: 3d7d407dfb05 ("platform/x86: amd-pmc: Add support for AMD Spill to DRAM STB feature")
> Signed-off-by: Corey Hickey <bugfood-c@fatooh.org>
> ---
>  drivers/platform/x86/amd/pmc/pmc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index bbb8edb62e00..5669f94c3d06 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -998,6 +998,11 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>  	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
>  	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
>  
> +	if (!phys_addr_hi && !phys_addr_low) {
> +		dev_err(dev->dev, "STB is not enabled on the system; disable enable_stb or contact system vendor\n");
> +		return -EINVAL;
> +	}
> +
>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>  
>  	/* Clear msg_port for other SMU operation */

[PATCH v3] platform/x86/amd/pmc: Detect when STB is not available

[Corey Hickey]

From: Corey Hickey <bugfood-c@fatooh.org>

Loading the amd_pmc module as:

    amd_pmc enable_stb=1

...can result in the following messages in the kernel ring buffer:

    amd_pmc AMDI0009:00: SMU cmd failed. err: 0xff
    ioremap on RAM at 0x0000000000000000 - 0x0000000000ffffff
    WARNING: CPU: 10 PID: 2151 at arch/x86/mm/ioremap.c:217 __ioremap_caller+0x2cd/0x340

Further debugging reveals that this occurs when the requests for
S2D_PHYS_ADDR_LOW and S2D_PHYS_ADDR_HIGH return a value of 0,
indicating that the STB is inaccessible. To prevent the ioremap
warning and provide clarity to the user, handle the invalid address
and display an error message.

Link: https://lore.kernel.org/platform-driver-x86/c588ff5d-3e04-4549-9a86-284b9b4419ba@amd.com
Fixes: 3d7d407dfb05 ("platform/x86: amd-pmc: Add support for AMD Spill to DRAM STB feature")
Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Signed-off-by: Corey Hickey <bugfood-c@fatooh.org>
---
 drivers/platform/x86/amd/pmc/pmc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index bbb8edb62e00..5669f94c3d06 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -998,6 +998,11 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
 	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
 	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
 
+	if (!phys_addr_hi && !phys_addr_low) {
+		dev_err(dev->dev, "STB is not enabled on the system; disable enable_stb or contact system vendor\n");
+		return -EINVAL;
+	}
+
 	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
 
 	/* Clear msg_port for other SMU operation */
-- 
2.45.2

Re: [PATCH v2] platform/x86/amd/pmc: Detect when STB is not available

[Corey Hickey]

On 2024-10-27 20:55, Shyam Sundar S K wrote:
> No need to put my name in the commit message.
> 
> You may simply the commit message to something like this:
> 
> Further debugging reveals that this occurs when the requests for
> S2D_PHYS_ADDR_LOW and S2D_PHYS_ADDR_HIGH return a value of 0,
> indicating that the STB is inaccessible. To prevent the ioremap
> warning and provide clarity to the user, handle the invalid address
> and display an error message.
> 
> With this addressed
> 
> Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

Ok, thank you. I have made these changes now as patch v3.

-Corey

Re: [PATCH v3] platform/x86/amd/pmc: Detect when STB is not available

[Ilpo Järvinen]

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

On Mon, 28 Oct 2024, Corey Hickey wrote:

> From: Corey Hickey <bugfood-c@fatooh.org>
> 
> Loading the amd_pmc module as:
> 
>     amd_pmc enable_stb=1
> 
> ...can result in the following messages in the kernel ring buffer:
> 
>     amd_pmc AMDI0009:00: SMU cmd failed. err: 0xff
>     ioremap on RAM at 0x0000000000000000 - 0x0000000000ffffff
>     WARNING: CPU: 10 PID: 2151 at arch/x86/mm/ioremap.c:217 __ioremap_caller+0x2cd/0x340
> 
> Further debugging reveals that this occurs when the requests for
> S2D_PHYS_ADDR_LOW and S2D_PHYS_ADDR_HIGH return a value of 0,
> indicating that the STB is inaccessible. To prevent the ioremap
> warning and provide clarity to the user, handle the invalid address
> and display an error message.
> 
> Link: https://lore.kernel.org/platform-driver-x86/c588ff5d-3e04-4549-9a86-284b9b4419ba@amd.com
> Fixes: 3d7d407dfb05 ("platform/x86: amd-pmc: Add support for AMD Spill to DRAM STB feature")
> Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Corey Hickey <bugfood-c@fatooh.org>
> ---
>  drivers/platform/x86/amd/pmc/pmc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index bbb8edb62e00..5669f94c3d06 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -998,6 +998,11 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>  	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
>  	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
>  
> +	if (!phys_addr_hi && !phys_addr_low) {
> +		dev_err(dev->dev, "STB is not enabled on the system; disable enable_stb or contact system vendor\n");
> +		return -EINVAL;
> +	}
> +
>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>  
>  	/* Clear msg_port for other SMU operation */
> 

Thanks for the update,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v3] platform/x86/amd/pmc: Detect when STB is not available

[Hans de Goede]

Hi,

On 28-Oct-24 7:02 PM, Corey Hickey wrote:
> From: Corey Hickey <bugfood-c@fatooh.org>
> 
> Loading the amd_pmc module as:
> 
>     amd_pmc enable_stb=1
> 
> ...can result in the following messages in the kernel ring buffer:
> 
>     amd_pmc AMDI0009:00: SMU cmd failed. err: 0xff
>     ioremap on RAM at 0x0000000000000000 - 0x0000000000ffffff
>     WARNING: CPU: 10 PID: 2151 at arch/x86/mm/ioremap.c:217 __ioremap_caller+0x2cd/0x340
> 
> Further debugging reveals that this occurs when the requests for
> S2D_PHYS_ADDR_LOW and S2D_PHYS_ADDR_HIGH return a value of 0,
> indicating that the STB is inaccessible. To prevent the ioremap
> warning and provide clarity to the user, handle the invalid address
> and display an error message.
> 
> Link: https://lore.kernel.org/platform-driver-x86/c588ff5d-3e04-4549-9a86-284b9b4419ba@amd.com
> Fixes: 3d7d407dfb05 ("platform/x86: amd-pmc: Add support for AMD Spill to DRAM STB feature")
> Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Corey Hickey <bugfood-c@fatooh.org>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans



> ---
>  drivers/platform/x86/amd/pmc/pmc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index bbb8edb62e00..5669f94c3d06 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -998,6 +998,11 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>  	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
>  	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
>  
> +	if (!phys_addr_hi && !phys_addr_low) {
> +		dev_err(dev->dev, "STB is not enabled on the system; disable enable_stb or contact system vendor\n");
> +		return -EINVAL;
> +	}
> +
>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>  
>  	/* Clear msg_port for other SMU operation */