RxFilter issues vcan

RxFilter issues vcan

[Sebastian Haas]

Hello everybody,

I played a bit with the RxFilters and noticed that they are behaving 
somewhat strange.

If I start candump this way:
sh@helios:~/workspace/node-can$ candump vcan0,100~7ff,101~7ff
  I want to receive any messages except 100h and 101h.

When I send a message which matches the filter, it is received twice:
sh@helios:~/workspace/node-can$ cansend vcan0 1ff#22
   vcan0  1FF   [1]  22
   vcan0  1FF   [1]  22

When I send a message which should not received at all, it is received:
sh@helios:~/workspace/node-can$ cansend vcan0 100#22
   vcan0  100   [1]  22

Did I misunderstood the filter here?

Cheers,
Sebastian

Re: RxFilter issues vcan

[Oliver Hartkopp]

Hi Sebastian,

On 29.05.2013 22:55, Sebastian Haas wrote:
> Hello everybody,
> 
> I played a bit with the RxFilters and noticed that they are behaving somewhat
> strange.
> 
> If I start candump this way:
> sh@helios:~/workspace/node-can$ candump vcan0,100~7ff,101~7ff
>  I want to receive any messages except 100h and 101h.
> 
> When I send a message which matches the filter, it is received twice:
> sh@helios:~/workspace/node-can$ cansend vcan0 1ff#22
>   vcan0  1FF   [1]  22
>   vcan0  1FF   [1]  22
> 
> When I send a message which should not received at all, it is received:
> sh@helios:~/workspace/node-can$ cansend vcan0 100#22
>   vcan0  100   [1]  22
> 
> Did I misunderstood the filter here?

The filters are independent and therefore "logical OR".

The 1st filter stops 100
The 2nd filter stops 101

But the second filter lets 100 pass.

If you want to remove 100 and 101, try:

candump vcan0,100~7FE

Or even better

candump vcan0,100~C00007FE

If you want to make sure to get only SFF frames without RTR.

Regards,
Oliver

Re: RxFilter issues vcan

[Sebastian Haas]

Hi,

Am 30.05.2013 06:58, schrieb Oliver Hartkopp:
> On 29.05.2013 22:55, Sebastian Haas wrote:
>> Hello everybody,
>>
>> I played a bit with the RxFilters and noticed that they are behaving somewhat
>> strange.
>>
>> If I start candump this way:
>> sh@helios:~/workspace/node-can$ candump vcan0,100~7ff,101~7ff
>>   I want to receive any messages except 100h and 101h.
>>
>> When I send a message which matches the filter, it is received twice:
>> sh@helios:~/workspace/node-can$ cansend vcan0 1ff#22
>>    vcan0  1FF   [1]  22
>>    vcan0  1FF   [1]  22
>>
>> When I send a message which should not received at all, it is received:
>> sh@helios:~/workspace/node-can$ cansend vcan0 100#22
>>    vcan0  100   [1]  22
>>
>> Did I misunderstood the filter here?
>
> The filters are independent and therefore "logical OR".
Ok. That explain why received 100h even though I tried to filter it out. 
But why receive a message twice while it was only sent once?

Sebastian

Re: RxFilter issues vcan

[Oliver Hartkopp]

Am 30.05.2013 10:34, schrieb Sebastian Haas:
> Hi,
>
> Am 30.05.2013 06:58, schrieb Oliver Hartkopp:
>> On 29.05.2013 22:55, Sebastian Haas wrote:
>>> Hello everybody,
>>>
>>> I played a bit with the RxFilters and noticed that they are behaving somewhat
>>> strange.
>>>
>>> If I start candump this way:
>>> sh@helios:~/workspace/node-can$ candump vcan0,100~7ff,101~7ff
>>>   I want to receive any messages except 100h and 101h.
>>>
>>> When I send a message which matches the filter, it is received twice:
>>> sh@helios:~/workspace/node-can$ cansend vcan0 1ff#22
>>>    vcan0  1FF   [1]  22
>>>    vcan0  1FF   [1]  22
>>>
>>> When I send a message which should not received at all, it is received:
>>> sh@helios:~/workspace/node-can$ cansend vcan0 100#22
>>>    vcan0  100   [1]  22
>>>
>>> Did I misunderstood the filter here?
>>
>> The filters are independent and therefore "logical OR".
> Ok. That explain why received 100h even though I tried to filter it out. But
> why receive a message twice while it was only sent once?

You have defined two(!) filters that let pass the CAN-ID 0x1FF.

So what else would you expect?

:-)

>
> Sebastian
> --
> 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: RxFilter issues vcan

[Sebastian Haas]

Am 30.05.2013 10:49, schrieb Oliver Hartkopp:
>>> On 29.05.2013 22:55, Sebastian Haas wrote:
>>>> If I start candump this way:
>>>> sh@helios:~/workspace/node-can$ candump vcan0,100~7ff,101~7ff
>>>>   I want to receive any messages except 100h and 101h.
>>>>
>>>> When I send a message which matches the filter, it is received twice:
>>>> sh@helios:~/workspace/node-can$ cansend vcan0 1ff#22
>>>>    vcan0  1FF   [1]  22
>>>>    vcan0  1FF   [1]  22
>>>>
>>>> When I send a message which should not received at all, it is received:
>>>> sh@helios:~/workspace/node-can$ cansend vcan0 100#22
>>>>    vcan0  100   [1]  22
>>>>
>>>> Did I misunderstood the filter here?
>>>
>>> The filters are independent and therefore "logical OR".
>> Ok. That explain why received 100h even though I tried to filter it
>> out. But
>> why receive a message twice while it was only sent once?
>
> You have defined two(!) filters that let pass the CAN-ID 0x1FF.
>
> So what else would you expect?
This is actually the way it meant to be? Why? Honestly this is a bug for 
me! The filter should stop when it matched and not going further 
generating ghost messages which were never sent?

Can this behaviour controlled by user space?

Sebastian

RFC: (optional) software filtering in candump

[Kurt Van Dijck]

On Thu, May 30, 2013 at 11:06:30AM +0200, Sebastian Haas wrote:
> Am 30.05.2013 10:49, schrieb Oliver Hartkopp:
> >>>On 29.05.2013 22:55, Sebastian Haas wrote:
> >>>>If I start candump this way:
> >>>>sh@helios:~/workspace/node-can$ candump vcan0,100~7ff,101~7ff
> >>>>  I want to receive any messages except 100h and 101h.
> >>>>
> >>>>When I send a message which matches the filter, it is received twice:
> >>>>sh@helios:~/workspace/node-can$ cansend vcan0 1ff#22
> >>>>   vcan0  1FF   [1]  22
> >>>>   vcan0  1FF   [1]  22
> >>>>
> >>>>When I send a message which should not received at all, it is received:
> >>>>sh@helios:~/workspace/node-can$ cansend vcan0 100#22
> >>>>   vcan0  100   [1]  22
> >>>>
> >>>>Did I misunderstood the filter here?
> >>>
> >>>The filters are independent and therefore "logical OR".
> >>Ok. That explain why received 100h even though I tried to filter it
> >>out. But
> >>why receive a message twice while it was only sent once?
> >
> >You have defined two(!) filters that let pass the CAN-ID 0x1FF.
> >
> >So what else would you expect?
> This is actually the way it meant to be? Why? Honestly this is a bug
> for me! The filter should stop when it matched and not going further
> generating ghost messages which were never sent?

I had initially the same feeling. The problem here IMHO is
what the user expects is different from what the kernel does.

What the user expects is a 'socket' based filter.
The kernel implements an optimised, socket-agnostic filter.
This is even more generic, but the kernel expects a CAN_RAW program
to set 'rough' filters and have the fine control in the program.

This actually works very good, and is a clever thing to do.

However, candump does expose this kernel behaviour to the user. The user,
as said, expects something different that what he gets.
So the user thinks it is a bug.

How to solve this: I think the correct way is to let candump
expose the filtering that a user expects, whether in userspace
or in kernelspace.
Is such approach efficient? no. Does that matter? probably not.
candump is, IMO, not an automation tool. It's an administrator/diagnostic tool.

Although I have no real code in mind yet, I think the expected
filtering is much easier to implement within a single program,
than it would be in kernelspace.

On the other hand, I have found candump's exposure of kernel work
a true friend during development.
So I'd vote to have an extra control on candump which filtering to use.

Any thoughts?

Kurt

Re: RFC: (optional) software filtering in candump

[Oliver Hartkopp]

Am 30.05.2013 12:17, schrieb Kurt Van Dijck:
> On Thu, May 30, 2013 at 11:06:30AM +0200, Sebastian Haas wrote:
>> Am 30.05.2013 10:49, schrieb Oliver Hartkopp:
>>>>> On 29.05.2013 22:55, Sebastian Haas wrote:
>>>>>> If I start candump this way:
>>>>>> sh@helios:~/workspace/node-can$ candump vcan0,100~7ff,101~7ff
>>>>>>   I want to receive any messages except 100h and 101h.
>>>>>>
>>>>>> When I send a message which matches the filter, it is received twice:
>>>>>> sh@helios:~/workspace/node-can$ cansend vcan0 1ff#22
>>>>>>    vcan0  1FF   [1]  22
>>>>>>    vcan0  1FF   [1]  22
>>>>>>
>>>>>> When I send a message which should not received at all, it is received:
>>>>>> sh@helios:~/workspace/node-can$ cansend vcan0 100#22
>>>>>>    vcan0  100   [1]  22
>>>>>>
>>>>>> Did I misunderstood the filter here?
>>>>>
>>>>> The filters are independent and therefore "logical OR".
>>>> Ok. That explain why received 100h even though I tried to filter it
>>>> out. But
>>>> why receive a message twice while it was only sent once?
>>>
>>> You have defined two(!) filters that let pass the CAN-ID 0x1FF.
>>>
>>> So what else would you expect?
>> This is actually the way it meant to be? Why? Honestly this is a bug
>> for me! The filter should stop when it matched and not going further
>> generating ghost messages which were never sent?
>
> I had initially the same feeling. The problem here IMHO is
> what the user expects is different from what the kernel does.
>
> What the user expects is a 'socket' based filter.
> The kernel implements an optimised, socket-agnostic filter.
> This is even more generic, but the kernel expects a CAN_RAW program
> to set 'rough' filters and have the fine control in the program.
>
> This actually works very good, and is a clever thing to do.
>
> However, candump does expose this kernel behaviour to the user. The user,
> as said, expects something different that what he gets.
> So the user thinks it is a bug.
>
> How to solve this: I think the correct way is to let candump
> expose the filtering that a user expects, whether in userspace
> or in kernelspace.
> Is such approach efficient? no. Does that matter? probably not.
> candump is, IMO, not an automation tool. It's an administrator/diagnostic tool.
>
> Although I have no real code in mind yet, I think the expected
> filtering is much easier to implement within a single program,
> than it would be in kernelspace.
>
> On the other hand, I have found candump's exposure of kernel work
> a true friend during development.
> So I'd vote to have an extra control on candump which filtering to use.
>
> Any thoughts?

