Re: [PATCH net-next 03/10] batman-adv: add missing kernel-doc comments

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 14647 bytes --]

> --- Patch [3]: [PATCH net-next 03/10] batman-adv: add missing kernel-doc comments ---
[...]
> > diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
> > index 22622283f59b0..a9e80330fcb63 100644
> > --- a/net/batman-adv/bat_iv_ogm.c
> > +++ b/net/batman-adv/bat_iv_ogm.c
> > @@ -171,6 +171,14 @@ batadv_iv_ogm_orig_get(struct batadv_priv *bat_priv, const u8 *addr)
> >  	return NULL;
> >  }
> >  
> > +/**
> > + * batadv_iv_ogm_neigh_new() - retrieve or create a B.A.T.M.A.N. IV neighbour
> > + * @hard_iface: the interface where the neighbour is connected to
> > + * @neigh_addr: the mac address of the neighbour
> > + * @orig_node: originator object representing the neighbour
> > + *
> > + * Return: pointer to the neigh_node or NULL in case of failure
> > + */
> >  static struct batadv_neigh_node *
> >  batadv_iv_ogm_neigh_new(struct batadv_hard_iface *hard_iface,
> >  			const u8 *neigh_addr,
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Should the Return: line mention that the neighbour comes back with an
> elevated refcount?  batadv_iv_ogm_neigh_new() just forwards the result of
> batadv_neigh_node_get_or_create(), so the caller owns a reference that has
> to be released with batadv_neigh_node_put().

Often not done this way. Somebody interested in writing kernel-doc is free to 
adjust this everywhere in batman-adv.

I will not submit directly a patch.

> 
> [ ... ]
> 
> > @@ -1765,6 +1873,14 @@ static void batadv_iv_ogm_process(const struct sk_buff *skb, int ogm_offset,
> >  	batadv_orig_node_put(orig_node);
> >  }
> >  
> > +/**
> > + * batadv_iv_send_outstanding_bat_ogm_packet() - work-queue helper to emit a
> > + *  queued forward packet
> > + * @work: work item embedded in the forward packet
> > + *
> > + * Emit the queued OGM forward packet and, for own primary-interface packets,
> > + * schedule the next periodic OGM. The forward packet is freed afterwards.
> > + */
> >  static void batadv_iv_send_outstanding_bat_ogm_packet(struct work_struct *work)
> >  {
> >  	struct delayed_work *delayed_work;
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Is "for own primary-interface packets" the condition that is actually
> implemented?  The body reads:
> 
> 	if (forw_packet->own &&
> 	    forw_packet->if_incoming == forw_packet->if_outgoing)
> 		batadv_iv_ogm_schedule(forw_packet->if_incoming);
> 
> and batadv_iv_ogm_schedule_buff() queues own OGMs of secondary interfaces
> with own == 1 and if_incoming == if_outgoing as well:
> 
> 	scheduled = batadv_iv_ogm_queue_add(bat_priv, ogm_buff->buf, ogm_buff->len,
> 					    hard_iface, hard_iface, 1, send_time);
> 
> so the re-schedule branch is taken for secondary interfaces too.

It is about the primary interface packets. See __batadv_forw_bcast_packet's 
call of batadv_forw_bcast_packet_if. See parameters primary_if (incoming_if) 
and hard_iface (outgoing_if). Check batadv_forw_bcast_packet_to_list() to find 
the only place which sets own_packet.

[...]
> Separately, is "The forward packet is freed afterwards" true in all cases?
> The free only happens when the steal succeeds:
> 
> 	if (batadv_forw_packet_steal(forw_packet,
> 				     &bat_priv->forw_bat_list_lock))
> 		batadv_forw_packet_free(forw_packet, dropped);

See documentation of batadv_forw_packet_steal. For the caller, the skb is no longer usable.

> [ ... ]
> 
> > @@ -2328,6 +2461,14 @@ static void batadv_iv_init_sel_class(struct batadv_priv *bat_priv)
> >  	WRITE_ONCE(bat_priv->gw.sel_class, 20);
> >  }
> >  
> > +/**
> > + * batadv_iv_gw_get_best_gw_node() - retrieve the best gateway node based on
> > + *  the B.A.T.M.A.N. IV metric and the configured GW selection class
> > + * @bat_priv: the bat priv with all the mesh interface information
> > + *
> > + * Return: gateway node with the highest score for the current selection class,
> > + *  or NULL if no eligible gateway exists.
> > + */
> >  static struct batadv_gw_node *
> >  batadv_iv_gw_get_best_gw_node(struct batadv_priv *bat_priv)
> >  {
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Same question for the returned reference here: curr_gw is returned with an
> extra kref taken inside the RCU loop:
> 
> 				curr_gw = gw_node;
> 				kref_get(&curr_gw->refcount);
> 
> Should the Return: line say the gateway node is returned with increased
> refcount, like batadv_gw_get_selected_orig() does?

Often not done this way. Somebody interested in writing kernel-doc is free to 
adjust this everywhere in batman-adv.

I will not submit directly a patch.

> > diff --git a/net/batman-adv/bat_v.c b/net/batman-adv/bat_v.c
> > index db6f5bdcaa985..0068f0e238da6 100644
> > --- a/net/batman-adv/bat_v.c
> > +++ b/net/batman-adv/bat_v.c
> 
> [ ... ]
> 
> > @@ -444,6 +475,16 @@ batadv_v_orig_dump(struct sk_buff *msg, struct netlink_callback *cb,
> >  	cb->args[2] = sub;
> >  }
> >  
> > +/**
> > + * batadv_v_neigh_cmp() - compare two B.A.T.M.A.N. V neighbours by throughput
> > + * @neigh1: first neighbour to compare
> > + * @if_outgoing1: outgoing interface to use for @neigh1
> > + * @neigh2: second neighbour to compare
> > + * @if_outgoing2: outgoing interface to use for @neigh2
> > + *
> > + * Return: a positive value if @neigh1 is better, a negative value if @neigh2
> > + *  is better and 0 if both have equal throughput
> > + */
> >  static int batadv_v_neigh_cmp(struct batadv_neigh_node *neigh1,
> >  			      struct batadv_hard_iface *if_outgoing1,
> >  			      struct batadv_neigh_node *neigh2,
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Is 0 only returned for equal throughput?  batadv_v_neigh_cmp() also returns
> the initial value on the error paths when the ifinfo lookup fails:
> 
> 	int ret = 0;
> 
> 	ifinfo1 = batadv_neigh_ifinfo_get(neigh1, if_outgoing1);
> 	if (!ifinfo1)
> 		goto err_ifinfo1;
> 
> 	ifinfo2 = batadv_neigh_ifinfo_get(neigh2, if_outgoing2);
> 	if (!ifinfo2)
> 		goto err_ifinfo2;
> 
> A caller cannot distinguish that from "both have equal throughput".  Should
> the Return: description cover the failed lookup case?

A person more proficient in writing kernel-doc is free to do this and update 
it everywhere. Will not submit a patch.


> > diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
> > index b6867576bbafa..1950b8809d99f 100644
> > --- a/net/batman-adv/hard-interface.c
> > +++ b/net/batman-adv/hard-interface.c
> 
> [ ... ]
> 
> > @@ -467,6 +478,14 @@ int batadv_hardif_no_broadcast(struct batadv_hard_iface *if_outgoing,
> >  	return ret;
> >  }
> >  
> > +/**
> > + * batadv_hardif_get_active() - retrieve an active hard interface for a mesh
> > + *  interface
> > + * @mesh_iface: mesh interface to search
> > + *
> > + * Return: first hard interface in BATADV_IF_ACTIVE state attached to
> > + *  @mesh_iface, or NULL if none is active.
> > + */
> >  static struct batadv_hard_iface *
> >  batadv_hardif_get_active(struct net_device *mesh_iface)
> >  {
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> The returned interface carries a reference taken in the loop:
> 
> 		if (hard_iface->if_status == BATADV_IF_ACTIVE &&
> 		    kref_get_unless_zero(&hard_iface->refcount))
> 			goto out;
> 
> Should the Return: line state that the caller has to release it with
> batadv_hardif_put()?

Often not done this way. Somebody interested in writing kernel-doc is free to 
adjust this everywhere in batman-adv.

I will not submit directly a patch.

> [ ... ]
> 
> > @@ -502,6 +530,15 @@ static void batadv_primary_if_update_addr(struct batadv_priv *bat_priv,
> >  	batadv_hardif_put(primary_if);
> >  }
> >  
> > +/**
> > + * batadv_primary_if_select() - select the new primary interface
> > + * @bat_priv: the bat priv with all the mesh interface information
> > + * @new_hard_iface: new primary interface, may be NULL
> > + *
> > + * Replace the currently selected primary interface with @new_hard_iface,
> > + * invoke the algorithm-specific primary_set hook and update the originator
> > + * MAC address.
> > + */
> >  static void batadv_primary_if_select(struct batadv_priv *bat_priv,
> >  				     struct batadv_hard_iface *new_hard_iface)
> >  {
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Would it be worth documenting the locking context here?  The writer
> serialization of batadv_primary_if_select() comes from RTNL only:
> 
> 	ASSERT_RTNL();
> 
> 	if (new_hard_iface)
> 		kref_get(&new_hard_iface->refcount);
> 
> 	curr_hard_iface = rcu_replace_pointer(bat_priv->primary_if,
> 					      new_hard_iface, 1);
> 
> A contributor promoting this helper based on the new comment alone would not
> learn that RTNL has to be held.

Often not done this way. Somebody interested in writing kernel-doc is free to 
adjust this everywhere in batman-adv.

I will not submit directly a patch.


> [ ... ]
> 
> > @@ -534,6 +578,15 @@ batadv_hardif_is_iface_up(const struct batadv_hard_iface *hard_iface)
> >  	return false;
> >  }
> >  
> > +/**
> > + * batadv_check_known_mac_addr() - warn about duplicate hard interface MAC
> > + *  addresses
> > + * @hard_iface: hard interface that was just added or had its MAC changed
> > + *
> > + * Iterate over all hard interfaces of the same mesh interface and emit a
> > + * warning if another in-use interface shares the same MAC address as
> > + * @hard_iface.
> > + */
> >  static void batadv_check_known_mac_addr(const struct batadv_hard_iface *hard_iface)
> >  {
> >  	struct net_device *mesh_iface = hard_iface->mesh_iface;
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Same question for this one: the lower device list is walked with the
> non-RCU iterator, which requires RTNL:
> 
> 	netdev_for_each_lower_private(mesh_iface, tmp_hard_iface, iter) {
> 
> Should the new comment note that the caller must hold RTNL?

Often not done this way. Somebody interested in writing kernel-doc is free to adjust this everywhere in batman-adv.

I will not submit directly a patch.

> > diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c
> > index 73becb0549488..78b81daaeff9a 100644
> > --- a/net/batman-adv/main.c
> > +++ b/net/batman-adv/main.c
> 
> [ ... ]
> 
> > @@ -492,6 +513,13 @@ int batadv_batman_skb_recv(struct sk_buff *skb, struct net_device *dev,
> >  	return NET_RX_DROP;
> >  }
> >  
> > +/**
> > + * batadv_recv_handler_init() - initialise the RX handler dispatch table
> > + *
> > + * Initialise all entries of the RX handler table as either "unhandled" or with
> > + * protocol indepentend handlers, and perform compile-time size sanity checks on
>                     ^^^^^^^^^^^
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Typo, "indepentend" should be "independent".

Will not send a resent the patches because of thos. But an interested 
contributor can go through the whole code and fix various typos.

> 
> > + * all on-wire packet structs.
> > + */
> >  static void batadv_recv_handler_init(void)
> >  {
> >  	int i;
> 
> > diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
> > index dae5e1d8c0385..b12ec8d1b5451 100644
> > --- a/net/batman-adv/translation-table.c
> > +++ b/net/batman-adv/translation-table.c
> 
> [ ... ]
> 
> > @@ -1224,6 +1239,17 @@ int batadv_tt_local_dump(struct sk_buff *msg, struct netlink_callback *cb)
> >  	return ret;
> >  }
> >  
> > +/**
> > + * batadv_tt_local_set_pending() - mark a local TT entry as pending removal
> > + * @bat_priv: the bat priv with all the mesh interface information
> > + * @tt_local_entry: local TT entry to mark
> > + * @flags: TT change flags to announce together with the pending removal
> > + * @message: debug message describing the reason for the change
> > + *
> > + * Schedule the TT change announcement and set BATADV_TT_CLIENT_PENDING on the
> > + * entry. The entry is kept in the local table until the next TTVN increment
> > + * so that a consistency-check response can still be answered.
> > + */
> >  static void
> >  batadv_tt_local_set_pending(struct batadv_priv *bat_priv,
> >  			    struct batadv_tt_local_entry *tt_local_entry,
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Should the calling context be part of this comment?  One caller runs it with
> the TT hash bucket lock held:
> 
> batadv_tt_local_purge() {
> 	...
> 		spin_lock_bh(list_lock);
> 		batadv_tt_local_purge_list(bat_priv, head, timeout);
> 		spin_unlock_bh(list_lock);
> 	...
> }
> 
> and batadv_tt_local_purge_list() then calls batadv_tt_local_set_pending(),
> so the helper runs in atomic context with that spinlock held.

Uhm, but the list_lock is there because of the list access in 
batadv_tt_local_purge_list() and not because of batadv_tt_local_set_pending().

And it is rather uncommon to document what callers do.

> 
> [ ... ]
> 
> > @@ -2338,6 +2419,16 @@ static void batadv_tt_global_table_free(struct batadv_priv *bat_priv)
> >  	bat_priv->tt.global_hash = NULL;
> >  }
> >  
> > +/**
> > + * _batadv_is_ap_isolated() - check whether two clients are AP-isolated from
> > + *  each other
> > + * @tt_local_entry: local TT entry of the sending client
> > + * @tt_global_entry: global TT entry of the destination client
> > + *
> > + * Return: true if traffic between the two clients should be dropped because
> > + *  either both are WiFi clients or both carry the ISOLATION flag; false
> > + *  otherwise
> > + */
> >  static bool
> >  _batadv_is_ap_isolated(struct batadv_tt_local_entry *tt_local_entry,
> >  		       struct batadv_tt_global_entry *tt_global_entry)
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Do the sending/destination roles hold for both call sites?
> batadv_is_ap_isolated() passes the destination as the local entry and the
> source as the global entry:
> 
> 	tt_local_entry = batadv_tt_local_hash_find(bat_priv, dst, vid);
> 	...
> 	tt_global_entry = batadv_tt_global_hash_find(bat_priv, src, vid);
> 	...
> 	if (_batadv_is_ap_isolated(tt_local_entry, tt_global_entry))
> 
> which is the reverse of the documented roles, while batadv_transtable_search()
> matches them (local from src, global from addr).  The current test is
> symmetric, so nothing misbehaves today; would a wording without the
> sender/destination roles describe both callers?

Might send a patch later because this can really be confusing.

Regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]