Request-for-testing: j1939

Request-for-testing: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 952 bytes --]

Hello,

I've worked my way a bit through the lower layers of the j1939 stack.
While there I cleaned up the livetime handling of the j1939 object and
the bind() and connect() functions. Unplugging a USB network device
while a application is running should now be possible.

The patch stack consists of ~100 patches, ~15 of them are cleanups for
the af_can. I'm not going to post the patches here for review, testing
is much more welcome. Maybe we can put together a testsuite.

The branch is available at my kernel.org linux-can-next repo in the
j1939 branch.

https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=j1939

regards,
Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Request-for-testing: j1939

[Kurt Van Dijck]

Marc,

Looks good.
I grouped the patches in 3 groups:
* Patches we discussed
* code (style) fixes
* networking layer patches that appear to be improve the code
  in a way I couldn't have done.

I'll try to run some tests on it during the next couple of weeks.

Kind regards,
Kurt

--- Original message ---
> Date: Wed, 23 Aug 2017 17:17:54 +0200
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> To: "linux-can@vger.kernel.org" <linux-can@vger.kernel.org>, Kurt Van Dijck
>  <dev.kurt@vandijck-laurijssen.be>, David Jander <david@protonic.nl>
> Subject: Request-for-testing: j1939
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>  Thunderbird/52.2.1
> 
> Hello,
> 
> I've worked my way a bit through the lower layers of the j1939 stack.
> While there I cleaned up the livetime handling of the j1939 object and
> the bind() and connect() functions. Unplugging a USB network device
> while a application is running should now be possible.
> 
> The patch stack consists of ~100 patches, ~15 of them are cleanups for
> the af_can. I'm not going to post the patches here for review, testing
> is much more welcome. Maybe we can put together a testsuite.
> 
> The branch is available at my kernel.org linux-can-next repo in the
> j1939 branch.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=j1939
> 
> regards,
> Marc
> -- 
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
> 

Re: Request-for-testing: j1939

[Kurt Van Dijck]

Marc,

I got to testing your work.

commit 89b45fdafe8c7edbff6c2bd9bb51460732410e21 introduces a problem:
if I do 'ip link set can0 down' when j1939 sockets are open,
then j1939sk_release produces a page fault.

That particular commit decouples name & sa from the (j1939) netdev,
but j1939sk_release will do that too.
I imagined that 'ip link set can0 down; ip link set can0 up' is
something a client program may choose to survive, and so the netdev down
event isn't propagated that way. If a program chose to quit,
j1939sk_release is called which cleans up bound names & addresses.

Any opinions?
I will try with that commit reverted. I don't expect negative side
effects :-)

I did not experiment yet with device removal, something this commit did
not catch either.

Kurt

> --- Original message ---
> > Date: Wed, 23 Aug 2017 17:17:54 +0200
> > From: Marc Kleine-Budde <mkl@pengutronix.de>
> > To: "linux-can@vger.kernel.org" <linux-can@vger.kernel.org>, Kurt Van Dijck
> >  <dev.kurt@vandijck-laurijssen.be>, David Jander <david@protonic.nl>
> > Subject: Request-for-testing: j1939
> > User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
> >  Thunderbird/52.2.1
> > 
> > Hello,
> > 
> > I've worked my way a bit through the lower layers of the j1939 stack.
> > While there I cleaned up the livetime handling of the j1939 object and
> > the bind() and connect() functions. Unplugging a USB network device
> > while a application is running should now be possible.
> > 
> > The patch stack consists of ~100 patches, ~15 of them are cleanups for
> > the af_can. I'm not going to post the patches here for review, testing
> > is much more welcome. Maybe we can put together a testsuite.
> > 
> > The branch is available at my kernel.org linux-can-next repo in the
> > j1939 branch.
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=j1939
> > 
> > regards,
> > Marc
> > -- 
> > Pengutronix e.K.                  | Marc Kleine-Budde           |
> > Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> > Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> > Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
> > 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request-for-testing: j1939

[Kurt Van Dijck]

