Re: [net-next PATCH 0/8] configuration support for switch headers & phy

Re: [net-next PATCH 0/8] configuration support for switch headers & phy

[Hariprasad Kelam]

Hi Andrew ,

Please see inline,


> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Sunday, March 21, 2021 7:45 PM
> To: Hariprasad Kelam <hkelam@marvell.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; kuba@kernel.org;
> davem@davemloft.net; Sunil Kovvuri Goutham <sgoutham@marvell.com>;
> Linu Cherian <lcherian@marvell.com>; Geethasowjanya Akula
> <gakula@marvell.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Subbaraya Sundeep Bhatta <sbhatta@marvell.com>
> Subject: [EXT] Re: [net-next PATCH 0/8] configuration support for switch
> headers & phy
> 
> On Sun, Mar 21, 2021 at 05:39:50PM +0530, Hariprasad Kelam wrote:
> > This series of patches add support for parsing switch headers and
> > configuration support for phy modulation type(NRZ or PAM4).
> >
> > PHYs that support changing modulation type ,user can configure it
> > through private flags pam4.
> >
> > Marvell switches support DSA(distributed switch architecture) with
> > different switch headers like FDSA and EDSA. This patch series adds
> > private flags to enable user to configure interface in fdsa/edsa mode
> > such that flow steering (forwading packets to pf/vf depending on
> > switch header fields) and packet parsing can be acheived.
> 
> Hi Hariprasad
> 
> Private flags sound very wrong here. I would expect to see some integration
> between the switchdev/DSA driver and the MAC driver.
> Please show how this works in combination with drivers/net/dsa/mv88e6xxx
> or drivers/net/ethernet/marvell/prestera.
> 
	Octeontx2 silicon supports NPC (network parser and cam) unit , through which packet parsing and packet classification is achieved.
              Packet parsing extracting different fields from each layer.
				  DMAC + SMAC  --> LA
		                               VLAN ID --> LB
		                               SIP + DIP --> LC
                                                            TCP SPORT + DPORT --> LD
    And packet classification is achieved through  flow identification in key extraction and mcam search key . User can install mcam rules
    With action as  
		forward packet to PF and to receive  queue 0
		forward packet to VF and  with as RSS ( Receive side scaling)
		drop the packet 
		etc..

   Now with switch header ( EDSA /FDSA) and HIGIG2 appended to regular packet , NPC can not parse these
   Ingress packets as these headers does not have fixed headers. To achieve this Special PKIND( port kind) is allocated in hardware
   which will help NPC to parse the packets.

 For example incase of EDSA 8 byte header which is placed right after SMAC , special PKIND reserved for EDSA helps NPC to 
 Identify the  input packet is EDSA . Such that NPC can extract fields in this header and forward to 
 Parse rest of the headers.

 Same is the case with higig2 header where 16 bytes header is placed at start of the packet.

In this case private flags helps user to configure interface in EDSA/FDSA or HIGIG2. Such that special
PKIND reserved for that header are assigned to the interface.  The scope of the patch series is how
User can configure interface mode as switch header(HIGIG2/EDSA etc) .In our case no DSA logical
Ports are created as these headers can be stripped by NPC.

Thanks,
Hariprasad k
 


> 	  Andrew

Re: [net-next PATCH 0/8] configuration support for switch headers & phy

[Andrew Lunn]

On Tue, Mar 23, 2021 at 06:13:28PM +0000, Hariprasad Kelam wrote:
> 
> Hi Andrew ,
> 
> Please see inline,

No need to say that. That is the correct way to right emails.

