* [net-next PATCH 0/5] flexcan: fix hanging flexcan_close()
@ 2020-11-19 8:52 Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 1/5] can: flexcan: factor out enabling and disabling of interrupts into separate function Marc Kleine-Budde
` (4 more replies)
0 siblings, 5 replies; 8+ messages in thread
From: Marc Kleine-Budde @ 2020-11-19 8:52 UTC (permalink / raw)
To: linux-can; +Cc: Joakim Zhang, kernel
Hello,
There haven been reports, that the flexcan_close() soradically hangs during
simultanious ifdown, sending of CAN messages and probably open CAN bus:
| (__schedule) from [<808bbd34>] (schedule+0x90/0xb8)
| (schedule) from [<808bf274>] (schedule_timeout+0x1f8/0x24c)
| (schedule_timeout) from [<8016be44>] (msleep+0x18/0x1c)
| (msleep) from [<80746a64>] (napi_disable+0x60/0x70)
| (napi_disable) from [<8052fdd0>] (flexcan_close+0x2c/0x140)
| (flexcan_close) from [<80744930>] (__dev_close_many+0xb8/0xd8)
| (__dev_close_many) from [<8074db9c>] (__dev_change_flags+0xd0/0x1a0)
| (__dev_change_flags) from [<8074dc84>] (dev_change_flags+0x18/0x48)
| (dev_change_flags) from [<80760c24>] (do_setlink+0x44c/0x7b4)
| (do_setlink) from [<80761560>] (rtnl_newlink+0x374/0x68c)
I was unable to reproduce the issue, but a cleanup of the flexcan close
sequence has probably fixed the problem at the reporting user.
As this fix spans 5 patches and changes the flexcan_open() and flexcan_close()
are rather big, I'm reluctant to push this via net/master now. So I think this
series should go via net-next/master in my next pull request.
regards,
Marc
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/5] can: flexcan: factor out enabling and disabling of interrupts into separate function
2020-11-19 8:52 [net-next PATCH 0/5] flexcan: fix hanging flexcan_close() Marc Kleine-Budde
@ 2020-11-19 8:52 ` Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 2/5] can: flexcan: move enabling/disabling of interrupts from flexcan_chip_{start,stop}() to callers Marc Kleine-Budde
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Marc Kleine-Budde @ 2020-11-19 8:52 UTC (permalink / raw)
To: linux-can; +Cc: Joakim Zhang, kernel, Marc Kleine-Budde
The upcoming patches are going to move the enabling and disabling of the
interrupts. Introduce convenience functions to make these patches simpler.
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/flexcan.c | 41 ++++++++++++++++++++++++++-------------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 06e03ab6f43a..56af58c4179b 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1383,6 +1383,31 @@ static void flexcan_ram_init(struct net_device *dev)
priv->write(reg_ctrl2, ®s->ctrl2);
}
+static void flexcan_chip_interrupts_enable(const struct net_device *dev)
+{
+ const struct flexcan_priv *priv = netdev_priv(dev);
+ struct flexcan_regs __iomem *regs = priv->regs;
+ u64 reg_imask;
+
+ disable_irq(dev->irq);
+ priv->write(priv->reg_ctrl_default, ®s->ctrl);
+ reg_imask = priv->rx_mask | priv->tx_mask;
+ priv->write(upper_32_bits(reg_imask), ®s->imask2);
+ priv->write(lower_32_bits(reg_imask), ®s->imask1);
+ enable_irq(dev->irq);
+}
+
+static void flexcan_chip_interrupts_disable(const struct net_device *dev)
+{
+ const struct flexcan_priv *priv = netdev_priv(dev);
+ struct flexcan_regs __iomem *regs = priv->regs;
+
+ priv->write(0, ®s->imask2);
+ priv->write(0, ®s->imask1);
+ priv->write(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
+ ®s->ctrl);
+}
+
/* flexcan_chip_start
*
* this functions is entered with clocks enabled
@@ -1393,7 +1418,6 @@ static int flexcan_chip_start(struct net_device *dev)
struct flexcan_priv *priv = netdev_priv(dev);
struct flexcan_regs __iomem *regs = priv->regs;
u32 reg_mcr, reg_ctrl, reg_ctrl2, reg_mecr;
- u64 reg_imask;
int err, i;
struct flexcan_mb __iomem *mb;
@@ -1611,13 +1635,7 @@ static int flexcan_chip_start(struct net_device *dev)
priv->can.state = CAN_STATE_ERROR_ACTIVE;
- /* enable interrupts atomically */
- disable_irq(dev->irq);
- priv->write(priv->reg_ctrl_default, ®s->ctrl);
- reg_imask = priv->rx_mask | priv->tx_mask;
- priv->write(upper_32_bits(reg_imask), ®s->imask2);
- priv->write(lower_32_bits(reg_imask), ®s->imask1);
- enable_irq(dev->irq);
+ flexcan_chip_interrupts_enable(dev);
/* print chip status */
netdev_dbg(dev, "%s: reading mcr=0x%08x ctrl=0x%08x\n", __func__,
@@ -1637,7 +1655,6 @@ static int flexcan_chip_start(struct net_device *dev)
static int __flexcan_chip_stop(struct net_device *dev, bool disable_on_error)
{
struct flexcan_priv *priv = netdev_priv(dev);
- struct flexcan_regs __iomem *regs = priv->regs;
int err;
/* freeze + disable module */
@@ -1648,11 +1665,7 @@ static int __flexcan_chip_stop(struct net_device *dev, bool disable_on_error)
if (err && !disable_on_error)
goto out_chip_unfreeze;
- /* Disable all interrupts */
- priv->write(0, ®s->imask2);
- priv->write(0, ®s->imask1);
- priv->write(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
- ®s->ctrl);
+ flexcan_chip_interrupts_disable(dev);
priv->can.state = CAN_STATE_STOPPED;
--
2.29.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/5] can: flexcan: move enabling/disabling of interrupts from flexcan_chip_{start,stop}() to callers
2020-11-19 8:52 [net-next PATCH 0/5] flexcan: fix hanging flexcan_close() Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 1/5] can: flexcan: factor out enabling and disabling of interrupts into separate function Marc Kleine-Budde
@ 2020-11-19 8:52 ` Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 3/5] can: flexcan: flexcan_rx_offload_setup(): factor out mailbox and rx-offload setup into separate function Marc Kleine-Budde
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Marc Kleine-Budde @ 2020-11-19 8:52 UTC (permalink / raw)
To: linux-can; +Cc: Joakim Zhang, kernel, Marc Kleine-Budde
The function flexcan_chip_start() first configures the CAN controller and then
enables the interrupt, flexcan_chip_stop() does the opposite.
In an upcoming patch the order of operations in flexcan_open() and
flexcan_close() are changed. This requires
flexcan_chip_start()/flexcan_chip_stop_disable_on_error() and
flexcan_chip_interrupts_{enable,disable}() to be independent of each other.
This patch moves the enabling of the interrupts from flexcan_chip_start() to
its callers flexcan_open() and flexcan_resume(). Likewise the disabling of the
interrupts is moved from __flexcan_chip_stop() to its indirect callers
flexcan_close() and flexcan_suspend().
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/flexcan.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 56af58c4179b..52ce26edfb0f 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1635,8 +1635,6 @@ static int flexcan_chip_start(struct net_device *dev)
priv->can.state = CAN_STATE_ERROR_ACTIVE;
- flexcan_chip_interrupts_enable(dev);
-
/* print chip status */
netdev_dbg(dev, "%s: reading mcr=0x%08x ctrl=0x%08x\n", __func__,
priv->read(®s->mcr), priv->read(®s->ctrl));
@@ -1665,8 +1663,6 @@ static int __flexcan_chip_stop(struct net_device *dev, bool disable_on_error)
if (err && !disable_on_error)
goto out_chip_unfreeze;
- flexcan_chip_interrupts_disable(dev);
-
priv->can.state = CAN_STATE_STOPPED;
return 0;
@@ -1754,6 +1750,8 @@ static int flexcan_open(struct net_device *dev)
if (err)
goto out_offload_del;
+ flexcan_chip_interrupts_enable(dev);
+
can_led_event(dev, CAN_LED_EVENT_OPEN);
can_rx_offload_enable(&priv->offload);
@@ -1782,6 +1780,7 @@ static int flexcan_close(struct net_device *dev)
netif_stop_queue(dev);
can_rx_offload_disable(&priv->offload);
flexcan_chip_stop_disable_on_error(dev);
+ flexcan_chip_interrupts_disable(dev);
can_rx_offload_del(&priv->offload);
free_irq(dev->irq, dev);
@@ -1805,6 +1804,8 @@ static int flexcan_set_mode(struct net_device *dev, enum can_mode mode)
if (err)
return err;
+ flexcan_chip_interrupts_enable(dev);
+
netif_wake_queue(dev);
break;
@@ -2193,6 +2194,8 @@ static int __maybe_unused flexcan_suspend(struct device *device)
if (err)
return err;
+ flexcan_chip_interrupts_disable(dev);
+
err = pinctrl_pm_select_sleep_state(device);
if (err)
return err;
@@ -2228,6 +2231,8 @@ static int __maybe_unused flexcan_resume(struct device *device)
err = flexcan_chip_start(dev);
if (err)
return err;
+
+ flexcan_chip_interrupts_enable(dev);
}
}
--
2.29.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/5] can: flexcan: flexcan_rx_offload_setup(): factor out mailbox and rx-offload setup into separate function
2020-11-19 8:52 [net-next PATCH 0/5] flexcan: fix hanging flexcan_close() Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 1/5] can: flexcan: factor out enabling and disabling of interrupts into separate function Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 2/5] can: flexcan: move enabling/disabling of interrupts from flexcan_chip_{start,stop}() to callers Marc Kleine-Budde
@ 2020-11-19 8:52 ` Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 4/5] can: flexcan: flexcan_open(): completely initialize controller before requesting IRQ Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 5/5] can: flexcan: flexcan_close(): change order if commands to properly shut down the controller Marc Kleine-Budde
4 siblings, 0 replies; 8+ messages in thread
From: Marc Kleine-Budde @ 2020-11-19 8:52 UTC (permalink / raw)
To: linux-can; +Cc: Joakim Zhang, kernel, Marc Kleine-Budde
In an upcoming patch the order of operations in flexcan_open() are changed.
Introduce convenience function to make that patch simpler.
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/flexcan.c | 74 ++++++++++++++++++++++-----------------
1 file changed, 42 insertions(+), 32 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 52ce26edfb0f..cbce8a3d090c 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1383,6 +1383,47 @@ static void flexcan_ram_init(struct net_device *dev)
priv->write(reg_ctrl2, ®s->ctrl2);
}
+static int flexcan_rx_offload_setup(struct net_device *dev)
+{
+ struct flexcan_priv *priv = netdev_priv(dev);
+ int err;
+
+ if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
+ priv->mb_size = sizeof(struct flexcan_mb) + CANFD_MAX_DLEN;
+ else
+ priv->mb_size = sizeof(struct flexcan_mb) + CAN_MAX_DLEN;
+ priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
+ (sizeof(priv->regs->mb[1]) / priv->mb_size);
+
+ if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)
+ priv->tx_mb_reserved =
+ flexcan_get_mb(priv, FLEXCAN_TX_MB_RESERVED_OFF_TIMESTAMP);
+ else
+ priv->tx_mb_reserved =
+ flexcan_get_mb(priv, FLEXCAN_TX_MB_RESERVED_OFF_FIFO);
+ priv->tx_mb_idx = priv->mb_count - 1;
+ priv->tx_mb = flexcan_get_mb(priv, priv->tx_mb_idx);
+ priv->tx_mask = FLEXCAN_IFLAG_MB(priv->tx_mb_idx);
+
+ priv->offload.mailbox_read = flexcan_mailbox_read;
+
+ if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP) {
+ priv->offload.mb_first = FLEXCAN_RX_MB_OFF_TIMESTAMP_FIRST;
+ priv->offload.mb_last = priv->mb_count - 2;
+
+ priv->rx_mask = GENMASK_ULL(priv->offload.mb_last,
+ priv->offload.mb_first);
+ err = can_rx_offload_add_timestamp(dev, &priv->offload);
+ } else {
+ priv->rx_mask = FLEXCAN_IFLAG_RX_FIFO_OVERFLOW |
+ FLEXCAN_IFLAG_RX_FIFO_AVAILABLE;
+ err = can_rx_offload_add_fifo(dev, &priv->offload,
+ FLEXCAN_NAPI_WEIGHT);
+ }
+
+ return err;
+}
+
static void flexcan_chip_interrupts_enable(const struct net_device *dev)
{
const struct flexcan_priv *priv = netdev_priv(dev);
@@ -1710,38 +1751,7 @@ static int flexcan_open(struct net_device *dev)
if (err)
goto out_transceiver_disable;
- if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
- priv->mb_size = sizeof(struct flexcan_mb) + CANFD_MAX_DLEN;
- else
- priv->mb_size = sizeof(struct flexcan_mb) + CAN_MAX_DLEN;
- priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
- (sizeof(priv->regs->mb[1]) / priv->mb_size);
-
- if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)
- priv->tx_mb_reserved =
- flexcan_get_mb(priv, FLEXCAN_TX_MB_RESERVED_OFF_TIMESTAMP);
- else
- priv->tx_mb_reserved =
- flexcan_get_mb(priv, FLEXCAN_TX_MB_RESERVED_OFF_FIFO);
- priv->tx_mb_idx = priv->mb_count - 1;
- priv->tx_mb = flexcan_get_mb(priv, priv->tx_mb_idx);
- priv->tx_mask = FLEXCAN_IFLAG_MB(priv->tx_mb_idx);
-
- priv->offload.mailbox_read = flexcan_mailbox_read;
-
- if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP) {
- priv->offload.mb_first = FLEXCAN_RX_MB_OFF_TIMESTAMP_FIRST;
- priv->offload.mb_last = priv->mb_count - 2;
-
- priv->rx_mask = GENMASK_ULL(priv->offload.mb_last,
- priv->offload.mb_first);
- err = can_rx_offload_add_timestamp(dev, &priv->offload);
- } else {
- priv->rx_mask = FLEXCAN_IFLAG_RX_FIFO_OVERFLOW |
- FLEXCAN_IFLAG_RX_FIFO_AVAILABLE;
- err = can_rx_offload_add_fifo(dev, &priv->offload,
- FLEXCAN_NAPI_WEIGHT);
- }
+ err = flexcan_rx_offload_setup(dev);
if (err)
goto out_free_irq;
--
2.29.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 4/5] can: flexcan: flexcan_open(): completely initialize controller before requesting IRQ
2020-11-19 8:52 [net-next PATCH 0/5] flexcan: fix hanging flexcan_close() Marc Kleine-Budde
` (2 preceding siblings ...)
2020-11-19 8:52 ` [PATCH 3/5] can: flexcan: flexcan_rx_offload_setup(): factor out mailbox and rx-offload setup into separate function Marc Kleine-Budde
@ 2020-11-19 8:52 ` Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 5/5] can: flexcan: flexcan_close(): change order if commands to properly shut down the controller Marc Kleine-Budde
4 siblings, 0 replies; 8+ messages in thread
From: Marc Kleine-Budde @ 2020-11-19 8:52 UTC (permalink / raw)
To: linux-can; +Cc: Joakim Zhang, kernel, Marc Kleine-Budde
This patch changes the order in which the flexcan controller is brought up
during flexcan_open(). It makes sure that the chip is completely initialized
before the IRQs are requested and finally enabled.
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/flexcan.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index cbce8a3d090c..0fb768dee99f 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1747,32 +1747,33 @@ static int flexcan_open(struct net_device *dev)
if (err)
goto out_close;
- err = request_irq(dev->irq, flexcan_irq, IRQF_SHARED, dev->name, dev);
+ err = flexcan_rx_offload_setup(dev);
if (err)
goto out_transceiver_disable;
- err = flexcan_rx_offload_setup(dev);
+ err = flexcan_chip_start(dev);
if (err)
- goto out_free_irq;
+ goto out_can_rx_offload_del;
- /* start chip and queuing */
- err = flexcan_chip_start(dev);
+ can_rx_offload_enable(&priv->offload);
+
+ err = request_irq(dev->irq, flexcan_irq, IRQF_SHARED, dev->name, dev);
if (err)
- goto out_offload_del;
+ goto out_can_rx_offload_disable;
flexcan_chip_interrupts_enable(dev);
can_led_event(dev, CAN_LED_EVENT_OPEN);
- can_rx_offload_enable(&priv->offload);
netif_start_queue(dev);
return 0;
- out_offload_del:
+ out_can_rx_offload_disable:
+ can_rx_offload_disable(&priv->offload);
+ flexcan_chip_stop(dev);
+ out_can_rx_offload_del:
can_rx_offload_del(&priv->offload);
- out_free_irq:
- free_irq(dev->irq, dev);
out_transceiver_disable:
flexcan_transceiver_disable(priv);
out_close:
--
2.29.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 5/5] can: flexcan: flexcan_close(): change order if commands to properly shut down the controller
2020-11-19 8:52 [net-next PATCH 0/5] flexcan: fix hanging flexcan_close() Marc Kleine-Budde
` (3 preceding siblings ...)
2020-11-19 8:52 ` [PATCH 4/5] can: flexcan: flexcan_open(): completely initialize controller before requesting IRQ Marc Kleine-Budde
@ 2020-11-19 8:52 ` Marc Kleine-Budde
2020-11-19 9:46 ` Joakim Zhang
4 siblings, 1 reply; 8+ messages in thread
From: Marc Kleine-Budde @ 2020-11-19 8:52 UTC (permalink / raw)
To: linux-can; +Cc: Joakim Zhang, kernel, Marc Kleine-Budde
There haven been reports, that the flexcan_close() soradically hangs during
simultanious ifdown, sending of CAN messages and probably open CAN bus:
| (__schedule) from [<808bbd34>] (schedule+0x90/0xb8)
| (schedule) from [<808bf274>] (schedule_timeout+0x1f8/0x24c)
| (schedule_timeout) from [<8016be44>] (msleep+0x18/0x1c)
| (msleep) from [<80746a64>] (napi_disable+0x60/0x70)
| (napi_disable) from [<8052fdd0>] (flexcan_close+0x2c/0x140)
| (flexcan_close) from [<80744930>] (__dev_close_many+0xb8/0xd8)
| (__dev_close_many) from [<8074db9c>] (__dev_change_flags+0xd0/0x1a0)
| (__dev_change_flags) from [<8074dc84>] (dev_change_flags+0x18/0x48)
| (dev_change_flags) from [<80760c24>] (do_setlink+0x44c/0x7b4)
| (do_setlink) from [<80761560>] (rtnl_newlink+0x374/0x68c)
I was unable to reproduce the issue, but a cleanup of the flexcan close
sequence has probably fixed the problem at the reporting user.
This patch changes the sequence in flexcan_close() to:
- stop the TX queue
- disable the interrupts on the chip level and wait via free_irq()
synchronously for the interrupt handler to finish
- disable RX offload, which disables synchronously NAPI
- disable the flexcan on the chip level
- free RX offload
- disable the transceiver
- close the CAN device
- disable the clocks
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/flexcan.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 0fb768dee99f..002e93f2b249 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1789,15 +1789,16 @@ static int flexcan_close(struct net_device *dev)
struct flexcan_priv *priv = netdev_priv(dev);
netif_stop_queue(dev);
+ flexcan_chip_interrupts_disable(dev);
+ free_irq(dev->irq, dev);
can_rx_offload_disable(&priv->offload);
flexcan_chip_stop_disable_on_error(dev);
flexcan_chip_interrupts_disable(dev);
can_rx_offload_del(&priv->offload);
- free_irq(dev->irq, dev);
flexcan_transceiver_disable(priv);
-
close_candev(dev);
+
pm_runtime_put(priv->dev);
can_led_event(dev, CAN_LED_EVENT_STOP);
--
2.29.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH 5/5] can: flexcan: flexcan_close(): change order if commands to properly shut down the controller
2020-11-19 8:52 ` [PATCH 5/5] can: flexcan: flexcan_close(): change order if commands to properly shut down the controller Marc Kleine-Budde
@ 2020-11-19 9:46 ` Joakim Zhang
2020-11-19 9:49 ` Marc Kleine-Budde
0 siblings, 1 reply; 8+ messages in thread
From: Joakim Zhang @ 2020-11-19 9:46 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: kernel
> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2020年11月19日 16:53
> To: linux-can@vger.kernel.org
> Cc: Joakim Zhang <qiangqing.zhang@nxp.com>; kernel@pengutronix.de; Marc
> Kleine-Budde <mkl@pengutronix.de>
> Subject: [PATCH 5/5] can: flexcan: flexcan_close(): change order if commands
> to properly shut down the controller
>
> There haven been reports, that the flexcan_close() soradically hangs during
> simultanious ifdown, sending of CAN messages and probably open CAN bus:
>
> | (__schedule) from [<808bbd34>] (schedule+0x90/0xb8)
> | (schedule) from [<808bf274>] (schedule_timeout+0x1f8/0x24c)
> | (schedule_timeout) from [<8016be44>] (msleep+0x18/0x1c)
> | (msleep) from [<80746a64>] (napi_disable+0x60/0x70)
> | (napi_disable) from [<8052fdd0>] (flexcan_close+0x2c/0x140)
> | (flexcan_close) from [<80744930>] (__dev_close_many+0xb8/0xd8)
> | (__dev_close_many) from [<8074db9c>] (__dev_change_flags+0xd0/0x1a0)
> | (__dev_change_flags) from [<8074dc84>] (dev_change_flags+0x18/0x48)
> | (dev_change_flags) from [<80760c24>] (do_setlink+0x44c/0x7b4)
> | (do_setlink) from [<80761560>] (rtnl_newlink+0x374/0x68c)
>
> I was unable to reproduce the issue, but a cleanup of the flexcan close
> sequence has probably fixed the problem at the reporting user.
>
> This patch changes the sequence in flexcan_close() to:
> - stop the TX queue
> - disable the interrupts on the chip level and wait via free_irq()
> synchronously for the interrupt handler to finish
> - disable RX offload, which disables synchronously NAPI
> - disable the flexcan on the chip level
> - free RX offload
> - disable the transceiver
> - close the CAN device
> - disable the clocks
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
> drivers/net/can/flexcan.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
> 0fb768dee99f..002e93f2b249 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -1789,15 +1789,16 @@ static int flexcan_close(struct net_device *dev)
> struct flexcan_priv *priv = netdev_priv(dev);
>
> netif_stop_queue(dev);
> + flexcan_chip_interrupts_disable(dev);
> + free_irq(dev->irq, dev);
> can_rx_offload_disable(&priv->offload);
> flexcan_chip_stop_disable_on_error(dev);
> flexcan_chip_interrupts_disable(dev);
Hi Marc,
Is it a special treatment? flexcan_chip_interrupts_disable called twice?
flexcan_chip_interrupts_disable(dev);
free_irq(dev->irq, dev);
can_rx_offload_disable(&priv->offload);
flexcan_chip_stop_disable_on_error(dev);
flexcan_chip_interrupts_disable(dev);
Best Regards,
Joakim Zhang
> can_rx_offload_del(&priv->offload);
> - free_irq(dev->irq, dev);
> flexcan_transceiver_disable(priv);
> -
> close_candev(dev);
> +
> pm_runtime_put(priv->dev);
>
> can_led_event(dev, CAN_LED_EVENT_STOP);
> --
> 2.29.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 5/5] can: flexcan: flexcan_close(): change order if commands to properly shut down the controller
2020-11-19 9:46 ` Joakim Zhang
@ 2020-11-19 9:49 ` Marc Kleine-Budde
0 siblings, 0 replies; 8+ messages in thread
From: Marc Kleine-Budde @ 2020-11-19 9:49 UTC (permalink / raw)
To: Joakim Zhang, linux-can; +Cc: kernel
[-- Attachment #1.1: Type: text/plain, Size: 3219 bytes --]
On 11/19/20 10:46 AM, Joakim Zhang wrote:
>
>> -----Original Message-----
>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>> Sent: 2020年11月19日 16:53
>> To: linux-can@vger.kernel.org
>> Cc: Joakim Zhang <qiangqing.zhang@nxp.com>; kernel@pengutronix.de; Marc
>> Kleine-Budde <mkl@pengutronix.de>
>> Subject: [PATCH 5/5] can: flexcan: flexcan_close(): change order if commands
>> to properly shut down the controller
>>
>> There haven been reports, that the flexcan_close() soradically hangs during
>> simultanious ifdown, sending of CAN messages and probably open CAN bus:
>>
>> | (__schedule) from [<808bbd34>] (schedule+0x90/0xb8)
>> | (schedule) from [<808bf274>] (schedule_timeout+0x1f8/0x24c)
>> | (schedule_timeout) from [<8016be44>] (msleep+0x18/0x1c)
>> | (msleep) from [<80746a64>] (napi_disable+0x60/0x70)
>> | (napi_disable) from [<8052fdd0>] (flexcan_close+0x2c/0x140)
>> | (flexcan_close) from [<80744930>] (__dev_close_many+0xb8/0xd8)
>> | (__dev_close_many) from [<8074db9c>] (__dev_change_flags+0xd0/0x1a0)
>> | (__dev_change_flags) from [<8074dc84>] (dev_change_flags+0x18/0x48)
>> | (dev_change_flags) from [<80760c24>] (do_setlink+0x44c/0x7b4)
>> | (do_setlink) from [<80761560>] (rtnl_newlink+0x374/0x68c)
>>
>> I was unable to reproduce the issue, but a cleanup of the flexcan close
>> sequence has probably fixed the problem at the reporting user.
>>
>> This patch changes the sequence in flexcan_close() to:
>> - stop the TX queue
>> - disable the interrupts on the chip level and wait via free_irq()
>> synchronously for the interrupt handler to finish
>> - disable RX offload, which disables synchronously NAPI
>> - disable the flexcan on the chip level
>> - free RX offload
>> - disable the transceiver
>> - close the CAN device
>> - disable the clocks
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>> drivers/net/can/flexcan.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
>> 0fb768dee99f..002e93f2b249 100644
>> --- a/drivers/net/can/flexcan.c
>> +++ b/drivers/net/can/flexcan.c
>> @@ -1789,15 +1789,16 @@ static int flexcan_close(struct net_device *dev)
>> struct flexcan_priv *priv = netdev_priv(dev);
>>
>> netif_stop_queue(dev);
>> + flexcan_chip_interrupts_disable(dev);
>> + free_irq(dev->irq, dev);
>> can_rx_offload_disable(&priv->offload);
>> flexcan_chip_stop_disable_on_error(dev);
>> flexcan_chip_interrupts_disable(dev);
>
> Hi Marc,
>
> Is it a special treatment? flexcan_chip_interrupts_disable called twice?
Thanks for catching this. It's a mistake, will fix.
> flexcan_chip_interrupts_disable(dev);
> free_irq(dev->irq, dev);
> can_rx_offload_disable(&priv->offload);
> flexcan_chip_stop_disable_on_error(dev);
> flexcan_chip_interrupts_disable(dev);
Thanks for the review,
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 |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-11-19 9:50 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-19 8:52 [net-next PATCH 0/5] flexcan: fix hanging flexcan_close() Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 1/5] can: flexcan: factor out enabling and disabling of interrupts into separate function Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 2/5] can: flexcan: move enabling/disabling of interrupts from flexcan_chip_{start,stop}() to callers Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 3/5] can: flexcan: flexcan_rx_offload_setup(): factor out mailbox and rx-offload setup into separate function Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 4/5] can: flexcan: flexcan_open(): completely initialize controller before requesting IRQ Marc Kleine-Budde
2020-11-19 8:52 ` [PATCH 5/5] can: flexcan: flexcan_close(): change order if commands to properly shut down the controller Marc Kleine-Budde
2020-11-19 9:46 ` Joakim Zhang
2020-11-19 9:49 ` Marc Kleine-Budde
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).