From a6e4bc5304033e434fabccabb230b8e9ff55d76f Mon Sep 17 00:00:00 2001 From: Wolfgang Grandegger Date: Thu, 8 Oct 2009 22:17:11 +0000 Subject: can: make the number of echo skb's configurable This patch allows the CAN controller driver to define the number of echo skb's used for the local loopback (echo), as suggested by Kurt Van Dijck, with the function: struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max); The CAN drivers have been adapted accordingly. For the ems_usb driver, as suggested by Sebastian Haas, the number of echo skb's has been increased to 10, which improves the transmission performance a lot. Signed-off-by: Wolfgang Grandegger Signed-off-by: Kurt Van Dijck Signed-off-by: David S. Miller --- include/linux/can/dev.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'include/linux/can/dev.h') diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index 5824b20b5fcb..1d3f7f00e3af 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -29,8 +29,6 @@ enum can_mode { /* * CAN common private data */ -#define CAN_ECHO_SKB_MAX 4 - struct can_priv { struct can_device_stats can_stats; @@ -44,15 +42,16 @@ struct can_priv { int restart_ms; struct timer_list restart_timer; - struct sk_buff *echo_skb[CAN_ECHO_SKB_MAX]; - int (*do_set_bittiming)(struct net_device *dev); int (*do_set_mode)(struct net_device *dev, enum can_mode mode); int (*do_get_state)(const struct net_device *dev, enum can_state *state); + + unsigned int echo_skb_max; + struct sk_buff **echo_skb; }; -struct net_device *alloc_candev(int sizeof_priv); +struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max); void free_candev(struct net_device *dev); int open_candev(struct net_device *dev); @@ -64,8 +63,9 @@ void unregister_candev(struct net_device *dev); int can_restart_now(struct net_device *dev); void can_bus_off(struct net_device *dev); -void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, int idx); -void can_get_echo_skb(struct net_device *dev, int idx); -void can_free_echo_skb(struct net_device *dev, int idx); +void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, + unsigned int idx); +void can_get_echo_skb(struct net_device *dev, unsigned int idx); +void can_free_echo_skb(struct net_device *dev, unsigned int idx); #endif /* CAN_DEV_H */ -- cgit v1.2.3 From 7b6856a0296a8f187bb88ba31fa83a08abba7966 Mon Sep 17 00:00:00 2001 From: Wolfgang Grandegger Date: Tue, 20 Oct 2009 00:08:01 -0700 Subject: can: provide library functions for skb allocation This patch makes the private functions alloc_can_skb() and alloc_can_err_skb() of the at91_can driver public and adapts all drivers to use these. While making the patch I realized, that the skb's are *not* setup consistently. It's now done as shown below: skb->protocol = htons(ETH_P_CAN); skb->pkt_type = PACKET_BROADCAST; skb->ip_summed = CHECKSUM_UNNECESSARY; *cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame)); memset(*cf, 0, sizeof(struct can_frame)); The frame is zeroed out to avoid uninitialized data to be passed to user space. Some drivers or library code did not set "pkt_type" or "ip_summed". Also, "__constant_htons()" should not be used for runtime invocations, as pointed out by David Miller. Signed-off-by: Wolfgang Grandegger Signed-off-by: David S. Miller --- drivers/net/can/at91_can.c | 32 ----------------------------- drivers/net/can/dev.c | 42 ++++++++++++++++++++++++++++++++------- drivers/net/can/sja1000/sja1000.c | 12 ++--------- drivers/net/can/ti_hecc.c | 17 ++++------------ drivers/net/can/usb/ems_usb.c | 16 ++------------- include/linux/can/dev.h | 4 ++++ 6 files changed, 47 insertions(+), 76 deletions(-) (limited to 'include/linux/can/dev.h') diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c index b13fd9114130..cbe3fce53e3b 100644 --- a/drivers/net/can/at91_can.c +++ b/drivers/net/can/at91_can.c @@ -221,38 +221,6 @@ static inline void set_mb_mode(const struct at91_priv *priv, unsigned int mb, set_mb_mode_prio(priv, mb, mode, 0); } -static struct sk_buff *alloc_can_skb(struct net_device *dev, - struct can_frame **cf) -{ - struct sk_buff *skb; - - skb = netdev_alloc_skb(dev, sizeof(struct can_frame)); - if (unlikely(!skb)) - return NULL; - - skb->protocol = htons(ETH_P_CAN); - skb->ip_summed = CHECKSUM_UNNECESSARY; - *cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame)); - - return skb; -} - -static struct sk_buff *alloc_can_err_skb(struct net_device *dev, - struct can_frame **cf) -{ - struct sk_buff *skb; - - skb = alloc_can_skb(dev, cf); - if (unlikely(!skb)) - return NULL; - - memset(*cf, 0, sizeof(struct can_frame)); - (*cf)->can_id = CAN_ERR_FLAG; - (*cf)->can_dlc = CAN_ERR_DLC; - - return skb; -} - /* * Swtich transceiver on or off */ diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index 39b99f57c265..c3db111d2ff5 100644 --- a/drivers/net/can/dev.c +++ b/drivers/net/can/dev.c @@ -366,17 +366,12 @@ void can_restart(unsigned long data) can_flush_echo_skb(dev); /* send restart message upstream */ - skb = dev_alloc_skb(sizeof(struct can_frame)); + skb = alloc_can_err_skb(dev, &cf); if (skb == NULL) { err = -ENOMEM; goto restart; } - skb->dev = dev; - skb->protocol = htons(ETH_P_CAN); - cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame)); - memset(cf, 0, sizeof(struct can_frame)); - cf->can_id = CAN_ERR_FLAG | CAN_ERR_RESTARTED; - cf->can_dlc = CAN_ERR_DLC; + cf->can_id |= CAN_ERR_RESTARTED; netif_rx(skb); @@ -449,6 +444,39 @@ static void can_setup(struct net_device *dev) dev->features = NETIF_F_NO_CSUM; } +struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf) +{ + struct sk_buff *skb; + + skb = netdev_alloc_skb(dev, sizeof(struct can_frame)); + if (unlikely(!skb)) + return NULL; + + skb->protocol = htons(ETH_P_CAN); + skb->pkt_type = PACKET_BROADCAST; + skb->ip_summed = CHECKSUM_UNNECESSARY; + *cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame)); + memset(*cf, 0, sizeof(struct can_frame)); + + return skb; +} +EXPORT_SYMBOL_GPL(alloc_can_skb); + +struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf) +{ + struct sk_buff *skb; + + skb = alloc_can_skb(dev, cf); + if (unlikely(!skb)) + return NULL; + + (*cf)->can_id = CAN_ERR_FLAG; + (*cf)->can_dlc = CAN_ERR_DLC; + + return skb; +} +EXPORT_SYMBOL_GPL(alloc_can_err_skb); + /* * Allocate and setup space for the CAN network device */ diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c index 96d8be4253f8..1a9c5958bbd7 100644 --- a/drivers/net/can/sja1000/sja1000.c +++ b/drivers/net/can/sja1000/sja1000.c @@ -296,11 +296,9 @@ static void sja1000_rx(struct net_device *dev) uint8_t dlc; int i; - skb = dev_alloc_skb(sizeof(struct can_frame)); + skb = alloc_can_skb(dev, &cf); if (skb == NULL) return; - skb->dev = dev; - skb->protocol = htons(ETH_P_CAN); fi = priv->read_reg(priv, REG_FI); dlc = fi & 0x0F; @@ -351,15 +349,9 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status) enum can_state state = priv->can.state; uint8_t ecc, alc; - skb = dev_alloc_skb(sizeof(struct can_frame)); + skb = alloc_can_err_skb(dev, &cf); if (skb == NULL) return -ENOMEM; - skb->dev = dev; - skb->protocol = htons(ETH_P_CAN); - cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame)); - memset(cf, 0, sizeof(struct can_frame)); - cf->can_id = CAN_ERR_FLAG; - cf->can_dlc = CAN_ERR_DLC; if (isrc & IRQ_DOI) { /* data overrun interrupt */ diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c index 23a7128e4eb7..07e8016b17ec 100644 --- a/drivers/net/can/ti_hecc.c +++ b/drivers/net/can/ti_hecc.c @@ -535,18 +535,15 @@ static int ti_hecc_rx_pkt(struct ti_hecc_priv *priv, int mbxno) u32 data, mbx_mask; unsigned long flags; - skb = netdev_alloc_skb(priv->ndev, sizeof(struct can_frame)); + skb = alloc_can_skb(priv->ndev, &cf); if (!skb) { if (printk_ratelimit()) dev_err(priv->ndev->dev.parent, - "ti_hecc_rx_pkt: netdev_alloc_skb() failed\n"); + "ti_hecc_rx_pkt: alloc_can_skb() failed\n"); return -ENOMEM; } - skb->protocol = __constant_htons(ETH_P_CAN); - skb->ip_summed = CHECKSUM_UNNECESSARY; mbx_mask = BIT(mbxno); - cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame)); data = hecc_read_mbx(priv, mbxno, HECC_CANMID); if (data & HECC_CANMID_IDE) cf->can_id = (data & CAN_EFF_MASK) | CAN_EFF_FLAG; @@ -656,19 +653,13 @@ static int ti_hecc_error(struct net_device *ndev, int int_status, struct sk_buff *skb; /* propogate the error condition to the can stack */ - skb = netdev_alloc_skb(ndev, sizeof(struct can_frame)); + skb = alloc_can_err_skb(ndev, &cf); if (!skb) { if (printk_ratelimit()) dev_err(priv->ndev->dev.parent, - "ti_hecc_error: netdev_alloc_skb() failed\n"); + "ti_hecc_error: alloc_can_err_skb() failed\n"); return -ENOMEM; } - skb->protocol = __constant_htons(ETH_P_CAN); - skb->ip_summed = CHECKSUM_UNNECESSARY; - cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame)); - memset(cf, 0, sizeof(struct can_frame)); - cf->can_id = CAN_ERR_FLAG; - cf->can_dlc = CAN_ERR_DLC; if (int_status & HECC_CANGIF_WLIF) { /* warning level int */ if ((int_status & HECC_CANGIF_BOIF) == 0) { diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c index a65f56a9cd3d..3685f3e42d12 100644 --- a/drivers/net/can/usb/ems_usb.c +++ b/drivers/net/can/usb/ems_usb.c @@ -311,14 +311,10 @@ static void ems_usb_rx_can_msg(struct ems_usb *dev, struct ems_cpc_msg *msg) int i; struct net_device_stats *stats = &dev->netdev->stats; - skb = netdev_alloc_skb(dev->netdev, sizeof(struct can_frame)); + skb = alloc_can_skb(dev->netdev, &cf); if (skb == NULL) return; - skb->protocol = htons(ETH_P_CAN); - - cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame)); - cf->can_id = msg->msg.can_msg.id; cf->can_dlc = min_t(u8, msg->msg.can_msg.length, 8); @@ -346,18 +342,10 @@ static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg) struct sk_buff *skb; struct net_device_stats *stats = &dev->netdev->stats; - skb = netdev_alloc_skb(dev->netdev, sizeof(struct can_frame)); + skb = alloc_can_err_skb(dev->netdev, &cf); if (skb == NULL) return; - skb->protocol = htons(ETH_P_CAN); - - cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame)); - memset(cf, 0, sizeof(struct can_frame)); - - cf->can_id = CAN_ERR_FLAG; - cf->can_dlc = CAN_ERR_DLC; - if (msg->type == CPC_MSG_TYPE_CAN_STATE) { u8 state = msg->msg.can_state; diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index 1d3f7f00e3af..1ed2a5cc03f5 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -68,4 +68,8 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, void can_get_echo_skb(struct net_device *dev, unsigned int idx); void can_free_echo_skb(struct net_device *dev, unsigned int idx); +struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf); +struct sk_buff *alloc_can_err_skb(struct net_device *dev, + struct can_frame **cf); + #endif /* CAN_DEV_H */ -- cgit v1.2.3 From c7cd606f60e7679c7f9eee7010f02a6f000209c1 Mon Sep 17 00:00:00 2001 From: Oliver Hartkopp Date: Sat, 12 Dec 2009 04:13:21 +0000 Subject: can: Fix data length code handling in rx path A valid CAN dataframe can have a data length code (DLC) of 0 .. 8 data bytes. When reading the CAN controllers register the 4-bit value may contain values from 0 .. 15 which may exceed the reserved space in the socket buffer! The ISO 11898-1 Chapter 8.4.2.3 (DLC field) says that register values > 8 should be reduced to 8 without any error reporting or frame drop. This patch introduces a new helper macro to cast a given 4-bit data length code (dlc) to __u8 and ensure the DLC value to be max. 8 bytes. The different handlings in the rx path of the CAN netdevice drivers are fixed. Signed-off-by: Oliver Hartkopp Signed-off-by: Wolfgang Grandegger Signed-off-by: David S. Miller --- drivers/net/can/at91_can.c | 2 +- drivers/net/can/bfin_can.c | 2 +- drivers/net/can/mcp251x.c | 13 +++---------- drivers/net/can/mscan/mscan.c | 3 ++- drivers/net/can/sja1000/sja1000.c | 18 ++++++++---------- drivers/net/can/ti_hecc.c | 2 +- drivers/net/can/usb/ems_usb.c | 2 +- include/linux/can/dev.h | 9 +++++++++ 8 files changed, 26 insertions(+), 25 deletions(-) (limited to 'include/linux/can/dev.h') diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c index cbe3fce53e3b..d0ec17878ffc 100644 --- a/drivers/net/can/at91_can.c +++ b/drivers/net/can/at91_can.c @@ -474,7 +474,7 @@ static void at91_read_mb(struct net_device *dev, unsigned int mb, reg_msr = at91_read(priv, AT91_MSR(mb)); if (reg_msr & AT91_MSR_MRTR) cf->can_id |= CAN_RTR_FLAG; - cf->can_dlc = min_t(__u8, (reg_msr >> 16) & 0xf, 8); + cf->can_dlc = get_can_dlc((reg_msr >> 16) & 0xf); *(u32 *)(cf->data + 0) = at91_read(priv, AT91_MDL(mb)); *(u32 *)(cf->data + 4) = at91_read(priv, AT91_MDH(mb)); diff --git a/drivers/net/can/bfin_can.c b/drivers/net/can/bfin_can.c index c7fc1de28173..0ec1524523cc 100644 --- a/drivers/net/can/bfin_can.c +++ b/drivers/net/can/bfin_can.c @@ -392,7 +392,7 @@ static void bfin_can_rx(struct net_device *dev, u16 isrc) cf->can_id |= CAN_RTR_FLAG; /* get data length code */ - cf->can_dlc = bfin_read16(®->chl[obj].dlc); + cf->can_dlc = get_can_dlc(bfin_read16(®->chl[obj].dlc) & 0xF); /* get payload */ for (i = 0; i < 8; i += 2) { diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c index 78b1b69b2921..9c5a1537939c 100644 --- a/drivers/net/can/mcp251x.c +++ b/drivers/net/can/mcp251x.c @@ -403,9 +403,8 @@ static void mcp251x_hw_rx_frame(struct spi_device *spi, u8 *buf, for (i = 1; i < RXBDAT_OFF; i++) buf[i] = mcp251x_read_reg(spi, RXBCTRL(buf_idx) + i); - len = buf[RXBDLC_OFF] & RXBDLC_LEN_MASK; - if (len > 8) - len = 8; + + len = get_can_dlc(buf[RXBDLC_OFF] & RXBDLC_LEN_MASK); for (; i < (RXBDAT_OFF + len); i++) buf[i] = mcp251x_read_reg(spi, RXBCTRL(buf_idx) + i); } else { @@ -455,13 +454,7 @@ static void mcp251x_hw_rx(struct spi_device *spi, int buf_idx) (buf[RXBSIDL_OFF] >> RXBSIDL_SHIFT); } /* Data length */ - frame->can_dlc = buf[RXBDLC_OFF] & RXBDLC_LEN_MASK; - if (frame->can_dlc > 8) { - dev_warn(&spi->dev, "invalid frame recevied\n"); - priv->net->stats.rx_errors++; - dev_kfree_skb(skb); - return; - } + frame->can_dlc = get_can_dlc(buf[RXBDLC_OFF] & RXBDLC_LEN_MASK); memcpy(frame->data, buf + RXBDAT_OFF, frame->can_dlc); priv->net->stats.rx_packets++; diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c index bb06dfb58f25..07346f880ca6 100644 --- a/drivers/net/can/mscan/mscan.c +++ b/drivers/net/can/mscan/mscan.c @@ -297,7 +297,8 @@ static void mscan_get_rx_frame(struct net_device *dev, struct can_frame *frame) frame->can_id |= can_id >> 1; if (can_id & 1) frame->can_id |= CAN_RTR_FLAG; - frame->can_dlc = in_8(®s->rx.dlr) & 0xf; + + frame->can_dlc = get_can_dlc(in_8(®s->rx.dlr) & 0xf); if (!(frame->can_id & CAN_RTR_FLAG)) { void __iomem *data = ®s->rx.dsr1_0; diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c index b4ba88a31075..542a4f7255b4 100644 --- a/drivers/net/can/sja1000/sja1000.c +++ b/drivers/net/can/sja1000/sja1000.c @@ -293,15 +293,14 @@ static void sja1000_rx(struct net_device *dev) uint8_t fi; uint8_t dreg; canid_t id; - uint8_t dlc; int i; + /* create zero'ed CAN frame buffer */ skb = alloc_can_skb(dev, &cf); if (skb == NULL) return; fi = priv->read_reg(priv, REG_FI); - dlc = fi & 0x0F; if (fi & FI_FF) { /* extended frame format (EFF) */ @@ -318,16 +317,15 @@ static void sja1000_rx(struct net_device *dev) | (priv->read_reg(priv, REG_ID2) >> 5); } - if (fi & FI_RTR) + if (fi & FI_RTR) { id |= CAN_RTR_FLAG; + } else { + cf->can_dlc = get_can_dlc(fi & 0x0F); + for (i = 0; i < cf->can_dlc; i++) + cf->data[i] = priv->read_reg(priv, dreg++); + } cf->can_id = id; - cf->can_dlc = dlc; - for (i = 0; i < dlc; i++) - cf->data[i] = priv->read_reg(priv, dreg++); - - while (i < 8) - cf->data[i++] = 0; /* release receive buffer */ priv->write_reg(priv, REG_CMR, CMD_RRB); @@ -335,7 +333,7 @@ static void sja1000_rx(struct net_device *dev) netif_rx(skb); stats->rx_packets++; - stats->rx_bytes += dlc; + stats->rx_bytes += cf->can_dlc; } static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status) diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c index 07e8016b17ec..5c993c2da528 100644 --- a/drivers/net/can/ti_hecc.c +++ b/drivers/net/can/ti_hecc.c @@ -552,7 +552,7 @@ static int ti_hecc_rx_pkt(struct ti_hecc_priv *priv, int mbxno) data = hecc_read_mbx(priv, mbxno, HECC_CANMCF); if (data & HECC_CANMCF_RTR) cf->can_id |= CAN_RTR_FLAG; - cf->can_dlc = data & 0xF; + cf->can_dlc = get_can_dlc(data & 0xF); data = hecc_read_mbx(priv, mbxno, HECC_CANMDL); *(u32 *)(cf->data) = cpu_to_be32(data); if (cf->can_dlc > 4) { diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c index 591eb0eb1c2b..efbb05c71bf4 100644 --- a/drivers/net/can/usb/ems_usb.c +++ b/drivers/net/can/usb/ems_usb.c @@ -316,7 +316,7 @@ static void ems_usb_rx_can_msg(struct ems_usb *dev, struct ems_cpc_msg *msg) return; cf->can_id = le32_to_cpu(msg->msg.can_msg.id); - cf->can_dlc = min_t(u8, msg->msg.can_msg.length, 8); + cf->can_dlc = get_can_dlc(msg->msg.can_msg.length & 0xF); if (msg->type == CPC_MSG_TYPE_EXT_CAN_FRAME || msg->type == CPC_MSG_TYPE_EXT_RTR_FRAME) diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index 1ed2a5cc03f5..3db7767d2a17 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -51,6 +51,15 @@ struct can_priv { struct sk_buff **echo_skb; }; +/* + * get_can_dlc(value) - helper macro to cast a given data length code (dlc) + * to __u8 and ensure the dlc value to be max. 8 bytes. + * + * To be used in the CAN netdriver receive path to ensure conformance with + * ISO 11898-1 Chapter 8.4.2.3 (DLC field) + */ +#define get_can_dlc(i) (min_t(__u8, (i), 8)) + struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max); void free_candev(struct net_device *dev); -- cgit v1.2.3