> Hello,
> 
> I've worked my way a bit through the lower layers of the j1939 stack.
> While there I cleaned up the livetime handling of the j1939 object and
> the bind() and connect() functions. Unplugging a USB network device
> while a application is running should now be possible.
> 
> The patch stack consists of ~100 patches, ~15 of them are cleanups for
> the af_can. I'm not going to post the patches here for review, testing
> is much more welcome. Maybe we can put together a testsuite.
> 
> The branch is available at my kernel.org linux-can-next repo in the
> j1939 branch.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=j1939

Marc,

I completed some tests on your branch.
I did amend some tiny patches, please find them
at https://github.com/kurt-vd/linux/tree/can-next-j1939

I did revert 1 patch which I mentioned already some days ago,
I did 2 optimizations, eliminating locks, and added 2 fixes.

I did take the effort to test with 2 CAN adapters this time, since
Elenita had pointed me to a mistake that wasn't revealed doing tests
locally using vcan :-)

The stack seems operational, ready for more tests.

Kurt

Re: Request-for-testing: j1939

[Kurt Van Dijck]

Small update: unplugging the hardware produces a problem with open
sockets. I think this is what you tried to solve, and I reverted since
it dealt with iface down ...
You probably needed to tests against 'ENODEV' rather than ENETDOWN to
satisfy my tests :-)
I'll work on this ...

Kurt

> > Hello,
> > 
> > I've worked my way a bit through the lower layers of the j1939 stack.
> > While there I cleaned up the livetime handling of the j1939 object and
> > the bind() and connect() functions. Unplugging a USB network device
> > while a application is running should now be possible.
> > 
> > The patch stack consists of ~100 patches, ~15 of them are cleanups for
> > the af_can. I'm not going to post the patches here for review, testing
> > is much more welcome. Maybe we can put together a testsuite.
> > 
> > The branch is available at my kernel.org linux-can-next repo in the
> > j1939 branch.
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=j1939
> 
> Marc,
> 
> I completed some tests on your branch.
> I did amend some tiny patches, please find them
> at https://github.com/kurt-vd/linux/tree/can-next-j1939
> 
> I did revert 1 patch which I mentioned already some days ago,
> I did 2 optimizations, eliminating locks, and added 2 fixes.
> 
> I did take the effort to test with 2 CAN adapters this time, since
> Elenita had pointed me to a mistake that wasn't revealed doing tests
> locally using vcan :-)
> 
> The stack seems operational, ready for more tests.
> 
> Kurt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request-for-testing: j1939

[Kurt Van Dijck]

Marc et al.,

I finalized a working branch (only static addressing tested so far).
Device management behaves well at this point, transport protocol send &
recv, ...

I pushed a new https://github.com/kurt-vd/linux/tree/can-next-j1939
(well, I was a bit supprised I could push this one, without --force
even).
Unfortunately, I must postpone tests with dynamic addressing at least a
week now.

@Marc,
I suppose that when the tiny bits are fixed, then this can easily be
rebased to today's can-next without major impact, isn't it?

Kind regards,
Kurt

--- Original message ---
> Date:   Sun, 8 Oct 2017 15:44:41 +0200
> From: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
> To: Marc Kleine-Budde <mkl@pengutronix.de>, "linux-can@vger.kernel.org"
>  <linux-can@vger.kernel.org>, David Jander <david@protonic.nl>, Elenita
>  Hinds <ecathinds@gmail.com>
> Subject: Re: Request-for-testing: j1939
> User-Agent: Mutt/1.5.22 (2013-10-16)
> 
> Small update: unplugging the hardware produces a problem with open
> sockets. I think this is what you tried to solve, and I reverted since
> it dealt with iface down ...
> You probably needed to tests against 'ENODEV' rather than ENETDOWN to
> satisfy my tests :-)
> I'll work on this ...
> 
> Kurt
> 
> > > Hello,
> > > 
> > > I've worked my way a bit through the lower layers of the j1939 stack.
> > > While there I cleaned up the livetime handling of the j1939 object and
> > > the bind() and connect() functions. Unplugging a USB network device
> > > while a application is running should now be possible.
> > > 
> > > The patch stack consists of ~100 patches, ~15 of them are cleanups for
> > > the af_can. I'm not going to post the patches here for review, testing
> > > is much more welcome. Maybe we can put together a testsuite.
> > > 
> > > The branch is available at my kernel.org linux-can-next repo in the
> > > j1939 branch.
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=j1939
> > 
> > Marc,
> > 
> > I completed some tests on your branch.
> > I did amend some tiny patches, please find them
> > at https://github.com/kurt-vd/linux/tree/can-next-j1939
> > 
> > I did revert 1 patch which I mentioned already some days ago,
> > I did 2 optimizations, eliminating locks, and added 2 fixes.
> > 
> > I did take the effort to test with 2 CAN adapters this time, since
> > Elenita had pointed me to a mistake that wasn't revealed doing tests
> > locally using vcan :-)
> > 
> > The stack seems operational, ready for more tests.
> > 
> > Kurt
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-can" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request-for-testing: j1939

