Re: [PATCH net-next v3 1/5] ipv4: igmp: get rid of IGMPV3_{QQIC,MRC} and simplify calculation

[Ido Schimmel <idosch@nvidia.com>]

On Fri, Apr 03, 2026 at 03:00:46PM +0000, Ujjal Roy wrote:
> Get rid of the IGMPV3_MRC macro and use the igmpv3_mrt() API to
> calculate the Max Resp Time from the Maximum Response Code.
> 
> Similarly, for IGMPV3_QQIC, use the igmpv3_qqi() API to calculate
> the Querier's Query Interval from the QQIC field.
> 
> Signed-off-by: Ujjal Roy <royujjal@gmail.com>

Looks fine, but see a few nits below

> ---
>  include/linux/igmp.h      | 84 +++++++++++++++++++++++++++++++++++----
>  net/bridge/br_multicast.c |  2 +-
>  net/ipv4/igmp.c           |  6 +--
>  3 files changed, 80 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/igmp.h b/include/linux/igmp.h
> index 073b30a9b850..0624410e75c6 100644
> --- a/include/linux/igmp.h
> +++ b/include/linux/igmp.h
> @@ -92,15 +92,83 @@ struct ip_mc_list {
>  	struct rcu_head		rcu;
>  };
>  
> +/* RFC3376, relevant sections:
> + *  - 4.1.1. Maximum Response Code
> + *  - 4.1.7. QQIC (Querier's Query Interval Code)
> + *
> + * For both MRC and QQIC, values >= 128 use the same floating-point
> + * encoding as follows:
> + *
> + *  0 1 2 3 4 5 6 7
> + * +-+-+-+-+-+-+-+-+
> + * |1| exp | mant  |
> + * +-+-+-+-+-+-+-+-+
> + */
> +#define IGMPV3_FP_EXP(value)		(((value) >> 4) & 0x07)
> +#define IGMPV3_FP_MAN(value)		((value) & 0x0f)
> +
> +/* IGMPV3 floating-point exponential field threshold */
> +#define IGMPV3_EXP_MIN_THRESHOLD	128
> +
>  /* V3 exponential field decoding */
> -#define IGMPV3_MASK(value, nb) ((nb)>=32 ? (value) : ((1<<(nb))-1) & (value))
> -#define IGMPV3_EXP(thresh, nbmant, nbexp, value) \
> -	((value) < (thresh) ? (value) : \
> -        ((IGMPV3_MASK(value, nbmant) | (1<<(nbmant))) << \
> -         (IGMPV3_MASK((value) >> (nbmant), nbexp) + (nbexp))))
> -
> -#define IGMPV3_QQIC(value) IGMPV3_EXP(0x80, 4, 3, value)
> -#define IGMPV3_MRC(value) IGMPV3_EXP(0x80, 4, 3, value)
> +
> +/* IGMPv3 QQIC/MRC 8-bit exponential field decode.
> + *
> + * RFC3376, 4.1.1 & 4.1.7. defines the decoding formula:
> + *      0 1 2 3 4 5 6 7
> + *     +-+-+-+-+-+-+-+-+
> + *     |1| exp | mant  |
> + *     +-+-+-+-+-+-+-+-+
> + * Max Resp Time = (mant | 0x10) << (exp + 3)
> + * QQI = (mant | 0x10) << (exp + 3)
> + */
> +static inline unsigned long igmpv3_exp_field_decode(const u8 code)
> +{
> +	/* RFC3376, relevant sections:
> +	 *  - 4.1.1. Maximum Response Code
> +	 *  - 4.1.7. QQIC (Querier's Query Interval Code)
> +	 */

I find it weird to have a comment at the beginning of the function when
there's already a comment above the function. Let's remove this comment
since this information is already present in the MRC/QQIC functions
below.

> +	if (code < IGMPV3_EXP_MIN_THRESHOLD) {
> +		return code;
> +	} else {
> +		unsigned long mc_man, mc_exp;
> +
> +		mc_exp = IGMPV3_FP_EXP(code);
> +		mc_man = IGMPV3_FP_MAN(code);
> +
> +		return (mc_man | 0x10) << (mc_exp + 3);
> +	}
> +}
> +
> +/* Calculate Max Resp Time from Maximum Response Code
> + *
> + * After decode, MRC represents the Maximum Response Time (MRT) in units
> + * of 0.1 seconds (100 ms).
> + */
> +static inline unsigned long igmpv3_mrt(const struct igmpv3_query *ih3)
> +{
> +	/* RFC3376, relevant sections:
> +	 *  - 4.1.1. Maximum Response Code
> +	 *  - 8.3. Query Response Interval
> +	 */

Please move this to the comment above the function

> +	return igmpv3_exp_field_decode(ih3->code);
> +}
> +
> +/* Calculate Querier's Query Interval from Querier's Query Interval Code
> + *
> + * After decode, QQIC represents the Querier's Query Interval in units
> + * of seconds.
> + */
> +static inline unsigned long igmpv3_qqi(const struct igmpv3_query *ih3)
> +{
> +	/* RFC3376, relevant sections:
> +	 *  - 4.1.7. QQIC (Querier's Query Interval Code)
> +	 *  - 8.2. Query Interval
> +	 *  - 8.12. Older Version Querier Present Timeout
> +	 *    (the [Query Interval] in the last Query received)
> +	 */

Likewise

> +	return igmpv3_exp_field_decode(ih3->qqic);
> +}
>  
>  static inline int ip_mc_may_pull(struct sk_buff *skb, unsigned int len)
>  {
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 881d866d687a..9fec76e887bc 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -3518,7 +3518,7 @@ static void br_ip4_multicast_query(struct net_bridge_mcast *brmctx,
>  			goto out;
>  
>  		max_delay = ih3->code ?
> -			    IGMPV3_MRC(ih3->code) * (HZ / IGMP_TIMER_SCALE) : 1;
> +			    igmpv3_mrt(ih3) * (HZ / IGMP_TIMER_SCALE) : 1;
>  	} else {
>  		goto out;
>  	}
> diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
> index a674fb44ec25..d7eff36d98c3 100644
> --- a/net/ipv4/igmp.c
> +++ b/net/ipv4/igmp.c
> @@ -991,7 +991,7 @@ static bool igmp_heard_query(struct in_device *in_dev, struct sk_buff *skb,
>  		 * different encoding. We use the v3 encoding as more likely
>  		 * to be intended in a v3 query.
>  		 */
> -		max_delay = IGMPV3_MRC(ih3->code)*(HZ/IGMP_TIMER_SCALE);
> +		max_delay = igmpv3_mrt(ih3) * (HZ / IGMP_TIMER_SCALE);
>  		if (!max_delay)
>  			max_delay = 1;	/* can't mod w/ 0 */
>  	} else { /* v3 */
> @@ -1006,7 +1006,7 @@ static bool igmp_heard_query(struct in_device *in_dev, struct sk_buff *skb,
>  			ih3 = igmpv3_query_hdr(skb);
>  		}
>  
> -		max_delay = IGMPV3_MRC(ih3->code)*(HZ/IGMP_TIMER_SCALE);
> +		max_delay = igmpv3_mrt(ih3) * (HZ / IGMP_TIMER_SCALE);
>  		if (!max_delay)
>  			max_delay = 1;	/* can't mod w/ 0 */
>  		WRITE_ONCE(in_dev->mr_maxdelay, max_delay);
> @@ -1016,7 +1016,7 @@ static bool igmp_heard_query(struct in_device *in_dev, struct sk_buff *skb,
>  		 * configured value.
>  		 */
>  		in_dev->mr_qrv = ih3->qrv ?: READ_ONCE(net->ipv4.sysctl_igmp_qrv);
> -		in_dev->mr_qi = IGMPV3_QQIC(ih3->qqic)*HZ ?: IGMP_QUERY_INTERVAL;
> +		in_dev->mr_qi = igmpv3_qqi(ih3) * HZ ? : IGMP_QUERY_INTERVAL;
>  
>  		/* RFC3376, 8.3. Query Response Interval:
>  		 * The number of seconds represented by the [Query Response
> -- 
> 2.43.0
>