Errors enabling bridge with KSZ9897 DSA switch driver

Errors enabling bridge with KSZ9897 DSA switch driver

[Robert Hancock]

I have a device using a KSZ9897 switch
(CONFIG_MICROCHIP_KSZ_SPI_DRIVER=y) using a 4.19.9 kernel, where I am
trying to enable a bridge on some of the ports using systemd-networkd.
However, it seems to be getting an error when it tries to configure the
ports to be part of the bridge. systemd-networkd complains with:

lan1: Set link
lan1: Could not join netdev: Operation not supported
lan1: Failed

and the kernel complains:

lan: bridge flag offload is not supported 4(lan1)

That message is coming from br_switchdev_set_port_flag in
net/bridge/br_switchdev.c. Adding some more output to that statement
tells me:

lan: bridge flag offload is not supported, flags 18656 mask 64 support
0, 4(lan1)

which appears to mean something is trying to enable BR_BCAST_FLOOD,
BR_MCAST_FLOOD, BR_PROMISC, BR_FLOOD, BR_LEARNING on the port, but it is
failing to enable BR_FLOOD because brport_flags_support is 0. I am just
using the default bridge settings in systemd-networkd, so that is
nothing that I am specifying explicitly.

However, if I look at brctl, it appears that the devices have in fact
been added to the bridge, but obviously systemd-networkd still treats
this as a failure.

I'm not sure what that means.. is this a kernel problem?
systemd-networkd problem? Are we somehow trying to do something
unsupported on this hardware? Any advice would be appreciated.

-- 
Robert Hancock
Senior Software Developer
SED Systems
Email: hancock@sedsystems.ca

Re: Errors enabling bridge with KSZ9897 DSA switch driver

[Robert Hancock]

On 2018-12-22 8:29 p.m., Robert Hancock wrote:
> I have a device using a KSZ9897 switch
> (CONFIG_MICROCHIP_KSZ_SPI_DRIVER=y) using a 4.19.9 kernel, where I am
> trying to enable a bridge on some of the ports using systemd-networkd.
> However, it seems to be getting an error when it tries to configure the
> ports to be part of the bridge. systemd-networkd complains with:
> 
> lan1: Set link
> lan1: Could not join netdev: Operation not supported
> lan1: Failed
> 
> and the kernel complains:
> 
> lan: bridge flag offload is not supported 4(lan1)
> 
> That message is coming from br_switchdev_set_port_flag in
> net/bridge/br_switchdev.c. Adding some more output to that statement
> tells me:
> 
> lan: bridge flag offload is not supported, flags 18656 mask 64 support
> 0, 4(lan1)
> 
> which appears to mean something is trying to enable BR_BCAST_FLOOD,
> BR_MCAST_FLOOD, BR_PROMISC, BR_FLOOD, BR_LEARNING on the port, but it is
> failing to enable BR_FLOOD because brport_flags_support is 0. I am just
> using the default bridge settings in systemd-networkd, so that is
> nothing that I am specifying explicitly.
> 
> However, if I look at brctl, it appears that the devices have in fact
> been added to the bridge, but obviously systemd-networkd still treats
> this as a failure.
> 
> I'm not sure what that means.. is this a kernel problem?
> systemd-networkd problem? Are we somehow trying to do something
> unsupported on this hardware? Any advice would be appreciated.
> 

Looking at this a bit further, it seems like the problem is in the code 
in net/bridge/br_switchdev.c around where the message is printed:

     err = switchdev_port_attr_get(p->dev, &attr);
     if (err == -EOPNOTSUPP)
         return 0;
     if (err)
         return err;

     /* Check if specific bridge flag attribute offload is supported */
     if (!(attr.u.brport_flags_support & mask)) {
         br_warn(p->br, "bridge flag offload is not supported %u(%s)\n",
             (unsigned int)p->port_no, p->dev->name);
         return -EOPNOTSUPP;
     }

If the port doesn't support the switchdev_port_attr_get operation at 
all, we just ignore it and return 0. However, if it does support that 
operation (as DSA does), but doesn't have the particular bridge flag 
supported in brport_flags_support (which it won't here, since 
brport_flags_support is always set to 0 for DSA), we complain and return 
-EOPNOTSUPP. That return value propagates all the way back to userspace 
and fails the entire netlink operation that it was trying to do, causing 
systemd-networkd to fail the bridge startup.

Shouldn't this code just be returning 0 in this case?

Re: Errors enabling bridge with KSZ9897 DSA switch driver

[Andrew Lunn]

On Sat, Dec 22, 2018 at 08:29:08PM -0600, Robert Hancock wrote:
> I have a device using a KSZ9897 switch
> (CONFIG_MICROCHIP_KSZ_SPI_DRIVER=y) using a 4.19.9 kernel, where I am
> trying to enable a bridge on some of the ports using systemd-networkd.

Hi Robert

I don't think many of use use such a setup, so we probably have not
noticed this.

