Re: [PATCH v10 6/6] clk: scmi: Add i.MX95 OEM extension support for SCMI clock driver

Hi Stephen,

On Fri, Aug 14, 2026 at 09:17:48AM -0700, Stephen Boyd wrote:
>+Brian and Peng
>
>It doesn't link for allmodconfig build

My bad.

I just posted v11 to address the allmodconfig build issue, very minor changes:
-obj-$(CONFIG_COMMON_CLK_SCMI)           += clk-scmi.o
+scmi-clk-y                             := clk-scmi.o clk-scmi-oem.o
+obj-$(CONFIG_COMMON_CLK_SCMI)           += scmi-clk.o

no more changes in v11.

Thanks,
Peng

>
>Quoting [email protected] (2026-06-12 01:58:12)
>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>> 
>> New issues:
>> - [High] The driver fails to link when compiled as a module (`CONFIG_COMMON_CLK_SCMI=m`) because `clk-scmi.c` and `clk-scmi-oem.c` are built as separate modules without exporting their shared symbols.
>> - [High] Incorrect unit conversion for spread spectrum percentage causes valid spread values to be truncated to 0.
>> 
>> Pre-existing issues:
>> - [High] Missing bounds check on SCMI firmware-provided parent clock index leads to heap out-of-bounds read and wild pointer dereference.
>> --
>> 
>> commit 02412e4d55042034939e6eece844d1bd4d62d5e9
>> Author: Peng Fan <[email protected]>
>> 
>> clk: scmi: Add i.MX95 OEM extension support for SCMI clock driver
>> 
>> Introduces clk-scmi-oem.c to support vendor-specific OEM extensions for the
>> SCMI clock driver. This enables querying OEM-specific features and setting
>> spread spectrum configurations.
>> 
>> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> > index 6a726331b6c9e..c2ae700ec0f2a 100644
>> > --- a/drivers/clk/Makefile
>> > +++ b/drivers/clk/Makefile
>> > @@ -98,7 +98,7 @@ obj-$(CONFIG_COMMON_CLK_RP1)            += clk-rp1.o
>> >  obj-$(CONFIG_COMMON_CLK_RPMI)                += clk-rpmi.o
>> >  obj-$(CONFIG_COMMON_CLK_HI655X)              += clk-hi655x.o
>> >  obj-$(CONFIG_COMMON_CLK_S2MPS11)     += clk-s2mps11.o
>> > -obj-$(CONFIG_COMMON_CLK_SCMI)           += clk-scmi.o
>> > +obj-$(CONFIG_COMMON_CLK_SCMI)           += clk-scmi.o clk-scmi-oem.o
>> 
>> [Severity: High]
>> Will this fail to link when compiled as a module (CONFIG_COMMON_CLK_SCMI=m)?
>> 
>> It looks like Kbuild will treat clk-scmi.o and clk-scmi-oem.o as separate
>> kernel modules rather than linking them into a single composite module.
>> Since neither file exports their shared symbols (like scmi_proto_clk_ops
>> or scmi_clk_oem_init), won't this result in undefined reference errors?
>> 
>> Should this use a composite module definition instead?
>> 
>> scmi-clk-y := clk-scmi.o clk-scmi-oem.o
>> obj-$(CONFIG_COMMON_CLK_SCMI) += scmi-clk.o
>> 
>> [ ... ]
>> 
>> > diff --git a/drivers/clk/clk-scmi-oem.c b/drivers/clk/clk-scmi-oem.c
>> > new file mode 100644
>> > index 0000000000000..be11d359b4ec3
>> > --- /dev/null
>> > +++ b/drivers/clk/clk-scmi-oem.c
>> 
>> [ ... ]
>> 
>> > +static int
>> > +scmi_clk_imx_set_spread_spectrum(struct clk_hw *hw,
>> > +                              const struct clk_spread_spectrum *ss_conf)
>> > +{
>> > +     struct scmi_clk *clk = to_scmi_clk(hw);
>> > +     int ret;
>> > +     u32 val;
>> > +
>> > +     /*
>> > +      * extConfigValue[7:0]   - spread percentage (%)
>> > +      * extConfigValue[23:8]  - Modulation Frequency
>> > +      * extConfigValue[24]    - Enable/Disable
>> > +      * extConfigValue[31:25] - Reserved
>> > +      */
>> > +     val = FIELD_PREP(SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK, ss_conf->spread_bp / 10000);
>> 
>> [Severity: High]
>> Does this integer division by 10000 inadvertently truncate valid spread
>> spectrum values to 0?
>> 
>> Since ss_conf->spread_bp is in basis points (where 100 basis points equals
>> 1%), and standard configurations usually range from 50 to 500 basis points
>> (0.5% to 5%), dividing by 10000 will always result in zero. Could this
>> divide by 100 instead to correctly convert basis points to a percentage?
>> 
>> [ ... ]
>> 
>> > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
>> > index d88e78cc9a12e..2dd50c5b4ea8f 100644
>> > --- a/drivers/clk/clk-scmi.c
>> > +++ b/drivers/clk/clk-scmi.c
>> 
>> [ ... ]
>> 
>> > @@ -406,7 +419,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
>> >                * to avoid sharing the devm_ allocated clk_ops between multiple
>> >                * SCMI clk driver instances.
>> >                */
>> > -             scmi_ops = scmi_clk_ops_select(sclk, transport_is_atomic,
>> > +             scmi_ops = scmi_clk_ops_select(sdev, sclk, transport_is_atomic,
>> >                                              atomic_threshold_us,
>> >                                              scmi_clk_ops_db,
>> >                                              ARRAY_SIZE(scmi_clk_ops_db));
>> 
>> [Severity: High]
>> This is a pre-existing issue, but is it possible for the SCMI firmware to
>> trigger an out-of-bounds read right after this ops setup?
>> 
>> Looking at scmi_clocks_probe(), it uses the parent index provided by the
>> SCMI firmware to directly access the dynamically allocated hws array:
>> 
>> drivers/clk/clk-scmi.c:scmi_clocks_probe() {
>>     ...
>>         for (int i = 0; i < sclk->info->num_parents; i++) {
>>                 sclk->parent_data[i].index = sclk->info->parents[i];
>>                 sclk->parent_data[i].hw = hws[sclk->info->parents[i]];
>>     ...
>> }
>> 
>> If the firmware provides an index greater than or equal to the total clock
>> count, won't this result in an out-of-bounds read on the heap, potentially
>> dereferencing a wild pointer? Should this index be verified against count
>> first?
>> 
>> -- 
>> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
>
>