Hi Kalle,

> -----Original Message-----
> From: Kalle Valo [mailto:kv...@codeaurora.org]
> Sent: 2017年5月18日 22:33
> To: Xinming Hu
> Cc: Linux Wireless; Brian Norris; Dmitry Torokhov; raja...@google.com;
> Zhiyuan Yang; Cathy Luo; Xinming Hu; Ganapathi Bhat
> Subject: Re: [PATCH v2 5/6] mwifiex: do not aggregate tcp ack in usb tx
> aggregation queue
> 
> Xinming Hu <huxinming...@gmail.com> writes:
> 
> > From: Xinming Hu <h...@marvell.com>
> >
> > Tcp ack should be send as soon to avoid throuput drop during receive
> > tcp traffic.
> >
> > Signed-off-by: Xinming Hu <h...@marvell.com>
> > Signed-off-by: Cathy Luo <c...@marvell.com>
> > Signed-off-by: Ganapathi Bhat <gb...@marvell.com>
> 
> [...]
> 
> > +static bool is_piggyback_tcp_ack(struct sk_buff *skb) {
> > +   struct ethhdr *ethh = NULL;
> > +   struct iphdr  *iph = NULL;
> > +   struct tcphdr *tcph = NULL;
> > +
> > +   ethh = (struct ethhdr *)skb->data;
> > +   if (ntohs(ethh->h_proto) != ETH_P_IP)
> > +           return false;
> > +
> > +   iph = (struct iphdr *)((u8 *)ethh + sizeof(struct ethhdr));
> > +   if (iph->protocol != IPPROTO_TCP)
> > +           return false;
> > +
> > +   tcph = (struct tcphdr *)((u8 *)iph + iph->ihl * 4);
> > +   /* Piggyback ack without payload*/
> > +   if (*((u8 *)tcph + 13) == 0x10 &&
> > +       ntohs(iph->tot_len) <= (iph->ihl + tcph->doff) * 4) {
> > +           return true;
> > +   }
> 
> It's rather ugly to use magic values (13 and 0x10) like that. Can't you use 
> some
> of the existing defines? At least I see TCP_FLAG_ACK and struct tcphdr::ack
> being available, so you should even have choises what to use.
> 

My wrong. Will resend V3 serials.
Thanks for the review.

Regards,
Simon
> --
> Kalle Valo

Reply via email to