Bug? -- NEVER getting controller-problem{back-to-error-active}

Bug? -- NEVER getting controller-problem{back-to-error-active}

[ajneu]

Hi,

I'm using a PEAK USB Adapter (PCAN-USB FD) for normal CAN Frames (standard
format), and lsmod shows peak_usb as it should.

When running on a can bus and continually send out messages (one frame each
second).
But there is no other participant, so I will get NO ACK.


So berr-counter-tx counts up until it reaches
96:
-> enter state ERROR-WARNING.

   I then get a frame:
   Error: 
	Id  : 0x20000004
	Data: 0x0 0x8 0x0 0x0 0x0 0x0 0x0 0x0 
	controller-problem{tx-error-warning}


Then berr-counter-tx continues to count up to 
128:
-> enter state ERROR-PASSIVE.

   I then get a frame:
   Error: 
	Id  : 0x20000004
	Data: 0x0 0x20 0x0 0x0 0x0 0x0 0x0 0x0 
	controller-problem{tx-error-passive}


Then if I connect another participant, berr-counter-tx counter will start
decrementing (bec. of successfull ACK)
When berr-counter-tx is 
127:
-> enter state ERROR_WARNING.

   I then get a frame:
   Error: 
	Id  : 0x20000004
	Data: 0x0 0x8 0x0 0x0 0x0 0x0 0x0 0x0 
	controller-problem{tx-error-warning}


When berr-counter-tx reaches 
95: 
-> enter state ERROR_ACTIVE

BUT I NEVER GET the frame
	Id  : 0x20000004
	Data: 0x0 0x40 0x0 0x0 0x0 0x0 0x0 0x0 
	controller-problem{back-to-error-active}
WHY?
WHY DO I NOT GET THAT FRAME?
IS THIS A BUG???


The command
   ip -details link show can0     
shows...

3: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode
DEFAULT group default qlen 10
    link/can  promiscuity 0
    can state ERROR-ACTIVE (berr-counter tx 94 rx 80) restart-ms 0
          bitrate 250000 sample-point 0.875
          tq 62 prop-seg 27 phase-seg1 28 phase-seg2 8 sjw 1
          pcan_usb_fd: tseg1 1..64 tseg2 1..16 sjw 1..16 brp 1..1024 brp-inc 1
          pcan_usb_fd: dtseg1 1..16 dtseg2 1..8 dsjw 1..4 dbrp 1..1024
dbrp-inc 1
          clock 80000000

...so I'm definately in error-active.
(But I never received back-to-error-active. Why? Is this a bug?)

Thanks.
ajneu



PS:
------------------------------------
When berr-counter-tx switches from 96 to
95:
and I then do command
   ip -details link show can0     
it shows...

3: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode
DEFAULT group default qlen 10
    link/can  promiscuity 0
    can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
          bitrate 250000 sample-point 0.875
          tq 62 prop-seg 27 phase-seg1 28 phase-seg2 8 sjw 1
          pcan_usb_fd: tseg1 1..64 tseg2 1..16 sjw 1..16 brp 1..1024 brp-inc 1
          pcan_usb_fd: dtseg1 1..16 dtseg2 1..8 dsjw 1..4 dbrp 1..1024
dbrp-inc 1
          clock 80000000


That ALSO SEEMS TO BE A BUG. I expect berr-counter-tx to be 95 and not 0 as
shown.
But when berr-counter-tx decrements one more, I get 

3: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode
DEFAULT group default qlen 10
    link/can  promiscuity 0
    can state ERROR-ACTIVE (berr-counter tx 94 rx 80) restart-ms 0
          bitrate 250000 sample-point 0.875
          tq 62 prop-seg 27 phase-seg1 28 phase-seg2 8 sjw 1
          pcan_usb_fd: tseg1 1..64 tseg2 1..16 sjw 1..16 brp 1..1024 brp-inc 1
          pcan_usb_fd: dtseg1 1..16 dtseg2 1..8 dsjw 1..4 dbrp 1..1024
dbrp-inc 1
          clock 80000000

Re: Bug? -- NEVER getting controller-problem{back-to-error-active}

[ajneu]

