Re: [PATCH v2] can: tcan4x5x: put tcan into sleep when removing driver

Thank you for your contribution! Sashiko AI review found 3 potential issue(=
s) to consider:

New issues:
- [High] The function signature of `tcan4x5x_power_enable()` was changed to=
 accept a `struct tcan4x5x_priv *` instead of `struct regulator *`, but the=
 call site in `tcan4x5x_can_remove()` was not updated. This causes a severe=
 type mismatch leading to a wild pointer dereference.
- [Medium] The transceiver is not explicitly put into sleep mode via SPI du=
ring driver removal if it is powered by a regulator, undermining the patch'=
s intended power savings.

Pre-existing issues:
- [High] The polling hrtimer is not synchronously cancelled during device t=
eardown, allowing its callback to execute concurrently on freed memory.
--

--- Patch [1]: [PATCH v2] can: tcan4x5x: put tcan into sleep when removing =
driver ---
commit b49d2b844c12ed05e9ff4437bc458dbace9b23e8
Author: Sean Nyekjaer <[email protected]>

can: tcan4x5x: put tcan into sleep when removing driver

Put the tcan4x5x transceiver into sleep mode when the driver is
removed, instead of leaving it in its current operating mode.
This reduces power consumption(3mA@12V) once the driver is
no longer bound to the device.

> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_ca=
n/tcan4x5x-core.c
> index 31cc9d0abd453..67902b8b0fab4 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -211,15 +211,21 @@ static int tcan4x5x_write_fifo(struct m_can_classde=
v *cdev,
>  	return regmap_bulk_write(priv->regmap, TCAN4X5X_MRAM_START + addr_offse=
t, val, val_count);
>  }
> =20
> -static int tcan4x5x_power_enable(struct regulator *reg, int enable)
> +static int tcan4x5x_power_enable(struct tcan4x5x_priv *priv, int enable)
>  {

[Severity: High]
Did this signature change miss updating the call site in
tcan4x5x_can_remove()?

When the driver is unloaded, tcan4x5x_can_remove() still passes priv->power
which is a struct regulator pointer:

drivers/net/can/m_can/tcan4x5x-core.c:tcan4x5x_can_remove() {
	...
	tcan4x5x_power_enable(priv->power, 0);
	...
}

Could this cause a panic when tcan4x5x_power_enable() tries to dereference
the regulator pointer as a struct tcan4x5x_priv?

> -	if (IS_ERR_OR_NULL(reg))
> +	if (IS_ERR_OR_NULL(priv->power)) {
> +		if (priv->reset_gpio && !enable)
> +			return regmap_update_bits(priv->regmap, TCAN4X5X_CONFIG,
> +						  TCAN4X5X_MODE_SEL_MASK,
> +						  TCAN4X5X_MODE_SLEEP);
> +
>  		return 0;
> +	}

[Severity: Medium]
If the device tree specifies a regulator for the transceiver, will this
skip putting the device into sleep mode?

Since the sleep configuration is gated inside the check for
IS_ERR_OR_NULL(priv->power), if a regulator is present, the function skips
this block and just disables the regulator.

If this regulator is shared with other components on the board, does it lea=
ve
the transceiver powered and in its previous state instead of entering sleep
mode?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
with the polling hrtimer during device teardown?

During device teardown, m_can_disable_all_interrupts() uses a non-blocking
cancel:

drivers/net/can/m_can/m_can.c:m_can_disable_all_interrupts() {
	...
	if (!cdev->net->irq) {
		netdev_dbg(cdev->net, "Stop hrtimer\n");
		hrtimer_try_to_cancel(&cdev->hrtimer);
	}
}

If the timer callback m_can_polling_timer() is concurrently running on anot=
her
CPU, hrtimer_try_to_cancel() will fail.

If the callback reads the state before it is updated to CAN_STATE_STOPPED,
will it reschedule itself and eventually execute on freed memory?

drivers/net/can/m_can/m_can.c:m_can_polling_timer() {
	...
	hrtimer_forward_now(timer, ms_to_ktime(HRTIMER_POLL_INTERVAL_MS));

	return HRTIMER_RESTART;
}

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803101927.1712=
[email protected]?part=3D1