Re: [PATCH net-next 04/13] bridge: mrp: Expose function br_mrp_port_open

[Horatiu Vultur]

The 04/20/2020 20:30, Nikolay Aleksandrov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 20/04/2020 18:09, Horatiu Vultur wrote:
> > In case the HW is capable to detect when the MRP ring is open or closed. It is
> > expected that the network driver will notify the SW that the ring is open or
> > closed.
> >
> > The function br_mrp_port_open is used to notify the kernel that one of the ports
> > stopped receiving MRP_Test frames. The argument 'loc' has a value of '1' when
> > the port stopped receiving MRP_Test and '0' when it started to receive MRP_Test.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
> > ---
> >  include/linux/mrp_bridge.h | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >  create mode 100644 include/linux/mrp_bridge.h
> >
> > diff --git a/include/linux/mrp_bridge.h b/include/linux/mrp_bridge.h
> > new file mode 100644
> > index 000000000000..61c1f0d395c7
> > --- /dev/null
> > +++ b/include/linux/mrp_bridge.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#ifndef _LINUX_MRP_BRIDGE_H
> > +#define _LINUX_MRO_BRIDGE_H
> > +
> > +#include <linux/netdevice.h>
> > +
> > +/* The drivers are responsible to call this function when it detects that the
> > + * MRP port stopped receiving MRP_Test frames or it started to receive MRP_Test.
> > + * The argument dev represents the port and loc(Lost of Continuity) has a value
> > + * of 1 when it stopped receiving MRP_Test frames and a value of 0 when it
> > + * started to receive frames.
> > + * Needs to be called with rcu_read_lock().
> > + *
> > + * This eventually notify the userspace which is required to react on these
> > + * changes.
> > + */
> > +
> > +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> > +int br_mrp_port_open(struct net_device *dev, u8 loc);
> > +#else
> > +static inline int br_mrp_port_open(struct net_device *dev, u8 loc)
> > +{
> > +}
> > +#endif
> > +
> > +#endif
> >

Hi Nik,
> 
> I see that in order to pass this "loc" to br_ifinfo_notify() you add a new port variable,
> but it's a boolean, so you can just use the port flags for it which will simplify things a lot
> and remove some ifdefs along the way. And please rename it to something longer (so it's easier
> to get what it does).

I will update to use the port flags.

> 
> One question - now that it's always sent to user-space as IFLA_BRPORT_MRP_RING_OPEN attribute
> wouldn't it be a problem, or is it read as a current state?
> I mean you'll receive that attribute about a port even if you get a completely unrelated to MRP
> notification (e.g. about a vlan change).

It should not be a problem. The default value is 0, which means that the
port has not lost the continuity.
So let's say that a port lost the continuity then the userspace will
receive a call where attribute IFLA_BRPORT_MRP_RING_OPEN will have the
value 1. This is as expected.
Then as you suggested, there is a vlan change, the userspace will
receive a notification but again the attribute IFLA_BRPORT_MRP_RING_OPEN
is 1, because the actual value is stored in the port variable, or port
flags in the new patch). But the userspace can detect this case and just
ignores it.
The same will apply also if the userspace will receive multiple
notification if the port didn't lose the continuity.

> 
> Thanks,
>  Nik

-- 
/Horatiu