| Message ID | 20200919144332.3665538-1-andrew@lunn.ch |
|---|---|
| Headers | show |
| Series | Add per port devlink regions | expand |
On Sat, Sep 19, 2020 at 04:43:28PM +0200, Andrew Lunn wrote: > > DSA only instantiates devlink ports for switch ports which are used. > For this hardware, only 4 user ports and the CPU port have devlink > ports, which explains the discontinuous port regions. This is not so much a choice, as it is a workaround of the fact that dsa_port_setup(), which registers devlink ports with devlink, is called after ds->ops->setup(), so you can't register your port regions from the same place as the global regions now. So you're doing it from ds->ops->port_enable(), which is the DSA wrapper for .ndo_open(). So, consequently, your port regions will only be registered when the port is up, and will be unregistered when it goes down. Is that what you want? I understand that users probably think they want to debug only the ports that they actively use, but I've heard of at least one problem in the past which was caused by invalid settings (flooding in that case) on a port that was down. Sure, this is probably a rare situation, but as I said, somebody trying to use port regions to debug something like that is probably going to have a hard time, because it isn't an easy surgery to figure the probe ordering out. Thanks, -Vladimir
On 9/20/2020 4:33 PM, Vladimir Oltean wrote: > On Sat, Sep 19, 2020 at 04:43:28PM +0200, Andrew Lunn wrote: >> >> DSA only instantiates devlink ports for switch ports which are used. >> For this hardware, only 4 user ports and the CPU port have devlink >> ports, which explains the discontinuous port regions. > > This is not so much a choice, as it is a workaround of the fact that > dsa_port_setup(), which registers devlink ports with devlink, is called > after ds->ops->setup(), so you can't register your port regions from > the same place as the global regions now. > > So you're doing it from ds->ops->port_enable(), which is the DSA wrapper > for .ndo_open(). So, consequently, your port regions will only be > registered when the port is up, and will be unregistered when it goes > down. Is that what you want? I understand that users probably think they > want to debug only the ports that they actively use, but I've heard of > at least one problem in the past which was caused by invalid settings > (flooding in that case) on a port that was down. Sure, this is probably > a rare situation, but as I said, somebody trying to use port regions to > debug something like that is probably going to have a hard time, because > it isn't an easy surgery to figure the probe ordering out. Being able to debug the switch configuration as soon as it gets registered with DSA all the way through enabling an user port has definitively a lot of value so we should aim to support that use case.
On Sun, Sep 20, 2020 at 11:33:41PM +0000, Vladimir Oltean wrote: > On Sat, Sep 19, 2020 at 04:43:28PM +0200, Andrew Lunn wrote: > > > > DSA only instantiates devlink ports for switch ports which are used. > > For this hardware, only 4 user ports and the CPU port have devlink > > ports, which explains the discontinuous port regions. > > This is not so much a choice, as it is a workaround of the fact that > dsa_port_setup(), which registers devlink ports with devlink, is called > after ds->ops->setup(), so you can't register your port regions from > the same place as the global regions now. Correct. > So you're doing it from ds->ops->port_enable(), which is the DSA wrapper > for .ndo_open(). So, consequently, your port regions will only be > registered when the port is up, and will be unregistered when it goes > down. Is that what you want? I understand that users probably think they > want to debug only the ports that they actively use, but I've heard of > at least one problem in the past which was caused by invalid settings > (flooding in that case) on a port that was down. Sure, this is probably > a rare situation, but as I said, somebody trying to use port regions to > debug something like that is probably going to have a hard time, because > it isn't an easy surgery to figure the probe ordering out. I did intially create the port instances at the same time as the global ones, and it died a horrible death. And i was aiming to register a region for each port, not just those which are used. This splits into two problems. 1) Devlink has no concept of a port which is unused. We simply don't register unused ports. So we need to add a new devlink_port_flavour: DEVLINK_PORT_FLAVOUR_UNUSED. That seems easy enough. 2) We need to rearrange the order the core sets stuff up, such that it registers devlink ports before calling the DSA driver setup() method. I think that is possible after a quick look at the code. Andrew