[patch net-next 0/3] devlink: fix order of port and netdev register in drivers

[patch net-next 0/3] devlink: fix order of port and netdev register in drivers

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.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

[patch net-next 1/3] funeth: unregister devlink port after netdevice unregister

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Fix the order of destroy_netdev() flow and unregister the devlink port
after calling unregister_netdev().

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/ethernet/fungible/funeth/funeth_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/fungible/funeth/funeth_main.c b/drivers/net/ethernet/fungible/funeth/funeth_main.c
index b6de2ad82a32..6980455fb909 100644
--- a/drivers/net/ethernet/fungible/funeth/funeth_main.c
+++ b/drivers/net/ethernet/fungible/funeth/funeth_main.c
@@ -1829,8 +1829,8 @@ static void fun_destroy_netdev(struct net_device *netdev)
 
 	fp = netdev_priv(netdev);
 	devlink_port_type_clear(&fp->dl_port);
-	devlink_port_unregister(&fp->dl_port);
 	unregister_netdev(netdev);
+	devlink_port_unregister(&fp->dl_port);
 	fun_ktls_cleanup(fp);
 	fun_free_stats_area(fp);
 	fun_free_rss(fp);
-- 
2.37.1

[patch net-next 2/3] ice: reorder PF/representor devlink port register/unregister flows

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Make sure that netdevice is registered/unregistered while devlink port
is registered.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 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 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 8a80da8e910e..938ba8c215cb 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -2988,9 +2988,6 @@ int ice_vsi_release(struct ice_vsi *vsi)
 		clear_bit(ICE_VSI_NETDEV_REGISTERED, vsi->state);
 	}
 
-	if (vsi->type == ICE_VSI_PF)
-		ice_devlink_destroy_pf_port(pf);
-
 	if (test_bit(ICE_FLAG_RSS_ENA, pf->flags))
 		ice_rss_clean(vsi);
 
@@ -3048,6 +3045,9 @@ int ice_vsi_release(struct ice_vsi *vsi)
 		}
 	}
 
+	if (vsi->type == ICE_VSI_PF)
+		ice_devlink_destroy_pf_port(pf);
+
 	if (vsi->type == ICE_VSI_VF &&
 	    vsi->agg_node && vsi->agg_node->valid)
 		vsi->agg_node->num_vsis--;
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 0ccc8a750374..747f27c4e761 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4599,6 +4599,10 @@ static int ice_register_netdev(struct ice_pf *pf)
 	if (!vsi || !vsi->netdev)
 		return -EIO;
 
+	err = ice_devlink_create_pf_port(pf);
+	if (err)
+		goto err_devlink_create;
+
 	err = register_netdev(vsi->netdev);
 	if (err)
 		goto err_register_netdev;
@@ -4606,17 +4610,13 @@ static int ice_register_netdev(struct ice_pf *pf)
 	set_bit(ICE_VSI_NETDEV_REGISTERED, vsi->state);
 	netif_carrier_off(vsi->netdev);
 	netif_tx_stop_all_queues(vsi->netdev);
-	err = ice_devlink_create_pf_port(pf);
-	if (err)
-		goto err_devlink_create;
 
 	devlink_port_type_eth_set(&pf->devlink_port, vsi->netdev);
 
 	return 0;
-err_devlink_create:
-	unregister_netdev(vsi->netdev);
-	clear_bit(ICE_VSI_NETDEV_REGISTERED, vsi->state);
 err_register_netdev:
+	ice_devlink_destroy_pf_port(pf);
+err_devlink_create:
 	free_netdev(vsi->netdev);
 	vsi->netdev = NULL;
 	clear_bit(ICE_VSI_NETDEV_ALLOCD, vsi->state);
diff --git a/drivers/net/ethernet/intel/ice/ice_repr.c b/drivers/net/ethernet/intel/ice/ice_repr.c
index 0dac67cd9c77..bd31748aae1b 100644
--- a/drivers/net/ethernet/intel/ice/ice_repr.c
+++ b/drivers/net/ethernet/intel/ice/ice_repr.c
@@ -377,10 +377,10 @@ static void ice_repr_rem(struct ice_vf *vf)
 	if (!vf->repr)
 		return;
 
-	ice_devlink_destroy_vf_port(vf);
 	kfree(vf->repr->q_vector);
 	vf->repr->q_vector = NULL;
 	unregister_netdev(vf->repr->netdev);
