From: Frieder Schrempf <[email protected]>
Since 8ce8c0abcba3 the driver queues work via priv->restart_work when
resuming after suspend, even when the interface was not previously
enabled. This causes a null dereference error as the workqueue is
only allocated and initialized in mcp251x_open().
To fix this we move the workqueue init to mcp251x_can_probe() as
there is no reason to do it later and repeat it whenever
mcp251x_open() is called.
Fixes: 8ce8c0abcba3 ("can: mcp251x: only reset hardware as required")
Cc: [email protected]
Signed-off-by: Frieder Schrempf <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
Changes in v2:
* Remove the out_clean label in mcp251x_open()
* Add Andy's R-b tag
* Add 'From' tag
Hi Marc, I'm sending a v2 mainly because I noticed that v1 is missing
the 'From' tag and as my company's mailserver always sends my name
reversed this causes incorrect author information in git. So if possible
you could fix this up. If this is too much work, just leave it as is.
Thanks!
---
drivers/net/can/spi/mcp251x.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index a57da43680d8..6f888b771589 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -956,8 +956,6 @@ static int mcp251x_stop(struct net_device *net)
priv->force_quit = 1;
free_irq(spi->irq, priv);
- destroy_workqueue(priv->wq);
- priv->wq = NULL;
mutex_lock(&priv->mcp_lock);
@@ -1224,15 +1222,6 @@ static int mcp251x_open(struct net_device *net)
goto out_close;
}
- priv->wq = alloc_workqueue("mcp251x_wq", WQ_FREEZABLE | WQ_MEM_RECLAIM,
- 0);
- if (!priv->wq) {
- ret = -ENOMEM;
- goto out_clean;
- }
- INIT_WORK(&priv->tx_work, mcp251x_tx_work_handler);
- INIT_WORK(&priv->restart_work, mcp251x_restart_work_handler);
-
ret = mcp251x_hw_wake(spi);
if (ret)
goto out_free_wq;
@@ -1252,7 +1241,6 @@ static int mcp251x_open(struct net_device *net)
out_free_wq:
destroy_workqueue(priv->wq);
-out_clean:
free_irq(spi->irq, priv);
mcp251x_hw_sleep(spi);
out_close:
@@ -1373,6 +1361,15 @@ static int mcp251x_can_probe(struct spi_device *spi)
if (ret)
goto out_clk;
+ priv->wq = alloc_workqueue("mcp251x_wq", WQ_FREEZABLE | WQ_MEM_RECLAIM,
+ 0);
+ if (!priv->wq) {
+ ret = -ENOMEM;
+ goto out_clk;
+ }
+ INIT_WORK(&priv->tx_work, mcp251x_tx_work_handler);
+ INIT_WORK(&priv->restart_work, mcp251x_restart_work_handler);
+
priv->spi = spi;
mutex_init(&priv->mcp_lock);
@@ -1417,6 +1414,8 @@ static int mcp251x_can_probe(struct spi_device *spi)
return 0;
error_probe:
+ destroy_workqueue(priv->wq);
+ priv->wq = NULL;
mcp251x_power_enable(priv->power, 0);
out_clk:
@@ -1438,6 +1437,9 @@ static int mcp251x_can_remove(struct spi_device *spi)
mcp251x_power_enable(priv->power, 0);
+ destroy_workqueue(priv->wq);
+ priv->wq = NULL;
+
clk_disable_unprepare(priv->clk);
free_candev(net);
--
2.25.1
On 05.05.2021 09:14:15, Frieder Schrempf wrote:
> From: Frieder Schrempf <[email protected]>
>
> Since 8ce8c0abcba3 the driver queues work via priv->restart_work when
> resuming after suspend, even when the interface was not previously
> enabled. This causes a null dereference error as the workqueue is
> only allocated and initialized in mcp251x_open().
>
> To fix this we move the workqueue init to mcp251x_can_probe() as
> there is no reason to do it later and repeat it whenever
> mcp251x_open() is called.
>
> Fixes: 8ce8c0abcba3 ("can: mcp251x: only reset hardware as required")
> Cc: [email protected]
> Signed-off-by: Frieder Schrempf <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
> Changes in v2:
> * Remove the out_clean label in mcp251x_open()
> * Add Andy's R-b tag
> * Add 'From' tag
>
> Hi Marc, I'm sending a v2 mainly because I noticed that v1 is missing
> the 'From' tag and as my company's mailserver always sends my name
> reversed this causes incorrect author information in git. So if possible
> you could fix this up. If this is too much work, just leave it as is.
> Thanks!
Done.
I've also squashed this fixup:
| --- a/drivers/net/can/spi/mcp251x.c
| +++ b/drivers/net/can/spi/mcp251x.c
| @@ -1224,13 +1224,13 @@ static int mcp251x_open(struct net_device *net)
|
| ret = mcp251x_hw_wake(spi);
| if (ret)
| - goto out_free_wq;
| + goto out_free_irq;
| ret = mcp251x_setup(net, spi);
| if (ret)
| - goto out_free_wq;
| + goto out_free_irq;
| ret = mcp251x_set_normal_mode(spi);
| if (ret)
| - goto out_free_wq;
| + goto out_free_irq;
|
| can_led_event(net, CAN_LED_EVENT_OPEN);
|
| @@ -1239,8 +1239,7 @@ static int mcp251x_open(struct net_device *net)
|
| return 0;
|
| -out_free_wq:
| - destroy_workqueue(priv->wq);
| +out_free_irq:
| free_irq(spi->irq, priv);
| mcp251x_hw_sleep(spi);
| out_close:
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 05.05.21 09:51, Marc Kleine-Budde wrote:
> On 05.05.2021 09:14:15, Frieder Schrempf wrote:
>> From: Frieder Schrempf <[email protected]>
>>
>> Since 8ce8c0abcba3 the driver queues work via priv->restart_work when
>> resuming after suspend, even when the interface was not previously
>> enabled. This causes a null dereference error as the workqueue is
>> only allocated and initialized in mcp251x_open().
>>
>> To fix this we move the workqueue init to mcp251x_can_probe() as
>> there is no reason to do it later and repeat it whenever
>> mcp251x_open() is called.
>>
>> Fixes: 8ce8c0abcba3 ("can: mcp251x: only reset hardware as required")
>> Cc: [email protected]
>> Signed-off-by: Frieder Schrempf <[email protected]>
>> Reviewed-by: Andy Shevchenko <[email protected]>
>> ---
>> Changes in v2:
>> * Remove the out_clean label in mcp251x_open()
>> * Add Andy's R-b tag
>> * Add 'From' tag
>>
>> Hi Marc, I'm sending a v2 mainly because I noticed that v1 is missing
>> the 'From' tag and as my company's mailserver always sends my name
>> reversed this causes incorrect author information in git. So if possible
>> you could fix this up. If this is too much work, just leave it as is.
>> Thanks!
>
> Done.
Thanks!
>
> I've also squashed this fixup:
Oh dear, I really should have looked at this more closely.
Thanks a lot and sorry for the mess!
>
> | --- a/drivers/net/can/spi/mcp251x.c
> | +++ b/drivers/net/can/spi/mcp251x.c
> | @@ -1224,13 +1224,13 @@ static int mcp251x_open(struct net_device *net)
> |
> | ret = mcp251x_hw_wake(spi);
> | if (ret)
> | - goto out_free_wq;
> | + goto out_free_irq;
> | ret = mcp251x_setup(net, spi);
> | if (ret)
> | - goto out_free_wq;
> | + goto out_free_irq;
> | ret = mcp251x_set_normal_mode(spi);
> | if (ret)
> | - goto out_free_wq;
> | + goto out_free_irq;
> |
> | can_led_event(net, CAN_LED_EVENT_OPEN);
> |
> | @@ -1239,8 +1239,7 @@ static int mcp251x_open(struct net_device *net)
> |
> | return 0;
> |
> | -out_free_wq:
> | - destroy_workqueue(priv->wq);
> | +out_free_irq:
> | free_irq(spi->irq, priv);
> | mcp251x_hw_sleep(spi);
> | out_close:
>
> Marc
>