1. Patchew
  2. linux
  3. View series

[PATCH net-next] ax25: merge repeat codes in ax25_dev_device_down()

Lu Wei posted 1 patch 3 years, 11 months ago 
net/ax25/ax25_dev.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
[PATCH net-next] ax25: merge repeat codes in ax25_dev_device_down()
Posted by Lu Wei 3 years, 11 months ago 
Merge repeat codes to reduce the duplication.

Signed-off-by: Lu Wei <luwei32@huawei.com>
---
 net/ax25/ax25_dev.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index d2a244e1c260..b80fccbac62a 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -115,23 +115,13 @@ void ax25_dev_device_down(struct net_device *dev)
 
 	if ((s = ax25_dev_list) == ax25_dev) {
 		ax25_dev_list = s->next;
-		spin_unlock_bh(&ax25_dev_lock);
-		ax25_dev_put(ax25_dev);
-		dev->ax25_ptr = NULL;
-		dev_put_track(dev, &ax25_dev->dev_tracker);
-		ax25_dev_put(ax25_dev);
-		return;
+		goto unlock_put;
 	}
 
 	while (s != NULL && s->next != NULL) {
 		if (s->next == ax25_dev) {
 			s->next = ax25_dev->next;
-			spin_unlock_bh(&ax25_dev_lock);
-			ax25_dev_put(ax25_dev);
-			dev->ax25_ptr = NULL;
-			dev_put_track(dev, &ax25_dev->dev_tracker);
-			ax25_dev_put(ax25_dev);
-			return;
+			goto unlock_put;
 		}
 
 		s = s->next;
@@ -139,6 +129,14 @@ void ax25_dev_device_down(struct net_device *dev)
 	spin_unlock_bh(&ax25_dev_lock);
 	dev->ax25_ptr = NULL;
 	ax25_dev_put(ax25_dev);
+	return;
+
+unlock_put:
+	spin_unlock_bh(&ax25_dev_lock);
+	ax25_dev_put(ax25_dev);
+	dev->ax25_ptr = NULL;
+	dev_put_track(dev, &ax25_dev->dev_tracker);
+	ax25_dev_put(ax25_dev);
 }
 
 int ax25_fwd_ioctl(unsigned int cmd, struct ax25_fwd_struct *fwd)
-- 
2.17.1
Re: [PATCH net-next] ax25: merge repeat codes in ax25_dev_device_down()
Posted by Thomas Osterried 3 years, 11 months ago 
Hello,

three comments.


1. We need time for questions and discussions

In the past, we had several problems with patches that went upstream which
obviously not have been tested.
We have several requests by our community at linux-hams, that we need
to have a chance to read a patch proposal, and have time to test it,
before things become worse.


2. Due to some patches that went in the current torwalds-tree, ax25 became
unusable in a production environment (!).

I'll come to this in another mail, with description and proposal for a fix.
We are currently testing it and like to send it on linux-hams with request
to comment and test.


3. About this patch for ax25_dev_device_down() which reached netdev-next:

Looks good regarding the changes.

But when looking at it, it raises a question due to an older patch, that introduced
dev_put_track().
It's just a question of comprehension:
  If the position of the device that has to be removed is
    - at the head of the device list: do dev_put_track()
    - in the the device list or at the end: dev_put_track()
    - not in the device list: do _not_ dev_put_track().
      Why? - Not obviously clear. I think, because an interface could exist,
      but is set to down and thus is not part of the list (-> then you can't see
      it as /proc/sys/net/ax25/<name>).


-> Personally, I'd consider

- this better readable:

	int found = 0;

	if (ax25_dev_list == ax25_dev) {
		ax25_dev_list = s->next;
		found = 1;
	} else {
		for (s = ax25_dev_list; s != NULL && s->next != NULL; s = s->next) {
			if (s->next == ax25_dev) {
				s->next = s->next->next;
				found = 1;
				break;
			}
		}
	}

	spin_unlock_bh(&ax25_dev_lock);
	ax25_dev_put(ax25_dev);
	dev->ax25_ptr = NULL;
	if (found)
		dev_put_track(dev, &ax25_dev->dev_tracker);
	ax25_dev_put(ax25_dev);


- ..or with goto:

	int found = 1;

	if (ax25_dev_list == ax25_dev) {
		ax25_dev_list = s->next;
		goto out;
	}
	for (s = ax25_dev_list; s != NULL && s->next != NULL; s = s->next) {
		if (s->next == ax25_dev) {
			s->next = s->next->next;
			goto out;
		}
	}
	found = 0;

