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

["Thomas Körper" <Thomas.Koerper@esd.eu>]

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
kvaser_usb counts rx_dropped++ if skb alloc failed, and setting 
cf->data[6]/cf->data[7] to the counter values seems also rarely used)

...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);
	}
}


Regards,
    Thomas


-----Ursprüngliche Nachricht-----
Von: Andri Yngvason [mailto:andri.yngvason@marel.com] 
Gesendet: Montag, 16. März 2015 13:46
An: linux-can@vger.kernel.org
Cc: Thomas Körper
Betreff: Re: [PATCH V5 1/1] can: Add support for esd CAN PCIe/402 card

Quoting Thomas Körper (2015-03-16 12:15:13)
[...]
> +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;
> +       int is_busoff;
> +       int is_passive;
> +       int is_warning;
> +       u8 txerr;
> +       u8 rxerr;
> +
> +       txerr = (u8)(msg->reg_status >> 8);
> +       rxerr = (u8)msg->reg_status;
> +       is_warning = (msg->reg_status & ACC_REG_STATUS_MASK_STATUS_ES) != 0;
> +       is_passive = (msg->reg_status & ACC_REG_STATUS_MASK_STATUS_EP) != 0;
> +       is_busoff = (msg->reg_status & ACC_REG_STATUS_MASK_STATUS_BS) != 0;
> +
> +       skb = alloc_can_err_skb(core->net_dev, &cf);
> +       if (skb) {
> +               if (is_busoff) {
> +                       priv->can.state = CAN_STATE_BUS_OFF;
> +                       /* bus-offs counted by can_bus_off() */
> +                       cf->can_id |= CAN_ERR_BUSOFF;
> +               } else if (is_passive) {
> +                       priv->can.state = CAN_STATE_ERROR_PASSIVE;
> +                       priv->can.can_stats.error_passive++;
> +                       cf->data[1] = (txerr > rxerr) ?
> +                                       CAN_ERR_CRTL_TX_PASSIVE :
> +                                       CAN_ERR_CRTL_RX_PASSIVE;
> +                       cf->can_id |= CAN_ERR_CRTL;
> +                       cf->data[6] = txerr;
> +                       cf->data[7] = rxerr;
> +               } else if (is_warning) {
> +                       priv->can.state = CAN_STATE_ERROR_WARNING;
> +                       priv->can.can_stats.error_warning++;
> +                       cf->data[1] = (txerr > rxerr) ?
> +                                       CAN_ERR_CRTL_TX_WARNING :
> +                                       CAN_ERR_CRTL_RX_WARNING;
> +                       cf->can_id |= CAN_ERR_CRTL;
> +                       cf->data[6] = txerr;
> +                       cf->data[7] = rxerr;
> +               } else {
> +                       priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +                       /* restarts counted in dev.c */
>
There is now a back-to-error-active message available for use, so you can do
    cf->can_id = CAN_ERR_CRTL;
    cf->data[1] = CAN_ERROR_CRTL_ACTIVE.
However, you could shave off a few lines in this code by using
can_change_state().
>
> +               }
> +
> +               netif_rx(skb);
> +               stats->rx_packets++;
> +               stats->rx_bytes += cf->can_dlc;
> +       }
> +
> +       if (is_busoff) {
> +               acc_write32(core, ACC_CORE_OF_TX_ABORT_MASK, 0xffff);
> +               can_bus_off(core->net_dev);
> +       }
> +}
> +
[...]

can_change_state() is now used in mscan, flexcan, sja1000, kvaser and peak.
Those drivers should provide sufficent examples for you to learn how to use the
function.

Best regards,
Andri Yngvason