On Tue, May 07, 2024 at 02:59:17PM +0530, Ratheesh Kannoth wrote:

On 2024-05-07 at 12:33:39, Duoming Zhou (duoming@zju.edu.cn) wrote:

quoted

The origin ax25_dev_list implements its own single linked list,
which is complicated and error-prone. For example, when deleting
the node of ax25_dev_list in ax25_dev_device_down(), we have to
operate on the head node and other nodes separately.

This patch uses kernel universal linked list to replace original
ax25_dev_list, which make the operation of ax25_dev_list easier.
There are two points that need to notice:

[1] We should add a check to judge whether the list is empty before
INIT_LIST_HEAD in ax25_dev_device_up(), otherwise it will empty the
list for each new ax25_dev added.

[2] We should do "dev->ax25_ptr = ax25_dev;" and "dev->ax25_ptr = NULL;"
while holding the spinlock, otherwise the ax25_dev_device_up() and
ax25_dev_device_down() could race, we're not guaranteed to find a match
ax25_dev in ax25_dev_device_down().

Suggested-by: Dan Carpenter <redacted>
Signed-off-by: Duoming Zhou <redacted>
-ax25_dev *ax25_dev_list;
+static struct list_head ax25_dev_list;
 DEFINE_SPINLOCK(ax25_dev_lock);

 ax25_dev *ax25_addr_ax25dev(ax25_address *addr)

@@ -34,7 +35,7 @@ ax25_dev *ax25_addr_ax25dev(ax25_address *addr)
 	ax25_dev *ax25_dev, *res = NULL;

 	spin_lock_bh(&ax25_dev_lock);
-	for (ax25_dev = ax25_dev_list; ax25_dev != NULL; ax25_dev = ax25_dev->next)
+	list_for_each_entry(ax25_dev, &ax25_dev_list, list)
 		if (ax25cmp(addr, (const ax25_address *)ax25_dev->dev->dev_addr) == 0) {
 			res = ax25_dev;
 			ax25_dev_hold(ax25_dev);

@@ -52,6 +53,9 @@ void ax25_dev_device_up(struct net_device *dev)
 {
 	ax25_dev *ax25_dev;

+	/* Initialized the list for the first entry */
+	if (!ax25_dev_list.next)
+		INIT_LIST_HEAD(&ax25_dev_list);

if you define ax25_dev_list using 'static LIST_HEAD(ax25_dev_list)', you need this conditional check and
initialization ?

Ah, yes.  That's the proper way to do it.

regards,
dan carpenter