out:
	spin_unlock_bh(&ax25_dev_lock);
	ax25_dev_put(ax25_dev);
	dev->ax25_ptr = NULL;
	if (found)
		dev_put_track(dev, &ax25_dev->dev_tracker);
	ax25_dev_put(ax25_dev);



- ..than this:

	if ((s = ax25_dev_list) == ax25_dev) {
		ax25_dev_list = s->next;
		goto unlock_put;
	}

	while (s != NULL && s->next != NULL) {
		if (s->next == ax25_dev) {
			s->next = ax25_dev->next;
			goto unlock_put;
		}

		s = s->next;
	}
	spin_unlock_bh(&ax25_dev_lock);
	dev->ax25_ptr = NULL;
	ax25_dev_put(ax25_dev);
	return;

unlock_put:
	spin_unlock_bh(&ax25_dev_lock);
	ax25_dev_put(ax25_dev);
	dev->ax25_ptr = NULL;
	dev_put_track(dev, &ax25_dev->dev_tracker);
	ax25_dev_put(ax25_dev);



vy 73,
	- Thomas  dl9sau


On Mon, May 16, 2022 at 02:28:04PM +0800, Lu Wei wrote:
> Merge repeat codes to reduce the duplication.
> 
> Signed-off-by: Lu Wei <luwei32@huawei.com>
> ---
>  net/ax25/ax25_dev.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
> index d2a244e1c260..b80fccbac62a 100644
> --- a/net/ax25/ax25_dev.c
> +++ b/net/ax25/ax25_dev.c
> @@ -115,23 +115,13 @@ void ax25_dev_device_down(struct net_device *dev)
>  
>  	if ((s = ax25_dev_list) == ax25_dev) {
>  		ax25_dev_list = s->next;
> -		spin_unlock_bh(&ax25_dev_lock);
> -		ax25_dev_put(ax25_dev);
> -		dev->ax25_ptr = NULL;
> -		dev_put_track(dev, &ax25_dev->dev_tracker);
> -		ax25_dev_put(ax25_dev);
> -		return;
> +		goto unlock_put;
>  	}
>  
>  	while (s != NULL && s->next != NULL) {
>  		if (s->next == ax25_dev) {
>  			s->next = ax25_dev->next;
> -			spin_unlock_bh(&ax25_dev_lock);
> -			ax25_dev_put(ax25_dev);
> -			dev->ax25_ptr = NULL;
> -			dev_put_track(dev, &ax25_dev->dev_tracker);
> -			ax25_dev_put(ax25_dev);
> -			return;
> +			goto unlock_put;
>  		}
>  
>  		s = s->next;
> @@ -139,6 +129,14 @@ void ax25_dev_device_down(struct net_device *dev)
>  	spin_unlock_bh(&ax25_dev_lock);
>  	dev->ax25_ptr = NULL;
>  	ax25_dev_put(ax25_dev);
> +	return;
> +
> +unlock_put:
> +	spin_unlock_bh(&ax25_dev_lock);
> +	ax25_dev_put(ax25_dev);
> +	dev->ax25_ptr = NULL;
> +	dev_put_track(dev, &ax25_dev->dev_tracker);
> +	ax25_dev_put(ax25_dev);
>  }
>  
>  int ax25_fwd_ioctl(unsigned int cmd, struct ax25_fwd_struct *fwd)
> -- 
> 2.17.1
> 
>
Re: [PATCH net-next] ax25: merge repeat codes in ax25_dev_device_down()
Posted by Jakub Kicinski 3 years, 11 months ago 
On Wed, 18 May 2022 00:38:37 +0200 Thomas Osterried wrote:
> 1. We need time for questions and discussions
> 
> In the past, we had several problems with patches that went upstream which
> obviously not have been tested.
> We have several requests by our community at linux-hams, that we need
> to have a chance to read a patch proposal, and have time to test it,
> before things become worse.

Any input on:

https://patchwork.kernel.org/project/netdevbpf/patch/20220525112850.102363-1-duoming@zju.edu.cn/

?
Re: [PATCH net-next] ax25: merge repeat codes in ax25_dev_device_down()
Posted by patchwork-bot+netdevbpf@kernel.org 3 years, 11 months ago 
Hello:

This patch was applied to netdev/net-next.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Mon, 16 May 2022 14:28:04 +0800 you wrote:
> Merge repeat codes to reduce the duplication.
> 
> Signed-off-by: Lu Wei <luwei32@huawei.com>
> ---
>  net/ax25/ax25_dev.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)

Here is the summary with links:
  - [net-next] ax25: merge repeat codes in ax25_dev_device_down()
    https://git.kernel.org/netdev/net-next/c/a968c799eb1d

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

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.