Wed, May 24, 2017 at 09:24:13PM CEST, kubak...@wp.pl wrote:
>On Wed, 24 May 2017 14:35:14 +0200, Jiri Pirko wrote:
>> >+void nfp_devlink_port_unregister(struct nfp_port *port)
>> >+{
>> >+ /* Due to unpleasant lock ordering we may see the port go away before
>> >+ * we have fully probed.
>>
>> Could you elaborate on this a bit more please?
>
>It's partially due to peculiarities of the management FW more than
>kernel stuff. Unfortunately some ethtool media config requires reboot
>to be applied, so we print a friendly message to the logs and
>unregister the associated netdevs. Which means once netdevs get
>registered ports may go away.
>
>Enter devlink, I need the ability to grab the adapater lock in
>split/unsplit callbacks to find the ports, which implies having to drop
>that lock before I register devlink. And only after I register devlink
>can I register the ports.
>
>I could do init without registering anything, drop the adapter lock,
>register devlink, and then grab the adapter lock back and register
>devlink ports and netdevs. But there is another issue...
>
>Since I look for ports on a list maintained in the adapter struct,
>driver code doesn't care if devlink_port has been registered or not.
>The moment devlink is registered, split/unsplit requests will be
>accepted - potentially trying to unregister devlink_port before the
>register could happen.
>
>Further down the line, also, the eswitch mode setting is coming. Which
>means the moment I register devlink itself ports will get shuffled (due
>to the plan of registering VFs as ports :)).
>
>I feel like registering devlink should be the last action of the
>driver, really. My plan was to keep that simple if() for now, and once
>we get to extending devlink with SR-IOV stuff also add the ability to
>pre-register ports. Allow registering ports on not-yet-registered
>devlink (probably put them on a private list within struct devlink).
>This would make devlink_register() a single point when everything
>devlink becomes visible, atomically, instead of devlink itself coming
>first and then ports following.
>
>Does that make sense? Am I misreading the code (again :S)?
Well in mlxsw, we internally maintain a list of port and we assign a
pointer to the port struct only after all is initialized:
...
mlxsw_sp->ports[local_port] = mlxsw_sp_port;
err = register_netdev(dev);
...
Then in split, we check:
mlxsw_sp_port = mlxsw_sp->ports[local_port];
if (!mlxsw_sp_port) {
dev_err(mlxsw_sp->bus_info->dev, "Port number \"%d\" does not exist\n",
local_port);
return -EINVAL;
}
I guess that you can do something similar. You should ensure that split
won't happen for half-initialized ports.
