Re: [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Murali Karicheri <m-karicheri2@ti.com>
Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
	David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Network Development <netdev@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	nsekhar@ti.com, Grygorii Strashko <grygorii.strashko@ti.com>
Subject: Re: [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP
Date: Tue, 8 Sep 2020 20:50:26 +0200	[thread overview]
Message-ID: <CA+FuTSde4fAiy6t-Q4o1C8wdU4ZYqNt0Qd2F8PsrnXvA8q03=A@mail.gmail.com> (raw)
In-Reply-To: <CA+FuTSeE_O_XozfnzDED_S4of-NwtRCN+oWr=O3JPpByfCz3Vg@mail.gmail.com>

> > >>>
> > >>> Shouldn't it show vlan_do_receive() ?
> > >>>
> > >>>       if (skb_vlan_tag_present(skb)) {
> > >>>           if (pt_prev) {
> > >>>               ret = deliver_skb(skb, pt_prev, orig_dev);
> > >>>               pt_prev = NULL;
> > >>>           }
> > >>>           if (vlan_do_receive(&skb))
> > >>>               goto another_round;
> > >>>           else if (unlikely(!skb))
> > >>>               goto out;
> > >>>       }
> > >>>
> > >>> Thanks
> > >>>
> > >>
> > >> I did an ftrace today and I find vlan_do_receive() is called for the
> > >> incoming frames before passing SKB to hsr_handle_frame(). If someone
> > >> can review this, it will help. Thanks.
> > >>
> > >> https://pastebin.ubuntu.com/p/CbRzXjwjR5/
> > >
> > > hsr_handle_frame is an rx_handler called after
> > > __netif_receive_skb_core called vlan_do_receive and jumped back to
> > > another_round.
> >
> > Yes. hsr_handle_frame() is a rx_handler() after the above code that
> > does vlan_do_receive(). The ftrace shows vlan_do_receive() is called
> > followed by call to hsr_handle_frame(). From ifconfig I can see both
> > hsr and vlan interface stats increments by same count. So I assume,
> > vlan_do_receive() is called initially and it removes the tag, update
> > stats and then return true and go for another round. Do you think that
> > is the case?
>
> That was my understanding.
>
> > vlan_do_receive() calls vlan_find_dev(skb->dev, vlan_proto, vlan_id)
> > to retrieve the real netdevice (real device). However VLAN device is
> > attached to hsr device (real device), but SKB will have HSR slave
> > Ethernet netdevice (in our case it is cpsw device) and vlan_find_dev()
> > would have failed since there is no vlan_info in cpsw netdev struct. So
> > below code  in vlan_do_receive() should have failed and return false.
> >
> >         vlan_dev = vlan_find_dev(skb->dev, vlan_proto, vlan_id);
> >         if (!vlan_dev)
> >                 return false;
> >
> > So how does it goes for another_round ? May be vlan_find_dev is
> > finding the hsr netdevice?
>
> It's good to answer this through code inspection and/or
> instrumentation. I do not have the answer immediately either.

Agreed that from reading the code I would vlan_do_receive to not find
a vlan dev associated with the physical nic, then passes the packet
unmodified to hsr_handle_frame.

Perhaps this seems to work because skb_vlan_untag has already
pulled the tag out of the packet?

But then you should not see counters increased on the vlan dev.