+	ice_devlink_destroy_vf_port(vf);
 	free_netdev(vf->repr->netdev);
 	vf->repr->netdev = NULL;
 #ifdef CONFIG_ICE_SWITCHDEV
-- 
2.37.1

[patch net-next 3/3] ionic: change order of devlink port register and netdev register

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Make sure that devlink port is registered first and register netdev
after. Unregister netdev before devlnk port unregister.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 .../net/ethernet/pensando/ionic/ionic_bus_pci.c  | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
index 0a7a757494bc..ce436e97324a 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
@@ -320,16 +320,16 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			dev_err(dev, "Cannot enable existing VFs: %d\n", err);
 	}
 
-	err = ionic_lif_register(ionic->lif);
+	err = ionic_devlink_register(ionic);
 	if (err) {
-		dev_err(dev, "Cannot register LIF: %d, aborting\n", err);
+		dev_err(dev, "Cannot register devlink: %d\n", err);
 		goto err_out_deinit_lifs;
 	}
 
-	err = ionic_devlink_register(ionic);
+	err = ionic_lif_register(ionic->lif);
 	if (err) {
-		dev_err(dev, "Cannot register devlink: %d\n", err);
-		goto err_out_deregister_lifs;
+		dev_err(dev, "Cannot register LIF: %d, aborting\n", err);
+		goto err_out_deregister_devlink;
 	}
 
 	mod_timer(&ionic->watchdog_timer,
@@ -337,8 +337,8 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	return 0;
 
-err_out_deregister_lifs:
-	ionic_lif_unregister(ionic->lif);
+err_out_deregister_devlink:
+	ionic_devlink_unregister(ionic);
 err_out_deinit_lifs:
 	ionic_vf_dealloc(ionic);
 	ionic_lif_deinit(ionic->lif);
@@ -380,8 +380,8 @@ static void ionic_remove(struct pci_dev *pdev)
 	del_timer_sync(&ionic->watchdog_timer);
 
 	if (ionic->lif) {
-		ionic_devlink_unregister(ionic);
 		ionic_lif_unregister(ionic->lif);
+		ionic_devlink_unregister(ionic);
 		ionic_lif_deinit(ionic->lif);
 		ionic_lif_free(ionic->lif);
 		ionic->lif = NULL;
-- 
2.37.1

Re: [patch net-next 3/3] ionic: change order of devlink port register and netdev register

[Shannon Nelson]

On 9/26/22 4:09 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Make sure that devlink port is registered first and register netdev
> after. Unregister netdev before devlnk port unregister.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
>   .../net/ethernet/pensando/ionic/ionic_bus_pci.c  | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> index 0a7a757494bc..ce436e97324a 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> @@ -320,16 +320,16 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>                          dev_err(dev, "Cannot enable existing VFs: %d\n", err);
>          }
> 
> -       err = ionic_lif_register(ionic->lif);
> +       err = ionic_devlink_register(ionic);
>          if (err) {
> -               dev_err(dev, "Cannot register LIF: %d, aborting\n", err);
> +               dev_err(dev, "Cannot register devlink: %d\n", err);
>                  goto err_out_deinit_lifs;
>          }
> 
> -       err = ionic_devlink_register(ionic);
> +       err = ionic_lif_register(ionic->lif);
>          if (err) {
> -               dev_err(dev, "Cannot register devlink: %d\n", err);
> -               goto err_out_deregister_lifs;
> +               dev_err(dev, "Cannot register LIF: %d, aborting\n", err);
> +               goto err_out_deregister_devlink;
>          }
> 
>          mod_timer(&ionic->watchdog_timer,
> @@ -337,8 +337,8 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> 
>          return 0;
> 
> -err_out_deregister_lifs:
> -       ionic_lif_unregister(ionic->lif);
> +err_out_deregister_devlink:
> +       ionic_devlink_unregister(ionic);
>   err_out_deinit_lifs:
>          ionic_vf_dealloc(ionic);
>          ionic_lif_deinit(ionic->lif);
> @@ -380,8 +380,8 @@ static void ionic_remove(struct pci_dev *pdev)
>          del_timer_sync(&ionic->watchdog_timer);
> 
>          if (ionic->lif) {
> -               ionic_devlink_unregister(ionic);
>                  ionic_lif_unregister(ionic->lif);
> +               ionic_devlink_unregister(ionic);
>                  ionic_lif_deinit(ionic->lif);
>                  ionic_lif_free(ionic->lif);
>                  ionic->lif = NULL;
> --
> 2.37.1
> 

That should be alright - thanks.  -sln

Acked-by: Shannon Nelson <snelson@pensando.io>

Re: [patch net-next 0/3] devlink: fix order of port and netdev register in drivers

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 26 Sep 2022 13:09:35 +0200 you wrote:
> From: Jiri Pirko <jiri@nvidia.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.
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] funeth: unregister devlink port after netdevice unregister
    https://git.kernel.org/netdev/net-next/c/dfe609491476
  - [net-next,2/3] ice: reorder PF/representor devlink port register/unregister flows
    https://git.kernel.org/netdev/net-next/c/a286ba738714
  - [net-next,3/3] ionic: change order of devlink port register and netdev register
    https://git.kernel.org/netdev/net-next/c/1fd7c08286ce

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [patch net-next 0/3] devlink: fix order of port and netdev register in drivers

[Lucero Palau, Alejandro]

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@nvidia.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
>

Re: [patch net-next 0/3] devlink: fix order of port and netdev register in drivers

[Jiri Pirko]

Tue, Oct 04, 2022 at 05:31:10PM CEST, alejandro.lucero-palau@amd.com wrote:
>Hi Jiri,

I don't understand why you send this as a reply to this patchset. I
don't see the relation to it.


>
>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?

Before. If devlink instance should be unregistered only after all other
related instances are gone.

Also, the devlink ports come and go during the devlink lifetime. When
you add a VF, split a port for example. There are many other cases.


>
>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 

What concurrency issue are you talking about?


>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 

It is not. As I wrote above, the devlink ports come and go.


>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@nvidia.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
>>
>

Re: [patch net-next 0/3] devlink: fix order of port and netdev register in drivers

[Lucero Palau, Alejandro]

On 10/5/22 09:49, Jiri Pirko wrote:
> Tue, Oct 04, 2022 at 05:31:10PM CEST, alejandro.lucero-palau@amd.com wrote:
>> Hi Jiri,
> I don't understand why you send this as a reply to this patchset. I
> don't see the relation to it.

I thought there was a relationship with ordering being the issue.

Apologies if this is not the right way for rising my concern.


>
>> 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?
> Before. If devlink instance should be unregistered only after all other
> related instances are gone.
>
> Also, the devlink ports come and go during the devlink lifetime. When
> you add a VF, split a port for example. There are many other cases.
>
>
>> 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
> What concurrency issue are you talking about?
>
1) devlink port function set ...

2) predoit inside devlink obtains devlink then the reference to devlink 
port. Code does a put on devlink but not on the devlink port.

