Re: [PATCH v5 04/11] HID: spi-hid: add spi-hid driver HID layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hid: output_report callback sleeps in atomic context
--

commit 954a3684cea1bad33a85c00ff6725f7553c75027
Author: Jingyuan Liang <jingyliang@chromium.org>

    HID: spi-hid: add spi-hid driver HID layer
    
    Add HID low level driver callbacks to register SPI as a HID driver, and
    an external touch device as a HID device.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 02a7608c4b882..fca7a44eeb9f9 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> +static int spi_hid_send_output_report(struct spi_hid *shid,
> +				      struct spi_hid_output_report *report)
> +{
> +	guard(mutex)(&shid->output_lock);
> +	return __spi_hid_send_output_report(shid, report);
> +}
> +
> +static int __spi_hid_sync_request(struct spi_hid *shid,
> +				  struct spi_hid_output_report *report)
> +{
> +	struct device *dev = &shid->spi->dev;
> +	int error;
> +
> +	reinit_completion(&shid->output_done);
> +
> +	error = __spi_hid_send_output_report(shid, report);
> +	if (error)
> +		return error;
> +
> +	error = wait_for_completion_interruptible_timeout(&shid->output_done,
> +							  msecs_to_jiffies(SPI_HID_RESP_TIMEOUT));
> +	if (error == 0) {
> +		dev_err(dev, "Response timed out\n");
> +		return -ETIMEDOUT;
> +	}
> +	if (error < 0)
> +		return error;
> +
> +	return 0;
> +}
> +
> +static int spi_hid_sync_request(struct spi_hid *shid,
> +				struct spi_hid_output_report *report)
> +{
> +	guard(mutex)(&shid->output_lock);
> +	return __spi_hid_sync_request(shid, report);
> +}
[ ... ]
> +static int spi_hid_ll_output_report(struct hid_device *hid, __u8 *buf,
> +				    size_t len)
> +{
> +	struct spi_device *spi = hid->driver_data;
> +	struct spi_hid *shid = spi_get_drvdata(spi);
> +	struct device *dev = &spi->dev;
> +	struct spi_hid_output_report report = {
> +		.report_type = OUTPUT_REPORT,
> +		.content_length = len - 1,
> +		.content_id = buf[0],
> +		.content = &buf[1],
> +	};
> +	int error;
> +
> +	if (!test_bit(SPI_HID_READY, &shid->flags)) {
> +		dev_err(dev, "%s called in unready state\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	if (shid->desc.no_output_report_ack)
> +		error = spi_hid_send_output_report(shid, &report);
> +	else
> +		error = spi_hid_sync_request(shid, &report);

[Severity: High]
Can this code sleep in an atomic context?

The HID subsystem requires the output_report callback to be asynchronous because
it can be invoked from atomic contexts, such as softirqs for LED triggers or
force feedback.

Both spi_hid_send_output_report() and spi_hid_sync_request() acquire a mutex,
which will sleep. Furthermore, spi_hid_sync_request() blocks by calling
wait_for_completion_interruptible_timeout() in __spi_hid_sync_request() while
waiting for the response.

Will calling these synchronous primitives from the output_report callback lead
to a scheduling while atomic BUG and cause a kernel panic?

> +
> +	if (error) {
> +		dev_err(dev, "failed to send output report\n");
> +		return error;
> +	}
> +
> +	return len;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-send-upstream-v5-0-384af01da3ee@chromium.org?part=4