[08/14] net: vlan: remove invalid VLAN protocol warning

  On MTK SoC ethernet, using NETIF_F_HW_VLAN_CTAG_RX in combination with hardware
special tag parsing can pass the special tag port metadata as VLAN protocol ID.
When the results is added as a skb hwaccel VLAN tag, it triggers a warning from
vlan_do_receive before calling the DSA tag receive function.
Remove this warning in order to properly pass the tag to the DSA receive function,
which will parse and clear it.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/8021q/vlan.h | 1 -
 1 file changed, 1 deletion(-)
  

On Mon, Nov 07, 2022 at 07:54:46PM +0100, Felix Fietkau wrote:
> On MTK SoC ethernet, using NETIF_F_HW_VLAN_CTAG_RX in combination with hardware
> special tag parsing can pass the special tag port metadata as VLAN protocol ID.
> When the results is added as a skb hwaccel VLAN tag, it triggers a warning from
> vlan_do_receive before calling the DSA tag receive function.
> Remove this warning in order to properly pass the tag to the DSA receive function,
> which will parse and clear it.
> 
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  net/8021q/vlan.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
> index 5eaf38875554..3f9c0406b266 100644
> --- a/net/8021q/vlan.h
> +++ b/net/8021q/vlan.h
> @@ -44,7 +44,6 @@ static inline int vlan_proto_idx(__be16 proto)
>  	case htons(ETH_P_8021AD):
>  		return VLAN_PROTO_8021AD;
>  	default:
> -		WARN(1, "invalid VLAN protocol: 0x%04x\n", ntohs(proto));

Why would you ever want to remove a warning that's telling you you're
doing something wrong?

Aren't you calling __vlan_hwaccel_put_tag() with the wrong thing (i.e.
htons(RX_DMA_VPID()) as opposed to VPID translated to something
digestible by the rest of the network stack.. ETH_P_8021Q, ETH_P_8021AD
etc)?

>  		return -EINVAL;
>  	}
>  }
> -- 
> 2.38.1
>
  

On 07.11.22 22:57, Vladimir Oltean wrote:
> On Mon, Nov 07, 2022 at 07:54:46PM +0100, Felix Fietkau wrote:
>> On MTK SoC ethernet, using NETIF_F_HW_VLAN_CTAG_RX in combination with hardware
>> special tag parsing can pass the special tag port metadata as VLAN protocol ID.
>> When the results is added as a skb hwaccel VLAN tag, it triggers a warning from
>> vlan_do_receive before calling the DSA tag receive function.
>> Remove this warning in order to properly pass the tag to the DSA receive function,
>> which will parse and clear it.
>> 
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>  net/8021q/vlan.h | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
>> index 5eaf38875554..3f9c0406b266 100644
>> --- a/net/8021q/vlan.h
>> +++ b/net/8021q/vlan.h
>> @@ -44,7 +44,6 @@ static inline int vlan_proto_idx(__be16 proto)
>>  	case htons(ETH_P_8021AD):
>>  		return VLAN_PROTO_8021AD;
>>  	default:
>> -		WARN(1, "invalid VLAN protocol: 0x%04x\n", ntohs(proto));
> 
> Why would you ever want to remove a warning that's telling you you're
> doing something wrong?
> 
> Aren't you calling __vlan_hwaccel_put_tag() with the wrong thing (i.e.
> htons(RX_DMA_VPID()) as opposed to VPID translated to something
> digestible by the rest of the network stack.. ETH_P_8021Q, ETH_P_8021AD
> etc)?
The MTK ethernet hardware treats the DSA special tag as a VLAN tag and 
reports it as such. The ethernet driver passes this on as a hwaccel tag, 
and the MTK DSA tag parser consumes it. The only thing that's sitting in 
the middle looking at the tag is the VLAN device lookup with that warning.

Whenever DSA is not being used, the MTK ethernet device can also process 
regular VLAN tags. For those tags, htons(RX_DMA_VPID()) will contain the 
correct VPID.

- Felix
  

On Tue, Nov 08, 2022 at 07:08:46AM +0100, Felix Fietkau wrote:
> On 07.11.22 22:57, Vladimir Oltean wrote:
> > Aren't you calling __vlan_hwaccel_put_tag() with the wrong thing (i.e.
> > htons(RX_DMA_VPID()) as opposed to VPID translated to something
> > digestible by the rest of the network stack.. ETH_P_8021Q, ETH_P_8021AD
> > etc)?
> 
> The MTK ethernet hardware treats the DSA special tag as a VLAN tag and
> reports it as such. The ethernet driver passes this on as a hwaccel tag, and
> the MTK DSA tag parser consumes it. The only thing that's sitting in the
> middle looking at the tag is the VLAN device lookup with that warning.
> 
> Whenever DSA is not being used, the MTK ethernet device can also process
> regular VLAN tags. For those tags, htons(RX_DMA_VPID()) will contain the
> correct VPID.

