Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
Quoting Ahmed S. Darwish (2015-01-23 06:07:34)
> On Wed, Jan 21, 2015 at 05:13:45PM +0100, Wolfgang Grandegger wrote:
> > On Wed, 21 Jan 2015 10:36:47 -0500, "Ahmed S. Darwish"
> > wrote:
> > > On Wed, Jan 21, 2015 at 03:00:15PM +, Andri Yngvason wrote:
> > >> Quoting Ahmed S. Darwish (2015-01-21 14:43:23)
> > >> > Hi!
> > >
> > > ...
> > >
> > >> > <-- Unplug the cable -->
> > >> >
> > >> > (000.009106) can0 2080 [8] 00 00 00 00 00 00 08 00
> > >> > ERRORFRAME
> > >> > bus-error
> > >> > error-counter-tx-rx{{8}{0}}
> > >> > (000.001872) can0 2080 [8] 00 00 00 00 00 00 10 00
> >
> > For a bus-errors I would also expcect some more information in the
> > data[2..3] fields. But these are always zero.
> >
>
> M16C error factors made it possible to report things like
> CAN_ERR_PROT_FORM/STUFF/BIT0/BIT1/TX in data[2], and
> CAN_ERR_PROT_LOC_ACK/CRC_DEL in data[3].
>
> Unfortunately such error factors are only reported in Leaf, but
> not in USBCan-II due to the wire format change in the error event:
>
> struct leaf_msg_error_event {
> u8 tid;
> u8 flags;
> __le16 time[3];
> u8 channel;
> u8 padding;
> u8 tx_errors_count;
> u8 rx_errors_count;
> u8 status;
> u8 error_factor;
> } __packed;
>
> struct usbcan_msg_error_event {
> u8 tid;
> u8 padding;
> u8 tx_errors_count_ch0;
> u8 rx_errors_count_ch0;
> u8 tx_errors_count_ch1;
> u8 rx_errors_count_ch1;
> u8 status_ch0;
> u8 status_ch1;
> __le16 time;
> } __packed;
>
> I speculate that the wire format was changed due to controller
> bugs in the USBCan-II, which was slightly mentioned in their
> data sheets here:
>
>
> http://www.kvaser.com/canlib-webhelp/page_hardware_specific_can_controllers.html
>
> So it seems there's really no way for filling such bus error
> info given the very limited amount of data exported :-(
>
We experienced similar problems with FlexCAN.
>
> The issue of incomplete data does not even stop here, kindly
> check below notes regarding reverse state transitions:
>
> > >> > ERRORFRAME
> > >> > bus-error
> > >> > error-counter-tx-rx{{16}{0}}
> > >> [...]
[...]
> > >> > ERRORFRAME
> > >> > bus-error
> > >> > error-counter-tx-rx{{128}{0}}
> > >> >
> > >> > (( Then a continous flood, exactly similar to the above packet,
> > >> > appears.
> > >> >Unfortunately this flooding is a firmware problem. ))
> > >> >
> > >> > <-- Replug the cable, after a good amount of time -->
> > >> >
> > >> Where are the reverse state transitions?
> > >> >
> > >
> > > Hmmm...
> > >
> > > [ ... ]
> > >>
> > >> Reverse state transitions are missing from the logs. See comments
> > above.
> > >>
> > >
> > > When the device is on the _receiving_ end, and I unplug the CAN cable
> > after
> > > introducing some noise to the level of reaching WARNING or PASSIVE, I
> > > receive a BUS_ERROR event with the rxerr count reset back to 0 or 1. In
> > > that case, the driver correctly transitions back the state to
> > ERROR_ACTIVE
> > > and candump produces something similar to:
> > >
> > > (000.000362) can0 208C [8] 00 40 40 00 00 00 00 01
> > > ERRORFRAME
> > > controller-problem{}
> > > protocol-violation{{back-to-error-active}{}}
> > > bus-error
> > > error-counter-tx-rx{{0}{1}}
> > >
> > > which is, AFAIK, the correct behaviour from the driver side.
> > >
> > > Meanwhile, when the device is on the _sending_ end and I re-plug the CAN
> > > cable again. Sometimes I receive events with txerr reset to 0 or 1, and
> > > the driver correctly reverts back to ERROR_ACTIVE in that case. But on
> > > another times like the quoted case above, I don't receive any events
> > > resetting txerr back -- only data packets on the bus.
> >
> >
Re: [PATCH v6 6/7] can: kvaser_usb: Consolidate and unify state change handling
Quoting Ahmed S. Darwish (2015-01-26 05:29:15)
> From: Ahmed S. Darwish
>
> Replace most of the can interface's state and error counters
> handling with the new can-dev can_change_state() mechanism.
>
> Suggested-by: Andri Yngvason
> Signed-off-by: Ahmed S. Darwish
> ---
> drivers/net/can/usb/kvaser_usb.c | 113 -
> 1 files changed, 49 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/net/can/usb/kvaser_usb.c
> b/drivers/net/can/usb/kvaser_usb.c
> index f57ce55..ddc2954 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -620,39 +620,44 @@ static void kvaser_usb_unlink_tx_urbs(struct
> kvaser_usb_net_priv *priv)
> }
>
> static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv
> *priv,
> -const struct
> kvaser_usb_error_summary *es)
> +const struct
> kvaser_usb_error_summary *es,
> +struct can_frame *cf)
> {
> struct net_device_stats *stats;
> - enum can_state new_state;
> -
> - stats = &priv->netdev->stats;
> - new_state = priv->can.state;
> + enum can_state cur_state, new_state, tx_state, rx_state;
>
> netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
>
> - if (es->status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) {
> - priv->can.can_stats.bus_off++;
> + stats = &priv->netdev->stats;
> + new_state = cur_state = priv->can.state;
> +
> + if (es->status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET))
> new_state = CAN_STATE_BUS_OFF;
> - } else if (es->status & M16C_STATE_BUS_PASSIVE) {
> - if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
> - priv->can.can_stats.error_passive++;
> + else if (es->status & M16C_STATE_BUS_PASSIVE)
> new_state = CAN_STATE_ERROR_PASSIVE;
> - } else if (es->status & M16C_STATE_BUS_ERROR) {
> - if ((priv->can.state < CAN_STATE_ERROR_WARNING) &&
> - ((es->txerr >= 96) || (es->rxerr >= 96))) {
> - priv->can.can_stats.error_warning++;
> + else if (es->status & M16C_STATE_BUS_ERROR) {
> + if ((es->txerr >= 256) || (es->rxerr >= 256))
> + new_state = CAN_STATE_BUS_OFF;
> + else if ((es->txerr >= 128) || (es->rxerr >= 128))
> + new_state = CAN_STATE_ERROR_PASSIVE;
> + else if ((es->txerr >= 96) || (es->rxerr >= 96))
> new_state = CAN_STATE_ERROR_WARNING;
> - } else if ((priv->can.state > CAN_STATE_ERROR_ACTIVE) &&
> - ((es->txerr < 96) && (es->rxerr < 96))) {
> + else if (cur_state > CAN_STATE_ERROR_ACTIVE)
> new_state = CAN_STATE_ERROR_ACTIVE;
> - }
> }
>
> if (!es->status)
> new_state = CAN_STATE_ERROR_ACTIVE;
>
> + if (new_state != cur_state) {
> + tx_state = (es->txerr >= es->rxerr) ? new_state : 0;
> + rx_state = (es->txerr <= es->rxerr) ? new_state : 0;
> +
> + can_change_state(priv->netdev, cf, tx_state, rx_state);
> + }
> +
> if (priv->can.restart_ms &&
> - (priv->can.state >= CAN_STATE_BUS_OFF) &&
> + (cur_state >= CAN_STATE_BUS_OFF) &&
> (new_state < CAN_STATE_BUS_OFF)) {
> priv->can.can_stats.restarts++;
> }
> @@ -664,18 +669,17 @@ static void kvaser_usb_rx_error_update_can_state(struct
> kvaser_usb_net_priv *pri
>
> priv->bec.txerr = es->txerr;
> priv->bec.rxerr = es->rxerr;
> - priv->can.state = new_state;
> }
>
> static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> const struct kvaser_msg *msg)
> {
> - struct can_frame *cf;
> + struct can_frame *cf, tmp_cf = { .can_id = CAN_ERR_FLAG, .can_dlc =
> CAN_ERR_DLC };
> struct sk_buff *skb;
> struct net_device_stats *stats;
> struct kvaser_usb_net_priv *priv;
> struct kvaser_usb_error_summary es = { };
> - enum can_state old_state;
> + enum can_state old_s
Re: [PATCH v6 5/7] can: kvaser_usb: Update interface state before exiting on OOM
Quoting Ahmed S. Darwish (2015-01-26 05:27:19)
> From: Ahmed S. Darwish
>
> Update all of the can interface's state and error counters before
> trying any skb allocation that can actually fail with -ENOMEM.
>
> Suggested-by: Marc Kleine-Budde
> Signed-off-by: Ahmed S. Darwish
> ---
> drivers/net/can/usb/kvaser_usb.c | 181
> ++
> 1 files changed, 105 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/net/can/usb/kvaser_usb.c
> b/drivers/net/can/usb/kvaser_usb.c
> index 7af379c..f57ce55 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -273,6 +273,10 @@ struct kvaser_msg {
> } u;
> } __packed;
>
> +struct kvaser_usb_error_summary {
> + u8 channel, status, txerr, rxerr, error_factor;
> +};
> +
> struct kvaser_usb_tx_urb_context {
> struct kvaser_usb_net_priv *priv;
> u32 echo_index;
> @@ -615,6 +619,54 @@ static void kvaser_usb_unlink_tx_urbs(struct
> kvaser_usb_net_priv *priv)
> priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> }
>
> +static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv
> *priv,
> +const struct
> kvaser_usb_error_summary *es)
> +{
> + struct net_device_stats *stats;
> + enum can_state new_state;
> +
> + stats = &priv->netdev->stats;
> + new_state = priv->can.state;
> +
> + netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
> +
> + if (es->status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) {
> + priv->can.can_stats.bus_off++;
> + new_state = CAN_STATE_BUS_OFF;
> + } else if (es->status & M16C_STATE_BUS_PASSIVE) {
> + if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
> + priv->can.can_stats.error_passive++;
> + new_state = CAN_STATE_ERROR_PASSIVE;
> + } else if (es->status & M16C_STATE_BUS_ERROR) {
> + if ((priv->can.state < CAN_STATE_ERROR_WARNING) &&
> + ((es->txerr >= 96) || (es->rxerr >= 96))) {
> + priv->can.can_stats.error_warning++;
> + new_state = CAN_STATE_ERROR_WARNING;
> + } else if ((priv->can.state > CAN_STATE_ERROR_ACTIVE) &&
> + ((es->txerr < 96) && (es->rxerr < 96))) {
> + new_state = CAN_STATE_ERROR_ACTIVE;
> + }
> + }
> +
> + if (!es->status)
> + new_state = CAN_STATE_ERROR_ACTIVE;
> +
> + if (priv->can.restart_ms &&
> + (priv->can.state >= CAN_STATE_BUS_OFF) &&
> + (new_state < CAN_STATE_BUS_OFF)) {
> + priv->can.can_stats.restarts++;
> + }
> +
> + if (es->error_factor) {
> + priv->can.can_stats.bus_error++;
> + stats->rx_errors++;
> + }
> +
> + priv->bec.txerr = es->txerr;
> + priv->bec.rxerr = es->rxerr;
> + priv->can.state = new_state;
> +}
> +
> static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> const struct kvaser_msg *msg)
> {
> @@ -622,30 +674,30 @@ static void kvaser_usb_rx_error(const struct kvaser_usb
> *dev,
> struct sk_buff *skb;
> struct net_device_stats *stats;
> struct kvaser_usb_net_priv *priv;
> - unsigned int new_state;
> - u8 channel, status, txerr, rxerr, error_factor;
> + struct kvaser_usb_error_summary es = { };
> + enum can_state old_state;
>
> switch (msg->id) {
> case CMD_CAN_ERROR_EVENT:
> - channel = msg->u.error_event.channel;
> - status = msg->u.error_event.status;
> - txerr = msg->u.error_event.tx_errors_count;
> - rxerr = msg->u.error_event.rx_errors_count;
> - error_factor = msg->u.error_event.error_factor;
> + es.channel = msg->u.error_event.channel;
> + es.status = msg->u.error_event.status;
> + es.txerr = msg->u.error_event.tx_errors_count;
> + es.rxerr = msg->u.error_event.rx_errors_count;
> + es.error_factor = msg->u.error_event.error_factor;
> break;
> case CMD_LOG_MESSAGE:
> - channel = msg->u.log_message.channel;
> - status = msg->u.log_message.data[0];
> - txerr = msg->u.log_message.data[2];
> - rxerr = msg->u.log_message.data[3];
> - error_factor = msg->u.log_message.data[1];
> + es.channel = msg->u.log_message.channel;
> + es.status = msg->u.log_message.data[0];
> + es.txerr = msg->u.log_message.data[2];
> + es.rxerr = msg->u.log_message.data[3];
> + es.error_factor = msg->u.log_message.data[1];
> break;
> case CMD_CHIP_STATE_EVENT:
> - channel = msg->u
Re: [PATCH v6 7/7] can: kvaser_usb: Add support for the USBcan-II family
Quoting Ahmed S. Darwish (2015-01-26 05:33:10)
> From: Ahmed S. Darwish
>
> CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
> divided into two major families: 'Leaf', and 'USBcanII'. From an
> Operating System perspective, the firmware of both families behave
> in a not too drastically different fashion.
>
> This patch adds support for the USBcanII family of devices to the
> current Kvaser Leaf-only driver.
>
> CAN frames sending, receiving, and error handling paths has been
> tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
> should also work nicely with other products in the same category.
>
> List of new devices supported by this driver update:
>
> - Kvaser USBcan II HS/HS
> - Kvaser USBcan II HS/LS
> - Kvaser USBcan Rugged ("USBcan Rev B")
> - Kvaser Memorator HS/HS
> - Kvaser Memorator HS/LS
> - Scania VCI2 (if you have the Kvaser logo on top)
>
> Signed-off-by: Ahmed S. Darwish
> ---
> drivers/net/can/usb/Kconfig |8 +-
> drivers/net/can/usb/kvaser_usb.c | 590
> ++
> 2 files changed, 474 insertions(+), 124 deletions(-)
>
> ** V6 Changelog:
> - Revert to the error-active state if the error counters were
> decreased by hardware
> - Rebase over a new set of upstream Leaf-driver bugfixes
>
> ** V5 Changelog:
> - Rebase on the new CAN error state changes added for the Leaf driver
> - Add minor changes (remove unused commands, constify poniters, etc.)
>
> ** V4 Changelog:
> - Use type-safe C methods instead of cpp macros
> - Remove defensive checks against non-existing families
> - Re-order methods to remove forward declarations
> - Smaller stuff spotted by earlier review (function prefexes, etc.)
>
> ** V3 Changelog:
> - Fix padding for the usbcan_msg_tx_acknowledge command
> - Remove kvaser_usb->max_channels and the MAX_NET_DEVICES macro
> - Rename commands to CMD_LEAF_xxx and CMD_USBCAN_xxx
> - Apply checkpatch.pl suggestions ('net/' comments, multi-line strings, etc.)
>
> ** V2 Changelog:
> - Update Kconfig entries
> - Use actual number of CAN channels (instead of max) where appropriate
> - Rebase over a new set of UsbcanII-independent driver fixes
>
> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> index a77db919..f6f5500 100644
> --- a/drivers/net/can/usb/Kconfig
> +++ b/drivers/net/can/usb/Kconfig
> @@ -25,7 +25,7 @@ config CAN_KVASER_USB
> tristate "Kvaser CAN/USB interface"
> ---help---
> This driver adds support for Kvaser CAN/USB devices like Kvaser
> - Leaf Light.
> + Leaf Light and Kvaser USBcan II.
>
> The driver provides support for the following devices:
> - Kvaser Leaf Light
> @@ -46,6 +46,12 @@ config CAN_KVASER_USB
> - Kvaser USBcan R
> - Kvaser Leaf Light v2
> - Kvaser Mini PCI Express HS
> + - Kvaser USBcan II HS/HS
> + - Kvaser USBcan II HS/LS
> + - Kvaser USBcan Rugged ("USBcan Rev B")
> + - Kvaser Memorator HS/HS
> + - Kvaser Memorator HS/LS
> + - Scania VCI2 (if you have the Kvaser logo on top)
>
> If unsure, say N.
>
> diff --git a/drivers/net/can/usb/kvaser_usb.c
> b/drivers/net/can/usb/kvaser_usb.c
> index ddc2954..17d28d7 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -6,10 +6,12 @@
> * Parts of this driver are based on the following:
> * - Kvaser linux leaf driver (version 4.78)
> * - CAN driver for esd CAN-USB/2
> + * - Kvaser linux usbcanII driver (version 5.3)
> *
> * Copyright (C) 2002-2006 KVASER AB, Sweden. All rights reserved.
> * Copyright (C) 2010 Matthias Fuchs , esd gmbh
> * Copyright (C) 2012 Olivier Sobrie
> + * Copyright (C) 2015 Valeo A.S.
> */
>
> #include
> @@ -30,8 +32,9 @@
> #define RX_BUFFER_SIZE 3072
> #define CAN_USB_CLOCK 800
> #define MAX_NET_DEVICES3
> +#define MAX_USBCAN_NET_DEVICES 2
>
> -/* Kvaser USB devices */
> +/* Kvaser Leaf USB devices */
> #define KVASER_VENDOR_ID 0x0bfd
> #define USB_LEAF_DEVEL_PRODUCT_ID 10
> #define USB_LEAF_LITE_PRODUCT_ID 11
> @@ -56,6 +59,24 @@
> #define USB_LEAF_LITE_V2_PRODUCT_ID288
> #define USB_MINI_PCIE_HS_PRODUCT_ID289
>
> +static inline bool kvaser_is_leaf(const struct usb_device_id *id)
> +{
> + return id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
> + id->idProduct <= USB_MINI_PCIE_HS_PRODUCT_ID;
> +}
> +
> +/* Kvaser USBCan-II devices */
> +#define USB_USBCAN_REVB_PRODUCT_ID 2
> +#define USB_VCI2_PRODUCT_ID3
> +#define USB_USBCAN2_PRODUCT_ID 4
> +#define USB_MEMORATOR_PRODUCT_ID 5
> +
> +static inline bool kvaser_is_usbcan(const struct usb_device_id *id)
> +{
> + return id->idProduct >= USB_USBCAN_REVB_PRODUCT_ID &&
> +
Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
Quoting Ahmed S. Darwish (2015-01-25 02:43:00)
> On Fri, Jan 23, 2015 at 10:32:13AM +0000, Andri Yngvason wrote:
> > Quoting Ahmed S. Darwish (2015-01-23 06:07:34)
> > > On Wed, Jan 21, 2015 at 05:13:45PM +0100, Wolfgang Grandegger wrote:
> > > > On Wed, 21 Jan 2015 10:36:47 -0500, "Ahmed S. Darwish"
> > > > wrote:
> > > > > On Wed, Jan 21, 2015 at 03:00:15PM +, Andri Yngvason wrote:
> > > > >> Quoting Ahmed S. Darwish (2015-01-21 14:43:23)
> > > > >> > Hi!
> > > > >
> > > > > ...
> > > > >
> > > > >> > <-- Unplug the cable -->
> > > > >> >
> > > > >> > (000.009106) can0 2080 [8] 00 00 00 00 00 00 08 00
> > > > >> > ERRORFRAME
> > > > >> > bus-error
> > > > >> > error-counter-tx-rx{{8}{0}}
> > > > >> > (000.001872) can0 2080 [8] 00 00 00 00 00 00 10 00
> > > >
> > > > For a bus-errors I would also expcect some more information in the
> > > > data[2..3] fields. But these are always zero.
> > > >
> > >
> > > M16C error factors made it possible to report things like
> > > CAN_ERR_PROT_FORM/STUFF/BIT0/BIT1/TX in data[2], and
> > > CAN_ERR_PROT_LOC_ACK/CRC_DEL in data[3].
> > >
> > > Unfortunately such error factors are only reported in Leaf, but
> > > not in USBCan-II due to the wire format change in the error event:
> > >
> > > struct leaf_msg_error_event {
> > > u8 tid;
> > > u8 flags;
> > > __le16 time[3];
> > > u8 channel;
> > > u8 padding;
> > > u8 tx_errors_count;
> > > u8 rx_errors_count;
> > > u8 status;
> > > u8 error_factor;
> > > } __packed;
> > >
> > > struct usbcan_msg_error_event {
> > > u8 tid;
> > > u8 padding;
> > > u8 tx_errors_count_ch0;
> > > u8 rx_errors_count_ch0;
> > > u8 tx_errors_count_ch1;
> > > u8 rx_errors_count_ch1;
> > > u8 status_ch0;
> > > u8 status_ch1;
> > > __le16 time;
> > > } __packed;
> > >
> > > I speculate that the wire format was changed due to controller
> > > bugs in the USBCan-II, which was slightly mentioned in their
> > > data sheets here:
> > >
> > >
> > > http://www.kvaser.com/canlib-webhelp/page_hardware_specific_can_controllers.html
> > >
> > > So it seems there's really no way for filling such bus error
> > > info given the very limited amount of data exported :-(
> > >
> > We experienced similar problems with FlexCAN.
>
> Hmm, I'll have a look there then...
>
> Although my initial instincts imply that the FlexCAN driver has
> access to the raw CAN registers, something I'm unable to do here.
> But maybe there's some black magic I'm missing :-)
>
Yes, it's memory mapped.
>
> [...]
>
> > >
> > > I've dumped _every_ message I receive from the firmware while
> > > disconnecting the CAN bus, waiting a while, and connecting it again.
> > > I really received _nothing_ from the firmware when the CAN bus was
> > > reconnected and the data packets were flowing again. Not even a
> > > single CHIP_STATE_EVENT, even after waiting for a long time.
> > >
> > > So it's basically:
> > > ...
> > > ERR EVENT, txerr=128, rxerr=0
> > > ERR EVENT, txerr=128, rxerr=0
> > > ERR EVENT, txerr=128, rxerr=0
> > > ...
> > >
> > > then complete silence, except the data frames. I've even tried with
> > > different versions of the firmware, but the same behaviour persisted.
> > >
> > > > > So, What can the driver do given the above?
> > > >
> > > > Little if the notification does not come.
> > > >
> > >
> > > We can poll the state by sending CMD_GET_CHIP_STATE to the firmware,
> > > and it will hopefully reply with a CHIP_STATE_EVENT response
> > > containing the new txerr and rxerr values that we can use for
> > > reverse state transitions.
> > >
Re: [PATCH v6 5/7] can: kvaser_usb: Update interface state before exiting on OOM
Quoting Marc Kleine-Budde (2015-01-26 10:50:12) > On 01/26/2015 11:28 AM, Andri Yngvason wrote: > > Quoting Ahmed S. Darwish (2015-01-26 05:27:19) > >> From: Ahmed S. Darwish > >> > >> Update all of the can interface's state and error counters before > >> trying any skb allocation that can actually fail with -ENOMEM. > >> > >> Suggested-by: Marc Kleine-Budde > >> Signed-off-by: Ahmed S. Darwish > >> --- > >> drivers/net/can/usb/kvaser_usb.c | 181 > >> ++ > >> 1 files changed, 105 insertions(+), 76 deletions(-) > [..] > > > Looks good to me. > > Can I add your Acked-by to 5-7? > Yes. -- Andri -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
Quoting Ahmed S. Darwish (2015-01-20 21:45:37)
> From: Ahmed S. Darwish
>
> Replace most of the can interface's state and error counters
> handling with the new can-dev can_change_state() mechanism.
>
> Suggested-by: Andri Yngvason
> Signed-off-by: Ahmed S. Darwish
> ---
> drivers/net/can/usb/kvaser_usb.c | 114
> +++
> 1 file changed, 55 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/can/usb/kvaser_usb.c
> b/drivers/net/can/usb/kvaser_usb.c
> index 971c5f9..0386d3f 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -620,40 +620,43 @@ static void kvaser_usb_unlink_tx_urbs(struct
> kvaser_usb_net_priv *priv)
> }
>
> static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv
> *priv,
> -const struct
> kvaser_usb_error_summary *es)
> +const struct
> kvaser_usb_error_summary *es,
> +struct can_frame *cf)
> {
> struct net_device_stats *stats;
> - enum can_state new_state;
> -
> - stats = &priv->netdev->stats;
> - new_state = priv->can.state;
> + enum can_state cur_state, new_state, tx_state, rx_state;
>
> netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
>
> - if (es->status & M16C_STATE_BUS_OFF) {
> - priv->can.can_stats.bus_off++;
> + stats = &priv->netdev->stats;
> + new_state = cur_state = priv->can.state;
> +
> + if (es->status & M16C_STATE_BUS_OFF)
> new_state = CAN_STATE_BUS_OFF;
> - } else if (es->status & M16C_STATE_BUS_PASSIVE) {
> - if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
> - priv->can.can_stats.error_passive++;
> + else if (es->status & M16C_STATE_BUS_PASSIVE)
> new_state = CAN_STATE_ERROR_PASSIVE;
> - }
>
> if (es->status == M16C_STATE_BUS_ERROR) {
> - if ((priv->can.state < CAN_STATE_ERROR_WARNING) &&
> - ((es->txerr >= 96) || (es->rxerr >= 96))) {
> - priv->can.can_stats.error_warning++;
> + if ((cur_state < CAN_STATE_ERROR_WARNING) &&
> + ((es->txerr >= 96) || (es->rxerr >= 96)))
> new_state = CAN_STATE_ERROR_WARNING;
> - } else if (priv->can.state > CAN_STATE_ERROR_ACTIVE) {
> + else if (cur_state > CAN_STATE_ERROR_ACTIVE)
> new_state = CAN_STATE_ERROR_ACTIVE;
> - }
> }
>
> if (!es->status)
> new_state = CAN_STATE_ERROR_ACTIVE;
>
> + if (new_state != cur_state) {
> + tx_state = (es->txerr >= es->rxerr) ? new_state : 0;
> + rx_state = (es->txerr <= es->rxerr) ? new_state : 0;
> +
> + can_change_state(priv->netdev, cf, tx_state, rx_state);
> + new_state = priv->can.state;
> + }
> +
> if (priv->can.restart_ms &&
> - (priv->can.state >= CAN_STATE_BUS_OFF) &&
> + (cur_state >= CAN_STATE_BUS_OFF) &&
> (new_state < CAN_STATE_BUS_OFF)) {
> priv->can.can_stats.restarts++;
> }
> @@ -665,18 +668,17 @@ static void kvaser_usb_rx_error_update_can_state(struct
> kvaser_usb_net_priv *pri
>
> priv->bec.txerr = es->txerr;
> priv->bec.rxerr = es->rxerr;
> - priv->can.state = new_state;
> }
>
> static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> const struct kvaser_msg *msg)
> {
> - struct can_frame *cf;
> + struct can_frame *cf, tmp_cf = { .can_id = CAN_ERR_FLAG, .can_dlc =
> CAN_ERR_DLC };
> struct sk_buff *skb;
> struct net_device_stats *stats;
> struct kvaser_usb_net_priv *priv;
> struct kvaser_usb_error_summary es = { };
> - enum can_state old_state;
> + enum can_state old_state, new_state;
>
> switch (msg->id) {
> case CMD_CAN_ERROR_EVENT:
> @@ -721,60 +723,54 @@ static void kvaser_usb_rx_error(const struct kvaser_usb
> *dev,
> }
>
> /* Update all of the can interface's state and error counters before
> -* trying any skb allocation that can actually fail with -ENOMEM.
>
Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
Quoting Marc Kleine-Budde (2015-01-21 10:44:54) > On 01/21/2015 11:33 AM, Andri Yngvason wrote: > > Quoting Ahmed S. Darwish (2015-01-20 21:45:37) > >> From: Ahmed S. Darwish > >> > >> Replace most of the can interface's state and error counters > >> handling with the new can-dev can_change_state() mechanism. > >> > >> Suggested-by: Andri Yngvason > >> Signed-off-by: Ahmed S. Darwish > >> --- > >> drivers/net/can/usb/kvaser_usb.c | 114 > >> +++ > >> 1 file changed, 55 insertions(+), 59 deletions(-) > >> > >> diff --git a/drivers/net/can/usb/kvaser_usb.c > >> b/drivers/net/can/usb/kvaser_usb.c > >> index 971c5f9..0386d3f 100644 > >> --- a/drivers/net/can/usb/kvaser_usb.c > >> +++ b/drivers/net/can/usb/kvaser_usb.c > > > Looks good. > > Is this an Acked-by? > ACK. -- Andri -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
Quoting Ahmed S. Darwish (2015-01-21 14:43:23)
> Hi!
>
> On Wed, Jan 21, 2015 at 12:53:58PM +0100, Wolfgang Grandegger wrote:
> > On Wed, 21 Jan 2015 10:33:19 +, Andri Yngvason
> > wrote:
> > > Quoting Ahmed S. Darwish (2015-01-20 21:45:37)
> > >> From: Ahmed S. Darwish
> > >>
> > >> Replace most of the can interface's state and error counters
> > >> handling with the new can-dev can_change_state() mechanism.
> > >>
> > >> Suggested-by: Andri Yngvason
> > >> Signed-off-by: Ahmed S. Darwish
> > >> ---
> > >> drivers/net/can/usb/kvaser_usb.c | 114
> > >> +++
> > >> 1 file changed, 55 insertions(+), 59 deletions(-)
> > >>
> > >> diff --git a/drivers/net/can/usb/kvaser_usb.c
> > >> b/drivers/net/can/usb/kvaser_usb.c
> > >> index 971c5f9..0386d3f 100644
> > >> --- a/drivers/net/can/usb/kvaser_usb.c
> > >> +++ b/drivers/net/can/usb/kvaser_usb.c
> >
> > ...
> > >
> > > Looks good.
> >
> > Would be nice to see some "candump" traces as well.
>
> Sure. The USBCan-II device trace below is generated after applying
> all patches in the series, especially patch #3, which fixes some
> some invalid CAN state transitions logic in the original driver.
[...]
>
> Afterwards, candump on a PC, Kvaser USB device on the sending end:
>
> ...
> (000.008784) can0 60A [1] C1
> (000.011341) can0 2A8 [8] C2 0A 00 00 00 00 00 00
> (000.009873) can0 03D [7] C3 0A 00 00 00 00 00
> (000.010394) can0 55C [8] C4 0A 00 00 00 00 00 00
> (000.009979) can0 45A [8] C5 0A 00 00 00 00 00 00
> (000.010125) can0 6E8 [8] C6 0A 00 00 00 00 00 00
> (000.010149) can0 4EE [8] C7 0A 00 00 00 00 00 00
> (000.010102) can0 5D2 [8] C8 0A 00 00 00 00 00 00
> (000.01) can0 61F [8] C9 0A 00 00 00 00 00 00
> (000.010271) can0 5F8 [8] CA 0A 00 00 00 00 00 00
>
> <-- Unplug the cable -->
>
> (000.009106) can0 2080 [8] 00 00 00 00 00 00 08 00 ERRORFRAME
> bus-error
> error-counter-tx-rx{{8}{0}}
> (000.001872) can0 2080 [8] 00 00 00 00 00 00 10 00 ERRORFRAME
> bus-error
> error-counter-tx-rx{{16}{0}}
[...]
> error-counter-tx-rx{{80}{0}}
> (000.001910) can0 2080 [8] 00 00 00 00 00 00 58 00 ERRORFRAME
> bus-error
> error-counter-tx-rx{{88}{0}}
> (000.001753) can0 2084 [8] 00 08 00 00 00 00 60 00 ERRORFRAME
> controller-problem{tx-error-warning}
Good.
> bus-error
> error-counter-tx-rx{{96}{0}}
> (000.001720) can0 2080 [8] 00 00 00 00 00 00 68 00 ERRORFRAME
> bus-error
> error-counter-tx-rx{{104}{0}}
> (000.001876) can0 2080 [8] 00 00 00 00 00 00 70 00 ERRORFRAME
> bus-error
> error-counter-tx-rx{{112}{0}}
> (000.001749) can0 2080 [8] 00 00 00 00 00 00 78 00 ERRORFRAME
> bus-error
> error-counter-tx-rx{{120}{0}}
> (000.001771) can0 2084 [8] 00 20 00 00 00 00 80 00 ERRORFRAME
> controller-problem{tx-error-passive}
Also good.
> bus-error
> error-counter-tx-rx{{128}{0}}
> (000.001868) can0 2080 [8] 00 00 00 00 00 00 80 00 ERRORFRAME
> bus-error
> error-counter-tx-rx{{128}{0}}
> (000.001982) can0 2080 [8] 00 00 00 00 00 00 80 00 ERRORFRAME
> bus-error
> error-counter-tx-rx{{128}{0}}
>
> (( Then a continous flood, exactly similar to the above packet, appears.
>Unfortunately this flooding is a firmware problem. ))
>
> <-- Replug the cable, after a good amount of time -->
>
Where are the reverse state transitions?
>
> (000.520485) can0 33D [4] CB 0A 00 00
> (000.002693) can0 42E [8] CC 0A 00 00 00 00 00 00
> (000.001795) can0 319 [4] CD 0A 00 00
> (000.002705) can0 3B1 [8] CE 0A 00 00 00 00 00 00
> (000.001295) can0 4CC [2] CF 0A
> (000.002205) can0 42B [6] D0 0A 00 00 00 00
> (000.001620) can0 5A2 [3] D1 0A 00
> (000.002636) can0 691 [8] D2 0A 00 00 00 00 00 00
> (000.002615) can0 36A [8] D3 0A 00 00 00 00 00 00
> (000.001729) can0 068 [4] D4 0A 00 00
> (000.001195) can0 4C8 [1] D5
> ...
>
> ##
>
> Bus-off Testing:
>
> candump on a PC, Kvaser device on the sending end. An i.mx6 ARM
> board with flexcan is on the receiving end:
>
> (000.010319) can0 5CC [8] 90 02
Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
Quoting Ahmed S. Darwish (2015-01-20 21:45:37)
> From: Ahmed S. Darwish
>
> Replace most of the can interface's state and error counters
> handling with the new can-dev can_change_state() mechanism.
>
> Suggested-by: Andri Yngvason
> Signed-off-by: Ahmed S. Darwish
> ---
> drivers/net/can/usb/kvaser_usb.c | 114
> +++
> 1 file changed, 55 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/can/usb/kvaser_usb.c
> b/drivers/net/can/usb/kvaser_usb.c
> index 971c5f9..0386d3f 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -620,40 +620,43 @@ static void kvaser_usb_unlink_tx_urbs(struct
> kvaser_usb_net_priv *priv)
> }
>
> static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv
> *priv,
> -const struct
> kvaser_usb_error_summary *es)
> +const struct
> kvaser_usb_error_summary *es,
> +struct can_frame *cf)
> {
> struct net_device_stats *stats;
> - enum can_state new_state;
> -
> - stats = &priv->netdev->stats;
> - new_state = priv->can.state;
> + enum can_state cur_state, new_state, tx_state, rx_state;
>
> netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
>
> - if (es->status & M16C_STATE_BUS_OFF) {
> - priv->can.can_stats.bus_off++;
> + stats = &priv->netdev->stats;
> + new_state = cur_state = priv->can.state;
> +
> + if (es->status & M16C_STATE_BUS_OFF)
> new_state = CAN_STATE_BUS_OFF;
> - } else if (es->status & M16C_STATE_BUS_PASSIVE) {
> - if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
> - priv->can.can_stats.error_passive++;
> + else if (es->status & M16C_STATE_BUS_PASSIVE)
> new_state = CAN_STATE_ERROR_PASSIVE;
> - }
>
> if (es->status == M16C_STATE_BUS_ERROR) {
> - if ((priv->can.state < CAN_STATE_ERROR_WARNING) &&
> - ((es->txerr >= 96) || (es->rxerr >= 96))) {
> - priv->can.can_stats.error_warning++;
> + if ((cur_state < CAN_STATE_ERROR_WARNING) &&
> + ((es->txerr >= 96) || (es->rxerr >= 96)))
> new_state = CAN_STATE_ERROR_WARNING;
> - } else if (priv->can.state > CAN_STATE_ERROR_ACTIVE) {
> + else if (cur_state > CAN_STATE_ERROR_ACTIVE)
> new_state = CAN_STATE_ERROR_ACTIVE;
> - }
> }
>
> if (!es->status)
> new_state = CAN_STATE_ERROR_ACTIVE;
>
> + if (new_state != cur_state) {
> + tx_state = (es->txerr >= es->rxerr) ? new_state : 0;
> + rx_state = (es->txerr <= es->rxerr) ? new_state : 0;
> +
> + can_change_state(priv->netdev, cf, tx_state, rx_state);
This (below) is redundant. It doesn't harm but at this point can_change_state
has set priv->can.state to new_state.
> + new_state = priv->can.state;
> + }
> +
> if (priv->can.restart_ms &&
> - (priv->can.state >= CAN_STATE_BUS_OFF) &&
> + (cur_state >= CAN_STATE_BUS_OFF) &&
> (new_state < CAN_STATE_BUS_OFF)) {
> priv->can.can_stats.restarts++;
> }
> @@ -665,18 +668,17 @@ static void kvaser_usb_rx_error_update_can_state(struct
> kvaser_usb_net_priv *pri
>
> priv->bec.txerr = es->txerr;
> priv->bec.rxerr = es->rxerr;
> - priv->can.state = new_state;
> }
>
> static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> const struct kvaser_msg *msg)
> {
> - struct can_frame *cf;
> + struct can_frame *cf, tmp_cf = { .can_id = CAN_ERR_FLAG, .can_dlc =
> CAN_ERR_DLC };
> struct sk_buff *skb;
> struct net_device_stats *stats;
> struct kvaser_usb_net_priv *priv;
> struct kvaser_usb_error_summary es = { };
> - enum can_state old_state;
> + enum can_state old_state, new_state;
>
> switch (msg->id) {
> case CMD_CAN_ERROR_EVENT:
> @@ -721,60 +723,54 @@ static void kvaser_usb_rx_error(const struct kvaser_usb
> *dev,
> }
>
> /* Update all of the can interfa
Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
Quoting Ahmed S. Darwish (2015-01-21 15:36:47)
> On Wed, Jan 21, 2015 at 03:00:15PM +0000, Andri Yngvason wrote:
> > Quoting Ahmed S. Darwish (2015-01-21 14:43:23)
> > > Hi!
>
> ...
>
> > > <-- Unplug the cable -->
> > >
> > > (000.009106) can0 2080 [8] 00 00 00 00 00 00 08 00 ERRORFRAME
> > > bus-error
> > > error-counter-tx-rx{{8}{0}}
> > > (000.001872) can0 2080 [8] 00 00 00 00 00 00 10 00 ERRORFRAME
> > > bus-error
> > > error-counter-tx-rx{{16}{0}}
> > [...]
> > > error-counter-tx-rx{{80}{0}}
> > > (000.001910) can0 2080 [8] 00 00 00 00 00 00 58 00 ERRORFRAME
> > > bus-error
> > > error-counter-tx-rx{{88}{0}}
> > > (000.001753) can0 2084 [8] 00 08 00 00 00 00 60 00 ERRORFRAME
> > > controller-problem{tx-error-warning}
> > Good.
> > > bus-error
> > > error-counter-tx-rx{{96}{0}}
>
> Nice.
>
> > > (000.001720) can0 2080 [8] 00 00 00 00 00 00 68 00 ERRORFRAME
> > > bus-error
> > > error-counter-tx-rx{{104}{0}}
> > > (000.001876) can0 2080 [8] 00 00 00 00 00 00 70 00 ERRORFRAME
> > > bus-error
> > > error-counter-tx-rx{{112}{0}}
> > > (000.001749) can0 2080 [8] 00 00 00 00 00 00 78 00 ERRORFRAME
> > > bus-error
> > > error-counter-tx-rx{{120}{0}}
> > > (000.001771) can0 2084 [8] 00 20 00 00 00 00 80 00 ERRORFRAME
> > > controller-problem{tx-error-passive}
> > Also good.
> > > bus-error
> > > error-counter-tx-rx{{128}{0}}
>
> Also nice :-)
>
> > > (000.001868) can0 2080 [8] 00 00 00 00 00 00 80 00 ERRORFRAME
> > > bus-error
> > > error-counter-tx-rx{{128}{0}}
> > > (000.001982) can0 2080 [8] 00 00 00 00 00 00 80 00 ERRORFRAME
> > > bus-error
> > > error-counter-tx-rx{{128}{0}}
> > >
> > > (( Then a continous flood, exactly similar to the above packet, appears.
> > >Unfortunately this flooding is a firmware problem. ))
> > >
> > > <-- Replug the cable, after a good amount of time -->
> > >
> > Where are the reverse state transitions?
> > >
>
> Hmmm...
>
> [ ... ]
> >
> > Reverse state transitions are missing from the logs. See comments above.
> >
>
> When the device is on the _receiving_ end, and I unplug the CAN cable after
> introducing some noise to the level of reaching WARNING or PASSIVE, I
> receive a BUS_ERROR event with the rxerr count reset back to 0 or 1. In
> that case, the driver correctly transitions back the state to ERROR_ACTIVE
> and candump produces something similar to:
>
> (000.000362) can0 208C [8] 00 40 40 00 00 00 00 01 ERRORFRAME
> controller-problem{}
> protocol-violation{{back-to-error-active}{}}
> bus-error
> error-counter-tx-rx{{0}{1}}
>
> which is, AFAIK, the correct behaviour from the driver side.
>
> Meanwhile, when the device is on the _sending_ end and I re-plug the CAN
> cable again. Sometimes I receive events with txerr reset to 0 or 1, and
> the driver correctly reverts back to ERROR_ACTIVE in that case. But on
> another times like the quoted case above, I don't receive any events
> resetting txerr back -- only data packets on the bus.
>
> So, What can the driver do given the above?
>
So what you're telling us is that the state does not got back to error-active
unless there is something else transmitting on the bus?
If that's the case, it's almost definitely because state changes aren't
triggering interrupts.
An rx event will give you an interrupt which yields a napi poll which means that
the state will be polled, so in that case you don't need the interrupts.
Look for "Consolidate and unify state change handling" on gmane. There was a lot
of discussion about this kind of issues, especially regarding FlexCAN.
Best regards,
Andri
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
Quoting Marc Kleine-Budde (2015-01-21 22:59:23) > On 01/21/2015 05:20 PM, Andri Yngvason wrote: > > Marc, could you merge the "move bus_off++" patch before you merge this so > > that I > > won't have to incorporate this patch-set into it? > > ...included in the lastest pull-request to David. Use > tags/linux-can-next-for-3.20-20150121 of the can-next repo as you new base. > Thanks! -- Andri -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
