[Resend PATCH net] bridge: uapi: mrp: Fix MRP_PORT_ROLE

[Resend PATCH net] bridge: uapi: mrp: Fix MRP_PORT_ROLE

[Horatiu Vultur]

Currently the MRP_PORT_ROLE_NONE has the value 0x2 but this is in conflict
with the IEC 62439-2 standard. The standard defines the following port
roles: primary (0x0), secondary(0x1), interconnect(0x2).
Therefore remove the port role none.

Fixes: 4714d13791f831 ("bridge: uapi: mrp: Add mrp attributes.")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 include/uapi/linux/mrp_bridge.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h
index 84f15f48a7cb1..bee3665402129 100644
--- a/include/uapi/linux/mrp_bridge.h
+++ b/include/uapi/linux/mrp_bridge.h
@@ -36,7 +36,6 @@ enum br_mrp_port_state_type {
 enum br_mrp_port_role_type {
 	BR_MRP_PORT_ROLE_PRIMARY,
 	BR_MRP_PORT_ROLE_SECONDARY,
-	BR_MRP_PORT_ROLE_NONE,
 };
 
 enum br_mrp_tlv_header_type {
-- 
2.26.2

Re: [Resend PATCH net] bridge: uapi: mrp: Fix MRP_PORT_ROLE

[David Miller]

From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: Sat, 20 Jun 2020 15:14:03 +0200

> Currently the MRP_PORT_ROLE_NONE has the value 0x2 but this is in conflict
> with the IEC 62439-2 standard. The standard defines the following port
> roles: primary (0x0), secondary(0x1), interconnect(0x2).
> Therefore remove the port role none.
> 
> Fixes: 4714d13791f831 ("bridge: uapi: mrp: Add mrp attributes.")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

The code accepts arbitrary 32-bit values for the role in a configuration
but only PRIMARY and SECONDARY seem to be valid.

There is no validation that the value used makes sense.

In the future if we handle type interconnect, and we add checks, it will
break any existing applications.  Because they can validly pass any
non-zero valid and the code treats that as SECONDARY currently.

So you really can't just remove NONE, you have to add validation code
too so we don't run into problem in the future.

Thanks.

Re: [Resend PATCH net] bridge: uapi: mrp: Fix MRP_PORT_ROLE

[Horatiu Vultur]

The 06/22/2020 16:07, David Miller wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Horatiu Vultur <horatiu.vultur@microchip.com>
> Date: Sat, 20 Jun 2020 15:14:03 +0200
> 
> > Currently the MRP_PORT_ROLE_NONE has the value 0x2 but this is in conflict
> > with the IEC 62439-2 standard. The standard defines the following port
> > roles: primary (0x0), secondary(0x1), interconnect(0x2).
> > Therefore remove the port role none.
> >
> > Fixes: 4714d13791f831 ("bridge: uapi: mrp: Add mrp attributes.")
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> 
> The code accepts arbitrary 32-bit values for the role in a configuration
> but only PRIMARY and SECONDARY seem to be valid.
> 
> There is no validation that the value used makes sense.
> 
> In the future if we handle type interconnect, and we add checks, it will
> break any existing applications.  Because they can validly pass any
> non-zero valid and the code treats that as SECONDARY currently.
> 
> So you really can't just remove NONE, you have to add validation code
> too so we don't run into problem in the future.

Thanks for the explanation. I will add some code that checks
specifically for primary(0x0) and secondary(0x1) values and for any
other value to return -EINVAL.
Then in the future when we handle the type interconnect(0x2), we will
just extend this code to check for this value.

> 
> Thanks.

-- 
/Horatiu