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.
