From c8129487441e11617fab9770d9f98d9ae8fc7b3a Mon Sep 17 00:00:00 2001 From: Marc Kleine-Budde Date: Fri, 2 Apr 2021 12:05:39 +0200 Subject: can: skb: alloc_can{,fd}_skb(): set "cf" to NULL if skb allocation fails The handling of CAN bus errors typically consist of allocating a CAN error SKB using alloc_can_err_skb() followed by stats handling and filling the error details in the newly allocated CAN error SKB. Even if the allocation of the SKB fails the stats handling should not be skipped. The common pattern in CAN drivers is to allocate the skb and work on the struct can_frame pointer "cf", if it has been assigned by alloc_can_err_skb(). | skb = alloc_can_err_skb(priv->ndev, &cf); | | /* RX errors */ | if (bdiag1 & (MCP251XFD_REG_BDIAG1_DCRCERR | | MCP251XFD_REG_BDIAG1_NCRCERR)) { | netdev_dbg(priv->ndev, "CRC error\n"); | | stats->rx_errors++; | if (cf) | cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ; | } In case of an OOM alloc_can_err_skb() returns NULL, but doesn't set "cf" to NULL as well. For the above pattern to work the "cf" has to be initialized to NULL, which is easily forgotten. To solve this kind of problems, set "cf" to NULL if alloc_can_err_skb() returns NULL. Link: https://lore.kernel.org/r/20210402102245.1512583-1-mkl@pengutronix.de Suggested-by: Vincent MAILHOL Reviewed-by: Vincent Mailhol Signed-off-by: Marc Kleine-Budde --- drivers/net/can/dev/skb.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'drivers') diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c index 387c0bc0fb9c..61660248c69e 100644 --- a/drivers/net/can/dev/skb.c +++ b/drivers/net/can/dev/skb.c @@ -183,8 +183,11 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf) skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) + sizeof(struct can_frame)); - if (unlikely(!skb)) + if (unlikely(!skb)) { + *cf = NULL; + return NULL; + } skb->protocol = htons(ETH_P_CAN); skb->pkt_type = PACKET_BROADCAST; @@ -211,8 +214,11 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev, skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) + sizeof(struct canfd_frame)); - if (unlikely(!skb)) + if (unlikely(!skb)) { + *cfd = NULL; + return NULL; + } skb->protocol = htons(ETH_P_CANFD); skb->pkt_type = PACKET_BROADCAST; -- cgit v1.2.3 From 644022b1de9efc901494b8931c78700d49d35d3d Mon Sep 17 00:00:00 2001 From: Marc Kleine-Budde Date: Thu, 1 Apr 2021 10:37:31 +0200 Subject: can: m_can: m_can_receive_skb(): add missing error handling to can_rx_offload_queue_sorted() call In commit 1be37d3b0414 ("can: m_can: fix periph RX path: use rx-offload to ensure skbs are sent from softirq context") the RX path for peripherals (i.e. SPI based m_can controllers) was converted to the rx-offload infrastructure. However, the error handling for can_rx_offload_queue_sorted() was forgotten. can_rx_offload_queue_sorted() will return with an error if the internal queue is full. This patch adds the missing error handling, by increasing the rx_fifo_errors. Fixes: 1be37d3b0414 ("can: m_can: fix periph RX path: use rx-offload to ensure skbs are sent from softirq context") Link: https://lore.kernel.org/r/20210401084515.1455013-1-mkl@pengutronix.de Reported-by: coverity-bot Addresses-Coverity-ID: 1503583 ("Error handling issues") Reviewed-by: Kees Cook Cc: Torin Cooper-Bennun Signed-off-by: Marc Kleine-Budde --- drivers/net/can/m_can/m_can.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) (limited to 'drivers') diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 890ed826a355..34073cd077e4 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -466,10 +466,17 @@ static void m_can_receive_skb(struct m_can_classdev *cdev, struct sk_buff *skb, u32 timestamp) { - if (cdev->is_peripheral) - can_rx_offload_queue_sorted(&cdev->offload, skb, timestamp); - else + if (cdev->is_peripheral) { + struct net_device_stats *stats = &cdev->net->stats; + int err; + + err = can_rx_offload_queue_sorted(&cdev->offload, skb, + timestamp); + if (err) + stats->rx_fifo_errors++; + } else { netif_receive_skb(skb); + } } static void m_can_read_fifo(struct net_device *dev, u32 rxfs) -- cgit v1.2.3 From 8dc987519ae97fd3b994871720ae5f089654b599 Mon Sep 17 00:00:00 2001 From: Marc Kleine-Budde Date: Wed, 24 Jul 2019 11:51:32 +0200 Subject: can: c_can: remove unused enum BOSCH_C_CAN_PLATFORM This patch removes the unused enum BOSCH_C_CAN_PLATFORM. Link: https://lore.kernel.org/r/20210406110617.1865592-2-mkl@pengutronix.de Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.h | 1 - 1 file changed, 1 deletion(-) (limited to 'drivers') diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h index 8acedd9e63a7..06045f610f0e 100644 --- a/drivers/net/can/c_can/c_can.h +++ b/drivers/net/can/c_can/c_can.h @@ -149,7 +149,6 @@ static const u16 __maybe_unused reg_map_d_can[] = { }; enum c_can_dev_id { - BOSCH_C_CAN_PLATFORM, BOSCH_C_CAN, BOSCH_D_CAN, }; -- cgit v1.2.3 From 0084e298acfe643c68daf0a316bb27618ec2a852 Mon Sep 17 00:00:00 2001 From: Marc Kleine-Budde Date: Sun, 13 Dec 2020 17:25:15 +0100 Subject: can: mcp251xfd: add BQL support This patch re-adds BQL support to the driver. Support for netdev_xmit_more() will be added in a separate patch series. Link: https://lore.kernel.org/r/20210406110617.1865592-3-mkl@pengutronix.de Cc: Manivannan Sadhasivam Cc: Thomas Kopp Signed-off-by: Marc Kleine-Budde --- drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) (limited to 'drivers') diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c index 142eb4506b55..970dc570e7a5 100644 --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c @@ -335,6 +335,8 @@ static void mcp251xfd_ring_init(struct mcp251xfd_priv *priv) u8 len; int i, j; + netdev_reset_queue(priv->ndev); + /* TEF */ tef_ring = priv->tef; tef_ring->head = 0; @@ -1262,7 +1264,8 @@ mcp251xfd_handle_tefif_recover(const struct mcp251xfd_priv *priv, const u32 seq) static int mcp251xfd_handle_tefif_one(struct mcp251xfd_priv *priv, - const struct mcp251xfd_hw_tef_obj *hw_tef_obj) + const struct mcp251xfd_hw_tef_obj *hw_tef_obj, + unsigned int *frame_len_ptr) { struct net_device_stats *stats = &priv->ndev->stats; struct sk_buff *skb; @@ -1288,8 +1291,8 @@ mcp251xfd_handle_tefif_one(struct mcp251xfd_priv *priv, mcp251xfd_skb_set_timestamp(priv, skb, hw_tef_obj->ts); stats->tx_bytes += can_rx_offload_get_echo_skb(&priv->offload, - tef_tail, - hw_tef_obj->ts, NULL); + tef_tail, hw_tef_obj->ts, + frame_len_ptr); stats->tx_packets++; priv->tef->tail++; @@ -1347,6 +1350,7 @@ mcp251xfd_tef_obj_read(const struct mcp251xfd_priv *priv, static int mcp251xfd_handle_tefif(struct mcp251xfd_priv *priv) { struct mcp251xfd_hw_tef_obj hw_tef_obj[MCP251XFD_TX_OBJ_NUM_MAX]; + unsigned int total_frame_len = 0; u8 tef_tail, len, l; int err, i; @@ -1368,7 +1372,9 @@ static int mcp251xfd_handle_tefif(struct mcp251xfd_priv *priv) } for (i = 0; i < len; i++) { - err = mcp251xfd_handle_tefif_one(priv, &hw_tef_obj[i]); + unsigned int frame_len = 0; + + err = mcp251xfd_handle_tefif_one(priv, &hw_tef_obj[i], &frame_len); /* -EAGAIN means the Sequence Number in the TEF * doesn't match our tef_tail. This can happen if we * read the TEF objects too early. Leave loop let the @@ -1378,6 +1384,8 @@ static int mcp251xfd_handle_tefif(struct mcp251xfd_priv *priv) goto out_netif_wake_queue; if (err) return err; + + total_frame_len += frame_len; } out_netif_wake_queue: @@ -1403,6 +1411,7 @@ static int mcp251xfd_handle_tefif(struct mcp251xfd_priv *priv) return err; tx_ring->tail += len; + netdev_completed_queue(priv->ndev, len, total_frame_len); err = mcp251xfd_check_tef_tail(priv); if (err) @@ -2446,6 +2455,7 @@ static netdev_tx_t mcp251xfd_start_xmit(struct sk_buff *skb, struct mcp251xfd_priv *priv = netdev_priv(ndev); struct mcp251xfd_tx_ring *tx_ring = priv->tx; struct mcp251xfd_tx_obj *tx_obj; + unsigned int frame_len; u8 tx_head; int err; @@ -2464,7 +2474,10 @@ static netdev_tx_t mcp251xfd_start_xmit(struct sk_buff *skb, if (mcp251xfd_get_tx_free(tx_ring) == 0) netif_stop_queue(ndev); - can_put_echo_skb(skb, ndev, tx_head, 0); + frame_len = can_skb_get_frame_len(skb); + err = can_put_echo_skb(skb, ndev, tx_head, frame_len); + if (!err) + netdev_sent_queue(priv->ndev, frame_len); err = mcp251xfd_tx_obj_write(priv, tx_obj); if (err) -- cgit v1.2.3 From ef7a8c3e75994f2bdaa27b481de39ea000119d11 Mon Sep 17 00:00:00 2001 From: Marc Kleine-Budde Date: Mon, 15 Mar 2021 08:59:15 +0100 Subject: can: mcp251xfd: mcp251xfd_regmap_crc_read_one(): Factor out crc check into separate function This patch factors out the crc check into a separate function. This is preparation for the next patch. Link: https://lore.kernel.org/r/20210406110617.1865592-4-mkl@pengutronix.de Cc: Manivannan Sadhasivam Cc: Thomas Kopp Signed-off-by: Marc Kleine-Budde --- drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 30 ++++++++++++++++-------- 1 file changed, 20 insertions(+), 10 deletions(-) (limited to 'drivers') diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c index 314f868b3465..35557ac43c03 100644 --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c @@ -232,13 +232,31 @@ mcp251xfd_regmap_crc_write(void *context, count - data_offset); } +static int +mcp251xfd_regmap_crc_read_check_crc(const struct mcp251xfd_map_buf_crc * const buf_rx, + const struct mcp251xfd_map_buf_crc * const buf_tx, + unsigned int data_len) +{ + u16 crc_received, crc_calculated; + + crc_received = get_unaligned_be16(buf_rx->data + data_len); + crc_calculated = mcp251xfd_crc16_compute2(&buf_tx->cmd, + sizeof(buf_tx->cmd), + buf_rx->data, + data_len); + if (crc_received != crc_calculated) + return -EBADMSG; + + return 0; +} + + static int mcp251xfd_regmap_crc_read_one(struct mcp251xfd_priv *priv, struct spi_message *msg, unsigned int data_len) { const struct mcp251xfd_map_buf_crc *buf_rx = priv->map_buf_crc_rx; const struct mcp251xfd_map_buf_crc *buf_tx = priv->map_buf_crc_tx; - u16 crc_received, crc_calculated; int err; BUILD_BUG_ON(sizeof(buf_rx->cmd) != sizeof(__be16) + sizeof(u8)); @@ -248,15 +266,7 @@ mcp251xfd_regmap_crc_read_one(struct mcp251xfd_priv *priv, if (err) return err; - crc_received = get_unaligned_be16(buf_rx->data + data_len); - crc_calculated = mcp251xfd_crc16_compute2(&buf_tx->cmd, - sizeof(buf_tx->cmd), - buf_rx->data, - data_len); - if (crc_received != crc_calculated) - return -EBADMSG; - - return 0; + return mcp251xfd_regmap_crc_read_check_crc(buf_rx, buf_tx, data_len); } static int -- cgit v1.2.3 From c7eb923c3caf4c6a183465cc012dc368b199a4b2 Mon Sep 17 00:00:00 2001 From: Marc Kleine-Budde Date: Mon, 15 Mar 2021 10:59:09 +0100 Subject: can: mcp251xfd: mcp251xfd_regmap_crc_read(): work around broken CRC on TBC register MCP251XFD_REG_TBC is the time base counter register. It increments once per SYS clock tick, which is 20 or 40 MHz. Observation shows that if the lowest byte (which is transferred first on the SPI bus) of that register is 0x00 or 0x80 the calculated CRC doesn't always match the transferred one. To reproduce this problem let the driver read the TBC register in a high frequency. This can be done by attaching only the mcp251xfd CAN controller to a valid terminated CAN bus and send a single CAN frame. As there are no other CAN controller on the bus, the sent CAN frame is not ACKed and the mcp251xfd repeats it. If user space enables the bus error reporting, each of the NACK errors is reported with a time stamp (which is read from the TBC register) to user space. $ ip link set can0 down $ ip link set can0 up type can bitrate 500000 berr-reporting on $ cansend can0 4FF#ff.01.00.00.00.00.00.00 This leads to several error messages per second: | mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4, data=00 3a 86 da, CRC=0x7753) retrying. | mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4, data=80 01 b4 da, CRC=0x5830) retrying. | mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4, data=00 e9 23 db, CRC=0xa723) retrying. | mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4, data=00 8a 30 db, CRC=0x4a9c) retrying. | mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4, data=80 f3 43 db, CRC=0x66d2) retrying. If the highest bit in the lowest byte is flipped the transferred CRC matches the calculated one. We assume for now the CRC calculation in the chip works on wrong data and the transferred data is correct. This patch implements the following workaround: - If a CRC read error on the TBC register is detected and the lowest byte is 0x00 or 0x80, the highest bit of the lowest byte is flipped and the CRC is calculated again. - If the CRC now matches, the _original_ data is passed to the reader. For now we assume transferred data was OK. Link: https://lore.kernel.org/r/20210406110617.1865592-5-mkl@pengutronix.de Cc: Manivannan Sadhasivam Cc: Thomas Kopp Signed-off-by: Marc Kleine-Budde --- drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 34 ++++++++++++++++++++++++ 1 file changed, 34 insertions(+) (limited to 'drivers') diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c index 35557ac43c03..297491516a26 100644 --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c @@ -321,6 +321,40 @@ mcp251xfd_regmap_crc_read(void *context, if (err != -EBADMSG) return err; + /* MCP251XFD_REG_TBC is the time base counter + * register. It increments once per SYS clock tick, + * which is 20 or 40 MHz. + * + * Observation shows that if the lowest byte (which is + * transferred first on the SPI bus) of that register + * is 0x00 or 0x80 the calculated CRC doesn't always + * match the transferred one. + * + * If the highest bit in the lowest byte is flipped + * the transferred CRC matches the calculated one. We + * assume for now the CRC calculation in the chip + * works on wrong data and the transferred data is + * correct. + */ + if (reg == MCP251XFD_REG_TBC && + (buf_rx->data[0] == 0x0 || buf_rx->data[0] == 0x80)) { + /* Flip highest bit in lowest byte of le32 */ + buf_rx->data[0] ^= 0x80; + + /* re-check CRC */ + err = mcp251xfd_regmap_crc_read_check_crc(buf_rx, + buf_tx, + val_len); + if (!err) { + /* If CRC is now correct, assume + * transferred data was OK, flip bit + * back to original value. + */ + buf_rx->data[0] ^= 0x80; + goto out; + } + } + /* MCP251XFD_REG_OSC is the first ever reg we read from. * * The chip may be in deep sleep and this SPI transfer -- cgit v1.2.3