From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Grandegger <wg@grandegger.com>
Subject: Re: Bug? -- NEVER getting controller-problem{back-to-error-active}
Date: Tue, 21 Jun 2016 11:58:35 +0200
Message-ID: <57690FCB.70603@grandegger.com>
References: <loom.20160620T110554-742@post.gmane.org>
 <5767C65D.2010303@grandegger.com> <loom.20160620T145908-586@post.gmane.org>
 <5767F01F.50700@grandegger.com> <loom.20160620T154404-130@post.gmane.org>
 <5767FD85.9020701@grandegger.com>
 <eccb6d7c-b451-8f8a-d6c1-c2675002d424@peak-system.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Return-path: <linux-can-owner@vger.kernel.org>
Received: from mailproxy02.manitu.net ([217.11.48.150]:39755 "EHLO
	mailproxy02.manitu.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750696AbcFUKAd (ORCPT
	<rfc822;linux-can@vger.kernel.org>); Tue, 21 Jun 2016 06:00:33 -0400
In-Reply-To: <eccb6d7c-b451-8f8a-d6c1-c2675002d424@peak-system.com>
Sender: linux-can-owner@vger.kernel.org
List-ID: <linux-can.vger.kernel.org>
To: Stephane Grosjean <s.grosjean@peak-system.com>, ajneu <ajneu1@gmail.com>, linux-can@vger.kernel.org

Hello Stephane,

Am 21.06.2016 um 10:29 schrieb Stephane Grosjean:
> Hi,
>
> The reason of this:
>
>          } else {
>                  /* no error bit (so, no error skb, back to active
> state) */
>                  dev->can.state = CAN_STATE_ERROR_ACTIVE;
>                  pdev->bec.txerr = 0;
>                  pdev->bec.rxerr = 0;
>                  return 0;
>          }
>
> is given by the comment: AFAIR in these times, going (back) to
> ACTIVE_STATE was made like above, by the device driver itself.

Well, in the past we didn't care about *decreasing* state changes but 
new driver should, and this does include the "back-to-error-active" 
message, which is automatically generated by "change_state()". So far, 
only a few driver are using that function:

   $ grep -rl can_change_state .
   ./sja1000/sja1000.c
   ./usb/peak_usb/pcan_usb_fd.c
   ./usb/kvaser_usb.c
   ./flexcan.c
   ./mscan/mscan.c

> The lower condition:
>
>          /* allocate an skb to store the error frame */
>          skb = alloc_can_err_skb(netdev, &cf);
>          if (skb)
>                  can_change_state(netdev, cf, tx_state, rx_state);
>
> uses can_change_state() if and only if "skb" is not NULL, while there
> was no need of any skb to go (back) to ACTIVE_STATE.
>
> So, I suppose that the proposed change should also include something
> like the following one:
>
>           /* allocate an skb to store the error frame */
>           skb = alloc_can_err_skb(netdev, &cf);
> -        if (skb)
> +        if (skb || (new_state == CAN_STATE_ERROR_ACTIVE))
>                   can_change_state(netdev, cf, tx_state, rx_state);

can_change_state() should not be called if "skb" is NULL (because cf is 
not valid).

Wolfgang.