Yes :-)

I was thinking about a concatenated filter.

On the commandline it could look like this:

	candump vcan0,100~C00007FF&101~C00007FF,<more filters>

The idea would be to add to the existing functions

	can_rx_register()
	can_rx_unregister()

two new functions to af_can.c :

	can_rxcon_register()
	can_rxcon_unregister()

Which would take an array of filters belonging together (logical AND).

This array of filters is then registered but has a linked list between it's 
elements.

If one element is hit by an incoming CAN ID, all elements of this linked list 
are transversed, ...
... until we get back to the first element -> CAN ID passed all filters
... or the filter killed the CAN ID

Concatenated filters are always treated as a single filter, so it would not be 
possible to remove a single CAN-ID check from the linked list.

Looks possible and comfortable at first sight.
But i have no idea about possible runtime constrains and how we would need to 
limit the maximum number of linked list elements.

And if all this is easy to integrate ;-)

Regards,
Oliver

Re: RFC: (optional) software filtering in candump

[Kurt Van Dijck]

On Thu, May 30, 2013 at 02:07:03PM +0200, Oliver Hartkopp wrote:
> Am 30.05.2013 12:17, schrieb Kurt Van Dijck:
> >On Thu, May 30, 2013 at 11:06:30AM +0200, Sebastian Haas wrote:
> >>Am 30.05.2013 10:49, schrieb Oliver Hartkopp:
> >>>>>On 29.05.2013 22:55, Sebastian Haas wrote:
> >>>>>>If I start candump this way:
> >>>>>>sh@helios:~/workspace/node-can$ candump vcan0,100~7ff,101~7ff
> >>>>>>  I want to receive any messages except 100h and 101h.
> >>>>>>
> >>>>>>When I send a message which matches the filter, it is received twice:
> >>>>>>sh@helios:~/workspace/node-can$ cansend vcan0 1ff#22
> >>>>>>   vcan0  1FF   [1]  22
> >>>>>>   vcan0  1FF   [1]  22
> >>>>>>
> >>>>>>When I send a message which should not received at all, it is received:
> >>>>>>sh@helios:~/workspace/node-can$ cansend vcan0 100#22
> >>>>>>   vcan0  100   [1]  22
> >>>>>>
> >>>>>>Did I misunderstood the filter here?
> >>>>>
> >>>>>The filters are independent and therefore "logical OR".
> >>>>Ok. That explain why received 100h even though I tried to filter it
> >>>>out. But
> >>>>why receive a message twice while it was only sent once?
> >>>
> >>>You have defined two(!) filters that let pass the CAN-ID 0x1FF.
> >>>
> >>>So what else would you expect?
> >>This is actually the way it meant to be? Why? Honestly this is a bug
> >>for me! The filter should stop when it matched and not going further
> >>generating ghost messages which were never sent?
> >
> >I had initially the same feeling. The problem here IMHO is
> >what the user expects is different from what the kernel does.
> >
> >What the user expects is a 'socket' based filter.
> >The kernel implements an optimised, socket-agnostic filter.
> >This is even more generic, but the kernel expects a CAN_RAW program
> >to set 'rough' filters and have the fine control in the program.
> >
> >This actually works very good, and is a clever thing to do.
> >
> >However, candump does expose this kernel behaviour to the user. The user,
> >as said, expects something different that what he gets.
> >So the user thinks it is a bug.
> >
> >How to solve this: I think the correct way is to let candump
> >expose the filtering that a user expects, whether in userspace
> >or in kernelspace.
> >Is such approach efficient? no. Does that matter? probably not.
> >candump is, IMO, not an automation tool. It's an administrator/diagnostic tool.
> >
> >Although I have no real code in mind yet, I think the expected
> >filtering is much easier to implement within a single program,
> >than it would be in kernelspace.
> >
> >On the other hand, I have found candump's exposure of kernel work
> >a true friend during development.
> >So I'd vote to have an extra control on candump which filtering to use.
> >
> >Any thoughts?
> 
> Yes :-)
> 
> I was thinking about a concatenated filter.
> 
> On the commandline it could look like this:
> 
> 	candump vcan0,100~C00007FF&101~C00007FF,<more filters>
> 
> The idea would be to add to the existing functions
> 
> 	can_rx_register()
> 	can_rx_unregister()
> 
> two new functions to af_can.c :
> 
> 	can_rxcon_register()
> 	can_rxcon_unregister()
> 
> Which would take an array of filters belonging together (logical AND).
> 
> This array of filters is then registered but has a linked list
> between it's elements.
> 
> If one element is hit by an incoming CAN ID, all elements of this
> linked list are transversed, ...
> ... until we get back to the first element -> CAN ID passed all filters
> ... or the filter killed the CAN ID
> 
> Concatenated filters are always treated as a single filter, so it
> would not be possible to remove a single CAN-ID check from the
> linked list.
> 
> Looks possible and comfortable at first sight.
> But i have no idea about possible runtime constrains and how we
> would need to limit the maximum number of linked list elements.
> 
> And if all this is easy to integrate ;-)

Why would you put this in kernel? The code will (AFAIK) only serve
candump. That's why I proposed to put it there in the first place.

Kind regards,
Kurt

Re: RFC: (optional) software filtering in candump

[Sebastian Haas]

