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

["Slark Xiao" <slark_xiao@163.com>]

At 2026-01-16 10:44:41, "Jakub Kicinski" <kuba@kernel.org> wrote:
>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 */
>                       ^^^^^^^^^^^^^^     ^^^^^^^^^^
>
Hi Sergey, Loic,
Let's update it as :
+	skb = alloc_skb(NMEA_MAX_LEN * 2+2, GFP_ATOMIC);	/* GGA + RMC */

How do you think?


>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);
>> +}
>
>[ ... ]