Re: [net-next,v7,7/8] net: wwan: hwsim: support NMEA port emulation

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: wwan: hwsim: support NMEA port emulation

This commit adds NMEA port emulation to the WWAN hardware simulator for
GNSS port testing. The emulator generates GGA and RMC sentences every
second via a timer callback.

> diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c
> index 11d15dc39041..e4b1bbff9af2 100644
> --- a/drivers/net/wwan/wwan_hwsim.c
> +++ b/drivers/net/wwan/wwan_hwsim.c

[ ... ]

> +#if IS_ENABLED(CONFIG_GNSS)
> +#define NMEA_MAX_LEN		82	/* Max sentence length */
> +#define NMEA_TRAIL_LEN		5	/* '*' + Checksum + <CR><LF> */
> +#define NMEA_MAX_DATA_LEN	(NMEA_MAX_LEN - NMEA_TRAIL_LEN)
> +
> +static __printf(2, 3)
> +void wwan_hwsim_nmea_skb_push_sentence(struct sk_buff *skb,
> +				       const char *fmt, ...)
> +{
> +	unsigned char *s, *p;
> +	va_list ap;
> +	u8 cs = 0;
> +	int len;
> +
> +	s = skb_put(skb, NMEA_MAX_LEN + 1);	/* +'\0' */

[ ... ]

> +static void wwan_hwsim_nmea_emul_timer(struct timer_list *t)
> +{
> +	/* 43.74754722298909 N 11.25759835922875 E in DMM format */
> +	static const unsigned int coord[4 * 2] = { 43, 44, 8528, 0,
> +						   11, 15, 4559, 0 };
> +	struct wwan_hwsim_port *port = timer_container_of(port, t, nmea_emul.timer);
> +	struct sk_buff *skb;
> +	struct tm tm;
> +
> +	time64_to_tm(ktime_get_real_seconds(), 0, &tm);
> +
> +	mod_timer(&port->nmea_emul.timer, jiffies + HZ);	/* 1 second */
> +
> +	skb = alloc_skb(NMEA_MAX_LEN * 2, GFP_KERNEL);	/* GGA + RMC */
                       ^^^^^^^^^^^^^^     ^^^^^^^^^^

Two questions about this allocation:

1. The buffer size appears to be insufficient. The allocation requests
NMEA_MAX_LEN * 2 = 164 bytes, but wwan_hwsim_nmea_skb_push_sentence()
is called twice and each call uses skb_put(skb, NMEA_MAX_LEN + 1) which
reserves 83 bytes per sentence. The total needed is 166 bytes, which
exceeds the 164 bytes allocated. Would this cause skb_over_panic() to
trigger on the second skb_put() call?

2. This function is a timer callback registered via timer_setup() in
wwan_hwsim_nmea_emul_start(). Timer callbacks run in softirq context
where sleeping is not permitted. GFP_KERNEL allocations can sleep when
memory pressure triggers reclaim. Should this be GFP_ATOMIC instead to
avoid "scheduling while atomic" warnings?

> +	if (!skb)
> +		return;
> +
> +	wwan_hwsim_nmea_skb_push_sentence(skb,
> +					  "$GPGGA,%02u%02u%02u.000,%02u%02u.%04u,%c,%03u%02u.%04u,%c,1,7,1.03,176.2,M,55.2,M,,",
> +					  tm.tm_hour, tm.tm_min, tm.tm_sec,
> +					  coord[0], coord[1], coord[2],
> +					  coord[3] ? 'S' : 'N',
> +					  coord[4], coord[5], coord[6],
> +					  coord[7] ? 'W' : 'E');
> +
> +	wwan_hwsim_nmea_skb_push_sentence(skb,
> +					  "$GPRMC,%02u%02u%02u.000,A,%02u%02u.%04u,%c,%03u%02u.%04u,%c,0.02,31.66,%02u%02u%02u,,,A",
> +					  tm.tm_hour, tm.tm_min, tm.tm_sec,
> +					  coord[0], coord[1], coord[2],
> +					  coord[3] ? 'S' : 'N',
> +					  coord[4], coord[5], coord[6],
> +					  coord[7] ? 'W' : 'E',
> +					  tm.tm_mday, tm.tm_mon + 1,
> +					  (unsigned int)tm.tm_year - 100);
> +
> +	wwan_port_rx(port->wwan, skb);
> +}

[ ... ]