Re: [RFC PATCH net] net: dsa: tear down devlink port regions when tearing down the devlink port on error

On Tue, Sep 07, 2021 at 09:49:48AM -0700, Florian Fainelli wrote:

On 9/7/2021 9:43 AM, Andrew Lunn wrote:

quoted

On Tue, Sep 07, 2021 at 08:47:35AM -0700, Florian Fainelli wrote:

quoted

On 9/7/2021 8:44 AM, Jakub Kicinski wrote:

quoted

On Sun, 5 Sep 2021 14:07:35 +0300 Vladimir Oltean wrote:

quoted

Again, fallback but not during devlink port register. The devlink port
was registered just fine, but our plans changed midway. If you want to
create a net device with an associated devlink port, first you need to
create the devlink port and then the net device, then you need to link
the two using devlink_port_type_eth_set, at least according to my
understanding.

So the failure is during the creation of the **net device**, we now have a
devlink port which was originally intended to be of the Ethernet type
and have a physical flavour, but it will not be backed by any net device,
because the creation of that just failed. So the question is simply what
to do with that devlink port.

Is the failure you're referring to discovered inside the
register_netdevice() call?

It is before, at the time we attempt to connect to the PHY device, prior to
registering the netdev, we may fail that PHY connection, tearing down the
entire switch because of that is highly undesirable.

Maybe we should re-order things a little bit and try to register devlink
ports only after we successfully registered with the PHY/SFP and prior to
registering the netdev?

Maybe, but it should not really matter. EPROBE_DEFER exists, and can
happen. The probe can fail for other reasons. All core code should be
cleanly undoable. Maybe we are pushing it a little by only wanting to
undo a single port, rather than the whole switch, but still, i would
make the core handle this, not rearrange the driver. It is not robust
otherwise.

Well yes, in case my comment was not clear, I was referring to the way that
DSA register devlink ports, not how the mv88e6xxx driver does it. That is
assuming that it is possible and there was not a reason for configuring the
devlink ports ahead of the switch driver coming up.

There is a comment in dsa_switch_setup:

	/* Setup devlink port instances now, so that the switch
	 * setup() can register regions etc, against the ports
	 */

The fact of the matter is that in the current driver-facing API, there
is no better place to register devlink port regions than .setup: we have
no .port_setup and .port_teardown. This also forces us to register the
devlink ports earlier than we register the net devices.

In one of my previous replies to Leon I did indicate the introduction of
these two methods as a possibly less horrible way of packaging what we
have now in this patch as .port_reinit_as_unused, which is basically a
.port_teardown followed immediately by a .port_setup, as it would be
implemented by mv88e6xxx.

With the introduction of .port_setup and .port_teardown as an immediate
bug fix, reordering the devlink port registration vs the netdev connection
to the PHY could be done as further work, but it would sort of be a moot
point, since it would not solve any problem anymore.