On 08.06.2022 18:51:06, Dario Binacchi wrote:
> 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>
>
> ---
>
> 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 | 69 +++++++++++++++++++----------------------
> 1 file changed, 32 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index 6162a9c21672..5d87e25e2285 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,79 @@ 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:
> + dev_kfree_skb(skb);
Can you increase an error counter in this situation, too?
Marc
> }
>
> /* parse tty input stream */
> --
> 2.32.0
>
>
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
