Netdev Archive on
help / color / mirror / Atom feed
From: Vladimir Oltean <>
To: Leon Romanovsky <>
Cc: Vladimir Oltean <>,, Andrew Lunn <>,
	Vivien Didelot <>,
	Florian Fainelli <>
Subject: Re: [RFC PATCH net] net: dsa: tear down devlink port regions when tearing down the devlink port on error
Date: Sun, 5 Sep 2021 11:45:18 +0300	[thread overview]
Message-ID: <20210905084518.emlagw76qmo44rpw@skbuf> (raw)
In-Reply-To: <YTRswWukNB0zDRIc@unreal>

On Sun, Sep 05, 2021 at 10:07:45AM +0300, Leon Romanovsky wrote:
> On Fri, Sep 03, 2021 at 02:17:38AM +0300, Vladimir Oltean wrote:
> > Commit 86f8b1c01a0a ("net: dsa: Do not make user port errors fatal")
> > decided it was fine to ignore errors on certain ports that fail to
> > probe, and go on with the ports that do probe fine.
> > 
> > Commit fb6ec87f7229 ("net: dsa: Fix type was not set for devlink port")
> > noticed that devlink_port_type_eth_set(dlp, dp->slave); does not get
> > called, and devlink notices after a timeout of 3700 seconds and prints a
> > WARN_ON. So it went ahead to unregister the devlink port. And because
> > there exists an UNUSED port flavour, we actually re-register the devlink
> > port as UNUSED.
> > 
> > Commit 08156ba430b4 ("net: dsa: Add devlink port regions support to
> > DSA") added devlink port regions, which are set up by the driver and not
> > by DSA.
> > 
> > When we trigger the devlink port deregistration and reregistration as
> > unused, devlink now prints another WARN_ON, from here:
> > 
> > devlink_port_unregister:
> > 	WARN_ON(!list_empty(&devlink_port->region_list));
> > 
> > So the port still has regions, which makes sense, because they were set
> > up by the driver, and the driver doesn't know we're unregistering the
> > devlink port.
> > 
> > Somebody needs to tear them down, and optionally (actually it would be
> > nice, to be consistent) set them up again for the new devlink port.
> > 
> > But DSA's layering stays in our way quite badly here.
> I don't know anything about DSA

It is sufficient to know in this case that it is a multi-port networking

> and what led to the decision to ignore devlink registration errors,

But we are not ignoring devlink registration errors...

The devlink_port must be initialized prior to initializing the net_device.

Initializing a certain net_device may fail due to reasons such as "PHY
not found". It is desirable in certain cases for a net_device
initialization failure to not fail the entire switch probe.

So at the very least, rollback of the registration of that port must be
performed before continuing => the devlink_port needs to be unregistered
when the net_device initialization has failed.

> but devlink core is relying on the simple assumption that everything
> is initialized correctly.
> So if DSA needs to have not-initialized port, it should do all the needed
> hacks internally.

So the current problem is that the DSA framework does not ask the hardware
driver whether it has devlink port regions which need to be torn down
before unregistering the devlink port.

I was expecting the feedback to be "we need to introduce new methods in
struct dsa_switch_ops which do .port_setup and .port_teardown, similar
to the already existing per-switch .setup and .teardown, and drivers
which set up devlink port regions should set these up from the port
methods, so that DSA can simply call those when it needs to tear down a
devlink port without tearing down the entire switch and devlink instance".
The proposed patch is horrible and I agree, but not for the reasons you
might think it is.

Either way, "all the needed hacks" are already done internally, and from
devlink's perspective everything is initialized correctly, not sure what
this comment is about. I am really not changing anything in DSA's
interaction with the devlink core, other than ensuring we do not
unregister a devlink port with regions on it.

  reply	other threads:[~2021-09-05  8:49 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-02 23:17 Vladimir Oltean
2021-09-05  7:07 ` Leon Romanovsky
2021-09-05  8:45   ` Vladimir Oltean [this message]
2021-09-05 10:25     ` Leon Romanovsky
2021-09-05 10:31       ` Vladimir Oltean
2021-09-05 10:47         ` Leon Romanovsky
2021-09-05 11:07           ` Vladimir Oltean
2021-09-05 13:01             ` Leon Romanovsky
2021-09-05 15:01               ` Vladimir Oltean
2021-09-07 15:44             ` Jakub Kicinski
2021-09-07 15:47               ` Florian Fainelli
2021-09-07 16:43                 ` Andrew Lunn
2021-09-07 16:49                   ` Florian Fainelli
2021-09-07 22:59                     ` Leon Romanovsky
2021-09-08 20:21                     ` Vladimir Oltean

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210905084518.emlagw76qmo44rpw@skbuf \ \ \ \ \ \ \ \
    --subject='Re: [RFC PATCH net] net: dsa: tear down devlink port regions when tearing down the devlink port on error' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).