Re: [RFC PATCH] firmware: arm_scmi: skip empty CLOCK_DESCRIBE_RATES replies

[Jay Buddhabhatti <jbuddhab@amd.com>]

Hi Sudeep,

On 10/5/2026 9:01 PM, Sudeep Holla wrote:
> On Mon, Oct 05, 2026 at 06:06:09PM +0530, Jay Buddhabhatti wrote:
>> Hi Sudeep,
>>
>> Thanks for the review. Please find my response inline.
>>
>> On 10/1/2026 8:14 PM, Sudeep Holla wrote:
>>> On Thu, Oct 01, 2026 at 04:45:38AM -0700, Jay Buddhabhatti wrote:
>>>> Some platforms advertise reserved or uninstantiated clock IDs that still
>>>> succeed CLOCK_DESCRIBE_RATES with zero rates. After dynamic rate
>>>> allocation, kcalloc(0) returns ZERO_SIZE_PTR and protocol init then
>>>> dereferences rates[0], which panics.
>>>>
>>>> Do not allocate or index the rate array when the firmware reports an
>>>> empty list, so unused IDs are skipped instead of taking down the SCMI
>>>> clock provider.
>>>>
>>>> Fixes: 62ba967595e0 ("firmware: arm_scmi: Make clock rates allocation dynamic")
>>>> Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
>>>> ---
>>>> The SCMI server is the source of this zero rate and successful response
>>>> and it should be fixed in SCMI server. This defensive check in Linux is
>>>> still useful because firmware responses must be validated before
>>>> de-referencing dynamically allocated data, The panic is a Linux
>>>> regression introduced by dynamic rate allocation; previous fixed array
>>>> tolerated the same response and other SCMI implementations could return
>>>> the same unexpected response.
>>>> ---
>>>>    drivers/firmware/arm_scmi/clock.c | 18 ++++++++++++++++++
>>>>    1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
>>>> index 0278705d809e..8934a95527e2 100644
>>>> --- a/drivers/firmware/arm_scmi/clock.c
>>>> +++ b/drivers/firmware/arm_scmi/clock.c
>>>> @@ -8,6 +8,7 @@
>>>>    #include <linux/math64.h>
>>>>    #include <linux/module.h>
>>>>    #include <linux/limits.h>
>>>> +#include <linux/slab.h>
>>>>    #include <linux/sort.h>
>>>>    #include "protocols.h"
>>>> @@ -484,6 +485,13 @@ iter_clk_describe_update_state(struct scmi_iterator_state *st,
>>>>    	if (!st->max_resources) {
>>>>    		unsigned int tot_rates = st->num_returned + st->num_remaining;
>>>> +		/*
>>>> +		 * Unused/reserved clock IDs return 0 rates. kmalloc(0)
>>>> +		 * returns ZERO_SIZE_PTR and must not be dereferenced.
>>>> +		 */
>>>> +		if (!tot_rates)
>>>> +			return 0;
>>>> +
>>>>    		p->clkd->r.rates = devm_kcalloc(p->dev, tot_rates,
>>>>    						sizeof(*p->clkd->r.rates), GFP_KERNEL);
>>>>    		if (!p->clkd->r.rates)
>>>> @@ -505,6 +513,9 @@ iter_clk_describe_process_response(const struct scmi_protocol_handle *ph,
>>>>    	struct scmi_clk_ipriv *p = priv;
>>>>    	const struct scmi_msg_resp_clock_describe_rates *r = response;
>>>> +	if (ZERO_OR_NULL_PTR(p->clkd->r.rates))
>>>> +		return -EPROTO;
>>>> +
>>>>    	p->clkd->r.rates[p->clkd->r.num_rates] = RATE_TO_U64(r->rate[st->loop_idx]);
>>>>    	/* Count only effectively discovered rates */
>>>> @@ -622,6 +633,13 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph,
>>>>    	if (ret)
>>>>    		return ret;
>>>> +	/*
>>>> +	 * Some platforms expose reserved clock IDs with an empty
>>>> +	 * CLOCK_DESCRIBE_RATES reply. Do not dereference rates[].
>>>> +	 */
>>>> +	if (!clkd->r.num_rates || ZERO_OR_NULL_PTR(clkd->r.rates))
>>>> +		return 0;
>>>> +
>>>
>>> I expect the Clock rate control bit to be unset in the permissions for
>>> these clock, else it may be dangerous to do this. Please add that check.
>>
>> I will add that check in new version. If the Clock rate control bit is set,
>> Linux clears its local copy by setting rate_ctrl_forbidden. The clock
>> remains registered, but clk-scmi does not install set_rate and
>> scmi_clock_rate_set() returns -EACCES. This avoids exposing rate changes
>> when no valid rates were described.
>>
> 
> You did mention this is issue in the firmware. If the generic solution(once
> agreed upon) doesn't work on your platform, then you need to fix it with
> a quirk I am afraid. I will let you propose the patch and take it from there.

Agreed, Linux should not override the platform permissions. I'll drop
that in v2. If CLOCK_DESCRIBE_RATES returns no rates, v2 accepts the
reply only if:

  - the clock has no name. clk-scmi already skips unnamed clocks via
    scmi_clock_info_get(), so these are never registered, or
  - the platform reports rate control as not allowed. The clock is kept
    for enable/disable and no set_rate is installed.

Otherwise it logs an error and invalidates the clock, so that it is not
registered with an undefined rate range.

The de-reference fixes from v1 stay as they are, since they
avoid the panic in all cases.

I checked our platform SCMI server: all the reserved IDs that return an
empty rate list also return an empty name, so they were never exposed
by clk-scmi. With v2 they are skipped silently, and every clock that is
exposed has a valid rate list, so I don't think a quirk is needed.

Does this sound reasonable to you? If so, I'll post v2.

Thanks,
Jay
>