[Elenita Hinds]

Hi Kurt,

I’ve been testing on my end, including the latest commits below, and here are a couple of feedback:

1) [Very minor]  In net/can/j1939/main.c, line 315: I wonder if the ‘printk’ statement is still necessary for a final release.

2) I’m currently encountering a problem where the receiving of packets stops at some point. My test source is still sending the J1939 packets and the device is receiving these packets (verified through an oscilloscope) but both ‘candump’ and ‘testj1939’ tools stop printing data at some point (same packet termination). If I run without the j1939 and just raw CAN using ‘candump', the tool runs infinitely. I’m still digging through the code but I wonder if this is something you’ve seen before and may have some advice.

Thanks,
Elenita



> On Oct 10, 2017, at 11:44 AM, Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be> wrote:
> 
> Marc et al.,
> 
> I finalized a working branch (only static addressing tested so far).
> Device management behaves well at this point, transport protocol send &
> recv, ...
> 
> I pushed a new https://github.com/kurt-vd/linux/tree/can-next-j1939
> (well, I was a bit supprised I could push this one, without --force
> even).
> Unfortunately, I must postpone tests with dynamic addressing at least a
> week now.
> 
> @Marc,
> I suppose that when the tiny bits are fixed, then this can easily be
> rebased to today's can-next without major impact, isn't it?
> 
> Kind regards,
> Kurt
> 
> --- Original message ---
>> Date:   Sun, 8 Oct 2017 15:44:41 +0200
>> From: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
>> To: Marc Kleine-Budde <mkl@pengutronix.de>, "linux-can@vger.kernel.org"
>> <linux-can@vger.kernel.org>, David Jander <david@protonic.nl>, Elenita
>> Hinds <ecathinds@gmail.com>
>> Subject: Re: Request-for-testing: j1939
>> User-Agent: Mutt/1.5.22 (2013-10-16)
>> 
>> Small update: unplugging the hardware produces a problem with open
>> sockets. I think this is what you tried to solve, and I reverted since
>> it dealt with iface down ...
>> You probably needed to tests against 'ENODEV' rather than ENETDOWN to
>> satisfy my tests :-)
>> I'll work on this ...
>> 
>> Kurt
>> 
>>>> Hello,
>>>> 
>>>> I've worked my way a bit through the lower layers of the j1939 stack.
>>>> While there I cleaned up the livetime handling of the j1939 object and
>>>> the bind() and connect() functions. Unplugging a USB network device
>>>> while a application is running should now be possible.
>>>> 
>>>> The patch stack consists of ~100 patches, ~15 of them are cleanups for
>>>> the af_can. I'm not going to post the patches here for review, testing
>>>> is much more welcome. Maybe we can put together a testsuite.
>>>> 
>>>> The branch is available at my kernel.org linux-can-next repo in the
>>>> j1939 branch.
>>>> 
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=j1939
>>> 
>>> Marc,
>>> 
>>> I completed some tests on your branch.
>>> I did amend some tiny patches, please find them
>>> at https://github.com/kurt-vd/linux/tree/can-next-j1939
>>> 
>>> I did revert 1 patch which I mentioned already some days ago,
>>> I did 2 optimizations, eliminating locks, and added 2 fixes.
>>> 
>>> I did take the effort to test with 2 CAN adapters this time, since
>>> Elenita had pointed me to a mistake that wasn't revealed doing tests
>>> locally using vcan :-)
>>> 
>>> The stack seems operational, ready for more tests.
>>> 
>>> Kurt
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request-for-testing: j1939