Am 30.05.2013 14:35, schrieb Kurt Van Dijck:
> On Thu, May 30, 2013 at 02:07:03PM +0200, Oliver Hartkopp wrote:
>> Am 30.05.2013 12:17, schrieb Kurt Van Dijck:
>>> On Thu, May 30, 2013 at 11:06:30AM +0200, Sebastian Haas wrote:
>>>> Am 30.05.2013 10:49, schrieb Oliver Hartkopp:
>>>>>>> On 29.05.2013 22:55, Sebastian Haas wrote:
>>>>>>>> If I start candump this way:
>>>>>>>> sh@helios:~/workspace/node-can$ candump vcan0,100~7ff,101~7ff
>>>>>>>>   I want to receive any messages except 100h and 101h.
>>>>>>>>
>>>>>>>> When I send a message which matches the filter, it is received twice:
>>>>>>>> sh@helios:~/workspace/node-can$ cansend vcan0 1ff#22
>>>>>>>>    vcan0  1FF   [1]  22
>>>>>>>>    vcan0  1FF   [1]  22
>>>>>>>>
>>>>>>>> When I send a message which should not received at all, it is received:
>>>>>>>> sh@helios:~/workspace/node-can$ cansend vcan0 100#22
>>>>>>>>    vcan0  100   [1]  22
>>>>>>>>
>>>>>>>> Did I misunderstood the filter here?
>>>>>>>
>>>>>>> The filters are independent and therefore "logical OR".
>>>>>> Ok. That explain why received 100h even though I tried to filter it
>>>>>> out. But
>>>>>> why receive a message twice while it was only sent once?
>>>>>
>>>>> You have defined two(!) filters that let pass the CAN-ID 0x1FF.
>>>>>
>>>>> So what else would you expect?
>>>> This is actually the way it meant to be? Why? Honestly this is a bug
>>>> for me! The filter should stop when it matched and not going further
>>>> generating ghost messages which were never sent?
>>>
>>> I had initially the same feeling. The problem here IMHO is
>>> what the user expects is different from what the kernel does.
>>>
>>> What the user expects is a 'socket' based filter.
>>> The kernel implements an optimised, socket-agnostic filter.
>>> This is even more generic, but the kernel expects a CAN_RAW program
>>> to set 'rough' filters and have the fine control in the program.
>>>
>>> This actually works very good, and is a clever thing to do.
>>>
>>> However, candump does expose this kernel behaviour to the user. The user,
>>> as said, expects something different that what he gets.
>>> So the user thinks it is a bug.
>>>
>>> How to solve this: I think the correct way is to let candump
>>> expose the filtering that a user expects, whether in userspace
>>> or in kernelspace.
>>> Is such approach efficient? no. Does that matter? probably not.
>>> candump is, IMO, not an automation tool. It's an administrator/diagnostic tool.
>>>
>>> Although I have no real code in mind yet, I think the expected
>>> filtering is much easier to implement within a single program,
>>> than it would be in kernelspace.
>>>
>>> On the other hand, I have found candump's exposure of kernel work
>>> a true friend during development.
>>> So I'd vote to have an extra control on candump which filtering to use.
>>>
>>> Any thoughts?
>>
>> Yes :-)
>>
>> I was thinking about a concatenated filter.
>>
>> On the commandline it could look like this:
>>
>> 	candump vcan0,100~C00007FF&101~C00007FF,<more filters>
>>
>> The idea would be to add to the existing functions
>>
>> 	can_rx_register()
>> 	can_rx_unregister()
>>
>> two new functions to af_can.c :
>>
>> 	can_rxcon_register()
>> 	can_rxcon_unregister()
>>
>> Which would take an array of filters belonging together (logical AND).
>>
>> This array of filters is then registered but has a linked list
>> between it's elements.
>>
>> If one element is hit by an incoming CAN ID, all elements of this
>> linked list are transversed, ...
>> ... until we get back to the first element -> CAN ID passed all filters
>> ... or the filter killed the CAN ID
>>
>> Concatenated filters are always treated as a single filter, so it
>> would not be possible to remove a single CAN-ID check from the
>> linked list.
>>
>> Looks possible and comfortable at first sight.
>> But i have no idea about possible runtime constrains and how we
>> would need to limit the maximum number of linked list elements.
>>
>> And if all this is easy to integrate ;-)
>
> Why would you put this in kernel? The code will (AFAIK) only serve
> candump. That's why I proposed to put it there in the first place.
There is more in the world than just candump! In fact my Node extension 
for SocketCAN (https://github.com/sebi2k1/node-can) exposes the filter 
interface to the user as well. It is simply not fast enough to do it in 
Javascript. Furthermore I think filtering the message already in the 
kernel has also an performance advantage (especially on low-budget 
embedded Linux system).

IMHO the current implementation is just broken. I don't care for the 
internals of the filter mechanism but generating ghost message is simply 
wrong.

I like the proposal of Oliver, just because I can then also use AND 
filters and not just OR now.

I still dont understand why I receive the same CAN message twice just 
because the filter matched? Can someone explain the logic behind that?

Cheers,
Sebastian

Re: RFC: (optional) software filtering in candump

[Oliver Hartkopp]

On 30.05.2013 17:37, Sebastian Haas wrote:
> Am 30.05.2013 14:35, schrieb Kurt Van Dijck:
>> On Thu, May 30, 2013 at 02:07:03PM +0200, Oliver Hartkopp wrote:
>>> Am 30.05.2013 12:17, schrieb Kurt Van Dijck:
>>>> On Thu, May 30, 2013 at 11:06:30AM +0200, Sebastian Haas wrote:
>>>>> Am 30.05.2013 10:49, schrieb Oliver Hartkopp:
>>>>>>>> On 29.05.2013 22:55, Sebastian Haas wrote:
>>>>>>>>> If I start candump this way:
>>>>>>>>> sh@helios:~/workspace/node-can$ candump vcan0,100~7ff,101~7ff
>>>>>>>>>   I want to receive any messages except 100h and 101h.
>>>>>>>>>
>>>>>>>>> When I send a message which matches the filter, it is received twice:
>>>>>>>>> sh@helios:~/workspace/node-can$ cansend vcan0 1ff#22
>>>>>>>>>    vcan0  1FF   [1]  22
>>>>>>>>>    vcan0  1FF   [1]  22
>>>>>>>>>
>>>>>>>>> When I send a message which should not received at all, it is received:
>>>>>>>>> sh@helios:~/workspace/node-can$ cansend vcan0 100#22
>>>>>>>>>    vcan0  100   [1]  22
>>>>>>>>>
>>>>>>>>> Did I misunderstood the filter here?
>>>>>>>>
>>>>>>>> The filters are independent and therefore "logical OR".
>>>>>>> Ok. That explain why received 100h even though I tried to filter it
>>>>>>> out. But
>>>>>>> why receive a message twice while it was only sent once?
>>>>>>
>>>>>> You have defined two(!) filters that let pass the CAN-ID 0x1FF.
>>>>>>
>>>>>> So what else would you expect?
>>>>> This is actually the way it meant to be? Why? Honestly this is a bug
>>>>> for me! The filter should stop when it matched and not going further
>>>>> generating ghost messages which were never sent?
>>>>
>>>> I had initially the same feeling. The problem here IMHO is
>>>> what the user expects is different from what the kernel does.
>>>>
>>>> What the user expects is a 'socket' based filter.
>>>> The kernel implements an optimised, socket-agnostic filter.
>>>> This is even more generic, but the kernel expects a CAN_RAW program
>>>> to set 'rough' filters and have the fine control in the program.
>>>>
>>>> This actually works very good, and is a clever thing to do.
>>>>
>>>> However, candump does expose this kernel behaviour to the user. The user,
>>>> as said, expects something different that what he gets.
>>>> So the user thinks it is a bug.
>>>>
>>>> How to solve this: I think the correct way is to let candump
>>>> expose the filtering that a user expects, whether in userspace
>>>> or in kernelspace.
>>>> Is such approach efficient? no. Does that matter? probably not.
>>>> candump is, IMO, not an automation tool. It's an administrator/diagnostic
>>>> tool.
>>>>
>>>> Although I have no real code in mind yet, I think the expected
>>>> filtering is much easier to implement within a single program,
>>>> than it would be in kernelspace.
>>>>
>>>> On the other hand, I have found candump's exposure of kernel work
>>>> a true friend during development.
>>>> So I'd vote to have an extra control on candump which filtering to use.
>>>>
>>>> Any thoughts?
>>>
>>> Yes :-)
>>>
>>> I was thinking about a concatenated filter.
>>>
>>> On the commandline it could look like this:
>>>
>>>     candump vcan0,100~C00007FF&101~C00007FF,<more filters>
>>>
>>> The idea would be to add to the existing functions
>>>
>>>     can_rx_register()
>>>     can_rx_unregister()
>>>
>>> two new functions to af_can.c :
>>>
>>>     can_rxcon_register()
>>>     can_rxcon_unregister()
>>>
>>> Which would take an array of filters belonging together (logical AND).
>>>
>>> This array of filters is then registered but has a linked list
>>> between it's elements.
>>>
>>> If one element is hit by an incoming CAN ID, all elements of this
>>> linked list are transversed, ...
>>> ... until we get back to the first element -> CAN ID passed all filters
>>> ... or the filter killed the CAN ID
>>>
>>> Concatenated filters are always treated as a single filter, so it
>>> would not be possible to remove a single CAN-ID check from the
>>> linked list.
>>>
>>> Looks possible and comfortable at first sight.
>>> But i have no idea about possible runtime constrains and how we
>>> would need to limit the maximum number of linked list elements.
>>>
>>> And if all this is easy to integrate ;-)
>>
>> Why would you put this in kernel? The code will (AFAIK) only serve
>> candump. That's why I proposed to put it there in the first place.
> There is more in the world than just candump! In fact my Node extension for
> SocketCAN (https://github.com/sebi2k1/node-can) exposes the filter interface
> to the user as well. It is simply not fast enough to do it in Javascript.
> Furthermore I think filtering the message already in the kernel has also an
> performance advantage (especially on low-budget embedded Linux system).
> 
> IMHO the current implementation is just broken. I don't care for the internals
> of the filter mechanism but generating ghost message is simply wrong.
> 
> I like the proposal of Oliver, just because I can then also use AND filters
> and not just OR now.

Probably we can reduce the changes by providing AND concatenations of inverted
filters only.

This is a requirement i've seen more often so far.

> 
> I still dont understand why I receive the same CAN message twice just because
> the filter matched? Can someone explain the logic behind that?
> 

You added two independent filters, both matching 0x1FF.

When you invoke this:

candump vcan0,100~7ff,101~7ff,000:000

you will get the CAN frame with the CAN ID 1FF three times.

Clearly now?

Regards,
Oliver

Re: RFC: (optional) software filtering in candump

[Sebastian Haas]

>> I still dont understand why I receive the same CAN message twice just because
>> the filter matched? Can someone explain the logic behind that?
>>
>
> You added two independent filters, both matching 0x1FF.
>
> When you invoke this:
>
> candump vcan0,100~7ff,101~7ff,000:000
>
> you will get the CAN frame with the CAN ID 1FF three times.
>
> Clearly now?
I understoot how the filter system is working, but don't why it was 
implemented this way.

What is the use case for an application to receive messages twice or 
more just because more filters matched? I cannot imagine any reason to 
process more frames than the node actually receive.

Cheers,
Sebastian

Re: RFC: (optional) software filtering in candump

[Oliver Hartkopp]

Hi all,

On 30.05.2013 17:37, Sebastian Haas wrote:
> Am 30.05.2013 14:35, schrieb Kurt Van Dijck:
>> On Thu, May 30, 2013 at 02:07:03PM +0200, Oliver Hartkopp wrote:
>>> Am 30.05.2013 12:17, schrieb Kurt Van Dijck:
>>>> On Thu, May 30, 2013 at 11:06:30AM +0200, Sebastian Haas wrote:
>>>>> Am 30.05.2013 10:49, schrieb Oliver Hartkopp:
>>>>>>>> On 29.05.2013 22:55, Sebastian Haas wrote:
>>>>>>>>> If I start candump this way:
>>>>>>>>> sh@helios:~/workspace/node-can$ candump vcan0,100~7ff,101~7ff
>>>>>>>>>   I want to receive any messages except 100h and 101h.
>>>>>>>>>
>>>>>>>>> When I send a message which matches the filter, it is received twice:
>>>>>>>>> sh@helios:~/workspace/node-can$ cansend vcan0 1ff#22
>>>>>>>>>    vcan0  1FF   [1]  22
>>>>>>>>>    vcan0  1FF   [1]  22
>>>>>>>>>
>>>>>>>>> When I send a message which should not received at all, it is received:
>>>>>>>>> sh@helios:~/workspace/node-can$ cansend vcan0 100#22
>>>>>>>>>    vcan0  100   [1]  22
>>>>>>>>>
>>>>>>>>> Did I misunderstood the filter here?
>>>>>>>>
>>>>>>>> The filters are independent and therefore "logical OR".
>>>>>>> Ok. That explain why received 100h even though I tried to filter it
>>>>>>> out. But
>>>>>>> why receive a message twice while it was only sent once?
>>>>>>
>>>>>> You have defined two(!) filters that let pass the CAN-ID 0x1FF.
>>>>>>
>>>>>> So what else would you expect?
>>>>> This is actually the way it meant to be? Why? Honestly this is a bug
>>>>> for me! The filter should stop when it matched and not going further
>>>>> generating ghost messages which were never sent?
>>>>
>>>> I had initially the same feeling. The problem here IMHO is
>>>> what the user expects is different from what the kernel does.
>>>>
>>>> What the user expects is a 'socket' based filter.
>>>> The kernel implements an optimised, socket-agnostic filter.
>>>> This is even more generic, but the kernel expects a CAN_RAW program
>>>> to set 'rough' filters and have the fine control in the program.
>>>>
>>>> This actually works very good, and is a clever thing to do.
>>>>
>>>> However, candump does expose this kernel behaviour to the user. The user,
>>>> as said, expects something different that what he gets.
>>>> So the user thinks it is a bug.
>>>>
>>>> How to solve this: I think the correct way is to let candump
>>>> expose the filtering that a user expects, whether in userspace
>>>> or in kernelspace.
>>>> Is such approach efficient? no. Does that matter? probably not.
>>>> candump is, IMO, not an automation tool. It's an administrator/diagnostic
>>>> tool.
>>>>
>>>> Although I have no real code in mind yet, I think the expected
>>>> filtering is much easier to implement within a single program,
>>>> than it would be in kernelspace.
>>>>
>>>> On the other hand, I have found candump's exposure of kernel work
>>>> a true friend during development.
>>>> So I'd vote to have an extra control on candump which filtering to use.
>>>>
>>>> Any thoughts?

I used the last days to check different concepts and implementation ideas how
to integrate another filter policy into the 'socket-agnostic' filters in
af_can.c Kurt wrote this nice pleading about :-)

