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

+Brian and Peng

It doesn't link for allmodconfig build

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