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 <[email protected]> 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/[email protected]?part=4