It turned out that linked filters would become a mess in implementation and
configuration (e.g. extensive locking / performance reduction). Additionally
it would only make sense for a concatenation of inverted filters, like:

100~7FF,101~7FF,103~7FF

Finally i dropped the idea to fiddle with the really efficient filters in
af_can.c that follow the original idea of the CAN controller HW filters.

The main drawback of the independent filters is that the user does not expect
to get the same message several times due to the match of overlapping filters.

Below you will find an approach that does not push this problem into
userspace but only omits identical frames that had multiple matches.

The idea is to introduce a new socket option CAN_RAW_FILTER_UNIQUE for the
RAW socket, that catches and omits duplicate frames when it's enabled.
This is a per-socket option and is active for all configured filters applied
to this socket.

It's a relatively small and clear change and it fixes your "bug".

Here's the output of candump with and without the 'unique' option when
invoking "cansend vcan0 1FF#" in another terminal:

$ ./candump vcan0,100~7FF,101~7FF
  vcan0  1FF   [0] 
  vcan0  1FF   [0] 

$ ./candump vcan0,100~7FF,101~7FF,U
  vcan0  1FF   [0] 

I hope this fit your needs ?! :-)

Regards,
Oliver



diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
index a814062..965af9e 100644
--- a/include/uapi/linux/can/raw.h
+++ b/include/uapi/linux/can/raw.h
@@ -25,6 +25,7 @@ enum {
 	CAN_RAW_LOOPBACK,	/* local loopback (default:on)       */
 	CAN_RAW_RECV_OWN_MSGS,	/* receive my own msgs (default:off) */
 	CAN_RAW_FD_FRAMES,	/* allow CAN FD frames (default:off) */
+	CAN_RAW_FILTER_UNIQUE,	/* eliminate multiple filter matches */
 };
 
 #endif
diff --git a/net/can/raw.c b/net/can/raw.c
index 1085e65..e4233bd 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -76,6 +76,11 @@ MODULE_ALIAS("can-proto-1");
  * storing the single filter in dfilter, to avoid using dynamic memory.
  */
 
+struct unique_can_frame {
+	struct sk_buff *skb;
+	ktime_t tstamp;
+};
+
 struct raw_sock {
 	struct sock sk;
 	int bound;
@@ -84,10 +89,12 @@ struct raw_sock {
 	int loopback;
 	int recv_own_msgs;
 	int fd_frames;
+	int filter_unique;
 	int count;                 /* number of active filters */
 	struct can_filter dfilter; /* default/single filter */
 	struct can_filter *filter; /* pointer to filter(s) */
 	can_err_mask_t err_mask;
+	struct unique_can_frame uniq_cf;
 };
 
 /*
@@ -121,6 +128,16 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 	if (!ro->recv_own_msgs && oskb->sk == sk)
 		return;
 
+	/* eliminate multiple filter matches for the same skb */
+	if (ro->filter_unique) {
+		if (ro->uniq_cf.skb == oskb &&
+		    ktime_equal(ro->uniq_cf.tstamp, oskb->tstamp))
+			return;
+
+		ro->uniq_cf.skb = oskb;
+		ro->uniq_cf.tstamp = oskb->tstamp;
+	}
+
 	/* do not pass frames with DLC > 8 to a legacy socket */
 	if (!ro->fd_frames) {
 		struct canfd_frame *cfd = (struct canfd_frame *)oskb->data;
@@ -302,6 +319,8 @@ static int raw_init(struct sock *sk)
 	ro->loopback         = 1;
 	ro->recv_own_msgs    = 0;
 	ro->fd_frames        = 0;
+	ro->filter_unique    = 0;
+	ro->uniq_cf.skb      = NULL;
 
 	/* set notifier */
 	ro->notifier.notifier_call = raw_notifier;
@@ -589,6 +608,15 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 
 		break;
 
+	case CAN_RAW_FILTER_UNIQUE:
+		if (optlen != sizeof(ro->filter_unique))
+			return -EINVAL;
+
+		if (copy_from_user(&ro->filter_unique, optval, optlen))
+			return -EFAULT;
+
+		break;
+
 	default:
 		return -ENOPROTOOPT;
 	}
@@ -653,6 +681,12 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
 		val = &ro->fd_frames;
 		break;
 
+	case CAN_RAW_FILTER_UNIQUE:
+		if (len > sizeof(int))
+			len = sizeof(int);
+		val = &ro->filter_unique;
+		break;
+
 	default:
 		return -ENOPROTOOPT;
 	}


diff --git a/candump.c b/candump.c
index b489cd9..cd7bd74 100644
--- a/candump.c
+++ b/candump.c
@@ -130,6 +130,7 @@ void print_usage(char *prg)
 	fprintf(stderr, " <can_id>:<can_mask> (matches when <received_can_id> & mask == can_id & mask)\n");
 	fprintf(stderr, " <can_id>~<can_mask> (matches when <received_can_id> & mask != can_id & mask)\n");
 	fprintf(stderr, " #<error_mask>       (set error frame filter, see include/linux/can/error.h)\n");
+	fprintf(stderr, " [u|U]               (set unique mode to eliminate multiple filter matches)\n");
 	fprintf(stderr, "\nCAN IDs, masks and data content are given and expected in hexadecimal values.\n");
 	fprintf(stderr, "When can_id and can_mask are both 8 digits, they are assumed to be 29 bit EFF.\n");
 	fprintf(stderr, "Without any given filter all data frames are received ('0:0' default filter).\n");
@@ -217,6 +218,7 @@ int main(int argc, char **argv)
 	int rcvbuf_size = 0;
 	int opt, ret;
 	int currmax, numfilter;
+	int unique_filter;
 	char *ptr, *nptr;
 	struct sockaddr_can addr;
 	char ctrlmsg[CMSG_SPACE(sizeof(struct timeval)) + CMSG_SPACE(sizeof(__u32))];
@@ -466,6 +468,7 @@ int main(int argc, char **argv)
 
 			numfilter = 0;
 			err_mask = 0;
+			unique_filter = 0;
 
 			while (nptr) {
 
@@ -483,6 +486,8 @@ int main(int argc, char **argv)
  					rfilter[numfilter].can_id |= CAN_INV_FILTER;
  					rfilter[numfilter].can_mask &= ~CAN_ERR_FLAG;
 					numfilter++;
+				} else if (*ptr == 'u' || *ptr == 'U') {
+					unique_filter = 1;
 				} else if (sscanf(ptr, "#%x", &err_mask) != 1) { 
 					fprintf(stderr, "Error in filter option parsing: '%s'\n", ptr);
 					return 1;
@@ -497,6 +502,10 @@ int main(int argc, char **argv)
 				setsockopt(s[i], SOL_CAN_RAW, CAN_RAW_FILTER,
 					   rfilter, numfilter * sizeof(struct can_filter));
 
+			if (unique_filter)
+				setsockopt(s[i], SOL_CAN_RAW, CAN_RAW_FILTER_UNIQUE,
+					   &unique_filter, sizeof(unique_filter));
+
 			free(rfilter);
 
 		} /* if (nptr) */
diff --git a/include/socketcan/can/raw.h b/include/socketcan/can/raw.h
index ee683c5..1ea9461 100644
--- a/include/socketcan/can/raw.h
+++ b/include/socketcan/can/raw.h
@@ -25,6 +25,7 @@ enum {
 	CAN_RAW_LOOPBACK,	/* local loopback (default:on)       */
 	CAN_RAW_RECV_OWN_MSGS,	/* receive my own msgs (default:off) */
 	CAN_RAW_FD_FRAMES,	/* allow CAN FD frames (default:off) */
+	CAN_RAW_FILTER_UNIQUE,	/* eliminate multiple filter matches */
 };
 
 #endif

Re: RFC: (optional) software filtering in candump

[Sebastian Haas]

Hi all,