3) driver is removed. devlink port is removed. devlink is not because 
the put.

4) devlink port reference is wrong.


>> 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
> It is not. As I wrote above, the devlink ports come and go.
>
>
>> 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@nvidia.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
>>>

Re: [patch net-next 0/3] devlink: fix order of port and netdev register in drivers

[Jiri Pirko]

Wed, Oct 05, 2022 at 10:18:29AM CEST, alejandro.lucero-palau@amd.com wrote:
>
>On 10/5/22 09:49, Jiri Pirko wrote:
>> Tue, Oct 04, 2022 at 05:31:10PM CEST, alejandro.lucero-palau@amd.com wrote:
>>> Hi Jiri,
>> I don't understand why you send this as a reply to this patchset. I
>> don't see the relation to it.
>
>I thought there was a relationship with ordering being the issue.
>
>Apologies if this is not the right way for rising my concern.
>
>
>>
>>> 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?
>> Before. If devlink instance should be unregistered only after all other
>> related instances are gone.
>>
>> Also, the devlink ports come and go during the devlink lifetime. When
>> you add a VF, split a port for example. There are many other cases.
>>
>>
>>> 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
>> What concurrency issue are you talking about?
>>
>1) devlink port function set ...
>
>2) predoit inside devlink obtains devlink then the reference to devlink 
>port. Code does a put on devlink but not on the devlink port.

devl_lock is taken here.


>
>3) driver is removed. devlink port is removed. devlink is not because 

devl_lock taken before port is removed and will block there.

I don't see any problem. Did you actually encoutered any problem?


>the put.
>
>4) devlink port reference is wrong.
>
>
>>> 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
>> It is not. As I wrote above, the devlink ports come and go.
>>
>>
>>> 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@nvidia.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
>>>>
>

