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

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

[Sean Nyekjaer]

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.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Changes since v1:
 - Moved enter sleep mode into tcan4x5x_power_enable()

 drivers/net/can/m_can/tcan4x5x-core.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 31cc9d0abd45..67902b8b0fab 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_classdev *cdev,
 	return regmap_bulk_write(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, val, val_count);
 }
 
-static int tcan4x5x_power_enable(struct regulator *reg, int enable)
+static int tcan4x5x_power_enable(struct tcan4x5x_priv *priv, int enable)
 {
-	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;
+	}
 
 	if (enable)
-		return regulator_enable(reg);
+		return regulator_enable(priv->power);
 	else
-		return regulator_disable(reg);
+		return regulator_disable(priv->power);
 }
 
 static int tcan4x5x_write_tcan_reg(struct m_can_classdev *cdev,
@@ -476,7 +482,7 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
 		goto out_m_can_class_free_dev;
 	}
 
-	ret = tcan4x5x_power_enable(priv->power, 1);
+	ret = tcan4x5x_power_enable(priv, 1);
 	if (ret) {
 		dev_err(&spi->dev, "Enabling regulator failed %pe\n",
 			ERR_PTR(ret));
@@ -531,7 +537,7 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
 	return 0;
 
 out_power:
-	tcan4x5x_power_enable(priv->power, 0);
+	tcan4x5x_power_enable(priv, 0);
  out_m_can_class_free_dev:
 	m_can_class_free_dev(mcan_class->net);
 	return ret;
-- 
2.55.0

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

[sashiko-bot]

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 during 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 teardown, 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 <sean@geanix.com>

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_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_classdev *cdev,
>  	return regmap_bulk_write(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, val, val_count);
>  }
>  
> -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 leave
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 another
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;
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803101927.17122-1-sean@geanix.com?part=1

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

On 03.08.2026 10:33:19, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
[...]
> - [Medium] The transceiver is not explicitly put into sleep mode via
>   SPI during driver removal if it is powered by a regulator, undermining
>   the patch's intended power savings.

Bad Bot! Read the data sheet. The transceiver/PHY is integrated into the
device, so if the regulator powers down the device, the transceiver/PHY
is powered down, too.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Sean Nyekjaer]

On Monday, 3 August 2026 at 13:05, sashiko-bot@kernel.org <sashiko-bot@kernel.org> wrote:

> 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.

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 patch's intended power savings.

Intended, as the regulator will power it off.

> 
> Pre-existing issues:
> - [High] The polling hrtimer is not synchronously cancelled during device teardown, 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 <sean@geanix.com>
> 
> 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_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_classdev *cdev,
> >  	return regmap_bulk_write(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, val, val_count);
> >  }
> >
> > -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 leave
> 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 another
> 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;
> }
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260803101927.17122-1-sean@geanix.com?part=1
> 
>

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2474 bytes --]

On 03.08.2026 12:19:26, Sean Nyekjaer wrote:
> 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.
>
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>

Seems you patch is not complete:

| drivers/net/can/m_can/tcan4x5x-core.c: In function ‘tcan4x5x_can_remove’:
| drivers/net/can/m_can/tcan4x5x-core.c:552:35: error: passing argument 1 of ‘tcan4x5x_power_enable’ from incompatible pointer type [-Wincompatible-pointer-types]
|   552 |         tcan4x5x_power_enable(priv->power, 0);
|       |                               ~~~~^~~~~~~
|       |                                   |
|       |                                   struct regulator *
| drivers/net/can/m_can/tcan4x5x-core.c:214:56: note: expected ‘struct tcan4x5x_priv *’ but argument is of type ‘struct regulator *’
|   214 | static int tcan4x5x_power_enable(struct tcan4x5x_priv *priv, int enable)
|       |                                  ~~~~~~~~~~~~~~~~~~~~~~^~~~

> ---
> Changes since v1:
>  - Moved enter sleep mode into tcan4x5x_power_enable()
>
>  drivers/net/can/m_can/tcan4x5x-core.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 31cc9d0abd45..67902b8b0fab 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_classdev *cdev,
>  	return regmap_bulk_write(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, val, val_count);
>  }
>
> -static int tcan4x5x_power_enable(struct regulator *reg, int enable)
> +static int tcan4x5x_power_enable(struct tcan4x5x_priv *priv, int enable)
>  {

If you add:

        struct regulator *reg = priv->power;

the diff should be smaller.

> -	if (IS_ERR_OR_NULL(reg))
> +	if (IS_ERR_OR_NULL(priv->power)) {

Can you please add a comment that the reset GPIO is needed to get the
device out of sleep mode.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Sean Nyekjaer]

On Monday, 3 August 2026 at 13:27, Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 03.08.2026 12:19:26, Sean Nyekjaer wrote:
> > 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.
> >
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> 
> Seems you patch is not complete:
> 
> | drivers/net/can/m_can/tcan4x5x-core.c: In function ‘tcan4x5x_can_remove’:
> | drivers/net/can/m_can/tcan4x5x-core.c:552:35: error: passing argument 1 of ‘tcan4x5x_power_enable’ from incompatible pointer type [-Wincompatible-pointer-types]
> |   552 |         tcan4x5x_power_enable(priv->power, 0);
> |       |                               ~~~~^~~~~~~
> |       |                                   |
> |       |                                   struct regulator *
> | drivers/net/can/m_can/tcan4x5x-core.c:214:56: note: expected ‘struct tcan4x5x_priv *’ but argument is of type ‘struct regulator *’
> |   214 | static int tcan4x5x_power_enable(struct tcan4x5x_priv *priv, int enable)
> |       |                                  ~~~~~~~~~~~~~~~~~~~~~~^~~~

Yes.

> 
> > ---
> > Changes since v1:
> >  - Moved enter sleep mode into tcan4x5x_power_enable()
> >
> >  drivers/net/can/m_can/tcan4x5x-core.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> > index 31cc9d0abd45..67902b8b0fab 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_classdev *cdev,
> >  	return regmap_bulk_write(priv->regmap, TCAN4X5X_MRAM_START + addr_offset, val, val_count);
> >  }
> >
> > -static int tcan4x5x_power_enable(struct regulator *reg, int enable)
> > +static int tcan4x5x_power_enable(struct tcan4x5x_priv *priv, int enable)
> >  {
> 
> If you add:
> 
>         struct regulator *reg = priv->power;
> 
> the diff should be smaller.

Yes.

> 
> > -	if (IS_ERR_OR_NULL(reg))
> > +	if (IS_ERR_OR_NULL(priv->power)) {
> 
> Can you please add a comment that the reset GPIO is needed to get the
> device out of sleep mode.

Will do! Sorry first day after vacation.

/Sean