On Thu, 2 Feb 2017 15:40:19 -0800, Michael Chan wrote: > On Thu, Feb 2, 2017 at 2:56 PM, Jakub Kicinski <kubak...@wp.pl> wrote: > > On Thu, 2 Feb 2017 11:55:29 -0500, Michael Chan wrote: > >> @@ -755,8 +757,8 @@ static void bnxt_reuse_rx_agg_bufs(struct bnxt_napi > >> *bnapi, u16 cp_cons, > >> > >> static struct sk_buff *bnxt_rx_skb(struct bnxt *bp, > >> struct bnxt_rx_ring_info *rxr, u16 cons, > >> - u16 prod, u8 *data, dma_addr_t dma_addr, > >> - unsigned int len) > >> + u16 prod, void *data, dma_addr_t dma_addr, > >> + unsigned int offset_and_len) > >> { > >> int err; > >> struct sk_buff *skb; > >> @@ -776,7 +778,7 @@ static struct sk_buff *bnxt_rx_skb(struct bnxt *bp, > >> } > >> > >> skb_reserve(skb, BNXT_RX_OFFSET); > >> - skb_put(skb, len); > >> + skb_put(skb, offset_and_len & 0xffff); > >> return skb; > >> } > >> > > > > Sorry to be a pain but I still don't understand (a) why you make this > > change in the first patch if it's only needed from patch 5 on; > > Because I'm thinking ahead. The function is now a function pointer > and I want to make the parameter change now.
You're thinking ahead, reviewers have to guess ahead :) > > (b) why > > do you encode the two parameters in a single u32? It's the seventh > > parameter so it's going on the stack anyway, no? > > Both the length and the offset come from the hardware's rx completion > record. Both are u16. The offset happens to be in the upper 16-bit > in the hardware record. So it is convenient to encode it like this > and I chose to do it like this. Of course, using a separate parameter > will also work. Yes, I initially thought you read them out this way straight from the descriptor but you actually combine them into this form: + unsigned int payload_len; + + payload_len = (le32_to_cpu(rxcmp->rx_cmp_misc_v1) & + RX_CMP_PAYLOAD_OFFSET) | len; + skb = bp->rx_skb_func(bp, rxr, cons, prod, data, dma_addr, I don't mind though, I was just hoping this is some clever optimization technique I could learn :)