[Kurt Van Dijck]

> Hi Kurt,
> 
> I’ve been testing on my end, including the latest commits below, and here are a couple of feedback:
> 
> 1) [Very minor]  In net/can/j1939/main.c, line 315: I wonder if the ‘printk’ statement is still necessary for a final release.

That line (I think marc added it) must indeed go at some point.
It triggered me to make less use of that function.

> 
> 2) I’m currently encountering a problem where the receiving of packets stops at some point. My test source is still sending the J1939 packets and the device is receiving these packets (verified through an oscilloscope) but both ‘candump’ and ‘testj1939’ tools stop printing data at some point (same packet termination). If I run without the j1939 and just raw CAN using ‘candump', the tool runs infinitely. I’m still digging through the code but I wonder if this is something you’ve seen before and may have some advice.

This sounds bad, but I cannot yet believe it is related to can-j1939
only. The most probable I can think of that adding can-j1939 introduces
more work on the cpu and that may have negative side effects, but again,
that's pure guessing.

I haven't encountered it yet.
I'm not doing long tests anymore lately, so I may have missed it.

This deserves to be investigated: which hardware do you use?

Kurt

> 
> Thanks,
> Elenita
> 
> 
> 
> > On Oct 10, 2017, at 11:44 AM, Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be> wrote:
> > 
> > Marc et al.,
> > 
> > I finalized a working branch (only static addressing tested so far).
> > Device management behaves well at this point, transport protocol send &
> > recv, ...
> > 
> > I pushed a new https://github.com/kurt-vd/linux/tree/can-next-j1939
> > (well, I was a bit supprised I could push this one, without --force
> > even).
> > Unfortunately, I must postpone tests with dynamic addressing at least a
> > week now.
> > 
> > @Marc,
> > I suppose that when the tiny bits are fixed, then this can easily be
> > rebased to today's can-next without major impact, isn't it?
> > 
> > Kind regards,
> > Kurt
> > 
> > --- Original message ---
> >> Date:   Sun, 8 Oct 2017 15:44:41 +0200
> >> From: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
> >> To: Marc Kleine-Budde <mkl@pengutronix.de>, "linux-can@vger.kernel.org"
> >> <linux-can@vger.kernel.org>, David Jander <david@protonic.nl>, Elenita
> >> Hinds <ecathinds@gmail.com>
> >> Subject: Re: Request-for-testing: j1939
> >> User-Agent: Mutt/1.5.22 (2013-10-16)
> >> 
> >> Small update: unplugging the hardware produces a problem with open
> >> sockets. I think this is what you tried to solve, and I reverted since
> >> it dealt with iface down ...
> >> You probably needed to tests against 'ENODEV' rather than ENETDOWN to
> >> satisfy my tests :-)
> >> I'll work on this ...
> >> 
> >> Kurt
> >> 
> >>>> Hello,
> >>>> 
> >>>> I've worked my way a bit through the lower layers of the j1939 stack.
> >>>> While there I cleaned up the livetime handling of the j1939 object and
> >>>> the bind() and connect() functions. Unplugging a USB network device
> >>>> while a application is running should now be possible.
> >>>> 
> >>>> The patch stack consists of ~100 patches, ~15 of them are cleanups for
> >>>> the af_can. I'm not going to post the patches here for review, testing
> >>>> is much more welcome. Maybe we can put together a testsuite.
> >>>> 
> >>>> The branch is available at my kernel.org linux-can-next repo in the
> >>>> j1939 branch.
> >>>> 
> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=j1939
> >>> 
> >>> Marc,
> >>> 
> >>> I completed some tests on your branch.
> >>> I did amend some tiny patches, please find them
> >>> at https://github.com/kurt-vd/linux/tree/can-next-j1939
> >>> 
> >>> I did revert 1 patch which I mentioned already some days ago,
> >>> I did 2 optimizations, eliminating locks, and added 2 fixes.
> >>> 
> >>> I did take the effort to test with 2 CAN adapters this time, since
> >>> Elenita had pointed me to a mistake that wasn't revealed doing tests
> >>> locally using vcan :-)
> >>> 
> >>> The stack seems operational, ready for more tests.
> >>> 
> >>> Kurt
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-can" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: Request-for-testing: j1939

