Re: [PATCH] can: add new socket option CAN_RAW_FILTER_UNIQUE to eliminate duplicate matches

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hi Marc,

I know why I thought it will get messy ...

E.g. when you notch three CAN IDs like this:

candump can0,100~7FF,200~7FF,400~7FF

You will get *without* the unify option:

cansend can0 100#	-> two times CAN frame with 0x100
cansend can0 200#	-> two times CAN frame with 0x200
cansend can0 400#	-> two times CAN frame with 0x400
cansend can0 333#	-> three times CAN frame with 0x333

You will get *with* the unify option from the posted patch:

cansend can0 100#	-> one time CAN frame with 0x100
cansend can0 200#	-> one time CAN frame with 0x200
cansend can0 400#	-> one time CAN frame with 0x400
cansend can0 333#	-> one time CAN frame with 0x333

... the same as 'candump can0' %-(

So what you probably *want* to have is:

cansend can0 100#	-> no CAN frame
cansend can0 200#	-> no CAN frame
cansend can0 400#	-> no CAN frame
cansend can0 333#	-> one CAN frame with 0x333

Right?

An approach to solve exactly this use case could be to check whether *all* 
configured filters have passed the CAN frame.

This could be done by counting the receptions of the identical frames:

diff --git a/net/can/raw.c b/net/can/raw.c
index e593dc7..3022c25 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -78,6 +78,7 @@ MODULE_ALIAS("can-proto-1");
  struct unique_can_frame {
  	struct sk_buff *skb;
  	ktime_t tstamp;
+	int rx_count;
  };

  struct raw_sock {
@@ -130,11 +131,15 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
  	/* 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;
+		    ktime_equal(ro->uniq_cf.tstamp, oskb->tstamp)){
+			ro->uniq_cf.rx_count++;
+			if (ro->uniq_cf.rx_count < ro->count)
+				return;
+		} else {
+			ro->uniq_cf.skb = oskb;
+			ro->uniq_cf.tstamp = oskb->tstamp;
+			ro->uniq_cf.rx_count = 1;
+		}
  	}

  	/* do not pass non-CAN2.0 frames to a legacy socket */

(based on the patch before)

But does it really make sense to put such a tricky thing into the raw socket?

It flips the OR sematic to an AND semantic over the provided filters.
And then the name of the socket option should be adapted too.
Something like "all filters must match"

Regards,
Oliver



On 17.03.2015 14:23, Oliver Hartkopp wrote:
> Hi Marc,
>
> On 17.03.2015 13:58, Oliver Hartkopp wrote:
>> The CAN_RAW socket can set multiple CAN identifier specific filters that lead
>> to multiple filters in the af_can.c filter processing. These filters are
>> indenpendent from each other which leads to logical OR'ed filters when applied.
>>
>> Especially when the filter is intended to notch single CAN IDs or CAN ID ranges
>> a problem emerges when using more than just one filter: The notched CAN IDs
>> will be delivered once while the other CAN IDs will be delivered twice.
>>
>> This patch implements a new option to make sure that every CAN frame which is
>> filtered for a specific socket is only delivered once to the user space.
>> This is independent from the number of matching CAN filters of this socket.
>
> Just to make sure, this patch really helps:
>
> With the example above all CAN frames are delivered just once. The OR'ed
> filters still match. So you won't have the solution that you can have
> different notched areas of CAN IDs ...
>
> I hope I expressed myself clearly.
>
> Regards,
> Oliver
>
> --
> 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