On Fri, Oct 9, 2026 at 3:42 PM <[email protected]> wrote: > > 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
Not really. HID core doesn't call ->output_report() from atomic context (LED updates are sent from a workqueue), and as noted in my reply on the documentation patch, USB-HID and I2C-HID already sleep in this callback too.