> > Hi Hariprasad
> > 
> > Private flags sound very wrong here. I would expect to see some integration
> > between the switchdev/DSA driver and the MAC driver.
> > Please show how this works in combination with drivers/net/dsa/mv88e6xxx
> > or drivers/net/ethernet/marvell/prestera.
> > 
> 	Octeontx2 silicon supports NPC (network parser and cam) unit , through which packet parsing and packet classification is achieved.
>               Packet parsing extracting different fields from each layer.
> 				  DMAC + SMAC  --> LA
> 		                               VLAN ID --> LB
> 		                               SIP + DIP --> LC
>                                                             TCP SPORT + DPORT --> LD
>     And packet classification is achieved through  flow identification in key extraction and mcam search key . User can install mcam rules
>     With action as  
> 		forward packet to PF and to receive  queue 0
> 		forward packet to VF and  with as RSS ( Receive side scaling)
> 		drop the packet 
> 		etc..
> 
>    Now with switch header ( EDSA /FDSA) and HIGIG2 appended to regular packet , NPC can not parse these
>    Ingress packets as these headers does not have fixed headers. To achieve this Special PKIND( port kind) is allocated in hardware
>    which will help NPC to parse the packets.
> 
>  For example incase of EDSA 8 byte header which is placed right after SMAC , special PKIND reserved for EDSA helps NPC to 
>  Identify the  input packet is EDSA . Such that NPC can extract fields in this header and forward to 
>  Parse rest of the headers.
> 
>  Same is the case with higig2 header where 16 bytes header is placed at start of the packet.
> 
> In this case private flags helps user to configure interface in EDSA/FDSA or HIGIG2. Such that special
> PKIND reserved for that header are assigned to the interface.  The scope of the patch series is how
> User can configure interface mode as switch header(HIGIG2/EDSA etc) .In our case no DSA logical
> Ports are created as these headers can be stripped by NPC.

So you completely skipped how this works with mv88e6xxx or
prestera. If you need this private flag for some out of mainline
Marvell SDK, it is very unlikely to be accepted.

	Andrew

Re: [net-next PATCH 0/8] configuration support for switch headers & phy

[Sunil Kovvuri]

> > > Hi Hariprasad
> > >
> > > Private flags sound very wrong here. I would expect to see some integration
> > > between the switchdev/DSA driver and the MAC driver.
> > > Please show how this works in combination with drivers/net/dsa/mv88e6xxx
> > > or drivers/net/ethernet/marvell/prestera.
> > >
> >       Octeontx2 silicon supports NPC (network parser and cam) unit , through which packet parsing and packet classification is achieved.
> >               Packet parsing extracting different fields from each layer.
> >                                 DMAC + SMAC  --> LA
> >                                              VLAN ID --> LB
> >                                              SIP + DIP --> LC
> >                                                             TCP SPORT + DPORT --> LD
> >     And packet classification is achieved through  flow identification in key extraction and mcam search key . User can install mcam rules
> >     With action as
> >               forward packet to PF and to receive  queue 0
> >               forward packet to VF and  with as RSS ( Receive side scaling)
> >               drop the packet
> >               etc..
> >
> >    Now with switch header ( EDSA /FDSA) and HIGIG2 appended to regular packet , NPC can not parse these
> >    Ingress packets as these headers does not have fixed headers. To achieve this Special PKIND( port kind) is allocated in hardware
> >    which will help NPC to parse the packets.
> >
> >  For example incase of EDSA 8 byte header which is placed right after SMAC , special PKIND reserved for EDSA helps NPC to
> >  Identify the  input packet is EDSA . Such that NPC can extract fields in this header and forward to
> >  Parse rest of the headers.
> >
> >  Same is the case with higig2 header where 16 bytes header is placed at start of the packet.
> >
> > In this case private flags helps user to configure interface in EDSA/FDSA or HIGIG2. Such that special
> > PKIND reserved for that header are assigned to the interface.  The scope of the patch series is how
> > User can configure interface mode as switch header(HIGIG2/EDSA etc) .In our case no DSA logical
> > Ports are created as these headers can be stripped by NPC.
>
> So you completely skipped how this works with mv88e6xxx or
> prestera. If you need this private flag for some out of mainline
> Marvell SDK, it is very unlikely to be accepted.
>
>         Andrew

What we are trying to do here has no dependency on DSA drivers and
neither impacts that functionality.
Here we are just notifying the HW to parse the packets properly.

Thanks,
Sunil.

Re: [net-next PATCH 0/8] configuration support for switch headers & phy

[Andrew Lunn]

> > So you completely skipped how this works with mv88e6xxx or
> > prestera. If you need this private flag for some out of mainline
> > Marvell SDK, it is very unlikely to be accepted.
> >
> >         Andrew
> 
> What we are trying to do here has no dependency on DSA drivers and
> neither impacts that functionality.

So this is an indirect way of saying: Yes, this is for some out of
mainline Marvell SDK.

> Here we are just notifying the HW to parse the packets properly.

But the correct way for this to happen is probably some kernel
internal API between the MAC and the DSA driver. Mainline probably has
no need for this private flag.

   Andrew

Re: [net-next PATCH 0/8] configuration support for switch headers & phy

[Sunil Kovvuri]

