Re: [PATCH v14 1/5] docs: driver-api: gpio: rpmsg gpio driver over rpmsg bus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Missing endianness specification and implementation for multi-byte protocol fields.
- [High] The driver's direction_output callback causes hardware glitches by setting the direction before the initial value.
- [Medium] Inconsistent field sizes for REPLY and NOTIFICATION messages truncate the 'line' field.
- [Low] Incorrect command code in the SET_WAKEUP request diagram.
- [Low] Incorrect Linux error code usage (-ETIMEOUT).
- [Low] Grammatical error in SET_WAKEUP description.
--

commit 7712498eeadf5080c07364375a209ec4b75b63fd
Author: Shenwei Wang <shenwei.wang@nxp.com>
docs: driver-api: gpio: rpmsg gpio driver over rpmsg bus

This commit introduces the documentation for the GPIO RPMSG transport
protocol used over the RPMSG bus. It details the message formats,
command codes, and notification structures for communicating with
remote processors.

> diff --git a/Documentation/driver-api/gpio/gpio-rpmsg.rst b/Documentation/driver-api/gpio/gpio-rpmsg.rst
> new file mode 100644
> index 0000000000000..7d351ff0adb0e
> --- /dev/null
> +++ b/Documentation/driver-api/gpio/gpio-rpmsg.rst
> @@ -0,0 +1,271 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +GPIO RPMSG (Remote Processor Messaging) Protocol
> +================================================

[ ... ]

> +Message Format
> +--------------
> +
> +The RPMSG message consists of a 8-byte packet with the following layout:
> +
> +.. code-block:: none
> +
> +   +------+------+------+------+------+------+------+------+
> +   | 0x00 | 0x01 | 0x02 | 0x03 | 0x04 | 0x05 | 0x06 | 0x07 |
> +   |     cmd     |    line     |           value           |
> +   +------+------+------+------+------+------+------+------+

[Severity: High]
Does this message format lack an explicit endianness specification for the
multi-byte fields (cmd, line, value)?

Without defining the endianness here, the driver implementation in
drivers/gpio/gpio-rpmsg.c:rpmsg_gpio_msg_prepare() assigns native integer types
directly:

	msg->type = cmd;
	msg->gpio = line;
	msg->value = val;

Could this cause malformed packets when communicating between a big-endian host
and the remote processor?

> +
> +- **cmd**: Command code, used for GPIO_RPMSG_SEND messages.

[ ... ]

> +GPIO Commands
> +-------------
> +
> +Commands are specified in the **Cmd** field.
> +
> +The SEND message is always sent from Linux to the remote firmware. Each
> +SEND corresponds to a single REPLY message. The GPIO driver should
> +serialize messages and determine whether a REPLY message is required. If a
> +REPLY message is expected but not received within the specified timeout
> +period (currently 1 second in the Linux driver), the driver should return
> +-ETIMEOUT.

[Severity: Low]
Should this use the standard Linux kernel error code -ETIMEDOUT instead of
-ETIMEOUT?

> +
> +GET_DIRECTION (Cmd=2)

[ ... ]

> +SET_DIRECTION (Cmd=3)
> +~~~~~~~~~~~~~~~~~~~~~
> +
> +**Request:**
> +
> +.. code-block:: none
> +
> +   +------+------+------+------+------+------+------+------+
> +   | 0x00 | 0x01 | 0x02 | 0x03 | 0x04 | 0x05 | 0x06 | 0x07 |
> +   |      3      |    line     |           value           |
> +   +------+------+------+------+------+------+------+------+
> +
> +- **value**: Direction.
> +
> +  - 0: None
> +  - 1: Output
> +  - 2: Input

[Severity: High]
Does this command lack a parameter to specify the initial output level,
requiring a separate SET_VALUE call?

In drivers/gpio/gpio-rpmsg.c:rpmsg_gpio_direction_output(), the driver sends
the SET_DIRECTION command before the SET_VALUE command:

	rpmsg_gpio_msg_prepare(port, line, VIRTIO_GPIO_MSG_SET_DIRECTION,
			       VIRTIO_GPIO_DIRECTION_OUT);

	ret = rpmsg_gpio_send_message(port);
	if (ret)
		return ret;

	rpmsg_gpio_msg_prepare(port, line, VIRTIO_GPIO_MSG_SET_VALUE, val);

	return rpmsg_gpio_send_message(port);

Could this sequence cause a hardware glitch by driving an unintended default
state on the physical GPIO line during the IPC transit time, before the
SET_VALUE command is processed?

> +
> +**Reply:**

[ ... ]

> +SET_WAKEUP (Cmd=16)
> +~~~~~~~~~~~~~~~~~~~
> +
> +**Request:**
> +
> +.. code-block:: none
> +
> +   +------+------+------+------+------+------+------+------+
> +   | 0x00 | 0x01 | 0x02 | 0x03 | 0x04 | 0x05 | 0x06 | 0x07 |
> +   |      1      |    line     |           value           |
> +   +------+------+------+------+------+------+------+------+

[Severity: Low]
Does the command field in this diagram say '1' when it should say '16' or
'0x10' to match the SET_WAKEUP (Cmd=16) heading?

> +
> +- **value**: Wakeup enable.
> +
> +  The remote system should always aim to stay in a power-efficient state by
> +  shutting down or clock-gating the GPIO blocks that aren't in use. Since
> +  the remoteproc driver is responsible for managing the power states of the
> +  remote firmware, the GPIO driver does not require to know the firmware's
> +  running states.

[Severity: Low]
Should "does not require to know" be rephrased to "does not need to know" for
better grammar?

> +
> +  When the wakeup bit is set, the remote firmware should configure the line

[ ... ]

> +Notification Message
> +--------------------
> +
> +Notifications are sent by the remote core and they have
> +**Type=2 (GPIO_RPMSG_NOTIFY)**:
> +
> +When a GPIO line asserts an interrupt on the remote processor, the firmware
> +should immediately mask the corresponding interrupt source and send a
> +notification message to the Linux. Upon completion of the interrupt
> +handling on the Linux side, the driver should issue a
> +command **SET_IRQ_TYPE** to the firmware to unmask the interrupt.
> +
> +A Notification message can arrive between a SEND and its REPLY message,
> +and the driver is expected to handle this scenario.
> +
> +.. code-block:: none
> +
> +   +------+------+--------+
> +   | 0x00 | 0x01 |  0x02  |
> +   |   2  | line | trigger|
> +   +------+------+--------+

[Severity: Medium]
Does this notification message truncate the line index to 8 bits?

The SEND message diagram earlier explicitly defines an 8-byte layout with a
16-bit line field (occupying bytes 0x02 and 0x03). However, this NOTIFICATION
diagram and the driver's struct rpmsg_gpio_response in
drivers/gpio/gpio-rpmsg.c restrict the line index to an 8-bit __u8:

	struct rpmsg_gpio_response {
		...
		__u8 line;
		...
	};

If an interrupt occurs on a line index greater than 255, could this cause the
driver to mishandle the IRQ due to truncation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625155432.815185-1-shenwei.wang@oss.nxp.com?part=1