* [PATCH can v2] can: flexcan: flexcan_mailbox_read() fix return value for drop = true
@ 2022-08-11 9:42 Marc Kleine-Budde
2022-08-12 9:19 ` Uwe Kleine-König
2022-08-12 10:46 ` Thorsten Scherer
0 siblings, 2 replies; 3+ messages in thread
From: Marc Kleine-Budde @ 2022-08-11 9:42 UTC (permalink / raw)
To: linux-can; +Cc: Marc Kleine-Budde, Uwe Kleine-König
The following happened on an i.MX25 using flexcan with many packets on
the bus:
The rx-offload queue reached a length more than skb_queue_len_max. In
can_rx_offload_offload_one() the drop variable was set to true which
made the call to .mailbox_read() (here: flexcan_mailbox_read()) to
_always_ return ERR_PTR(-ENOBUFS) and drop the rx'ed CAN frame. So
can_rx_offload_offload_one() returned ERR_PTR(-ENOBUFS), too.
can_rx_offload_irq_offload_fifo() looks as follows:
| while (1) {
| skb = can_rx_offload_offload_one(offload, 0);
| if (IS_ERR(skb))
| continue;
| if (!skb)
| break;
| ...
| }
The flexcan driver wrongly always returns ERR_PTR(-ENOBUFS) if drop is
requested, even if there is no CAN frame pending. As the i.MX25 is a
single core CPU, while the rx-offload processing is active, there is
no thread to process packets from the offload queue. So the queue
doesn't get any shorter and this results is a tight loop.
Instead of always returning ERR_PTR(-ENOBUFS) if drop is requested,
return NULL if no CAN frame is pending.
Fixes: 4e9c9484b085 ("can: rx-offload: Prepare for CAN FD support")
Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/flexcan/flexcan-core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
index f857968efed7..ccb438eca517 100644
--- a/drivers/net/can/flexcan/flexcan-core.c
+++ b/drivers/net/can/flexcan/flexcan-core.c
@@ -941,11 +941,6 @@ static struct sk_buff *flexcan_mailbox_read(struct can_rx_offload *offload,
u32 reg_ctrl, reg_id, reg_iflag1;
int i;
- if (unlikely(drop)) {
- skb = ERR_PTR(-ENOBUFS);
- goto mark_as_read;
- }
-
mb = flexcan_get_mb(priv, n);
if (priv->devtype_data.quirks & FLEXCAN_QUIRK_USE_RX_MAILBOX) {
@@ -974,6 +969,11 @@ static struct sk_buff *flexcan_mailbox_read(struct can_rx_offload *offload,
reg_ctrl = priv->read(&mb->can_ctrl);
}
+ if (unlikely(drop)) {
+ skb = ERR_PTR(-ENOBUFS);
+ goto mark_as_read;
+ }
+
if (reg_ctrl & FLEXCAN_MB_CNT_EDL)
skb = alloc_canfd_skb(offload->dev, &cfd);
else
--
2.35.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH can v2] can: flexcan: flexcan_mailbox_read() fix return value for drop = true
2022-08-11 9:42 [PATCH can v2] can: flexcan: flexcan_mailbox_read() fix return value for drop = true Marc Kleine-Budde
@ 2022-08-12 9:19 ` Uwe Kleine-König
2022-08-12 10:46 ` Thorsten Scherer
1 sibling, 0 replies; 3+ messages in thread
From: Uwe Kleine-König @ 2022-08-12 9:19 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can
[-- Attachment #1: Type: text/plain, Size: 1729 bytes --]
Hello,
On Thu, Aug 11, 2022 at 11:42:54AM +0200, Marc Kleine-Budde wrote:
> The following happened on an i.MX25 using flexcan with many packets on
> the bus:
>
> The rx-offload queue reached a length more than skb_queue_len_max. In
> can_rx_offload_offload_one() the drop variable was set to true which
> made the call to .mailbox_read() (here: flexcan_mailbox_read()) to
> _always_ return ERR_PTR(-ENOBUFS) and drop the rx'ed CAN frame. So
> can_rx_offload_offload_one() returned ERR_PTR(-ENOBUFS), too.
>
> can_rx_offload_irq_offload_fifo() looks as follows:
>
> | while (1) {
> | skb = can_rx_offload_offload_one(offload, 0);
> | if (IS_ERR(skb))
> | continue;
> | if (!skb)
> | break;
> | ...
> | }
>
> The flexcan driver wrongly always returns ERR_PTR(-ENOBUFS) if drop is
> requested, even if there is no CAN frame pending. As the i.MX25 is a
> single core CPU, while the rx-offload processing is active, there is
> no thread to process packets from the offload queue. So the queue
> doesn't get any shorter and this results is a tight loop.
>
> Instead of always returning ERR_PTR(-ENOBUFS) if drop is requested,
> return NULL if no CAN frame is pending.
>
> Fixes: 4e9c9484b085 ("can: rx-offload: Prepare for CAN FD support")
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
That change fixes the problem I saw.
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Thanks for your followup!
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH can v2] can: flexcan: flexcan_mailbox_read() fix return value for drop = true
2022-08-11 9:42 [PATCH can v2] can: flexcan: flexcan_mailbox_read() fix return value for drop = true Marc Kleine-Budde
2022-08-12 9:19 ` Uwe Kleine-König
@ 2022-08-12 10:46 ` Thorsten Scherer
1 sibling, 0 replies; 3+ messages in thread
From: Thorsten Scherer @ 2022-08-12 10:46 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, Uwe Kleine-König
Hello,
On Thu, Aug 11, 2022 at 11:42:54AM +0200, Marc Kleine-Budde wrote:
> The following happened on an i.MX25 using flexcan with many packets on
> the bus:
>
> The rx-offload queue reached a length more than skb_queue_len_max. In
> can_rx_offload_offload_one() the drop variable was set to true which
> made the call to .mailbox_read() (here: flexcan_mailbox_read()) to
> _always_ return ERR_PTR(-ENOBUFS) and drop the rx'ed CAN frame. So
> can_rx_offload_offload_one() returned ERR_PTR(-ENOBUFS), too.
>
> can_rx_offload_irq_offload_fifo() looks as follows:
>
> | while (1) {
> | skb = can_rx_offload_offload_one(offload, 0);
> | if (IS_ERR(skb))
> | continue;
> | if (!skb)
> | break;
> | ...
> | }
>
> The flexcan driver wrongly always returns ERR_PTR(-ENOBUFS) if drop is
> requested, even if there is no CAN frame pending. As the i.MX25 is a
> single core CPU, while the rx-offload processing is active, there is
> no thread to process packets from the offload queue. So the queue
> doesn't get any shorter and this results is a tight loop.
>
> Instead of always returning ERR_PTR(-ENOBUFS) if drop is requested,
> return NULL if no CAN frame is pending.
>
> Fixes: 4e9c9484b085 ("can: rx-offload: Prepare for CAN FD support")
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
This patch (rebased onto v5.15) fixes the issue on our i.MX25 board.
Tested-by: Thorsten Scherer <t.scherer@eckelmann.de>
Thank you!
Best regards
Thorsten
> ---
> drivers/net/can/flexcan/flexcan-core.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index f857968efed7..ccb438eca517 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c
> @@ -941,11 +941,6 @@ static struct sk_buff *flexcan_mailbox_read(struct can_rx_offload *offload,
> u32 reg_ctrl, reg_id, reg_iflag1;
> int i;
>
> - if (unlikely(drop)) {
> - skb = ERR_PTR(-ENOBUFS);
> - goto mark_as_read;
> - }
> -
> mb = flexcan_get_mb(priv, n);
>
> if (priv->devtype_data.quirks & FLEXCAN_QUIRK_USE_RX_MAILBOX) {
> @@ -974,6 +969,11 @@ static struct sk_buff *flexcan_mailbox_read(struct can_rx_offload *offload,
> reg_ctrl = priv->read(&mb->can_ctrl);
> }
>
> + if (unlikely(drop)) {
> + skb = ERR_PTR(-ENOBUFS);
> + goto mark_as_read;
> + }
> +
> if (reg_ctrl & FLEXCAN_MB_CNT_EDL)
> skb = alloc_canfd_skb(offload->dev, &cfd);
> else
> --
> 2.35.1
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-08-12 10:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-11 9:42 [PATCH can v2] can: flexcan: flexcan_mailbox_read() fix return value for drop = true Marc Kleine-Budde
2022-08-12 9:19 ` Uwe Kleine-König
2022-08-12 10:46 ` Thorsten Scherer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.