Re: [PATCH] realtek: correct egress frame port verification

[Sander Vanheule]

Hi Birger,

On Sun, 2022-06-19 at 11:40 +0200, Birger Koblitz wrote:
> Hi,
> 
> On 19.06.22 10:56, Sander Vanheule wrote:
> 
> > -   h->cpu_tag[1] = h->cpu_tag[2] = 0;
> > -   if (prio >= 0)
> > -   h->cpu_tag[2] = BIT(13) | prio << 8; // Enable and set 
> > Priority Queue
> > +   h->cpu_tag[1] = 0;
> > +   /* Enable (AS_QID) and set Priority Queue (QID) */
> > +   h->cpu_tag[2] = (BIT(5) | (prio & 0x1f)) << 8;
> You are removing the possibility to let the SoC choose a Queue on its own 
> based on congestion,
> if you always enable AS_QID. There was a reason that there were negative 
> Queue-ID values, which
> denoted allowing the SoC to choose, i.e. AS_QID was not set.

I changed the code this way because prio was always positive, or made so by 
masking out the upper
bits (rtl8380/rtl8390). The passed value (sk_buff::priority >> 1) positive 
(unsigned) too. There
should be no change in behaviour with this patch.

Currently preparing a second version (series) of these changes, which I will 
send shortly. Please
let me know how this changes (hopefully improves) behaviour on the affected 
platforms, as I won't
have time to test this in the next days.

Best,
Sander

___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [PATCH] realtek: correct egress frame port verification

[Arinc UNAL (Xeront) via openwrt-devel]

The sender domain has a DMARC Reject/Quarantine policy which disallows
sending mailing list messages using the original "From" header.

To mitigate this problem, the original message has been wrapped
automatically by the mailing list software.--- Begin Message ---
On 19.06.2022 11:56, Sander Vanheule wrote:
> Destination switch ports for outgoing frame can range from 0 to
> CPU_PORT-1, and frame priorities should also always be positive.
> 
> Refactor the code to only generate egress frame CPU headers when the a
> valid destination port number is available, and make the code a bit more
> consistent between different switch generations. Change the dest_port
> and prio argument types from 'int' to 'unsigned int', since only
> positive values are valid.
> 
> This fixes the issue where egress frames on switch port 0 did not
> receive a VLAN tag, because they are sent out without a CPU header.
> Also fixes a potential issue with invalid (negative) egress port numbers
> on RTL93xx switches.
> 
> Reported-by: Arınç ÜNAL 
> Reported-by: Birger Koblitz 
> Signed-off-by: Sander Vanheule 

Thanks for doing this!

Acked-by: Arınç ÜNAL 

