[PATCH net 0/2] MRP without hardware offload?

[PATCH net 0/2] MRP without hardware offload?

[Rasmus Villemoes]

Hi Horatiu and net folks

I'm having quite some trouble getting MRP working in a simple setup
involving three mv88e6250 switches in a ring, with one node set as
manager and the other two as clients.

I'm reasonably confident these two patches are necessary and correct
(though the second one affects quite a bit more than MRP, so comments
welcome), but they are not sufficient - for example, I'm wondering
about why there doesn't seem to be any code guarding against sending a
test packet back out the port it came in.

I have tried applying a few more patches, but since the end result
still doesn't seem to result in a working MRP setup, I'm a bit out of
ideas, and not proposing any of those yet.

Has anyone managed to set up an MRP ring with no hardware offload
support? I'm using commit 9030e898a2f232fdb4a3b2ec5e91fa483e31eeaf
from https://github.com/microchip-ung/mrp.git and kernel v5.10.2.

Rasmus Villemoes (2):
  net: mrp: fix definitions of MRP test packets
  net: switchdev: don't set port_obj_info->handled true when -EOPNOTSUPP

 include/uapi/linux/mrp_bridge.h |  4 ++--
 net/switchdev/switchdev.c       | 23 +++++++++++++----------
 2 files changed, 15 insertions(+), 12 deletions(-)

-- 
2.23.0

[PATCH net 1/2] net: mrp: fix definitions of MRP test packets

[Rasmus Villemoes]

Wireshark says that the MRP test packets cannot be decoded - and the
reason for that is that there's a two-byte hole filled with garbage
between the "transitions" and "timestamp" members.

So Wireshark decodes the two garbage bytes and the top two bytes of
the timestamp written by the kernel as the timestamp value (which thus
fluctuates wildly), and interprets the lower two bytes of the
timestamp as a new (type, length) pair, which is of course broken.

While my copy of the MRP standard is still under way [*], I cannot
imagine the standard specifying a two-byte hole here, and whoever
wrote the Wireshark decoding code seems to agree with that.

The struct definitions live under include/uapi/, but they are not
really part of any kernel<->userspace API/ABI, so fixing the
definitions by adding the packed attribute should not cause any
compatibility issues.

The remaining on-the-wire packet formats likely also don't contain
holes, but pahole and manual inspection says the current definitions
suffice. So adding the packed attribute to those is not strictly
needed, but might be done for good measure.

[*] I will never understand how something hidden behind a +1000$
paywall can be called a standard.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 include/uapi/linux/mrp_bridge.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h
index 6aeb13ef0b1e..d1d0cf65916d 100644
--- a/include/uapi/linux/mrp_bridge.h
+++ b/include/uapi/linux/mrp_bridge.h
@@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
 	__be16 state;
 	__be16 transitions;
 	__be32 timestamp;
-};
+} __attribute__((__packed__));
 
 struct br_mrp_ring_topo_hdr {
 	__be16 prio;
@@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr {
 	__be16 state;
 	__be16 transitions;
 	__be32 timestamp;
-};
+} __attribute__((__packed__));
 
 struct br_mrp_in_topo_hdr {
 	__u8 sa[ETH_ALEN];
-- 
2.23.0

Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets

[Horatiu Vultur]

The 12/23/2020 15:45, Rasmus Villemoes wrote:

Hi Rasmus,
> 
> Wireshark says that the MRP test packets cannot be decoded - and the
> reason for that is that there's a two-byte hole filled with garbage
> between the "transitions" and "timestamp" members.
> 
> So Wireshark decodes the two garbage bytes and the top two bytes of
> the timestamp written by the kernel as the timestamp value (which thus
> fluctuates wildly), and interprets the lower two bytes of the
> timestamp as a new (type, length) pair, which is of course broken.
> 
> While my copy of the MRP standard is still under way [*], I cannot
> imagine the standard specifying a two-byte hole here, and whoever
> wrote the Wireshark decoding code seems to agree with that.
> 
> The struct definitions live under include/uapi/, but they are not
> really part of any kernel<->userspace API/ABI, so fixing the
> definitions by adding the packed attribute should not cause any
> compatibility issues.
> 
> The remaining on-the-wire packet formats likely also don't contain
> holes, but pahole and manual inspection says the current definitions
> suffice. So adding the packed attribute to those is not strictly
> needed, but might be done for good measure.
> 
> [*] I will never understand how something hidden behind a +1000$
> paywall can be called a standard.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  include/uapi/linux/mrp_bridge.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h
> index 6aeb13ef0b1e..d1d0cf65916d 100644
> --- a/include/uapi/linux/mrp_bridge.h
> +++ b/include/uapi/linux/mrp_bridge.h
> @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
>         __be16 state;
>         __be16 transitions;
>         __be32 timestamp;
> -};
> +} __attribute__((__packed__));

Yes, I agree that this should be packed but it also needs to be 32 bit
alligned, so extra 2 bytes are needed.
The same will apply also for 'br_mrp_ring_topo_hdr'

> 
>  struct br_mrp_ring_topo_hdr {
>         __be16 prio;
> @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr {
>         __be16 state;
>         __be16 transitions;
>         __be32 timestamp;
> -};
> +} __attribute__((__packed__));
> 
>  struct br_mrp_in_topo_hdr {
>         __u8 sa[ETH_ALEN];
> --
> 2.23.0
> 

-- 
/Horatiu

Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets

[Andrew Lunn]

> > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
> >         __be16 state;
> >         __be16 transitions;
> >         __be32 timestamp;
> > -};
> > +} __attribute__((__packed__));
> 
> Yes, I agree that this should be packed but it also needs to be 32 bit
> alligned, so extra 2 bytes are needed.

The full structure is:

struct br_mrp_ring_test_hdr {
	__be16 prio;	
	__u8 sa[ETH_ALEN];
	__be16 port_role;
	__be16 state;
	__be16 transitions;
	__be32 timestamp;
};

If i'm looking at this correctly, the byte offsets are:

0-1   prio
2-7   sa
8-9   port_role
10-11 state
12-13 transition

With packed you get

14-17 timestamp

which is not 32 bit aligned.

Do you mean the whole structure must be 32 bit aligned? We need to add
two reserved bytes to the end of the structure?

    Andrew

Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets

[Horatiu Vultur]

Hi Andrew,

The 12/23/2020 19:41, Andrew Lunn wrote:
> 
> > > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
> > >         __be16 state;
> > >         __be16 transitions;
> > >         __be32 timestamp;
> > > -};
> > > +} __attribute__((__packed__));
> >
> > Yes, I agree that this should be packed but it also needs to be 32 bit
> > alligned, so extra 2 bytes are needed.
> 
> The full structure is:
> 
> struct br_mrp_ring_test_hdr {
>         __be16 prio;
>         __u8 sa[ETH_ALEN];
>         __be16 port_role;
>         __be16 state;
>         __be16 transitions;
>         __be32 timestamp;
> };
> 
> If i'm looking at this correctly, the byte offsets are:
> 
> 0-1   prio
> 2-7   sa
> 8-9   port_role
> 10-11 state
> 12-13 transition
> 
> With packed you get
> 
> 14-17 timestamp
> 
> which is not 32 bit aligned.
> 
> Do you mean the whole structure must be 32 bit aligned? We need to add
> two reserved bytes to the end of the structure?

Sorry, I looked too fast over this. First some info, in front of the
'br_mrp_ring_test_hdr', there is 'br_mrp_tlv_hdr' which is 2
bytes. So the frame will look something like this:

... |---------|----------------|----------------------|------------| ....
... | MRP Ver | br_mrp_tlv_hdr | br_mrp_ring_test_hdr | Common TLV | ....
... |---------|----------------|----------------------|------------| ....

The standard says that for br_mrp_tlv_hdr + br_mrp_ring_test, 32 bit
alignment shall be ensured. So my understanding was that it needs to be
at word boundary(4 bytes).

So based on this, if we use packed then we get the following offsets
0	type
1	length
2-3	prio
4-9	sa
10-11	port_role
12-13	state
14-15	transition
16-19	timestamp.

Which is 20 bytes, that fits correctly. So my understanding is we need to
use packed, to remove the hole between transition and timestamp as
Rasmus suggested and should NOT use these 2 extra bytes that I
mentioned because it would not be aligned anymore.

Here you can find few more details about MRP[1]

> 
>     Andrew

[1] https://www.youtube.com/watch?v=R3vQYfwiJ2M

-- 
/Horatiu

Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets

[Jakub Kicinski]

On Wed, 23 Dec 2020 15:45:32 +0100 Rasmus Villemoes wrote:
> Wireshark says that the MRP test packets cannot be decoded - and the
> reason for that is that there's a two-byte hole filled with garbage
> between the "transitions" and "timestamp" members.
> 
> So Wireshark decodes the two garbage bytes and the top two bytes of
> the timestamp written by the kernel as the timestamp value (which thus
> fluctuates wildly), and interprets the lower two bytes of the
> timestamp as a new (type, length) pair, which is of course broken.
> 
> While my copy of the MRP standard is still under way [*], I cannot
> imagine the standard specifying a two-byte hole here, and whoever
> wrote the Wireshark decoding code seems to agree with that.
> 
> The struct definitions live under include/uapi/, but they are not
> really part of any kernel<->userspace API/ABI, so fixing the
> definitions by adding the packed attribute should not cause any
> compatibility issues.
> 
> The remaining on-the-wire packet formats likely also don't contain
> holes, but pahole and manual inspection says the current definitions
> suffice. So adding the packed attribute to those is not strictly
> needed, but might be done for good measure.
> 
> [*] I will never understand how something hidden behind a +1000$
> paywall can be called a standard.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  include/uapi/linux/mrp_bridge.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h
> index 6aeb13ef0b1e..d1d0cf65916d 100644
> --- a/include/uapi/linux/mrp_bridge.h
> +++ b/include/uapi/linux/mrp_bridge.h
> @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
>  	__be16 state;
>  	__be16 transitions;
>  	__be32 timestamp;
> -};
> +} __attribute__((__packed__));
>  
>  struct br_mrp_ring_topo_hdr {
>  	__be16 prio;
> @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr {
>  	__be16 state;
>  	__be16 transitions;
>  	__be32 timestamp;
> -};
> +} __attribute__((__packed__));
>  
>  struct br_mrp_in_topo_hdr {
>  	__u8 sa[ETH_ALEN];

Can we use this opportunity to move the definitions of these structures
out of the uAPI to a normal kernel header?

Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets

[Horatiu Vultur]

The 12/28/2020 14:24, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, 23 Dec 2020 15:45:32 +0100 Rasmus Villemoes wrote:
> > Wireshark says that the MRP test packets cannot be decoded - and the
> > reason for that is that there's a two-byte hole filled with garbage
> > between the "transitions" and "timestamp" members.
> >
> > So Wireshark decodes the two garbage bytes and the top two bytes of
> > the timestamp written by the kernel as the timestamp value (which thus
> > fluctuates wildly), and interprets the lower two bytes of the
> > timestamp as a new (type, length) pair, which is of course broken.
> >
> > While my copy of the MRP standard is still under way [*], I cannot
> > imagine the standard specifying a two-byte hole here, and whoever
> > wrote the Wireshark decoding code seems to agree with that.
> >
> > The struct definitions live under include/uapi/, but they are not
> > really part of any kernel<->userspace API/ABI, so fixing the
> > definitions by adding the packed attribute should not cause any
> > compatibility issues.
> >
> > The remaining on-the-wire packet formats likely also don't contain
> > holes, but pahole and manual inspection says the current definitions
> > suffice. So adding the packed attribute to those is not strictly
> > needed, but might be done for good measure.
> >
> > [*] I will never understand how something hidden behind a +1000$
> > paywall can be called a standard.
> >
> > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > ---
> >  include/uapi/linux/mrp_bridge.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h
> > index 6aeb13ef0b1e..d1d0cf65916d 100644
> > --- a/include/uapi/linux/mrp_bridge.h
> > +++ b/include/uapi/linux/mrp_bridge.h
> > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
> >       __be16 state;
> >       __be16 transitions;
> >       __be32 timestamp;
> > -};
> > +} __attribute__((__packed__));
> >
> >  struct br_mrp_ring_topo_hdr {
> >       __be16 prio;
> > @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr {
> >       __be16 state;
> >       __be16 transitions;
> >       __be32 timestamp;
> > -};
> > +} __attribute__((__packed__));
> >
> >  struct br_mrp_in_topo_hdr {
> >       __u8 sa[ETH_ALEN];
> 
> Can we use this opportunity to move the definitions of these structures
> out of the uAPI to a normal kernel header?

Or maybe we can just remove them, especially if they are not used by the
kernel.

-- 
/Horatiu

Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets

[Rasmus Villemoes]

On 28/12/2020 23.24, Jakub Kicinski wrote:
> On Wed, 23 Dec 2020 15:45:32 +0100 Rasmus Villemoes wrote:
>> Wireshark says that the MRP test packets cannot be decoded - and the
>> reason for that is that there's a two-byte hole filled with garbage
>> between the "transitions" and "timestamp" members.
>>
>> So Wireshark decodes the two garbage bytes and the top two bytes of
>> the timestamp written by the kernel as the timestamp value (which thus
>> fluctuates wildly), and interprets the lower two bytes of the
>> timestamp as a new (type, length) pair, which is of course broken.
>>
>> While my copy of the MRP standard is still under way [*], I cannot
>> imagine the standard specifying a two-byte hole here, and whoever
>> wrote the Wireshark decoding code seems to agree with that.
>>
>> The struct definitions live under include/uapi/, but they are not
>> really part of any kernel<->userspace API/ABI, so fixing the
>> definitions by adding the packed attribute should not cause any
>> compatibility issues.
>>
>> The remaining on-the-wire packet formats likely also don't contain
>> holes, but pahole and manual inspection says the current definitions
>> suffice. So adding the packed attribute to those is not strictly
>> needed, but might be done for good measure.
>>
>> [*] I will never understand how something hidden behind a +1000$
>> paywall can be called a standard.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>  include/uapi/linux/mrp_bridge.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h
>> index 6aeb13ef0b1e..d1d0cf65916d 100644
>> --- a/include/uapi/linux/mrp_bridge.h
>> +++ b/include/uapi/linux/mrp_bridge.h
>> @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
>>  	__be16 state;
>>  	__be16 transitions;
>>  	__be32 timestamp;
>> -};
>> +} __attribute__((__packed__));
>>  
>>  struct br_mrp_ring_topo_hdr {
>>  	__be16 prio;
>> @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr {
>>  	__be16 state;
>>  	__be16 transitions;
>>  	__be32 timestamp;
>> -};
>> +} __attribute__((__packed__));
>>  
>>  struct br_mrp_in_topo_hdr {
>>  	__u8 sa[ETH_ALEN];
> 
> Can we use this opportunity to move the definitions of these structures
> out of the uAPI to a normal kernel header?
> 

Jakub, can we apply this patch to net, then later move the definitions
out of uapi (and deleting the unused ones in the process)?

Thanks,
Rasmus



-- 
Rasmus Villemoes
Software Developer
Prevas A/S
Hedeager 3
DK-8200 Aarhus N
+45 51210274
rasmus.villemoes@prevas.dk
www.prevas.dk

Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets

[Jakub Kicinski]

On Tue, 19 Jan 2021 16:42:14 +0100 Rasmus Villemoes wrote:
> On 28/12/2020 23.24, Jakub Kicinski wrote:
> > On Wed, 23 Dec 2020 15:45:32 +0100 Rasmus Villemoes wrote:  
> >> Wireshark says that the MRP test packets cannot be decoded - and the
> >> reason for that is that there's a two-byte hole filled with garbage
> >> between the "transitions" and "timestamp" members.
> >>
> >> So Wireshark decodes the two garbage bytes and the top two bytes of
> >> the timestamp written by the kernel as the timestamp value (which thus
> >> fluctuates wildly), and interprets the lower two bytes of the
> >> timestamp as a new (type, length) pair, which is of course broken.
> >>
> >> While my copy of the MRP standard is still under way [*], I cannot
> >> imagine the standard specifying a two-byte hole here, and whoever
> >> wrote the Wireshark decoding code seems to agree with that.
> >>
> >> The struct definitions live under include/uapi/, but they are not
> >> really part of any kernel<->userspace API/ABI, so fixing the
> >> definitions by adding the packed attribute should not cause any
> >> compatibility issues.
> >>
> >> The remaining on-the-wire packet formats likely also don't contain
> >> holes, but pahole and manual inspection says the current definitions
> >> suffice. So adding the packed attribute to those is not strictly
> >> needed, but might be done for good measure.
> >>
> >> [*] I will never understand how something hidden behind a +1000$
> >> paywall can be called a standard.
> >>
> >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >> ---
> >>  include/uapi/linux/mrp_bridge.h | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h
> >> index 6aeb13ef0b1e..d1d0cf65916d 100644
> >> --- a/include/uapi/linux/mrp_bridge.h
> >> +++ b/include/uapi/linux/mrp_bridge.h
> >> @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
> >>  	__be16 state;
> >>  	__be16 transitions;
> >>  	__be32 timestamp;
> >> -};
> >> +} __attribute__((__packed__));
> >>  
> >>  struct br_mrp_ring_topo_hdr {
> >>  	__be16 prio;
> >> @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr {
> >>  	__be16 state;
> >>  	__be16 transitions;
> >>  	__be32 timestamp;
> >> -};
> >> +} __attribute__((__packed__));
> >>  
> >>  struct br_mrp_in_topo_hdr {
> >>  	__u8 sa[ETH_ALEN];  
> > 
> > Can we use this opportunity to move the definitions of these structures
> > out of the uAPI to a normal kernel header?
> 
> Jakub, can we apply this patch to net, then later move the definitions
> out of uapi (and deleting the unused ones in the process)?

This has been lost in the patchwork archives already, we'll need a
fresh posting anyway. Why not do the move as a part of the same series?
Doesn't seems like much work, unless I'm missing something..

[PATCH net 2/2] net: switchdev: don't set port_obj_info->handled true when -EOPNOTSUPP

[Rasmus Villemoes]

It's not true that switchdev_port_obj_notify() only inspects the
->handled field of "struct switchdev_notifier_port_obj_info" if
call_switchdev_blocking_notifiers() returns 0 - there's a WARN_ON()
triggering for a non-zero return combined with ->handled not being
true. But the real problem here is that -EOPNOTSUPP is not being
properly handled.

The wrapper functions switchdev_handle_port_obj_add() et al change a
return value of -EOPNOTSUPP to 0, and the treatment of ->handled in
switchdev_port_obj_notify() seems to be designed to change that back
to -EOPNOTSUPP in case nobody actually acted on the notifier (i.e.,
everybody returned -EOPNOTSUPP).

Currently, as soon as some device down the stack passes the check_cb()
check, ->handled gets set to true, which means that
switchdev_port_obj_notify() cannot actually ever return -EOPNOTSUPP.

This, for example, means that the detection of hardware offload
support in the MRP code is broken - br_mrp_set_ring_role() always ends
up setting mrp->ring_role_offloaded to 1, despite not a single
mainline driver implementing any of the SWITCHDEV_OBJ_ID*_MRP. So
since the MRP code thinks the generation of MRP test frames has been
offloaded, no such frames are actually put on the wire.

So, continue to set ->handled true if any callback returns success or
any error distinct from -EOPNOTSUPP. But if all the callbacks return
-EOPNOTSUPP, make sure that ->handled stays false, so the logic in
switchdev_port_obj_notify() can propagate that information.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 net/switchdev/switchdev.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 23d868545362..2c1ffc9ba2eb 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -460,10 +460,11 @@ static int __switchdev_handle_port_obj_add(struct net_device *dev,
 	extack = switchdev_notifier_info_to_extack(&port_obj_info->info);
 
 	if (check_cb(dev)) {
-		/* This flag is only checked if the return value is success. */
-		port_obj_info->handled = true;
-		return add_cb(dev, port_obj_info->obj, port_obj_info->trans,
-			      extack);
+		err = add_cb(dev, port_obj_info->obj, port_obj_info->trans,
+			     extack);
+		if (err != -EOPNOTSUPP)
+			port_obj_info->handled = true;
+		return err;
 	}
 
 	/* Switch ports might be stacked under e.g. a LAG. Ignore the
@@ -515,9 +516,10 @@ static int __switchdev_handle_port_obj_del(struct net_device *dev,
 	int err = -EOPNOTSUPP;
 
 	if (check_cb(dev)) {
-		/* This flag is only checked if the return value is success. */
-		port_obj_info->handled = true;
-		return del_cb(dev, port_obj_info->obj);
+		err = del_cb(dev, port_obj_info->obj);
+		if (err != -EOPNOTSUPP)
+			port_obj_info->handled = true;
+		return err;
 	}
 
 	/* Switch ports might be stacked under e.g. a LAG. Ignore the
@@ -568,9 +570,10 @@ static int __switchdev_handle_port_attr_set(struct net_device *dev,
 	int err = -EOPNOTSUPP;
 
 	if (check_cb(dev)) {
-		port_attr_info->handled = true;
-		return set_cb(dev, port_attr_info->attr,
-			      port_attr_info->trans);
+		err = set_cb(dev, port_attr_info->attr, port_attr_info->trans);
+		if (err != -EOPNOTSUPP)
+			port_attr_info->handled = true;
+		return err;
 	}
 
 	/* Switch ports might be stacked under e.g. a LAG. Ignore the
-- 
2.23.0

Re: [PATCH net 2/2] net: switchdev: don't set port_obj_info->handled true when -EOPNOTSUPP

[Jakub Kicinski]

On Wed, 23 Dec 2020 15:45:33 +0100 Rasmus Villemoes wrote:
> It's not true that switchdev_port_obj_notify() only inspects the
> ->handled field of "struct switchdev_notifier_port_obj_info" if  
> call_switchdev_blocking_notifiers() returns 0 - there's a WARN_ON()
> triggering for a non-zero return combined with ->handled not being
> true. But the real problem here is that -EOPNOTSUPP is not being
> properly handled.
> 
> The wrapper functions switchdev_handle_port_obj_add() et al change a
> return value of -EOPNOTSUPP to 0, and the treatment of ->handled in
> switchdev_port_obj_notify() seems to be designed to change that back
> to -EOPNOTSUPP in case nobody actually acted on the notifier (i.e.,
> everybody returned -EOPNOTSUPP).
> 
> Currently, as soon as some device down the stack passes the check_cb()
> check, ->handled gets set to true, which means that
> switchdev_port_obj_notify() cannot actually ever return -EOPNOTSUPP.
> 
> This, for example, means that the detection of hardware offload
> support in the MRP code is broken - br_mrp_set_ring_role() always ends
> up setting mrp->ring_role_offloaded to 1, despite not a single
> mainline driver implementing any of the SWITCHDEV_OBJ_ID*_MRP. So
> since the MRP code thinks the generation of MRP test frames has been
> offloaded, no such frames are actually put on the wire.
> 
> So, continue to set ->handled true if any callback returns success or
> any error distinct from -EOPNOTSUPP. But if all the callbacks return
> -EOPNOTSUPP, make sure that ->handled stays false, so the logic in
> switchdev_port_obj_notify() can propagate that information.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Please make sure you CC the folks who may have something to say about
this - Jiri, Ivan, Ido, Florian, etc.

Re: [PATCH net 0/2] MRP without hardware offload?

[Horatiu Vultur]

The 12/23/2020 15:45, Rasmus Villemoes wrote:
> 
> Hi Horatiu and net folks

Hi Rasmus,

> 
> I'm having quite some trouble getting MRP working in a simple setup
> involving three mv88e6250 switches in a ring, with one node set as
> manager and the other two as clients.
> 
> I'm reasonably confident these two patches are necessary and correct
> (though the second one affects quite a bit more than MRP, so comments
> welcome), but they are not sufficient - for example, I'm wondering
> about why there doesn't seem to be any code guarding against sending a
> test packet back out the port it came in.
> 
> I have tried applying a few more patches, but since the end result
> still doesn't seem to result in a working MRP setup, I'm a bit out of
> ideas, and not proposing any of those yet.
> 
> Has anyone managed to set up an MRP ring with no hardware offload
> support? I'm using commit 9030e898a2f232fdb4a3b2ec5e91fa483e31eeaf
> from https://github.com/microchip-ung/mrp.git and kernel v5.10.2.

I was expecting that you still need to do something in the switchdev
callbacks. Because otherwise I expect that the HW will flood these
frames. For a client I was expecting to add a MDB entry and have the
ring ports in this entry. While for a manager you can have also an MDB
where the host joined and could return -EOPNOTSUPP so then the SW will
detect when it stops receiving these frames.

Most of my tests where done when there was not HW offload at all(no
switchdev) or when there was MRP hardware offload.

> 
> Rasmus Villemoes (2):
>   net: mrp: fix definitions of MRP test packets
>   net: switchdev: don't set port_obj_info->handled true when -EOPNOTSUPP
> 
>  include/uapi/linux/mrp_bridge.h |  4 ++--
>  net/switchdev/switchdev.c       | 23 +++++++++++++----------
>  2 files changed, 15 insertions(+), 12 deletions(-)
> 
> --
> 2.23.0
> 

-- 
/Horatiu