Re: [PATCH v4 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Embedded arrays `read_approval_header` and `read_approval_body` are used for SPI DMA transfers without cacheline alignment, risking memory corruption.
- [Medium] Unchecked return values of reset operations in `spi_hid_dev_init` and `spi_hid_resume`
--

commit ea35a3a23739acaca367a8d250ef25c96d22df36
Author: Angela Czubak <acz@semihalf.com>

HID: spi-hid: add transport driver skeleton for HID over SPI bus

Create spi-hid folder and add Kconfig and Makefile for spi-hid driver.
Add basic device structure, definitions, and probe/remove functions.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> new file mode 100644
> index 0000000000000..02a7608c4b882
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
> @@ -0,0 +1,213 @@

[ ... ]

> +/* Driver context */
> +struct spi_hid {
> +	struct spi_device	*spi;	/* spi device. */
> +	struct hid_device	*hid;	/* pointer to corresponding HID dev. */
> +
> +	struct spihid_ops	*ops;
> +	struct spi_hid_conf	*conf;
> +
> +	enum hidspi_power_state power_state;
> +
> +	u32 regulator_error_count;
> +	int regulator_last_error;
> +	u32 bus_error_count;
> +	int bus_last_error;
> +	u32 dir_count;	/* device initiated reset count. */
> +};

[Severity: High]
Does embedding the read_approval_header and read_approval_body arrays directly
in this struct for SPI DMA transfers risk memory corruption?

When these buffers are passed as tx_buf to spi_sync() (e.g. in
spi_hid_input_sync()), they lack the ____cacheline_aligned directive. On
architectures with non-coherent DMA, cache maintenance operations could
encompass adjacent struct members.

Concurrent CPU writes to those adjacent members during the DMA mapping window
will be destroyed, leading to memory corruption.

Should these buffers be cacheline aligned, or dynamically allocated?

[ ... ]

> +int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
> +		       struct spi_hid_conf *conf)
> +{

[ ... ]

> +	/*
> +	 * At the end of probe we initialize the device:
> +	 *   0) assert reset, bias the interrupt line
> +	 *   1) sleep minimal reset delay
> +	 *   2) request IRQ
> +	 *   3) power up the device
> +	 *   4) deassert reset (high)
> +	 * After this we expect an IRQ with a reset response.
> +	 */
> +
> +	shid->ops->assert_reset(shid->ops);
> +
> +	shid->ops->sleep_minimal_reset_delay(shid->ops);

[ ... ]

> +	error = shid->ops->power_up(shid->ops);
> +	if (error) {
> +		dev_err(dev, "%s: could not power up\n", __func__);
> +		return error;
> +	}
> +
> +	shid->ops->deassert_reset(shid->ops);

[Severity: Medium]
Can hardware reset failures be masked here?

The assert_reset and deassert_reset callbacks return integer error codes.
However, their return values are ignored in spi_hid_core_probe() (as well as
resume paths).

If deassert_reset() fails, the device may remain in reset, causing the driver
to hang waiting for a reset response IRQ that will never arrive.

Should these return values be checked and handled?

> +
> +	dev_dbg(dev, "%s: d3 -> %s\n", __func__,
> +		spi_hid_power_mode_string(shid->power_state));
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=3