> ---
>   .../drivers/net/ethernet/rtl838x_eth.c| 86 +--
>   .../drivers/net/ethernet/rtl838x_eth.h|  2 +-
>   2 files changed, 40 insertions(+), 48 deletions(-)
> 
> diff --git 
> a/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c 
> b/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c
> index cf6aabc6142f..241a5787f820 100644
> --- a/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c
> +++ b/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c
> @@ -92,54 +92,44 @@ struct notify_b {
>   u32 reserved2[8];
>   };
>   
> -static void rtl838x_create_tx_header(struct p_hdr *h, int dest_port, int 
> prio)
> +static void rtl838x_create_tx_header(struct p_hdr *h, unsigned int 
> dest_port, unsigned int prio)
>   {
> - prio &= 0x7;
> -
> - if (dest_port > 0) {
> - // cpu_tag[0] is reserved on the RTL83XX SoCs
> - h->cpu_tag[1] = 0x0401;  // BIT 10: RTL8380_CPU_TAG, BIT0: 
> L2LEARNING on
> - h->cpu_tag[2] = 0x0200;  // Set only AS_DPM, to enable DPM 
> settings below
> - h->cpu_tag[3] = 0x;
> - h->cpu_tag[4] = BIT(dest_port) >> 16;
> - h->cpu_tag[5] = BIT(dest_port) & 0x;
> - // Set internal priority and AS_PRIO
> - if (prio >= 0)
> - h->cpu_tag[2] |= (prio | 0x8) << 12;
> - }
> + // cpu_tag[0] is reserved on the RTL83XX SoCs
> + h->cpu_tag[1] = 0x0401;  // BIT 10: RTL8380_CPU_TAG, BIT0: L2LEARNING on
> + h->cpu_tag[2] = 0x0200;  // Set only AS_DPM, to enable DPM settings 
> below
> + h->cpu_tag[3] = 0x;
> + h->cpu_tag[4] = BIT(dest_port) >> 16;
> + h->cpu_tag[5] = BIT(dest_port) & 0x;
> + // Set internal priority and AS_PRIO
> + h->cpu_tag[2] |= ((prio & 0x7) | BIT(3)) << 12;
>   }
>   
> -static void rtl839x_create_tx_header(struct p_hdr *h, int dest_port, int 
> prio)
> +static void rtl839x_create_tx_header(struct p_hdr *h, unsigned int 
> dest_port, unsigned int prio)
>   {
> - prio &= 0x7;
> -
> - if (dest_port > 0) {
> - // cpu_tag[0] is reserved on the RTL83XX SoCs
> - h->cpu_tag[1] = 0x0100; // RTL8390_CPU_TAG marker
> - h->cpu_tag[2] = h->cpu_tag[3] = h->cpu_tag[4] = h->cpu_tag[5] = 
> 0;
> - // h->cpu_tag[1] |= BIT(1) | BIT(0); // Bypass filter 1/2
> - if (dest_port >= 32) {
> - dest_port -= 32;
> - h->cpu_tag[2] = BIT(dest_port) >> 16;
> - h->cpu_tag[3] = BIT(dest_port) & 0x;
> - } else {
> - h->cpu_tag[4] = BIT(dest_port) >> 16;
> - h->cpu_tag[5] = BIT(dest_port) & 0x;
> - }
> - h->cpu_tag[2] |= BIT(5); // Enable destination port mask use
> - h->cpu_tag[2] |= BIT(8); // Enable L2 Learning
> - // Set internal priority and AS_PRIO
> - if (prio >= 0)
> - h->cpu_tag[1] |= prio | BIT(3);
> + // cpu_tag[0] is reserved on the RTL83XX SoCs
> + h->cpu_tag[1] = 0x0100; // RTL8390_CPU_TAG marker
> + h->cpu_tag[2] = h->cpu_tag[3] = h->cpu_tag[4] = h->cpu_tag[5] = 0;
> + // h->cpu_tag[1] |= BIT(1) | BIT(0); // Bypass filter 1/2
> + if (dest_port >= 32) {
> + dest_port -= 32;
> + h->cpu_tag[2] = BIT(dest_port) >> 16;
> + h->cpu_tag[3] = BIT(dest_port) & 0x;
> + } else {
> + h->cpu_tag[4] = BIT(dest_port) >> 16;
> + h->cpu_tag[5] = BIT(dest_port) & 0x;
>   }
> + h->cpu_tag[2] |= BIT(5); // Enable destination port mask use
> + h->cpu_tag[2] |= BIT(8); // Enable L2 Learning
> + // Set internal priority and AS_PRIO
> + h->cpu_tag[1] 

Re: [PATCH] realtek: correct egress frame port verification

[Birger Koblitz]

Hi,

On 19.06.22 10:56, Sander Vanheule wrote:

> - h->cpu_tag[1] = h->cpu_tag[2] = 0;
> - if (prio >= 0)
> - h->cpu_tag[2] = BIT(13) | prio << 8; // Enable and set Priority 
> Queue
> + h->cpu_tag[1] = 0;
> + /* Enable (AS_QID) and set Priority Queue (QID) */
> + h->cpu_tag[2] = (BIT(5) | (prio & 0x1f)) << 8;
You are removing the possibility to let the SoC choose a Queue on its own based 
on congestion,
if you always enable AS_QID. There was a reason that there were negative 
Queue-ID values, which
denoted allowing the SoC to choose, i.e. AS_QID was not set.

Cheers,
  Birger


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH] realtek: correct egress frame port verification

[Sander Vanheule]

Destination switch ports for outgoing frame can range from 0 to
CPU_PORT-1, and frame priorities should also always be positive.

Refactor the code to only generate egress frame CPU headers when the a
valid destination port number is available, and make the code a bit more
consistent between different switch generations. Change the dest_port
and prio argument types from 'int' to 'unsigned int', since only
positive values are valid.

This fixes the issue where egress frames on switch port 0 did not
receive a VLAN tag, because they are sent out without a CPU header.
Also fixes a potential issue with invalid (negative) egress port numbers
on RTL93xx switches.

Reported-by: Arınç ÜNAL 
Reported-by: Birger Koblitz 
Signed-off-by: Sander Vanheule 
---
 .../drivers/net/ethernet/rtl838x_eth.c| 86 +--
 .../drivers/net/ethernet/rtl838x_eth.h|  2 +-
 2 files changed, 40 insertions(+), 48 deletions(-)

diff --git a/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c 
b/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c
index cf6aabc6142f..241a5787f820 100644
--- a/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c
+++ b/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c
@@ -92,54 +92,44 @@ struct notify_b {
u32 reserved2[8];
 };
 
-static void rtl838x_create_tx_header(struct p_hdr *h, int dest_port, int prio)
+static void rtl838x_create_tx_header(struct p_hdr *h, unsigned int dest_port, 
unsigned int prio)
 {
-   prio &= 0x7;
-
-   if (dest_port > 0) {
-   // cpu_tag[0] is reserved on the RTL83XX SoCs
-   h->cpu_tag[1] = 0x0401;  // BIT 10: RTL8380_CPU_TAG, BIT0: 
L2LEARNING on
-   h->cpu_tag[2] = 0x0200;  // Set only AS_DPM, to enable DPM 
settings below
-   h->cpu_tag[3] = 0x;
-   h->cpu_tag[4] = BIT(dest_port) >> 16;
-   h->cpu_tag[5] = BIT(dest_port) & 0x;
-   // Set internal priority and AS_PRIO
-   if (prio >= 0)
-   h->cpu_tag[2] |= (prio | 0x8) << 12;
-   }
+   // cpu_tag[0] is reserved on the RTL83XX SoCs
+   h->cpu_tag[1] = 0x0401;  // BIT 10: RTL8380_CPU_TAG, BIT0: L2LEARNING on
+   h->cpu_tag[2] = 0x0200;  // Set only AS_DPM, to enable DPM settings 
below
+   h->cpu_tag[3] = 0x;
+   h->cpu_tag[4] = BIT(dest_port) >> 16;
+   h->cpu_tag[5] = BIT(dest_port) & 0x;
+   // Set internal priority and AS_PRIO
+   h->cpu_tag[2] |= ((prio & 0x7) | BIT(3)) << 12;
 }
 
-static void rtl839x_create_tx_header(struct p_hdr *h, int dest_port, int prio)
+static void rtl839x_create_tx_header(struct p_hdr *h, unsigned int dest_port, 
unsigned int prio)
 {
-   prio &= 0x7;
-
-   if (dest_port > 0) {
-   // cpu_tag[0] is reserved on the RTL83XX SoCs
-   h->cpu_tag[1] = 0x0100; // RTL8390_CPU_TAG marker
-   h->cpu_tag[2] = h->cpu_tag[3] = h->cpu_tag[4] = h->cpu_tag[5] = 
0;
-   // h->cpu_tag[1] |= BIT(1) | BIT(0); // Bypass filter 1/2
-   if (dest_port >= 32) {
-   dest_port -= 32;
-   h->cpu_tag[2] = BIT(dest_port) >> 16;
-   h->cpu_tag[3] = BIT(dest_port) & 0x;
-   } else {
-   h->cpu_tag[4] = BIT(dest_port) >> 16;
-   h->cpu_tag[5] = BIT(dest_port) & 0x;
-   }
-   h->cpu_tag[2] |= BIT(5); // Enable destination port mask use
-   h->cpu_tag[2] |= BIT(8); // Enable L2 Learning
-   // Set internal priority and AS_PRIO
-   if (prio >= 0)
-   h->cpu_tag[1] |= prio | BIT(3);
+   // cpu_tag[0] is reserved on the RTL83XX SoCs
+   h->cpu_tag[1] = 0x0100; // RTL8390_CPU_TAG marker
+   h->cpu_tag[2] = h->cpu_tag[3] = h->cpu_tag[4] = h->cpu_tag[5] = 0;
+   // h->cpu_tag[1] |= BIT(1) | BIT(0); // Bypass filter 1/2
+   if (dest_port >= 32) {
+   dest_port -= 32;
+   h->cpu_tag[2] = BIT(dest_port) >> 16;
+   h->cpu_tag[3] = BIT(dest_port) & 0x;
+   } else {
+   h->cpu_tag[4] = BIT(dest_port) >> 16;
+   h->cpu_tag[5] = BIT(dest_port) & 0x;
}
+   h->cpu_tag[2] |= BIT(5); // Enable destination port mask use
+   h->cpu_tag[2] |= BIT(8); // Enable L2 Learning
+   // Set internal priority and AS_PRIO
+   h->cpu_tag[1] |= (prio & 0x7) | BIT(3);
 }
 
-static void rtl930x_create_tx_header(struct p_hdr *h, int dest_port, int prio)
+static void rtl930x_create_tx_header(struct p_hdr *h, unsigned int dest_port, 
unsigned int prio)
 {
h->cpu_tag[0] = 0x8000;  // CPU tag marker
-   h->cpu_tag[1] = h->cpu_tag[2] = 0;
-   if (prio >= 0)
-   h->cpu_tag[2] = BIT(13) | prio << 8; // Enable and set Priority 
Queue
+   h->cpu_tag[1] = 0;
+   /* Enable