On Thu, Jun 04, 2026 at 02:08:50PM +0000, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 6 potential issue(s) > to consider: > > New issues: > - [High] The value written to `RK_MMU_AUTO_GATING` sets bit 1 instead of > clearing it, contradicting the inline comment and hardware workaround logic. > - [High] The error handling path for `noc_init()` failure leaks the IOMMU > group reference and leaves the device in an unbalanced PM runtime state. > - [High] A malformed device tree with excessive NPU core nodes triggers an > out-of-bounds write in `rocket_probe()` array access. > - [High] The NOC bus de-idle sequence via the PMU is missing from the runtime > PM resume path. > - [Medium] The `rockchip,pmu` property is functionally mandatory for RK3568 > but is not added to the schema's conditional `required` list. > > Pre-existing issues: > - [High] The clock names in the `core->clks` array are uninitialized before > being passed to `devm_clk_bulk_get()`, causing the framework to fetch the > wrong clocks. > -- > > commit 6c61da961bed3360a99f625d4a3b1599e7bfe685 > Author: Midgy BALON <[email protected]> > > dt-bindings: npu: rockchip, rk3588-rknn-core: Add RK3568 > > The RK3568 carries a single core of the same NVDLA-derived NPU IP as the > RK3588. Add its compatible. On RK3568 the NPU NOC bus-idle and power > gating are controlled through the system PMU rather than a dedicated > register block. > > > diff --git > > a/Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml > > b/Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml > > index caca2a4903cd1..af9936b32e9fe 100644 > > --- a/Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml > > +++ b/Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml > [ ... ] > > @@ -75,7 +83,15 @@ required: > > - resets > > - reset-names > > - npu-supply > > - - sram-supply > > + > > +if: > > + properties: > > + compatible: > > + contains: > > + const: rockchip,rk3588-rknn-core > > +then: > > + required: > > + - sram-supply > > [Severity: Medium] > Does the missing rockchip,pmu property from the conditional required block > cause incomplete device trees to pass validation? > > The rockchip,pmu property is functionally mandatory for the > rockchip,rk3568-rknn-core variant as the driver unconditionally fails to probe > without it. Missing this mandatory property here could allow invalid device > trees to go undetected.
This looks valid. Additionally, the condition should forbid this
property on rk3588.
pw-bot: changes-requested
>
> > additionalProperties: false
> >
> > diff --git a/drivers/accel/rocket/rocket_core.c
> > b/drivers/accel/rocket/rocket_core.c
Why is the bot commenting on driver stuff here?
Cheers,
Conor.
> > index b3b2fa9ba645a..634f78dfe2887 100644
> > --- a/drivers/accel/rocket/rocket_core.c
> > +++ b/drivers/accel/rocket/rocket_core.c
> [ ... ]
> > @@ -83,6 +89,15 @@ int rocket_core_init(struct rocket_core *core)
> > return err;
> > }
> >
> > + if (core->soc_data->noc_init) {
> > + err = core->soc_data->noc_init(core);
> > + if (err) {
> > + pm_runtime_put_sync(dev);
> > + rocket_job_fini(core);
> > + return err;
> > + }
> > + }
>
> [Severity: High]
> Does this error path leak the IOMMU group reference and leave the device in an
> unbalanced PM runtime state?
>
> By returning directly after pm_runtime_put_sync() and rocket_job_fini(), the
> code misses calling rocket_core_fini() which handles iommu_group_put(),
> pm_runtime_disable(), and pm_runtime_dont_use_autosuspend().
>
> > +
> > version = rocket_pc_readl(core, VERSION);
> > version += rocket_pc_readl(core, VERSION_NUM) & 0xffff;
> >
> > diff --git a/drivers/accel/rocket/rocket_device.c
> > b/drivers/accel/rocket/rocket_device.c
> > index 46e6ee1e72c5f..6186f4faa3a2a 100644
> > --- a/drivers/accel/rocket/rocket_device.c
> > +++ b/drivers/accel/rocket/rocket_device.c
> [ ... ]
> > @@ -27,17 +25,14 @@ struct rocket_device *rocket_device_init(struct
> > platform_device *pdev,
> > ddev = &rdev->ddev;
> > dev_set_drvdata(dev, rdev);
> >
> > - for_each_compatible_node(core_node, NULL, "rockchip,rk3588-rknn-core")
> > - if (of_device_is_available(core_node))
> > - num_cores++;
> > -
> > - rdev->cores = devm_kcalloc(dev, num_cores, sizeof(*rdev->cores),
> > GFP_KERNEL);
> > + rdev->cores = devm_kcalloc(dev, soc_data->num_cores,
> > sizeof(*rdev->cores),
> > + GFP_KERNEL);
>
> [Severity: High]
> Can this code overflow rdev->cores[] if the device tree contains more NPU
> nodes than soc_data->num_cores?
>
> With this change, the array size is fixed to soc_data->num_cores, but
> rocket_probe() in drivers/accel/rocket/rocket_drv.c is still called for every
> matching node. If there are too many nodes in the device tree, it appears
> rocket_probe() will write out of bounds here:
>
> drivers/accel/rocket/rocket_drv.c:rocket_probe() {
> ...
> unsigned int core = rdev->num_cores;
> ...
> rdev->cores[core].rdev = rdev;
> rdev->cores[core].dev = &pdev->dev;
> rdev->cores[core].index = core;
> ...
> }
>
> > if (!rdev->cores)
> > return ERR_PTR(-ENOMEM);
> >
> > diff --git a/drivers/accel/rocket/rocket_drv.c
> > b/drivers/accel/rocket/rocket_drv.c
> > index 8bbbce594883e..5a72d0b5f4dff 100644
> > --- a/drivers/accel/rocket/rocket_drv.c
> > +++ b/drivers/accel/rocket/rocket_drv.c
> [ ... ]
> > @@ -213,8 +219,64 @@ static void rocket_remove(struct platform_device *pdev)
> > }
> > }
> >
> > +/*
> > + * On RK3568 the NPU NOC bus is gated and idle out of reset and must be
> > + * powered on and de-idled through the PMU before the NPU is reachable.
> > PMU
> > + * registers use a write-mask protocol: the upper 16 bits enable writes to
> > the
> > + * matching lower 16 bits.
> > + *
> > + * The NPU's high-speed clock is a PVTPLL managed by TF-A via SCMI and
> > must be
> > + * running before the NOC acknowledges the de-idle request. Force a real
> > SCMI
> > + * rate change (an intermediate rate defeats the clock framework's
> > + * unchanged-rate shortcut) now that the power domain is on and clocks
> > enabled.
> > + */
> > +#define ROCKET_RK3568_SCMI_CLK 2
> > +
> > +static int rk3568_noc_init(struct rocket_core *core)
> > +{
> > + struct regmap *pmu;
> > + unsigned int val;
> > + int ret;
> > +
> > + clk_set_rate(core->clks[ROCKET_RK3568_SCMI_CLK].clk, 600000000UL);
> > + clk_set_rate(core->clks[ROCKET_RK3568_SCMI_CLK].clk, 1000000000UL);
>
> [Severity: High]
> This is a pre-existing issue, but does this unintentionally alter the system
> aclk rate?
>
> Looking at rocket_core_init(), the core->clks array is passed to
> devm_clk_bulk_get() without initializing the id strings:
>
> drivers/accel/rocket/rocket_core.c:rocket_core_init() {
> ...
> err = devm_clk_bulk_get(dev, ARRAY_SIZE(core->clks), core->clks);
> ...
> }
>
> Because the id fields are NULL, the framework fetches the first DT clock
> (aclk) for all elements. Calling clk_set_rate() here seems to alter the
> system aclk frequency instead of the intended SCMI clock.
>
> > +
> > + pmu = syscon_regmap_lookup_by_phandle(core->dev->of_node,
> > "rockchip,pmu");
> > + if (IS_ERR(pmu))
> > + return dev_err_probe(core->dev, PTR_ERR(pmu),
> > + "failed to get PMU regmap\n");
> > +
> > + /* Power on the NPU power domain (PWR_GATE_SFTCON bit 1 = 0). */
> > + regmap_write(pmu, 0xa0, BIT(1 + 16));
> > +
> > + /* Disable NPU NOC auto-idle (NOC_AUTO_CON0 bit 2). */
> > + regmap_write(pmu, 0x70, BIT(2 + 16));
> > +
> > + /* Request NPU bus de-idle (BUS_IDLE_SFTCON0 bit 2 = 0). */
> > + regmap_write(pmu, 0x50, BIT(2 + 16));
> > +
> > + /* Wait for the bus to report active (BUS_IDLE_ST bit 2 = 0). */
> > + ret = regmap_read_poll_timeout(pmu, 0x68, val, !(val & BIT(2)), 10,
> > 1000);
> > + if (ret)
> > + dev_err(core->dev, "timed out waiting for NPU bus de-idle\n");
> > +
> > + return ret;
> > +}
>
> [Severity: High]
> Is a NOC bus de-idle sequence required in the runtime PM resume path as well?
>
> If the generic power domain (genpd) powers off the NPU during runtime suspend,
> the NOC bus idle state would reset. Currently, rocket_device_runtime_resume()
> enables clocks but doesn't repeat this PMU de-idle sequence, which could leave
> the AXI bus gated and cause subsequent hardware accesses to hang or crash.
>
> > +
> > static const struct rocket_soc_data rk3568_soc_data = {
> > .num_cores = 1,
> > .dma_bits = 32,
> > .noc_init = rk3568_noc_init,
> > };
> >
> > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> > index 0013cf196c573..7d40ecb26123f 100644
> > --- a/drivers/iommu/rockchip-iommu.c
> > +++ b/drivers/iommu/rockchip-iommu.c
> > @@ -952,6 +952,18 @@ static int rk_iommu_enable(struct rk_iommu *iommu)
> >
> > ret = rk_iommu_enable_paging(iommu);
> >
> > + if (!ret) {
> > + /*
> > + * RK356x v1 IOMMU: RK_MMU_AUTO_GATING bit 1 enables page-walker
> > + * auto clock-gating; the walker's AXI master then loses its
> > clock
> > + * between transactions and a TLB-miss page walk never
> > completes,
> > + * leaving the IOMMU stuck (PAGING_ENABLED, never IDLE). Clear
> > + * bit 1 (keep bit 0, the slave-port gate) once paging is
> > enabled.
> > + */
> > + for (i = 0; i < iommu->num_mmu; i++)
> > + rk_iommu_write(iommu->bases[i], RK_MMU_AUTO_GATING,
> > 0x2);
>
> [Severity: High]
> Will this write operation inadvertently set bit 1 instead of clearing it?
>
> Writing 0x2 (binary 10) sets bit 1 and clears bit 0, which contradicts the
> inline comment indicating that bit 1 should be cleared and bit 0 kept. This
> appears to enable the buggy page-walker auto clock-gating and disable the
> slave-port gate.
>
> > + }
> > +
> > out_disable_stall:
> > rk_iommu_disable_stall(iommu);
> > out_disable_clocks:
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/[email protected]?part=7
signature.asc
Description: PGP signature
