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: stable@vger.kernel.org Signed-off-by: Frieder Schrempf frieder.schrempf@kontron.de --- drivers/net/can/spi/mcp251x.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c index a57da43680d8..42e8e5791c9f 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; @@ -1373,6 +1362,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 +1415,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 +1438,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);
On Tue, May 04, 2021 at 06:01:48PM +0200, Schrempf Frieder wrote:
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.
AFAIU the IRQ is freed at ->ndo_stop() which is guaranteed to be called before ->remove(). If it's the case, we are fine.
FWIW, Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
Fixes: 8ce8c0abcba3 ("can: mcp251x: only reset hardware as required") Cc: stable@vger.kernel.org Signed-off-by: Frieder Schrempf frieder.schrempf@kontron.de
drivers/net/can/spi/mcp251x.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c index a57da43680d8..42e8e5791c9f 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;
@@ -1373,6 +1362,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 +1415,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 +1438,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 04.05.2021 18:01:48, Schrempf Frieder wrote:
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: stable@vger.kernel.org Signed-off-by: Frieder Schrempf frieder.schrempf@kontron.de
Added to linux-can/testing.
Thanks, Marc
On 04.05.2021 18:01:48, Schrempf Frieder wrote:
Fixes: 8ce8c0abcba3 ("can: mcp251x: only reset hardware as required") Cc: stable@vger.kernel.org Signed-off-by: Frieder Schrempf frieder.schrempf@kontron.de
Thanks for finding and fixing this and sorry for introducing the bug.
Regards Timo
Hi Schrempf,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mkl-can-next/testing] [also build test WARNING on v5.12 next-20210504] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Schrempf-Frieder/can-mcp251x-Fix-re... base: https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git testing config: x86_64-randconfig-r012-20210503 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 8f5a2a5836cc8e4c1def2bdeb022e7b496623439) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # https://github.com/0day-ci/linux/commit/a6e3fbb55cde65e2254ce0351b92997d1472... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Schrempf-Frieder/can-mcp251x-Fix-resume-from-sleep-before-interface-was-brought-up/20210505-000504 git checkout a6e3fbb55cde65e2254ce0351b92997d14724726 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
drivers/net/can/spi/mcp251x.c:1244:1: warning: unused label 'out_clean' [-Wunused-label]
out_clean: ^~~~~~~~~~ drivers/net/can/spi/mcp251x.c:1335:17: warning: cast to smaller integer type 'enum mcp251x_model' from 'const void *' [-Wvoid-pointer-to-enum-cast] priv->model = (enum mcp251x_model)match; ^~~~~~~~~~~~~~~~~~~~~~~~~ 2 warnings generated.
vim +/out_clean +1244 drivers/net/can/spi/mcp251x.c
e0000163e30eeb drivers/net/can/mcp251x.c Christian Pellegrin 2009-11-02 1193 bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1194 static int mcp251x_open(struct net_device *net) bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1195 { bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1196 struct mcp251x_priv *priv = netdev_priv(net); bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1197 struct spi_device *spi = priv->spi; 6a07c2305ab200 drivers/net/can/spi/mcp251x.c Phil Elwell 2017-11-14 1198 unsigned long flags = 0; bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1199 int ret; bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1200 bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1201 ret = open_candev(net); bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1202 if (ret) { bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1203 dev_err(&spi->dev, "unable to set initial baudrate!\n"); bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1204 return ret; bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1205 } bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1206 bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1207 mutex_lock(&priv->mcp_lock); 1ddff7da0faecf drivers/net/can/mcp251x.c Alexander Shiyan 2013-08-19 1208 mcp251x_power_enable(priv->transceiver, 1); bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1209 bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1210 priv->force_quit = 0; bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1211 priv->tx_skb = NULL; bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1212 priv->tx_len = 0; bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1213 8de29a5c34a5ff drivers/net/can/spi/mcp251x.c Andy Shevchenko 2019-08-26 1214 if (!dev_fwnode(&spi->dev)) 6a07c2305ab200 drivers/net/can/spi/mcp251x.c Phil Elwell 2017-11-14 1215 flags = IRQF_TRIGGER_FALLING; 6a07c2305ab200 drivers/net/can/spi/mcp251x.c Phil Elwell 2017-11-14 1216 bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1217 ret = request_threaded_irq(spi->irq, NULL, mcp251x_can_ist, 3964576307edf4 drivers/net/can/spi/mcp251x.c Alexander Shiyan 2019-01-31 1218 flags | IRQF_ONESHOT, dev_name(&spi->dev), 3964576307edf4 drivers/net/can/spi/mcp251x.c Alexander Shiyan 2019-01-31 1219 priv); bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1220 if (ret) { bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1221 dev_err(&spi->dev, "failed to acquire irq %d\n", spi->irq); 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1222 goto out_close; bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1223 } bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1224 8ce8c0abcba314 drivers/net/can/spi/mcp251x.c Timo Schlüßler 2019-10-11 1225 ret = mcp251x_hw_wake(spi); 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1226 if (ret) 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1227 goto out_free_wq; aa68172235ba7a drivers/net/can/spi/mcp251x.c Marc Kleine-Budde 2017-12-06 1228 ret = mcp251x_setup(net, spi); 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1229 if (ret) 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1230 goto out_free_wq; bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1231 ret = mcp251x_set_normal_mode(spi); 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1232 if (ret) 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1233 goto out_free_wq; eb072a9baebefe drivers/net/can/mcp251x.c Fabio Baltieri 2012-12-18 1234 eb072a9baebefe drivers/net/can/mcp251x.c Fabio Baltieri 2012-12-18 1235 can_led_event(net, CAN_LED_EVENT_OPEN); eb072a9baebefe drivers/net/can/mcp251x.c Fabio Baltieri 2012-12-18 1236 bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1237 netif_wake_queue(net); 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1238 mutex_unlock(&priv->mcp_lock); bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1239 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1240 return 0; 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1241 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1242 out_free_wq: 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1243 destroy_workqueue(priv->wq); 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 @1244 out_clean: 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1245 free_irq(spi->irq, priv); 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1246 mcp251x_hw_sleep(spi); 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1247 out_close: 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1248 mcp251x_power_enable(priv->transceiver, 0); 375f755899b8fc drivers/net/can/spi/mcp251x.c Weitao Hou 2019-06-25 1249 close_candev(net); bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1250 mutex_unlock(&priv->mcp_lock); bf66f3736a945d drivers/net/can/mcp251x.c Christian Pellegrin 2010-02-03 1251 return ret; e0000163e30eeb drivers/net/can/mcp251x.c Christian Pellegrin 2009-11-02 1252 } e0000163e30eeb drivers/net/can/mcp251x.c Christian Pellegrin 2009-11-02 1253
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 05.05.2021 03:25:57, kernel test robot wrote:
Hi Schrempf,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mkl-can-next/testing] [also build test WARNING on v5.12 next-20210504] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Schrempf-Frieder/can-mcp251x-Fix-re... base: https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git testing config: x86_64-randconfig-r012-20210503 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 8f5a2a5836cc8e4c1def2bdeb022e7b496623439) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # https://github.com/0day-ci/linux/commit/a6e3fbb55cde65e2254ce0351b92997d1472... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Schrempf-Frieder/can-mcp251x-Fix-resume-from-sleep-before-interface-was-brought-up/20210505-000504 git checkout a6e3fbb55cde65e2254ce0351b92997d14724726 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
drivers/net/can/spi/mcp251x.c:1244:1: warning: unused label 'out_clean' [-Wunused-label]
out_clean: ^~~~~~~~~~
Thanks, I'll squash that into the original patch.
drivers/net/can/spi/mcp251x.c:1335:17: warning: cast to smaller integer type 'enum mcp251x_model' from 'const void *' [-Wvoid-pointer-to-enum-cast] priv->model = (enum mcp251x_model)match; ^~~~~~~~~~~~~~~~~~~~~~~~~
This is technically correct, but we only put the enum into the struct of_device_id:
| static const struct of_device_id mcp251x_of_match[] = { | { | .compatible = "microchip,mcp2510", | .data = (void *)CAN_MCP251X_MCP2510, | }, | { | .compatible = "microchip,mcp2515", | .data = (void *)CAN_MCP251X_MCP2515, | }, | { | .compatible = "microchip,mcp25625", | .data = (void *)CAN_MCP251X_MCP25625, | }, | { } | };
An intermediate cast to kernel_ulong_t silences the warning:
| - priv->model = (enum mcp251x_model)match; | + priv->model = (enum mcp251x_model)(kernel_ulong_t)match;
I'll send a patch.
regards, Marc
linux-stable-mirror@lists.linaro.org
-
Andy Shevchenko -
kernel test robot -
Marc Kleine-Budde -
Schlüßler, Timo -
Schrempf Frieder