So I don't object to the overall theme of having the DSA master offload
the parsing and removal of the DSA tag, but you knock down a bit too
many fences if you carry the DSA tag in skb->vlan_present (not only VLAN
upper device lookup, but also the flow dissector).

What other information will be present in the offloaded DSA headers
except source port information? Maxime Chevallier is also working on a
similar problem for qca8k, except in that case, the RX DSA offload seems
to not be optional for him.

https://patchwork.kernel.org/project/netdevbpf/patch/20221104174151.439008-4-maxime.chevallier@bootlin.com/

Would a solution based on METADATA_HW_PORT_MUX and dst_metadata that
point to refcounted, preallocated structs work for Mediatek SoCs with
DSA, or would more information be necessary?

Meaning: mtk_eth_soc attaches the dst_metadata to the skb, tag_mtk.c
retrieves and removes it.
  

On 08.11.22 10:00, Vladimir Oltean wrote:
> On Tue, Nov 08, 2022 at 07:08:46AM +0100, Felix Fietkau wrote:
>> On 07.11.22 22:57, Vladimir Oltean wrote:
>> > Aren't you calling __vlan_hwaccel_put_tag() with the wrong thing (i.e.
>> > htons(RX_DMA_VPID()) as opposed to VPID translated to something
>> > digestible by the rest of the network stack.. ETH_P_8021Q, ETH_P_8021AD
>> > etc)?
>> 
>> The MTK ethernet hardware treats the DSA special tag as a VLAN tag and
>> reports it as such. The ethernet driver passes this on as a hwaccel tag, and
>> the MTK DSA tag parser consumes it. The only thing that's sitting in the
>> middle looking at the tag is the VLAN device lookup with that warning.
>> 
>> Whenever DSA is not being used, the MTK ethernet device can also process
>> regular VLAN tags. For those tags, htons(RX_DMA_VPID()) will contain the
>> correct VPID.
> 
> So I don't object to the overall theme of having the DSA master offload
> the parsing and removal of the DSA tag, but you knock down a bit too
> many fences if you carry the DSA tag in skb->vlan_present (not only VLAN
> upper device lookup, but also the flow dissector).
> 
> What other information will be present in the offloaded DSA headers
> except source port information? Maxime Chevallier is also working on a
> similar problem for qca8k, except in that case, the RX DSA offload seems
> to not be optional for him.
> 
> https://patchwork.kernel.org/project/netdevbpf/patch/20221104174151.439008-4-maxime.chevallier@bootlin.com/
> 
> Would a solution based on METADATA_HW_PORT_MUX and dst_metadata that
> point to refcounted, preallocated structs work for Mediatek SoCs with
> DSA, or would more information be necessary?
> 
> Meaning: mtk_eth_soc attaches the dst_metadata to the skb, tag_mtk.c
> retrieves and removes it.
I need to look into how METADATA_HW_PORT_MUX works, but I think it could 
work.

- Felix
  

On Tue, Nov 08, 2022 at 10:20:44AM +0100, Felix Fietkau wrote:
> I need to look into how METADATA_HW_PORT_MUX works, but I think it could
> work.

Could you please coordinate with Maxime to come up with something
common? Currently he proposes a generic "oob" tagger, while you propose
that we stay with the "mtk"/"qca" taggers, but they are taught to look
after offloaded metadata rather than in the packet. IMO your proposal
sounds better; the name of the tagging protocol is already exposed to
user space via /sys/class/net/<dsa-master>/dsa/tagging and therefore ABI.
It's just that we need a way to figure out how to make the flow
dissector and other layers not adjust for DSA header length if the DSA
tag is offloaded and not present in the packet.
  

On 08.11.22 10:40, Vladimir Oltean wrote:
> On Tue, Nov 08, 2022 at 10:20:44AM +0100, Felix Fietkau wrote:
>> I need to look into how METADATA_HW_PORT_MUX works, but I think it could
>> work.
> 
> Could you please coordinate with Maxime to come up with something
> common? Currently he proposes a generic "oob" tagger, while you propose
> that we stay with the "mtk"/"qca" taggers, but they are taught to look
> after offloaded metadata rather than in the packet. IMO your proposal
> sounds better; the name of the tagging protocol is already exposed to
> user space via /sys/class/net/<dsa-master>/dsa/tagging and therefore ABI.
> It's just that we need a way to figure out how to make the flow
> dissector and other layers not adjust for DSA header length if the DSA
> tag is offloaded and not present in the packet.
I think it depends on the hardware. If you can rely on the hardware 
always reporting the port out-of-band, a generic "oob" tagger would be 
fine.
In my case, it depends on whether a second non-DSA ethernet MAC is 
active on the same device, so I do need to continue using the "mtk" tag 
driver.
The flow dissector part is already solved: I simply used the existing 
.flow_dissect() tag op.

