Add proper error handling on failure to enumerate clocks features or
rates.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
drivers/firmware/arm_scmi/clock.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index c9b62edce4fd..bf956305a8fe 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -402,10 +402,16 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(attributes))
clk->rate_change_requested_notifications = true;
if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) {
- if (SUPPORTS_PARENT_CLOCK(attributes))
- scmi_clock_possible_parents(ph, clk_id, cinfo);
- if (SUPPORTS_GET_PERMISSIONS(attributes))
- scmi_clock_get_permissions(ph, clk_id, clk);
+ if (SUPPORTS_PARENT_CLOCK(attributes)) {
+ ret = scmi_clock_possible_parents(ph, clk_id, cinfo);
+ if (ret)
+ return ret;
+ }
+ if (SUPPORTS_GET_PERMISSIONS(attributes)) {
+ ret = scmi_clock_get_permissions(ph, clk_id, clk);
+ if (ret)
+ return ret;
+ }
if (SUPPORTS_EXTENDED_CONFIG(attributes))
clk->extended_config = true;
}
@@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
cinfo->clkds[clkid].id = clkid;
ret = scmi_clock_attributes_get(ph, clkid, cinfo);
- if (!ret)
- scmi_clock_describe_rates_get(ph, clkid, cinfo);
+ if (ret)
+ return ret;
+
+ ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
+ if (ret)
+ return ret;
}
if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) {
--
2.53.0On 10.03.2026 19:40, Cristian Marussi wrote:
> Add proper error handling on failure to enumerate clocks features or
> rates.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
This patch landed yesterday in linux-next as commit 0d8b0c8068a8
("firmware: arm_scmi: Harden clock protocol initialization"). In my
tests I found that it causes a regression on RK3568 Odroid-M1 board
(arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts), cpufreq and GPU
device are not probed properly:
# dmesg | grep scmi
scmi_core: SCMI protocol bus registered
arm-scmi arm-scmi.0.auto: Using scmi_smc_transport
arm-scmi arm-scmi.0.auto: SCMI max-rx-timeout: 30ms / max-msg-size:
104bytes / max-msg: 20
scmi_protocol scmi_dev.1: Enabled polling mode TX channel - prot_id:16
arm-scmi arm-scmi.0.auto: SCMI Notifications - Core Enabled.
arm-scmi arm-scmi.0.auto: Malformed reply - real_sz:8 calc_sz:4
(loop_num_ret:1)
arm-scmi arm-scmi.0.auto: SCMI Protocol v2.0 'rockchip:' Firmware
version 0x0
arm-scmi arm-scmi.0.auto: Enabling SCMI Quirk
[quirk_clock_rates_triplet_out_of_spec]
scmi-clocks scmi_dev.3: probe with driver scmi-clocks failed with error -22
# cat /sys/kernel/debug/devices_deferred
fde60000.gpu
cpufreq-dt
# dmesg | grep fde60000.gpu
rockchip-pm-domain fdd90000.power-management:power-controller:
sync_state() pending due to fde60000.gpu
panfrost fde60000.gpu: get clock failed -517
panfrost fde60000.gpu: clk init failed -517
panfrost fde60000.gpu: get clock failed -517
panfrost fde60000.gpu: clk init failed -517
...
> ---
> drivers/firmware/arm_scmi/clock.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index c9b62edce4fd..bf956305a8fe 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -402,10 +402,16 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
> SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(attributes))
> clk->rate_change_requested_notifications = true;
> if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) {
> - if (SUPPORTS_PARENT_CLOCK(attributes))
> - scmi_clock_possible_parents(ph, clk_id, cinfo);
> - if (SUPPORTS_GET_PERMISSIONS(attributes))
> - scmi_clock_get_permissions(ph, clk_id, clk);
> + if (SUPPORTS_PARENT_CLOCK(attributes)) {
> + ret = scmi_clock_possible_parents(ph, clk_id, cinfo);
> + if (ret)
> + return ret;
> + }
> + if (SUPPORTS_GET_PERMISSIONS(attributes)) {
> + ret = scmi_clock_get_permissions(ph, clk_id, clk);
> + if (ret)
> + return ret;
> + }
> if (SUPPORTS_EXTENDED_CONFIG(attributes))
> clk->extended_config = true;
> }
> @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> cinfo->clkds[clkid].id = clkid;
> ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> - if (!ret)
> - scmi_clock_describe_rates_get(ph, clkid, cinfo);
> + if (ret)
> + return ret;
> +
> + ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> + if (ret)
> + return ret;
> }
>
> if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) {
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Wed, Mar 25, 2026 at 12:02:41PM +0100, Marek Szyprowski wrote:
> On 10.03.2026 19:40, Cristian Marussi wrote:
> > Add proper error handling on failure to enumerate clocks features or
> > rates.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>
Hi Marek,
> This patch landed yesterday in linux-next as commit 0d8b0c8068a8
> ("firmware: arm_scmi: Harden clock protocol initialization"). In my
> tests I found that it causes a regression on RK3568 Odroid-M1 board
> (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts), cpufreq and GPU
> device are not probed properly:
>
> # dmesg | grep scmi
> scmi_core: SCMI protocol bus registered
> arm-scmi arm-scmi.0.auto: Using scmi_smc_transport
> arm-scmi arm-scmi.0.auto: SCMI max-rx-timeout: 30ms / max-msg-size:
> 104bytes / max-msg: 20
> scmi_protocol scmi_dev.1: Enabled polling mode TX channel - prot_id:16
> arm-scmi arm-scmi.0.auto: SCMI Notifications - Core Enabled.
> arm-scmi arm-scmi.0.auto: Malformed reply - real_sz:8 calc_sz:4
> (loop_num_ret:1)
> arm-scmi arm-scmi.0.auto: SCMI Protocol v2.0 'rockchip:' Firmware
> version 0x0
> arm-scmi arm-scmi.0.auto: Enabling SCMI Quirk
> [quirk_clock_rates_triplet_out_of_spec]
> scmi-clocks scmi_dev.3: probe with driver scmi-clocks failed with error -22
>
Yes there are multiple reports of issues on this hardening, the series
is on hold and wont go into v7.1 as of now...it needs some basic fixes
and various quirks probably to address non-compliant firmwares...
It will be pushed to next again with a few more fixes in the coming
days and then we'll need to figure out how many quirks will be needed on
top of that and if it is acceptable at all...
Thanks for the report,
Cristian
Hi,
Am Mittwoch, 25. März 2026, 13:27:48 CET schrieb Cristian Marussi:
> On Wed, Mar 25, 2026 at 12:02:41PM +0100, Marek Szyprowski wrote:
> > On 10.03.2026 19:40, Cristian Marussi wrote:
> > > Add proper error handling on failure to enumerate clocks features or
> > > rates.
> > >
> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >
>
> Hi Marek,
>
> > This patch landed yesterday in linux-next as commit 0d8b0c8068a8
> > ("firmware: arm_scmi: Harden clock protocol initialization"). In my
> > tests I found that it causes a regression on RK3568 Odroid-M1 board
> > (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts), cpufreq and GPU
> > device are not probed properly:
> >
> > # dmesg | grep scmi
> > scmi_core: SCMI protocol bus registered
> > arm-scmi arm-scmi.0.auto: Using scmi_smc_transport
> > arm-scmi arm-scmi.0.auto: SCMI max-rx-timeout: 30ms / max-msg-size:
> > 104bytes / max-msg: 20
> > scmi_protocol scmi_dev.1: Enabled polling mode TX channel - prot_id:16
> > arm-scmi arm-scmi.0.auto: SCMI Notifications - Core Enabled.
> > arm-scmi arm-scmi.0.auto: Malformed reply - real_sz:8 calc_sz:4
> > (loop_num_ret:1)
> > arm-scmi arm-scmi.0.auto: SCMI Protocol v2.0 'rockchip:' Firmware
> > version 0x0
> > arm-scmi arm-scmi.0.auto: Enabling SCMI Quirk
> > [quirk_clock_rates_triplet_out_of_spec]
> > scmi-clocks scmi_dev.3: probe with driver scmi-clocks failed with error -22
> >
>
> Yes there are multiple reports of issues on this hardening, the series
> is on hold and wont go into v7.1 as of now...it needs some basic fixes
> and various quirks probably to address non-compliant firmwares...
>
> It will be pushed to next again with a few more fixes in the coming
> days and then we'll need to figure out how many quirks will be needed on
> top of that and if it is acceptable at all...
Just for the records: imx95 (maybe imx94 as well) is also affected by this.
My board doesn't boot at all, because all the clocks are provided by SCMI.
With this diff I can see it's the 'ext' clock
-->8---
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -1253,8 +1253,11 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
cinfo->clkds[clkid].id = clkid;
ret = scmi_clock_attributes_get(ph, clkid, cinfo);
- if (ret)
+ if (ret) {
+ dev_warn(ph->dev, "scmi_clock_attributes_get failed for '%s': %d\n",
+ cinfo->clkds->info.name, ret);
return ret;
+ }
ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
if (ret)
-->8---
> arm-scmi arm-scmi.0.auto: scmi_clock_attributes_get failed for 'ext': -2
> scmi-clocks scmi_dev.6: probe with driver scmi-clocks failed with error -2
What's the idea of how to proceeed as apparently several platforms are
affected?
Best regards,
Alexander
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/On Thu, Mar 26, 2026 at 09:55:18AM +0100, Alexander Stein wrote:
> Hi,
>
Hi,
> Am Mittwoch, 25. März 2026, 13:27:48 CET schrieb Cristian Marussi:
> > On Wed, Mar 25, 2026 at 12:02:41PM +0100, Marek Szyprowski wrote:
> > > On 10.03.2026 19:40, Cristian Marussi wrote:
> > > > Add proper error handling on failure to enumerate clocks features or
> > > > rates.
> > > >
> > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > >
> >
> > Hi Marek,
> >
> > > This patch landed yesterday in linux-next as commit 0d8b0c8068a8
> > > ("firmware: arm_scmi: Harden clock protocol initialization"). In my
> > > tests I found that it causes a regression on RK3568 Odroid-M1 board
> > > (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts), cpufreq and GPU
> > > device are not probed properly:
> > >
> > > # dmesg | grep scmi
> > > scmi_core: SCMI protocol bus registered
> > > arm-scmi arm-scmi.0.auto: Using scmi_smc_transport
> > > arm-scmi arm-scmi.0.auto: SCMI max-rx-timeout: 30ms / max-msg-size:
> > > 104bytes / max-msg: 20
> > > scmi_protocol scmi_dev.1: Enabled polling mode TX channel - prot_id:16
> > > arm-scmi arm-scmi.0.auto: SCMI Notifications - Core Enabled.
> > > arm-scmi arm-scmi.0.auto: Malformed reply - real_sz:8 calc_sz:4
> > > (loop_num_ret:1)
> > > arm-scmi arm-scmi.0.auto: SCMI Protocol v2.0 'rockchip:' Firmware
> > > version 0x0
> > > arm-scmi arm-scmi.0.auto: Enabling SCMI Quirk
> > > [quirk_clock_rates_triplet_out_of_spec]
> > > scmi-clocks scmi_dev.3: probe with driver scmi-clocks failed with error -22
> > >
> >
> > Yes there are multiple reports of issues on this hardening, the series
> > is on hold and wont go into v7.1 as of now...it needs some basic fixes
> > and various quirks probably to address non-compliant firmwares...
> >
> > It will be pushed to next again with a few more fixes in the coming
> > days and then we'll need to figure out how many quirks will be needed on
> > top of that and if it is acceptable at all...
>
> Just for the records: imx95 (maybe imx94 as well) is also affected by this.
> My board doesn't boot at all, because all the clocks are provided by SCMI.
>
Sorry for the late reply, thanks for the report...
> With this diff I can see it's the 'ext' clock
> -->8---
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -1253,8 +1253,11 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> cinfo->clkds[clkid].id = clkid;
> ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> - if (ret)
> + if (ret) {
> + dev_warn(ph->dev, "scmi_clock_attributes_get failed for '%s': %d\n",
> + cinfo->clkds->info.name, ret);
> return ret;
> + }
>
> ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> if (ret)
> -->8---
> > arm-scmi arm-scmi.0.auto: scmi_clock_attributes_get failed for 'ext': -2
> > scmi-clocks scmi_dev.6: probe with driver scmi-clocks failed with error -2
>
> What's the idea of how to proceeed as apparently several platforms are
> affected?
>
The series is on hold of course due to some residual bugs and all of
these reports of misbehaving firmwares...as I was saying elsewhere we
dont want of course to break existing boards in the wild that will most
probably never get a FW fix, BUT at the same time we do NOT want to
legalise/normalize this out of spec behaviour by leaving the kernel
code as it is...I mean at least we'd like to try to discourage this
mis-implementations in the future FWs ...
At the end, this could mean some quirks applied to multiple platforms
and vendors and maybe some relaxation in the checks, certainly some noisy
annoying logs on the side :P
Thanks,
Cristian
On Thu, Mar 26, 2026 at 09:55:18AM +0100, Alexander Stein wrote:
> Hi,
>
> Am Mittwoch, 25. März 2026, 13:27:48 CET schrieb Cristian Marussi:
> > On Wed, Mar 25, 2026 at 12:02:41PM +0100, Marek Szyprowski wrote:
> > > On 10.03.2026 19:40, Cristian Marussi wrote:
> > > > Add proper error handling on failure to enumerate clocks features or
> > > > rates.
> > > >
> > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > >
> >
> > Hi Marek,
> >
> > > This patch landed yesterday in linux-next as commit 0d8b0c8068a8
> > > ("firmware: arm_scmi: Harden clock protocol initialization"). In my
> > > tests I found that it causes a regression on RK3568 Odroid-M1 board
> > > (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts), cpufreq and GPU
> > > device are not probed properly:
> > >
> > > # dmesg | grep scmi
> > > scmi_core: SCMI protocol bus registered
> > > arm-scmi arm-scmi.0.auto: Using scmi_smc_transport
> > > arm-scmi arm-scmi.0.auto: SCMI max-rx-timeout: 30ms / max-msg-size:
> > > 104bytes / max-msg: 20
> > > scmi_protocol scmi_dev.1: Enabled polling mode TX channel - prot_id:16
> > > arm-scmi arm-scmi.0.auto: SCMI Notifications - Core Enabled.
> > > arm-scmi arm-scmi.0.auto: Malformed reply - real_sz:8 calc_sz:4
> > > (loop_num_ret:1)
> > > arm-scmi arm-scmi.0.auto: SCMI Protocol v2.0 'rockchip:' Firmware
> > > version 0x0
> > > arm-scmi arm-scmi.0.auto: Enabling SCMI Quirk
> > > [quirk_clock_rates_triplet_out_of_spec]
> > > scmi-clocks scmi_dev.3: probe with driver scmi-clocks failed with error -22
> > >
> >
> > Yes there are multiple reports of issues on this hardening, the series
> > is on hold and wont go into v7.1 as of now...it needs some basic fixes
> > and various quirks probably to address non-compliant firmwares...
> >
> > It will be pushed to next again with a few more fixes in the coming
> > days and then we'll need to figure out how many quirks will be needed on
> > top of that and if it is acceptable at all...
>
> Just for the records: imx95 (maybe imx94 as well) is also affected by this.
> My board doesn't boot at all, because all the clocks are provided by SCMI.
>
> With this diff I can see it's the 'ext' clock
> -->8---
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -1253,8 +1253,11 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> cinfo->clkds[clkid].id = clkid;
> ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> - if (ret)
> + if (ret) {
> + dev_warn(ph->dev, "scmi_clock_attributes_get failed for '%s': %d\n",
> + cinfo->clkds->info.name, ret);
> return ret;
> + }
>
> ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> if (ret)
> -->8---
> > arm-scmi arm-scmi.0.auto: scmi_clock_attributes_get failed for 'ext': -2
> > scmi-clocks scmi_dev.6: probe with driver scmi-clocks failed with error -2
>
> What's the idea of how to proceeed as apparently several platforms are
> affected?
>
Not exactly answer to the above question, but more discussion here:
https://lore.kernel.org/all/20260324-scmi-clock-fix-v1-v1-1-65c21935824b@nxp.com
--
Regards,
Sudeep
Hi Cristian,
On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> Add proper error handling on failure to enumerate clocks features or
> rates.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
Thanks for your patch!
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> cinfo->clkds[clkid].id = clkid;
> ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> - if (!ret)
> - scmi_clock_describe_rates_get(ph, clkid, cinfo);
> + if (ret)
> + return ret;
This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
do not support the SCMI CLOCK_ATTRIBUTES command.
Before, these clocks were still instantiated, but were further unusable.
After, the whole clock driver fails to initialize, and no SCMI clocks
are available at all.
> +
> + ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> + if (ret)
> + return ret;
> }
>
> if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) {
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
>
> On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > Add proper error handling on failure to enumerate clocks features or
> > rates.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
Hi,
>
> Thanks for your patch!
>
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
>
> > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > cinfo->clkds[clkid].id = clkid;
> > ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > - if (!ret)
> > - scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > + if (ret)
> > + return ret;
>
> This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> do not support the SCMI CLOCK_ATTRIBUTES command.
> Before, these clocks were still instantiated, but were further unusable.
> After, the whole clock driver fails to initialize, and no SCMI clocks
> are available at all.
...and this is exactly what I feared while doing this sort of hardening :P
So there are a few possible solutions (beside reverting this straight away)
The easy fix would be instead change the above in a
if (ret)
continue;
...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
FW releases to fix this :D
Another option could be leave it as it is, since indeed it is the correct enforced
behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
quirk targeting the affected FW releases...
This latter option, though, while enforcing the correct behaviour AND
fixing your R-Car issue, leaves open the door for a number of possible
failures of other unknowingly buggy Vendors similarly deployed firmwares...
...that could be solved with more quirks of course...but...worth it ?
Thoughts ?
Let's see also what @Sudeep thinks about this...
Thanks for testing !
Cristian
On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:
> On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> > Hi Cristian,
> >
> > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > Add proper error handling on failure to enumerate clocks features or
> > > rates.
> > >
> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>
> Hi,
>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> >
> > > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > > cinfo->clkds[clkid].id = clkid;
> > > ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > - if (!ret)
> > > - scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > + if (ret)
> > > + return ret;
> >
> > This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> > do not support the SCMI CLOCK_ATTRIBUTES command.
> > Before, these clocks were still instantiated, but were further unusable.
> > After, the whole clock driver fails to initialize, and no SCMI clocks
> > are available at all.
>
> ...and this is exactly what I feared while doing this sort of hardening :P
>
> So there are a few possible solutions (beside reverting this straight away)
>
> The easy fix would be instead change the above in a
>
> if (ret)
> continue;
>
> ...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
> FW releases to fix this :D
>
> Another option could be leave it as it is, since indeed it is the correct enforced
> behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
> quirk targeting the affected FW releases...
>
> This latter option, though, while enforcing the correct behaviour AND
> fixing your R-Car issue, leaves open the door for a number of possible
> failures of other unknowingly buggy Vendors similarly deployed firmwares...
>
> ...that could be solved with more quirks of course...but...worth it ?
>
> Thoughts ?
>
> Let's see also what @Sudeep thinks about this...
>
I prefer to fix it as a quirk to prevent similar issues on newer platforms if
the firmware baselines are derived from it. In the worst case, we can relax
the hardening until we figure out a proper quirk-based solution.
--
Regards,
Sudeep
On Thu, Mar 12, 2026 at 03:33:52PM +0000, Sudeep Holla wrote:
> On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:
> > On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> > > Hi Cristian,
> > >
> > > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > Add proper error handling on failure to enumerate clocks features or
> > > > rates.
> > > >
> > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >
> > Hi,
> >
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/firmware/arm_scmi/clock.c
> > > > +++ b/drivers/firmware/arm_scmi/clock.c
> > >
> > > > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > > for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > > > cinfo->clkds[clkid].id = clkid;
> > > > ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > > - if (!ret)
> > > > - scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > > + if (ret)
> > > > + return ret;
> > >
> > > This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> > > do not support the SCMI CLOCK_ATTRIBUTES command.
> > > Before, these clocks were still instantiated, but were further unusable.
> > > After, the whole clock driver fails to initialize, and no SCMI clocks
> > > are available at all.
> >
> > ...and this is exactly what I feared while doing this sort of hardening :P
> >
> > So there are a few possible solutions (beside reverting this straight away)
> >
> > The easy fix would be instead change the above in a
> >
> > if (ret)
> > continue;
> >
> > ...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
> > FW releases to fix this :D
> >
> > Another option could be leave it as it is, since indeed it is the correct enforced
> > behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
> > quirk targeting the affected FW releases...
> >
> > This latter option, though, while enforcing the correct behaviour AND
> > fixing your R-Car issue, leaves open the door for a number of possible
> > failures of other unknowingly buggy Vendors similarly deployed firmwares...
> >
> > ...that could be solved with more quirks of course...but...worth it ?
> >
> > Thoughts ?
> >
> > Let's see also what @Sudeep thinks about this...
> >
>
> I prefer to fix it as a quirk to prevent similar issues on newer platforms if
> the firmware baselines are derived from it. In the worst case, we can relax
> the hardening until we figure out a proper quirk-based solution.
Ok, I can post a V3 with a dummy quirk 'template' RFC to be filled by
Geert with proper versioning....so I can check that there are no
surprises round the (quirked) corner...
Thanks,
Cristian
Hi Cristian,
On Thu, 12 Mar 2026 at 17:36, Cristian Marussi <cristian.marussi@arm.com> wrote:
> On Thu, Mar 12, 2026 at 03:33:52PM +0000, Sudeep Holla wrote:
> > On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:
> > > On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> > > > Hi Cristian,
> > > >
> > > > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > > Add proper error handling on failure to enumerate clocks features or
> > > > > rates.
> > > > >
> > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > >
> > > > > --- a/drivers/firmware/arm_scmi/clock.c
> > > > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > >
> > > > > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > > > for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > > > > cinfo->clkds[clkid].id = clkid;
> > > > > ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > > > - if (!ret)
> > > > > - scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > > > + if (ret)
> > > > > + return ret;
> > > >
> > > > This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> > > > do not support the SCMI CLOCK_ATTRIBUTES command.
Apparently it is not just CLOCK_ATTRIBUTES, but also
CLOCK_DESCRIBE_RATES.
> > > > Before, these clocks were still instantiated, but were further unusable.
> > > > After, the whole clock driver fails to initialize, and no SCMI clocks
> > > > are available at all.
> > >
> > > ...and this is exactly what I feared while doing this sort of hardening :P
> > >
> > > So there are a few possible solutions (beside reverting this straight away)
> > >
> > > The easy fix would be instead change the above in a
> > >
> > > if (ret)
> > > continue;
> > >
> > > ...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
> > > FW releases to fix this :D
> > >
> > > Another option could be leave it as it is, since indeed it is the correct enforced
> > > behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
> > > quirk targeting the affected FW releases...
> > >
> > > This latter option, though, while enforcing the correct behaviour AND
> > > fixing your R-Car issue, leaves open the door for a number of possible
> > > failures of other unknowingly buggy Vendors similarly deployed firmwares...
> > >
> > > ...that could be solved with more quirks of course...but...worth it ?
> > >
> > > Thoughts ?
> > >
> > > Let's see also what @Sudeep thinks about this...
> >
> > I prefer to fix it as a quirk to prevent similar issues on newer platforms if
> > the firmware baselines are derived from it. In the worst case, we can relax
> > the hardening until we figure out a proper quirk-based solution.
>
> Ok, I can post a V3 with a dummy quirk 'template' RFC to be filled by
> Geert with proper versioning....so I can check that there are no
> surprises round the (quirked) corner...
Unfortunately you cannot "continue" from a quirk, without resorting
to a goto, so I sent a fix: "[PATCH] firmware: arm_scmi: Support loop
control in quirk code snippets"[1].
Then I came up with the following preliminary (have to check more
firmware versions) quirk (Gmail whitespace-damaged):
diff --git a/drivers/firmware/arm_scmi/clock.c
b/drivers/firmware/arm_scmi/clock.c
index f62f9492bd42afbc..6f2af6e9084836c6 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -1230,6 +1230,18 @@ static const struct scmi_protocol_events
clk_protocol_events = {
.num_events = ARRAY_SIZE(clk_events),
};
+#define QUIRK_RCAR_X5H_NO_ATTRIBUTES \
+ ({ \
+ if (ret == -EREMOTEIO || ret == -EOPNOTSUPP) \
+ continue; \
+ })
+
+#define QUIRK_RCAR_X5H_NO_RATES
\
+ ({ \
+ if (ret == -EOPNOTSUPP) \
+ ret = 0; \
+ })
+
static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
{
int clkid, ret;
@@ -1254,10 +1266,12 @@ static int scmi_clock_protocol_init(const
struct scmi_protocol_handle *ph)
for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
cinfo->clkds[clkid].id = clkid;
ret = scmi_clock_attributes_get(ph, clkid, cinfo);
+ SCMI_QUIRK(clock_rcar_x5h_no_attributes,
QUIRK_RCAR_X5H_NO_ATTRIBUTES);
if (ret)
return ret;
ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
+ SCMI_QUIRK(clock_rcar_x5h_no_attributes,
QUIRK_RCAR_X5H_NO_RATES);
if (ret)
return ret;
}
diff --git a/drivers/firmware/arm_scmi/quirks.c
b/drivers/firmware/arm_scmi/quirks.c
index 3772139a758c8a78..5a69f119e1b6c806 100644
--- a/drivers/firmware/arm_scmi/quirks.c
+++ b/drivers/firmware/arm_scmi/quirks.c
@@ -172,6 +172,8 @@ struct scmi_quirk {
/* Global Quirks Definitions */
DEFINE_SCMI_QUIRK(clock_rates_triplet_out_of_spec, NULL, NULL, NULL);
DEFINE_SCMI_QUIRK(perf_level_get_fc_force, "Qualcomm", NULL, "0x20000-");
+DEFINE_SCMI_QUIRK(clock_rcar_x5h_no_attributes, "Renesas", NULL, "0x10a0000",
+ "renesas,r8a78000");
/*
* Quirks Pointers Array
@@ -182,6 +184,7 @@ DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
"Qualcomm", NULL, "0x20000-");
static struct scmi_quirk *scmi_quirks_table[] = {
__DECLARE_SCMI_QUIRK_ENTRY(clock_rates_triplet_out_of_spec),
__DECLARE_SCMI_QUIRK_ENTRY(perf_level_get_fc_force),
+ __DECLARE_SCMI_QUIRK_ENTRY(clock_rcar_x5h_no_attributes),
NULL
};
diff --git a/drivers/firmware/arm_scmi/quirks.h
b/drivers/firmware/arm_scmi/quirks.h
index 74bf6406dd043049..13f28d13bbd74d4c 100644
--- a/drivers/firmware/arm_scmi/quirks.h
+++ b/drivers/firmware/arm_scmi/quirks.h
@@ -48,5 +48,6 @@ static inline void scmi_quirks_enable(struct device
*dev, const char *vend,
/* Quirk delarations */
DECLARE_SCMI_QUIRK(clock_rates_triplet_out_of_spec);
DECLARE_SCMI_QUIRK(perf_level_get_fc_force);
+DECLARE_SCMI_QUIRK(clock_rcar_x5h_no_attributes);
#endif /* _SCMI_QUIRKS_H */
Does that look like what you have in mind?
Thanks!
[1] https://lore.kernel.org/51de914cddef8fa86c2e7dd5397e5df759c45464.1773675224.git.geert+renesas@glider.be/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Mon, Mar 16, 2026 at 04:50:17PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
>
> On Thu, 12 Mar 2026 at 17:36, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > On Thu, Mar 12, 2026 at 03:33:52PM +0000, Sudeep Holla wrote:
> > > On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:
> > > > On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> > > > > Hi Cristian,
> > > > >
> > > > > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > > > Add proper error handling on failure to enumerate clocks features or
> > > > > > rates.
> > > > > >
> > > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
Hi Geert,
> > > >
> > > > > > --- a/drivers/firmware/arm_scmi/clock.c
> > > > > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > > >
> > > > > > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > > > > for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > > > > > cinfo->clkds[clkid].id = clkid;
> > > > > > ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > > > > - if (!ret)
> > > > > > - scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > >
> > > > > This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> > > > > do not support the SCMI CLOCK_ATTRIBUTES command.
>
> Apparently it is not just CLOCK_ATTRIBUTES, but also
> CLOCK_DESCRIBE_RATES.
>
I was indeed suspicious that I could have opened a can of worms by
more strictly enfrocing these...
> > > > > Before, these clocks were still instantiated, but were further unusable.
> > > > > After, the whole clock driver fails to initialize, and no SCMI clocks
> > > > > are available at all.
> > > >
> > > > ...and this is exactly what I feared while doing this sort of hardening :P
> > > >
> > > > So there are a few possible solutions (beside reverting this straight away)
> > > >
> > > > The easy fix would be instead change the above in a
> > > >
> > > > if (ret)
> > > > continue;
> > > >
> > > > ...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
> > > > FW releases to fix this :D
> > > >
> > > > Another option could be leave it as it is, since indeed it is the correct enforced
> > > > behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
> > > > quirk targeting the affected FW releases...
> > > >
> > > > This latter option, though, while enforcing the correct behaviour AND
> > > > fixing your R-Car issue, leaves open the door for a number of possible
> > > > failures of other unknowingly buggy Vendors similarly deployed firmwares...
> > > >
> > > > ...that could be solved with more quirks of course...but...worth it ?
> > > >
> > > > Thoughts ?
> > > >
> > > > Let's see also what @Sudeep thinks about this...
> > >
> > > I prefer to fix it as a quirk to prevent similar issues on newer platforms if
> > > the firmware baselines are derived from it. In the worst case, we can relax
> > > the hardening until we figure out a proper quirk-based solution.
> >
> > Ok, I can post a V3 with a dummy quirk 'template' RFC to be filled by
> > Geert with proper versioning....so I can check that there are no
> > surprises round the (quirked) corner...
>
> Unfortunately you cannot "continue" from a quirk, without resorting
> to a goto, so I sent a fix: "[PATCH] firmware: arm_scmi: Support loop
> control in quirk code snippets"[1].
>
Yes ... just realized that this afternoon when trying to draft a
quirk... (see other thread)
> Then I came up with the following preliminary (have to check more
> firmware versions) quirk (Gmail whitespace-damaged):
>
> diff --git a/drivers/firmware/arm_scmi/clock.c
> b/drivers/firmware/arm_scmi/clock.c
> index f62f9492bd42afbc..6f2af6e9084836c6 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -1230,6 +1230,18 @@ static const struct scmi_protocol_events
> clk_protocol_events = {
> .num_events = ARRAY_SIZE(clk_events),
> };
>
> +#define QUIRK_RCAR_X5H_NO_ATTRIBUTES \
> + ({ \
> + if (ret == -EREMOTEIO || ret == -EOPNOTSUPP) \
> + continue; \
> + })
> +
> +#define QUIRK_RCAR_X5H_NO_RATES
> \
> + ({ \
> + if (ret == -EOPNOTSUPP) \
> + ret = 0; \
> + })
> +
> static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> {
> int clkid, ret;
> @@ -1254,10 +1266,12 @@ static int scmi_clock_protocol_init(const
> struct scmi_protocol_handle *ph)
> for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> cinfo->clkds[clkid].id = clkid;
> ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> + SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> QUIRK_RCAR_X5H_NO_ATTRIBUTES);
> if (ret)
> return ret;
>
> ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> + SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> QUIRK_RCAR_X5H_NO_RATES);
> if (ret)
> return ret;
> }
> diff --git a/drivers/firmware/arm_scmi/quirks.c
> b/drivers/firmware/arm_scmi/quirks.c
> index 3772139a758c8a78..5a69f119e1b6c806 100644
> --- a/drivers/firmware/arm_scmi/quirks.c
> +++ b/drivers/firmware/arm_scmi/quirks.c
> @@ -172,6 +172,8 @@ struct scmi_quirk {
> /* Global Quirks Definitions */
> DEFINE_SCMI_QUIRK(clock_rates_triplet_out_of_spec, NULL, NULL, NULL);
> DEFINE_SCMI_QUIRK(perf_level_get_fc_force, "Qualcomm", NULL, "0x20000-");
> +DEFINE_SCMI_QUIRK(clock_rcar_x5h_no_attributes, "Renesas", NULL, "0x10a0000",
> + "renesas,r8a78000");
>
> /*
> * Quirks Pointers Array
> @@ -182,6 +184,7 @@ DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
> "Qualcomm", NULL, "0x20000-");
> static struct scmi_quirk *scmi_quirks_table[] = {
> __DECLARE_SCMI_QUIRK_ENTRY(clock_rates_triplet_out_of_spec),
> __DECLARE_SCMI_QUIRK_ENTRY(perf_level_get_fc_force),
> + __DECLARE_SCMI_QUIRK_ENTRY(clock_rcar_x5h_no_attributes),
> NULL
> };
>
> diff --git a/drivers/firmware/arm_scmi/quirks.h
> b/drivers/firmware/arm_scmi/quirks.h
> index 74bf6406dd043049..13f28d13bbd74d4c 100644
> --- a/drivers/firmware/arm_scmi/quirks.h
> +++ b/drivers/firmware/arm_scmi/quirks.h
> @@ -48,5 +48,6 @@ static inline void scmi_quirks_enable(struct device
> *dev, const char *vend,
> /* Quirk delarations */
> DECLARE_SCMI_QUIRK(clock_rates_triplet_out_of_spec);
> DECLARE_SCMI_QUIRK(perf_level_get_fc_force);
> +DECLARE_SCMI_QUIRK(clock_rcar_x5h_no_attributes);
>
> #endif /* _SCMI_QUIRKS_H */
>
> Does that look like what you have in mind?
> Thanks!
Yes in quirk I was only addressing NOT_ATTRIBUTES and mimicing the old
behaviour with continue, BUT if the set of clocks not supporting attributes
and the set of clocks not suppporting rates is disjoint, I feel we need your
double quirks :P
If you are still finding out the exact FW versions that are failing maybe
it is better if you carry on and test the quirk-framework fix above together
with your quirks and we can make sure to pick all up together...
...OR maybe better I can also drop for now my offending patch that breaks
your FW from my V3 series and you can pick it up and post it later with
your quirks and the Quirk framework fix you propsoed so that we are sure
that we dont break anything while fixing all of this...
Also because we are already in V4 and I dont want to risk to post the
breaking fix (that was at the end broke since forever) BUT not the quirks...
Let's see what @Sudeep thinks
Thanks,
Cristian
Hi Cristian,
On Mon, 16 Mar 2026 at 17:14, Cristian Marussi <cristian.marussi@arm.com> wrote:
> On Mon, Mar 16, 2026 at 04:50:17PM +0100, Geert Uytterhoeven wrote:
> > On Thu, 12 Mar 2026 at 17:36, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > On Thu, Mar 12, 2026 at 03:33:52PM +0000, Sudeep Holla wrote:
> > > > On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:
> > > > > On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> > > > > > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > > > > Add proper error handling on failure to enumerate clocks features or
> > > > > > > rates.
> > > > > > >
> > > > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > > > > > > --- a/drivers/firmware/arm_scmi/clock.c
> > > > > > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > > > >
> > > > > > > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > > > > > for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > > > > > > cinfo->clkds[clkid].id = clkid;
> > > > > > > ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > > > > > - if (!ret)
> > > > > > > - scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > > > > > + if (ret)
> > > > > > > + return ret;
> > > > > >
> > > > > > This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> > > > > > do not support the SCMI CLOCK_ATTRIBUTES command.
> >
> > Apparently it is not just CLOCK_ATTRIBUTES, but also
> > CLOCK_DESCRIBE_RATES.
>
> I was indeed suspicious that I could have opened a can of worms by
> more strictly enfrocing these...
>
> > > > > > Before, these clocks were still instantiated, but were further unusable.
> > > > > > After, the whole clock driver fails to initialize, and no SCMI clocks
> > > > > > are available at all.
> > > > >
> > > > > ...and this is exactly what I feared while doing this sort of hardening :P
> > > > >
> > > > > So there are a few possible solutions (beside reverting this straight away)
> > > > >
> > > > > The easy fix would be instead change the above in a
> > > > >
> > > > > if (ret)
> > > > > continue;
> > > > >
> > > > > ...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
> > > > > FW releases to fix this :D
> > > > >
> > > > > Another option could be leave it as it is, since indeed it is the correct enforced
> > > > > behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
> > > > > quirk targeting the affected FW releases...
> > > > >
> > > > > This latter option, though, while enforcing the correct behaviour AND
> > > > > fixing your R-Car issue, leaves open the door for a number of possible
> > > > > failures of other unknowingly buggy Vendors similarly deployed firmwares...
> > > > >
> > > > > ...that could be solved with more quirks of course...but...worth it ?
> > > > >
> > > > > Thoughts ?
> > > > >
> > > > > Let's see also what @Sudeep thinks about this...
> > > >
> > > > I prefer to fix it as a quirk to prevent similar issues on newer platforms if
> > > > the firmware baselines are derived from it. In the worst case, we can relax
> > > > the hardening until we figure out a proper quirk-based solution.
> > >
> > > Ok, I can post a V3 with a dummy quirk 'template' RFC to be filled by
> > > Geert with proper versioning....so I can check that there are no
> > > surprises round the (quirked) corner...
> >
> > Unfortunately you cannot "continue" from a quirk, without resorting
> > to a goto, so I sent a fix: "[PATCH] firmware: arm_scmi: Support loop
> > control in quirk code snippets"[1].
>
> Yes ... just realized that this afternoon when trying to draft a
> quirk... (see other thread)
>
> > Then I came up with the following preliminary (have to check more
> > firmware versions) quirk (Gmail whitespace-damaged):
> >
> > diff --git a/drivers/firmware/arm_scmi/clock.c
> > b/drivers/firmware/arm_scmi/clock.c
> > index f62f9492bd42afbc..6f2af6e9084836c6 100644
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -1230,6 +1230,18 @@ static const struct scmi_protocol_events
> > clk_protocol_events = {
> > .num_events = ARRAY_SIZE(clk_events),
> > };
> >
> > +#define QUIRK_RCAR_X5H_NO_ATTRIBUTES \
> > + ({ \
> > + if (ret == -EREMOTEIO || ret == -EOPNOTSUPP) \
> > + continue; \
> > + })
> > +
> > +#define QUIRK_RCAR_X5H_NO_RATES
> > \
> > + ({ \
> > + if (ret == -EOPNOTSUPP) \
> > + ret = 0; \
> > + })
> > +
> > static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > {
> > int clkid, ret;
> > @@ -1254,10 +1266,12 @@ static int scmi_clock_protocol_init(const
> > struct scmi_protocol_handle *ph)
> > for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > cinfo->clkds[clkid].id = clkid;
> > ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > + SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > QUIRK_RCAR_X5H_NO_ATTRIBUTES);
> > if (ret)
> > return ret;
> >
> > ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > + SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > QUIRK_RCAR_X5H_NO_RATES);
> > if (ret)
> > return ret;
> > }
> > Does that look like what you have in mind?
> > Thanks!
>
> Yes in quirk I was only addressing NOT_ATTRIBUTES and mimicing the old
> behaviour with continue, BUT if the set of clocks not supporting attributes
> and the set of clocks not suppporting rates is disjoint, I feel we need your
> double quirks :P
I could have used
SCMI_QUIRK(clock_rcar_x5h_no_attributes, QUIRK_RCAR_X5H_NO_ATTRIBUTES);
after both scmi_clock_attributes_get() and
scmi_clock_describe_rates_get(), but I wanted to keep the check as
strict as possible: the former returns two error codes to ignore,
the latter only one.
> If you are still finding out the exact FW versions that are failing maybe
> it is better if you carry on and test the quirk-framework fix above together
> with your quirks and we can make sure to pick all up together...
It is not urgent, as R-Car X5H SCMI support is not yet upstream.
> ...OR maybe better I can also drop for now my offending patch that breaks
> your FW from my V3 series and you can pick it up and post it later with
> your quirks and the Quirk framework fix you propsoed so that we are sure
> that we dont break anything while fixing all of this...
While there is indeed a chance that this hardening regresses on
platforms that are already upstream...
> Also because we are already in V4 and I dont want to risk to post the
> breaking fix (that was at the end broke since forever) BUT not the quirks...
s/in V4/at rc4/?
> Let's see what @Sudeep thinks
OK.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Cristian,
On Mon, 16 Mar 2026 at 17:35, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Mon, 16 Mar 2026 at 17:14, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > On Mon, Mar 16, 2026 at 04:50:17PM +0100, Geert Uytterhoeven wrote:
> > > Then I came up with the following preliminary (have to check more
> > > firmware versions) quirk (Gmail whitespace-damaged):
> > >
> > > diff --git a/drivers/firmware/arm_scmi/clock.c
> > > b/drivers/firmware/arm_scmi/clock.c
> > > index f62f9492bd42afbc..6f2af6e9084836c6 100644
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > @@ -1230,6 +1230,18 @@ static const struct scmi_protocol_events
> > > clk_protocol_events = {
> > > .num_events = ARRAY_SIZE(clk_events),
> > > };
> > >
> > > +#define QUIRK_RCAR_X5H_NO_ATTRIBUTES \
> > > + ({ \
> > > + if (ret == -EREMOTEIO || ret == -EOPNOTSUPP) \
> > > + continue; \
> > > + })
> > > +
> > > +#define QUIRK_RCAR_X5H_NO_RATES
> > > \
> > > + ({ \
> > > + if (ret == -EOPNOTSUPP) \
> > > + ret = 0; \
> > > + })
> > > +
> > > static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > {
> > > int clkid, ret;
> > > @@ -1254,10 +1266,12 @@ static int scmi_clock_protocol_init(const
> > > struct scmi_protocol_handle *ph)
> > > for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > > cinfo->clkds[clkid].id = clkid;
> > > ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > + SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > > QUIRK_RCAR_X5H_NO_ATTRIBUTES);
> > > if (ret)
> > > return ret;
> > >
> > > ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > + SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > > QUIRK_RCAR_X5H_NO_RATES);
> > > if (ret)
> > > return ret;
> > > }
>
> > > Does that look like what you have in mind?
> > > Thanks!
> >
> > Yes in quirk I was only addressing NOT_ATTRIBUTES and mimicing the old
> > behaviour with continue, BUT if the set of clocks not supporting attributes
> > and the set of clocks not suppporting rates is disjoint, I feel we need your
> > double quirks :P
>
> I could have used
>
> SCMI_QUIRK(clock_rcar_x5h_no_attributes, QUIRK_RCAR_X5H_NO_ATTRIBUTES);
>
> after both scmi_clock_attributes_get() and
> scmi_clock_describe_rates_get(), but I wanted to keep the check as
> strict as possible: the former returns two error codes to ignore,
> the latter only one.
So these are two mitigations:
#define QUIRK_RCAR_X5H_NO_ATTRIBUTES ({ ... })
SCMI_QUIRK(clock_rcar_x5h_no_attributes, QUIRK_RCAR_X5H_NO_ATTRIBUTES);
and
#define QUIRK_RCAR_X5H_NO_RATES ({ ... })
SCMI_QUIRK(clock_rcar_x5h_no_attributes, QUIRK_RCAR_X5H_NO_RATES);
gated by a single quirk entry clock_rcar_x5h_no_attributes:
DECLARE_SCMI_QUIRK(clock_rcar_x5h_no_attributes);
DEFINE_SCMI_QUIRK(clock_rcar_x5h_no_attributes, "Renesas", NULL,
"0x10a0000", "renesas,r8a78000");
__DECLARE_SCMI_QUIRK_ENTRY(clock_rcar_x5h_no_attributes),
In general, when a specific SCMI implementation has multiple quirks
and needs multiple mitigations, do you prefer to have individual
entries for each quirk plus mitigation, or just a single entry with
multiple mitigations (which may not be limited to a single protocol,
unlike my example above)?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Mon, Mar 16, 2026 at 05:35:26PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
>
> On Mon, 16 Mar 2026 at 17:14, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > On Mon, Mar 16, 2026 at 04:50:17PM +0100, Geert Uytterhoeven wrote:
> > > On Thu, 12 Mar 2026 at 17:36, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > On Thu, Mar 12, 2026 at 03:33:52PM +0000, Sudeep Holla wrote:
> > > > > On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:
> > > > > > On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> > > > > > > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > > > > > Add proper error handling on failure to enumerate clocks features or
> > > > > > > > rates.
> > > > > > > >
> > > > > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>
> > > > > > > > --- a/drivers/firmware/arm_scmi/clock.c
> > > > > > > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > > > > >
> > > > > > > > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > > > > > > for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > > > > > > > cinfo->clkds[clkid].id = clkid;
> > > > > > > > ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > > > > > > - if (!ret)
> > > > > > > > - scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > > > > > > + if (ret)
> > > > > > > > + return ret;
> > > > > > >
> > > > > > > This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> > > > > > > do not support the SCMI CLOCK_ATTRIBUTES command.
> > >
> > > Apparently it is not just CLOCK_ATTRIBUTES, but also
> > > CLOCK_DESCRIBE_RATES.
> >
> > I was indeed suspicious that I could have opened a can of worms by
> > more strictly enfrocing these...
> >
> > > > > > > Before, these clocks were still instantiated, but were further unusable.
> > > > > > > After, the whole clock driver fails to initialize, and no SCMI clocks
> > > > > > > are available at all.
> > > > > >
> > > > > > ...and this is exactly what I feared while doing this sort of hardening :P
> > > > > >
> > > > > > So there are a few possible solutions (beside reverting this straight away)
> > > > > >
> > > > > > The easy fix would be instead change the above in a
> > > > > >
> > > > > > if (ret)
> > > > > > continue;
> > > > > >
> > > > > > ...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
> > > > > > FW releases to fix this :D
> > > > > >
> > > > > > Another option could be leave it as it is, since indeed it is the correct enforced
> > > > > > behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
> > > > > > quirk targeting the affected FW releases...
> > > > > >
> > > > > > This latter option, though, while enforcing the correct behaviour AND
> > > > > > fixing your R-Car issue, leaves open the door for a number of possible
> > > > > > failures of other unknowingly buggy Vendors similarly deployed firmwares...
> > > > > >
> > > > > > ...that could be solved with more quirks of course...but...worth it ?
> > > > > >
> > > > > > Thoughts ?
> > > > > >
> > > > > > Let's see also what @Sudeep thinks about this...
> > > > >
> > > > > I prefer to fix it as a quirk to prevent similar issues on newer platforms if
> > > > > the firmware baselines are derived from it. In the worst case, we can relax
> > > > > the hardening until we figure out a proper quirk-based solution.
> > > >
> > > > Ok, I can post a V3 with a dummy quirk 'template' RFC to be filled by
> > > > Geert with proper versioning....so I can check that there are no
> > > > surprises round the (quirked) corner...
> > >
> > > Unfortunately you cannot "continue" from a quirk, without resorting
> > > to a goto, so I sent a fix: "[PATCH] firmware: arm_scmi: Support loop
> > > control in quirk code snippets"[1].
> >
> > Yes ... just realized that this afternoon when trying to draft a
> > quirk... (see other thread)
> >
> > > Then I came up with the following preliminary (have to check more
> > > firmware versions) quirk (Gmail whitespace-damaged):
> > >
> > > diff --git a/drivers/firmware/arm_scmi/clock.c
> > > b/drivers/firmware/arm_scmi/clock.c
> > > index f62f9492bd42afbc..6f2af6e9084836c6 100644
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > @@ -1230,6 +1230,18 @@ static const struct scmi_protocol_events
> > > clk_protocol_events = {
> > > .num_events = ARRAY_SIZE(clk_events),
> > > };
> > >
> > > +#define QUIRK_RCAR_X5H_NO_ATTRIBUTES \
> > > + ({ \
> > > + if (ret == -EREMOTEIO || ret == -EOPNOTSUPP) \
> > > + continue; \
> > > + })
> > > +
> > > +#define QUIRK_RCAR_X5H_NO_RATES
> > > \
> > > + ({ \
> > > + if (ret == -EOPNOTSUPP) \
> > > + ret = 0; \
> > > + })
> > > +
> > > static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > {
> > > int clkid, ret;
> > > @@ -1254,10 +1266,12 @@ static int scmi_clock_protocol_init(const
> > > struct scmi_protocol_handle *ph)
> > > for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > > cinfo->clkds[clkid].id = clkid;
> > > ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > + SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > > QUIRK_RCAR_X5H_NO_ATTRIBUTES);
> > > if (ret)
> > > return ret;
> > >
> > > ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > + SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > > QUIRK_RCAR_X5H_NO_RATES);
> > > if (ret)
> > > return ret;
> > > }
>
> > > Does that look like what you have in mind?
> > > Thanks!
> >
> > Yes in quirk I was only addressing NOT_ATTRIBUTES and mimicing the old
> > behaviour with continue, BUT if the set of clocks not supporting attributes
> > and the set of clocks not suppporting rates is disjoint, I feel we need your
> > double quirks :P
>
> I could have used
>
> SCMI_QUIRK(clock_rcar_x5h_no_attributes, QUIRK_RCAR_X5H_NO_ATTRIBUTES);
>
> after both scmi_clock_attributes_get() and
> scmi_clock_describe_rates_get(), but I wanted to keep the check as
> strict as possible: the former returns two error codes to ignore,
> the latter only one.
>
> > If you are still finding out the exact FW versions that are failing maybe
> > it is better if you carry on and test the quirk-framework fix above together
> > with your quirks and we can make sure to pick all up together...
>
> It is not urgent, as R-Car X5H SCMI support is not yet upstream.
Ah ok.
>
> > ...OR maybe better I can also drop for now my offending patch that breaks
> > your FW from my V3 series and you can pick it up and post it later with
> > your quirks and the Quirk framework fix you propsoed so that we are sure
> > that we dont break anything while fixing all of this...
>
> While there is indeed a chance that this hardening regresses on
> platforms that are already upstream...
Yes indeed...
>
> > Also because we are already in V4 and I dont want to risk to post the
> > breaking fix (that was at the end broke since forever) BUT not the quirks...
>
> s/in V4/at rc4/?
yep... -rc4 I meant..
>
> > Let's see what @Sudeep thinks
>
> OK.
>
Thanks,
Cristian
© 2016 - 2026 Red Hat, Inc.