| Message ID | 1739926689-151827-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | pmdomain: rockchip: Check if smcc could be handled by TA | expand |
Am Mittwoch, 19. Februar 2025, 01:58:09 MEZ schrieb Shawn Lin: > Non-existent trusted-firmware could lead smcc calls into some > unset location which breaks the system. > > Reported-by: Steven Price <steven.price@arm.com> > Cc: Steven Price <steven.price@arm.com> > Suggested-by: Heiko Stuebner <heiko@sntech.de> > Fixes: 58ebba35ddab ("pmdomain: rockchip: Add smc call to inform firmware") > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> Reviewed-by: Heiko Stuebner <heiko@sntech.de> > --- > Hi Ulf, this's a follow-up patch fixing the issue Steven saw. > > drivers/pmdomain/rockchip/pm-domains.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c > index 49842f1..27a5c68 100644 > --- a/drivers/pmdomain/rockchip/pm-domains.c > +++ b/drivers/pmdomain/rockchip/pm-domains.c > @@ -572,9 +572,10 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd, > } > > /* Inform firmware to keep this pd on or off */ > - arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG, > - pmu->info->pwr_offset + pd_pwr_offset, > - pd->info->pwr_mask, on, 0, 0, 0, &res); > + if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_NONE) > + arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG, > + pmu->info->pwr_offset + pd_pwr_offset, > + pd->info->pwr_mask, on, 0, 0, 0, &res); > } > > static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on) >
On 19/02/2025 00:58, Shawn Lin wrote: > Non-existent trusted-firmware could lead smcc calls into some > unset location which breaks the system. > > Reported-by: Steven Price <steven.price@arm.com> > Cc: Steven Price <steven.price@arm.com> > Suggested-by: Heiko Stuebner <heiko@sntech.de> > Fixes: 58ebba35ddab ("pmdomain: rockchip: Add smc call to inform firmware") > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> Tested-by: Steven Price <steven.price@arm.com> Although one note below... > --- > Hi Ulf, this's a follow-up patch fixing the issue Steven saw. > > drivers/pmdomain/rockchip/pm-domains.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c > index 49842f1..27a5c68 100644 > --- a/drivers/pmdomain/rockchip/pm-domains.c > +++ b/drivers/pmdomain/rockchip/pm-domains.c > @@ -572,9 +572,10 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd, > } > > /* Inform firmware to keep this pd on or off */ > - arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG, > - pmu->info->pwr_offset + pd_pwr_offset, > - pd->info->pwr_mask, on, 0, 0, 0, &res); > + if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_NONE) > + arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG, > + pmu->info->pwr_offset + pd_pwr_offset, > + pd->info->pwr_mask, on, 0, 0, 0, &res); Note that if the conduit is SMCCC_CONDUIT_HVC then this will still attempt an SMC. I'm not sure if this situation can happen in practice. There is a (horrifyingly complex) macro arm_smccc_1_1_invoke() which will automatically use the correct conduit, and even copes with the SMCCC_CONDUIT_NONE case (by simply failing the call). Steve > } > > static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
On Wed, Feb 19, 2025 at 08:58:09AM +0800, Shawn Lin wrote: > Non-existent trusted-firmware could lead smcc calls into some > unset location which breaks the system. > s/smcc/SMC or s/smcc/SMCCC please. There is nothing called smcc -- Regards, Sudeep
+ Sudeep On Wed, 19 Feb 2025 at 10:34, Steven Price <steven.price@arm.com> wrote: > > On 19/02/2025 00:58, Shawn Lin wrote: > > Non-existent trusted-firmware could lead smcc calls into some > > unset location which breaks the system. > > > > Reported-by: Steven Price <steven.price@arm.com> > > Cc: Steven Price <steven.price@arm.com> > > Suggested-by: Heiko Stuebner <heiko@sntech.de> > > Fixes: 58ebba35ddab ("pmdomain: rockchip: Add smc call to inform firmware") > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > Tested-by: Steven Price <steven.price@arm.com> > > Although one note below... > > > --- > > Hi Ulf, this's a follow-up patch fixing the issue Steven saw. > > > > drivers/pmdomain/rockchip/pm-domains.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c > > index 49842f1..27a5c68 100644 > > --- a/drivers/pmdomain/rockchip/pm-domains.c > > +++ b/drivers/pmdomain/rockchip/pm-domains.c > > @@ -572,9 +572,10 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd, > > } > > > > /* Inform firmware to keep this pd on or off */ > > - arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG, > > - pmu->info->pwr_offset + pd_pwr_offset, > > - pd->info->pwr_mask, on, 0, 0, 0, &res); > > + if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_NONE) > > + arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG, > > + pmu->info->pwr_offset + pd_pwr_offset, > > + pd->info->pwr_mask, on, 0, 0, 0, &res); > > Note that if the conduit is SMCCC_CONDUIT_HVC then this will still > attempt an SMC. I'm not sure if this situation can happen in practice. > > There is a (horrifyingly complex) macro arm_smccc_1_1_invoke() which > will automatically use the correct conduit, and even copes with the > SMCCC_CONDUIT_NONE case (by simply failing the call). > > Steve Thanks for letting us know! Note that, arm_smccc_1_1_invoke() is also a bit problematic, as it doesn't compile with Clang-20 and Thumb2 mode. See commit 885f5669f2ab. [...] As the current approach seems fine too, I decided to pick up the $patch as is and by updating the commit message, according to Sudeep's comment. Kind regards Uffe
Hi Shawn, kernel test robot noticed the following build errors: [auto build test ERROR on next-20250218] [cannot apply to v6.14-rc3 v6.14-rc2 v6.14-rc1 linus/master v6.14-rc3] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Shawn-Lin/pmdomain-rockchip-Check-if-smcc-could-be-handled-by-TA/20250219-122924 base: next-20250218 patch link: https://lore.kernel.org/r/1739926689-151827-1-git-send-email-shawn.lin%40rock-chips.com patch subject: [PATCH] pmdomain: rockchip: Check if smcc could be handled by TA config: i386-buildonly-randconfig-001-20250220 (https://download.01.org/0day-ci/archive/20250220/202502201601.rQYwZmA8-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250220/202502201601.rQYwZmA8-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202502201601.rQYwZmA8-lkp@intel.com/ All errors (new ones prefixed by >>): ld: drivers/pmdomain/rockchip/pm-domains.o: in function `rockchip_do_pmu_set_power_domain': >> drivers/pmdomain/rockchip/pm-domains.c:575: undefined reference to `arm_smccc_1_1_get_conduit' vim +575 drivers/pmdomain/rockchip/pm-domains.c 537 538 static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd, 539 bool on) 540 { 541 struct rockchip_pmu *pmu = pd->pmu; 542 struct generic_pm_domain *genpd = &pd->genpd; 543 u32 pd_pwr_offset = pd->info->pwr_offset; 544 bool is_on, is_mem_on = false; 545 struct arm_smccc_res res; 546 547 if (pd->info->pwr_mask == 0) 548 return; 549 550 if (on && pd->info->mem_status_mask) 551 is_mem_on = rockchip_pmu_domain_is_mem_on(pd); 552 553 if (pd->info->pwr_w_mask) 554 regmap_write(pmu->regmap, pmu->info->pwr_offset + pd_pwr_offset, 555 on ? pd->info->pwr_w_mask : 556 (pd->info->pwr_mask | pd->info->pwr_w_mask)); 557 else 558 regmap_update_bits(pmu->regmap, pmu->info->pwr_offset + pd_pwr_offset, 559 pd->info->pwr_mask, on ? 0 : -1U); 560 561 wmb(); 562 563 if (is_mem_on && rockchip_pmu_domain_mem_reset(pd)) 564 return; 565 566 if (readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on, 567 is_on == on, 0, 10000)) { 568 dev_err(pmu->dev, 569 "failed to set domain '%s', val=%d\n", 570 genpd->name, is_on); 571 return; 572 } 573 574 /* Inform firmware to keep this pd on or off */ > 575 if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_NONE) 576 arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG, 577 pmu->info->pwr_offset + pd_pwr_offset, 578 pd->info->pwr_mask, on, 0, 0, 0, &res); 579 } 580
Hi Shawn,
kernel test robot noticed the following build errors:
[auto build test ERROR on next-20250218]
[cannot apply to v6.14-rc3 v6.14-rc2 v6.14-rc1 linus/master v6.14-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Shawn-Lin/pmdomain-rockchip-Check-if-smcc-could-be-handled-by-TA/20250219-122924
base: next-20250218
patch link: https://lore.kernel.org/r/1739926689-151827-1-git-send-email-shawn.lin%40rock-chips.com
patch subject: [PATCH] pmdomain: rockchip: Check if smcc could be handled by TA
config: x86_64-buildonly-randconfig-005-20250220 (https://download.01.org/0day-ci/archive/20250220/202502201459.wqX8VWY9-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250220/202502201459.wqX8VWY9-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502201459.wqX8VWY9-lkp@intel.com/
All errors (new ones prefixed by >>):
ld: drivers/pmdomain/rockchip/pm-domains.o: in function `rockchip_pd_power':
>> pm-domains.c:(.text+0x3bad): undefined reference to `arm_smccc_1_1_get_conduit'
diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c index 49842f1..27a5c68 100644 --- a/drivers/pmdomain/rockchip/pm-domains.c +++ b/drivers/pmdomain/rockchip/pm-domains.c @@ -572,9 +572,10 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd, } /* Inform firmware to keep this pd on or off */ - arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG, - pmu->info->pwr_offset + pd_pwr_offset, - pd->info->pwr_mask, on, 0, 0, 0, &res); + if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_NONE) + arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG, + pmu->info->pwr_offset + pd_pwr_offset, + pd->info->pwr_mask, on, 0, 0, 0, &res); } static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
Non-existent trusted-firmware could lead smcc calls into some unset location which breaks the system. Reported-by: Steven Price <steven.price@arm.com> Cc: Steven Price <steven.price@arm.com> Suggested-by: Heiko Stuebner <heiko@sntech.de> Fixes: 58ebba35ddab ("pmdomain: rockchip: Add smc call to inform firmware") Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- Hi Ulf, this's a follow-up patch fixing the issue Steven saw. drivers/pmdomain/rockchip/pm-domains.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)