Am 01.06.2013 16:15, schrieb Oliver Hartkopp:
> Hi all,
>
> On 30.05.2013 17:37, Sebastian Haas wrote:
>> Am 30.05.2013 14:35, schrieb Kurt Van Dijck:
>>> On Thu, May 30, 2013 at 02:07:03PM +0200, Oliver Hartkopp wrote:
>>>> Am 30.05.2013 12:17, schrieb Kurt Van Dijck:
>>>>> On Thu, May 30, 2013 at 11:06:30AM +0200, Sebastian Haas wrote:
>>>>>> Am 30.05.2013 10:49, schrieb Oliver Hartkopp:
>>>>>>>>> On 29.05.2013 22:55, Sebastian Haas wrote:
>>>>>>>>>> If I start candump this way:
>>>>>>>>>> sh@helios:~/workspace/node-can$ candump vcan0,100~7ff,101~7ff
>>>>>>>>>>    I want to receive any messages except 100h and 101h.
>>>>>>>>>>
>>>>>>>>>> When I send a message which matches the filter, it is received twice:
>>>>>>>>>> sh@helios:~/workspace/node-can$ cansend vcan0 1ff#22
>>>>>>>>>>     vcan0  1FF   [1]  22
>>>>>>>>>>     vcan0  1FF   [1]  22
>>>>>>>>>>
>>>>>>>>>> When I send a message which should not received at all, it is received:
>>>>>>>>>> sh@helios:~/workspace/node-can$ cansend vcan0 100#22
>>>>>>>>>>     vcan0  100   [1]  22
>>>>>>>>>>
>>>>>>>>>> Did I misunderstood the filter here?
>>>>>>>>>
>>>>>>>>> The filters are independent and therefore "logical OR".
>>>>>>>> Ok. That explain why received 100h even though I tried to filter it
>>>>>>>> out. But
>>>>>>>> why receive a message twice while it was only sent once?
>>>>>>>
>>>>>>> You have defined two(!) filters that let pass the CAN-ID 0x1FF.
>>>>>>>
>>>>>>> So what else would you expect?
>>>>>> This is actually the way it meant to be? Why? Honestly this is a bug
>>>>>> for me! The filter should stop when it matched and not going further
>>>>>> generating ghost messages which were never sent?
>>>>>
>>>>> I had initially the same feeling. The problem here IMHO is
>>>>> what the user expects is different from what the kernel does.
>>>>>
>>>>> What the user expects is a 'socket' based filter.
>>>>> The kernel implements an optimised, socket-agnostic filter.
>>>>> This is even more generic, but the kernel expects a CAN_RAW program
>>>>> to set 'rough' filters and have the fine control in the program.
>>>>>
>>>>> This actually works very good, and is a clever thing to do.
>>>>>
>>>>> However, candump does expose this kernel behaviour to the user. The user,
>>>>> as said, expects something different that what he gets.
>>>>> So the user thinks it is a bug.
>>>>>
>>>>> How to solve this: I think the correct way is to let candump
>>>>> expose the filtering that a user expects, whether in userspace
>>>>> or in kernelspace.
>>>>> Is such approach efficient? no. Does that matter? probably not.
>>>>> candump is, IMO, not an automation tool. It's an administrator/diagnostic
>>>>> tool.
>>>>>
>>>>> Although I have no real code in mind yet, I think the expected
>>>>> filtering is much easier to implement within a single program,
>>>>> than it would be in kernelspace.
>>>>>
>>>>> On the other hand, I have found candump's exposure of kernel work
>>>>> a true friend during development.
>>>>> So I'd vote to have an extra control on candump which filtering to use.
>>>>>
>>>>> Any thoughts?
>
> I used the last days to check different concepts and implementation ideas how
> to integrate another filter policy into the 'socket-agnostic' filters in
> af_can.c Kurt wrote this nice pleading about :-)
>
> It turned out that linked filters would become a mess in implementation and
> configuration (e.g. extensive locking / performance reduction). Additionally
> it would only make sense for a concatenation of inverted filters, like:
>
> 100~7FF,101~7FF,103~7FF
>
> Finally i dropped the idea to fiddle with the really efficient filters in
> af_can.c that follow the original idea of the CAN controller HW filters.
>
> The main drawback of the independent filters is that the user does not expect
> to get the same message several times due to the match of overlapping filters.
>
> Below you will find an approach that does not push this problem into
> userspace but only omits identical frames that had multiple matches.
>
> The idea is to introduce a new socket option CAN_RAW_FILTER_UNIQUE for the
> RAW socket, that catches and omits duplicate frames when it's enabled.
> This is a per-socket option and is active for all configured filters applied
> to this socket.
>
> It's a relatively small and clear change and it fixes your "bug".
>
> Here's the output of candump with and without the 'unique' option when
> invoking "cansend vcan0 1FF#" in another terminal:
>
> $ ./candump vcan0,100~7FF,101~7FF
>    vcan0  1FF   [0]
>    vcan0  1FF   [0]
>
> $ ./candump vcan0,100~7FF,101~7FF,U
>    vcan0  1FF   [0]
>
> I hope this fit your needs ?! :-)
Yes it does. It may not be a "clean" solution, but it fixes the issue.

Sebastian

RFC SFF bitfield filter - was Re: RFC: (optional) software filtering in candump

[Oliver Hartkopp]

> Am 01.06.2013 16:15, schrieb Oliver Hartkopp:

>> The idea is to introduce a new socket option CAN_RAW_FILTER_UNIQUE for the
>> RAW socket, that catches and omits duplicate frames when it's enabled.
>> This is a per-socket option and is active for all configured filters applied
>> to this socket.
>>
>> It's a relatively small and clear change and it fixes your "bug".
>>
>> Here's the output of candump with and without the 'unique' option when
>> invoking "cansend vcan0 1FF#" in another terminal:
>>
>> $ ./candump vcan0,100~7FF,101~7FF
>>    vcan0  1FF   [0]
>>    vcan0  1FF   [0]
>>
>> $ ./candump vcan0,100~7FF,101~7FF,U
>>    vcan0  1FF   [0]
>>
>> I hope this fit your needs ?! :-)
> Yes it does. It may not be a "clean" solution, but it fixes the issue.
> 

Yes. It is a bit of a fix that handles overlapping filters but is does not
provide a logical AND operation on a bigger number of inverted filters.

E.g. i wanted to remove some single SFF CAN IDs in the 3xx and 5xx range,
which lead to something like this:

candump vcan0,100~C0000100,500:C0000500,333:C00007FF, ...

100~C0000100 -> NOT 1XX 3XX 5XX 7XX SFF frames
500:C0000500 -> add 5XX 7XX again
333:C00007FF, ... -> add single IDs in the 1XX 3XX range

It was a mess. The current filters are not optimized for complex requirements.

Even if it only would be usable for the SFF CAN IDs 0 .. 2048, i wonder if it
would help to configure a 2048 bit bitfield for a 'really individual' filter.

The CAN_RAW socket would get a new sockopt which sets a 128 byte array
(2048/8) to specify for each single CAN ID if it can pass the filter:

u8 bfield[128] /* 2048 bit */

Check at CAN frame reception:

if (can_id & FFFFF800 == 0) { /* no EFF / RTR / ERR and max 0x7FF */

    if ( bfield[can_id >> 3] & 1 << (can_id & 7) ) {

        /* can_id matched */

    }
}

What is your opinion?

Does this kind of individual bitfield filter help for your use-cases?

Regards,
Oliver

Re: RFC SFF bitfield filter - was Re: RFC: (optional) software filtering in candump

[Kurt Van Dijck]

On Sun, Jun 02, 2013 at 11:59:14AM +0200, Oliver Hartkopp wrote:
> 
> > Am 01.06.2013 16:15, schrieb Oliver Hartkopp:
> 
> >> The idea is to introduce a new socket option CAN_RAW_FILTER_UNIQUE for the
> >> RAW socket, that catches and omits duplicate frames when it's enabled.
> >> This is a per-socket option and is active for all configured filters applied
> >> to this socket.
> >>
> >> It's a relatively small and clear change and it fixes your "bug".
> >>
> >> Here's the output of candump with and without the 'unique' option when
> >> invoking "cansend vcan0 1FF#" in another terminal:
> >>
> >> $ ./candump vcan0,100~7FF,101~7FF
> >>    vcan0  1FF   [0]
> >>    vcan0  1FF   [0]
> >>
> >> $ ./candump vcan0,100~7FF,101~7FF,U
> >>    vcan0  1FF   [0]
> >>
> >> I hope this fit your needs ?! :-)
> > Yes it does. It may not be a "clean" solution, but it fixes the issue.
> > 
> 
> Yes. It is a bit of a fix that handles overlapping filters but is does not
> provide a logical AND operation on a bigger number of inverted filters.
> 
> E.g. i wanted to remove some single SFF CAN IDs in the 3xx and 5xx range,
> which lead to something like this:
> 
> candump vcan0,100~C0000100,500:C0000500,333:C00007FF, ...
> 
> 100~C0000100 -> NOT 1XX 3XX 5XX 7XX SFF frames
> 500:C0000500 -> add 5XX 7XX again
> 333:C00007FF, ... -> add single IDs in the 1XX 3XX range
> 
> It was a mess. The current filters are not optimized for complex requirements.
> 
> Even if it only would be usable for the SFF CAN IDs 0 .. 2048, i wonder if it
> would help to configure a 2048 bit bitfield for a 'really individual' filter.
> 
> The CAN_RAW socket would get a new sockopt which sets a 128 byte array
> (2048/8) to specify for each single CAN ID if it can pass the filter:
> 
> u8 bfield[128] /* 2048 bit */
> 
> Check at CAN frame reception:
> 
> if (can_id & FFFFF800 == 0) { /* no EFF / RTR / ERR and max 0x7FF */
> 
>     if ( bfield[can_id >> 3] & 1 << (can_id & 7) ) {
> 
>         /* can_id matched */
> 
>     }
> }
> 
> What is your opinion?
> 
> Does this kind of individual bitfield filter help for your use-cases?

This works for 11bit, but makes 29bit behave differently.
Therefore, I think it's not such good idea (yet).

Kind regards,
Kurt

Re: RFC: (optional) software filtering in candump

[Kurt Van Dijck]

Oliver

Nice work. It is indeed a workaround. Still some tiny small remark:

Regardless of the defaults in kernel,
I think the default of candump should be to have unique CAN frames.
Why?
1) candump is to be used by 'less advanced users' also :-)
   Those do not understand why to turn 'unique filtering' on.
2) Advanced users understand the filtering and understand
   when to turn unique filtering off.

Kind regards,
Kurt

