* Re: [PATCH V2] can: mcp251x: add error handling for power enable inopen and resume
2026-03-12 7:46 ` Marc Kleine-Budde
@ 2026-03-12 12:48 ` 悬溺
2026-03-12 13:01 ` Marc Kleine-Budde
0 siblings, 1 reply; 5+ messages in thread
From: 悬溺 @ 2026-03-12 12:48 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Vincent Mailhol, Jakub Kicinski, Bartosz Golaszewski,
Maud Spierings, Marco Crivellari, Alban Bedel, gszhai, 25125332,
25125283, 23120469, linux-can, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 9849 bytes --]
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, jump to error path to close
candev without attempting to disable power again.
In mcp251x_can_resume(), properly check return values of power enable
calls for both power and transceiver regulators. If any fails, return
the error code to the PM framework and log the failure.
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..0bc208814758 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(&spi->dev, "failed to enable power: %d\n", ret);
+ goto out;
+ }
+ }
+
+ if (priv->after_suspend & AFTER_SUSPEND_UP) {
+ ret = mcp251x_power_enable(priv->transceiver, 1);
+ if (ret) {
+ dev_err(&spi->dev, "failed to enable transceiver: %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
发自我的iPhone
------------------ Original ------------------
From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Thu,Mar 12,2026 3:46 PM
To: Wenyuan Li <2063309626@qq.com>
Cc: Vincent Mailhol <mailhol@kernel.org>, Jakub Kicinski <kuba@kernel.org>, Bartosz Golaszewski <brgl@kernel.org>, Maud Spierings <maudspierings@gocontroll.com>, Marco Crivellari <marco.crivellari@suse.com>, Alban Bedel <alban.bedel@lht.dlh.de>, gszhai <gszhai@bjtu.edu.cn>, 25125332 <25125332@bjtu.edu.cn>, 25125283 <25125283@bjtu.edu.cn>, 23120469 <23120469@bjtu.edu.cn>, linux-can <linux-can@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] can: mcp251x: add error handling for power enable inopen and resume
On 11.03.2026 23:50:53, 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 | 28 +++++++++++++++++++++++-----
> 1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
> index bb7782582f40..807293a7857f 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);
^^^^^^^^^^^
The original code enables the "transceiver"
> + 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->power, 1);
^^^^^
You enable the "power".
> + 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,6 +1546,7 @@ static int __maybe_unused mcp251x_can_resume(struct device *dev)
>
> priv->force_quit = 0;
> enable_irq(spi->irq);
> +out:
> return 0;
Do you want to return "ret" instead of "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 |
[-- Attachment #1.2: Type: text/html, Size: 17884 bytes --]
[-- Attachment #2: v2-0001-mcp251x-add-error-handling-for-power-enable-durin.patch --]
[-- Type: application/octet-stream, Size: 2898 bytes --]
From 65d484eff16453737f87b0c605599f438d4cddb1 Mon Sep 17 00:00:00 2001
From: Wenyuan Li <2063309626@qq.com>
Date: Thu, 12 Mar 2026 19:43:03 +0800
Subject: [PATCH v2] mcp251x: add error handling for power enable during open
and resume
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, jump to error path to close
candev without attempting to disable power again.
In mcp251x_can_resume(), properly check return values of power enable
calls for both power and transceiver regulators. If any fails, return
the error code to the PM framework and log the failure.
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..0bc208814758 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(&spi->dev, "failed to enable power: %d\n", ret);
+ goto out;
+ }
+ }
+
+ if (priv->after_suspend & AFTER_SUSPEND_UP) {
+ ret = mcp251x_power_enable(priv->transceiver, 1);
+ if (ret) {
+ dev_err(&spi->dev, "failed to enable transceiver: %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
^ permalink raw reply [flat|nested] 5+ messages in thread