ajneu <ajneu1 <at> gmail.com> writes:
> I'm using a PEAK USB Adapter (PCAN-USB FD) for normal CAN Frames (standard
> format), and lsmod shows peak_usb as it should.
> 
> When running on a can bus and continually send out messages (one frame each
> second).
> But there is no other participant, so I will get NO ACK.
> 
> So berr-counter-tx counts up until it reaches
> 96:
> -> enter state ERROR-WARNING.
> 
>    I then get a frame:
>    Error: 
> 	Id  : 0x20000004
> 	Data: 0x0 0x8 0x0 0x0 0x0 0x0 0x0 0x0 
> 	controller-problem{tx-error-warning}
> [...]

I should mention which settings I'm using (I hope the settings are ok.)

        const can_err_mask_t err_mask =
            ( CAN_ERR_TX_TIMEOUT   /* TX timeout (by netdevice driver) */
            | CAN_ERR_LOSTARB      /* lost arbitration    / data[0]    */
            | CAN_ERR_CRTL         /* controller problems / data[1]    */
            | CAN_ERR_PROT         /* protocol violations / data[2..3] */
            | CAN_ERR_TRX          /* transceiver status  / data[4]    */
            | CAN_ERR_ACK          /* received no ACK on transmission */
            | CAN_ERR_BUSOFF       /* bus off */
            //CAN_ERR_BUSERROR     /* bus error (may flood!) */
            | CAN_ERR_RESTARTED    /* controller restarted */
        );

        ret = setsockopt(sock_fd, SOL_CAN_RAW, CAN_RAW_ERR_FILTER,
&err_mask, sizeof(err_mask));


AND

        // Write should block, when socketcan's own queue is full.
        // To have this behaviour (and not get error ENOBUFS) set SO_SNDBUF
to its minimum value.
        // See https://rtime.felk.cvut.cz/can/socketcan-qdisc-final.pdf#page=21
        const int sndbuf = 0;
        if (setsockopt(sock_fd, SOL_SOCKET, SO_SNDBUF, &sndbuf,
sizeof(sndbuf)) < 0) {
            std::cerr << "Error setting SO_SNDBUF in setsockopt" <<
std::strerror(errno) << std::endl;
            return -1;
        }

Re: Bug? -- NEVER getting controller-problem{back-to-error-active}

[Wolfgang Grandegger]

Hello,

Am 20.06.2016 um 11:12 schrieb ajneu:
> Hi,
> 
> I'm using a PEAK USB Adapter (PCAN-USB FD) for normal CAN Frames (standard
> format), and lsmod shows peak_usb as it should.
> 
> When running on a can bus and continually send out messages (one frame each
> second).
> But there is no other participant, so I will get NO ACK.
> 
> 
> So berr-counter-tx counts up until it reaches
> 96:
> -> enter state ERROR-WARNING.
> 
>     I then get a frame:
>     Error:
> 	Id  : 0x20000004
> 	Data: 0x0 0x8 0x0 0x0 0x0 0x0 0x0 0x0
> 	controller-problem{tx-error-warning}
> 
> 
> Then berr-counter-tx continues to count up to
> 128:
> -> enter state ERROR-PASSIVE.
> 
>     I then get a frame:
>     Error:
> 	Id  : 0x20000004
> 	Data: 0x0 0x20 0x0 0x0 0x0 0x0 0x0 0x0
> 	controller-problem{tx-error-passive}
> 
> 
> Then if I connect another participant, berr-counter-tx counter will start
> decrementing (bec. of successfull ACK)
> When berr-counter-tx is
> 127:
> -> enter state ERROR_WARNING.
> 
>     I then get a frame:
>     Error:
> 	Id  : 0x20000004
> 	Data: 0x0 0x8 0x0 0x0 0x0 0x0 0x0 0x0
> 	controller-problem{tx-error-warning}
> 
> 
> When berr-counter-tx reaches
> 95:
> -> enter state ERROR_ACTIVE
> 
> BUT I NEVER GET the frame
> 	Id  : 0x20000004
> 	Data: 0x0 0x40 0x0 0x0 0x0 0x0 0x0 0x0
> 	controller-problem{back-to-error-active}
> WHY?
> WHY DO I NOT GET THAT FRAME?
> IS THIS A BUG???
> 
> 
> The command
>     ip -details link show can0
> shows...
> 
> 3: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode
> DEFAULT group default qlen 10
>      link/can  promiscuity 0
>      can state ERROR-ACTIVE (berr-counter tx 94 rx 80) restart-ms 0
>            bitrate 250000 sample-point 0.875
>            tq 62 prop-seg 27 phase-seg1 28 phase-seg2 8 sjw 1
>            pcan_usb_fd: tseg1 1..64 tseg2 1..16 sjw 1..16 brp 1..1024 brp-inc 1
>            pcan_usb_fd: dtseg1 1..16 dtseg2 1..8 dsjw 1..4 dbrp 1..1024
> dbrp-inc 1
>            clock 80000000
> 
> ...so I'm definately in error-active.
> (But I never received back-to-error-active. Why? Is this a bug?)
> 
> Thanks.
> ajneu
> 
> 
> 
> PS:
> ------------------------------------
> When berr-counter-tx switches from 96 to
> 95:
> and I then do command
>     ip -details link show can0
> it shows...
> 
> 3: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode
> DEFAULT group default qlen 10
>      link/can  promiscuity 0
>      can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
>            bitrate 250000 sample-point 0.875
>            tq 62 prop-seg 27 phase-seg1 28 phase-seg2 8 sjw 1
>            pcan_usb_fd: tseg1 1..64 tseg2 1..16 sjw 1..16 brp 1..1024 brp-inc 1
>            pcan_usb_fd: dtseg1 1..16 dtseg2 1..8 dsjw 1..4 dbrp 1..1024
> dbrp-inc 1
>            clock 80000000
> 
> 
> That ALSO SEEMS TO BE A BUG. I expect berr-counter-tx to be 95 and not 0 as
> shown.
> But when berr-counter-tx decrements one more, I get
> 
> 3: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode
> DEFAULT group default qlen 10
>      link/can  promiscuity 0
>      can state ERROR-ACTIVE (berr-counter tx 94 rx 80) restart-ms 0
>            bitrate 250000 sample-point 0.875
>            tq 62 prop-seg 27 phase-seg1 28 phase-seg2 8 sjw 1
>            pcan_usb_fd: tseg1 1..64 tseg2 1..16 sjw 1..16 brp 1..1024 brp-inc 1
>            pcan_usb_fd: dtseg1 1..16 dtseg2 1..8 dsjw 1..4 dbrp 1..1024
> dbrp-inc 1
>            clock 80000000
> 

