On Fri, Oct 9, 2026 at 3:41 PM <[email protected]> wrote: > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) > to consider: > - [Low] spi-hid: data race on bus_error_count in spi_hid_error_handler > -- > > commit a262e333d16eb58c8aa593f81c7497790c2b01b7 > Author: Angela Czubak <[email protected]> > > HID: spi-hid: add ACPI support for HID over SPI > > Detect SPI HID devices described in ACPI. > > > diff --git a/drivers/hid/spi-hid/spi-hid-core.c > > b/drivers/hid/spi-hid/spi-hid-core.c > > index 83678e6835818..f4e9b4e1294ed 100644 > > --- a/drivers/hid/spi-hid/spi-hid-core.c > > +++ b/drivers/hid/spi-hid/spi-hid-core.c > > [ ... ] > > > @@ -327,8 +303,21 @@ static void spi_hid_error_handler(struct spi_hid *shid) > > > > shid->power_state = HIDSPI_ON; > > > > - shid->ops->deassert_reset(shid->ops); > > + error = shid->ops->deassert_reset(shid->ops); > > + if (error) { > > + dev_err(dev, "failed to deassert reset: %d\n", error); > > + shid->bus_error_count++; > > + shid->bus_last_error = error; > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > [Severity: Low] > Does this introduce a data race with other updaters of bus_error_count? > > Userspace sending an output report via __spi_hid_send_output_report() checks > SPI_HID_READY locklessly. If SPI_HID_READY is read as 1 just before > spi_hid_error_handler() clears it, the output report path proceeds to acquire > io_lock and invoke spi_hid_output(), which updates bus_error_count on error > (under io_lock). > > Because spi_hid_error_handler() updates bus_error_count here without holding > io_lock, both threads can execute shid->bus_error_count++ concurrently, which > can lead to lost increments or torn reads via sysfs.
This is harmless in practice: bus_error_count and bus_last_error are only debug counters shown in sysfs, and losing an increment would need two failures at the same moment. > > > + /* > > + * Without a successful reset the device will not send a reset > > + * response, so no IRQ would trigger another recovery attempt. > > + * Retry from here; reset_attempts bounds the number of tries. > > + */ > > + set_bit(SPI_HID_ERROR, &shid->flags); > > + schedule_work(&shid->reset_work); > > + } > > > > + /* Balance disable_irq() above; a retry disables it again. */ > > enable_irq(shid->spi->irq); > > } > > -- > Sashiko AI review · > https://sashiko.dev/#/patchset/[email protected]?part=7
