Re: [PATCH iwl-next v3 3/6] devlink: add devlink_fmsg_dump_skb() function

[Simon Horman <horms@kernel.org>]

On Wed, Aug 21, 2024 at 03:37:11PM +0200, Przemek Kitszel wrote:
> From: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
> 
> Add devlink_fmsg_dump_skb() function that adds some diagnostic
> information about skb (like length, pkt type, MAC, etc) to devlink
> fmsg mechanism using bunch of devlink_fmsg_put() function calls.
> 
> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

...

> diff --git a/net/devlink/health.c b/net/devlink/health.c
> index acb8c0e174bb..b98ca650284c 100644
> --- a/net/devlink/health.c
> +++ b/net/devlink/health.c
> @@ -1241,3 +1241,70 @@ int devlink_nl_health_reporter_test_doit(struct sk_buff *skb,
>  
>  	return reporter->ops->test(reporter, info->extack);
>  }
> +
> +/**
> + * devlink_fmsg_dump_skb - Dump sk_buffer structure
> + * @fmsg: devlink formatted message pointer
> + * @skb: pointer to skb
> + *
> + * Dump diagnostic information about sk_buff structure, like headroom, length,
> + * tailroom, MAC, etc.
> + */
> +void devlink_fmsg_dump_skb(struct devlink_fmsg *fmsg, const struct sk_buff *skb)
> +{
> +	struct skb_shared_info *sh = skb_shinfo(skb);
> +	struct sock *sk = skb->sk;
> +	bool has_mac, has_trans;
> +
> +	has_mac = skb_mac_header_was_set(skb);
> +	has_trans = skb_transport_header_was_set(skb);
> +
> +	devlink_fmsg_pair_nest_start(fmsg, "skb");
> +	devlink_fmsg_obj_nest_start(fmsg);
> +	devlink_fmsg_put(fmsg, "actual len", skb->len);
> +	devlink_fmsg_put(fmsg, "head len", skb_headlen(skb));
> +	devlink_fmsg_put(fmsg, "data len", skb->data_len);
> +	devlink_fmsg_put(fmsg, "tail len", skb_tailroom(skb));
> +	devlink_fmsg_put(fmsg, "MAC", has_mac ? skb->mac_header : -1);
> +	devlink_fmsg_put(fmsg, "MAC len",
> +			 has_mac ? skb_mac_header_len(skb) : -1);
> +	devlink_fmsg_put(fmsg, "network hdr", skb->network_header);
> +	devlink_fmsg_put(fmsg, "network hdr len",
> +			 has_trans ? skb_network_header_len(skb) : -1);
> +	devlink_fmsg_put(fmsg, "transport hdr",
> +			 has_trans ? skb->transport_header : -1);
> +	devlink_fmsg_put(fmsg, "csum", skb->csum);

Hi,

One minor nit here, which I don't think needs to stop progress of this
patchset. Sparse warns that:

error: no generic selection for 'restricted __wsum const [usertype] csum'

I believe this can be addressed by casting: (__force __u32) skb->csum,
perhaps incorporated into devlink_fmsg_put(). Which seems fine enough for
this case.

However, my observation is that there are a lot of sparse warnings
present in the tree due to similar issues around the use of __wsum.
And IMHO naked casts are error prone and not obviously correct to the
reader (me). So I wonder if there is some value in introducing some
helpers. E.g.

	wsum_to_cpu()
	cpu_to_wsum()

To my mind, that would clearly be out of scope for this patchset.
But It seems appropriate to raise this as it's been on my mind for a while.

...