Allow non "disabled" state for !netif_oper_up() links

Allow non "disabled" state for !netif_oper_up() links

[Rodolfo Giometti]

This patch allows non "disabled" state even if the link is UP and runnign
but with no carrier.

I'm trying to implement a new MRP daemon in userspace and when a MRM detects
that one of its ring connections has no carrier, the normative states that
such link shall be set as BLOCKED (or LISTENING for Linux bridges). Current
code blocks this operation.

I think kernel code should implement mechanisms and not policies!

Rodolfo Giometti

---

 br_netlink.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

[PATCH] net br_netlink.c:y allow non "disabled" state for !netif_oper_up() links

[Rodolfo Giometti]

A generic loop-free network protocol (such as STP or MRP and others) may
require that a link not in an operational state be into a non "disabled"
state (such as listening).

For example MRP states that a MRM should set into a "BLOCKED" state (which is
equivalent to the LISTENING state for Linux bridges) one of its ring
connection if it detects that this connection is "DOWN" (that is the
NO-CARRIER status).

Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
---
 net/bridge/br_netlink.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 5c6c4305ed23..3f9f45c3d274 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -841,11 +841,8 @@ static int br_set_port_state(struct net_bridge_port *p, u8 state)
 	if (p->br->stp_enabled == BR_KERNEL_STP)
 		return -EBUSY;
 
-	/* if device is not up, change is not allowed
-	 * if link is not present, only allowable state is disabled
-	 */
-	if (!netif_running(p->dev) ||
-	    (!netif_oper_up(p->dev) && state != BR_STATE_DISABLED))
+	/* if device is not up, change is not allowed */
+	if (!netif_running(p->dev))
 		return -ENETDOWN;
 
 	br_set_state(p, state);
-- 
2.34.1

Re: [PATCH] net br_netlink.c:y allow non "disabled" state for !netif_oper_up() links

[Andrew Lunn]

On Wed, Nov 09, 2022 at 04:24:10PM +0100, Rodolfo Giometti wrote:
> A generic loop-free network protocol (such as STP or MRP and others) may
> require that a link not in an operational state be into a non "disabled"
> state (such as listening).
> 
> For example MRP states that a MRM should set into a "BLOCKED" state (which is
> equivalent to the LISTENING state for Linux bridges) one of its ring
> connection if it detects that this connection is "DOWN" (that is the
> NO-CARRIER status).

Does MRP explain Why?

This change seems odd, and "Because the standard says so" is not the
best of explanations.

     Andrew

Re: [PATCH] net br_netlink.c:y allow non "disabled" state for !netif_oper_up() links

[Rodolfo Giometti]

On 09/11/22 18:34, Andrew Lunn wrote:
> On Wed, Nov 09, 2022 at 04:24:10PM +0100, Rodolfo Giometti wrote:
>> A generic loop-free network protocol (such as STP or MRP and others) may
>> require that a link not in an operational state be into a non "disabled"
>> state (such as listening).
>>
>> For example MRP states that a MRM should set into a "BLOCKED" state (which is
>> equivalent to the LISTENING state for Linux bridges) one of its ring
>> connection if it detects that this connection is "DOWN" (that is the
>> NO-CARRIER status).
> 
> Does MRP explain Why?
> 
> This change seems odd, and "Because the standard says so" is not the
> best of explanations.

A MRM instance has two ports: primary port (PRM_RPort) and secondary port 
(SEC_RPort).

When both ports are UP (that is the CARRIER is on) the MRM is into the 
Ring_closed state and the PRM_RPort is in forwarding state while the SEC_RPort 
is in blocking state (remember that MRP blocking is equal to Linux bridge 
listening).

If the PRM_RPort losts its carrier and the link goes down the normative states that:

- ports role swap (PRM_RPort becomes SEC_RPort and vice versa).

- SEC_RPort must be set into blocking state.

- PRM_RPort must be set into forwarding state.

Then the MRM moves into a new state called Primary-UP. In this state, when the 
SEC_RPort returns to UP state (that is the CARRIER is up) it's returns into the 
Ring_closed state where both ports have the right status, that is the PRM_RPort 
is in forwarding state while the SEC_RPort is in blocking state.

This is just an example of one single case, but consider that, in general, when 
the carrier is lost the port state is moved into blocking so that when the 
carrier returns the port it's already into the right state.

Hope it's clearer now.

