Re: [PATCH net-next 22/37] can: rcar_canfd: Add support for Renesas RZ/G3L

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The RZ/G3L binding never makes resets/reset-names required,
  but the driver gives RZ/G3L the same reset handling as RZ/G3S.

Pre-existing issues:
- [High] Use-after-free and double free of the channel net_device when
  register_candev() fails during probe.
- [Medium] Unbalanced clock disable/unprepare (and register access with
  the clocks gated) when rcar_canfd_global_init() fails.

--

can: rcar_canfd: Add support for Renesas RZ/G3L

This patch adds the renesas,r9a08g046-canfd compatible to
rcar_canfd_of_table and maps it to the existing RZ/G3S hw_info
(r9a08g045_hw_info). The commit message says RZ/G3L has 3 channels,
11 interrupts and 288 buffers.

Link: https://patch.msgid.link/20261001075742.21961-4-biju.das.jz@bp.renesas.com (local)

[Severity: Medium]
With this patch, RZ/G3L probes and resets exactly like RZ/G3S. Should
the binding also make resets and reset-names required for RZ/G3L?

In Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml,
r9a08g046 is missing from the allOf block that requires them:

  - if:
      properties:
        compatible:
          contains:
            enum:
              - renesas,r9a08g045-canfd
              - renesas,r9a09g047-canfd
              - renesas,rzg2l-canfd
    then:
      required:
        - resets
        - reset-names

The RZ/G3L block only has "resets: minItems: 2", which applies only when
the property is present. So an RZ/G3L node without resets would pass
dt_binding_check and dtbs_check.

The driver would not catch this either. rcar_canfd_probe() gets both
resets as optional:

    gpriv->rstc1 = devm_reset_control_get_optional_exclusive(dev, "rstp_n");
    ...
    gpriv->rstc2 = devm_reset_control_get_optional_exclusive(dev, "rstc_n");

In that case reset_control_reset(NULL) in rcar_canfd_global_init()
does nothing and returns 0. Could the IP then stay in reset with
nothing printed to explain it?

The same RZ/G3L block also sets minItems: 11 for interrupts and
interrupt-names, but has no maxItems: 11. Is that intended?

The schema gap comes from "dt-bindings: can: renesas,rcar-canfd:
Document RZ/G3L SoC" earlier in this series. It is still there at the
end of the series.

[Severity: High]
This isn't a bug introduced by this patch, but RZ/G3L adds a third
channel, so is the register_candev() failure path in
rcar_canfd_channel_probe() safe?

gpriv->ch[ch] is set before registration. The failure path then frees
ndev, and priv with it, without clearing gpriv->ch[ch]:

rcar_canfd_channel_probe() {
    ...
	gpriv->ch[priv->channel] = priv;
	err = register_candev(ndev);
	if (err) {
		...
		goto fail_candev;
	}
    ...
fail_candev:
	netif_napi_del(&priv->napi);
fail:
	free_candev(ndev);
	return err;
}

rcar_canfd_probe() then calls remove for every enabled channel:

fail_channel:
	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels)
		rcar_canfd_channel_remove(gpriv, ch);

The only check in rcar_canfd_channel_remove() is for a NULL priv:

	if (priv) {
		unregister_candev(priv->ndev);
		netif_napi_del(&priv->napi);
		free_candev(priv->ndev);
	}

For the channel that just failed, does this read priv->ndev from freed
memory? It would also call unregister_candev() on a netdev that was
never registered, then free_candev() on the same net_device again.

The early assignment to gpriv->ch[] goes back to at least commit
c5048a7b2c23a.

[Severity: Medium]
This is a pre-existing issue, but when rcar_canfd_global_init() fails,
does rcar_canfd_probe() release the clocks and resets a second time?

rcar_canfd_global_init() already cleans up on its own error paths:

fail_mode:
	rcar_canfd_disable_global_interrupts(gpriv);
fail_reset2:
	reset_control_assert(gpriv->rstc2);
fail_reset1:
	reset_control_assert(gpriv->rstc1);
fail_ram_clk:
	clk_disable_unprepare(gpriv->clk_ram);
fail_clk:
	clk_disable_unprepare(gpriv->clkp);
	return err;

But rcar_canfd_probe() still sends every error to fail_mode:

	err = rcar_canfd_global_init(gpriv);
	if (err)
		goto fail_mode;
    ...
fail_mode:
	rcar_canfd_global_deinit(gpriv, false);

rcar_canfd_global_deinit() calls rcar_canfd_disable_global_interrupts(),
which writes GCTR/GERFL while clkp is off and the resets are asserted.
It then asserts the resets again and calls clk_disable_unprepare() on
clk_ram and clkp a second time.

Would this trigger the clock core's "already disabled" and "already
unprepared" warnings? If the clocks are shared, could it also remove
another consumer's enable reference?

Possible triggers are a reset_control_reset() error, or a
readl_poll_timeout() timeout in rcar_canfd_reset_controller() or in
the GOPM mode check. The global_init/global_deinit split came from
commit fa5f4ec8fff8b.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015015336.816543-2-mkl%40pengutronix.de