[PATCH v5 03/12] can: slcan: use the alloc_can_skb() helper

From: Dario Binacchi
Date: Tue Jun 28 2022 - 12:36:01 EST


It is used successfully by most (if not all) CAN device drivers. It
allows to remove replicated code.

Signed-off-by: Dario Binacchi <dario.binacchi@xxxxxxxxxxxxxxxxxxxx>

---

(no changes since v3)

Changes in v3:
- Increment the error counter in case of decoding failure.

Changes in v2:
- Put the data into the allocated skb directly instead of first
filling the "cf" on the stack and then doing a memcpy().

drivers/net/can/slcan.c | 70 +++++++++++++++++++----------------------
1 file changed, 33 insertions(+), 37 deletions(-)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 6162a9c21672..c39580b142e0 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -54,6 +54,7 @@
#include <linux/kernel.h>
#include <linux/workqueue.h>
#include <linux/can.h>
+#include <linux/can/dev.h>
#include <linux/can/skb.h>
#include <linux/can/can-ml.h>

@@ -143,85 +144,80 @@ static struct net_device **slcan_devs;
static void slc_bump(struct slcan *sl)
{
struct sk_buff *skb;
- struct can_frame cf;
+ struct can_frame *cf;
int i, tmp;
u32 tmpid;
char *cmd = sl->rbuff;

- memset(&cf, 0, sizeof(cf));
+ skb = alloc_can_skb(sl->dev, &cf);
+ if (unlikely(!skb)) {
+ sl->dev->stats.rx_dropped++;
+ return;
+ }

switch (*cmd) {
case 'r':
- cf.can_id = CAN_RTR_FLAG;
+ cf->can_id = CAN_RTR_FLAG;
fallthrough;
case 't':
/* store dlc ASCII value and terminate SFF CAN ID string */
- cf.len = sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN];
+ cf->len = sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN];
sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN] = 0;
/* point to payload data behind the dlc */
cmd += SLC_CMD_LEN + SLC_SFF_ID_LEN + 1;
break;
case 'R':
- cf.can_id = CAN_RTR_FLAG;
+ cf->can_id = CAN_RTR_FLAG;
fallthrough;
case 'T':
- cf.can_id |= CAN_EFF_FLAG;
+ cf->can_id |= CAN_EFF_FLAG;
/* store dlc ASCII value and terminate EFF CAN ID string */
- cf.len = sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN];
+ cf->len = sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN];
sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN] = 0;
/* point to payload data behind the dlc */
cmd += SLC_CMD_LEN + SLC_EFF_ID_LEN + 1;
break;
default:
- return;
+ goto decode_failed;
}

if (kstrtou32(sl->rbuff + SLC_CMD_LEN, 16, &tmpid))
- return;
+ goto decode_failed;

- cf.can_id |= tmpid;
+ cf->can_id |= tmpid;

/* get len from sanitized ASCII value */
- if (cf.len >= '0' && cf.len < '9')
- cf.len -= '0';
+ if (cf->len >= '0' && cf->len < '9')
+ cf->len -= '0';
else
- return;
+ goto decode_failed;

/* RTR frames may have a dlc > 0 but they never have any data bytes */
- if (!(cf.can_id & CAN_RTR_FLAG)) {
- for (i = 0; i < cf.len; i++) {
+ if (!(cf->can_id & CAN_RTR_FLAG)) {
+ for (i = 0; i < cf->len; i++) {
tmp = hex_to_bin(*cmd++);
if (tmp < 0)
- return;
- cf.data[i] = (tmp << 4);
+ goto decode_failed;
+
+ cf->data[i] = (tmp << 4);
tmp = hex_to_bin(*cmd++);
if (tmp < 0)
- return;
- cf.data[i] |= tmp;
+ goto decode_failed;
+
+ cf->data[i] |= tmp;
}
}

- skb = dev_alloc_skb(sizeof(struct can_frame) +
- sizeof(struct can_skb_priv));
- if (!skb)
- return;
-
- skb->dev = sl->dev;
- skb->protocol = htons(ETH_P_CAN);
- skb->pkt_type = PACKET_BROADCAST;
- skb->ip_summed = CHECKSUM_UNNECESSARY;
-
- can_skb_reserve(skb);
- can_skb_prv(skb)->ifindex = sl->dev->ifindex;
- can_skb_prv(skb)->skbcnt = 0;
-
- skb_put_data(skb, &cf, sizeof(struct can_frame));
-
sl->dev->stats.rx_packets++;
- if (!(cf.can_id & CAN_RTR_FLAG))
- sl->dev->stats.rx_bytes += cf.len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ sl->dev->stats.rx_bytes += cf->len;

netif_rx(skb);
+ return;
+
+decode_failed:
+ sl->dev->stats.rx_errors++;
+ dev_kfree_skb(skb);
}

/* parse tty input stream */
--
2.32.0