However, despite this special case, I think that kernel code should implement 
mechanisms and not policies, shouldn't it? If user space needs a non operational 
port (that is with no carrier) into the listening state, why we should prevent it?

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: [PATCH] net br_netlink.c:y allow non "disabled" state for !netif_oper_up() links

[Andrew Lunn]

On Wed, Nov 09, 2022 at 07:19:22PM +0100, Rodolfo Giometti wrote:
> On 09/11/22 18:34, Andrew Lunn wrote:
> > On Wed, Nov 09, 2022 at 04:24:10PM +0100, Rodolfo Giometti wrote:
> > > A generic loop-free network protocol (such as STP or MRP and others) may
> > > require that a link not in an operational state be into a non "disabled"
> > > state (such as listening).
> > > 
> > > For example MRP states that a MRM should set into a "BLOCKED" state (which is
> > > equivalent to the LISTENING state for Linux bridges) one of its ring
> > > connection if it detects that this connection is "DOWN" (that is the
> > > NO-CARRIER status).
> > 
> > Does MRP explain Why?
> > 
> > This change seems odd, and "Because the standard says so" is not the
> > best of explanations.
> 
> A MRM instance has two ports: primary port (PRM_RPort) and secondary port
> (SEC_RPort).
> 
> When both ports are UP (that is the CARRIER is on) the MRM is into the
> Ring_closed state and the PRM_RPort is in forwarding state while the
> SEC_RPort is in blocking state (remember that MRP blocking is equal to Linux
> bridge listening).
> 
> If the PRM_RPort losts its carrier and the link goes down the normative states that:
> 
> - ports role swap (PRM_RPort becomes SEC_RPort and vice versa).
> 
> - SEC_RPort must be set into blocking state.
> 
> - PRM_RPort must be set into forwarding state.
> 
> Then the MRM moves into a new state called Primary-UP. In this state, when
> the SEC_RPort returns to UP state (that is the CARRIER is up) it's returns
> into the Ring_closed state where both ports have the right status, that is
> the PRM_RPort is in forwarding state while the SEC_RPort is in blocking
> state.
> 
> This is just an example of one single case, but consider that, in general,
> when the carrier is lost the port state is moved into blocking so that when
> the carrier returns the port it's already into the right state.
> 
> Hope it's clearer now.

Yes, please add this to the commit message. The commit message is
supposed to explain Why, and this is a good example.
 
> However, despite this special case, I think that kernel code should
> implement mechanisms and not policies, shouldn't it? If user space needs a
> non operational port (that is with no carrier) into the listening state, why
> we should prevent it?

Did you dig deeper? Does the bridge make use of switchdev to tell the
hardware about this state change while the carrier is down? I also
wonder what the hardware drivers do? Since this is a change in
behaviour, they might not actually do anything. So then you have to
consider does it make sense for the bridge to set the state again
after the carrier comes up?

       Andrew

Re: [PATCH] net br_netlink.c:y allow non "disabled" state for !netif_oper_up() links

[Rodolfo Giometti]

On 09/11/22 19:46, Andrew Lunn wrote:
> On Wed, Nov 09, 2022 at 07:19:22PM +0100, Rodolfo Giometti wrote:
>> On 09/11/22 18:34, Andrew Lunn wrote:
>>> On Wed, Nov 09, 2022 at 04:24:10PM +0100, Rodolfo Giometti wrote:
>>>> A generic loop-free network protocol (such as STP or MRP and others) may
>>>> require that a link not in an operational state be into a non "disabled"
>>>> state (such as listening).
>>>>
>>>> For example MRP states that a MRM should set into a "BLOCKED" state (which is
>>>> equivalent to the LISTENING state for Linux bridges) one of its ring
>>>> connection if it detects that this connection is "DOWN" (that is the
>>>> NO-CARRIER status).
>>>
>>> Does MRP explain Why?
>>>
>>> This change seems odd, and "Because the standard says so" is not the
>>> best of explanations.
>>
>> A MRM instance has two ports: primary port (PRM_RPort) and secondary port
>> (SEC_RPort).
>>
>> When both ports are UP (that is the CARRIER is on) the MRM is into the
>> Ring_closed state and the PRM_RPort is in forwarding state while the
>> SEC_RPort is in blocking state (remember that MRP blocking is equal to Linux
>> bridge listening).
>>
>> If the PRM_RPort losts its carrier and the link goes down the normative states that:
>>
>> - ports role swap (PRM_RPort becomes SEC_RPort and vice versa).
>>
>> - SEC_RPort must be set into blocking state.
>>
>> - PRM_RPort must be set into forwarding state.
>>
>> Then the MRM moves into a new state called Primary-UP. In this state, when
>> the SEC_RPort returns to UP state (that is the CARRIER is up) it's returns
>> into the Ring_closed state where both ports have the right status, that is
>> the PRM_RPort is in forwarding state while the SEC_RPort is in blocking
>> state.
>>
>> This is just an example of one single case, but consider that, in general,
>> when the carrier is lost the port state is moved into blocking so that when
>> the carrier returns the port it's already into the right state.
>>
>> Hope it's clearer now.
> 
> Yes, please add this to the commit message. The commit message is
> supposed to explain Why, and this is a good example.

