Re: [PATCH 2/2] power: supply: Add GL.iNet GL-E750 MCU battery driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gl-e750-mcu: unconditional buffer clearing drops async UART events
- [High] gl-e750-mcu: newline-less status replies block async JSON parsing
--

commit df0b44f7ad4b4bbb857745d8458ec2b575186c6f
Author: Serv Pol <servpol@tuta.io>

power: supply: Add GL.iNet GL-E750 MCU battery driver

This patch adds a power supply driver for the GL.iNet GL-E750 travel router's
status microcontroller. It polls the battery status via JSON over UART every
30 seconds, and shuts the system down cleanly if a shut_down JSON event is
received from the MCU.

> diff --git a/drivers/power/supply/gl-e750-mcu.c b/drivers/power/supply/gl-e750-mcu.c
> index 0000000000000..0799e41d58a9a 100644
> --- /dev/null
> +++ b/drivers/power/supply/gl-e750-mcu.c

[ ... ]

> +static size_t mcu_receive_buf(struct serdev_device *serdev, const u8 *data,
> +			      size_t count)
> +{

[ ... ]

> +		/* JSON events from the MCU come without a line ending */
> +		if (c == '}' && mcu->rx_line[0] == '{' &&
> +		    strncmp(mcu->rx_line, "{OK}", 4)) {

[Severity: High]
Does this logic correctly handle asynchronous events appended to a pending
status reply?

If the MCU replies to a status request without a newline, the reply stays in
rx_line until the 1.5s timeout. If an asynchronous JSON event (like
{"shut_down": "1"}) arrives during this window, it is appended to rx_line.

The fast-path JSON parser in mcu_receive_buf() will ignore the new event
because strncmp(mcu->rx_line, "{OK}", 4) evaluates to 0 (the buffer still
starts with the pending status reply).

> +			mcu->rx_line[mcu->rx_len] = '\0';
> +			mcu_handle_line(mcu, mcu->rx_line);
> +			mcu->rx_len = 0;
> +		}
> +	}
> +	spin_unlock_irqrestore(&mcu->lock, flags);
> +
> +	return count;
> +}

[ ... ]

> +static void mcu_poll_work(struct work_struct *work)
> +{

[ ... ]

> +	spin_lock_irqsave(&mcu->lock, flags);
> +	mcu->rx_len = 0;

[Severity: High]
Could this unconditional buffer clearing drop asynchronous UART events?

If the MCU is actively transmitting an asynchronous event (e.g., flipping the
power switch) when this 30-second poll triggers, the partially received message
currently in rx_line is discarded here. This can lead to an abrupt hard power
cut instead of a clean system shutdown.

> +	reinit_completion(&mcu->reply);
> +	spin_unlock_irqrestore(&mcu->lock, flags);
> +
> +	ret = serdev_device_write(mcu->serdev, mcu_status_req,
> +				  sizeof(mcu_status_req) - 1, timeout);
> +	if (ret < 0) {
> +		dev_warn_ratelimited(&mcu->serdev->dev, "write failed: %d\n", ret);
> +	} else if (!wait_for_completion_timeout(&mcu->reply, timeout)) {
> +		/* the reply may also come without a line ending */
> +		spin_lock_irqsave(&mcu->lock, flags);
> +		mcu->rx_line[mcu->rx_len] = '\0';
> +		if (!(mcu->rx_len > 4 && !strncmp(mcu->rx_line, "{OK}", 4) &&
> +		      mcu_parse_status(mcu, mcu->rx_line)))

[Severity: High]
Will this parse the combined buffer correctly?

When the 1.5s timeout subsequently triggers, mcu_parse_status() attempts to
parse the concatenated string. The parsing fails because the final numeric field
(cycles) is concatenated with the JSON event (e.g., 10{"shut_down": "1"}), which
fails strict string-to-integer conversion (kstrtoint()). Both messages are then
lost.

> +			dev_dbg(&mcu->serdev->dev, "no status reply\n");
> +		mcu->rx_len = 0;

[Severity: High]
Similarly, does clearing the buffer here truncate asynchronous events that were
just being received when the timeout expires?

Like the pre-emptive clear at the start of mcu_poll_work(), clearing rx_len
here on timeout risks discarding partially received asynchronous messages. Both
issues can result in missing the critical "shut_down" signal.

> +		spin_unlock_irqrestore(&mcu->lock, flags);
> +	}
> +
> +	schedule_delayed_work(&mcu->poll_work,
> +			      msecs_to_jiffies(MCU_POLL_INTERVAL_MS));
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-gl-e750-mcu-v1-0-499d2e31f786@tuta.io?part=2