On 08/29/2018 05:35 PM, Brad Drehmer wrote: > Some devices that we communicate with require that a byte be incremented every time that a message is sent. The patches below implement this functionality, with modifications made to both the can-gw kernel module and the cangw can-utils application. The user specifies which byte of the message should be incremented and the maximum value that the counter should go up to before resetting and beginning to count again from 0. > > > can-gw (https://github.com/DV-Electronics/linux/tree/fbd509a7c1373157ade87b145823bfa4a0662cf6) > > --- net/can/gw.c.orig 2018-08-29 10:07:59.255121361 -0400 > +++ net/can/gw.c 2018-08-29 10:10:30.631121361 -0400 > @@ -101,6 +101,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; > @@ -201,6 +209,23 @@ 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]) > + */ /* Multiline comments in the net subsystem start on the first * line... */ > + > + if (re > -9 && re < 8) > + return 0; > + else > + return -EINVAL; The way is to check for the error and return it. Remove the else and return 0; at the end of the function. > +} > + > static inline int calc_idx(int idx, int rx_dlc) > { > if (idx < 0) > @@ -346,6 +371,18 @@ 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 = msgcounter->value + 1; Can the u8 already overflow here? > + > + if(count > msgcounter->max_count) if (count >= msgcounter->max_count) count = 0; else count++; > + count = 0; // roll over the counter No C++ comments please - I'm not sure if we need these comments here. The code is quite readable. > + > + msgcounter->value = count; // increment for next time It's not _always_ incremented, but sometimes rolled over. So the comment is not 1000% correct :) > + > + cf->data[msgcounter->result_idx] = count; > +} > + > /* the receive & process & send function */ > static void can_can_gw_rcv(struct sk_buff *skb, void *data) > { > @@ -418,8 +455,12 @@ static void can_can_gw_rcv(struct sk_buf > 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); > + > + /* 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); > > @@ -554,6 +595,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) > @@ -619,6 +666,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 }, > @@ -718,9 +766,22 @@ 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; > > + 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]); > > > > --- include/uapi/linux/can/gw.h.orig 2018-08-29 10:06:35.919121361 -0400 > +++ include/uapi/linux/can/gw.h 2018-08-29 10:09:53.571121361 -0400 > @@ -69,6 +69,7 @@ enum { > CGW_MOD_OR, /* CAN frame modification binary OR */ > CGW_MOD_XOR, /* CAN frame modification binary XOR */ > CGW_MOD_SET, /* CAN frame modification set alternate values */ > + CGW_COUNTER, /* set message counter into data[index] */ You're modifying the userspace ABI, please don't do that. Add new values at the end of the enum. > CGW_CS_XOR, /* set data[] XOR checksum into data[index] */ > CGW_CS_CRC8, /* set data[] CRC8 checksum into data[index] */ > CGW_HANDLED, /* number of handled CAN frames */ > @@ -106,6 +107,16 @@ 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 max_count; // roll over counter when you hit this maximum > + __u8 value; // incremented every time that a message is sent > +} __attribute__((packed)); newline please > +/* length of message counter parameters. idx = index in CAN frame data[] */ > +#define CGW_COUNTER_LEN sizeof(struct cgw_counter) > + > +/* checksum modifiers */ > struct cgw_csum_xor { > __s8 from_idx; > __s8 to_idx; > @@ -161,7 +172,7 @@ enum { > * Limit the number of hops of this specific rule. Usually the received CAN > * frame can be processed as much as 'max_hops' times (which is given at module > * load time of the can-gw module). This value is used to reduce the number of > - * possible hops for this gateway rule to a value smaller then max_hops. > + * possible hops for this gateway rule to a value smaller than max_hops. Ok - but please make it a separate patch. > * > * CGW_MOD_UID (length 4 bytes): > * Optional non-zero user defined routing job identifier to alter existing > > > > > can-utils (https://github.com/DV-Electronics/can-utils/tree/45c38854b984996abc0230aa5f556e7a734ad802) Please make userspace changes a seperate patch, too. > > --- cangw.c.orig 2018-08-29 10:26:08.679121361 -0400 > +++ cangw.c 2018-08-29 10:29:01.663121361 -0400 > @@ -184,6 +184,13 @@ void print_cs_crc8(struct cgw_csum_crc8 > print_cs_crc8_profile(cs_crc8); > } > > +void print_counter(struct cgw_counter *msgcounter) > +{ > + printf("-n %d:%d:%d ", > + msgcounter->result_idx, msgcounter->max_count, > + msgcounter->value); Pease indent consistently. > +} > + > void print_usage(char *prg) > { > fprintf(stderr, "\nUsage: %s [options]\n\n", prg); > @@ -200,6 +207,7 @@ void print_usage(char *prg) > fprintf(stderr, " -l <hops> (limit the number of frame hops / routings)\n"); > fprintf(stderr, " -f <filter> (set CAN filter)\n"); > fprintf(stderr, " -m <mod> (set frame modifications)\n"); > + fprintf(stderr, " -n <result_idx>:<max_count>:<value> (message counter)\n"); > fprintf(stderr, " -x <from_idx>:<to_idx>:<result_idx>:<init_xor_val> (XOR checksum)\n"); > fprintf(stderr, " -c <from>:<to>:<result>:<init_val>:<xor_val>:<crctab[256]> (CRC8 cs)\n"); > fprintf(stderr, " -p <profile>:[<profile_data>] (CRC8 checksum profile & parameters)\n"); > @@ -421,6 +429,7 @@ int parse_rtlist(char *prgname, unsigned > case CGW_MOD_SET: > case CGW_MOD_UID: > case CGW_LIM_HOPS: > + case CGW_COUNTER: > case CGW_CS_XOR: > case CGW_CS_CRC8: > break; > @@ -501,6 +510,10 @@ int parse_rtlist(char *prgname, unsigned > printf("-l %d ", *(__u8 *)RTA_DATA(rta)); > break; > > + case CGW_COUNTER: > + print_counter((struct cgw_counter *)RTA_DATA(rta)); > + break; > + > case CGW_CS_XOR: > print_cs_xor((struct cgw_csum_xor *)RTA_DATA(rta)); > break; > @@ -542,6 +555,7 @@ int main(int argc, char **argv) > > int cmd = UNSPEC; > int have_filter = 0; > + int have_counter = 0; > int have_cs_xor = 0; > int have_cs_crc8 = 0; > > @@ -565,6 +579,7 @@ int main(int argc, char **argv) > struct can_filter filter; > struct sockaddr_nl nladdr; > > + struct cgw_counter msgcounter; > struct cgw_csum_xor cs_xor; > struct cgw_csum_crc8 cs_crc8; > char crc8tab[513] = {0}; > @@ -574,10 +589,11 @@ int main(int argc, char **argv) > int i; > > memset(&req, 0, sizeof(req)); > + memset(&msgcounter, 0, sizeof(msgcounter)); > memset(&cs_xor, 0, sizeof(cs_xor)); > memset(&cs_crc8, 0, sizeof(cs_crc8)); > > - while ((opt = getopt(argc, argv, "ADFLs:d:teiu:l:f:c:p:x:m:?")) != -1) { > + while ((opt = getopt(argc, argv, "ADFLs:d:teiu:l:f:c:p:n:x:m:?")) != -1) { > switch (opt) { > > case 'A': > @@ -690,6 +706,17 @@ int main(int argc, char **argv) > exit(0); > break; > > + case 'n': // increment result_idx by one each time that a message is sent, rolling over at max no C++ comments please > + if ((sscanf(optarg, "%hhd:%hhd:%hhd", > + &msgcounter.result_idx, &msgcounter.max_count, > + &msgcounter.value) == 3)) { > + have_counter = 1; > + } else { > + printf("Bad counter definition '%s'.\n", optarg); > + exit(1); > + } > + break; > + > default: > fprintf(stderr, "Unknown option %c\n", opt); > print_usage(basename(argv[0])); > @@ -709,8 +736,8 @@ int main(int argc, char **argv) > exit(1); > } > > - if (!modidx && (have_cs_crc8 || have_cs_xor)) { > - printf("-c or -x can only be used in conjunction with -m\n"); > + if (!modidx && (have_counter || have_cs_crc8 || have_cs_xor)) { > + printf("-n or -c or -x can only be used in conjunction with -m\n"); > exit(1); > } > > @@ -762,6 +789,9 @@ int main(int argc, char **argv) > if (have_filter) > addattr_l(&req.nh, sizeof(req), CGW_FILTER, &filter, sizeof(filter)); > > + if (have_counter) > + addattr_l(&req.nh, sizeof(req), CGW_COUNTER, &msgcounter, sizeof(msgcounter)); > + > if (have_cs_crc8) > addattr_l(&req.nh, sizeof(req), CGW_CS_CRC8, &cs_crc8, sizeof(cs_crc8)); > > > > --- gw.h.orig 2018-08-29 10:32:19.951121361 -0400 > +++ gw.h 2018-08-29 10:32:56.199121361 -0400 > @@ -70,6 +70,7 @@ enum { > CGW_MOD_OR, /* CAN frame modification binary OR */ > CGW_MOD_XOR, /* CAN frame modification binary XOR */ > CGW_MOD_SET, /* CAN frame modification set alternate values */ > + CGW_COUNTER, /* set message counter into data[index] */ > CGW_CS_XOR, /* set data[] XOR checksum into data[index] */ > CGW_CS_CRC8, /* set data[] CRC8 checksum into data[index] */ > CGW_HANDLED, /* number of handled CAN frames */ > @@ -107,6 +108,15 @@ 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 max_count; // roll over counter when you hit this maximum > + __u8 value; // incremented every time that a message is sent > +} __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; > @@ -162,7 +172,7 @@ enum { > * Limit the number of hops of this specific rule. Usually the received CAN > * frame can be processed as much as 'max_hops' times (which is given at module > * load time of the can-gw module). This value is used to reduce the number of > - * possible hops for this gateway rule to a value smaller then max_hops. > + * possible hops for this gateway rule to a value smaller than max_hops. > * > * CGW_MOD_UID (length 4 bytes): > * Optional non-zero user defined routing job identifier to alter existing > > > > > 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. > 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:
signature.asc
Description: OpenPGP digital signature
