1. Patchew
  2. linux
  3. View series

[PATCH v2] can: mcp251x: add error handling for power enable in open and resume

Wenyuan Li posted 1 patch 3 weeks, 1 day ago
There is a newer version of this series
drivers/net/can/spi/mcp251x.c | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
[PATCH v2] can: mcp251x: add error handling for power enable in open and resume
Posted by Wenyuan Li 3 weeks, 1 day ago 
Add missing error handling for mcp251x_power_enable() calls in both
mcp251x_open() and mcp251x_can_resume() functions.

In mcp251x_open(), if power enable fails, the driver should not continue
with device initialization. Add proper error checking and jump to
existing out_close label.

In mcp251x_can_resume(), if power enable fails during system resume,
propagate the error to PM framework and log the error with dev_err()
for debugging.

This ensures the driver properly handles power control failures and
maintains correct device state.
Signed-off-by: Wenyuan Li <2063309626@qq.com>

 drivers/net/can/spi/mcp251x.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index bb7782582f40..28324eed1303 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -1225,7 +1225,11 @@ static int mcp251x_open(struct net_device *net)
 	}
 
 	mutex_lock(&priv->mcp_lock);
-	mcp251x_power_enable(priv->transceiver, 1);
+	ret = mcp251x_power_enable(priv->transceiver, 1);
+	if (ret) {
+		dev_err(&spi->dev, "failed to enable power: %d\n", ret);
+		goto out_close_candev;
+	}
 
 	priv->force_quit = 0;
 	priv->tx_skb = NULL;
@@ -1272,6 +1276,7 @@ static int mcp251x_open(struct net_device *net)
 	mcp251x_hw_sleep(spi);
 out_close:
 	mcp251x_power_enable(priv->transceiver, 0);
+out_close_candev:
 	close_candev(net);
 	mutex_unlock(&priv->mcp_lock);
 	if (release_irq)
@@ -1516,11 +1521,23 @@ static int __maybe_unused mcp251x_can_resume(struct device *dev)
 {
 	struct spi_device *spi = to_spi_device(dev);
 	struct mcp251x_priv *priv = spi_get_drvdata(spi);
+	int ret = 0;
 
-	if (priv->after_suspend & AFTER_SUSPEND_POWER)
-		mcp251x_power_enable(priv->power, 1);
-	if (priv->after_suspend & AFTER_SUSPEND_UP)
-		mcp251x_power_enable(priv->transceiver, 1);
+	if (priv->after_suspend & AFTER_SUSPEND_POWER) {
+		ret = mcp251x_power_enable(priv->power, 1);
+		if (ret) {
+			dev_err(dev, "Failed to restore power: %d\n", ret);
+			goto out;
+		}
+	}
+
+	if (priv->after_suspend & AFTER_SUSPEND_UP) {
+		ret = mcp251x_power_enable(priv->transceiver, 1);
+		if (ret) {
+			dev_err(dev, "Failed to restore power: %d\n", ret);
+			goto out;
+		}
+	}
 
 	if (priv->after_suspend & (AFTER_SUSPEND_POWER | AFTER_SUSPEND_UP))
 		queue_work(priv->wq, &priv->restart_work);
@@ -1529,7 +1546,8 @@ static int __maybe_unused mcp251x_can_resume(struct device *dev)
 
 	priv->force_quit = 0;
 	enable_irq(spi->irq);
-	return 0;
+out:
+	return ret;
 }
 
 static SIMPLE_DEV_PM_OPS(mcp251x_can_pm_ops, mcp251x_can_suspend,
-- 
2.43.0
Re: [PATCH v2] can: mcp251x: add error handling for power enable in open and resume
Posted by Marc Kleine-Budde 3 weeks, 1 day ago 
Hello nyuan Li,

thanks for sending the v2.

On 15.03.2026 21:24:51, Wenyuan Li wrote:
> Add missing error handling for mcp251x_power_enable() calls in both
> mcp251x_open() and mcp251x_can_resume() functions.
>
> In mcp251x_open(), if power enable fails, the driver should not continue
> with device initialization. Add proper error checking and jump to
> existing out_close label.
>
> In mcp251x_can_resume(), if power enable fails during system resume,
> propagate the error to PM framework and log the error with dev_err()
> for debugging.
>
> This ensures the driver properly handles power control failures and
> maintains correct device state.
> Signed-off-by: Wenyuan Li <2063309626@qq.com>
>
>  drivers/net/can/spi/mcp251x.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
> index bb7782582f40..28324eed1303 100644
> --- a/drivers/net/can/spi/mcp251x.c
> +++ b/drivers/net/can/spi/mcp251x.c
> @@ -1225,7 +1225,11 @@ static int mcp251x_open(struct net_device *net)
>  	}
>
>  	mutex_lock(&priv->mcp_lock);
> -	mcp251x_power_enable(priv->transceiver, 1);
> +	ret = mcp251x_power_enable(priv->transceiver, 1);
> +	if (ret) {
> +		dev_err(&spi->dev, "failed to enable power: %d\n", ret);

As there can be 2 regulators, I think it's better to print which one was
failing. What about: "failed to enable transceiver power".

Please use "%pe" and "ERR_PTR(ret)" to print the error value, this way
it's translated into human readable text.

> +		goto out_close_candev;
> +	}
>
>  	priv->force_quit = 0;
>  	priv->tx_skb = NULL;
> @@ -1272,6 +1276,7 @@ static int mcp251x_open(struct net_device *net)
>  	mcp251x_hw_sleep(spi);
>  out_close:
>  	mcp251x_power_enable(priv->transceiver, 0);
> +out_close_candev:
>  	close_candev(net);
>  	mutex_unlock(&priv->mcp_lock);
>  	if (release_irq)
> @@ -1516,11 +1521,23 @@ static int __maybe_unused mcp251x_can_resume(struct device *dev)
>  {
>  	struct spi_device *spi = to_spi_device(dev);
>  	struct mcp251x_priv *priv = spi_get_drvdata(spi);
> +	int ret = 0;
>
> -	if (priv->after_suspend & AFTER_SUSPEND_POWER)
> -		mcp251x_power_enable(priv->power, 1);
> -	if (priv->after_suspend & AFTER_SUSPEND_UP)
> -		mcp251x_power_enable(priv->transceiver, 1);
> +	if (priv->after_suspend & AFTER_SUSPEND_POWER) {
> +		ret = mcp251x_power_enable(priv->power, 1);
> +		if (ret) {
> +			dev_err(dev, "Failed to restore power: %d\n", ret);

use %pe here, too.

> +			goto out;
> +		}
> +	}
> +
> +	if (priv->after_suspend & AFTER_SUSPEND_UP) {
> +		ret = mcp251x_power_enable(priv->transceiver, 1);
> +		if (ret) {
> +			dev_err(dev, "Failed to restore power: %d\n", ret);

"failed to restore transceiver power"

use %pe here, too.

Do we want to power down the "priv->power" regulator? in case of an
error?

> +			goto out;
> +		}
> +	}
>
>  	if (priv->after_suspend & (AFTER_SUSPEND_POWER | AFTER_SUSPEND_UP))
>  		queue_work(priv->wq, &priv->restart_work);
> @@ -1529,7 +1546,8 @@ static int __maybe_unused mcp251x_can_resume(struct device *dev)
>
>  	priv->force_quit = 0;
>  	enable_irq(spi->irq);
> -	return 0;
> +out:
> +	return ret;
>  }
>
>  static SIMPLE_DEV_PM_OPS(mcp251x_can_pm_ops, mcp251x_can_suspend,
> --
> 2.43.0
>
>

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   |

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.