- Felix
  

On Tue, Nov 08, 2022 at 10:46:54AM +0100, Felix Fietkau wrote:
> I think it depends on the hardware. If you can rely on the hardware always
> reporting the port out-of-band, a generic "oob" tagger would be fine.
> In my case, it depends on whether a second non-DSA ethernet MAC is active on
> the same device, so I do need to continue using the "mtk" tag driver.

It's not only about the time dimension (always OOB, vs sometimes OOB),
but also about what is conveyed through the OOB tag. I can see 2 vendors
agreeing today on a common "oob" tagger only to diverge in the future
when they'll need to convey more information than just port id. What do
you do with the tagging protocol string names then. Gotta make them
unique from the get go, can't export "oob" to user space IMO.

> The flow dissector part is already solved: I simply used the existing
> .flow_dissect() tag op.

Yes, well this seems like a generic enough case (DSA offload tag present
=> don't shift network header because it's where it should be) to treat
it in the generic flow dissector and not have to invoke any tagger-specific
fixup method.
  

On 08.11.22 11:08, Vladimir Oltean wrote:
> On Tue, Nov 08, 2022 at 10:46:54AM +0100, Felix Fietkau wrote:
>> I think it depends on the hardware. If you can rely on the hardware always
>> reporting the port out-of-band, a generic "oob" tagger would be fine.
>> In my case, it depends on whether a second non-DSA ethernet MAC is active on
>> the same device, so I do need to continue using the "mtk" tag driver.
> 
> It's not only about the time dimension (always OOB, vs sometimes OOB),
> but also about what is conveyed through the OOB tag. I can see 2 vendors
> agreeing today on a common "oob" tagger only to diverge in the future
> when they'll need to convey more information than just port id. What do
> you do with the tagging protocol string names then. Gotta make them
> unique from the get go, can't export "oob" to user space IMO.
> 
>> The flow dissector part is already solved: I simply used the existing
>> .flow_dissect() tag op.
> 
> Yes, well this seems like a generic enough case (DSA offload tag present
> => don't shift network header because it's where it should be) to treat
> it in the generic flow dissector and not have to invoke any tagger-specific
> fixup method.
In that case I think we shouldn't use METADATA_HW_PORT_MUX, since it is 
already used for other purposes. I will add a new metadata type 
METADATA_DSA instead.

- Felix
  

On Tue, Nov 08, 2022 at 11:24:51AM +0100, Felix Fietkau wrote:
> On 08.11.22 11:08, Vladimir Oltean wrote:
> > On Tue, Nov 08, 2022 at 10:46:54AM +0100, Felix Fietkau wrote:
> > > I think it depends on the hardware. If you can rely on the hardware always
> > > reporting the port out-of-band, a generic "oob" tagger would be fine.
> > > In my case, it depends on whether a second non-DSA ethernet MAC is active on
> > > the same device, so I do need to continue using the "mtk" tag driver.
> > 
> > It's not only about the time dimension (always OOB, vs sometimes OOB),
> > but also about what is conveyed through the OOB tag. I can see 2 vendors
> > agreeing today on a common "oob" tagger only to diverge in the future
> > when they'll need to convey more information than just port id. What do
> > you do with the tagging protocol string names then. Gotta make them
> > unique from the get go, can't export "oob" to user space IMO.
> > 
> > > The flow dissector part is already solved: I simply used the existing
> > > .flow_dissect() tag op.
> > 
> > Yes, well this seems like a generic enough case (DSA offload tag present
> > => don't shift network header because it's where it should be) to treat
> > it in the generic flow dissector and not have to invoke any tagger-specific
> > fixup method.
> 
> In that case I think we shouldn't use METADATA_HW_PORT_MUX, since it is
> already used for other purposes. I will add a new metadata type METADATA_DSA
> instead.

Which case, flow dissector figuring out that DSA offload tag is present?
Well, the skb can only carry one dst pointer ATM, so if it's METADATA_HW_PORT_MUX
but it belongs to SR-IOV on the DSA master, we have bigger problems anyway.
So, proto == ETH_P_XDSA && have METADATA_HW_PORT_MUX should be pretty
good indication that DSA offload tag is present.

Anyway I raised this concern as well to Jakub as well. Seems to be
theoretical at the moment. Using METADATA_HW_PORT_MUX seems to be fine
right now. Can be changed later if needed; it's not ABI.
  

