From: Fedor Ross fedor.ross@ifm.com
The mcp251xfd controller needs an idle bus to enter 'Normal CAN 2.0 mode' or . The maximum length of a CAN frame is 736 bits (64 data bytes, CAN-FD, EFF mode, worst case bit stuffing and interframe spacing). For low bit rates like 10 kbit/s the arbitrarily chosen MCP251XFD_POLL_TIMEOUT_US of 1 ms is too small.
Otherwise during polling for the CAN controller to enter 'Normal CAN 2.0 mode' the timeout limit is exceeded and the configuration fails with:
| $ ip link set dev can1 up type can bitrate 10000 | [ 731.911072] mcp251xfd spi2.1 can1: Controller failed to enter mode CAN 2.0 Mode (6) and stays in Configuration Mode (4) (con=0x068b0760, osc=0x00000468). | [ 731.927192] mcp251xfd spi2.1 can1: CRC read error at address 0x0e0c (length=4, data=00 00 00 00, CRC=0x0000) retrying. | [ 731.938101] A link change request failed with some changes committed already. Interface can1 may have been left with an inconsistent configuration, please check. | RTNETLINK answers: Connection timed out
Make MCP251XFD_POLL_TIMEOUT_US timeout calculation dynamic. Use maximum of 1ms and bit time of 1 full 64 data bytes CAN-FD frame in EFF mode, worst case bit stuffing and interframe spacing at the current bit rate.
For easier backporting define the macro MCP251XFD_FRAME_LEN_MAX_BITS that holds the max frame length in bits, which is 736. This can be replaced by can_frame_bits(true, true, true, true, CANFD_MAX_DLEN) in a cleanup patch later.
Fixes: 55e5b97f003e8 ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN") Signed-off-by: Fedor Ross fedor.ross@ifm.com Signed-off-by: Marek Vasut marex@denx.de Cc: Vincent Mailhol mailhol.vincent@wanadoo.fr Cc: Manivannan Sadhasivam mani@kernel.org Cc: Thomas Kopp thomas.kopp@microchip.com Cc: stable@vger.kernel.org Signed-off-by: Marc Kleine-Budde mkl@pengutronix.de --- Hello,
picking up Fedor's and Marek's work. I decided to make it a minimal patch and add stable on Cc. The mentioned cleanup patch that replaces 736 by can_frame_bits() can be done later and will go upstream via can-next.
regards, Marc
v3: - use 736 as max CAN frame length, calculated by Vincent Mailhol's 80a2fbce456e ("can: length: refactor frame lengths definition to add size in bits") - update commit message - drop patch 2/2
v2: https://lore.kernel.org/all/20230505222820.126441-1-marex@denx.de - Add macros for CAN_BIT_STUFFING_OVERHEAD and CAN_IDLE_CONDITION_SAMPLES (thanks Thomas, but please double check the comments) - Update commit message
v1: https://lore.kernel.org/all/20230504195059.4706-1-marex@denx.de
drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 4 +++- drivers/net/can/spi/mcp251xfd/mcp251xfd.h | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c index 68df6d4641b5..876e8e3cbb0b 100644 --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c @@ -227,6 +227,7 @@ static int __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv, const u8 mode_req, bool nowait) { + const struct can_bittiming *bt = &priv->can.bittiming; u32 con = 0, con_reqop, osc = 0; u8 mode; int err; @@ -251,7 +252,8 @@ __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv, FIELD_GET(MCP251XFD_REG_CON_OPMOD_MASK, con) == mode_req, MCP251XFD_POLL_SLEEP_US, - MCP251XFD_POLL_TIMEOUT_US); + max_t(unsigned long, MCP251XFD_POLL_TIMEOUT_US, + MCP251XFD_FRAME_LEN_MAX_BITS * USEC_PER_SEC / bt->bitrate)); if (err != -ETIMEDOUT && err != -EBADMSG) return err;
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h index 7024ff0cc2c0..24510b3b8020 100644 --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h @@ -387,6 +387,7 @@ static_assert(MCP251XFD_TIMESTAMP_WORK_DELAY_SEC < #define MCP251XFD_OSC_STAB_TIMEOUT_US (10 * MCP251XFD_OSC_STAB_SLEEP_US) #define MCP251XFD_POLL_SLEEP_US (10) #define MCP251XFD_POLL_TIMEOUT_US (USEC_PER_MSEC) +#define MCP251XFD_FRAME_LEN_MAX_BITS (736)
/* Misc */ #define MCP251XFD_NAPI_WEIGHT 32
On 17.07.2023 12:08:15, Marc Kleine-Budde wrote:
From: Fedor Ross fedor.ross@ifm.com
The mcp251xfd controller needs an idle bus to enter 'Normal CAN 2.0 mode' or . The maximum length of a CAN frame is 736 bits (64 data bytes, CAN-FD, EFF mode, worst case bit stuffing and interframe spacing). For low bit rates like 10 kbit/s the arbitrarily chosen MCP251XFD_POLL_TIMEOUT_US of 1 ms is too small.
Otherwise during polling for the CAN controller to enter 'Normal CAN 2.0 mode' the timeout limit is exceeded and the configuration fails with:
| $ ip link set dev can1 up type can bitrate 10000 | [ 731.911072] mcp251xfd spi2.1 can1: Controller failed to enter mode CAN 2.0 Mode (6) and stays in Configuration Mode (4) (con=0x068b0760, osc=0x00000468). | [ 731.927192] mcp251xfd spi2.1 can1: CRC read error at address 0x0e0c (length=4, data=00 00 00 00, CRC=0x0000) retrying. | [ 731.938101] A link change request failed with some changes committed already. Interface can1 may have been left with an inconsistent configuration, please check. | RTNETLINK answers: Connection timed out
Make MCP251XFD_POLL_TIMEOUT_US timeout calculation dynamic. Use maximum of 1ms and bit time of 1 full 64 data bytes CAN-FD frame in EFF mode, worst case bit stuffing and interframe spacing at the current bit rate.
For easier backporting define the macro MCP251XFD_FRAME_LEN_MAX_BITS that holds the max frame length in bits, which is 736. This can be replaced by can_frame_bits(true, true, true, true, CANFD_MAX_DLEN) in a cleanup patch later.
Fixes: 55e5b97f003e8 ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN") Signed-off-by: Fedor Ross fedor.ross@ifm.com Signed-off-by: Marek Vasut marex@denx.de Cc: Vincent Mailhol mailhol.vincent@wanadoo.fr Cc: Manivannan Sadhasivam mani@kernel.org Cc: Thomas Kopp thomas.kopp@microchip.com Cc: stable@vger.kernel.org Signed-off-by: Marc Kleine-Budde mkl@pengutronix.de
Hello,
picking up Fedor's and Marek's work. I decided to make it a minimal patch and add stable on Cc. The mentioned cleanup patch that replaces 736 by can_frame_bits() can be done later and will go upstream via can-next.
regards, Marc
v3:
- use 736 as max CAN frame length, calculated by Vincent Mailhol's 80a2fbce456e ("can: length: refactor frame lengths definition to add size in bits")
- update commit message
- drop patch 2/2
v2: https://lore.kernel.org/all/20230505222820.126441-1-marex@denx.de
- Add macros for CAN_BIT_STUFFING_OVERHEAD and CAN_IDLE_CONDITION_SAMPLES (thanks Thomas, but please double check the comments)
- Update commit message
v1: https://lore.kernel.org/all/20230504195059.4706-1-marex@denx.de
drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 4 +++- drivers/net/can/spi/mcp251xfd/mcp251xfd.h | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c index 68df6d4641b5..876e8e3cbb0b 100644 --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c @@ -227,6 +227,7 @@ static int __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv, const u8 mode_req, bool nowait) {
- const struct can_bittiming *bt = &priv->can.bittiming; u32 con = 0, con_reqop, osc = 0; u8 mode; int err;
@@ -251,7 +252,8 @@ __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv, FIELD_GET(MCP251XFD_REG_CON_OPMOD_MASK, con) == mode_req, MCP251XFD_POLL_SLEEP_US,
MCP251XFD_POLL_TIMEOUT_US);
max_t(unsigned long, MCP251XFD_POLL_TIMEOUT_US,
MCP251XFD_FRAME_LEN_MAX_BITS * USEC_PER_SEC / bt->bitrate));
This segfaults with a div by zero if bitrate is not set, yet. Fixed in v4.
regards, Marc
linux-stable-mirror@lists.linaro.org