| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251121-refined-economic-oyster-5de770-mkl@pengutronix.de>
Date: Fri, 21 Nov 2025 14:03:03 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Arun Muthusamy <arun.muthusamy@...sler.com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
mailhol@...nel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-can@...r.kernel.org
Subject: Re: [PATCH 10/10] can: grcan: Add CANFD support alongside legacy CAN
On 18.11.2025 10:21:15, Arun Muthusamy wrote:
> Include CANFD support with the legacy CAN support, enabling
> support for extended data payloads up to 64 bytes, higher bit rates,
> handle canecho frames.
>
> Signed-off-by: Arun Muthusamy <arun.muthusamy@...sler.com>
> ---
> drivers/net/can/grcan.c | 240 ++++++++++++++++++++++++++++------------
> 1 file changed, 167 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> index 8753bff4f917..ff7ab979d2c9 100644
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c
> @@ -44,6 +44,8 @@
>
> #define GRCAN_RESERVE_SIZE(slot1, slot2) (((slot2) - (slot1)) / 4 - 1)
>
> +#define CHECK_SLOT_FDF(slot) ((slot) & GRCAN_RX_FDF)
> +
> struct grcan_registers {
> u32 conf; /* 0x00 */
> u32 stat; /* 0x04 */
> @@ -181,8 +183,11 @@ struct grcan_registers {
> | GRCAN_IRQ_TXAHBERR | GRCAN_IRQ_RXAHBERR \
> | GRCAN_IRQ_TXLOSS)
> #define GRCAN_IRQ_DEFAULT (GRCAN_IRQ_RX | GRCAN_IRQ_TX | GRCAN_IRQ_ERRORS)
> +#define GRCAN_CTRL_MODES (CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_ONE_SHOT)
> +#define GRCAN_CTRL_MODES_FD (CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_FD)
IMHO no need for these defines, just list the CAN_CTRLMODE
>
> #define GRCAN_MSG_SIZE 16
> +#define GRCAN_CLASSIC_DATA_SIZE 8
>
> #define GRCAN_MSG_IDE 0x80000000
> #define GRCAN_MSG_RTR 0x40000000
> @@ -264,6 +269,12 @@ struct grcan_registers {
> #define GRCANFD_FDBTR_PS2_BIT 5
> #define GRCANFD_FDBTR_SJW_BIT 0
>
> +#define GRCAN_TX_BRS BIT(25)
> +#define GRCAN_TX_FDF BIT(26)
> +
> +#define GRCAN_RX_BRS BIT(25)
> +#define GRCAN_RX_FDF BIT(26)
> +
> /* Hardware capabilities */
> struct grcan_hwcap {
> /* CAN-FD capable, indicates GRCANFD IP.
> @@ -326,6 +337,13 @@ struct grcan_priv {
>
> struct sk_buff **echo_skb; /* We allocate this on our own */
>
> + /*
> + * Since the CAN FD frame has a variable length, this variable is used
> + * to keep track of the index of the CAN echo skb (socket buffer) frame.
> + * It's important for ensuring that we correctly manage the echo skb.
> + */
> + u32 echo_skb_idx;
> +
> /* The echo skb pointer, pointing into echo_skb and indicating which
> * frames can be echoed back. See the "Notes on the tx cyclic buffer
> * handling"-comment for grcan_start_xmit for more details.
> @@ -637,7 +655,7 @@ static int catch_up_echo_skb(struct net_device *dev, int budget, bool echo)
> struct grcan_registers __iomem *regs = priv->regs;
> struct grcan_dma *dma = &priv->dma;
> struct net_device_stats *stats = &dev->stats;
> - int i, work_done;
> + int work_done;
>
> /* Updates to priv->eskbp and wake-ups of the queue needs to
> * be atomic towards the reads of priv->eskbp and shut-downs
> @@ -648,19 +666,22 @@ static int catch_up_echo_skb(struct net_device *dev, int budget, bool echo)
> for (work_done = 0; work_done < budget || budget < 0; work_done++) {
> if (priv->eskbp == txrd)
> break;
> - i = priv->eskbp / GRCAN_MSG_SIZE;
> - if (echo) {
> - /* Normal echo of messages */
> - stats->tx_packets++;
> - stats->tx_bytes += can_get_echo_skb(dev, i, NULL);
> - } else {
> - /* For cleanup of untransmitted messages */
> - can_free_echo_skb(dev, i, NULL);
> - }
>
> priv->eskbp = grcan_ring_add(priv->eskbp, GRCAN_MSG_SIZE,
> dma->tx.size);
> txrd = grcan_read_reg(®s->txrd);
> +
> + /* Grab the packet once the packet is send or free untransmitted packet*/
> + if (priv->eskbp == txrd) {
> + if (echo) {
> + /* Normal echo of messages */
> + stats->tx_packets++;
> + stats->tx_bytes += can_get_echo_skb(dev, priv->echo_skb_idx, NULL);
> + } else {
> + /* For cleanup of untransmitted messages */
> + can_free_echo_skb(dev, priv->echo_skb_idx, NULL);
> + }
> + }
> }
> return work_done;
> }
> @@ -1174,6 +1195,7 @@ static int grcan_set_mode(struct net_device *dev, enum can_mode mode)
> if (!(priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY))
> netif_wake_queue(dev);
> }
> + priv->echo_skb_idx = 0;
> spin_unlock_irqrestore(&priv->lock, flags);
> return err;
> }
> @@ -1223,7 +1245,6 @@ static int grcan_open(struct net_device *dev)
> netif_start_queue(dev);
> priv->resetting = false;
> priv->closing = false;
> -
> spin_unlock_irqrestore(&priv->lock, flags);
>
> return 0;
> @@ -1294,20 +1315,29 @@ static void grcan_transmit_catch_up(struct net_device *dev)
> spin_unlock_irqrestore(&priv->lock, flags);
> }
>
> +static int grcan_numbds(int len)
> +{
> + if (len <= GRCAN_CLASSIC_DATA_SIZE)
> + return 1;
> + return 1 + ((len - GRCAN_CLASSIC_DATA_SIZE + GRCAN_MSG_SIZE) / GRCAN_MSG_SIZE);
> +}
> +
> static int grcan_receive(struct net_device *dev, int budget)
> {
> struct grcan_priv *priv = netdev_priv(dev);
> struct grcan_registers __iomem *regs = priv->regs;
> struct grcan_dma *dma = &priv->dma;
> struct net_device_stats *stats = &dev->stats;
> - struct can_frame *cf;
> + struct canfd_frame *cf;
> struct sk_buff *skb;
> - u32 wr, rd, startrd;
> + u32 wr, rd, dlc, startrd;
> u32 *slot;
> u32 rtr, eff;
> - int work_done = 0;
> + u8 *data;
> + int i, bds, payload_offset, copy_len, work_done = 0;
Please take care of the infamous reverse-xmas-tree.
>
> rd = grcan_read_reg(®s->rxrd);
> +
> startrd = rd;
> for (work_done = 0; work_done < budget; work_done++) {
> /* Check for packet to receive */
> @@ -1315,44 +1345,70 @@ static int grcan_receive(struct net_device *dev, int budget)
> if (rd == wr)
> break;
>
> - /* Take care of packet */
> - skb = alloc_can_skb(dev, &cf);
> - if (skb == NULL) {
> + slot = dma->rx.buf + rd;
> +
> + if (CHECK_SLOT_FDF(slot[1]))
IMHO "slot[1] & GRCAN_RX_FDF" is more readable
> + skb = alloc_canfd_skb(dev, &cf);
> + else
> + skb = alloc_can_skb(priv->dev, (struct can_frame **)&cf);
> +
> + if (unlikely(!skb)) {
> netdev_err(dev,
> "dropping frame: skb allocation failed\n");
> stats->rx_dropped++;
> continue;
> }
>
> - slot = dma->rx.buf + rd;
> - eff = slot[0] & GRCAN_MSG_IDE;
> - rtr = slot[0] & GRCAN_MSG_RTR;
> - if (eff) {
> - cf->can_id = ((slot[0] & GRCAN_MSG_EID)
> - >> GRCAN_MSG_EID_BIT);
> - cf->can_id |= CAN_EFF_FLAG;
> - } else {
> - cf->can_id = ((slot[0] & GRCAN_MSG_BID)
> - >> GRCAN_MSG_BID_BIT);
> - }
> - cf->len = can_cc_dlc2len((slot[1] & GRCAN_MSG_DLC)
> - >> GRCAN_MSG_DLC_BIT);
> - if (rtr) {
> - cf->can_id |= CAN_RTR_FLAG;
> - } else {
> - if (cf->can_dlc > 0) {
> - *(u32 *)(cf->data) = slot[2];
> - if (cf->can_dlc > 4)
> - *(u32 *)(cf->data + 4) = slot[3];
> + dlc = (slot[1] & GRCAN_MSG_DLC) >> GRCAN_MSG_DLC_BIT;
> + if (CHECK_SLOT_FDF(slot[1]))
> + cf->len = can_fd_dlc2len(dlc);
> + else
> + cf->len = can_cc_dlc2len(dlc);
> +
> + bds = grcan_numbds(cf->len);
> + payload_offset = 0;
> + data = cf->data;
> +
> + for (i = 0; i < bds; i++) {
> + slot = dma->rx.buf + rd;
> +
> + if (i == 0) {
> + eff = slot[0] & GRCAN_MSG_IDE;
> + rtr = slot[0] & GRCAN_MSG_RTR;
> + if (eff) {
> + cf->can_id = ((slot[0] & GRCAN_MSG_EID)
> + >> GRCAN_MSG_EID_BIT);
you can use up to 100 chars now, but please move the ">>" to the end of
the line
> + cf->can_id |= CAN_EFF_FLAG;
> + } else {
> + cf->can_id = ((slot[0] & GRCAN_MSG_BID)
> + >> GRCAN_MSG_BID_BIT);
> + }
> + if (rtr)
> + cf->can_id |= CAN_RTR_FLAG;
> +
> + dlc = (slot[1] & GRCAN_MSG_DLC) >> GRCAN_MSG_DLC_BIT;
> + if (CHECK_SLOT_FDF(slot[1]))
> + cf->len = can_fd_dlc2len(dlc);
> + else
> + cf->len = can_cc_dlc2len(dlc);
> +
> + copy_len = min(cf->len, GRCAN_CLASSIC_DATA_SIZE);
> + memcpy(data, &slot[2], copy_len);
> + payload_offset += copy_len;
> + } else {
> + copy_len = min(cf->len - payload_offset, GRCAN_MSG_SIZE);
> + memcpy(data + payload_offset, slot, copy_len);
> + payload_offset += copy_len;
> }
> -
> - stats->rx_bytes += cf->len;
> + rd += GRCAN_MSG_SIZE;
> + if (rd >= dma->tx.size)
> + rd -= dma->tx.size;
> }
> - stats->rx_packets++;
>
> + /* Update statistics and read pointer */
> + stats->rx_packets++;
> + stats->rx_bytes += cf->len;
> netif_receive_skb(skb);
> -
> - rd = grcan_ring_add(rd, GRCAN_MSG_SIZE, dma->rx.size);
> }
>
> /* Make sure everything is read before allowing hardware to
> @@ -1479,12 +1535,15 @@ static netdev_tx_t grcan_start_xmit(struct sk_buff *skb,
> struct grcan_priv *priv = netdev_priv(dev);
> struct grcan_registers __iomem *regs = priv->regs;
> struct grcan_dma *dma = &priv->dma;
> - struct can_frame *cf = (struct can_frame *)skb->data;
> - u32 id, txwr, txrd, space, txctrl;
> - int slotindex;
> - u32 *slot;
> - u32 rtr, eff, dlc, tmp, err;
> + struct can_frame *cf;
> + struct canfd_frame *cfd;
> + int i, bds, copy_len, payload_offset;
> unsigned long flags;
> + u8 *payload;
> + u8 len;
> + u32 *slot;
> + u32 eff, rtr, dlc, tmp, err, can_id;
> + u32 id, txwr, txrd, space, txctrl;
reverse-xmas-tree for all vars you touch please
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists