All of lore.kernel.org
 help / color / mirror / Atom feed
From: Horatiu Vultur <horatiu.vultur@microchip.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>,
	lkml <linux-kernel@vger.kernel.org>,
	netdev <netdev@vger.kernel.org>,
	<bridge@lists.linux-foundation.org>,
	"David S. Miller" <davem@davemloft.net>,
	Roopa Prabhu <roopa@cumulusnetworks.com>,
	Jakub Kicinski <jakub.kicinski@netronome.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Jeff Kirsher <jeffrey.t.kirsher@intel.com>,
	<anirudh.venkataramanan@intel.com>,
	David Ahern <dsahern@gmail.com>, "Jiri Pirko" <jiri@mellanox.com>,
	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
Subject: Re: [RFC net-next Patch 0/3] net: bridge: mrp: Add support for Media Redundancy Protocol(MRP)
Date: Fri, 10 Jan 2020 18:25:36 +0100	[thread overview]
Message-ID: <20200110172536.42rdfwdc6eiwsw7m@soft-dev3.microsemi.net> (raw)
In-Reply-To: <CA+h21hrq7U4EdqSgpYQRjK8rkcJdvD5jXCSOH_peA-R4xCocTg@mail.gmail.com>


Hi Valdimir and Andrew

The 01/10/2020 18:21, Vladimir Oltean wrote:
> I think it would help your case if you explained a bit more about the
> hw offload primitives you have implemented internally. I believe you
> are talking about the frame generation engine in the Ocelot switch
> which has 1024 frame slots that are periodically sent based on one of
> 8 timers. For receive, I believe that the functionality is to offload
> the consumption of these periodic frames, and just raise an interrupt
> if frames were expected but not received.
Yes something like this. But it is worth mention that it is not just about
injecting frames, sequence number needs to be incremented (by HW) etc.

> For your use case of MRP, it makes perfect sense to have this. I am
> just not sure (and not knowledgeable enough in Linux) what this engine
> is offloading from the operating system's perspective.
We will try to make that more clear.

> Your justification for implementing MRP in the kernel seems to be that
> it's better to make MRP part of the kernel uapi than a configuration
> interface for your periodic engine, which in principle I agree with.
> I'm just not sure if the offload that you propose will have a trivial
> path into the kernel either, so it would make sense for reviewers to
> see everything put together first.
You are right. The decision of start by publishing a pure SW implementation with
no HW offload was not the best.

I can do a new RFC that does including the HW offload hooks, and
describe what configurations we do when these hooks are called. The
actual HW which implements these hooks is still not released (and the
SwitchDev driver for this device is still not submitted).

> Horatiu, could you also give some references to the frames that need
> to be sent. I've no idea what information they need to contain, if the
> contents is dynamic, or static, etc.
It is dynamic - but trivial... Here is a dump from WireShark with
annotation on what our HW can update:

Ethernet II, Src: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1), Dst: Iec_00:00:01 (01:15:4e:00:00:01)
    Destination: Iec_00:00:01 (01:15:4e:00:00:01)
    Source: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1)
    Type: MRP (0x88e3)
PROFINET MRP MRP_Test, MRP_Common, MRP_End
    MRP_Version: 1
    MRP_TLVHeader.Type: MRP_Test (0x02)
        MRP_TLVHeader.Type: MRP_Test (0x02)
        MRP_TLVHeader.Length: 18
        MRP_Prio: 0x1f40 High priorities
        MRP_SA: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1)
        MRP_PortRole: Primary ring port (0x0000)
        MRP_RingState: Ring closed (0x0001)
        MRP_Transition: 0x0001
        MRP_TimeStamp [ms]: 0x000cf574             <---------- Updated automatic
    MRP_TLVHeader.Type: MRP_Common (0x01)
        MRP_TLVHeader.Type: MRP_Common (0x01)
        MRP_TLVHeader.Length: 18
        MRP_SequenceID: 0x00e9                     <---------- Updated automatic
        MRP_DomainUUID: ffffffff-ffff-ffff-ffff-ffffffffffff
    MRP_TLVHeader.Type: MRP_End (0x00)
        MRP_TLVHeader.Type: MRP_End (0x00)
        MRP_TLVHeader.Length: 0

But all the fields can change, but to change the other fields we need to
interact with the HW. Other SoC may have other capabilities in their
offload. As an example, if the ring becomes open then the fields
MRP_RingState and MRP_Transition need to change and in our case this
requires SW interference.

Would you like a PCAP file as an example? Or do you want a better
description of the frame format.

/Horatiu

WARNING: multiple messages have this Message-ID (diff)
From: Horatiu Vultur <horatiu.vultur@microchip.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Jakub Kicinski <jakub.kicinski@netronome.com>,
	Nikolay Aleksandrov <nikolay@cumulusnetworks.com>,
	netdev <netdev@vger.kernel.org>,
	Roopa Prabhu <roopa@cumulusnetworks.com>,
	bridge@lists.linux-foundation.org,
	lkml <linux-kernel@vger.kernel.org>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	anirudh.venkataramanan@intel.com, Jiri Pirko <jiri@mellanox.com>,
	Jeff Kirsher <jeffrey.t.kirsher@intel.com>,
	David Ahern <dsahern@gmail.com>,
	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [Bridge] [RFC net-next Patch 0/3] net: bridge: mrp: Add support for Media Redundancy Protocol(MRP)
Date: Fri, 10 Jan 2020 18:25:36 +0100	[thread overview]
Message-ID: <20200110172536.42rdfwdc6eiwsw7m@soft-dev3.microsemi.net> (raw)
In-Reply-To: <CA+h21hrq7U4EdqSgpYQRjK8rkcJdvD5jXCSOH_peA-R4xCocTg@mail.gmail.com>


Hi Valdimir and Andrew

The 01/10/2020 18:21, Vladimir Oltean wrote:
> I think it would help your case if you explained a bit more about the
> hw offload primitives you have implemented internally. I believe you
> are talking about the frame generation engine in the Ocelot switch
> which has 1024 frame slots that are periodically sent based on one of
> 8 timers. For receive, I believe that the functionality is to offload
> the consumption of these periodic frames, and just raise an interrupt
> if frames were expected but not received.
Yes something like this. But it is worth mention that it is not just about
injecting frames, sequence number needs to be incremented (by HW) etc.

> For your use case of MRP, it makes perfect sense to have this. I am
> just not sure (and not knowledgeable enough in Linux) what this engine
> is offloading from the operating system's perspective.
We will try to make that more clear.

> Your justification for implementing MRP in the kernel seems to be that
> it's better to make MRP part of the kernel uapi than a configuration
> interface for your periodic engine, which in principle I agree with.
> I'm just not sure if the offload that you propose will have a trivial
> path into the kernel either, so it would make sense for reviewers to
> see everything put together first.
You are right. The decision of start by publishing a pure SW implementation with
no HW offload was not the best.

I can do a new RFC that does including the HW offload hooks, and
describe what configurations we do when these hooks are called. The
actual HW which implements these hooks is still not released (and the
SwitchDev driver for this device is still not submitted).

> Horatiu, could you also give some references to the frames that need
> to be sent. I've no idea what information they need to contain, if the
> contents is dynamic, or static, etc.
It is dynamic - but trivial... Here is a dump from WireShark with
annotation on what our HW can update:

Ethernet II, Src: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1), Dst: Iec_00:00:01 (01:15:4e:00:00:01)
    Destination: Iec_00:00:01 (01:15:4e:00:00:01)
    Source: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1)
    Type: MRP (0x88e3)
PROFINET MRP MRP_Test, MRP_Common, MRP_End
    MRP_Version: 1
    MRP_TLVHeader.Type: MRP_Test (0x02)
        MRP_TLVHeader.Type: MRP_Test (0x02)
        MRP_TLVHeader.Length: 18
        MRP_Prio: 0x1f40 High priorities
        MRP_SA: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1)
        MRP_PortRole: Primary ring port (0x0000)
        MRP_RingState: Ring closed (0x0001)
        MRP_Transition: 0x0001
        MRP_TimeStamp [ms]: 0x000cf574             <---------- Updated automatic
    MRP_TLVHeader.Type: MRP_Common (0x01)
        MRP_TLVHeader.Type: MRP_Common (0x01)
        MRP_TLVHeader.Length: 18
        MRP_SequenceID: 0x00e9                     <---------- Updated automatic
        MRP_DomainUUID: ffffffff-ffff-ffff-ffff-ffffffffffff
    MRP_TLVHeader.Type: MRP_End (0x00)
        MRP_TLVHeader.Type: MRP_End (0x00)
        MRP_TLVHeader.Length: 0

But all the fields can change, but to change the other fields we need to
interact with the HW. Other SoC may have other capabilities in their
offload. As an example, if the ring becomes open then the fields
MRP_RingState and MRP_Transition need to change and in our case this
requires SW interference.

Would you like a PCAP file as an example? Or do you want a better
description of the frame format.

/Horatiu

  parent reply	other threads:[~2020-01-10 17:25 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-09 15:06 [RFC net-next Patch 0/3] net: bridge: mrp: Add support for Media Redundancy Protocol(MRP) Horatiu Vultur
2020-01-09 15:06 ` [Bridge] " Horatiu Vultur
2020-01-09 15:06 ` [RFC net-next Patch 1/3] net: bridge: mrp: Add support for Media Redundancy Protocol Horatiu Vultur
2020-01-09 15:06   ` [Bridge] " Horatiu Vultur
2020-01-09 15:06 ` [RFC net-next Patch 2/3] net: bridge: mrp: Integrate MRP into the bridge Horatiu Vultur
2020-01-09 15:06   ` [Bridge] " Horatiu Vultur
2020-01-09 15:06 ` [RFC net-next Patch 3/3] net: bridge: mrp: Add netlink support to configure MRP Horatiu Vultur
2020-01-09 15:06   ` [Bridge] " Horatiu Vultur
2020-01-10 14:27   ` kbuild test robot
2020-01-10 14:52   ` kbuild test robot
2020-01-09 16:19 ` [RFC net-next Patch 0/3] net: bridge: mrp: Add support for Media Redundancy Protocol(MRP) Stephen Hemminger
2020-01-09 16:19   ` [Bridge] " Stephen Hemminger
2020-01-09 17:41   ` Asbjørn Sloth Tønnesen
2020-01-09 17:41     ` Asbjørn Sloth Tønnesen
2020-01-10  9:02   ` Horatiu Vultur
2020-01-10  9:02     ` [Bridge] " Horatiu Vultur
2020-01-10 15:36     ` Stephen Hemminger
2020-01-10 15:36       ` [Bridge] " Stephen Hemminger
2020-01-10 14:13 ` Nikolay Aleksandrov
2020-01-10 14:13   ` [Bridge] " Nikolay Aleksandrov
2020-01-10 15:38   ` Nikolay Aleksandrov
2020-01-10 15:38     ` [Bridge] " Nikolay Aleksandrov
2020-01-10 16:04   ` Horatiu Vultur
2020-01-10 16:04     ` [Bridge] " Horatiu Vultur
2020-01-10 16:21     ` Vladimir Oltean
2020-01-10 16:21       ` [Bridge] " Vladimir Oltean
2020-01-10 16:48       ` Andrew Lunn
2020-01-10 16:48         ` [Bridge] " Andrew Lunn
2020-01-10 17:25       ` Horatiu Vultur [this message]
2020-01-10 17:25         ` Horatiu Vultur
2020-01-10 17:56         ` Andrew Lunn
2020-01-10 17:56           ` [Bridge] " Andrew Lunn
2020-01-10 20:12           ` Horatiu Vultur
2020-01-10 20:12             ` [Bridge] " Horatiu Vultur
2020-01-10 20:33             ` Andrew Lunn
2020-01-10 20:33               ` [Bridge] " Andrew Lunn
2020-01-10 19:27   ` David Miller
2020-01-10 19:27     ` [Bridge] " David Miller
2020-01-10 20:03     ` nikolay
2020-01-10 20:03       ` [Bridge] " nikolay
2020-01-10 20:24       ` Horatiu Vultur
2020-01-10 20:24         ` [Bridge] " Horatiu Vultur

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200110172536.42rdfwdc6eiwsw7m@soft-dev3.microsemi.net \
    --to=horatiu.vultur@microchip.com \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=andrew@lunn.ch \
    --cc=anirudh.venkataramanan@intel.com \
    --cc=bridge@lists.linux-foundation.org \
    --cc=davem@davemloft.net \
    --cc=dsahern@gmail.com \
    --cc=jakub.kicinski@netronome.com \
    --cc=jeffrey.t.kirsher@intel.com \
    --cc=jiri@mellanox.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=nikolay@cumulusnetworks.com \
    --cc=olteanv@gmail.com \
    --cc=roopa@cumulusnetworks.com \
    --cc=vivien.didelot@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.