> 
> It turned out that linked filters would become a mess in implementation and
> configuration (e.g. extensive locking / performance reduction). Additionally
> it would only make sense for a concatenation of inverted filters, like:
> 
> 100~7FF,101~7FF,103~7FF
> 
> Finally i dropped the idea to fiddle with the really efficient filters in
> af_can.c that follow the original idea of the CAN controller HW filters.
> 
> The main drawback of the independent filters is that the user does not expect
> to get the same message several times due to the match of overlapping filters.
> 
> Below you will find an approach that does not push this problem into
> userspace but only omits identical frames that had multiple matches.
> 
> The idea is to introduce a new socket option CAN_RAW_FILTER_UNIQUE for the
> RAW socket, that catches and omits duplicate frames when it's enabled.
> This is a per-socket option and is active for all configured filters applied
> to this socket.
> 
> It's a relatively small and clear change and it fixes your "bug".
> 
> Here's the output of candump with and without the 'unique' option when
> invoking "cansend vcan0 1FF#" in another terminal:
> 
> $ ./candump vcan0,100~7FF,101~7FF
>   vcan0  1FF   [0] 
>   vcan0  1FF   [0] 
> 
> $ ./candump vcan0,100~7FF,101~7FF,U
>   vcan0  1FF   [0] 
> 
> I hope this fit your needs ?! :-)
> 
> Regards,
> Oliver
> 
> 
> 
> diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
> index a814062..965af9e 100644
> --- a/include/uapi/linux/can/raw.h
> +++ b/include/uapi/linux/can/raw.h
> @@ -25,6 +25,7 @@ enum {
>  	CAN_RAW_LOOPBACK,	/* local loopback (default:on)       */
>  	CAN_RAW_RECV_OWN_MSGS,	/* receive my own msgs (default:off) */
>  	CAN_RAW_FD_FRAMES,	/* allow CAN FD frames (default:off) */
> +	CAN_RAW_FILTER_UNIQUE,	/* eliminate multiple filter matches */
>  };
>  
>  #endif
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 1085e65..e4233bd 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -76,6 +76,11 @@ MODULE_ALIAS("can-proto-1");
>   * storing the single filter in dfilter, to avoid using dynamic memory.
>   */
>  
> +struct unique_can_frame {
> +	struct sk_buff *skb;
> +	ktime_t tstamp;
> +};
> +
>  struct raw_sock {
>  	struct sock sk;
>  	int bound;
> @@ -84,10 +89,12 @@ struct raw_sock {
>  	int loopback;
>  	int recv_own_msgs;
>  	int fd_frames;
> +	int filter_unique;
>  	int count;                 /* number of active filters */
>  	struct can_filter dfilter; /* default/single filter */
>  	struct can_filter *filter; /* pointer to filter(s) */
>  	can_err_mask_t err_mask;
> +	struct unique_can_frame uniq_cf;
>  };
>  
>  /*
> @@ -121,6 +128,16 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
>  	if (!ro->recv_own_msgs && oskb->sk == sk)
>  		return;
>  
> +	/* eliminate multiple filter matches for the same skb */
> +	if (ro->filter_unique) {
> +		if (ro->uniq_cf.skb == oskb &&
> +		    ktime_equal(ro->uniq_cf.tstamp, oskb->tstamp))
> +			return;
> +
> +		ro->uniq_cf.skb = oskb;
> +		ro->uniq_cf.tstamp = oskb->tstamp;
> +	}
> +
>  	/* do not pass frames with DLC > 8 to a legacy socket */
>  	if (!ro->fd_frames) {
>  		struct canfd_frame *cfd = (struct canfd_frame *)oskb->data;
> @@ -302,6 +319,8 @@ static int raw_init(struct sock *sk)
>  	ro->loopback         = 1;
>  	ro->recv_own_msgs    = 0;
>  	ro->fd_frames        = 0;
> +	ro->filter_unique    = 0;
> +	ro->uniq_cf.skb      = NULL;
>  
>  	/* set notifier */
>  	ro->notifier.notifier_call = raw_notifier;
> @@ -589,6 +608,15 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>  
>  		break;
>  
> +	case CAN_RAW_FILTER_UNIQUE:
> +		if (optlen != sizeof(ro->filter_unique))
> +			return -EINVAL;
> +
> +		if (copy_from_user(&ro->filter_unique, optval, optlen))
> +			return -EFAULT;
> +
> +		break;
> +
>  	default:
>  		return -ENOPROTOOPT;
>  	}
> @@ -653,6 +681,12 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
>  		val = &ro->fd_frames;
>  		break;
>  
> +	case CAN_RAW_FILTER_UNIQUE:
> +		if (len > sizeof(int))
> +			len = sizeof(int);
> +		val = &ro->filter_unique;
> +		break;
> +
>  	default:
>  		return -ENOPROTOOPT;
>  	}
> 
> 
> diff --git a/candump.c b/candump.c
> index b489cd9..cd7bd74 100644
> --- a/candump.c
> +++ b/candump.c
> @@ -130,6 +130,7 @@ void print_usage(char *prg)
>  	fprintf(stderr, " <can_id>:<can_mask> (matches when <received_can_id> & mask == can_id & mask)\n");
>  	fprintf(stderr, " <can_id>~<can_mask> (matches when <received_can_id> & mask != can_id & mask)\n");
>  	fprintf(stderr, " #<error_mask>       (set error frame filter, see include/linux/can/error.h)\n");
> +	fprintf(stderr, " [u|U]               (set unique mode to eliminate multiple filter matches)\n");
>  	fprintf(stderr, "\nCAN IDs, masks and data content are given and expected in hexadecimal values.\n");
>  	fprintf(stderr, "When can_id and can_mask are both 8 digits, they are assumed to be 29 bit EFF.\n");
>  	fprintf(stderr, "Without any given filter all data frames are received ('0:0' default filter).\n");
> @@ -217,6 +218,7 @@ int main(int argc, char **argv)
>  	int rcvbuf_size = 0;
>  	int opt, ret;
>  	int currmax, numfilter;
> +	int unique_filter;
>  	char *ptr, *nptr;
>  	struct sockaddr_can addr;
>  	char ctrlmsg[CMSG_SPACE(sizeof(struct timeval)) + CMSG_SPACE(sizeof(__u32))];
> @@ -466,6 +468,7 @@ int main(int argc, char **argv)
>  
>  			numfilter = 0;
>  			err_mask = 0;
> +			unique_filter = 0;

Re: RFC SFF bitfield filter - was Re: RFC: (optional) software filtering in candump

[Sebastian Haas]

Am 02.06.2013 13:17, schrieb Kurt Van Dijck:
> On Sun, Jun 02, 2013 at 11:59:14AM +0200, Oliver Hartkopp wrote:
>>
>>> Am 01.06.2013 16:15, schrieb Oliver Hartkopp:
>>
>>>> The idea is to introduce a new socket option CAN_RAW_FILTER_UNIQUE for the
>>>> RAW socket, that catches and omits duplicate frames when it's enabled.
>>>> This is a per-socket option and is active for all configured filters applied
>>>> to this socket.
>>>>
>>>> It's a relatively small and clear change and it fixes your "bug".
>>>>
>>>> Here's the output of candump with and without the 'unique' option when
>>>> invoking "cansend vcan0 1FF#" in another terminal:
>>>>
>>>> $ ./candump vcan0,100~7FF,101~7FF
>>>>     vcan0  1FF   [0]
>>>>     vcan0  1FF   [0]
>>>>
>>>> $ ./candump vcan0,100~7FF,101~7FF,U
>>>>     vcan0  1FF   [0]
>>>>
>>>> I hope this fit your needs ?! :-)
>>> Yes it does. It may not be a "clean" solution, but it fixes the issue.
>>>
>>
>> Yes. It is a bit of a fix that handles overlapping filters but is does not
>> provide a logical AND operation on a bigger number of inverted filters.
>>
>> E.g. i wanted to remove some single SFF CAN IDs in the 3xx and 5xx range,
>> which lead to something like this:
>>
>> candump vcan0,100~C0000100,500:C0000500,333:C00007FF, ...
>>
>> 100~C0000100 -> NOT 1XX 3XX 5XX 7XX SFF frames
>> 500:C0000500 -> add 5XX 7XX again
>> 333:C00007FF, ... -> add single IDs in the 1XX 3XX range
>>
>> It was a mess. The current filters are not optimized for complex requirements.
>>
>> Even if it only would be usable for the SFF CAN IDs 0 .. 2048, i wonder if it
>> would help to configure a 2048 bit bitfield for a 'really individual' filter.
>>
>> The CAN_RAW socket would get a new sockopt which sets a 128 byte array
>> (2048/8) to specify for each single CAN ID if it can pass the filter:
>>
>> u8 bfield[128] /* 2048 bit */
>>
>> Check at CAN frame reception:
>>
>> if (can_id & FFFFF800 == 0) { /* no EFF / RTR / ERR and max 0x7FF */
>>
>>      if ( bfield[can_id >> 3] & 1 << (can_id & 7) ) {
>>
>>          /* can_id matched */
>>
>>      }
>> }
>>
>> What is your opinion?
>>
>> Does this kind of individual bitfield filter help for your use-cases?
>
> This works for 11bit, but makes 29bit behave differently.
> Therefore, I think it's not such good idea (yet).
I agree. This is a nice approach, but limited to SFF.

I think we "just" need to find a good algorithm to process a set of 
filters. The guys of the IP paket filter doing more complex stuff and 
they need to process much higher datarates then 1MBit ;-)

I will try to understand how the current filter system is implemented, 
maybe I can make more substanial comments on that.

Sebastian

AW: RFC: (optional) software filtering in candump

[Sandro Anders | CarMedialab]

Hi all,

your first approach fix the problem I have. But I can work with the current implementation as I now understand the mechanism behind. The biggest problem is that the documentation implies an linked filter list not an vector of individual filter elements. This should be changed to avoid such misunderstanding. 
@Oliver: According to your bitfield idea I have to agree with Kurt it will not handle extended frames which getting more important. 

Thanks and best regards,
Sandro

