Hi Thomas,
Thomas Körper wrote:
> Hello,
>
>> If you return with NETDEV_TX_BUSY, you should *not* free the skb and
>> therefore *not* increment "stats->tx_dropped". In contrast, if you
>> return with NETDEV_TX_OK, the *driver* must take care of the skb. Do you
>> realized the difference and problem now?
>>
>> Wolfgang.
>
> So I added a dev_kfree_skb(skb) to the first "dropping" condition where
> NETDEV_TX_OK is returned.
>
> In the busy case just stats->tx_fifo_errors is incremented.
> (Is it appropriate there?, no other driver seems to use that)
>
>
> Now it is:
>
> #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 i;
>
> if ((cf->can_id & CAN_EFF_FLAG) && (priv->board->eff_supp == 0)) {
> stats->tx_errors++;
You can remove the above line.
> stats->tx_dropped++;
> dev_kfree_skb(skb);
s/dev_kfree_skb/kfree_skb/
> return NETDEV_TX_OK;
> }
That error is handled properly.
> 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);
As you know, can_put_echo_skb() can free the skb and therefore...
> if (esd331_write(&msg, priv->board)) {
> stats->tx_fifo_errors++;
> return NETDEV_TX_BUSY;
... you cannot return NETDEV_TX_BUSY.
I had a closer look to the driver and it seems possible to check in
advance if the FIFO is already full. Then you can return with
NETDEV_TX_BUSY before calling can_put_echo_skb() and writing the data to
the FIFO. Also, how can it happen that the FIFO is full? As I see it,
only esd331_start_xmit() is writing to the FIFO one message at the time.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