OK. I'm going to do it in v2.

>> However, despite this special case, I think that kernel code should
>> implement mechanisms and not policies, shouldn't it? If user space needs a
>> non operational port (that is with no carrier) into the listening state, why
>> we should prevent it?
> 
> Did you dig deeper? Does the bridge make use of switchdev to tell the
> hardware about this state change while the carrier is down?

I think so since the function br_set_state() do it.

> I also
> wonder what the hardware drivers do? Since this is a change in
> behaviour, they might not actually do anything.

For instance Marvell switches just set the state (see 
linux/drivers/net/dsa/mv88e6xxx/port.c) without checking for carrier status:

int mv88e6xxx_port_set_state(struct mv88e6xxx_chip *chip, int port, u8 state)
{
         u16 reg;
         int err;

         err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL0, &reg);
         if (err)
                 return err;

         reg &= ~MV88E6XXX_PORT_CTL0_STATE_MASK;

         switch (state) {
         case BR_STATE_DISABLED:
                 state = MV88E6XXX_PORT_CTL0_STATE_DISABLED;
                 break;
         case BR_STATE_BLOCKING:
         case BR_STATE_LISTENING:
                 state = MV88E6XXX_PORT_CTL0_STATE_BLOCKING;
                 break;
         case BR_STATE_LEARNING:
                 state = MV88E6XXX_PORT_CTL0_STATE_LEARNING;
                 break;
         case BR_STATE_FORWARDING:
                 state = MV88E6XXX_PORT_CTL0_STATE_FORWARDING;
                 break;
         default:
                 return -EINVAL;
         }

         reg |= state;

         err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL0, reg);
         if (err)
                 return err;

         dev_dbg(chip->dev, "p%d: PortState set to %s\n", port,
                 mv88e6xxx_port_state_names[state]);

         return 0;
}

> So then you have to
> consider does it make sense for the bridge to set the state again
> after the carrier comes up?

Yes, of course we can do it but (in case of MRP) the state machine must be 
altered in several points and, again, why the kernel should force such behaviour 
(i.e. introducing a policy) when drivers just don't consider it (see the above 
example).

The kernel should implement mechanisms while all policies should be into user space.

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: [PATCH] net br_netlink.c:y allow non "disabled" state for !netif_oper_up() links

[Andrew Lunn]

> > I also
> > wonder what the hardware drivers do? Since this is a change in
> > behaviour, they might not actually do anything.
> 
> For instance Marvell switches just set the state (see
> linux/drivers/net/dsa/mv88e6xxx/port.c) without checking for carrier status:

Yes, that was one i checked myself. I think i remember reviewing a DSA
driver which did not have a mechanism to disable a port, other than
the STP state. So there is a danger the mac_down() call is going to
change the STP state, and the mac_up() call will change it again.

> Yes, of course we can do it but (in case of MRP) the state machine must be
> altered in several points and, again, why the kernel should force such
> behaviour (i.e. introducing a policy) when drivers just don't consider it
> (see the above example).
> 
> The kernel should implement mechanisms while all policies should be into user space.

While i agree the policy should not be in the kernel, you have history
against you. Since this was never a requirement, and on first
mentioning it, it seems like an odd requirement, there is no guarantee
it will actually work for all drivers. So either you have to:

1) Say some kernel drivers are probably broken, will do horrible
   things to your network instead of being redundant, test it well
   before deploying.

2) Monitor for the link up event, and set the STP state as required.

The in kernel bridge/STP code takes this second approach, which again
reinforces the fact that because drivers never needed to support this,
some probably don't.

     Andrew