Message ID | 2b600a5b-f928-fe73-5b30-8b2fc1398068@pengutronix.de |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: Mon, 6 Feb 2017 15:50:48 +0100 > this is a pull request of 16 patches for net-next/master. > > The first two patches by David Jander and me add the rx-offload > framework for CAN devices to the kernel. The remaining 14 patches > convert the flexcan driver to make use of it. Pulled, but I wonder if your comparisons does the right thing when the counters overflow. I think you need to do the same thing we do for TCP sequence number comparisons and code it like: static inline bool before(__u32 seq1, __u32 seq2) { return (__s32)(seq1-seq2) < 0; }
On 02/06/2017 05:12 PM, David Miller wrote: > From: Marc Kleine-Budde <mkl@pengutronix.de> > Date: Mon, 6 Feb 2017 15:50:48 +0100 > >> this is a pull request of 16 patches for net-next/master. >> >> The first two patches by David Jander and me add the rx-offload >> framework for CAN devices to the kernel. The remaining 14 patches >> convert the flexcan driver to make use of it. > > Pulled, but I wonder if your comparisons does the right thing when the > counters overflow. > > I think you need to do the same thing we do for TCP sequence number > comparisons and code it like: > > static inline bool before(__u32 seq1, __u32 seq2) > { > return (__s32)(seq1-seq2) < 0; > } Yes, I think it's basically the same as the TCP sequence number code, but obviously less readable. > static int can_rx_offload_compare(struct sk_buff *a, struct sk_buff *b) > { > const struct can_rx_offload_cb *cb_a, *cb_b; > > cb_a = can_rx_offload_get_cb(a); > cb_b = can_rx_offload_get_cb(b); > > /* Substract two u32 and return result as int, to keep > * difference steady around the u32 overflow. > */ > return cb_b->timestamp - cb_a->timestamp; > } This does the "(__s32)(seq1-seq2)" > skb_queue_reverse_walk(head, pos) { > const struct can_rx_offload_cb *cb_pos, *cb_new; > > cb_pos = can_rx_offload_get_cb(pos); > cb_new = can_rx_offload_get_cb(new); > > netdev_dbg(new->dev, > "%s: pos=0x%08x, new=0x%08x, diff=%10d, queue_len=%d\n", > __func__, > cb_pos->timestamp, cb_new->timestamp, > cb_new->timestamp - cb_pos->timestamp, > skb_queue_len(head)); > > if (compare(pos, new) < 0) And here the "return ... < 0;" > continue; > insert = pos; > break; > } Marc
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: Mon, 6 Feb 2017 17:43:43 +0100 > On 02/06/2017 05:12 PM, David Miller wrote: >> From: Marc Kleine-Budde <mkl@pengutronix.de> >> Date: Mon, 6 Feb 2017 15:50:48 +0100 >> >>> this is a pull request of 16 patches for net-next/master. >>> >>> The first two patches by David Jander and me add the rx-offload >>> framework for CAN devices to the kernel. The remaining 14 patches >>> convert the flexcan driver to make use of it. >> >> Pulled, but I wonder if your comparisons does the right thing when the >> counters overflow. >> >> I think you need to do the same thing we do for TCP sequence number >> comparisons and code it like: >> >> static inline bool before(__u32 seq1, __u32 seq2) >> { >> return (__s32)(seq1-seq2) < 0; >> } > > Yes, I think it's basically the same as the TCP sequence number code, > but obviously less readable. > >> static int can_rx_offload_compare(struct sk_buff *a, struct sk_buff *b) >> { >> const struct can_rx_offload_cb *cb_a, *cb_b; >> >> cb_a = can_rx_offload_get_cb(a); >> cb_b = can_rx_offload_get_cb(b); >> >> /* Substract two u32 and return result as int, to keep >> * difference steady around the u32 overflow. >> */ >> return cb_b->timestamp - cb_a->timestamp; >> } > > This does the "(__s32)(seq1-seq2)" ... > > And here the "return ... < 0;" Sure but what about these "can_rx_offload_le()" comparisons?
On 02/06/2017 05:59 PM, David Miller wrote: > Sure but what about these "can_rx_offload_le()" comparisons? This is to scan the mailboxes according to priority. On some CAN IP cores the mailboxes decrease in priority (0=high, 63=low) some increase (63=high, 0=low). The can_rx_offload_le() (le means less or equal) and can_rx_offload_inc() functions are used to abstract the order. We always only scan from highest to lower prio (offload->mb_first ... offload->mb_last): > for (i = offload->mb_first; > can_rx_offload_le(offload, i, offload->mb_last); > can_rx_offload_inc(offload, &i)) { So we never wrap around. Marc