Date:   Fri, 11 Nov 2022 10:43:30 +0100
From:   Rodolfo Giometti <giometti@...eenne.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, Roopa Prabhu <roopa@...dia.com>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        Stephen Hemminger <shemminger@...l.org>,
        Flavio Leitner <fbl@...hat.com>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH] net br_netlink.c:y allow non "disabled" state for
 !netif_oper_up() links

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@...eenne.com
Linux Device Driver                          giometti@...ux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Powered by blists - more mailing lists