From: Fedor Ross <fedor.ross(a)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(a)ifm.com>
Signed-off-by: Marek Vasut <marex(a)denx.de>
Cc: stable(a)vger.kernel.org
Link: https://lore.kernel.org/all/20230717-mcp251xfd-fix-increase-poll-timeout-v5…
Signed-off-by: Marc Kleine-Budde <mkl(a)pengutronix.de>
---
drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 10 ++++++++--
drivers/net/can/spi/mcp251xfd/mcp251xfd.h | 1 +
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index 68df6d4641b5..eebf967f4711 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -227,6 +227,8 @@ 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;
+ unsigned long timeout_us = MCP251XFD_POLL_TIMEOUT_US;
u32 con = 0, con_reqop, osc = 0;
u8 mode;
int err;
@@ -246,12 +248,16 @@ __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv,
if (mode_req == MCP251XFD_REG_CON_MODE_SLEEP || nowait)
return 0;
+ if (bt->bitrate)
+ timeout_us = max_t(unsigned long, timeout_us,
+ MCP251XFD_FRAME_LEN_MAX_BITS * USEC_PER_SEC /
+ bt->bitrate);
+
err = regmap_read_poll_timeout(priv->map_reg, MCP251XFD_REG_CON, con,
!mcp251xfd_reg_invalid(con) &&
FIELD_GET(MCP251XFD_REG_CON_OPMOD_MASK,
con) == mode_req,
- MCP251XFD_POLL_SLEEP_US,
- MCP251XFD_POLL_TIMEOUT_US);
+ MCP251XFD_POLL_SLEEP_US, timeout_us);
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
--
2.40.1
If the gs_usb device driver is unloaded (or unbound) before the
interface is shut down, the USB stack first calls the struct
usb_driver::disconnect and then the struct net_device_ops::ndo_stop
callback.
In gs_usb_disconnect() all pending bulk URBs are killed, i.e. no more
RX'ed CAN frames are send from the USB device to the host. Later in
gs_can_close() a reset control message is send to each CAN channel to
remove the controller from the CAN bus. In this race window the USB
device can still receive CAN frames from the bus and internally queue
them to be send to the host.
At least in the current version of the candlelight firmware, the queue
of received CAN frames is not emptied during the reset command. After
loading (or binding) the gs_usb driver, new URBs are submitted during
the struct net_device_ops::ndo_open callback and the candlelight
firmware starts sending its already queued CAN frames to the host.
However, this scenario was not considered when implementing the
hardware timestamp function. The cycle counter/time counter
infrastructure is set up (gs_usb_timestamp_init()) after the USBs are
submitted, resulting in a NULL pointer dereference if
timecounter_cyc2time() (via the call chain:
gs_usb_receive_bulk_callback() -> gs_usb_set_timestamp() ->
gs_usb_skb_set_timestamp()) is called too early.
Move the gs_usb_timestamp_init() function before the URBs are
submitted to fix this problem.
For a comprehensive solution, we need to consider gs_usb devices with
more than 1 channel. The cycle counter/time counter infrastructure is
setup per channel, but the RX URBs are per device. Once gs_can_open()
of _a_ channel has been called, and URBs have been submitted, the
gs_usb_receive_bulk_callback() can be called for _all_ available
channels, even for channels that are not running, yet. As cycle
counter/time counter has not set up, this will again lead to a NULL
pointer dereference.
Convert the cycle counter/time counter from a "per channel" to a "per
device" functionality. Also set it up, before submitting any URBs to
the device.
Further in gs_usb_receive_bulk_callback(), don't process any URBs for
not started CAN channels, only resubmit the URB.
Fixes: 45dfa45f52e6 ("can: gs_usb: add RX and TX hardware timestamp support")
Closes: https://github.com/candle-usb/candleLight_fw/issues/137#issuecomment-162353…
Cc: stable(a)vger.kernel.org
Cc: John Whittington <git(a)jbrengineering.co.uk>
Link: https://lore.kernel.org/all/20230716-gs_usb-fix-time-stamp-counter-v1-2-901…
Signed-off-by: Marc Kleine-Budde <mkl(a)pengutronix.de>
---
drivers/net/can/usb/gs_usb.c | 101 ++++++++++++++++++-----------------
1 file changed, 53 insertions(+), 48 deletions(-)
diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 85b7b59c8426..f418066569fc 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -303,12 +303,6 @@ struct gs_can {
struct can_bittiming_const bt_const, data_bt_const;
unsigned int channel; /* channel number */
- /* time counter for hardware timestamps */
- struct cyclecounter cc;
- struct timecounter tc;
- spinlock_t tc_lock; /* spinlock to guard access tc->cycle_last */
- struct delayed_work timestamp;
-
u32 feature;
unsigned int hf_size_tx;
@@ -325,6 +319,13 @@ struct gs_usb {
struct gs_can *canch[GS_MAX_INTF];
struct usb_anchor rx_submitted;
struct usb_device *udev;
+
+ /* time counter for hardware timestamps */
+ struct cyclecounter cc;
+ struct timecounter tc;
+ spinlock_t tc_lock; /* spinlock to guard access tc->cycle_last */
+ struct delayed_work timestamp;
+
unsigned int hf_size_rx;
u8 active_channels;
};
@@ -388,15 +389,15 @@ static int gs_cmd_reset(struct gs_can *dev)
GFP_KERNEL);
}
-static inline int gs_usb_get_timestamp(const struct gs_can *dev,
+static inline int gs_usb_get_timestamp(const struct gs_usb *parent,
u32 *timestamp_p)
{
__le32 timestamp;
int rc;
- rc = usb_control_msg_recv(dev->udev, 0, GS_USB_BREQ_TIMESTAMP,
+ rc = usb_control_msg_recv(parent->udev, 0, GS_USB_BREQ_TIMESTAMP,
USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
- dev->channel, 0,
+ 0, 0,
×tamp, sizeof(timestamp),
USB_CTRL_GET_TIMEOUT,
GFP_KERNEL);
@@ -410,20 +411,20 @@ static inline int gs_usb_get_timestamp(const struct gs_can *dev,
static u64 gs_usb_timestamp_read(const struct cyclecounter *cc) __must_hold(&dev->tc_lock)
{
- struct gs_can *dev = container_of(cc, struct gs_can, cc);
+ struct gs_usb *parent = container_of(cc, struct gs_usb, cc);
u32 timestamp = 0;
int err;
- lockdep_assert_held(&dev->tc_lock);
+ lockdep_assert_held(&parent->tc_lock);
/* drop lock for synchronous USB transfer */
- spin_unlock_bh(&dev->tc_lock);
- err = gs_usb_get_timestamp(dev, ×tamp);
- spin_lock_bh(&dev->tc_lock);
+ spin_unlock_bh(&parent->tc_lock);
+ err = gs_usb_get_timestamp(parent, ×tamp);
+ spin_lock_bh(&parent->tc_lock);
if (err)
- netdev_err(dev->netdev,
- "Error %d while reading timestamp. HW timestamps may be inaccurate.",
- err);
+ dev_err(&parent->udev->dev,
+ "Error %d while reading timestamp. HW timestamps may be inaccurate.",
+ err);
return timestamp;
}
@@ -431,14 +432,14 @@ static u64 gs_usb_timestamp_read(const struct cyclecounter *cc) __must_hold(&dev
static void gs_usb_timestamp_work(struct work_struct *work)
{
struct delayed_work *delayed_work = to_delayed_work(work);
- struct gs_can *dev;
+ struct gs_usb *parent;
- dev = container_of(delayed_work, struct gs_can, timestamp);
- spin_lock_bh(&dev->tc_lock);
- timecounter_read(&dev->tc);
- spin_unlock_bh(&dev->tc_lock);
+ parent = container_of(delayed_work, struct gs_usb, timestamp);
+ spin_lock_bh(&parent->tc_lock);
+ timecounter_read(&parent->tc);
+ spin_unlock_bh(&parent->tc_lock);
- schedule_delayed_work(&dev->timestamp,
+ schedule_delayed_work(&parent->timestamp,
GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
}
@@ -446,37 +447,38 @@ static void gs_usb_skb_set_timestamp(struct gs_can *dev,
struct sk_buff *skb, u32 timestamp)
{
struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
+ struct gs_usb *parent = dev->parent;
u64 ns;
- spin_lock_bh(&dev->tc_lock);
- ns = timecounter_cyc2time(&dev->tc, timestamp);
- spin_unlock_bh(&dev->tc_lock);
+ spin_lock_bh(&parent->tc_lock);
+ ns = timecounter_cyc2time(&parent->tc, timestamp);
+ spin_unlock_bh(&parent->tc_lock);
hwtstamps->hwtstamp = ns_to_ktime(ns);
}
-static void gs_usb_timestamp_init(struct gs_can *dev)
+static void gs_usb_timestamp_init(struct gs_usb *parent)
{
- struct cyclecounter *cc = &dev->cc;
+ struct cyclecounter *cc = &parent->cc;
cc->read = gs_usb_timestamp_read;
cc->mask = CYCLECOUNTER_MASK(32);
cc->shift = 32 - bits_per(NSEC_PER_SEC / GS_USB_TIMESTAMP_TIMER_HZ);
cc->mult = clocksource_hz2mult(GS_USB_TIMESTAMP_TIMER_HZ, cc->shift);
- spin_lock_init(&dev->tc_lock);
- spin_lock_bh(&dev->tc_lock);
- timecounter_init(&dev->tc, &dev->cc, ktime_get_real_ns());
- spin_unlock_bh(&dev->tc_lock);
+ spin_lock_init(&parent->tc_lock);
+ spin_lock_bh(&parent->tc_lock);
+ timecounter_init(&parent->tc, &parent->cc, ktime_get_real_ns());
+ spin_unlock_bh(&parent->tc_lock);
- INIT_DELAYED_WORK(&dev->timestamp, gs_usb_timestamp_work);
- schedule_delayed_work(&dev->timestamp,
+ INIT_DELAYED_WORK(&parent->timestamp, gs_usb_timestamp_work);
+ schedule_delayed_work(&parent->timestamp,
GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
}
-static void gs_usb_timestamp_stop(struct gs_can *dev)
+static void gs_usb_timestamp_stop(struct gs_usb *parent)
{
- cancel_delayed_work_sync(&dev->timestamp);
+ cancel_delayed_work_sync(&parent->timestamp);
}
static void gs_update_state(struct gs_can *dev, struct can_frame *cf)
@@ -560,6 +562,9 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
if (!netif_device_present(netdev))
return;
+ if (!netif_running(netdev))
+ goto resubmit_urb;
+
if (hf->echo_id == -1) { /* normal rx */
if (hf->flags & GS_CAN_FLAG_FD) {
skb = alloc_canfd_skb(dev->netdev, &cfd);
@@ -856,6 +861,9 @@ static int gs_can_open(struct net_device *netdev)
}
if (!parent->active_channels) {
+ if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+ gs_usb_timestamp_init(parent);
+
for (i = 0; i < GS_MAX_RX_URBS; i++) {
u8 *buf;
@@ -926,13 +934,9 @@ static int gs_can_open(struct net_device *netdev)
flags |= GS_CAN_MODE_FD;
/* if hardware supports timestamps, enable it */
- if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
+ if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
flags |= GS_CAN_MODE_HW_TIMESTAMP;
- /* start polling timestamp */
- gs_usb_timestamp_init(dev);
- }
-
/* finally start device */
dev->can.state = CAN_STATE_ERROR_ACTIVE;
dm.flags = cpu_to_le32(flags);
@@ -942,8 +946,6 @@ static int gs_can_open(struct net_device *netdev)
GFP_KERNEL);
if (rc) {
netdev_err(netdev, "Couldn't start device (err=%d)\n", rc);
- if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
- gs_usb_timestamp_stop(dev);
dev->can.state = CAN_STATE_STOPPED;
goto out_usb_kill_anchored_urbs;
@@ -960,9 +962,13 @@ static int gs_can_open(struct net_device *netdev)
out_usb_free_urb:
usb_free_urb(urb);
out_usb_kill_anchored_urbs:
- if (!parent->active_channels)
+ if (!parent->active_channels) {
usb_kill_anchored_urbs(&dev->tx_submitted);
+ if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+ gs_usb_timestamp_stop(parent);
+ }
+
close_candev(netdev);
return rc;
@@ -1011,14 +1017,13 @@ static int gs_can_close(struct net_device *netdev)
netif_stop_queue(netdev);
- /* stop polling timestamp */
- if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
- gs_usb_timestamp_stop(dev);
-
/* Stop polling */
parent->active_channels--;
if (!parent->active_channels) {
usb_kill_anchored_urbs(&parent->rx_submitted);
+
+ if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+ gs_usb_timestamp_stop(parent);
}
/* Stop sending URBs */
--
2.40.1
The gs_usb driver handles USB devices with more than 1 CAN channel.
The RX path for all channels share the same bulk endpoint (the
transmitted bulk data encodes the channel number). These per-device
resources are allocated and submitted by the first opened channel.
During this allocation, the resources are either released immediately
in case of a failure or the URBs are anchored. All anchored URBs are
finally killed with gs_usb_disconnect().
Currently, gs_can_open() returns with an error if the allocation of a
URB or a buffer fails. However, if usb_submit_urb() fails, the driver
continues with the URBs submitted so far, even if no URBs were
successfully submitted.
Treat every error as fatal and free all allocated resources
immediately.
Switch to goto-style error handling, to prepare the driver for more
per-device resource allocation.
Cc: stable(a)vger.kernel.org
Cc: John Whittington <git(a)jbrengineering.co.uk>
Link: https://lore.kernel.org/all/20230716-gs_usb-fix-time-stamp-counter-v1-1-901…
Signed-off-by: Marc Kleine-Budde <mkl(a)pengutronix.de>
---
drivers/net/can/usb/gs_usb.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index d476c2884008..85b7b59c8426 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -833,6 +833,7 @@ static int gs_can_open(struct net_device *netdev)
.mode = cpu_to_le32(GS_CAN_MODE_START),
};
struct gs_host_frame *hf;
+ struct urb *urb = NULL;
u32 ctrlmode;
u32 flags = 0;
int rc, i;
@@ -856,13 +857,14 @@ static int gs_can_open(struct net_device *netdev)
if (!parent->active_channels) {
for (i = 0; i < GS_MAX_RX_URBS; i++) {
- struct urb *urb;
u8 *buf;
/* alloc rx urb */
urb = usb_alloc_urb(0, GFP_KERNEL);
- if (!urb)
- return -ENOMEM;
+ if (!urb) {
+ rc = -ENOMEM;
+ goto out_usb_kill_anchored_urbs;
+ }
/* alloc rx buffer */
buf = kmalloc(dev->parent->hf_size_rx,
@@ -870,8 +872,8 @@ static int gs_can_open(struct net_device *netdev)
if (!buf) {
netdev_err(netdev,
"No memory left for USB buffer\n");
- usb_free_urb(urb);
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto out_usb_free_urb;
}
/* fill, anchor, and submit rx urb */
@@ -894,9 +896,7 @@ static int gs_can_open(struct net_device *netdev)
netdev_err(netdev,
"usb_submit failed (err=%d)\n", rc);
- usb_unanchor_urb(urb);
- usb_free_urb(urb);
- break;
+ goto out_usb_unanchor_urb;
}
/* Drop reference,
@@ -945,7 +945,8 @@ static int gs_can_open(struct net_device *netdev)
if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
gs_usb_timestamp_stop(dev);
dev->can.state = CAN_STATE_STOPPED;
- return rc;
+
+ goto out_usb_kill_anchored_urbs;
}
parent->active_channels++;
@@ -953,6 +954,18 @@ static int gs_can_open(struct net_device *netdev)
netif_start_queue(netdev);
return 0;
+
+out_usb_unanchor_urb:
+ usb_unanchor_urb(urb);
+out_usb_free_urb:
+ usb_free_urb(urb);
+out_usb_kill_anchored_urbs:
+ if (!parent->active_channels)
+ usb_kill_anchored_urbs(&dev->tx_submitted);
+
+ close_candev(netdev);
+
+ return rc;
}
static int gs_usb_get_state(const struct net_device *netdev,
--
2.40.1
From: Fedor Ross <fedor.ross(a)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(a)ifm.com>
Signed-off-by: Marek Vasut <marex(a)denx.de>
Cc: stable(a)vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl(a)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
---
Changes in v5:
- use max_t() instead of max() to fix compilation on arm64
- Link to v4: https://lore.kernel.org/all/20230717-mcp251xfd-fix-increase-poll-timeout-v4…
Changes in v4:
- fix division by 0, if bit rate is not set yet
- Link to v3: https://lore.kernel.org/all/20230717100815.75764-1-mkl@pengutronix.de
Changes in 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
- Link to v2: https://lore.kernel.org/all/20230505222820.126441-1-marex@denx.de
Changes in v2:
- Add macros for CAN_BIT_STUFFING_OVERHEAD and CAN_IDLE_CONDITION_SAMPLES
(thanks Thomas, but please double check the comments)
- Update commit message
- Link to v1: https://lore.kernel.org/all/20230504195059.4706-1-marex@denx.de
---
drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 10 ++++++++--
drivers/net/can/spi/mcp251xfd/mcp251xfd.h | 1 +
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index 68df6d4641b5..eebf967f4711 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -227,6 +227,8 @@ 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;
+ unsigned long timeout_us = MCP251XFD_POLL_TIMEOUT_US;
u32 con = 0, con_reqop, osc = 0;
u8 mode;
int err;
@@ -246,12 +248,16 @@ __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv,
if (mode_req == MCP251XFD_REG_CON_MODE_SLEEP || nowait)
return 0;
+ if (bt->bitrate)
+ timeout_us = max_t(unsigned long, timeout_us,
+ MCP251XFD_FRAME_LEN_MAX_BITS * USEC_PER_SEC /
+ bt->bitrate);
+
err = regmap_read_poll_timeout(priv->map_reg, MCP251XFD_REG_CON, con,
!mcp251xfd_reg_invalid(con) &&
FIELD_GET(MCP251XFD_REG_CON_OPMOD_MASK,
con) == mode_req,
- MCP251XFD_POLL_SLEEP_US,
- MCP251XFD_POLL_TIMEOUT_US);
+ MCP251XFD_POLL_SLEEP_US, timeout_us);
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
---
base-commit: 0dd1805fe498e0cf64f68e451a8baff7e64494ec
change-id: 20230717-mcp251xfd-fix-increase-poll-timeout-1f23b0e519b0
Best regards,
--
Marc Kleine-Budde <mkl(a)pengutronix.de>
Initialize local struct spi_mem_op at declaration to avoid having
garbage data from stack for members that were not explicitly
initialized afterwards. Zeroise the local struct after the first
use, so that we have it clean for the second use.
Fixes: d73ee7534cc5 ("mtd: spi-nor: core: perform a Soft Reset on shutdown")
Cc: stable(a)vger.kernel.org
Signed-off-by: Tudor Ambarus <tudor.ambarus(a)linaro.org>
---
drivers/mtd/spi-nor/core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 273258f7e77f..603791497523 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3235,11 +3235,9 @@ static int spi_nor_init(struct spi_nor *nor)
*/
static void spi_nor_soft_reset(struct spi_nor *nor)
{
- struct spi_mem_op op;
+ struct spi_mem_op op = SPINOR_SRSTEN_OP;
int ret;
- op = (struct spi_mem_op)SPINOR_SRSTEN_OP;
-
spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
ret = spi_mem_exec_op(nor->spimem, &op);
@@ -3248,6 +3246,7 @@ static void spi_nor_soft_reset(struct spi_nor *nor)
return;
}
+ memset(&op, 0, sizeof(op));
op = (struct spi_mem_op)SPINOR_SRST_OP;
spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
--
2.34.1
From: Fedor Ross <fedor.ross(a)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(a)ifm.com>
Signed-off-by: Marek Vasut <marex(a)denx.de>
Cc: Vincent Mailhol <mailhol.vincent(a)wanadoo.fr>
Cc: Manivannan Sadhasivam <mani(a)kernel.org>
Cc: Thomas Kopp <thomas.kopp(a)microchip.com>
Cc: stable(a)vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl(a)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
--
2.40.1