On Thu, Mar 25, 2021 at 6:20 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > So you completely skipped how this works with mv88e6xxx or
> > > prestera. If you need this private flag for some out of mainline
> > > Marvell SDK, it is very unlikely to be accepted.
> > >
> > >         Andrew
> >
> > What we are trying to do here has no dependency on DSA drivers and
> > neither impacts that functionality.
>
> So this is an indirect way of saying: Yes, this is for some out of
> mainline Marvell SDK.
>
> > Here we are just notifying the HW to parse the packets properly.
>
> But the correct way for this to happen is probably some kernel
> internal API between the MAC and the DSA driver. Mainline probably has
> no need for this private flag.
>
>    Andrew

Didn't get why you say so.
HW expects some info from SW to do the packet parsing properly and
this is specific to this hardware.
How can we generalize this ?
It's not just the DSA tags, the requirement is also for packets with
Higig header ie when system is connected to a switch
which appends Higig2 header to all pkts.

Thanks,
Sunil.

Re: [net-next PATCH 0/8] configuration support for switch headers & phy

[Andrew Lunn]

On Thu, Mar 25, 2021 at 06:32:12PM +0530, Sunil Kovvuri wrote:
> On Thu, Mar 25, 2021 at 6:20 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > > So you completely skipped how this works with mv88e6xxx or
> > > > prestera. If you need this private flag for some out of mainline
> > > > Marvell SDK, it is very unlikely to be accepted.
> > > >
> > > >         Andrew
> > >
> > > What we are trying to do here has no dependency on DSA drivers and
> > > neither impacts that functionality.
> >
> > So this is an indirect way of saying: Yes, this is for some out of
> > mainline Marvell SDK.
> >
> > > Here we are just notifying the HW to parse the packets properly.
> >
> > But the correct way for this to happen is probably some kernel
> > internal API between the MAC and the DSA driver. Mainline probably has
> > no need for this private flag.
> >
> >    Andrew
> 
> Didn't get why you say so.
> HW expects some info from SW to do the packet parsing properly and
> this is specific to this hardware.

Anything which needs out of mainline code is going to be rejected,
unless you can show there is an in mainline use case as well. Which is
why i keep pointing you to mv88e6xxx and prestira. That provides the
necessary {E}DSA tags. You have an mv88e6xxx and prestira switch being
controlled by Linux with DSA tagged frames flowing out of it into your
MAC driver. What is the big picture use case that requires this
private flag to enable DSA parsing? Why don't the MAC driver and the
DSA driver just talk to each other, and setup the parsing?

	Andrew

Re: [net-next PATCH 0/8] configuration support for switch headers & phy

[Sunil Kovvuri]

On Thu, Mar 25, 2021 at 6:51 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Mar 25, 2021 at 06:32:12PM +0530, Sunil Kovvuri wrote:
> > On Thu, Mar 25, 2021 at 6:20 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > > So you completely skipped how this works with mv88e6xxx or
> > > > > prestera. If you need this private flag for some out of mainline
> > > > > Marvell SDK, it is very unlikely to be accepted.
> > > > >
> > > > >         Andrew
> > > >
> > > > What we are trying to do here has no dependency on DSA drivers and
> > > > neither impacts that functionality.
> > >
> > > So this is an indirect way of saying: Yes, this is for some out of
> > > mainline Marvell SDK.
> > >
> > > > Here we are just notifying the HW to parse the packets properly.
> > >
> > > But the correct way for this to happen is probably some kernel
> > > internal API between the MAC and the DSA driver. Mainline probably has
> > > no need for this private flag.
> > >
> > >    Andrew
> >
> > Didn't get why you say so.
> > HW expects some info from SW to do the packet parsing properly and
> > this is specific to this hardware.
>
> Anything which needs out of mainline code is going to be rejected,
> unless you can show there is an in mainline use case as well. Which is
> why i keep pointing you to mv88e6xxx and prestira. That provides the
> necessary {E}DSA tags. You have an mv88e6xxx and prestira switch being
> controlled by Linux with DSA tagged frames flowing out of it into your
> MAC driver. What is the big picture use case that requires this
> private flag to enable DSA parsing? Why don't the MAC driver and the
> DSA driver just talk to each other, and setup the parsing?
>
>         Andrew

The usecase is simple, unlike DSA tag, this 4byte FDSA tag doesn't
have a ethertype,
so HW cannot recognize this header. If such packers arise, then HW parsing will
fail and RSS will not work.