> However, it seems to be getting an error when it tries to configure the
> ports to be part of the bridge. systemd-networkd complains with:
> 
> lan1: Set link
> lan1: Could not join netdev: Operation not supported
> lan1: Failed
> 
> and the kernel complains:
> 
> lan: bridge flag offload is not supported 4(lan1)
> 
> That message is coming from br_switchdev_set_port_flag in
> net/bridge/br_switchdev.c. Adding some more output to that statement
> tells me:
> 
> lan: bridge flag offload is not supported, flags 18656 mask 64 support
> 0, 4(lan1)
> 
> which appears to mean something is trying to enable BR_BCAST_FLOOD,
> BR_MCAST_FLOOD, BR_PROMISC, BR_FLOOD, BR_LEARNING on the port, but it is
> failing to enable BR_FLOOD because brport_flags_support is 0. I am just
> using the default bridge settings in systemd-networkd, so that is
> nothing that I am specifying explicitly.

I think this is a systemd problem. It should first query what flags
are supported. See 

dc0ecabd6231 ("net: switchdev: Add support for querying supported bridge flags by hardware")

And then only try to turn on flags which are supported.  If it tries
to turn on flags which are not supported, an error is the correct
thing to do.

      Andrew

Re: Errors enabling bridge with KSZ9897 DSA switch driver

[Robert Hancock]

On 2018-12-23 5:23 a.m., Andrew Lunn wrote:
> On Sat, Dec 22, 2018 at 08:29:08PM -0600, Robert Hancock wrote:
>> I have a device using a KSZ9897 switch
>> (CONFIG_MICROCHIP_KSZ_SPI_DRIVER=y) using a 4.19.9 kernel, where I am
>> trying to enable a bridge on some of the ports using systemd-networkd.
> 
> Hi Robert
> 
> I don't think many of use use such a setup, so we probably have not
> noticed this.
> 
>> However, it seems to be getting an error when it tries to configure the
>> ports to be part of the bridge. systemd-networkd complains with:
>>
>> lan1: Set link
>> lan1: Could not join netdev: Operation not supported
>> lan1: Failed
>>
>> and the kernel complains:
>>
>> lan: bridge flag offload is not supported 4(lan1)
>>
>> That message is coming from br_switchdev_set_port_flag in
>> net/bridge/br_switchdev.c. Adding some more output to that statement
>> tells me:
>>
>> lan: bridge flag offload is not supported, flags 18656 mask 64 support
>> 0, 4(lan1)
>>
>> which appears to mean something is trying to enable BR_BCAST_FLOOD,
>> BR_MCAST_FLOOD, BR_PROMISC, BR_FLOOD, BR_LEARNING on the port, but it is
>> failing to enable BR_FLOOD because brport_flags_support is 0. I am just
>> using the default bridge settings in systemd-networkd, so that is
>> nothing that I am specifying explicitly.
> 
> I think this is a systemd problem. It should first query what flags
> are supported. See
> 
> dc0ecabd6231 ("net: switchdev: Add support for querying supported bridge flags by hardware")
> 
> And then only try to turn on flags which are supported.  If it tries
> to turn on flags which are not supported, an error is the correct
> thing to do.

I may be missing some context about how the switchdev/DSA code works, 
but are those flags actually unsupported, or just unsupported in 
hardware offload? Userspace doesn't know or care about the hardware 
offload capability of the switch, it's just trying to set standard flags 
on the bridge. As far as I can tell, BR_FLOOD is enabled by default (as 
a bridge without that set isn't very generally useful), but systemd 
trying to explicitly set it to enabled causes the entire userspace 
netlink operation to fail. That doesn't seem right.

As I mentioned in my follow-up message, it seems like the function 
producing that message shouldn't be failing the operation in this case, 
just bailing out in the same way as if the port doesn't support 
retrieving switchdev attributes at all.

Re: Errors enabling bridge with KSZ9897 DSA switch driver

[Andrew Lunn]

On Sun, Dec 23, 2018 at 01:29:42PM -0600, Robert Hancock wrote:
> On 2018-12-23 5:23 a.m., Andrew Lunn wrote:
> >On Sat, Dec 22, 2018 at 08:29:08PM -0600, Robert Hancock wrote:
> >>I have a device using a KSZ9897 switch
> >>(CONFIG_MICROCHIP_KSZ_SPI_DRIVER=y) using a 4.19.9 kernel, where I am
> >>trying to enable a bridge on some of the ports using systemd-networkd.
> >
> >Hi Robert
> >
> >I don't think many of use use such a setup, so we probably have not
> >noticed this.
> >
> >>However, it seems to be getting an error when it tries to configure the
> >>ports to be part of the bridge. systemd-networkd complains with:
> >>
> >>lan1: Set link
> >>lan1: Could not join netdev: Operation not supported
> >>lan1: Failed
> >>
> >>and the kernel complains:
> >>
> >>lan: bridge flag offload is not supported 4(lan1)
> >>
> >>That message is coming from br_switchdev_set_port_flag in
> >>net/bridge/br_switchdev.c. Adding some more output to that statement
> >>tells me:
> >>
> >>lan: bridge flag offload is not supported, flags 18656 mask 64 support
> >>0, 4(lan1)
> >>
> >>which appears to mean something is trying to enable BR_BCAST_FLOOD,
> >>BR_MCAST_FLOOD, BR_PROMISC, BR_FLOOD, BR_LEARNING on the port, but it is
> >>failing to enable BR_FLOOD because brport_flags_support is 0. I am just
> >>using the default bridge settings in systemd-networkd, so that is
> >>nothing that I am specifying explicitly.
> >
> >I think this is a systemd problem. It should first query what flags
> >are supported. See
> >
> >dc0ecabd6231 ("net: switchdev: Add support for querying supported bridge flags by hardware")
> >
> >And then only try to turn on flags which are supported.  If it tries
> >to turn on flags which are not supported, an error is the correct
> >thing to do.
> 
> I may be missing some context about how the switchdev/DSA code works, but
> are those flags actually unsupported, or just unsupported in hardware
> offload? Userspace doesn't know or care about the hardware offload
> capability of the switch, it's just trying to set standard flags on the
> bridge.

Hi Robert

We really need Arkadi Sharshevsky to comment on this.

   Andrew

Re: Errors enabling bridge with KSZ9897 DSA switch driver

[Robert Hancock]

On 2018-12-23 2:57 p.m., Andrew Lunn wrote:
> On Sun, Dec 23, 2018 at 01:29:42PM -0600, Robert Hancock wrote:
>> On 2018-12-23 5:23 a.m., Andrew Lunn wrote:
>>> On Sat, Dec 22, 2018 at 08:29:08PM -0600, Robert Hancock wrote:
>>>> I have a device using a KSZ9897 switch
>>>> (CONFIG_MICROCHIP_KSZ_SPI_DRIVER=y) using a 4.19.9 kernel, where I am
>>>> trying to enable a bridge on some of the ports using systemd-networkd.
>>>
>>> Hi Robert
>>>
>>> I don't think many of use use such a setup, so we probably have not
>>> noticed this.
>>>
>>>> However, it seems to be getting an error when it tries to configure the
>>>> ports to be part of the bridge. systemd-networkd complains with:
>>>>
>>>> lan1: Set link
>>>> lan1: Could not join netdev: Operation not supported
>>>> lan1: Failed
>>>>
>>>> and the kernel complains:
>>>>
>>>> lan: bridge flag offload is not supported 4(lan1)
>>>>
>>>> That message is coming from br_switchdev_set_port_flag in
>>>> net/bridge/br_switchdev.c. Adding some more output to that statement
>>>> tells me:
>>>>
>>>> lan: bridge flag offload is not supported, flags 18656 mask 64 support
>>>> 0, 4(lan1)
>>>>
>>>> which appears to mean something is trying to enable BR_BCAST_FLOOD,
>>>> BR_MCAST_FLOOD, BR_PROMISC, BR_FLOOD, BR_LEARNING on the port, but it is
>>>> failing to enable BR_FLOOD because brport_flags_support is 0. I am just
>>>> using the default bridge settings in systemd-networkd, so that is
>>>> nothing that I am specifying explicitly.
>>>
>>> I think this is a systemd problem. It should first query what flags
>>> are supported. See
>>>
>>> dc0ecabd6231 ("net: switchdev: Add support for querying supported bridge flags by hardware")
>>>
>>> And then only try to turn on flags which are supported.  If it tries
>>> to turn on flags which are not supported, an error is the correct
>>> thing to do.
>>
>> I may be missing some context about how the switchdev/DSA code works, but
>> are those flags actually unsupported, or just unsupported in hardware
>> offload? Userspace doesn't know or care about the hardware offload
>> capability of the switch, it's just trying to set standard flags on the
>> bridge.
> 
> Hi Robert
> 
> We really need Arkadi Sharshevsky to comment on this.

FYI, changing the code in question to return 0 instead of -EOPNOTSUPP in
the missing brport_flags_support flag case allows systemd-networkd to
initialize the port successfully. We have some other issues in our
hardware setup that prevent me from testing that fully at the moment,
but I will likely submit that as a patch shortly unless I hear of a
better solution..

-- 
Robert Hancock
Senior Software Developer
SED Systems
Email: hancock@sedsystems.ca

Re: Errors enabling bridge with KSZ9897 DSA switch driver

[Andrew Lunn]

> FYI, changing the code in question to return 0 instead of -EOPNOTSUPP in
> the missing brport_flags_support flag case allows systemd-networkd to
> initialize the port successfully. We have some other issues in our
> hardware setup that prevent me from testing that fully at the moment,
> but I will likely submit that as a patch shortly unless I hear of a
> better solution..

Please wait until after new years when i get chance to look at this.

If you can get the supported flags, it seems odd to then ignore errors
when asked to enable something which is not supported. Either these
need to be mandatory, or systemd-networkd needs to ask before setting.

     Andrew