Does the following patch fix both issues?

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index ce44a03..ca17ac2 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -529,7 +529,7 @@ static int pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
        struct peak_usb_device *dev = usb_if->dev[pucan_stmsg_get_channel(sm)];
        struct pcan_usb_fd_device *pdev =
                        container_of(dev, struct pcan_usb_fd_device, dev);
-       enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
+       enum can_state new_state;
        enum can_state rx_state, tx_state;
        struct net_device *netdev = dev->netdev;
        struct can_frame *cf;
@@ -547,10 +547,7 @@ static int pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
                new_state = CAN_STATE_ERROR_WARNING;
        } 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;
+               new_state = CAN_STATE_ERROR_ACTIVE;
        }
 
        /* state hasn't changed */


Wolfgang.

Re: Bug? -- NEVER getting controller-problem{back-to-error-active}

[ajneu]

Wolfgang Grandegger <wg <at> grandegger.com> writes:

> 
> Does the following patch fix both issues?
> 
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> index ce44a03..ca17ac2 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>  <at>  <at>  -529,7 +529,7  <at>  <at>  static int
pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
>         struct peak_usb_device *dev =
usb_if->dev[pucan_stmsg_get_channel(sm)];
>         struct pcan_usb_fd_device *pdev =
>                         container_of(dev, struct pcan_usb_fd_device, dev);
> -       enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
> +       enum can_state new_state;
>         enum can_state rx_state, tx_state;
>         struct net_device *netdev = dev->netdev;
>         struct can_frame *cf;
>  <at>  <at>  -547,10 +547,7  <at>  <at>  static int
pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
>                 new_state = CAN_STATE_ERROR_WARNING;
>         } 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;
> +               new_state = CAN_STATE_ERROR_ACTIVE;
>         }
> 
>         /* state hasn't changed */
> 
> Wolfgang.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo <at> vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 


Hi Wolfgang,

yes wonderful: both issues are fixed with your patch!
Will that be going into the mainline linux kernel??

Thanks.

Regards,
ajneu


PS:
Note to self -- compiling kernel with Wolfgang's change:

## this is under Debian (stretch)
## (I'm currently using Kernel 4.6)
sudo aptitude install build-essential
sudo aptitude install linux-source
mkdir ~/kernel
cd    ~/kernel
tar xf /usr/src/linux-source-4.6.tar.xz
cd ~/kernel/linux-source*
cp /boot/config-4.6.0-1-amd64                          .config
cp /usr/src/linux-headers-4.6.0-1-amd64/Module.symvers .
make prepare
make modules_prepare
make SUBDIRS=scripts/mod
make SUBDIRS=drivers/net/can/usb/peak_usb modules