[Elenita Hinds]

Kurt,

I'm using a 1GHz ARM Cortex A8 processor (similar to BeagleBone Black Wireless).

I'm continuing to debug and will post an update of my findings.

Regards,
Elenita

On Wed, Oct 11, 2017 at 1:55 PM, Kurt Van Dijck
<dev.kurt@vandijck-laurijssen.be> wrote:
>> Hi Kurt,
>>
>> I’ve been testing on my end, including the latest commits below, and here are a couple of feedback:
>>
>> 1) [Very minor]  In net/can/j1939/main.c, line 315: I wonder if the ‘printk’ statement is still necessary for a final release.
>
> That line (I think marc added it) must indeed go at some point.
> It triggered me to make less use of that function.
>
>>
>> 2) I’m currently encountering a problem where the receiving of packets stops at some point. My test source is still sending the J1939 packets and the device is receiving these packets (verified through an oscilloscope) but both ‘candump’ and ‘testj1939’ tools stop printing data at some point (same packet termination). If I run without the j1939 and just raw CAN using ‘candump', the tool runs infinitely. I’m still digging through the code but I wonder if this is something you’ve seen before and may have some advice.
>
> This sounds bad, but I cannot yet believe it is related to can-j1939
> only. The most probable I can think of that adding can-j1939 introduces
> more work on the cpu and that may have negative side effects, but again,
> that's pure guessing.
>
> I haven't encountered it yet.
> I'm not doing long tests anymore lately, so I may have missed it.
>
> This deserves to be investigated: which hardware do you use?
>
> Kurt
>
>>
>> Thanks,
>> Elenita
>>
>>
>>
>> > On Oct 10, 2017, at 11:44 AM, Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be> wrote:
>> >
>> > Marc et al.,
>> >
>> > I finalized a working branch (only static addressing tested so far).
>> > Device management behaves well at this point, transport protocol send &
>> > recv, ...
>> >
>> > I pushed a new https://github.com/kurt-vd/linux/tree/can-next-j1939
>> > (well, I was a bit supprised I could push this one, without --force
>> > even).
>> > Unfortunately, I must postpone tests with dynamic addressing at least a
>> > week now.
>> >
>> > @Marc,
>> > I suppose that when the tiny bits are fixed, then this can easily be
>> > rebased to today's can-next without major impact, isn't it?
>> >
>> > Kind regards,
>> > Kurt
>> >
>> > --- Original message ---
>> >> Date:   Sun, 8 Oct 2017 15:44:41 +0200
>> >> From: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
>> >> To: Marc Kleine-Budde <mkl@pengutronix.de>, "linux-can@vger.kernel.org"
>> >> <linux-can@vger.kernel.org>, David Jander <david@protonic.nl>, Elenita
>> >> Hinds <ecathinds@gmail.com>
>> >> Subject: Re: Request-for-testing: j1939
>> >> User-Agent: Mutt/1.5.22 (2013-10-16)
>> >>
>> >> Small update: unplugging the hardware produces a problem with open
>> >> sockets. I think this is what you tried to solve, and I reverted since
>> >> it dealt with iface down ...
>> >> You probably needed to tests against 'ENODEV' rather than ENETDOWN to
>> >> satisfy my tests :-)
>> >> I'll work on this ...
>> >>
>> >> Kurt
>> >>
>> >>>> Hello,
>> >>>>
>> >>>> I've worked my way a bit through the lower layers of the j1939 stack.
>> >>>> While there I cleaned up the livetime handling of the j1939 object and
>> >>>> the bind() and connect() functions. Unplugging a USB network device
>> >>>> while a application is running should now be possible.
>> >>>>
>> >>>> The patch stack consists of ~100 patches, ~15 of them are cleanups for
>> >>>> the af_can. I'm not going to post the patches here for review, testing
>> >>>> is much more welcome. Maybe we can put together a testsuite.
>> >>>>
>> >>>> The branch is available at my kernel.org linux-can-next repo in the
>> >>>> j1939 branch.
>> >>>>
>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=j1939
>> >>>
>> >>> Marc,
>> >>>
>> >>> I completed some tests on your branch.
>> >>> I did amend some tiny patches, please find them
>> >>> at https://github.com/kurt-vd/linux/tree/can-next-j1939
>> >>>
>> >>> I did revert 1 patch which I mentioned already some days ago,
>> >>> I did 2 optimizations, eliminating locks, and added 2 fixes.
>> >>>
>> >>> I did take the effort to test with 2 CAN adapters this time, since
>> >>> Elenita had pointed me to a mistake that wasn't revealed doing tests
>> >>> locally using vcan :-)
>> >>>
>> >>> The stack seems operational, ready for more tests.
>> >>>
>> >>> Kurt
>> >>> --
>> >>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> >>> the body of a message to majordomo@vger.kernel.org
>> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> >> the body of a message to majordomo@vger.kernel.org
>> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: Request-for-testing: j1939

