Re: AW: [PATCH V5 1/1] can: Add support for esd CAN PCIe/402 card

[Andri Yngvason <andri.yngvason@marel.com>]

Hi Thomas,

Quoting Thomas Körper (2015-03-17 06:30:02)
> Hi Andri,
> 
> I've looked at the sources you mentioned, but I'm a little bit unsure now /
> the handling seems not perfectly consistent to me. (flexcan calls
> can_change_state() with tx/rx_state of 0 in the bus off path. Only
That's an oversight on my part.

> kvaser_usb counts rx_dropped++ if skb alloc failed, and setting 
Yes, Darwish is very diligent in handling OOM.

> cf->data[6]/cf->data[7] to the counter values seems also rarely used)
Some chips don't expose the error counters.

> 
> ...so let me show the reworked function first before I post a new patch:
> 
> static void
> handle_core_msg_errstatechange(struct acc_core *core,
>                                const struct acc_bmmsg_errstatechange *msg)
> {
>         struct acc_net_priv *priv = netdev_priv(core->net_dev);
>         struct net_device_stats *stats = &core->net_dev->stats;
>         struct can_frame *cf;
>         struct sk_buff *skb;
> 
>         skb = alloc_can_err_skb(core->net_dev, &cf);
>         if (skb) {
>                 enum can_state new_state;
>                 u8 txerr;
>                 u8 rxerr;
> 
>                 txerr = (u8)(msg->reg_status >> 8);
>                 rxerr = (u8)msg->reg_status;
> 
>                 cf->data[6] = txerr;
>                 cf->data[7] = rxerr;
> 
>                 if (msg->reg_status & ACC_REG_STATUS_MASK_STATUS_BS) {
>                         new_state = CAN_STATE_BUS_OFF;
>                 } else if (msg->reg_status & ACC_REG_STATUS_MASK_STATUS_EP) {
>                         new_state = CAN_STATE_ERROR_PASSIVE;
>                 } else if (msg->reg_status & ACC_REG_STATUS_MASK_STATUS_ES) {
>                         new_state = CAN_STATE_ERROR_WARNING;
>                 } else {
>                         new_state = CAN_STATE_ERROR_ACTIVE;
>                 }
> 
>                 if (new_state != priv->can.state) {
>                         enum can_state tx_state, rx_state;
> 
>                         tx_state = (txerr >= rxerr) ? new_state : 0;
>                         rx_state = (rxerr >= txerr) ? new_state : 0;
> 
>                         can_change_state(core->net_dev, cf, tx_state, rx_state);
>                 }
> 
>                 netif_rx(skb);
>                 stats->rx_packets++;
>                 stats->rx_bytes += cf->can_dlc;
>         } else {
>                 stats->rx_dropped++;
>         }
> 
>         if (msg->reg_status & ACC_REG_STATUS_MASK_STATUS_BS) {
>                 acc_write32(core, ACC_CORE_OF_TX_ABORT_MASK, 0xffff);
>                 can_bus_off(core->net_dev);
>         }
> }
> 

Looks good!

Best regards,
Andri