On 08.11.22 11:33, Vladimir Oltean wrote:
> On Tue, Nov 08, 2022 at 11:24:51AM +0100, Felix Fietkau wrote:
>> On 08.11.22 11:08, Vladimir Oltean wrote:
>> > On Tue, Nov 08, 2022 at 10:46:54AM +0100, Felix Fietkau wrote:
>> > > I think it depends on the hardware. If you can rely on the hardware always
>> > > reporting the port out-of-band, a generic "oob" tagger would be fine.
>> > > In my case, it depends on whether a second non-DSA ethernet MAC is active on
>> > > the same device, so I do need to continue using the "mtk" tag driver.
>> > 
>> > It's not only about the time dimension (always OOB, vs sometimes OOB),
>> > but also about what is conveyed through the OOB tag. I can see 2 vendors
>> > agreeing today on a common "oob" tagger only to diverge in the future
>> > when they'll need to convey more information than just port id. What do
>> > you do with the tagging protocol string names then. Gotta make them
>> > unique from the get go, can't export "oob" to user space IMO.
>> > 
>> > > The flow dissector part is already solved: I simply used the existing
>> > > .flow_dissect() tag op.
>> > 
>> > Yes, well this seems like a generic enough case (DSA offload tag present
>> > => don't shift network header because it's where it should be) to treat
>> > it in the generic flow dissector and not have to invoke any tagger-specific
>> > fixup method.
>> 
>> In that case I think we shouldn't use METADATA_HW_PORT_MUX, since it is
>> already used for other purposes. I will add a new metadata type METADATA_DSA
>> instead.
> 
> Which case, flow dissector figuring out that DSA offload tag is present?
> Well, the skb can only carry one dst pointer ATM, so if it's METADATA_HW_PORT_MUX
> but it belongs to SR-IOV on the DSA master, we have bigger problems anyway.
> So, proto == ETH_P_XDSA && have METADATA_HW_PORT_MUX should be pretty
> good indication that DSA offload tag is present.
> 
> Anyway I raised this concern as well to Jakub as well. Seems to be
> theoretical at the moment. Using METADATA_HW_PORT_MUX seems to be fine
> right now. Can be changed later if needed; it's not ABI.
Okay, I will stick with METADATA_HW_PORT_MUX for now. If we use it in 
the flow dissector to avoid the tagger specific fixup, we might as well 
use it in DSA to skip the tag proto receive call. What do you think?

- Felix
  

On Tue, Nov 08, 2022 at 11:42:09AM +0100, Felix Fietkau wrote:
> Okay, I will stick with METADATA_HW_PORT_MUX for now. If we use it in the
> flow dissector to avoid the tagger specific fixup, we might as well use it
> in DSA to skip the tag proto receive call. What do you think?

I suppose that dsa_switch_rcv() could test for the presence of a metadata_dst
and treat that generically if present, without unnecessarily calling down into
the tagging protocol ->rcv() call. The assumption being that the metadata_dst
is always formatted (by the DSA master) in a vendor-agnostic way.
  

On 08.11.22 11:52, Vladimir Oltean wrote:
> On Tue, Nov 08, 2022 at 11:42:09AM +0100, Felix Fietkau wrote:
>> Okay, I will stick with METADATA_HW_PORT_MUX for now. If we use it in the
>> flow dissector to avoid the tagger specific fixup, we might as well use it
>> in DSA to skip the tag proto receive call. What do you think?
> 
> I suppose that dsa_switch_rcv() could test for the presence of a metadata_dst
> and treat that generically if present, without unnecessarily calling down into
> the tagging protocol ->rcv() call. The assumption being that the metadata_dst
> is always formatted (by the DSA master) in a vendor-agnostic way.
Right. The assumption is that if we use METADATA_HW_PORT_MUX, the field 
md_dst->u.port_info.port_id will contain the index of the DSA port.

- Felix
  

On Tue, Nov 08, 2022 at 11:56:52AM +0100, Felix Fietkau wrote:
> On 08.11.22 11:52, Vladimir Oltean wrote:
> > On Tue, Nov 08, 2022 at 11:42:09AM +0100, Felix Fietkau wrote:
> > > Okay, I will stick with METADATA_HW_PORT_MUX for now. If we use it in the
> > > flow dissector to avoid the tagger specific fixup, we might as well use it
> > > in DSA to skip the tag proto receive call. What do you think?
> > 
> > I suppose that dsa_switch_rcv() could test for the presence of a metadata_dst
> > and treat that generically if present, without unnecessarily calling down into
> > the tagging protocol ->rcv() call. The assumption being that the metadata_dst
> > is always formatted (by the DSA master) in a vendor-agnostic way.
> 
> Right. The assumption is that if we use METADATA_HW_PORT_MUX, the field
> md_dst->u.port_info.port_id will contain the index of the DSA port.

Yes. Please coordinate with Maxime, see if it's possible to do something
similar (generic) on TX, depending on whether the DSA master reports it
can interpret metadata_dst as offloaded DSA tags. You didn't copy too
many folks to this patch set, so others might have missed it.