From a09721dd47c8468b3f2fdd73f40422699ffe26dd Mon Sep 17 00:00:00 2001 From: Marc Kleine-Budde Date: Thu, 11 Aug 2022 10:25:44 +0200 Subject: [PATCH 1/3] can: flexcan: flexcan_mailbox_read() fix return value for drop = true MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Changes since v1: https://lore.kernel.org/all/20220810144536.389237-1-u.kleine-koenig@pengutronix.de - don't break in can_rx_offload_irq_offload_fifo() in case of an error, return NULL in flexcan_mailbox_read() in case of no pending CAN frame instead Fixes: 4e9c9484b085 ("can: rx-offload: Prepare for CAN FD support") Link: https://lore.kernel.org/all/20220811094254.1864367-1-mkl@pengutronix.de Cc: stable@vger.kernel.org # v5.5 Suggested-by: Uwe Kleine-König Reviewed-by: Uwe Kleine-König Tested-by: Thorsten Scherer Signed-off-by: Marc Kleine-Budde --- 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 From 5440428b3da65408dba0241985acb7a05258b85e Mon Sep 17 00:00:00 2001 From: Marc Kleine-Budde Date: Tue, 20 Sep 2022 11:40:56 +0200 Subject: [PATCH 2/3] can: gs_usb: gs_can_open(): fix race dev->can.state condition The dev->can.state is set to CAN_STATE_ERROR_ACTIVE, after the device has been started. On busy networks the CAN controller might receive CAN frame between and go into an error state before the dev->can.state is assigned. Assign dev->can.state before starting the controller to close the race window. Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices") Link: https://lore.kernel.org/all/20220920195216.232481-1-mkl@pengutronix.de Signed-off-by: Marc Kleine-Budde --- drivers/net/can/usb/gs_usb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c index baf749c8cda3..553e2c7b0b12 100644 --- a/drivers/net/can/usb/gs_usb.c +++ b/drivers/net/can/usb/gs_usb.c @@ -824,6 +824,7 @@ static int gs_can_open(struct net_device *netdev) flags |= GS_CAN_MODE_TRIPLE_SAMPLE; /* finally start device */ + dev->can.state = CAN_STATE_ERROR_ACTIVE; dm->mode = cpu_to_le32(GS_CAN_MODE_START); dm->flags = cpu_to_le32(flags); rc = usb_control_msg(interface_to_usbdev(dev->iface), @@ -835,13 +836,12 @@ static int gs_can_open(struct net_device *netdev) if (rc < 0) { netdev_err(netdev, "Couldn't start device (err=%d)\n", rc); kfree(dm); + dev->can.state = CAN_STATE_STOPPED; return rc; } kfree(dm); - dev->can.state = CAN_STATE_ERROR_ACTIVE; - parent->active_channels++; if (!(dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)) netif_start_queue(netdev); From 0f2211f1cf58876f00d2fd8839e0fdadf0786894 Mon Sep 17 00:00:00 2001 From: Marc Kleine-Budde Date: Thu, 18 Aug 2022 14:10:08 +0200 Subject: [PATCH 3/3] can: gs_usb: gs_usb_set_phys_id(): return with error if identify is not supported Until commit 409c188c57cd ("can: tree-wide: advertise software timestamping capabilities") the ethtool_ops was only assigned for devices which support the GS_CAN_FEATURE_IDENTIFY feature. That commit assigns ethtool_ops unconditionally. This results on controllers without GS_CAN_FEATURE_IDENTIFY support for the following ethtool error: | $ ethtool -p can0 1 | Cannot identify NIC: Broken pipe Restore the correct error value by checking for GS_CAN_FEATURE_IDENTIFY in the gs_usb_set_phys_id() function. | $ ethtool -p can0 1 | Cannot identify NIC: Operation not supported While there use the variable "netdev" for the "struct net_device" pointer and "dev" for the "struct gs_can" pointer as in the rest of the driver. Fixes: 409c188c57cd ("can: tree-wide: advertise software timestamping capabilities") Link: http://lore.kernel.org/all/20220818143853.2671854-1-mkl@pengutronix.de Cc: Vincent Mailhol Signed-off-by: Marc Kleine-Budde --- drivers/net/can/usb/gs_usb.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c index 553e2c7b0b12..c1ff3c046d62 100644 --- a/drivers/net/can/usb/gs_usb.c +++ b/drivers/net/can/usb/gs_usb.c @@ -925,17 +925,21 @@ static int gs_usb_set_identify(struct net_device *netdev, bool do_identify) } /* blink LED's for finding the this interface */ -static int gs_usb_set_phys_id(struct net_device *dev, +static int gs_usb_set_phys_id(struct net_device *netdev, enum ethtool_phys_id_state state) { + const struct gs_can *dev = netdev_priv(netdev); int rc = 0; + if (!(dev->feature & GS_CAN_FEATURE_IDENTIFY)) + return -EOPNOTSUPP; + switch (state) { case ETHTOOL_ID_ACTIVE: - rc = gs_usb_set_identify(dev, GS_CAN_IDENTIFY_ON); + rc = gs_usb_set_identify(netdev, GS_CAN_IDENTIFY_ON); break; case ETHTOOL_ID_INACTIVE: - rc = gs_usb_set_identify(dev, GS_CAN_IDENTIFY_OFF); + rc = gs_usb_set_identify(netdev, GS_CAN_IDENTIFY_OFF); break; default: break; @@ -1072,9 +1076,10 @@ static struct gs_can *gs_make_candev(unsigned int channel, dev->feature |= GS_CAN_FEATURE_REQ_USB_QUIRK_LPC546XX | GS_CAN_FEATURE_QUIRK_BREQ_CANTACT_PRO; - if (le32_to_cpu(dconf->sw_version) > 1) - if (feature & GS_CAN_FEATURE_IDENTIFY) - netdev->ethtool_ops = &gs_usb_ethtool_ops; + /* GS_CAN_FEATURE_IDENTIFY is only supported for sw_version > 1 */ + if (!(le32_to_cpu(dconf->sw_version) > 1 && + feature & GS_CAN_FEATURE_IDENTIFY)) + dev->feature &= ~GS_CAN_FEATURE_IDENTIFY; kfree(bt_const);