## perform patch and then recompile with:

make SUBDIRS=drivers/net/can/usb/peak_usb modules
lsmod | grep can  ## see if previous peak_usb 
                  ## is still loaded. If this is the case then:
sudo rmmod peak_usb

## load newly compiled driver
sudo insmod drivers/net/can/usb/peak_usb/peak_usb.ko

Re: Bug? -- NEVER getting controller-problem{back-to-error-active}

[ajneu]

Wolfgang Grandegger <wg <at> grandegger.com> writes:

> 
> Does the following patch fix both issues?
> 
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> index ce44a03..ca17ac2 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>  <at>  <at>  -529,7 +529,7  <at>  <at>  static int
pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
>         struct peak_usb_device *dev =
usb_if->dev[pucan_stmsg_get_channel(sm)];
>         struct pcan_usb_fd_device *pdev =
>                         container_of(dev, struct pcan_usb_fd_device, dev);
> -       enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
> +       enum can_state new_state;
>         enum can_state rx_state, tx_state;
>         struct net_device *netdev = dev->netdev;
>         struct can_frame *cf;
>  <at>  <at>  -547,10 +547,7  <at>  <at>  static int
pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
>                 new_state = CAN_STATE_ERROR_WARNING;
>         } 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;
> +               new_state = CAN_STATE_ERROR_ACTIVE;
>         }
> 
>         /* state hasn't changed */
> 
> Wolfgang.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo <at> vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 


Hi Wolfgang,

yes wonderful: you patch works (and solves both issues)!
Will it be going into the mainline linux kernel?

Thanks,
ajneu


PS:
Note to self: compiling module peak_usb with Wolfgang's patch

## this is under Debian (stretch)
## (I'm currently using Kernel 4.6)
sudo aptitude install build-essential
sudo aptitude install linux-source
mkdir ~/kernel
cd    ~/kernel
tar xf /usr/src/linux-source-4.6.tar.xz
cd ~/kernel/linux-source*
cp /boot/config-4.6.0-1-amd64                          .config
cp /usr/src/linux-headers-4.6.0-1-amd64/Module.symvers .
make prepare
make modules_prepare
make SUBDIRS=scripts/mod
make SUBDIRS=drivers/net/can/usb/peak_usb modules

## perform patch and then recompile with:

make SUBDIRS=drivers/net/can/usb/peak_usb modules
lsmod | grep can  ## see if previous peak_usb 
                  ## is still loaded. If this is the case then:
sudo rmmod peak_usb

## load newly compiled driver
sudo insmod drivers/net/can/usb/peak_usb/peak_usb.ko

Re: Bug? -- NEVER getting controller-problem{back-to-error-active}

[Wolfgang Grandegger]

Am 20.06.2016 um 15:00 schrieb ajneu:
> Wolfgang Grandegger <wg <at> grandegger.com> writes:
>
>>
>> Does the following patch fix both issues?
>>
>> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>> index ce44a03..ca17ac2 100644
>> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>>   <at>  <at>  -529,7 +529,7  <at>  <at>  static int
> pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
>>          struct peak_usb_device *dev =
> usb_if->dev[pucan_stmsg_get_channel(sm)];
>>          struct pcan_usb_fd_device *pdev =
>>                          container_of(dev, struct pcan_usb_fd_device, dev);
>> -       enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
>> +       enum can_state new_state;
>>          enum can_state rx_state, tx_state;
>>          struct net_device *netdev = dev->netdev;
>>          struct can_frame *cf;
>>   <at>  <at>  -547,10 +547,7  <at>  <at>  static int
> pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
>>                  new_state = CAN_STATE_ERROR_WARNING;
>>          } 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;
>> +               new_state = CAN_STATE_ERROR_ACTIVE;
>>          }
>>
>>          /* state hasn't changed */
>>
>> Wolfgang.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> the body of a message to majordomo <at> vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>
>
> Hi Wolfgang,
>
> yes wonderful: you patch works (and solves both issues)!
> Will it be going into the mainline linux kernel?

I'm going to prepare a patch for mainline inclusion. Can I add your 
"Tested-by: ajneu <ajneu1@gmail.com>"?

Wolfgang.

Re: Bug? -- NEVER getting controller-problem{back-to-error-active}

[ajneu]

