Hello,

 think it's absolutely correct to state
  "Nothing takes the refcount since v4.9."
because in ax25_rt_add(),
  refcount_set(&ax25_rt->refcount, 1);
is used (for every new ax25_rt entry).

But nothing does an increment.
There's one function in include/net/ax25.h ,
  ax25_hold_route() which would refcount_inc(&ax25_rt->refcount)
but it's not called from anywhere.

=> It's value is always 1, and the deleting function ax25_put_route() decrements it again before freeing.
   I also see no sense in this (anymore).


Every struct ax25_route_list operation is assured with either
  write_lock_bh(&ax25_route_lock);
  write_unlock_bh(&ax25_route_lock);
or
  the struct ax25_route returned from functions is assured by calling read_lock(&ax25_route_lock).

-> No refcount is needed.


=> It's good to tidy up stuff that's needed anymore.

But keep in mind:
The code has strong similarities with include/net/x25.h and x25/x25_route.c ,
especially in the parts of ax25_hold_route() and ax25_rt_add().
This will get lost.


But there a things a bot does not know: human readable senteces.
ax25_get_route() is introduced with:

  /*
   *      Find AX.25 route
   *
   *      Only routes with a reference count of zero can be destroyed.
   *      Must be called with ax25_route_lock read locked.
   */

The first sentence informs: ax25_rt entries may be freed during the ax25_route_list operation.
It mentiones reference count (which will exist anymore).
The conclusion of the first sentence is "Must be called with ax25_route_lock read locked.". This is still true and assured.
I don't think it has to explain why the read lock is necessary (it's obvious, that routes could be deleted or added to the list). ->

  /*
   *      Find AX.25 route
   *
   *      Must be called with ax25_route_lock read locked.
   */

should be enough.

ff-topic:
=========

About read_lock)(): Inconsistent use.
It's
  directly called,
and by
  ax25_route_lock_use(), which calls read_lock():
  {
          read_lock(&ax25_route_lock);
  }

This makes the code harder to read.
There's also a function ax25_rt_seq_stop() that calls read_unlock() instead of calling ax25_route_lock_unuse(), which does the same.


vy 73,
	- Thomas  dl9sau

On Wed, Jan 26, 2022 at 11:11:00AM -0800, Jakub Kicinski wrote: