Message-ID: <6dd32faa-2651-31bf-da2e-e768b9966e36@amd.com>
Date:   Tue, 4 Oct 2022 15:31:10 +0000
From:   "Lucero Palau, Alejandro" <alejandro.lucero-palau@....com>
To:     Jiri Pirko <jiri@...nulli.us>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "dmichail@...gible.com" <dmichail@...gible.com>,
        "jesse.brandeburg@...el.com" <jesse.brandeburg@...el.com>,
        "anthony.l.nguyen@...el.com" <anthony.l.nguyen@...el.com>,
        "snelson@...sando.io" <snelson@...sando.io>,
        "drivers@...sando.io" <drivers@...sando.io>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "yangyingliang@...wei.com" <yangyingliang@...wei.com>
Subject: Re: [patch net-next 0/3] devlink: fix order of port and netdev
 register in drivers

Hi Jiri,

I think we have another issue with devlink_unregister and related 
devlink_port_unregister. It is likely not an issue with current drivers 
because the devlink ports are managed by netdev register/unregister 
code, and with your patch that will be fine.

But by definition, devlink does exist for those things not matching 
smoothly to netdevs, so it is expected devlink ports not related to 
existing netdevs at all. That is the case in a patch I'm working on for 
sfc ef100, where devlink ports are created at PF initialization, so 
related netdevs will not be there at that point, and they can not exist 
when the devlink ports are removed when the driver is removed.

So the question in this case is, should the devlink ports unregister 
before or after their devlink unregisters?

Since the ports are in a list owned by the devlink struct, I think it 
seems logical to unregister the ports first, and that is what I did. It 
works but there exists a potential concurrency issue with devlink user 
space operations. The devlink code takes care of race conditions involving the 
devlink struct with rcu plus get/put operations, but that is not the 
case for devlink ports.

Interestingly, unregistering the devlink first, and doing so with the 
ports without touching/releasing the devlink struct would solve the 
problem, but not sure this is the right approach here. It does not seem 
clean, and it would require documenting the right unwinding order and 
to add a check for DEVLINK_REGISTERED in devlink_port_unregister.

I think the right solution would be to add protection to devlink ports 
and likely other devlink objects with similar concurrency issues.


Let me know what you think about it.



On 9/26/22 13:09, Jiri Pirko wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> From: Jiri Pirko <jiri@...dia.com>
>
> Some of the drivers use wrong order in registering devlink port and
> netdev, registering netdev first. That was not intended as the devlink
> port is some sort of parent for the netdev. Fix the ordering.
>
> Note that the follow-up patchset is going to make this ordering
> mandatory.
>
> Jiri Pirko (3):
>    funeth: unregister devlink port after netdevice unregister
>    ice: reorder PF/representor devlink port register/unregister flows
>    ionic: change order of devlink port register and netdev register
>
>   .../net/ethernet/fungible/funeth/funeth_main.c   |  2 +-
>   drivers/net/ethernet/intel/ice/ice_lib.c         |  6 +++---
>   drivers/net/ethernet/intel/ice/ice_main.c        | 12 ++++++------
>   drivers/net/ethernet/intel/ice/ice_repr.c        |  2 +-
>   .../net/ethernet/pensando/ionic/ionic_bus_pci.c  | 16 ++++++++--------
>   5 files changed, 19 insertions(+), 19 deletions(-)
>
> --
> 2.37.1
>

Powered by blists - more mailing lists