> -----Ursprüngliche Nachricht-----
> Von: Kurt Van Dijck [mailto:kurt.van.dijck@eia.be]
> Gesendet: Sonntag, 2. Juni 2013 13:23
> An: Oliver Hartkopp
> Cc: Sebastian Haas; Sandro Anders | CarMedialab; linux-can Mailing List
> Betreff: Re: RFC: (optional) software filtering in candump
> 
> Oliver
> 
> Nice work. It is indeed a workaround. Still some tiny small remark:
> 
> Regardless of the defaults in kernel,
> I think the default of candump should be to have unique CAN frames.
> Why?
> 1) candump is to be used by 'less advanced users' also :-)
>    Those do not understand why to turn 'unique filtering' on.
> 2) Advanced users understand the filtering and understand
>    when to turn unique filtering off.
> 
> Kind regards,
> Kurt
> 
> >
> > It turned out that linked filters would become a mess in implementation
> and
> > configuration (e.g. extensive locking / performance reduction).
> Additionally
> > it would only make sense for a concatenation of inverted filters, like:
> >
> > 100~7FF,101~7FF,103~7FF
> >
> > Finally i dropped the idea to fiddle with the really efficient filters
> in
> > af_can.c that follow the original idea of the CAN controller HW filters.
> >
> > The main drawback of the independent filters is that the user does not
> expect
> > to get the same message several times due to the match of overlapping
> filters.
> >
> > Below you will find an approach that does not push this problem into
> > userspace but only omits identical frames that had multiple matches.
> >
> > The idea is to introduce a new socket option CAN_RAW_FILTER_UNIQUE for
> the
> > RAW socket, that catches and omits duplicate frames when it's enabled.
> > This is a per-socket option and is active for all configured filters
> applied
> > to this socket.
> >
> > It's a relatively small and clear change and it fixes your "bug".
> >
> > Here's the output of candump with and without the 'unique' option when
> > invoking "cansend vcan0 1FF#" in another terminal:
> >
> > $ ./candump vcan0,100~7FF,101~7FF
> >   vcan0  1FF   [0]
> >   vcan0  1FF   [0]
> >
> > $ ./candump vcan0,100~7FF,101~7FF,U
> >   vcan0  1FF   [0]
> >
> > I hope this fit your needs ?! :-)
> >
> > Regards,
> > Oliver
> >
> >
> >
> > diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
> > index a814062..965af9e 100644
> > --- a/include/uapi/linux/can/raw.h
> > +++ b/include/uapi/linux/can/raw.h
> > @@ -25,6 +25,7 @@ enum {
> >  	CAN_RAW_LOOPBACK,	/* local loopback (default:on)       */
> >  	CAN_RAW_RECV_OWN_MSGS,	/* receive my own msgs (default:off) */
> >  	CAN_RAW_FD_FRAMES,	/* allow CAN FD frames (default:off) */
> > +	CAN_RAW_FILTER_UNIQUE,	/* eliminate multiple filter matches */
> >  };
> >
> >  #endif
> > diff --git a/net/can/raw.c b/net/can/raw.c
> > index 1085e65..e4233bd 100644
> > --- a/net/can/raw.c
> > +++ b/net/can/raw.c
> > @@ -76,6 +76,11 @@ MODULE_ALIAS("can-proto-1");
> >   * storing the single filter in dfilter, to avoid using dynamic memory.
> >   */
> >
> > +struct unique_can_frame {
> > +	struct sk_buff *skb;
> > +	ktime_t tstamp;
> > +};
> > +
> >  struct raw_sock {
> >  	struct sock sk;
> >  	int bound;
> > @@ -84,10 +89,12 @@ struct raw_sock {
> >  	int loopback;
> >  	int recv_own_msgs;
> >  	int fd_frames;
> > +	int filter_unique;
> >  	int count;                 /* number of active filters */
> >  	struct can_filter dfilter; /* default/single filter */
> >  	struct can_filter *filter; /* pointer to filter(s) */
> >  	can_err_mask_t err_mask;
> > +	struct unique_can_frame uniq_cf;
> >  };
> >
> >  /*
> > @@ -121,6 +128,16 @@ static void raw_rcv(struct sk_buff *oskb, void
> *data)
> >  	if (!ro->recv_own_msgs && oskb->sk == sk)
> >  		return;
> >
> > +	/* eliminate multiple filter matches for the same skb */
> > +	if (ro->filter_unique) {
> > +		if (ro->uniq_cf.skb == oskb &&
> > +		    ktime_equal(ro->uniq_cf.tstamp, oskb->tstamp))
> > +			return;
> > +
> > +		ro->uniq_cf.skb = oskb;
> > +		ro->uniq_cf.tstamp = oskb->tstamp;
> > +	}
> > +
> >  	/* do not pass frames with DLC > 8 to a legacy socket */
> >  	if (!ro->fd_frames) {
> >  		struct canfd_frame *cfd = (struct canfd_frame *)oskb->data;
> > @@ -302,6 +319,8 @@ static int raw_init(struct sock *sk)
> >  	ro->loopback         = 1;
> >  	ro->recv_own_msgs    = 0;
> >  	ro->fd_frames        = 0;
> > +	ro->filter_unique    = 0;
> > +	ro->uniq_cf.skb      = NULL;
> >
> >  	/* set notifier */
> >  	ro->notifier.notifier_call = raw_notifier;
> > @@ -589,6 +608,15 @@ static int raw_setsockopt(struct socket *sock, int
> level, int optname,
> >
> >  		break;
> >
> > +	case CAN_RAW_FILTER_UNIQUE:
> > +		if (optlen != sizeof(ro->filter_unique))
> > +			return -EINVAL;
> > +
> > +		if (copy_from_user(&ro->filter_unique, optval, optlen))
> > +			return -EFAULT;
> > +
> > +		break;
> > +
> >  	default:
> >  		return -ENOPROTOOPT;
> >  	}
> > @@ -653,6 +681,12 @@ static int raw_getsockopt(struct socket *sock, int
> level, int optname,
> >  		val = &ro->fd_frames;
> >  		break;
> >
> > +	case CAN_RAW_FILTER_UNIQUE:
> > +		if (len > sizeof(int))
> > +			len = sizeof(int);
> > +		val = &ro->filter_unique;
> > +		break;
> > +
> >  	default:
> >  		return -ENOPROTOOPT;
> >  	}
> >
> >
> > diff --git a/candump.c b/candump.c
> > index b489cd9..cd7bd74 100644
> > --- a/candump.c
> > +++ b/candump.c
> > @@ -130,6 +130,7 @@ void print_usage(char *prg)
> >  	fprintf(stderr, " <can_id>:<can_mask> (matches when
> <received_can_id> & mask == can_id & mask)\n");
> >  	fprintf(stderr, " <can_id>~<can_mask> (matches when
> <received_can_id> & mask != can_id & mask)\n");
> >  	fprintf(stderr, " #<error_mask>       (set error frame filter, see
> include/linux/can/error.h)\n");
> > +	fprintf(stderr, " [u|U]               (set unique mode to eliminate
> multiple filter matches)\n");
> >  	fprintf(stderr, "\nCAN IDs, masks and data content are given and
> expected in hexadecimal values.\n");
> >  	fprintf(stderr, "When can_id and can_mask are both 8 digits, they
> are assumed to be 29 bit EFF.\n");
> >  	fprintf(stderr, "Without any given filter all data frames are
> received ('0:0' default filter).\n");
> > @@ -217,6 +218,7 @@ int main(int argc, char **argv)
> >  	int rcvbuf_size = 0;
> >  	int opt, ret;
> >  	int currmax, numfilter;
> > +	int unique_filter;
> >  	char *ptr, *nptr;
> >  	struct sockaddr_can addr;
> >  	char ctrlmsg[CMSG_SPACE(sizeof(struct timeval)) +
> CMSG_SPACE(sizeof(__u32))];
> > @@ -466,6 +468,7 @@ int main(int argc, char **argv)
> >
> >  			numfilter = 0;
> >  			err_mask = 0;
> > +			unique_filter = 0;

Re: RFC: (optional) software filtering in candump

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 5233 bytes --]

On 06/01/2013 04:15 PM, Oliver Hartkopp wrote:
> Hi all,
> 
> On 30.05.2013 17:37, Sebastian Haas wrote:
>> Am 30.05.2013 14:35, schrieb Kurt Van Dijck:
>>> On Thu, May 30, 2013 at 02:07:03PM +0200, Oliver Hartkopp wrote:
>>>> Am 30.05.2013 12:17, schrieb Kurt Van Dijck:
>>>>> On Thu, May 30, 2013 at 11:06:30AM +0200, Sebastian Haas wrote:
>>>>>> Am 30.05.2013 10:49, schrieb Oliver Hartkopp:
>>>>>>>>> On 29.05.2013 22:55, Sebastian Haas wrote:
>>>>>>>>>> If I start candump this way:
>>>>>>>>>> sh@helios:~/workspace/node-can$ candump vcan0,100~7ff,101~7ff
>>>>>>>>>>   I want to receive any messages except 100h and 101h.
>>>>>>>>>>
>>>>>>>>>> When I send a message which matches the filter, it is received twice:
>>>>>>>>>> sh@helios:~/workspace/node-can$ cansend vcan0 1ff#22
>>>>>>>>>>    vcan0  1FF   [1]  22
>>>>>>>>>>    vcan0  1FF   [1]  22
>>>>>>>>>>
>>>>>>>>>> When I send a message which should not received at all, it is received:
>>>>>>>>>> sh@helios:~/workspace/node-can$ cansend vcan0 100#22
>>>>>>>>>>    vcan0  100   [1]  22
>>>>>>>>>>
>>>>>>>>>> Did I misunderstood the filter here?
>>>>>>>>>
>>>>>>>>> The filters are independent and therefore "logical OR".
>>>>>>>> Ok. That explain why received 100h even though I tried to filter it
>>>>>>>> out. But
>>>>>>>> why receive a message twice while it was only sent once?
>>>>>>>
>>>>>>> You have defined two(!) filters that let pass the CAN-ID 0x1FF.
>>>>>>>
>>>>>>> So what else would you expect?
>>>>>> This is actually the way it meant to be? Why? Honestly this is a bug
>>>>>> for me! The filter should stop when it matched and not going further
>>>>>> generating ghost messages which were never sent?
>>>>>
>>>>> I had initially the same feeling. The problem here IMHO is
>>>>> what the user expects is different from what the kernel does.
>>>>>
>>>>> What the user expects is a 'socket' based filter.
>>>>> The kernel implements an optimised, socket-agnostic filter.
>>>>> This is even more generic, but the kernel expects a CAN_RAW program
>>>>> to set 'rough' filters and have the fine control in the program.
>>>>>
>>>>> This actually works very good, and is a clever thing to do.
>>>>>
>>>>> However, candump does expose this kernel behaviour to the user. The user,
>>>>> as said, expects something different that what he gets.
>>>>> So the user thinks it is a bug.
>>>>>
>>>>> How to solve this: I think the correct way is to let candump
>>>>> expose the filtering that a user expects, whether in userspace
>>>>> or in kernelspace.
>>>>> Is such approach efficient? no. Does that matter? probably not.
>>>>> candump is, IMO, not an automation tool. It's an administrator/diagnostic
>>>>> tool.
>>>>>
>>>>> Although I have no real code in mind yet, I think the expected
>>>>> filtering is much easier to implement within a single program,
>>>>> than it would be in kernelspace.
>>>>>
>>>>> On the other hand, I have found candump's exposure of kernel work
>>>>> a true friend during development.
>>>>> So I'd vote to have an extra control on candump which filtering to use.
>>>>>
>>>>> Any thoughts?
> 
> I used the last days to check different concepts and implementation ideas how
> to integrate another filter policy into the 'socket-agnostic' filters in
> af_can.c Kurt wrote this nice pleading about :-)
> 
> It turned out that linked filters would become a mess in implementation and
> configuration (e.g. extensive locking / performance reduction). Additionally
> it would only make sense for a concatenation of inverted filters, like:
> 
> 100~7FF,101~7FF,103~7FF
> 
> Finally i dropped the idea to fiddle with the really efficient filters in
> af_can.c that follow the original idea of the CAN controller HW filters.
> 
> The main drawback of the independent filters is that the user does not expect
> to get the same message several times due to the match of overlapping filters.
> 
> Below you will find an approach that does not push this problem into
> userspace but only omits identical frames that had multiple matches.
> 
> The idea is to introduce a new socket option CAN_RAW_FILTER_UNIQUE for the
> RAW socket, that catches and omits duplicate frames when it's enabled.
> This is a per-socket option and is active for all configured filters applied
> to this socket.
> 
> It's a relatively small and clear change and it fixes your "bug".
> 
> Here's the output of candump with and without the 'unique' option when
> invoking "cansend vcan0 1FF#" in another terminal:
> 
> $ ./candump vcan0,100~7FF,101~7FF
>   vcan0  1FF   [0] 
>   vcan0  1FF   [0] 
> 
> $ ./candump vcan0,100~7FF,101~7FF,U
>   vcan0  1FF   [0] 
> 
> I hope this fit your needs ?! :-)

Sorry for resurrecting this old thread, but a $CUSTOMER stumbled over
the same "issue". So there seems to be realworld need for a solution :)

Are there any new thoughts on the problem?

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: 819 bytes --]

