Marc Kleine-Budde wrote:
> Wolfgang Grandegger wrote:
>> 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. The skb's are now setup
>> as shown:
>
>> skb->dev = dev;
>
> nitpick: we use "netdev_alloc_skb()", so "skb->dev = dev" isn't needed
> any more. The code is allright, just the commit message is misleading.
>
>> skb->protocol = __constant_htons(ETH_P_CAN);
>> skb->ip_summed = CHECKSUM_UNNECESSARY;
>
>> Some drivers or library code used:
>
>> skb->protocol = htons(ETH_P_CAN); or
>> skb->pkt_type = PACKET_BROADCAST;
>
>> or did not set CHECKSUM_UNNECESSARY.
>
>> Please check and comment.
>
>> Marc, feel free to add your signed-off-by here.
>
> nitpick: as you did the patch, I can rather add my Acked-by: under your
> S-o-b.
>
>> Signed-off-by: Wolfgang Grandegger <[email protected]>
>> ---
>> kernel/2.6/drivers/net/can/at91_can.c | 32 ------------------
>> kernel/2.6/drivers/net/can/cc770/cc770.c | 13 +------
>> kernel/2.6/drivers/net/can/dev.c | 38
>> ++++++++++++++++++++--
>> kernel/2.6/drivers/net/can/esd_pci331.c | 14 +-------
>> kernel/2.6/drivers/net/can/mcp251x.c | 20 +----------
>> kernel/2.6/drivers/net/can/mscan/mscan.c | 6 ---
>> kernel/2.6/drivers/net/can/sja1000/sja1000.c | 12 +-----
>> kernel/2.6/drivers/net/can/softing/softing_main.c | 8 +---
>> kernel/2.6/drivers/net/can/usb/ems_usb.c | 16 +--------
>> kernel/2.6/include/socketcan/can/dev.h | 4 ++
>> 10 files changed, 54 insertions(+), 109 deletions(-)
>
>> Index: trunk/kernel/2.6/drivers/net/can/dev.c
>> ===================================================================
>> --- trunk.orig/kernel/2.6/drivers/net/can/dev.c
>> +++ trunk/kernel/2.6/drivers/net/can/dev.c
>> @@ -318,8 +318,7 @@ void can_put_echo_skb(struct sk_buff *sk
>> skb->sk = srcsk;
>
>> /* make settings for echo to reduce code in irq context */
>> - skb->protocol = htons(ETH_P_CAN);
>> - skb->pkt_type = PACKET_BROADCAST;
>
> is it save to remove this flag?
This was also my concern.
This pkt_type is also set in af_can.c and we should set it for now.
You might also use CAN netdevices with PACKET socket and wireshark, where the
PACKET_BROADCAST pkt_type is remarked.
If it is unnecessary i would send a separate patch to remove both of them.
>
>> + skb->protocol = __constant_htons(ETH_P_CAN);
>> skb->ip_summed = CHECKSUM_UNNECESSARY;
>> skb->dev = dev;
>
>> @@ -403,7 +402,8 @@ void can_restart(unsigned long data)
>> goto restart;
>> }
>> skb->dev = dev;
>> - skb->protocol = htons(ETH_P_CAN);
>> + 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 | CAN_ERR_RESTARTED;
>> @@ -496,6 +496,38 @@ static void can_setup(struct net_device
>> #endif
>> }
>
>> +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 = __constant_htons(ETH_P_CAN);
>> + skb->ip_summed = CHECKSUM_UNNECESSARY;
Please add 'skb->pkt_type = PACKET_BROADCAST' here.
>> + *cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
This could be also the place, where we could memset the struct can_frame to
zero, right?
Maybe we should do it here and leave it in alloc_can_err_skb() ...
>> +
>> + 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;
>> +
>> + memset(*cf, 0, sizeof(struct can_frame));
here :-)
>> + (*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
>> */
>> Index: trunk/kernel/2.6/drivers/net/can/at91_can.c
>> ===================================================================
>> --- trunk.orig/kernel/2.6/drivers/net/can/at91_can.c
>> +++ trunk/kernel/2.6/drivers/net/can/at91_can.c
>> @@ -221,38 +221,6 @@ static inline void set_mb_mode(const str
>> 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
>> */
>> Index: trunk/kernel/2.6/include/socketcan/can/dev.h
>> ===================================================================
>> --- trunk.orig/kernel/2.6/include/socketcan/can/dev.h
>> +++ trunk/kernel/2.6/include/socketcan/can/dev.h
>> @@ -88,4 +88,8 @@ void can_put_echo_skb(struct sk_buff *sk
>> 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 */
>> Index: trunk/kernel/2.6/drivers/net/can/cc770/cc770.c
>> ===================================================================
>> --- trunk.orig/kernel/2.6/drivers/net/can/cc770/cc770.c
>> +++ trunk/kernel/2.6/drivers/net/can/cc770/cc770.c
>> @@ -517,13 +517,10 @@ static void cc770_rx(struct net_device *
>> u32 id;
>> 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);
>
>> - cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
>> config = cc770_read_reg(priv, msgobj[mo].config);
>
>> if (ctrl1 & RMTPND_SET) {
>> @@ -582,15 +579,9 @@ static int cc770_err(struct net_device *
>
>> dev_dbg(ND2D(dev), "status interrupt (%#x)\n", status);
>
>> - 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 (status & STAT_BOFF) {
>> /* Disable interrupts */
>> Index: trunk/kernel/2.6/drivers/net/can/esd_pci331.c
>> ===================================================================
>> --- trunk.orig/kernel/2.6/drivers/net/can/esd_pci331.c
>> +++ trunk/kernel/2.6/drivers/net/can/esd_pci331.c
>> @@ -439,7 +439,7 @@ static int esd331_create_err_frame(struc
>> struct can_frame *cf;
>> struct sk_buff *skb;
>
>> - skb = dev_alloc_skb(sizeof(*cf));
>> + skb = alloc_can_err_skb(dev, &cf);
>> if (unlikely(skb == NULL))
>> return -ENOMEM;
>
>> @@ -449,13 +449,7 @@ static int esd331_create_err_frame(struc
>> stats = &dev->stats;
>> #endif
>
>> - skb->dev = dev;
>> - skb->protocol = htons(ETH_P_CAN);
>> - cf = (struct can_frame *)skb_put(skb, sizeof(*cf));
>> - memset(cf, 0, sizeof(*cf));
>> -
>> - cf->can_id = CAN_ERR_FLAG | idflags;
>> - cf->can_dlc = CAN_ERR_DLC;
>> + cf->can_id |= idflags;
>> cf->data[1] = d1;
>
>> netif_rx(skb);
>> @@ -481,14 +475,12 @@ static void esd331_irq_rx(struct net_dev
>> struct sk_buff *skb;
>> int i;
>
>> - skb = netdev_alloc_skb(dev, sizeof(*cfrm));
>> + skb = alloc_can_skb(dev, &cfrm);
>> if (unlikely(skb == NULL)) {
>> stats->rx_dropped++;
>> return;
>> }
>> - skb->protocol = htons(ETH_P_CAN);
>
>> - cfrm = (struct can_frame *)skb_put(skb, sizeof(*cfrm));
>> memset(cfrm, 0, sizeof(*cfrm));
This memset should be removed.
>
>> if (eff) {
>> Index: trunk/kernel/2.6/drivers/net/can/mcp251x.c
>> ===================================================================
>> --- trunk.orig/kernel/2.6/drivers/net/can/mcp251x.c
>> +++ trunk/kernel/2.6/drivers/net/can/mcp251x.c
>> @@ -357,15 +357,13 @@ static void mcp251x_hw_rx(struct spi_dev
>> struct sk_buff *skb;
>> struct can_frame *frame;
>
>> - skb = dev_alloc_skb(sizeof(struct can_frame));
>> + skb = alloc_can_skb(priv->net, &frame);
>> if (!skb) {
>> dev_err(&spi->dev, "%s: out of memory for Rx'd frame\n",
>> __func__);
>> priv->net->stats.rx_dropped++;
>> return;
>> }
>> - skb->dev = priv->net;
>> - frame = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
>
>> if (pdata->model == CAN_MCP251X_MCP2510) {
>> int i;
>> @@ -457,9 +455,6 @@ static void mcp251x_hw_rx(struct spi_dev
>> priv->net->stats.rx_packets++;
>> priv->net->stats.rx_bytes += frame->can_dlc;
>
>> - skb->protocol = __constant_htons(ETH_P_CAN);
>> - skb->pkt_type = PACKET_BROADCAST;
>> - skb->ip_summed = CHECKSUM_UNNECESSARY;
>> netif_rx(skb);
>> }
>
>> @@ -808,19 +803,8 @@ static void mcp251x_irq_work_handler(str
>> u8 eflag = mcp251x_read_reg(spi, EFLG);
>
>> /* create error frame */
>> - skb = dev_alloc_skb(sizeof(struct can_frame));
>> + skb = alloc_can_err_skb(net, &frame);
>> if (skb) {
>> - frame = (struct can_frame *)
>> - skb_put(skb, sizeof(struct can_frame));
>> - *(unsigned long long *)&frame->data = 0ULL;
>> - frame->can_id = CAN_ERR_FLAG;
>> - frame->can_dlc = CAN_ERR_DLC;
>> -
>> - skb->dev = net;
>> - skb->protocol = __constant_htons(ETH_P_CAN);
>> - skb->pkt_type = PACKET_BROADCAST;
>> - skb->ip_summed = CHECKSUM_UNNECESSARY;
>> -
>> /* Set error frame flags based on bus state */
>> if (eflag & EFLG_TXBO) {
>> frame->can_id |= CAN_ERR_BUSOFF;
>> Index: trunk/kernel/2.6/drivers/net/can/mscan/mscan.c
>> ===================================================================
>> --- trunk.orig/kernel/2.6/drivers/net/can/mscan/mscan.c
>> +++ trunk/kernel/2.6/drivers/net/can/mscan/mscan.c
>> @@ -364,7 +364,7 @@ static int mscan_rx_poll(struct net_devi
>> while (npackets < quota && ((canrflg = in_8(®s->canrflg)) &
>> (MSCAN_RXF | MSCAN_ERR_IF))) {
>
>> - skb = dev_alloc_skb(sizeof(struct can_frame));
>> + skb = alloc_can_skb(dev, &frame);
>> if (!skb) {
>> if (printk_ratelimit())
>> dev_notice(ND2D(dev), "packet dropped\n");
>> @@ -373,7 +373,6 @@ static int mscan_rx_poll(struct net_devi
>> continue;
>> }
>
>> - frame = (struct can_frame *)skb_put(skb, sizeof(*frame));
>> memset(frame, 0, sizeof(*frame));
Again: remove memset
>
>> if (canrflg & MSCAN_RXF) {
>> @@ -459,9 +458,6 @@ static int mscan_rx_poll(struct net_devi
>> }
>
>> npackets++;
>> - skb->dev = dev;
>> - skb->protocol = __constant_htons(ETH_P_CAN);
>> - skb->ip_summed = CHECKSUM_UNNECESSARY;
>> netif_receive_skb(skb);
>> }
>
>> Index: trunk/kernel/2.6/drivers/net/can/sja1000/sja1000.c
>> ===================================================================
>> --- trunk.orig/kernel/2.6/drivers/net/can/sja1000/sja1000.c
>> +++ trunk/kernel/2.6/drivers/net/can/sja1000/sja1000.c
>> @@ -308,11 +308,9 @@ static void sja1000_rx(struct net_device
>> 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;
>> @@ -370,15 +368,9 @@ static int sja1000_err(struct net_device
>> 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 */
>> Index: trunk/kernel/2.6/drivers/net/can/softing/softing_main.c
>> ===================================================================
>> --- trunk.orig/kernel/2.6/drivers/net/can/softing/softing_main.c
>> +++ trunk/kernel/2.6/drivers/net/can/softing/softing_main.c
>> @@ -143,16 +143,14 @@ int softing_rx(struct net_device *netdev
>> ktime_t ktime)
>> {
>> struct sk_buff *skb;
>> + struct can_frame *cf;
>> int ret;
>> struct net_device_stats *stats;
>
>> - skb = dev_alloc_skb(sizeof(msg));
>> + skb = alloc_can_skb(netdev, &cf);
>> if (!skb)
>> return -ENOMEM;
>> - skb->dev = netdev;
>> - skb->protocol = htons(ETH_P_CAN);
>> - skb->ip_summed = CHECKSUM_UNNECESSARY;
>> - memcpy(skb_put(skb, sizeof(*msg)), msg, sizeof(*msg));
>> + memcpy(cf, msg, sizeof(*msg));
>> skb->tstamp = ktime;
>> ret = netif_rx(skb);
>> if (ret == NET_RX_DROP) {
>> Index: trunk/kernel/2.6/drivers/net/can/usb/ems_usb.c
>> ===================================================================
>> --- trunk.orig/kernel/2.6/drivers/net/can/usb/ems_usb.c
>> +++ trunk/kernel/2.6/drivers/net/can/usb/ems_usb.c
>> @@ -311,14 +311,10 @@ static void ems_usb_rx_can_msg(struct em
>> 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);
>
>> @@ -349,18 +345,10 @@ static void ems_usb_rx_err(struct ems_us
>> 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;
>
The rest looks good to me.
Nice cleanup!
Regards,
Oliver
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
