Hi Oliver,
Here is the patch with the counter moved out of the 'if modidx' statements. The comment you proposed about it being the initial value isn't totally right because the first message that is sent will be one more than the value that you put in there. This is because every message is passed through the counter modification before sending. For example, when I initialize it in the userspace app, I initially set the counter_value to the max_count so that the first message sent will start at the min_count.
--- gw.h.orig 2018-01-28 16:20:33.000000000 -0500
+++ gw.h 2018-09-06 13:31:30.265601000 -0400
@@ -80,6 +80,7 @@ enum {
CGW_DELETED, /* number of deleted CAN frames (see max_hops param) */
CGW_LIM_HOPS, /* limit the number of hops of this specific rule */
CGW_MOD_UID, /* user defined identifier for modification updates */
+ CGW_COUNTER, /* set message counter into data[index] */
__CGW_MAX
};
@@ -107,6 +108,18 @@ struct cgw_frame_mod {
#define CGW_MODATTR_LEN sizeof(struct cgw_frame_mod)
+/* message counter modifiers */
+struct cgw_counter {
+ __s8 result_idx; /* byte index of the counter */
+ __u8 min_count; /* value that the counter is reset to after hitting max_count */
+ __u8 max_count; /* count up to this value */
+ __u8 counter_mask; /* allows you to not take up the whole byte with the counter */
+ __u8 counter_value; /* current value of counter */
+} __attribute__((packed));
+
+/* length of message counter parameters. idx = index in CAN frame data[] */
+#define CGW_COUNTER_LEN sizeof(struct cgw_counter)
+
struct cgw_csum_xor {
__s8 from_idx;
__s8 to_idx;
--- gw.c.orig 2018-01-28 16:20:33.000000000 -0500
+++ gw.c 2018-09-06 14:26:03.481939941 -0400
@@ -99,6 +99,14 @@ struct cf_mod {
void (*modfunc[MAX_MODFUNCTIONS])(struct can_frame *cf,
struct cf_mod *mod);
+ /* CAN frame counter increment after CAN frame modifications */
+ struct {
+ struct cgw_counter msgcounter;
+ } counter;
+ struct {
+ void (*msgcounter)(struct can_frame *cf, struct cgw_counter *msgcounter);
+ } counterfunc;
+
/* CAN frame checksum calculation after CAN frame modifications */
struct {
struct cgw_csum_xor xor;
@@ -199,6 +207,30 @@ static int cgw_chk_csum_parms(s8 fr, s8
return -EINVAL;
}
+static int cgw_chk_result_idx_parm(s8 re)
+{
+ /* absolute dlc values 0 .. 7 => 0 .. 7, e.g. data [0]
+ * relative to received dlc -1 .. -8 :
+ * e.g. for received dlc = 8
+ * -1 => index = 7 (data[7])
+ * -3 => index = 5 (data[5])
+ * -8 => index = 0 (data[0])
+ */
+
+ if (re < -8 || re > 7)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int cgw_chk_counter_min_max(u8 min_count, u8 max_count)
+{
+ if (max_count < min_count)
+ return -EINVAL;
+
+ return 0;
+}
+
static inline int calc_idx(int idx, int rx_dlc)
{
if (idx < 0)
@@ -344,6 +376,26 @@ static void cgw_csum_crc8_neg(struct can
cf->data[crc8->result_idx] = crc^crc8->final_xor_val;
}
+static void cgw_count(struct can_frame *cf, struct cgw_counter *msgcounter)
+{
+ u8 count, input, inverted_mask;
+ int idx = calc_idx(msgcounter->result_idx, cf->can_dlc);
+
+ if (idx < 0)
+ return;
+
+ count = msgcounter->min_count;
+
+ if (msgcounter->counter_value < msgcounter->max_count && msgcounter->counter_value < 0xFF)
+ count = msgcounter->counter_value + 1;
+
+ msgcounter->counter_value = count;
+
+ input = cf->data[idx];
+ inverted_mask = ~(msgcounter->counter_mask);
+ cf->data[idx] = (input & inverted_mask) | count;
+}
+
/* the receive & process & send function */
static void can_can_gw_rcv(struct sk_buff *skb, void *data)
{
@@ -415,6 +467,12 @@ static void can_can_gw_rcv(struct sk_buf
/* perform preprocessed modification functions if there are any */
while (modidx < MAX_MODFUNCTIONS && gwj->mod.modfunc[modidx])
(*gwj->mod.modfunc[modidx++])(cf, &gwj->mod);
+
+ /* apply counter updates if enabled */
+ if (gwj->mod.counterfunc.msgcounter) {
+ (*gwj->mod.counterfunc.msgcounter)(cf, &gwj->mod.counter.msgcounter);
+ modidx++;
+ }
/* check for checksum updates when the CAN frame has been modified */
if (modidx) {
@@ -551,6 +609,12 @@ static int cgw_put_job(struct sk_buff *s
goto cancel;
}
+ if (gwj->mod.counterfunc.msgcounter) {
+ if (nla_put(skb, CGW_COUNTER, CGW_COUNTER_LEN,
+ &gwj->mod.counter.msgcounter) < 0)
+ goto cancel;
+ }
+
if (gwj->mod.csumfunc.crc8) {
if (nla_put(skb, CGW_CS_CRC8, CGW_CS_CRC8_LEN,
&gwj->mod.csum.crc8) < 0)
@@ -617,6 +681,7 @@ static const struct nla_policy cgw_polic
[CGW_MOD_OR] = { .len = sizeof(struct cgw_frame_mod) },
[CGW_MOD_XOR] = { .len = sizeof(struct cgw_frame_mod) },
[CGW_MOD_SET] = { .len = sizeof(struct cgw_frame_mod) },
+ [CGW_COUNTER] = { .len = sizeof(struct cgw_counter) },
[CGW_CS_XOR] = { .len = sizeof(struct cgw_csum_xor) },
[CGW_CS_CRC8] = { .len = sizeof(struct cgw_csum_crc8) },
[CGW_SRC_IF] = { .type = NLA_U32 },
@@ -716,6 +781,28 @@ static int cgw_parse_attr(struct nlmsghd
mod->modfunc[modidx++] = mod_set_data;
}
+ /* check for counter operations after CAN frame modifications */
+ if (tb[CGW_COUNTER]) {
+ struct cgw_counter *c = nla_data(tb[CGW_COUNTER]);
+
+ err = cgw_chk_result_idx_parm(c->result_idx);
+ if (err)
+ return err;
+
+ err = cgw_chk_counter_min_max(c->min_count, c->max_count);
+ if (err)
+ return err;
+
+ /* sanity check on counter value */
+ if (c->counter_value < c->min_count || c->counter_value > c->max_count)
+ c->counter_value = c->max_count;
+
+ nla_memcpy(&mod->counter.msgcounter, tb[CGW_COUNTER],
+ CGW_COUNTER_LEN);
+
+ mod->counterfunc.msgcounter = cgw_count;
+ }
+
/* check for checksum operations after CAN frame modifications */
if (modidx) {
@@ -1058,3 +1145,4 @@ static __exit void cgw_module_exit(void)
module_init(cgw_module_init);
module_exit(cgw_module_exit);
+
Signed-off by: Brad Drehmer <b.drehmer@xxxxxxxxxxxxxxxxx>
TESTING THE FUTURE for OVER 20 YEARS
NOTICE: This e-mail and any attachments may contain confidential, privileged and proprietary information of D & V Electronics Ltd. and all such material is subject to copyright protection in favor of D & V Electronics Ltd. Any unauthorized disclosure or other use of this e-mail or the information contained herein or in any documents attached hereto may be unlawful and is strictly prohibited. If you have received this e-mail in error, please notify the sender immediately and delete this e-mail without reading, printing, copying or forwarding it to anyone. This email was sent from D & V Electronics Ltd. To receive no further email communications, you can send an email to unsubscribe@xxxxxxxxxxxxxxxxx. D & V Electronics is located at 130 Zenway Blvd. Woodbridge, Ontario, Canada.
-----Original Message-----
From: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
Sent: September 5, 2018 2:53 AM
To: Brad Drehmer <b.drehmer@xxxxxxxxxxxxxxxxx>; Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
Cc: linux-can@xxxxxxxxxxxxxxx
Subject: Re: [PATCH can-gw] add support for counter byte
Hi Brad,
On 09/04/2018 05:56 PM, Brad Drehmer wrote:
> Here's an updated patch. I included a parameter for the mask that Oliver mentioned, and also added a parameter for the minimum value instead of assuming that the counter resets to 0.
>
> --- ./gw.h.orig 2018-01-28 16:20:33.000000000 -0500
> +++ ./gw.h 2018-09-04 11:42:50.047000000 -0400
> @@ -80,6 +80,7 @@ enum {
> CGW_DELETED, /* number of deleted CAN frames (see max_hops param) */
> CGW_LIM_HOPS, /* limit the number of hops of this specific rule */
> CGW_MOD_UID, /* user defined identifier for modification updates */
> + CGW_COUNTER, /* set message counter into data[index] */
> __CGW_MAX
> };
>
> @@ -107,6 +108,18 @@ struct cgw_frame_mod {
>
> #define CGW_MODATTR_LEN sizeof(struct cgw_frame_mod)
>
> +/* message counter modifiers */
> +struct cgw_counter {
> + __s8 result_idx; /* byte index of the counter */
> + __u8 min_count; /* value that the counter is reset to after hitting max_count */
> + __u8 max_count; /* count up to this value */
> + __u8 counter_mask; /* allows you to not take up the whole byte with the counter */
> + __u8 counter_value; /* incremented every time that a message is sent */
/* initially given and current counter value */
Would this be correct?
> +} __attribute__((packed));
> +
> +/* length of message counter parameters. idx = index in CAN frame
> +data[] */ #define CGW_COUNTER_LEN sizeof(struct cgw_counter)
> +
> struct cgw_csum_xor {
> __s8 from_idx;
> __s8 to_idx;
>
>
> --- ./gw.c.orig 2018-01-28 16:20:33.000000000 -0500
> +++ ./gw.c 2018-09-04 11:20:01.695000000 -0400
> @@ -99,6 +99,14 @@ struct cf_mod {
> void (*modfunc[MAX_MODFUNCTIONS])(struct can_frame *cf,
> struct cf_mod *mod);
>
> + /* CAN frame counter increment after CAN frame modifications */
> + struct {
> + struct cgw_counter msgcounter;
> + } counter;
> + struct {
> + void (*msgcounter)(struct can_frame *cf, struct cgw_counter *msgcounter);
> + } counterfunc;
> +
> /* CAN frame checksum calculation after CAN frame modifications */
> struct {
> struct cgw_csum_xor xor; @@ -199,6 +207,30 @@ static
> int cgw_chk_csum_parms(s8 fr, s8
> return -EINVAL;
> }
>
> +static int cgw_chk_result_idx_parm(s8 re) {
> + /* absolute dlc values 0 .. 7 => 0 .. 7, e.g. data [0]
> + * relative to received dlc -1 .. -8 :
> + * e.g. for received dlc = 8
> + * -1 => index = 7 (data[7])
> + * -3 => index = 5 (data[5])
> + * -8 => index = 0 (data[0])
> + */
> +
> + if (re < -8 || re > 7)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int cgw_chk_counter_min_max(u8 min_count, u8 max_count) {
> + if (max_count < min_count)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static inline int calc_idx(int idx, int rx_dlc)
> {
> if (idx < 0)
> @@ -344,6 +376,26 @@ static void cgw_csum_crc8_neg(struct can
> cf->data[crc8->result_idx] = crc^crc8->final_xor_val;
> }
>
This function would have been interesting to review ;-)
> +static void cgw_count(struct can_frame *cf, struct cgw_counter
> +*msgcounter) {
> + u8 count, input, inverted_mask;
> + int idx = calc_idx(msgcounter->result_idx, cf->can_dlc);
> +
> + if (idx < 0)
> + return;
> +
> + count = msgcounter->min_count;
> +
> + if (msgcounter->counter_value < msgcounter->max_count && msgcounter->counter_value < 0xFF)
> + count = msgcounter->counter_value + 1;
> +
> + msgcounter->counter_value = count;
> +
> + input = cf->data[idx];
> + inverted_mask = ~(msgcounter->counter_mask);
> + cf->data[idx] = (input & inverted_mask) | count; }
> +
> /* the receive & process & send function */
> static void can_can_gw_rcv(struct sk_buff *skb, void *data)
> {
> @@ -415,9 +467,13 @@ static void can_can_gw_rcv(struct sk_buf
> /* perform preprocessed modification functions if there are any */
> while (modidx < MAX_MODFUNCTIONS && gwj->mod.modfunc[modidx])
> (*gwj->mod.modfunc[modidx++])(cf, &gwj->mod);
> -
> - /* check for checksum updates when the CAN frame has been modified */
> +
> if (modidx) {
> + /* check for counter updates when the CAN frame has been modified */
> + if (gwj->mod.counterfunc.msgcounter)
> + (*gwj->mod.counterfunc.msgcounter)(cf,
> + &gwj->mod.counter.msgcounter);
> +
Hm - no.
The msgcounter is an additional modification that can hapen.
So it just has to be outside this "if (modidx) {" block.
Like this:
/* perform preprocessed modification functions if there are any */
while (modidx < MAX_MODFUNCTIONS && gwj->mod.modfunc[modidx])
(*gwj->mod.modfunc[modidx++])(cf, &gwj->mod);
/* apply counter updates if enabled */
if (gwj->mod.counterfunc.msgcounter) {
(*gwj->mod.counterfunc.msgcounter)(cf, &gwj->mod.counter.msgcounter);
modidx++;
}
/* check for checksum updates when the CAN frame has been modified */ if (modidx) {
> + /* check for checksum updates when the CAN frame has
> + been modified */
> if (gwj->mod.csumfunc.crc8)
> (*gwj->mod.csumfunc.crc8)(cf,
> &gwj->mod.csum.crc8);
>
Ok?
Best,
Oliver
> @@ -551,6 +607,12 @@ static int cgw_put_job(struct sk_buff *s
> goto cancel;
> }
>
> + if (gwj->mod.counterfunc.msgcounter) {
> + if (nla_put(skb, CGW_COUNTER, CGW_COUNTER_LEN,
> + &gwj->mod.counter.msgcounter) < 0)
> + goto cancel;
> + }
> +
> if (gwj->mod.csumfunc.crc8) {
> if (nla_put(skb, CGW_CS_CRC8, CGW_CS_CRC8_LEN,
> &gwj->mod.csum.crc8) < 0) @@ -617,6
> +679,7 @@ static const struct nla_policy cgw_polic
> [CGW_MOD_OR] = { .len = sizeof(struct cgw_frame_mod) },
> [CGW_MOD_XOR] = { .len = sizeof(struct cgw_frame_mod) },
> [CGW_MOD_SET] = { .len = sizeof(struct cgw_frame_mod) },
> + [CGW_COUNTER] = { .len = sizeof(struct cgw_counter) },
> [CGW_CS_XOR] = { .len = sizeof(struct cgw_csum_xor) },
> [CGW_CS_CRC8] = { .len = sizeof(struct cgw_csum_crc8) },
> [CGW_SRC_IF] = { .type = NLA_U32 },
> @@ -716,9 +779,30 @@ static int cgw_parse_attr(struct nlmsghd
> mod->modfunc[modidx++] = mod_set_data;
> }
>
> - /* check for checksum operations after CAN frame modifications */
> if (modidx) {
> + /* check for counter operations after CAN frame modifications */
> + if (tb[CGW_COUNTER]) {
> + struct cgw_counter *c =
> + nla_data(tb[CGW_COUNTER]);
> +
> + err = cgw_chk_result_idx_parm(c->result_idx);
> + if (err)
> + return err;
> +
> + err = cgw_chk_counter_min_max(c->min_count, c->max_count);
> + if (err)
> + return err;
> +
> + /* sanity check on counter value */
> + if (c->counter_value < c->min_count || c->counter_value > c->max_count)
> + c->counter_value = c->max_count;
> +
> + nla_memcpy(&mod->counter.msgcounter, tb[CGW_COUNTER],
> + CGW_COUNTER_LEN);
> +
> + mod->counterfunc.msgcounter = cgw_count;
> + }
>
> + /* check for checksum operations after CAN frame
> + modifications */
> if (tb[CGW_CS_CRC8]) {
> struct cgw_csum_crc8 *c =
> nla_data(tb[CGW_CS_CRC8]);
>
>
>
> Signed-off by: Brad Drehmer <b.drehmer@xxxxxxxxxxxxxxxxx>
>
>
>
> TESTING THE FUTURE for OVER 20 YEARS
>
>
> NOTICE: This e-mail and any attachments may contain confidential, privileged and proprietary information of D & V Electronics Ltd. and all such material is subject to copyright protection in favor of D & V Electronics Ltd. Any unauthorized disclosure or other use of this e-mail or the information contained herein or in any documents attached hereto may be unlawful and is strictly prohibited. If you have received this e-mail in error, please notify the sender immediately and delete this e-mail without reading, printing, copying or forwarding it to anyone. This email was sent from D & V Electronics Ltd. To receive no further email communications, you can send an email to unsubscribe@xxxxxxxxxxxxxxxxx. D & V Electronics is located at 130 Zenway Blvd. Woodbridge, Ontario, Canada.
>
> -----Original Message-----
> From: Brad Drehmer
> Sent: August 29, 2018 2:53 PM
> To: 'Oliver Hartkopp' <socketcan@xxxxxxxxxxxx>; Marc Kleine-Budde
> <mkl@xxxxxxxxxxxxxx>
> Cc: linux-can@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH can-gw] add support for counter byte
>
> Thanks guys, I'll get back to you after addressing your concerns.
>
> Oliver,
> Right, I've been using "-n 6:15:0" to count from 0 to 15 in our application. Apologies for the email footer, but it is automatically inserted by our email server I'm afraid.
> I have some additional enhancements to support more types of checksum calculations too, but I'll wait until we work through this counter feature before trying to submit those.
>
> Brad
>
> -----Original Message-----
> From: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
> Sent: August 29, 2018 2:04 PM
> To: Brad Drehmer <b.drehmer@xxxxxxxxxxxxxxxxx>; Marc Kleine-Budde
> <mkl@xxxxxxxxxxxxxx>
> Cc: linux-can@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH can-gw] add support for counter byte
>
> Hi Brad, hi Marc,
>
> first: I think this functionality is cool :-)
>
> The review from Marc covered already most of my potential comments too.
>
> On 08/29/2018 07:30 PM, Brad Drehmer wrote:
>
>>
>> The index of the counter byte is used inside the cgw_count function in the line:
>> cf->data[msgcounter->result_idx] = count;
>>
>> To implement this feature I followed the lead of some of the existing features, so it should work in a similar way. cgw_chk_result_idx_parm is pretty much the same as cgw_chk_csum_parms, except instead of checking the validity of 3 indexes (from, to, and result), it checks only one (the result index where the counter goes).
>
> Marc is right. Your relative index idea does not work.
>
> If msgcounter->result_idx is negative, you need to do something like:
>
> int idx;
>
> if (msgcounter->result_idx >= 0)
>
> idx = msgcounter->result_idx;
>
> else
> idx = cf->can_dlc + msgcounter->result_idx;
>
> if (idx >= 0)
> cf->data[idx] = count;
>
> Got that?
>
> Additionally the comment/desciption, at which value of the max_count the rollover happens is unclear.
>
> E.g. to count from 0 .. 15
>
> .value has to be 0 and .max_count has to be 15?
>
> .value can be not 0, right?
>
> What happens if you start with .value = 234 and .max_count = 5 ??
>
> If .value is exposed to the API the counter starts there (but only the first time?!?).
>
> Finally I would suggest to add a .mask element, which can contain a bitmask for the byte where the index is stored in.
>
> E.g. if you want to have a rolling counter 0 .. 15 in data[0] and you would like to prevent the bits 4..7 in that data[0] you would be able to provide a mask of 0x0F to only modify the counter.
>
> Or do you thing we can calculate the mask automatically when the counters max_value are only allowed as 2^n-1 ?
>
> Best regards,
> Oliver
>
> ps. You have a pretty long footer. This is not common on mailing lists.
>
