Re: [PATCH 2/4] platform/x86/amd/pmc: Fix error handling in amd_stb_s2d_init()

[Ilpo Järvinen]

On Fri, 17 Jul 2026, Mario Limonciello wrote:

> amd_stb_s2d_init() has two problems on its error paths:
> 
>  - The return value of the S2D_TELEMETRY_SIZE SMU command is discarded.
>    When the SMU refuses the command (e.g. "SMU cmd failed. err: 0xff")
>    the failure is only noticed indirectly through the telemetry size
>    check and reported as -EIO, masking the real error.
> 
>  - dev->msg_port is switched to MSG_PORT_S2D before issuing the S2D SMU
>    commands but is only restored to MSG_PORT_PMC on the success path.
>    The early "return -EIO" leaves the port stuck on MSG_PORT_S2D, so all
>    subsequent SMU communication - including the s2idle prepare/restore
>    handlers - is directed at the wrong mailbox.

If you fix the second one first (see below, it seems another place needs 
similar fix), the first one can be fixed on top of it in own patch.

> Consolidate the exit path through a single label so the message port is
> always restored, and propagate the SMU command error directly instead of
> inferring it from the size.
> 
> Assisted-by: Claude:opus
> Reported-by: Francis De Brabandere <francisdb@xxxxxxxxx>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221759
> Tested-by: Francis De Brabandere <francisdb@xxxxxxxxx>
> Fixes: 3d7d407dfb05 ("platform/x86: amd-pmc: Add support for AMD Spill to DRAM STB feature")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
>  drivers/platform/x86/amd/pmc/mp1_stb.c | 28 +++++++++++++++-----------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
> index 753d630f3283d..6a048cb2605ec 100644
> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
> @@ -289,7 +289,7 @@ int amd_stb_s2d_init(struct amd_pmc_dev *dev)
>  	u32 phys_addr_low, phys_addr_hi;
>  	u64 stb_phys_addr;
>  	u32 size = 0;
> -	int ret;
> +	int ret = 0;
>  
>  	if (!enable_stb)
>  		return 0;
> @@ -306,13 +306,17 @@ int amd_stb_s2d_init(struct amd_pmc_dev *dev)
>  	/* Spill to DRAM feature uses separate SMU message port */
>  	dev->msg_port = MSG_PORT_S2D;
>  
> -	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->stb_arg.s2d_msg_id, true);
> -	if (size != S2D_TELEMETRY_BYTES_MAX)
> -		return -EIO;
> +	ret = amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->stb_arg.s2d_msg_id, true);
> +	if (ret)
> +		goto out;
> +	if (size != S2D_TELEMETRY_BYTES_MAX) {
> +		ret = -EIO;
> +		goto out;
> +	}
>  
> -	/* Get DRAM size */
> -	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->stb_arg.s2d_msg_id, true);
> -	if (ret || !dev->dram_size)
> +	/* Get DRAM size; fall back to the default if the query fails */
> +	if (amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->stb_arg.s2d_msg_id, true) ||
> +	    !dev->dram_size)
>  		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
>  
>  	/* Get STB DRAM address */
> @@ -321,12 +325,12 @@ int amd_stb_s2d_init(struct amd_pmc_dev *dev)
>  
>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>  
> -	/* Clear msg_port for other SMU operation */
> -	dev->msg_port = MSG_PORT_PMC;
> -
>  	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
>  	if (!dev->stb_virt_addr)
> -		return -ENOMEM;
> +		ret = -ENOMEM;
>  
> -	return 0;
> +out:
> +	/* Restore the default message port for subsequent SMU operations */
> +	dev->msg_port = MSG_PORT_PMC;

Sashiko (correctly?) points out amd_stb_debugfs_open_v2() has the same 
problem of leaving from msg_port.

To me it looks like having ->msg_port in dev is design error and it should 
be a parameter to the relevant functions instead or with some wrapping 
given by those functions that actually need to give the non-default port.

-- 
 i.