Hello Wolfgang,
>> This patch fixes sending erroneous frames when loopback is disabled. >> Also stats->txBytes is now only increased in TX-done interrupt and not >> earlier. >> >> Thomas >> >> >> Signed-off-by: thomas.koerper <[email protected]> >> --- >> Index: kernel/2.6/drivers/net/can/esd_pci331.c >> =================================================================== >> --- kernel/2.6/drivers/net/can/esd_pci331.c (revision 1103) >> +++ kernel/2.6/drivers/net/can/esd_pci331.c (working copy) >> @@ -318,32 +318,6 @@ >> return err; >> } >> >> -static int esd331_send(struct esd331_pci *board, u8 pci331net, unsigned >> long id, >> - int eff, int rtr, u16 dlc, u8 *data) >> -{ >> - struct esd331_can_msg msg; >> - int i; >> - >> - memset(&msg, 0, sizeof(msg)); >> - >> - if (eff) { >> - msg.cmmd = ESD331_I20_EX_BCAN; >> - msg.id = cpu_to_be16((id >> 16) & 0x1fff); >> - msg.x2 = cpu_to_be16(id); >> - } else { >> - msg.cmmd = ESD331_I20_BCAN; >> - msg.id = cpu_to_be16(id); >> - } >> - msg.net = pci331net; >> - msg.len = cpu_to_be16(rtr ? dlc | ESD331_RTR_FLAG : dlc); >> - >> - i = (dlc < 8) ? dlc : 8; >> - while (i--) >> - msg.data[i] = data[i]; >> - >> - return esd331_write(&msg, board); >> -} >> - >> static int esd331_write_allid(u8 net, struct esd331_pci *board) >> { >> struct esd331_can_msg mesg; >> @@ -583,6 +557,7 @@ >> case ESD331_I20_TXDONE: >> case ESD331_I20_EX_TXDONE: >> stats->tx_packets++; >> + stats->tx_bytes += msg.x1; >> can_get_echo_skb(dev, 0); >> netif_wake_queue(dev); >> break; >> @@ -693,24 +668,36 @@ >> struct net_device_stats *stats = &dev->stats; >> #endif >> struct can_frame *cf = (struct can_frame *)skb->data; >> - int err; >> + struct esd331_can_msg msg; >> + int err = NETDEV_TX_OK; > > As "err" does not hold an error code, "ret" seems more appropriate. > Maybe you do not even need that variable. Well, of course I could avoid the variable and just call "return NETDEV_TX..." anywhere in the function. But I think I was told not to do that and use "goto" instead. Just tell me how it should look like, will put the complete function at the end of this post. >> + int i; >> >> - can_put_echo_skb(skb, dev, 0); >> - >> if ((cf->can_id & CAN_EFF_FLAG) && (priv->board->eff_supp == 0)) { >> stats->tx_errors++; >> stats->tx_dropped++; >> - can_free_echo_skb(dev, 0); >> - err = -EOPNOTSUPP; >> goto out; >> } >> >> - err = esd331_send(priv->board, priv->boards_net, >> - cf->can_id & CAN_EFF_MASK, >> - cf->can_id & CAN_EFF_FLAG, >> - cf->can_id & CAN_RTR_FLAG, >> - cf->can_dlc, cf->data); >> - if (err) { >> + memset(&msg, 0, sizeof(msg)); >> + if (cf->can_id & CAN_EFF_FLAG) { >> + msg.cmmd = ESD331_I20_EX_BCAN; >> + msg.id = cpu_to_be16((cf->can_id & CAN_EFF_MASK) >> 16); >> + msg.x2 = cpu_to_be16(cf->can_id & CAN_EFF_MASK); >> + } else { >> + msg.cmmd = ESD331_I20_BCAN; >> + msg.id = cpu_to_be16(cf->can_id & CAN_EFF_MASK); >> + } >> + msg.x1 = cpu_to_be16(cf->can_dlc); >> + msg.net = priv->boards_net; >> + msg.len = cpu_to_be16((cf->can_id & CAN_RTR_FLAG) ? >> + cf->can_dlc | ESD331_RTR_FLAG : cf->can_dlc); >> + >> + for (i = 0; i < cf->can_dlc; i++) >> + msg.data[i] = cf->data[i]; >> + >> + can_put_echo_skb(skb, dev, 0); >> + if (esd331_write(&msg, priv->board)) { >> + err = NETDEV_TX_BUSY; >> stats->tx_fifo_errors++; >> stats->tx_errors++; >> stats->tx_dropped++; >> @@ -719,8 +706,8 @@ >> } > > You dropped the packet and you should therefore call can_free_echo_skb() > and return with NETDEV_TX_OK. Just missing in the patch cause that line's already there. > Can esd331_write() fail? Will it succeed the next time it is called? > The NETDEV_TX_BUSY might be more appropriate. Fails when card's fifo is full, therefore likely to succeed next time. But as NETDEV_TX_BUSY is also already returned then, I don't get what you mean. The function now looks: (only diff to posted patch is renaming err to ret) #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,32) static int esd331_start_xmit(struct sk_buff *skb, struct net_device *dev) #else static netdev_tx_t esd331_start_xmit(struct sk_buff *skb, struct net_device *dev) #endif { struct esd331_priv *priv = netdev_priv(dev); #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,23) struct net_device_stats *stats = can_get_stats(dev); #else struct net_device_stats *stats = &dev->stats; #endif struct can_frame *cf = (struct can_frame *)skb->data; struct esd331_can_msg msg; int ret = NETDEV_TX_OK; int i; if ((cf->can_id & CAN_EFF_FLAG) && (priv->board->eff_supp == 0)) { stats->tx_errors++; stats->tx_dropped++; goto out; } memset(&msg, 0, sizeof(msg)); if (cf->can_id & CAN_EFF_FLAG) { msg.cmmd = ESD331_I20_EX_BCAN; msg.id = cpu_to_be16((cf->can_id & CAN_EFF_MASK) >> 16); msg.x2 = cpu_to_be16(cf->can_id & CAN_EFF_MASK); } else { msg.cmmd = ESD331_I20_BCAN; msg.id = cpu_to_be16(cf->can_id & CAN_EFF_MASK); } msg.x1 = cpu_to_be16(cf->can_dlc); msg.net = priv->boards_net; msg.len = cpu_to_be16((cf->can_id & CAN_RTR_FLAG) ? cf->can_dlc | ESD331_RTR_FLAG : cf->can_dlc); for (i = 0; i < cf->can_dlc; i++) msg.data[i] = cf->data[i]; can_put_echo_skb(skb, dev, 0); if (esd331_write(&msg, priv->board)) { ret = NETDEV_TX_BUSY; stats->tx_fifo_errors++; stats->tx_errors++; stats->tx_dropped++; can_free_echo_skb(dev, 0); goto out; } netif_stop_queue(dev); dev->trans_start = jiffies; out: return ret; } Regards, Thomas _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
