diff mbox series

[v2] firmware/psci: demote suspend-mode warning to info level

Message ID 20221026135445.8004-1-johan+linaro@kernel.org
State New
Headers
Series [v2] firmware/psci: demote suspend-mode warning to info level |

Commit Message

Johan Hovold Oct. 26, 2022, 1:54 p.m. UTC 
  On some Qualcomm platforms, like SC8280XP, the attempt to set PC mode
during boot fails with PSCI_RET_DENIED and since commit 998fcd001feb
("firmware/psci: Print a warning if PSCI doesn't accept PC mode") this
is now logged at warning level:

	psci: failed to set PC mode: -3

As there is nothing users can do about the firmware behaving this way,
demote the warning to info level and clearly mark it as a firmware bug:

	psci: [Firmware Bug]: failed to set PC mode: -3

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/firmware/psci/psci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Ulf Hansson Oct. 27, 2022, 10:59 a.m. UTC | #1 
On Wed, 26 Oct 2022 at 15:57, Johan Hovold <johan+linaro@kernel.org> wrote:
>
> On some Qualcomm platforms, like SC8280XP, the attempt to set PC mode
> during boot fails with PSCI_RET_DENIED and since commit 998fcd001feb
> ("firmware/psci: Print a warning if PSCI doesn't accept PC mode") this
> is now logged at warning level:
>
>         psci: failed to set PC mode: -3
>
> As there is nothing users can do about the firmware behaving this way,
> demote the warning to info level and clearly mark it as a firmware bug:
>
>         psci: [Firmware Bug]: failed to set PC mode: -3
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Using the info level seems like a good compromise to me! So,

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/firmware/psci/psci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index e7bcfca4159f..f8fa32f0a130 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -165,7 +165,8 @@ int psci_set_osi_mode(bool enable)
>
>         err = invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE, suspend_mode, 0, 0);
>         if (err < 0)
> -               pr_warn("failed to set %s mode: %d\n", enable ? "OSI" : "PC", err);
> +               pr_info(FW_BUG "failed to set %s mode: %d\n",
> +                               enable ? "OSI" : "PC", err);
>         return psci_to_linux_errno(err);
>  }
>
> --
> 2.37.3
>
Mark Rutland Oct. 27, 2022, 12:15 p.m. UTC | #2 
On Wed, Oct 26, 2022 at 03:54:45PM +0200, Johan Hovold wrote:
> On some Qualcomm platforms, like SC8280XP, the attempt to set PC mode
> during boot fails with PSCI_RET_DENIED and since commit 998fcd001feb
> ("firmware/psci: Print a warning if PSCI doesn't accept PC mode") this
> is now logged at warning level:
> 
> 	psci: failed to set PC mode: -3
> 
> As there is nothing users can do about the firmware behaving this way,
> demote the warning to info level and clearly mark it as a firmware bug:
> 
> 	psci: [Firmware Bug]: failed to set PC mode: -3
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

On the assumption that we don't have any latent issues in this case, this looks
ok to me, so:

  Acked-by: Mark Rutland <mark.rutland@arm.com>

Sudeep, does this reasonable to you?

Are there any latent issues that mean we should keep this as a pr_warn()? 

Thanks,
Mark.

> ---
>  drivers/firmware/psci/psci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index e7bcfca4159f..f8fa32f0a130 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -165,7 +165,8 @@ int psci_set_osi_mode(bool enable)
>  
>  	err = invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE, suspend_mode, 0, 0);
>  	if (err < 0)
> -		pr_warn("failed to set %s mode: %d\n", enable ? "OSI" : "PC", err);
> +		pr_info(FW_BUG "failed to set %s mode: %d\n",
> +				enable ? "OSI" : "PC", err);
>  	return psci_to_linux_errno(err);
>  }
>  
> -- 
> 2.37.3
>
Sudeep Holla Oct. 27, 2022, 12:58 p.m. UTC | #3 
On Thu, Oct 27, 2022 at 01:15:59PM +0100, Mark Rutland wrote:
> On Wed, Oct 26, 2022 at 03:54:45PM +0200, Johan Hovold wrote:
> > On some Qualcomm platforms, like SC8280XP, the attempt to set PC mode
> > during boot fails with PSCI_RET_DENIED and since commit 998fcd001feb
> > ("firmware/psci: Print a warning if PSCI doesn't accept PC mode") this
> > is now logged at warning level:
> > 
> > 	psci: failed to set PC mode: -3
> > 
> > As there is nothing users can do about the firmware behaving this way,
> > demote the warning to info level and clearly mark it as a firmware bug:
> > 
> > 	psci: [Firmware Bug]: failed to set PC mode: -3
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> On the assumption that we don't have any latent issues in this case, this looks
> ok to me, so:
> 
>   Acked-by: Mark Rutland <mark.rutland@arm.com>
> 
> Sudeep, does this reasonable to you?
> 
> Are there any latent issues that mean we should keep this as a pr_warn()?

I am fine removing it as warning but making it debug may mask the issue
completely on the platforms that are using Linux itself for validation of
their PSCI firmware implementation. This sounds like a good compromise
instead of jumping from warning directly to debug. I just want to give a
chance for platforms noticing this and working on getting their firmware
fixed.

So for this version:

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep
Johan Hovold Nov. 30, 2022, 6:53 a.m. UTC | #4 
On Wed, Oct 26, 2022 at 03:54:45PM +0200, Johan Hovold wrote:
> On some Qualcomm platforms, like SC8280XP, the attempt to set PC mode
> during boot fails with PSCI_RET_DENIED and since commit 998fcd001feb
> ("firmware/psci: Print a warning if PSCI doesn't accept PC mode") this
> is now logged at warning level:
> 
> 	psci: failed to set PC mode: -3
> 
> As there is nothing users can do about the firmware behaving this way,
> demote the warning to info level and clearly mark it as a firmware bug:
> 
> 	psci: [Firmware Bug]: failed to set PC mode: -3
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/firmware/psci/psci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index e7bcfca4159f..f8fa32f0a130 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -165,7 +165,8 @@ int psci_set_osi_mode(bool enable)
>  
>  	err = invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE, suspend_mode, 0, 0);
>  	if (err < 0)
> -		pr_warn("failed to set %s mode: %d\n", enable ? "OSI" : "PC", err);
> +		pr_info(FW_BUG "failed to set %s mode: %d\n",
> +				enable ? "OSI" : "PC", err);
>  	return psci_to_linux_errno(err);
>  }

Mark and Lorenzo, I noticed this one hasn't been picked up yet. Is that
something you will do or is Arnd supposed to take it?

Johan
diff mbox series

Patch

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index e7bcfca4159f..f8fa32f0a130 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -165,7 +165,8 @@  int psci_set_osi_mode(bool enable)
 
 	err = invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE, suspend_mode, 0, 0);
 	if (err < 0)
-		pr_warn("failed to set %s mode: %d\n", enable ? "OSI" : "PC", err);
+		pr_info(FW_BUG "failed to set %s mode: %d\n",
+				enable ? "OSI" : "PC", err);
 	return psci_to_linux_errno(err);
 }