On Wed. 4 Oct. 2023 at 18:24, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
> The current at91_can driver uses NAPI to handle RX'ed CAN frames, the
> RX IRQ is disabled an a NAPI poll is scheduled. Then in at91_poll_rx()
^^
and
> the RX'ed CAN frames are tried to read in order from the device.
>
> This approach has 2 drawbacks:
>
> - Under high system load it might take too long from the initial RX
> IRQ to the NAPI poll function to run. This causes RX buffer
> overflows.
> - The algorithm to read the CAN frames in order is not bullet proof
> and may fail under certain use cases/system loads.
>
> The rx-offload helper fixes these problems by reading the RX'ed CAN
> frames in the interrupt handler and adding it a list sorted by RX
^
adding it *to* a list?
> timestamp. This list of RX'ed SKBs is then passed to the networking
> stack via NAPI.
>
> Convert the RX path to rx-offload, pass all CAN error frames with
> can_rx_offload_queue_timestamp().
>
> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> ---
> drivers/net/can/Kconfig | 1 +
> drivers/net/can/at91_can.c | 340 +++++++++++++--------------------------------
> 2 files changed, 100 insertions(+), 241 deletions(-)
>
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 649453a3c858..8d6fc0852bf7 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -89,6 +89,7 @@ config CAN_RX_OFFLOAD
> config CAN_AT91
> tristate "Atmel AT91 onchip CAN controller"
> depends on (ARCH_AT91 || COMPILE_TEST) && HAS_IOMEM
> + select CAN_RX_OFFLOAD
> help
> This is a driver for the SoC CAN controller in Atmel's AT91SAM9263
> and AT91SAM9X5 processors.
> diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
> index ca62aa027e5f..33fb1e5edea5 100644
> --- a/drivers/net/can/at91_can.c
> +++ b/drivers/net/can/at91_can.c
> @@ -3,7 +3,7 @@
> * at91_can.c - CAN network driver for AT91 SoC CAN controller
> *
> * (C) 2007 by Hans J. Koch <hjk@xxxxxxxxxxxx>
> - * (C) 2008, 2009, 2010, 2011 by Marc Kleine-Budde <kernel@xxxxxxxxxxxxxx>
> + * (C) 2008, 2009, 2010, 2011, 2023 by Marc Kleine-Budde <kernel@xxxxxxxxxxxxxx>
> */
>
> #include <linux/bitfield.h>
> @@ -26,6 +26,7 @@
>
> #include <linux/can/dev.h>
> #include <linux/can/error.h>
> +#include <linux/can/rx-offload.h>
>
> #define AT91_MB_MASK(i) ((1 << (i)) - 1)
>
> @@ -142,7 +143,6 @@ enum at91_devtype {
>
> struct at91_devtype_data {
> unsigned int rx_first;
> - unsigned int rx_split;
> unsigned int rx_last;
> unsigned int tx_shift;
> enum at91_devtype type;
> @@ -150,14 +150,13 @@ struct at91_devtype_data {
>
> struct at91_priv {
> struct can_priv can; /* must be the first member! */
> - struct napi_struct napi;
> + struct can_rx_offload offload;
> struct phy *transceiver;
>
> void __iomem *reg_base;
>
> unsigned int tx_head;
> unsigned int tx_tail;
> - unsigned int rx_next;
> struct at91_devtype_data devtype_data;
>
> struct clk *clk;
> @@ -166,9 +165,13 @@ struct at91_priv {
> canid_t mb0_id;
> };
>
> +static inline struct at91_priv *rx_offload_to_priv(struct can_rx_offload *offload)
> +{
> + return container_of(offload, struct at91_priv, offload);
> +}
> +
> static const struct at91_devtype_data at91_at91sam9263_data = {
> .rx_first = 1,
> - .rx_split = 8,
> .rx_last = 11,
> .tx_shift = 2,
> .type = AT91_DEVTYPE_SAM9263,
> @@ -176,7 +179,6 @@ static const struct at91_devtype_data at91_at91sam9263_data = {
>
> static const struct at91_devtype_data at91_at91sam9x5_data = {
> .rx_first = 0,
> - .rx_split = 4,
> .rx_last = 5,
> .tx_shift = 1,
> .type = AT91_DEVTYPE_SAM9X5,
> @@ -213,27 +215,6 @@ static inline unsigned int get_mb_rx_last(const struct at91_priv *priv)
> return priv->devtype_data.rx_last;
> }
>
> -static inline unsigned int get_mb_rx_split(const struct at91_priv *priv)
> -{
> - return priv->devtype_data.rx_split;
> -}
> -
> -static inline unsigned int get_mb_rx_num(const struct at91_priv *priv)
> -{
> - return get_mb_rx_last(priv) - get_mb_rx_first(priv) + 1;
> -}
> -
> -static inline unsigned int get_mb_rx_low_last(const struct at91_priv *priv)
> -{
> - return get_mb_rx_split(priv) - 1;
> -}
> -
> -static inline unsigned int get_mb_rx_low_mask(const struct at91_priv *priv)
> -{
> - return AT91_MB_MASK(get_mb_rx_split(priv)) &
> - ~AT91_MB_MASK(get_mb_rx_first(priv));
> -}
> -
> static inline unsigned int get_mb_tx_shift(const struct at91_priv *priv)
> {
> return priv->devtype_data.tx_shift;
> @@ -374,9 +355,8 @@ static void at91_setup_mailboxes(struct net_device *dev)
> for (i = get_mb_tx_first(priv); i <= get_mb_tx_last(priv); i++)
> set_mb_mode_prio(priv, i, AT91_MB_MODE_TX, 0);
>
> - /* Reset tx and rx helper pointers */
> + /* Reset tx helper pointers */
> priv->tx_head = priv->tx_tail = 0;
> - priv->rx_next = get_mb_rx_first(priv);
> }
>
> static int at91_set_bittiming(struct net_device *dev)
> @@ -548,34 +528,6 @@ static netdev_tx_t at91_start_xmit(struct sk_buff *skb, struct net_device *dev)
> return NETDEV_TX_OK;
> }
>
> -/**
> - * at91_activate_rx_low - activate lower rx mailboxes
> - * @priv: a91 context
> - *
> - * Reenables the lower mailboxes for reception of new CAN messages
> - */
> -static inline void at91_activate_rx_low(const struct at91_priv *priv)
> -{
> - u32 mask = get_mb_rx_low_mask(priv);
> -
> - at91_write(priv, AT91_TCR, mask);
> -}
> -
> -/**
> - * at91_activate_rx_mb - reactive single rx mailbox
> - * @priv: a91 context
> - * @mb: mailbox to reactivate
> - *
> - * Reenables given mailbox for reception of new CAN messages
> - */
> -static inline void at91_activate_rx_mb(const struct at91_priv *priv,
> - unsigned int mb)
> -{
> - u32 mask = 1 << mb;
> -
> - at91_write(priv, AT91_TCR, mask);
> -}
> -
> static inline u32 at91_get_timestamp(const struct at91_priv *priv)
> {
> return at91_read(priv, AT91_TIM);
> @@ -600,37 +552,60 @@ static void at91_rx_overflow_err(struct net_device *dev)
> {
> struct net_device_stats *stats = &dev->stats;
> struct sk_buff *skb;
> + struct at91_priv *priv = netdev_priv(dev);
> struct can_frame *cf;
> + u32 timestamp;
> + int err;
>
> netdev_dbg(dev, "RX buffer overflow\n");
> stats->rx_over_errors++;
> stats->rx_errors++;
>
> - skb = alloc_can_err_skb(dev, &cf);
> + skb = at91_alloc_can_err_skb(dev, &cf, ×tamp);
> if (unlikely(!skb))
> return;
>
> cf->can_id |= CAN_ERR_CRTL;
> cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>
> - netif_receive_skb(skb);
> + err = can_rx_offload_queue_timestamp(&priv->offload, skb, timestamp);
> + if (err)
> + stats->rx_fifo_errors++;
> }
>
> /**
> - * at91_read_mb - read CAN msg from mailbox (lowlevel impl)
> - * @dev: net device
> + * at91_mailbox_read - read CAN msg from mailbox
> + * @offload: rx-offload
> * @mb: mailbox number to read from
> - * @cf: can frame where to store message
> + * @timestamp: pointer to 32 bit timestamp
> + * @drop: true indicated mailbox to mark as read and drop frame
> *
> - * Reads a CAN message from the given mailbox and stores data into
> - * given can frame. "mb" and "cf" must be valid.
> + * Reads a CAN message from the given mailbox if not empty.
> */
> -static void at91_read_mb(struct net_device *dev, unsigned int mb,
> - struct can_frame *cf)
> +static struct sk_buff *at91_mailbox_read(struct can_rx_offload *offload,
> + unsigned int mb, u32 *timestamp,
> + bool drop)
> {
> - const struct at91_priv *priv = netdev_priv(dev);
> + const struct at91_priv *priv = rx_offload_to_priv(offload);
> + struct can_frame *cf;
> + struct sk_buff *skb;
> u32 reg_msr, reg_mid;
>
> + reg_msr = at91_read(priv, AT91_MSR(mb));
> + if (!(reg_msr & AT91_MSR_MRDY))
> + return NULL;
> +
> + if (unlikely(drop)) {
> + skb = ERR_PTR(-ENOBUFS);
> + goto mark_as_read;
> + }
> +
> + skb = alloc_can_skb(offload->dev, &cf);
> + if (unlikely(!skb)) {
> + skb = ERR_PTR(-ENOMEM);
> + goto mark_as_read;
> + }
> +
> reg_mid = at91_read(priv, AT91_MID(mb));
> if (reg_mid & AT91_MID_MIDE)
> cf->can_id = FIELD_GET(AT91_MID_MIDVA_MASK | AT91_MID_MIDVB_MASK, reg_mid) |
> @@ -638,7 +613,9 @@ static void at91_read_mb(struct net_device *dev, unsigned int mb,
> else
> cf->can_id = FIELD_GET(AT91_MID_MIDVA_MASK, reg_mid);
>
> - reg_msr = at91_read(priv, AT91_MSR(mb));
> + /* extend timstamp to full 32 bit */
^^^^^^^^
timestamp
> + *timestamp = FIELD_GET(AT91_MSR_MTIMESTAMP_MASK, reg_msr) << 16;
If I understand correctly, you only use the hardware timestamp for the
napi but you do not report it to the userland.
Not a criticism of this series, but it seems to me that it would be
easy to add one follow-up patch that would populate
skb_shared_hwtstamps->hwtstamp and update ethtool_ops->get_ts_info in
order to report those hardware timestamps to the user.
> cf->len = can_cc_dlc2len(FIELD_GET(AT91_MSR_MDLC_MASK, reg_msr));
>
> if (reg_msr & AT91_MSR_MRTR) {
> @@ -652,151 +629,12 @@ static void at91_read_mb(struct net_device *dev, unsigned int mb,
> at91_write(priv, AT91_MID(mb), AT91_MID_MIDE);
>
> if (unlikely(mb == get_mb_rx_last(priv) && reg_msr & AT91_MSR_MMI))
> - at91_rx_overflow_err(dev);
> -}
> + at91_rx_overflow_err(offload->dev);
(...)
This concludes my "review" of this series. Because I was scrolling
through it and not doing anything thorough, I will not be giving my
review-by tag even if there is a follow-up v2. That said, aside from
my comment on patch 01/27 and the random typo nitpick, nothing seemed
off.
Yours sincerely,
Vincent Mailhol