Hypothetically if we introduce some communication between MAC driver
and DSA driver,
wouldn't that also become specific to the device, what generic usecase
that communication
will have ?

Thanks,
Sunil.

Re: [net-next PATCH 0/8] configuration support for switch headers & phy

[Andrew Lunn]

> The usecase is simple, unlike DSA tag, this 4byte FDSA tag doesn't
> have a ethertype,
> so HW cannot recognize this header. If such packers arise, then HW parsing will
> fail and RSS will not work.
> 
> Hypothetically if we introduce some communication between MAC driver
> and DSA driver,
> wouldn't that also become specific to the device, what generic usecase
> that communication
> will have ?

Hi Sunil

We need to be careful with wording. Due to history, the Linux kernel
uses dsa to mean any driver to control an Ethernet switch. It does not
imply the {E}DSA protocol used by Marvell switches, or even that the
switch is a Marvell switch.

netdev_uses_dsa(ndev) will tell you if the MAC is being used to
connect to a switch. It is set by the Linux DSA core when the switch
cluster is setup. That could be before or after the MAC is configured
up, which makes it a bit hard to use, since you don't have a clear
indicator when to evaluate to determine if you need to change your
packet parsing.

netdev_uses_dsa() looks at ndev->dsa_ptr. This is a pointer to the
structure which represents the port on the switch the MAC is connected
to. In Linux DSA terms, this is the CPU port. You can follow
dsa_ptr->tag_ops which gives you the tagger operations, i.e. those
used to add and remove the header/trailer. One member of that is
proto. This contains the tagging protocol, so EDSA, DSA, or
potentially FDSA, if that is ever supported. And this is all within
the core DSA code, so is generic. It should work for any tagging
protocol used by any switch which Linux DSA supports.

So actually, everything you need is already present, you don't need a
private flag. But adding a notifier that the MAC has been connected to
a switch and ndev->dsa_ptr is set would be useful. We could maybe use
NETDEV_CHANGE for that, or NETDEV_CHANGELOWERSTATE, since the MAC is
below the switch slave interfaces.

      Andrew

Re: [net-next PATCH 0/8] configuration support for switch headers & phy

[Hariprasad Kelam]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Sunday, March 28, 2021 11:17 PM
> To: Sunil Kovvuri <sunil.kovvuri@gmail.com>
> Cc: Hariprasad Kelam <hkelam@marvell.com>; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; kuba@kernel.org; davem@davemloft.net;
> Sunil Kovvuri Goutham <sgoutham@marvell.com>; Linu Cherian
> <lcherian@marvell.com>; Geethasowjanya Akula <gakula@marvell.com>;
> Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Subbaraya Sundeep Bhatta
> <sbhatta@marvell.com>
> Subject: [EXT] Re: [net-next PATCH 0/8] configuration support for switch
> headers & phy
> 
> > The usecase is simple, unlike DSA tag, this 4byte FDSA tag doesn't
> > have a ethertype, so HW cannot recognize this header. If such packers
> > arise, then HW parsing will fail and RSS will not work.
> >
> > Hypothetically if we introduce some communication between MAC driver
> > and DSA driver, wouldn't that also become specific to the device, what
> > generic usecase that communication will have ?
> 
> Hi Sunil
> 
> We need to be careful with wording. Due to history, the Linux kernel uses
> dsa to mean any driver to control an Ethernet switch. It does not imply the
> {E}DSA protocol used by Marvell switches, or even that the switch is a
> Marvell switch.
> 
> netdev_uses_dsa(ndev) will tell you if the MAC is being used to connect to a
> switch. It is set by the Linux DSA core when the switch cluster is setup. That
> could be before or after the MAC is configured up, which makes it a bit hard
> to use, since you don't have a clear indicator when to evaluate to determine
> if you need to change your packet parsing.
> 
> netdev_uses_dsa() looks at ndev->dsa_ptr. This is a pointer to the structure
> which represents the port on the switch the MAC is connected to. In Linux
> DSA terms, this is the CPU port. You can follow dsa_ptr->tag_ops which gives
> you the tagger operations, i.e. those used to add and remove the
> header/trailer. One member of that is proto. This contains the tagging
> protocol, so EDSA, DSA, or potentially FDSA, if that is ever supported. And
> this is all within the core DSA code, so is generic. It should work for any
> tagging protocol used by any switch which Linux DSA supports.
> 
> So actually, everything you need is already present, you don't need a private
> flag. But adding a notifier that the MAC has been connected to a switch and
> ndev->dsa_ptr is set would be useful. We could maybe use NETDEV_CHANGE
> for that, or NETDEV_CHANGELOWERSTATE, since the MAC is below the
> switch slave interfaces.