Re: RFC: (optional) software filtering in candump

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]

On 06/01/2013 04:15 PM, Oliver Hartkopp wrote:
> I hope this fit your needs ?! :-)

Can I have your S-o-b for this patch?

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: 819 bytes --]

Re: RFC: (optional) software filtering in candump

[Oliver Hartkopp]

On 17.03.2015 12:34, Marc Kleine-Budde wrote:
> On 06/01/2013 04:15 PM, Oliver Hartkopp wrote:
>> I hope this fit your needs ?! :-)
>
> Can I have your S-o-b for this patch?

Oh. I've just seen, that I already provided a patch that time.

I'll check it once more and will give you feedback soon.

Regards,
Oliver

Re: RFC: (optional) software filtering in candump

[Oliver Hartkopp]

On 17.03.2015 13:04, Oliver Hartkopp wrote:
> On 17.03.2015 12:34, Marc Kleine-Budde wrote:
>> On 06/01/2013 04:15 PM, Oliver Hartkopp wrote:
>>> I hope this fit your needs ?! :-)
>>
>> Can I have your S-o-b for this patch?
>
> Oh. I've just seen, that I already provided a patch that time.
>
> I'll check it once more and will give you feedback soon.
>

I rebased the patch, added a comment, a ktime_set(0,0) initialization and 
wrote a proper commit message.

Does it fit your expectations?

Regards,
Oliver

Re: RFC: (optional) software filtering in candump

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

On 03/17/2015 02:02 PM, Oliver Hartkopp wrote:
> On 17.03.2015 13:04, Oliver Hartkopp wrote:
>> On 17.03.2015 12:34, Marc Kleine-Budde wrote:
>>> On 06/01/2013 04:15 PM, Oliver Hartkopp wrote:
>>>> I hope this fit your needs ?! :-)
>>>
>>> Can I have your S-o-b for this patch?
>>
>> Oh. I've just seen, that I already provided a patch that time.
>>
>> I'll check it once more and will give you feedback soon.
>>
> 
> I rebased the patch, added a comment, a ktime_set(0,0) initialization and 
> wrote a proper commit message.
> 
> Does it fit your expectations?

Thanks a lot, I hope so :)

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: 819 bytes --]

Re: RFC: (optional) software filtering in candump

[Oliver Hartkopp]

>> Additional a hardware timestamp can be retrieved from the hardware and passed
>> to the userspace. (HW timestamps)
> 
> Thanks Oliver for the explanation. It's clean now.
> Shouldn't hardware timestamping option be implemented like PTP?
> http://lxr.linux.no/linux+v3.9.6/net/core/timestamping.c#L104  (I don't need
> it but I couldn't find HW timestamp code for SJA1000 or MCP2515)

Yes, that's missing.

AFAIK only the PCAN USB (Pro) supports to pass the hardware timestamps to the
userspace. But when i took a quick look into the output, i wasn't able to
retrieve the HW timestamps.

I wrote a fix for the driver:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/net/can/usb/peak_usb/pcan_usb.c?id=c9faaa09e2a1335678f09c70a0d0eda095564bab

But i didn't manage to verify the HW timestamp output.

It's on my list of things i wanted to investigate when i'm bored ;-)

Regards,
Oliver

Re: RFC: (optional) software filtering in candump

[Janusz Uzycki]

> Did you see my patch to remove duplicates by matching the CAN-ID and the
> timestamp for the raw socket?

I read comments about the patch. Now I see the code and timestamp matching 
:)
Before I thought that each CAN frame is stamped by frame's circular id 
(in/after a queue). Then the filter would be even simpler (id filtering 
instead of can_id and timestamp).

>> The problem
>> could be a lot of frames in short time. Then additional field in 
>> can_frame
>> struct with packet_id in queue would be useful.
>
> No. For two reasons:
>
> 1. The struct can_frame contains the content of a CAN frame, which is
>   - a CAN Identifier
>   - a data length
>   - CAN payload data
>
>   all other stuff is not really CAN specific.

It is not CAN frame data, right, but important information about frame and 
the best timestamp could be used (eg. HW if available).

> 2. The struct can_frame is set in stone with Linux 2.6.25. Any change 
> breaks
>   the binary API

If the field member is added at the end of structure it is extention not 
breaker. On the other hand kernel has well defined API for timestamping.

>> The timestamp could be also
>> added in the same way. How SIOCGSTAMP works now?
>
> Use ioctl(s, SIOCGSTAMP, &tv) after read/recvmsg:
>
> See e.g. for a read() syscall
> https://gitorious.org/linux-can/can-utils/blobs/master/isotpdump.c#line214
>
> See e.g. for a recvmsg() syscall
> https://gitorious.org/linux-can/can-utils/blobs/master/candump.c#line663
>
>> What is difference in
>> SO_TIMESTAMP?
>
> It *is* SO_TIMESTAMP. The standard established programming interface in 
> POSIX
> systems. Therefore puting the timestamp into struct can_frame would have 
> been
> a mistake.
>
>
>> Where in kernel (socket layer or low level CAN driver, eq. irq)
>> the timestamp is generated?
>
> See netif_receive_skb() (softirq context)
> http://lxr.linux.no/#linux+v3.9.6/net/core/dev.c#L3547
>
> Or netif_rx() (irq context)
> http://lxr.linux.no/#linux+v3.9.6/net/core/dev.c#L3116
>
> These call net_timestamp_check() at
> http://lxr.linux.no/#linux+v3.9.6/net/core/dev.c#L1571
>
> which sets the timestamp if enabled and needed.
>
> So it's at irq level (e.g. for SJA1000 driver)
>
> Additional a hardware timestamp can be retrieved from the hardware and 
> passed
> to the userspace. (HW timestamps)

Thanks Oliver for the explanation. It's clean now.
Shouldn't hardware timestamping option be implemented like PTP? 
http://lxr.linux.no/linux+v3.9.6/net/core/timestamping.c#L104  (I don't need 
it but I couldn't find HW timestamp code for SJA1000 or MCP2515)

best regards
Janusz

Re: RFC: (optional) software filtering in candump

[Oliver Hartkopp]

On 14.06.2013 10:29, Janusz Uzycki wrote:
> Hi.
>  
> I read the discussion. I agree the documentation should better explain
> multifilter filter policy. I think good workaround could be duplicated
> timestamp filtering - it seems simple to realize in userspace.

Did you see my patch to remove duplicates by matching the CAN-ID and the
timestamp for the raw socket?

> The problem
> could be a lot of frames in short time. Then additional field in can_frame
> struct with packet_id in queue would be useful.

No. For two reasons:

1. The struct can_frame contains the content of a CAN frame, which is
   - a CAN Identifier
   - a data length
   - CAN payload data

   all other stuff is not really CAN specific.

2. The struct can_frame is set in stone with Linux 2.6.25. Any change breaks
   the binary API

> The timestamp could be also
> added in the same way. How SIOCGSTAMP works now?

Use ioctl(s, SIOCGSTAMP, &tv) after read/recvmsg:

See e.g. for a read() syscall
https://gitorious.org/linux-can/can-utils/blobs/master/isotpdump.c#line214

See e.g. for a recvmsg() syscall
https://gitorious.org/linux-can/can-utils/blobs/master/candump.c#line663

> What is difference in
> SO_TIMESTAMP? 

It *is* SO_TIMESTAMP. The standard established programming interface in POSIX
systems. Therefore puting the timestamp into struct can_frame would have been
a mistake.


> Where in kernel (socket layer or low level CAN driver, eq. irq)
> the timestamp is generated?

See netif_receive_skb() (softirq context)
http://lxr.linux.no/#linux+v3.9.6/net/core/dev.c#L3547

Or netif_rx() (irq context)
http://lxr.linux.no/#linux+v3.9.6/net/core/dev.c#L3116

These call net_timestamp_check() at
http://lxr.linux.no/#linux+v3.9.6/net/core/dev.c#L1571

which sets the timestamp if enabled and needed.

So it's at irq level (e.g. for SJA1000 driver)

Additional a hardware timestamp can be retrieved from the hardware and passed
to the userspace. (HW timestamps)

Best regards,
Oliver

Re: RFC: (optional) software filtering in candump

[Janusz Uzycki]

Hi.

I read the discussion. I agree the documentation should better explain 
multifilter filter policy. I think good workaround could be duplicated 
timestamp filtering - it seems simple to realize in userspace. The problem 
could be a lot of frames in short time. Then additional field in can_frame 
struct with packet_id in queue would be useful. The timestamp could be also 
added in the same way. How SIOCGSTAMP works now? What is difference in 
SO_TIMESTAMP? Where in kernel (socket layer or low level CAN driver, eq. 
irq) the timestamp is generated?

best regards
Janusz Uzycki

(text mode now)