> -----Original Message-----
> From: Sriram Krishnan (srirakr2) <srirakr2@xxxxxxxxx>
> Sent: Tuesday, July 21, 2020 3:10 AM
> To: David Miller <davem@xxxxxxxxxxxxx>
> Cc: KY Srinivasan <kys@xxxxxxxxxxxxx>; Haiyang Zhang
> <haiyangz@xxxxxxxxxxxxx>; Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>;
> wei.liu@xxxxxxxxxx; Malcolm Bumgardner (mbumgard)
> <mbumgard@xxxxxxxxx>; Umesha G M (ugm) <ugm@xxxxxxxxx>; Niranjan M
> M (nimm) <nimm@xxxxxxxxx>; xe-linux-external(mailer list) <xe-linux-
> external@xxxxxxxxx>; kuba@xxxxxxxxxx; linux-hyperv@xxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3] net: hyperv: add support for vlans in netvsc driver
>
>
>
> On 21/07/20, 4:57 AM, "David Miller" <davem@xxxxxxxxxxxxx> wrote:
>
> From: Sriram Krishnan <srirakr2@xxxxxxxxx>
> Date: Mon, 20 Jul 2020 22:15:51 +0530
>
> > + if (skb->protocol == htons(ETH_P_8021Q)) {
> > + u16 vlan_tci = 0;
> > + skb_reset_mac_header(skb);
>
> > Please place an empty line between basic block local variable declarations
> > and actual code.
>
> > + netdev_err(net,"Pop vlan err %x\n",pop_err);
>
> > A space is necessary before "pop_err".
>
> Consolidated list of comments addressed:
> > 1. Blank line between declaration and code.
> Done
>
> > 2. Error handling is different than other parts of this code.
> > probably just need a goto drop on error.
> Done
>
> > It seems like you are putting into message, then driver is putting it
> > into meta-data in next code block. Maybe it should be combined?
> Not done
> This was on purpose. Merging the two code blocks might break existing
> functionality.
> There could be other modes where the packet arrives with 802.1q already in
> the Skb and the skb->protocol needn’t be 802.1q.
>
> > packet->total_bytes should be updated too.
> Not done.
> The total_bytes needs be the total length of packet after the host OS adds the
> 802.1q header back in before tx. Updating the total_bytes to -= VLAN_HEADER
> will lead to packet drop in the Host OS driver.
If you make this change, did you see any drop in a live test? The
"packet->total_bytes" in struct hv_netvsc_packet is for book keeping
only, which is used for stats info, and not visible by the host at all.
I made this suggestion because the "regular" vlan packet length was
counted by bytes without the VLAN_HLEN(4) -- the vlan tag is
in the skb metadata, separately from the ethernet header. I want the
statistical data for AF_PACKET mode consistent with the regular case.
struct hv_netvsc_packet {
/* Bookkeeping stuff */
u8 cp_partial; /* partial copy into send buffer */
u8 rmsg_size; /* RNDIS header and PPI size */
u8 rmsg_pgcnt; /* page count of RNDIS header and PPI */
u8 page_buf_cnt;
u16 q_idx;
u16 total_packets;
u32 total_bytes;
u32 send_buf_index;
u32 total_data_buflen;
};
Thanks
- Haiyang