Hi Andrew,
	We are looking into  DSA to MAC driver communication options, will get back once we have clear picture.

Thanks,
Hariprasad k 
> 
>       Andrew

[net-next PATCH 0/8] configuration support for switch headers & phy

[Hariprasad Kelam]

This series of patches add support for parsing switch headers and
configuration support for phy modulation type(NRZ or PAM4).

PHYs that support changing modulation type ,user can configure it
through private flags pam4.

Marvell switches support DSA(distributed switch architecture) with
different switch headers like FDSA and EDSA. This patch series adds
private flags to enable user to configure interface in fdsa/edsa
mode such that flow steering (forwading packets to pf/vf depending on
switch header fields) and packet parsing can be acheived.

Also adds support for HIGIG2 protocol, user can configure interface
in higig mode through higig private flage, such that packet classification
and flow sterring achieved on packets with higig header


Felix Manlunas (2):
  octeontx2-af: Add new CGX_CMDs to set and get PHY modulation type
  octeontx2-pf: Add ethtool priv flag to control PAM4 on/off

Hariprasad Kelam (6):
  octeontx2-af: Support for parsing pkts with switch headers
  octeontx2-af: Do not allow VFs to overwrite PKIND config
  octeontx2-af: Put CGX LMAC also in Higig2 mode
  octeontx2-pf: Support to enable EDSA/Higig2 pkts parsing
  octeontx2-af: Add flow steering support for FDSA tag
  octeontx2-pf: Add ntuple filter support for FDSA

 .../net/ethernet/marvell/octeontx2/af/cgx.c   | 177 ++++++++++++-
 .../net/ethernet/marvell/octeontx2/af/cgx.h   |  19 +-
 .../ethernet/marvell/octeontx2/af/cgx_fw_if.h |   6 +
 .../net/ethernet/marvell/octeontx2/af/mbox.h  |  39 ++-
 .../net/ethernet/marvell/octeontx2/af/npc.h   |  14 +-
 .../net/ethernet/marvell/octeontx2/af/rvu.h   |   9 +
 .../ethernet/marvell/octeontx2/af/rvu_cgx.c   | 103 +++++++-
 .../marvell/octeontx2/af/rvu_debugfs.c        |   3 +-
 .../ethernet/marvell/octeontx2/af/rvu_nix.c   |  44 +++-
 .../ethernet/marvell/octeontx2/af/rvu_npc.c   |  76 ++++++
 .../marvell/octeontx2/af/rvu_npc_fs.c         |  14 +-
 .../marvell/octeontx2/nic/otx2_common.c       |   2 +
 .../marvell/octeontx2/nic/otx2_common.h       |  36 ++-
 .../marvell/octeontx2/nic/otx2_ethtool.c      | 244 ++++++++++++++++++
 .../marvell/octeontx2/nic/otx2_flows.c        |  58 ++++-
 .../ethernet/marvell/octeontx2/nic/otx2_pf.c  |  21 +-
 .../marvell/octeontx2/nic/otx2_txrx.c         |   1 +
 .../ethernet/marvell/octeontx2/nic/otx2_vf.c  |  10 +
 18 files changed, 832 insertions(+), 44 deletions(-)

--
2.17.1

Re: [net-next PATCH 0/8] configuration support for switch headers & phy

[Andrew Lunn]

On Sun, Mar 21, 2021 at 05:39:50PM +0530, Hariprasad Kelam wrote:
> This series of patches add support for parsing switch headers and
> configuration support for phy modulation type(NRZ or PAM4).
> 
> PHYs that support changing modulation type ,user can configure it
> through private flags pam4.
> 
> Marvell switches support DSA(distributed switch architecture) with
> different switch headers like FDSA and EDSA. This patch series adds
> private flags to enable user to configure interface in fdsa/edsa
> mode such that flow steering (forwading packets to pf/vf depending on
> switch header fields) and packet parsing can be acheived.

Hi Hariprasad 

Private flags sound very wrong here. I would expect to see some
integration between the switchdev/DSA driver and the MAC driver.
Please show how this works in combination with
drivers/net/dsa/mv88e6xxx or drivers/net/ethernet/marvell/prestera.

	  Andrew