Thomas Körper wrote:
> 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.
Well, "goto" should be used if there is a step by step cleanup to be
done, but that's not the case here.
> Just tell me how it should look like, will put the complete function at
> the end of this post.
I would remove err completely also to save some lines.
>>> + 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.
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.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
