drivers/net/can/spi/mcp251x.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)
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);
+ 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);
+ 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;
}
--
2.43.0On 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 |
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 |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
On 12.03.2026 20:48:43, 悬溺 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, 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> Please use git send-email as you did with the first patch. 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 |
> Add missing error handling for mcp251x_power_enable() calls in both
> mcp251x_open() and mcp251x_can_resume() functions.
…
> 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.
…
How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
…
> @@ -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;
How do you think about to use the return value at an other place
(at the end)?
>
> - 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_UP) {
> + 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_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;
> }
Would you like to avoid duplicate source code here?
Regards,
Markus
© 2016 - 2026 Red Hat, Inc.