[Kurt Van Dijck]

Marc,

I pushed another commit to my github repository, that fixes the
transport protocol behaviour when the local j1939 node is not
participating.

Kurt


> > I pulled in commit a9ca675 and re-tested -- works!
> 
> Great. I'm glad we found this.
> 
> It seems that the problem existed for quite some time.
> Since we passed an ISOBUS certification with an early prototype of this
> stack, I'm sure it went wrong afterwards some time ... but its getting
> too complicated to retrieve due to the out-of-tree existance of the
> stack :-(
> 
> > I was going to post
> > in the linux-can mail about when the timeframe of merging J1939 in the
> > linux mainline.
> 
> Yep, please share your findings on the linux-can mailing list.
> 
> > 
> > I have started to re-test address claiming and transmissions next.
> > 
> > One question - what is the significance of J1939_IDLE_ADDR (0xfe)? Is
> > this address specified in the J1939 spec (I can't find it).
> 
> 0xfe is to be used when an ecu was not able to claim an address.
> It can be anybody, and it is used by multiple ecu's, just to participate
> in the necessary network management communication.
> 
> > 
> > Thanks!
> > 
> > --elenita
> > 
> > On Tue, Nov 14, 2017 at 2:59 AM, Kurt Van Dijck
> > <dev.kurt@vandijck-laurijssen.be> wrote:
> > > Bravo,
> > >
> > > I think you pointed the right spot.
> > > I'm still looking why it got wrong like that,
> > > in the meanwhile, I pushed a fix in my github repository.
> > >
> > > I found a second similar problem.
> > >
> > > Kurt
> > >
> > >>
> > >> Replies below ...
> > >>
> > >> >I'm curious about the result.
> > >>
> > >> So adding more logging, "priv->ents[cb->addr.da].nusers" is always 0
> > >> (not set at all), as expected since the user space program is not
> > >> claiming any address. But in j1939_can_recv() of main.c,
> > >> skcb->dst_flags is being set to 1 for ECU_LOCAL. See log excerpt
> > >> below:
> > >>
> > >> [  103.744765] can_j1939: j1939sk_bind: j1939_name_local_get name=0
> > >>              <== No name, nusers not set
> > >> [  103.744827] can_j1939: j1939sk_bind: j1939_addr_local_get sa=255
> > >>                 <== No addr claimed, nusers not set
> > >> [  104.135692] j1939_can_recv: da=132 nusers=0 prev_dstflags=0
> > >> curr_dstflags=1  <== Multiframe, curr_dstflags should be 0, not 1
> > >> [  104.136966] j1939_can_recv: da=28   nusers=0 prev_dstflags=0
> > >> curr_dstflags=1  <== Multiframe, curr_dstflags should be 0, not 1
> > >> [  104.174357] j1939_can_recv: da=255 nusers=0 prev_dstflags=0
> > >> curr_dstflags=0  <== Multiframe, curr_dstflags=0 is good
> > >>
> > >> (Note: 'da' contains 'skcb->addr.da' value, 'nusers' =>
> > >> 'priv->ents[skcb->addr.da].nusers', '*_dstflags' => 'skcb->dst_flags'
> > >> before and after.)
> > >>
> > >> So a response transmission is attempted even though the destination
> > >> address is not claimed locally because j1939tp_im_receiver() was
> > >> returning 1.
> > >>
> > >> The problem could be the 'if' condition in lines 105-107 of main.c,
> > >> j1939_can_recv():
> > >> if (j1939_address_is_valid(skcb->addr.da) ||
> > >>     (j1939_address_is_unicast(skcb->addr.da) &&
> > >>      priv->ents[skcb->addr.da].nusers))
> > >>
> > >> As long as skcb->addr.da is not 255, the above logic will always
> > >> return true since j1939_address_is_valid() returns True and the rest
> > >> of the check is skipped because of the OR.
> > >>
> > >> Should the check be something like:
> > >> if ( (j1939_address_is_valid(skcb->addr.da) ||
> > >>       j1939_address_is_unicast(skcb->addr.da))  &&
> > >>       (priv->ents[skcb->addr.da].nusers) )
> > >>
> > >> or
> > >>
> > >> if (  j1939_address_is_valid(skcb->addr.da) &&
> > >>       j1939_address_is_unicast(skcb->addr.da) &&
> > >>       priv->ents[skcb->addr.da].nusers  )
> > >> ??
> > >>
> > >> Actually, is there a need to call j1939_address_is_valid()? DA=255 is
> > >> not allowed for RTS/CTS data transfers anyway and
> > >> j1939tp_im_receiver() is only called for RTS/CTS transmissions as far
> > >> as I can tell.
> > >>
> > >> >I figured now that I the can-j1939 node tries to participate in the
> > >> >replayed transport sessions, then 2 nodes on the CAN bus will try to
> > >> >send the same CAN id's, on a similar moment in time.
> > >> >That is not good at all, do you have bus-off recovery in place?
> > >>
> > >> Currently, only to configure the CAN interfaces to automatically
> > >> restart in case of a bus-off condition (log the error as well).
> > >>
> > >> Looking forward to more feedback.
> > >>
> > >> Thanks,
> > >> Elenita
> > >>
> > >> On Sat, Nov 11, 2017 at 6:17 AM, Kurt Van Dijck
> > >> <dev.kurt@vandijck-laurijssen.be> wrote:
> > >> >> > Do you replay the log from the same node?
> > >> >>
> > >> >> No, not the same node. The CAN frames are sent by a different node.
> > >> >> Are you familiar with PCAN devices, for example
> > >> >> https://gridconnect.com/can-usb.html? So a script running on a PC
> > >> >> sends the CAN frames through the PCAN adapter which is connected to
> > >> >> the hardware running Linux.
> > >> >
> > >> > I'm aware, I think gridconnect delivers rebranded peak can devices, see
> > >> > https://www.peak-system.com/PCAN-USB.199.0.html?L=1
> > >> >
> > >> >>
> > >> >> > Am I correct that you don't to intend to send a single j1939 frame since
> > >> >> > you replay all via a CAN_RAW socket?
> > >> >>
> > >> >> Yes. Currently, the way I'm able to test is using the test setup I
> > >> >> described above, so many frames will be sent and are captured by the
> > >> >> Linux-based hardware.
> > >> >
> > >> > I figured now that I the can-j1939 node tries to participate in the
> > >> > replayed transport sessions, then 2 nodes on the CAN bus will try to
> > >> > send the same CAN id's, on a similar moment in time.
> > >> > That is not good at all, do you have bus-off recovery in place?
> > >> >
> > >> >>
> > >> >> > You are now testing for dynamically addressed endpoints, not static ones.
> > >> >> > I think that what you try to test is already covered by 'j1939tp_im_receiver()', or it should have been covered.
> > >> >> > So the tests in j1939_can_recv (net/can/j1939/main.c) that add ECU_LOCAL are not accurate in your case,
> > >> >> > but I fail to see why.
> > >> >>
> > >> >> I looked at that part but was not quite sure about the intention of
> > >> >> ECU_LOCAL. But now, I understand -- so if no process within the device
> > >> >> claimed an address, the check "priv->ents[cb->addr.da].nusers" should
> > >> >> be 0, right?
> > >> >
> > >> > Well, ECU_LOCAL is cached copy of priv->ents[cb->addr.da].nusers, at the
> > >> > time of arrival, which is in this case not a problem.
> > >> >
> > >> >> I'll check why it is getting set.
> > >> >
> > >> > I'm curious about the result.
> > >> >
> > >> > Kurt
> > >> >
> > >> >>
> > >> >> > Maybe it helps to look to j1939_can_recv to solve your problem.
> > >> >>
> > >> >> Will do.
> > >> >>
> > >> >> Thanks, Kurt.
> > >> >>
> > >> >> --elenita
> > >> >>
> > >> >>
> > >> >> On Fri, Nov 10, 2017 at 8:42 AM, Kurt Van Dijck
> > >> >> <dev.kurt@vandijck-laurijssen.be> wrote:
> > >> >> > Hey,
> > >> >> >
> > >> >> > I remain puzzled with your setup.
> > >> >> > Do you replay the log from the same node?
> > >> >> >
> > >> >> > Am I correct that you don't to intend to send a single j1939 frame since
> > >> >> > you replay all via a CAN_RAW socket?
> > >> >> >
> > >> >> > Please also find some remarks on your hack below:
> > >> >> >
> > >> >> > --- Original message ---
> > >> >> >> Date: Mon, 6 Nov 2017 16:23:01 -0600
> > >> >> >> From: Elenita Hinds <ecathinds@gmail.com>
> > >> >> >> To: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
> > >> >> >> Subject: Re: Request-for-testing: j1939
> > >> >> >>
> > >> >> >> Hi Kurt,
> > >> >> >>
> > >> >> >> My temporary hack for the addressing issue is to create a new function
> > >> >> >> in bus.c called j1939_address_is_claimed():
> > >> >> >>
> > >> >> >> bool j1939_address_is_claimed(u8 addr, int ifindex)
> > >> >> >> {
> > >> >> >>     struct j1939_ecu *ecu = NULL;
> > >> >> >>     struct j1939_priv *priv;
> > >> >> >>     bool result = false;
> > >> >> >>
> > >> >> >>     if (!j1939_address_is_unicast(addr))
> > >> >> >>         return false;
> > >> >> >>
> > >> >> >>     priv = j1939_priv_get_by_ifindex(ifindex);
> > >> >> >>     if (!priv)
> > >> >> >>         return false;
> > >> >> >>
> > >> >> >>     read_lock_bh(&priv->lock);
> > >> >> >>     list_for_each_entry(ecu, &priv->ecus, list) {
> > >> >> >>         if (ecu->sa == addr) {
> > >> >> >>             /* Destination address claimed here */
> > >> >> >>             result = true;
> > >> >> >>             break;
> > >> >> >>         }
> > >> >> >>     }
> > >> >> >>     read_unlock_bh(&priv->lock);
> > >> >> >>
> > >> >> >>     return result;
> > >> >> >> }
> > >> >> >>
> > >> >> >>
> > >> >> >> This function is then called in transport.c, functions j1939xtp_rx_rts():
> > >> >> >>
> > >> >> >> if (j1939tp_im_receiver(session->skb))  {
> > >> >> >>         // TEST: Check if claimed by this device
> > >> >> >>         bool isClaimed = j1939_address_is_claimed(cb->addr.da,
> > >> >> >> session->skb->dev->ifindex);
> > >> >> >>         if ( (extd || (tp_cmd_bam != dat[0])) && (isClaimed == true) )
> > >> >> >>             j1939_session_schedule_txnow(session);
> > >> >> >> }
> > >> >> >
> > >> >> > You are now testing for dynamically addressed endpoints, not static
> > >> >> > ones.
> > >> >> > I think that what you try to test is already covered by
> > >> >> > 'j1939tp_im_receiver()', or it should have been covered.
> > >> >> > So the tests in j1939_can_recv (net/can/j1939/main.c) that add ECU_LOCAL
> > >> >> > are not accurate in your case, but I fail to see why.
> > >> >> > I bet it originates from 'replaying', of which I'm sure I never tested
> > >> >> > like that :-(
> > >> >> >
> > >> >> > Maybe it helps to look to j1939_can_recv to solve your problem.
> > >> >> >
> > >> >> > Kind regards,
> > >> >> > Kurt

Re: Request-for-testing: j1939

[Elenita Hinds]

Hi Marc,

I'm new here.  I can help test the branch as well and will send
feedback. I've been using Kurt's branch and will move to your branch.

Regards,

Elenita Hinds