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

On Monday, 3 August 2026 at 13:05, [email protected] <sashiko-bot@kern=
el.org> wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issu=
e(s) to consider:
>=20
> New issues:
> - [High] The function signature of `tcan4x5x_power_enable()` was changed =
to accept a `struct tcan4x5x_priv *` instead of `struct regulator *`, but t=
he call site in `tcan4x5x_can_remove()` was not updated. This causes a seve=
re type mismatch leading to a wild pointer dereference.

Will fix for v3 :)

> - [Medium] The transceiver is not explicitly put into sleep mode via SPI =
during driver removal if it is powered by a regulator, undermining the patc=
h's intended power savings.

Intended, as the regulator will power it off.

>=20
> Pre-existing issues:
> - [High] The polling hrtimer is not synchronously cancelled during device=
 teardown, allowing its callback to execute concurrently on freed memory.
> --
>=20
> --- Patch [1]: [PATCH v2] can: tcan4x5x: put tcan into sleep when removin=
g driver ---
> commit b49d2b844c12ed05e9ff4437bc458dbace9b23e8
> Author: Sean Nyekjaer <[email protected]>
>=20
> can: tcan4x5x: put tcan into sleep when removing driver
>=20
> 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.
>=20
> > diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_=
can/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_class=
dev *cdev,
> >  =09return regmap_bulk_write(priv->regmap, TCAN4X5X_MRAM_START + addr_o=
ffset, val, val_count);
> >  }
> >
> > -static int tcan4x5x_power_enable(struct regulator *reg, int enable)
> > +static int tcan4x5x_power_enable(struct tcan4x5x_priv *priv, int enabl=
e)
> >  {
>=20
> [Severity: High]
> Did this signature change miss updating the call site in
> tcan4x5x_can_remove()?
>=20
> When the driver is unloaded, tcan4x5x_can_remove() still passes priv->pow=
er
> which is a struct regulator pointer:
>=20
> drivers/net/can/m_can/tcan4x5x-core.c:tcan4x5x_can_remove() {
> =09...
> =09tcan4x5x_power_enable(priv->power, 0);
> =09...
> }
>=20
> Could this cause a panic when tcan4x5x_power_enable() tries to dereferenc=
e
> the regulator pointer as a struct tcan4x5x_priv?
>=20
> > -=09if (IS_ERR_OR_NULL(reg))
> > +=09if (IS_ERR_OR_NULL(priv->power)) {
> > +=09=09if (priv->reset_gpio && !enable)
> > +=09=09=09return regmap_update_bits(priv->regmap, TCAN4X5X_CONFIG,
> > +=09=09=09=09=09=09  TCAN4X5X_MODE_SEL_MASK,
> > +=09=09=09=09=09=09  TCAN4X5X_MODE_SLEEP);
> > +
> >  =09=09return 0;
> > +=09}
>=20
> [Severity: Medium]
> If the device tree specifies a regulator for the transceiver, will this
> skip putting the device into sleep mode?
>=20
> Since the sleep configuration is gated inside the check for
> IS_ERR_OR_NULL(priv->power), if a regulator is present, the function skip=
s
> this block and just disables the regulator.
>=20
> If this regulator is shared with other components on the board, does it l=
eave
> the transceiver powered and in its previous state instead of entering sle=
ep
> mode?
>=20
> [ ... ]
>=20
> [Severity: High]
> This isn't a bug introduced by this patch, but is there a race condition
> with the polling hrtimer during device teardown?
>=20
> During device teardown, m_can_disable_all_interrupts() uses a non-blockin=
g
> cancel:
>=20
> drivers/net/can/m_can/m_can.c:m_can_disable_all_interrupts() {
> =09...
> =09if (!cdev->net->irq) {
> =09=09netdev_dbg(cdev->net, "Stop hrtimer\n");
> =09=09hrtimer_try_to_cancel(&cdev->hrtimer);
> =09}
> }
>=20
> If the timer callback m_can_polling_timer() is concurrently running on an=
other
> CPU, hrtimer_try_to_cancel() will fail.
>=20
> If the callback reads the state before it is updated to CAN_STATE_STOPPED=
,
> will it reschedule itself and eventually execute on freed memory?
>=20
> drivers/net/can/m_can/m_can.c:m_can_polling_timer() {
> =09...
> =09hrtimer_forward_now(timer, ms_to_ktime(HRTIMER_POLL_INTERVAL_MS));
>=20
> =09return HRTIMER_RESTART;
> }
>=20
> --
> Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803101927.17=
[email protected]?part=3D1
>=20
>