Re: [patch net-next 0/3] devlink: fix order of port and netdev register in drivers

[Lucero Palau, Alejandro]

On 10/6/22 14:44, Jiri Pirko wrote:
> Wed, Oct 05, 2022 at 10:18:29AM CEST, alejandro.lucero-palau@amd.com wrote:
>> On 10/5/22 09:49, Jiri Pirko wrote:
>>> Tue, Oct 04, 2022 at 05:31:10PM CEST, alejandro.lucero-palau@amd.com wrote:
>>>> Hi Jiri,
>>> I don't understand why you send this as a reply to this patchset. I
>>> don't see the relation to it.
>> I thought there was a relationship with ordering being the issue.
>>
>> Apologies if this is not the right way for rising my concern.
>>
>>
>>>> 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?
>>> Before. If devlink instance should be unregistered only after all other
>>> related instances are gone.
>>>
>>> Also, the devlink ports come and go during the devlink lifetime. When
>>> you add a VF, split a port for example. There are many other cases.
>>>
>>>
>>>> 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
>>> What concurrency issue are you talking about?
>>>
>> 1) devlink port function set ...
>>
>> 2) predoit inside devlink obtains devlink then the reference to devlink
>> port. Code does a put on devlink but not on the devlink port.
> devl_lock is taken here.

This is embarrassing.

Somehow I misread the code assuming the protection was only based on the 
get operation, that the devlink lock was released there and not in the 
post_doit.

That goto unlock confused me, I guess, along with a bias looking for 
ordering issues.

Apologies.

Happy to see all is fine.

Thank you.

>
>> 3) driver is removed. devlink port is removed. devlink is not because
> devl_lock taken before port is removed and will block there.
>
> I don't see any problem. Did you actually encoutered any problem?
>
>
>> the put.
>>
>> 4) devlink port reference is wrong.
>>
>>
>>>> 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
>>> It is not. As I wrote above, the devlink ports come and go.
>>>
>>>
>>>> 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@nvidia.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
>>>>>

Re: [patch net-next 0/3] devlink: fix order of port and netdev register in drivers

[Jiri Pirko]

Thu, Oct 06, 2022 at 03:45:48PM CEST, alejandro.lucero-palau@amd.com wrote:
>
>On 10/6/22 14:44, Jiri Pirko wrote:
>> Wed, Oct 05, 2022 at 10:18:29AM CEST, alejandro.lucero-palau@amd.com wrote:
>>> On 10/5/22 09:49, Jiri Pirko wrote:
>>>> Tue, Oct 04, 2022 at 05:31:10PM CEST, alejandro.lucero-palau@amd.com wrote:
>>>>> Hi Jiri,
>>>> I don't understand why you send this as a reply to this patchset. I
>>>> don't see the relation to it.
>>> I thought there was a relationship with ordering being the issue.
>>>
>>> Apologies if this is not the right way for rising my concern.
>>>
>>>
>>>>> 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?
>>>> Before. If devlink instance should be unregistered only after all other
>>>> related instances are gone.
>>>>
>>>> Also, the devlink ports come and go during the devlink lifetime. When
>>>> you add a VF, split a port for example. There are many other cases.
>>>>
>>>>
>>>>> 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
>>>> What concurrency issue are you talking about?
>>>>
>>> 1) devlink port function set ...
>>>
>>> 2) predoit inside devlink obtains devlink then the reference to devlink
>>> port. Code does a put on devlink but not on the devlink port.
>> devl_lock is taken here.
>
>This is embarrassing.
>
>Somehow I misread the code assuming the protection was only based on the 
>get operation, that the devlink lock was released there and not in the 
>post_doit.
>
>That goto unlock confused me, I guess, along with a bias looking for 
>ordering issues.
>
>Apologies.

Np :) Happy to help.

>
>Happy to see all is fine.
>
>Thank you.
>
>>
>>> 3) driver is removed. devlink port is removed. devlink is not because
>> devl_lock taken before port is removed and will block there.
>>
>> I don't see any problem. Did you actually encoutered any problem?
>>
>>
>>> the put.
>>>
>>> 4) devlink port reference is wrong.
>>>
>>>
>>>>> 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
>>>> It is not. As I wrote above, the devlink ports come and go.
>>>>
>>>>
>>>>> 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@nvidia.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
>>>>>>
>