-----Original Message-----
From: Sriram Krishnan (srirakr2) <redacted>
Sent: Tuesday, July 21, 2020 3:10 AM
To: David Miller <davem@davemloft.net>
Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
[off-list ref]; Stephen Hemminger [off-list ref];
wei.liu@kernel.org; Malcolm Bumgardner (mbumgard)
[off-list ref]; Umesha G M (ugm) [off-list ref]; Niranjan M
M (nimm) [off-list ref]; xe-linux-external(mailer list) <xe-linux-
external@cisco.com>; kuba@kernel.org; linux-hyperv@vger.kernel.org;
netdev@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] net: hyperv: add support for vlans in netvsc driver



On 21/07/20, 4:57 AM, "David Miller" [off-list ref] wrote:

    From: Sriram Krishnan [off-list ref]
    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:

quoted

1. Blank line between declaration and code.

Done

quoted

2. Error handling is different than other parts of this code.
  probably just need a goto drop on error.

Done

quoted

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.

quoted

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