Date:   Thu, 20 Feb 2020 17:56:47 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     "Allan W. Nielsen" <allan.nielsen@...rochip.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Joergen Andreasen <joergen.andreasen@...rochip.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        netdev <netdev@...r.kernel.org>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net-next] net: mscc: ocelot: Workaround to allow traffic
 to CPU in standalone mode

On Thu, 20 Feb 2020 at 15:23, Allan W. Nielsen
<allan.nielsen@...rochip.com> wrote:
>
> Horatiu and I have looked further into this, done a few experiments, and
> discussed with the HW engineers who have a more detailed version of how
> the chips are working and how Ocelot and Felix differs.
>
> Here are our findings:
>
> - The most significant bit in the PGID table is "special" as it is a
>    CPU-copy bit.

Wow.
Looking at the code after this realization, it is so confusing to call
the NPI port "ocelot->cpu" now, since it doesn't benefit from this
"privilege" that a "real" CPU port (from the Ocelot hardware
perspective) has.
Not to mention how strange it is for the hardware to behave this way.

> - This bit is not being used in the source filtering!

So BIT(6) on Felix and BIT(11) on Ocelot are just being interpreted
for the first and second PGID lookup (destination and aggregation
masks) but not for source masks? Don't you want to actually document
this somewhere?

So the frame is copied to the CPU based on the AND between first and
second lookup, and to all the other ports, including the NPI port,
based on the first, second and third PGID lookup.
So frames can reach the NPI port directly, via 3 PGID lookups, or
indirectly, via 2 PGID lookups. What I did is make the direct path
work. You're suggesting me to set the indirect mode up.

> This means that
>    your original patch can be applied without breaking Ocelot (the
>    uninitialized cpu field must be fixed though).

So my patch makes the Felix NPI port work in an unintended way and
does not affect Ocelot in any way. Roger.

>    - Still I do not think we should do this as it is not the root-casuse
> - In Felix we have 2 ways to get frames to the CPU, in Ocelot we have 1
>    (Ocelot also has two if it uses an NPI port, but it does not do that
>    in the current driver).
>    - In Felix you can get frames to the CPU by either using the CPU port
>      (port 6), or by using the NPI port (which can be any in the range of
>      0-5).
>      - But you should only use the CPU port, and not the NPI port
>        directly. Using the NPI port directly will cause the two targets
>        to behave differently, and this is not what we do when testing all
>        the use-cases on the switch.

Differently in what way?

>    - In Ocelot you can only get frames to the CPU by using the CPU port
>      (port 11).
>
> Due to this, I very much think you need to fix this, such that Felix
> always port 6 to reach the CPU (with the exception of writing
> QSYS_EXT_CPU_CFG where you "connect" the CPU queue/port to the NPI
> port).
>

What PGIDs should I use for the NPI port if I use it with indirection
via port 6?

> If you do this change, then the Ocelot and Felix should start to work in
> the same way.
>
> Then, if you want the CPU to be part of the unicast flooding (this is
> where this discussion started), then you should add the CPU port to the
> PGID entry pointed at in ANA:ANA:FLOODING:FLD_UNICAST. This should be
> done for Felix and not for Ocelot.

No, the question is why don't _you_ want the CPU to be in the
FLD_UNICAST PGID (which is PGID_UC). The distinction you're making
between Felix and Ocelot here is quite arbitrary, and seems to be
based just on "your CPU is more powerful".

So right now, multicast and broadcast traffic goes to PGID_MC (61),
and unknown unicast goes to PGID_UC (60).
The destination ports mask for PGID_MC is
GENMASK(ocelot->num_phys_ports, 0) - 0x7f for me, 0xfff for you.
The destination ports mask for PGID_UC is
GENMASK(ocelot->num_phys_ports - 1, 0) - this is the hardware default
value - 0x3f for me, 0x7ff for you.
So you are keeping the non-physical CPU port in the destination mask
for broadcast, but not in that for unknown unicast. In the way the
system is configured, it is still susceptible to broadcast storms. So
I don't think there is any real benefit in crippling the system like
this. If port 6 was in PGID_UC by default, I would have never bat an
eye and more than likely never needed to know the gory details.

Is it possible to set up policers for traffic going to CPU? I _did_
see this phrase already in the manual:

"Frames where the DMAC lookup returned a PGID with the CPU port set
are always forwarded to the CPU even when the frame
is policed by the storm policers."

So the traffic I'm seeing now on the NPI port is not copied to the
CPU, it is forwarded.
If there's no other way to set up storm policers for the mode you're
suggesting me to change Felix to, then I would respectfully keep it
the way it is right now.

>
> If you want the analyser (where the MAC table sits), to "learn" the CPU
> MAC (which is needed if you do not want to have the CPU mac as a static
> entry in the MAC-table), then you need to set the 'src-port' to 6 (if it
> is Ocelot then it will be 11) in the IFH:
>

Please tell me more about this. Don't I need to set BYPASS=1 for
frames to go on the front-panel port specified in DEST? And doesn't
BYPASS=1 mean no source MAC learning for injected traffic?
As much as I would like the analyzer to run, I won't do it if it is
going to compromise xmit from Linux. I don't want traffic sent from
Linux to an unknown unicast in standalone mode to be flooded to all
front-panel ports with no way for me to control it.

> anielsen@...anielsen ~ $ ef hex ifh-oc1 help
> ifh-oc1          Injection Frame Header for Ocelot1
>
> Specify the ifh-oc1 header by using one or more of the following fields:
> - Name ------------ offset:width --- Description --------------------------
>    bypass              +  0:  1  Skip analyzer processing
>    b1-rew-mac          +  1:  1  Replace SMAC address
>    b1-rew-op           +  2:  9  Rewriter operation command
>    b0-masq             +  1:  1  Enable masquerading
>    b0-masq-port        +  2:  4  Masquerading port
>    rew-val             + 11: 32  Receive time stamp
>    res1                + 43: 17  Reserved
>    dest                + 60: 12  Destination set for the frame. Dest[11] is the CPU
>    res2                + 72:  9  Reserved
>    src-port            + 81:  4  The port number where the frame was injected (0-12)  <------------------- THIS FIELD
>    res3                + 85:  2  Reserved
>    trfm-timer          + 87:  4  Timer for periodic transmissions (1..8). If zero then normal injection
>    res4                + 91:  6  Reserved
>    dp                  + 97:  1  Drop precedence level after policing
>    pop-cnt             + 98:  2  Number of VLAN tags that must be popped
>    cpuq                +100:  8  CPU extraction queue mask
>    qos-class           +108:  3  Classified QoS class
>    tag-type            +111:  1  Tag information's associated Tag Protocol Identifier (TPID)
>    pcp                 +112:  3  Classified PCP
>    dei                 +115:  1  Classified DEI
>    vid                 +116: 12  Classified VID
>
>
> /Allan
>

Thanks,
-Vladimir

Powered by blists - more mailing lists