On Mon, Oct 19, 2026 at 12:22 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s)
> to consider:
> - [High] hid: spi-hid: spi_hid_bus_validate_header() rejects descriptors >
> max_input
> - [High] hid: spi-hid: spi_hid_dev_desc_response() clears reset_attempts too
> early
> - [Medium] hid: spi-hid: spi_hid_bus_validate_header() allows 0-length report
> body
> - [Medium] hid: spi-hid: stale flags and duplicate SPI_HID_ERROR cause
> spurious resets
> - [Medium] hid: spi-hid: unaborted sync request in __spi_hid_sync_request()
> on reset
> --
>
> commit 68d1db2b55c0e004ec2c06854ae93db414465298
> Author: Jingyuan Liang <[email protected]>
>
> HID: spi-hid: add HID SPI protocol implementation
>
> This commit implements the HID Over SPI Protocol Specification 1.0 in
> drivers/hid/spi-hid/spi-hid-core.c, handling SPI input/output transfers,
> descriptor parsing, interrupt processing, and error recovery resets.
>
> > diff --git a/drivers/hid/spi-hid/spi-hid-core.c
> > b/drivers/hid/spi-hid/spi-hid-core.c
> > index fca7a44eeb9f9..90b2c2fea6501 100644
> > --- a/drivers/hid/spi-hid/spi-hid-core.c
> > +++ b/drivers/hid/spi-hid/spi-hid-core.c
>
> [ ... ]
>
> > @@ -187,17 +350,94 @@ static const char *spi_hid_power_mode_string(enum
> > hidspi_power_state power_state
> >
> > static void spi_hid_stop_hid(struct spi_hid *shid)
> > {
> > - struct hid_device *hid = shid->hid;
> > + struct hid_device *hid;
> >
> > - shid->hid = NULL;
> > - clear_bit(SPI_HID_READY, &shid->flags);
> > + scoped_guard(mutex, &shid->io_lock) {
> > + hid = shid->hid;
> > + shid->hid = NULL;
> > + clear_bit(SPI_HID_READY, &shid->flags);
> > + }
> >
> > if (hid)
> > hid_destroy_device(hid);
> > }
> >
> > +static void spi_hid_error_handler(struct spi_hid *shid)
> > +{
> > + struct device *dev = &shid->spi->dev;
> > + int error;
> > +
> > + guard(mutex)(&shid->power_lock);
> > + if (shid->power_state == HIDSPI_OFF)
> > + return;
> > +
> > + disable_irq(shid->spi->irq);
> > +
> > + if (shid->reset_attempts++ >= SPI_HID_MAX_RESET_ATTEMPTS) {
> > + dev_err(dev, "unresponsive device, aborting\n");
> > + spi_hid_stop_hid(shid);
> > + shid->ops->assert_reset(shid->ops);
> > + error = shid->ops->power_down(shid->ops);
> > + if (error) {
> > + dev_err(dev, "failed to disable regulator\n");
> > + shid->regulator_error_count++;
> > + shid->regulator_last_error = error;
> > + }
> > + shid->power_state = HIDSPI_OFF;
> > + /* Dead device: leave the IRQ permanently disabled. */
> > + return;
> > + }
> > +
> > + clear_bit(SPI_HID_READY, &shid->flags);
> > + set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> > +
> > + shid->ops->assert_reset(shid->ops);
> > +
> > + shid->power_state = HIDSPI_OFF;
> > +
> > + /*
> > + * We want to cancel pending reset work as the device is being reset
> > + * to recover from an error. cancel_work_sync will put us in a
> > deadlock
> > + * because this function is scheduled in 'reset_work' and we should
> > + * avoid waiting for itself.
> > + */
> > + cancel_work(&shid->reset_work);
>
> [Severity: Medium]
> Should spi_hid_error_handler() also clear pending SPI_HID_RESET_RESPONSE,
> SPI_HID_CREATE_DEVICE, and SPI_HID_ERROR bits in shid->flags here?
Will fix this in v7.
>
> Furthermore, in __spi_hid_sync_request(), a timeout already sets
> SPI_HID_ERROR and schedules shid->reset_work, but its callers
> (spi_hid_reset_response(), spi_hid_report_descriptor_request(), and
> spi_hid_get_request()) unconditionally set SPI_HID_ERROR and schedule
> shid->reset_work a second time on error:
Calling them twice has no extra effect. At worst this causes one
extra reset, bounded by reset_attempts.
>
> spi_hid_get_request()
> __spi_hid_sync_request()
> set_bit(SPI_HID_ERROR, &shid->flags);
> schedule_work(&shid->reset_work);
> return -ETIMEDOUT;
> set_bit(SPI_HID_ERROR, &shid->flags);
> schedule_work(&shid->reset_work);
>
> If shid->reset_work starts on another CPU after the first schedule_work()
> and clears SPI_HID_ERROR via test_and_clear_bit(), the caller can set
> SPI_HID_ERROR and queue shid->reset_work again. If cancel_work() cancels
> the work item without clearing the flag bits, the stale SPI_HID_ERROR bit
> remains set in shid->flags and will trigger a second reset when
> spi_hid_reset_work() later checks pending flags to reschedule itself. And
> if the second schedule_work() runs after cancel_work(), reset_work will run
> a second time immediately.
>
> Also, if spi_hid_get_request() is called while SPI_HID_READY is cleared,
> __spi_hid_send_output_report() returns -ENODEV, which spi_hid_get_request()
> treats as a fatal error by setting SPI_HID_ERROR and scheduling a hardware
> reset.
Will fix this in v7.
>
> Finally, in spi_hid_reset_work(), SPI_HID_RESET_RESPONSE and
> SPI_HID_CREATE_DEVICE are checked before SPI_HID_ERROR in the if/else-if
> chain, so if SPI_HID_ERROR is set while one of those bits is pending, the
> worker will attempt normal initialization before running
> spi_hid_error_handler().
This ordering is intentional.
>
> > +
> > + shid->ops->sleep_minimal_reset_delay(shid->ops);
> > +
> > + shid->power_state = HIDSPI_ON;
> > +
> > + shid->ops->deassert_reset(shid->ops);
> > +
> > + enable_irq(shid->spi->irq);
> > +}
>
> [ ... ]
>
> > @@ -234,39 +491,208 @@ 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);
> > + return __spi_hid_send_output_report(shid, report, 0);
> > }
> >
> > 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);
> > + error = __spi_hid_send_output_report(shid, report,
> > +
> > spi_hid_response_type(report->report_type));
> > 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 (wait_for_completion_timeout(&shid->output_done,
> > +
> > msecs_to_jiffies(SPI_HID_RESP_TIMEOUT)))
> > + return 0;
> > +
> > + /* Drop late responses and block new HID requests until the reset. */
> > + scoped_guard(mutex, &shid->io_lock) {
> > + shid->expected_response = 0;
> > + clear_bit(SPI_HID_READY, &shid->flags);
> > }
> > - if (error < 0)
> > - return error;
> >
> > - return 0;
> > + dev_err(dev, "Response timed out\n");
> > + /* The device may still answer later: resync it with a reset. */
> > + set_bit(SPI_HID_ERROR, &shid->flags);
> > + schedule_work(&shid->reset_work);
> > + return -ETIMEDOUT;
> > }
>
> [Severity: Medium]
> What happens if a spontaneous firmware reset (RESET_RESPONSE in
> spi_hid_process_input_report()) or an IRQ error (spi_hid_error_handler())
> occurs while a synchronous request is waiting on shid->output_done in
> __spi_hid_sync_request() with shid->output_lock held?
>
> Because neither the RESET_RESPONSE path nor spi_hid_error_handler() aborts
> the in-flight request or wakes shid->output_done, the waiter continues
> holding shid->output_lock for the full SPI_HID_RESP_TIMEOUT (blocking
> spi_hid_reset_response() in reset_work) and then sets SPI_HID_ERROR and
> schedules reset_work when the timeout expires, resetting the device a
> second time after recovery.
The waiter times out after SPI_HID_RESP_TIMEOUT (1s) and triggers one
extra reset, bounded by reset_attempts. This only happens if the
device resets during a request, so I'd like to keep it as is for now
and look at aborting in-flight requests in a follow-up.
>
> [ ... ]
>
> > +static int spi_hid_dev_desc_response(struct spi_hid *shid,
> > + struct spi_hid_input_report *body)
> > +{
> > + struct hidspi_dev_descriptor *raw =
> > + (struct hidspi_dev_descriptor *)shid->input->content;
> > + struct device *dev = &shid->spi->dev;
> > +
> > + /* Validate device descriptor length before parsing */
> > + if (body->content_length != HIDSPI_DEVICE_DESCRIPTOR_SIZE) {
> > + dev_err(dev, "Invalid content length %d, expected %zu\n",
> > + body->content_length, HIDSPI_DEVICE_DESCRIPTOR_SIZE);
> > + return -EPROTO;
> > + }
> > +
> > + if (le16_to_cpu(raw->dev_desc_len) != HIDSPI_DEVICE_DESCRIPTOR_SIZE) {
> > + dev_err(dev, "Invalid wDeviceDescLength %d, expected %zu\n",
> > + le16_to_cpu(raw->dev_desc_len),
> > + HIDSPI_DEVICE_DESCRIPTOR_SIZE);
> > + return -EPROTO;
> > + }
> > +
> > + if (le16_to_cpu(raw->bcd_ver) != SPI_HID_SUPPORTED_VERSION) {
> > + dev_err(dev, "Unsupported device descriptor version %4x\n",
> > + le16_to_cpu(raw->bcd_ver));
> > + return -EPROTONOSUPPORT;
> > + }
> > +
> > + /* Fail before reset_attempts is cleared so resets stay bounded. */
> > + if (!le16_to_cpu(raw->rep_desc_len) ||
> > + le16_to_cpu(raw->rep_desc_len) > HID_MAX_DESCRIPTOR_SIZE) {
> > + dev_err(dev, "Invalid wReportDescLength %d, max %d\n",
> > + le16_to_cpu(raw->rep_desc_len),
> > + HID_MAX_DESCRIPTOR_SIZE);
> > + return -EPROTO;
> > + }
> > +
> > + spi_hid_parse_dev_desc(raw, &shid->desc);
> > +
> > + /* Reset attempts at every device descriptor fetch */
> > + shid->reset_attempts = 0;
> > + set_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
> > + schedule_work(&shid->reset_work);
> > +
> > + return 0;
> > +}
>
> [Severity: High]
> Does clearing shid->reset_attempts = 0 here in spi_hid_dev_desc_response()
> cause an unbounded reset loop if fetching or validating the report
> descriptor subsequently fails?
>
> When SPI_HID_CREATE_DEVICE runs spi_hid_create_device() -> hid_add_device()
> -> spi_hid_ll_parse() (or spi_hid_refresh_device()), it calls
> spi_hid_report_descriptor_request(). If that request times out or fails
> validation in spi_hid_dev_irq(), SPI_HID_ERROR is set and
> spi_hid_error_handler() increments shid->reset_attempts from 0 to 1 and
> resets the device.
>
> After the reset, if the device responds to DEVICE_DESCRIPTOR again,
> spi_hid_dev_desc_response() resets shid->reset_attempts back to 0 before
> REPORT_DESCRIPTOR is retried, so shid->reset_attempts oscillates between 0
> and 1 and never reaches SPI_HID_MAX_RESET_ATTEMPTS. Should
> shid->reset_attempts only be cleared once the report descriptor is fetched
> and SPI_HID_READY is set?
Will fix this in v7.
>
> [ ... ]
>
> > @@ -334,13 +766,205 @@ static int spi_hid_create_device(struct spi_hid
> > *shid)
>
> [ ... ]
>
> > +static int spi_hid_bus_validate_header(struct spi_hid *shid,
> > + struct spi_hid_input_header *header)
> > +{
> > + struct device *dev = &shid->spi->dev;
> > + /*
> > + * Body capacity of shid->input, matching spi_hid_alloc_buffers().
> > + * shid->bufsize only changes with the IRQ disabled, so it is stable
> > here.
> > + */
> > + u32 max_body = round_up(sizeof(shid->input->header) +
> > + sizeof(shid->input->body) + shid->bufsize, 4)
> > -
> > + sizeof(shid->input->header);
> > +
> > + if (header->version != SPI_HID_INPUT_HEADER_VERSION) {
> > + dev_err(dev, "Unknown input report version (v 0x%x)\n",
> > + header->version);
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * report_length is device-provided and is used as the size of the
> > body
> > + * transfer, so it must never exceed the input buffer.
> > + */
> > + if (header->report_length > max_body) {
> > + dev_err(dev, "Input report too big: %u > %u\n",
> > + header->report_length, max_body);
> > + return -EMSGSIZE;
> > + }
>
> [Severity: Medium]
> Can header->report_length be 0 (or smaller than sizeof(shid->input->body))
> here?
>
> In spi_hid_populate_input_header(), header->report_length is computed as
> (get_unaligned_le16(&buf[1]) & 0x3fff) * 4. If a malformed header or bus
> glitch during reset produces a length of 0, spi_hid_bus_validate_header()
> returns 0 because it only checks the upper bound.
>
> In spi_hid_dev_irq(), that bypasses the SPI_HID_RESET_PENDING check,
> performs a 0-byte body read via spi_hid_input_sync(), and then parses stale
> bytes from shid->input->body in spi_hid_process_input_report(), triggering
> spi_hid_irq_error(). Should spi_hid_bus_validate_header() reject
> header->report_length < sizeof(shid->input->body)?
Will fix this in v7.
>
> > +
> > + if (shid->desc.max_input_length != 0 &&
> > + header->report_length > shid->desc.max_input_length) {
> > + dev_err(dev, "Input report body size %u > max expected of
> > %u\n",
> > + header->report_length, shid->desc.max_input_length);
> > + return -EMSGSIZE;
> > + }
>
> [Severity: High]
> Won't this check in spi_hid_bus_validate_header() reject
> REPORT_DESCRIPTOR_RESPONSE (and post-reset DEVICE_DESCRIPTOR_RESPONSE)
> whenever the descriptor packet is larger than shid->desc.max_input_length?
>
> During initialization:
>
> spi_hid_dev_desc_response()
> spi_hid_parse_dev_desc(raw, &shid->desc)
> desc->max_input_length = le16_to_cpu(raw->max_input_len);
> set_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
> schedule_work(&shid->reset_work);
>
> When spi_hid_reset_work() then calls spi_hid_create_device() ->
> hid_add_device() -> spi_hid_ll_parse() ->
> spi_hid_report_descriptor_request(), shid->desc.max_input_length is already
> populated with wMaxInputLength.
>
> When the device replies with REPORT_DESCRIPTOR_RESPONSE, spi_hid_dev_irq()
> calls spi_hid_bus_validate_header() before reading the body or knowing the
> report type. Since wMaxInputLength is the maximum length of input reports
> while wReportDescLength (up to HID_MAX_DESCRIPTOR_SIZE = 4096) is typically
> much larger, header->report_length > shid->desc.max_input_length will fail
> with -EMSGSIZE and trigger spi_hid_irq_error() on every attempt until the
> device is aborted. (Note also the comment in spi_hid_find_max_report() that
> some devices set wMaxInputLength to an inaccurate length.)
Will fix this in v7.
>
> > +
> > + if (header->last_fragment_flag != 1) {
> > + dev_err(dev, "Multi-fragment reports not supported\n");
> > + return -EOPNOTSUPP;
> > + }
>
> [ ... ]
>
> > @@ -365,6 +989,8 @@ static int spi_hid_get_request(struct spi_hid *shid, u8
> > content_id,
> > dev_err(dev,
> > "Expected get request response not received! Error
> > %d\n",
> > error);
> > + set_bit(SPI_HID_ERROR, &shid->flags);
> > + schedule_work(&shid->reset_work);
> > return error;
> > }
>
> [ ... ]
>
> > @@ -397,9 +1023,81 @@ static int spi_hid_set_request(struct spi_hid *shid,
> > u8 *arg_buf, u16 arg_len,
> > return spi_hid_sync_request(shid, &report);
> > }
> >
> > -/* This is a placeholder. Will be implemented in the next patch. */
> > +/* Schedule a reset to recover from an error in the IRQ handler. */
> > +static irqreturn_t spi_hid_irq_error(struct spi_hid *shid)
> > +{
> > + set_bit(SPI_HID_ERROR, &shid->flags);
> > + schedule_work(&shid->reset_work);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
> > {
> > + struct spi_hid *shid = _shid;
> > + struct device *dev = &shid->spi->dev;
> > + struct spi_hid_input_header header;
> > + int error = 0;
> > +
> > + scoped_guard(mutex, &shid->io_lock) {
> > + if (shid->power_state == HIDSPI_OFF) {
> > + dev_warn(dev, "Device is off, ignoring interrupt\n");
> > + return IRQ_NONE;
> > + }
> > +
> > + error = spi_hid_input_sync(shid, shid->input->header,
> > + sizeof(shid->input->header), true);
> > + if (error) {
> > + dev_err(dev, "Failed to transfer header: %d\n",
> > error);
> > + return spi_hid_irq_error(shid);
> > + }
> > +
> > + if (shid->input_message.status < 0) {
> > + dev_warn(dev, "Error reading header: %d\n",
> > + shid->input_message.status);
> > + shid->bus_error_count++;
> > + shid->bus_last_error = shid->input_message.status;
> > + return spi_hid_irq_error(shid);
> > + }
> > +
> > + spi_hid_populate_input_header(shid->input->header, &header);
> > +
> > + error = spi_hid_bus_validate_header(shid, &header);
> > + if (error) {
> > + if (!test_bit(SPI_HID_RESET_PENDING, &shid->flags)) {
> > + dev_err(dev, "Failed to validate header:
> > %d\n", error);
> > + print_hex_dump(KERN_ERR, "spi_hid: header
> > buffer: ",
> > + DUMP_PREFIX_NONE, 16, 1,
> > shid->input->header,
> > + sizeof(shid->input->header),
> > false);
> > + shid->bus_error_count++;
> > + shid->bus_last_error = error;
> > + return spi_hid_irq_error(shid);
> > + }
> > + return IRQ_HANDLED;
> > + }
> > +
> > + error = spi_hid_input_sync(shid, shid->input->body,
> > header.report_length,
> > + false);
>
> [ ... ]
>
> > @@ -479,6 +1177,8 @@ static int spi_hid_ll_start(struct hid_device *hid)
>
> [ ... ]
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=5