Wolfgang Grandegger <wg <at> grandegger.com> writes:

> 
> 
> Am 20.06.2016 um 15:00 schrieb ajneu:
> > Hi Wolfgang,
> >
> > yes wonderful: you patch works (and solves both issues)!
> > Will it be going into the mainline linux kernel?
> 
> I'm going to prepare a patch for mainline inclusion. Can I add your 
> "Tested-by: ajneu <ajneu1 <at> gmail.com>"?

Well ok, you may. 
(Just note I only tested the bugs I reported, and can confirm: those bugs
are gone. I cannot account for any side-effects, since I didn't do any
"complete" tests. But scanning the code (*briefly*) it looks sortof ok... 

Just question: Your change eliminates 
   dev->can.state = CAN_STATE_ERROR_ACTIVE;
where dev references usb_if, which is passed into the function as parameter.
Is it ok to eliminate that call?? 
(Perhaps its equiv to the line 
   can_change_state(netdev, cf, tx_state, rx_state); 
which is called somewhat lower???)

Regards,
ajneu

Re: Bug? -- NEVER getting controller-problem{back-to-error-active}

[Wolfgang Grandegger]

hello,

Am 20.06.2016 um 15:53 schrieb ajneu:
> Wolfgang Grandegger <wg <at> grandegger.com> writes:
>
>>
>>
>> Am 20.06.2016 um 15:00 schrieb ajneu:
>>> Hi Wolfgang,
>>>
>>> yes wonderful: you patch works (and solves both issues)!
>>> Will it be going into the mainline linux kernel?
>>
>> I'm going to prepare a patch for mainline inclusion. Can I add your
>> "Tested-by: ajneu <ajneu1 <at> gmail.com>"?
>
> Well ok, you may.
> (Just note I only tested the bugs I reported, and can confirm: those bugs
> are gone. I cannot account for any side-effects, since I didn't do any
> "complete" tests. But scanning the code (*briefly*) it looks sortof ok...

Yes, the changes are obvious. Don't understand why 
CAN_STATE_ERROR_ACTIVE is handled differently. I have added the author.

> Just question: Your change eliminates
>     dev->can.state = CAN_STATE_ERROR_ACTIVE;
> where dev references usb_if, which is passed into the function as parameter.
> Is it ok to eliminate that call??
> (Perhaps its equiv to the line
>     can_change_state(netdev, cf, tx_state, rx_state);
> which is called somewhat lower???)

The state is set in can_change_state().:

http://lxr.free-electrons.com/source/drivers/net/can/dev.c#L341

Wolfgang.

Re: Bug? -- NEVER getting controller-problem{back-to-error-active}

[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.

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

Stéphane

Le 20/06/2016 à 16:28, Wolfgang Grandegger a écrit :
> hello,
>
> Am 20.06.2016 um 15:53 schrieb ajneu:
>> Wolfgang Grandegger <wg <at> grandegger.com> writes:
>>
>>>
>>>
>>> Am 20.06.2016 um 15:00 schrieb ajneu:
>>>> Hi Wolfgang,
>>>>
>>>> yes wonderful: you patch works (and solves both issues)!
>>>> Will it be going into the mainline linux kernel?
>>>
>>> I'm going to prepare a patch for mainline inclusion. Can I add your
>>> "Tested-by: ajneu <ajneu1 <at> gmail.com>"?
>>
>> Well ok, you may.
>> (Just note I only tested the bugs I reported, and can confirm: those 
>> bugs
>> are gone. I cannot account for any side-effects, since I didn't do any
>> "complete" tests. But scanning the code (*briefly*) it looks sortof 
>> ok...
>
> Yes, the changes are obvious. Don't understand why 
> CAN_STATE_ERROR_ACTIVE is handled differently. I have added the author.
>
>> Just question: Your change eliminates
>>     dev->can.state = CAN_STATE_ERROR_ACTIVE;
>> where dev references usb_if, which is passed into the function as 
>> parameter.
>> Is it ok to eliminate that call??
>> (Perhaps its equiv to the line
>>     can_change_state(netdev, cf, tx_state, rx_state);
>> which is called somewhat lower???)
>
> The state is set in can_change_state().:
>
> http://lxr.free-electrons.com/source/drivers/net/can/dev.c#L341
>
> Wolfgang.

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183 
Geschaeftsfuehrung: A.Gach, U.Wilhelm
--

Re: Bug? -- NEVER getting controller-problem{back-to-error-active}

[Wolfgang Grandegger]

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.