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. > + /* > + * 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
