@samitouri / QOSamiQemu / commits / 7e82393ed0

hw/i2c/aspeed: fix lost interrupts on back-to-back commands

QEMU executes I2C commands synchronously inside the CMD register write handler. On real hardware each command takes time on the bus, so the ISR can clear the previous interrupt status before the next completion arrives. In QEMU, when the guest ISR handles a TX_ACK and immediately issues the next command by writing to CMD, that command completes instantly — before the ISR returns to W1C-clear the first TX_ACK. Since the bit is already set, setting it again is a no-op. The ISR then clears it, wiping both completions at once. No interrupt fires for the second command and the driver stalls. This affects any multi-step I2C transaction: register reads, SMBus word reads, and PMBus device probes all fail ("Error: Read failed" from i2cget, -ETIMEDOUT from kernel drivers). The issue is exposed when the guest kernel includes commit "i2c: aspeed: Acknowledge Tx done with and without ACK irq late" [1] which defers W1C acknowledgment of TX_ACK until after the ISR has issued the next command. This means the old TX_ACK is still set when the next command completes synchronously, and the subsequent W1C wipes both completions at once. The trace below shows `i2cget -y 15 0x50 0x00` (read EEPROM register 0x00) failing without the fix. The first START+TX sets TX_ACK. The ISR handles it and issues a second TX to send the register address. That TX completes synchronously while TX_ACK is still set: aspeed_i2c_bus_cmd cmd=0x3 start|tx| intr=0x0 # START+TX, clean aspeed_i2c_bus_raise_interrupt intr=0x1 ack| # TX_ACK set aspeed_i2c_bus_read 0x10: 0x1 # ISR reads TX_ACK aspeed_i2c_bus_write 0x14: 0x2 # ISR issues TX cmd aspeed_i2c_bus_cmd cmd=0x400002 tx| intr=0x1 # TX runs, TX_ACK already set! aspeed_i2c_bus_raise_interrupt intr=0x1 ack| # re-set is no-op aspeed_i2c_bus_write 0x10: 0x1 # ISR W1C clears TX_ACK aspeed_i2c_bus_read 0x10: 0x0 # LOST — both ACKs wiped The driver sees INTR_STS=0 and never proceeds to the read phase. Fix this by tracking interrupt bits that collide with already-pending bits. Before calling aspeed_i2c_bus_handle_cmd(), save and clear INTR_STS so that only freshly set bits are visible after the call. Any overlap between the old and new bits is saved in pending_intr_sts. When the ISR later W1C-clears the old bits, re-apply the saved pending bits so the ISR sees them on its next loop iteration. With the fix, the same operation completes successfully: aspeed_i2c_bus_cmd cmd=0x3 start|tx| intr=0x0 # START+TX, clean aspeed_i2c_bus_raise_interrupt intr=0x1 ack| # TX_ACK set aspeed_i2c_bus_read 0x10: 0x1 # ISR reads TX_ACK aspeed_i2c_bus_write 0x14: 0x2 # ISR issues TX cmd aspeed_i2c_bus_cmd cmd=0x400002 tx| intr=0x0 # INTR_STS cleared first aspeed_i2c_bus_raise_interrupt intr=0x1 ack| # TX_ACK freshly set aspeed_i2c_bus_write 0x10: 0x1 # ISR W1C clears TX_ACK aspeed_i2c_bus_read 0x10: 0x1 # RE-DELIVERED from pending aspeed_i2c_bus_write 0x14: 0x1b # ISR proceeds: START+RX aspeed_i2c_bus_cmd cmd=0x40001b start|tx|rx|last| # read phase completes i2c_recv recv(addr:0x50) data:0x00 # data received [1] https://lore.kernel.org/all/20231211102217.2436294-3-quan@os.amperecomputing.com/ Signed-off-by: Jithu Joseph <jithu.joseph@oss.qualcomm.com> Fixes: 1602001195dc ("i2c: add aspeed i2c controller") Reviewed-by: Cédric Le Goater <clg@redhat.com> Link: https://lore.kernel.org/qemu-devel/20260311023712.2730185-1-jithu.joseph@oss.qualcomm.com Signed-off-by: Cédric Le Goater <clg@redhat.com>

Jithu Joseph committed Mar 10, 2026 at 19:37 UTC 7e82393ed058fd1415de399a9afff3793daa94c3

2 files changed +47

hw/i2c/aspeed_i2c.c

+46

@@ -628,6 +628,8 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,

628 AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);

629 bool handle_rx;

630 bool w1t;

631 + uint32_t old_intr;

632 + uint32_t cmd_intr;

633

634 trace_aspeed_i2c_bus_write(bus->id, offset, size, value);

635

@@ -665,6 +667,17 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,

667 break;

668 }

669 bus->regs[R_I2CM_INTR_STS] &= ~(value & 0xf007f07f);

670 + /*

671 + * Re-apply interrupts lost due to synchronous command completion.

672 + * When a command completes instantly during an MMIO write, the new

673 + * interrupt status bits collide with already-pending bits. After

674 + * the ISR clears them, re-apply the saved bits so the ISR can

675 + * process the new completion.

676 + */

677 + if (bus->pending_intr_sts) {

678 + bus->regs[R_I2CM_INTR_STS] |= bus->pending_intr_sts;

679 + bus->pending_intr_sts = 0;

680 + }

681 if (!bus->regs[R_I2CM_INTR_STS]) {

682 bus->controller->intr_status &= ~(1 << bus->id);

683 qemu_irq_lower(aic->bus_get_irq(bus));

@@ -708,7 +721,17 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,

721 bus->regs[R_I2CM_CMD] = value;

722 }

723

724 + old_intr = bus->regs[R_I2CM_INTR_STS];

725 + bus->regs[R_I2CM_INTR_STS] = 0;

726 aspeed_i2c_bus_handle_cmd(bus, value);

727 + /*

728 + * cmd_intr has only the bits handle_cmd freshly set.

729 + * Overlap with old_intr means the same bit was re-fired

730 + * and would be lost when the ISR W1C-clears the old one.

731 + */

732 + cmd_intr = bus->regs[R_I2CM_INTR_STS];

733 + bus->regs[R_I2CM_INTR_STS] = cmd_intr | old_intr;

734 + bus->pending_intr_sts |= old_intr & cmd_intr;

735 aspeed_i2c_bus_raise_interrupt(bus);

736 break;

737 case A_I2CM_DMA_TX_ADDR:

@@ -845,6 +868,8 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,

868 {

869 AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);

870 bool handle_rx;

871 + uint32_t old_intr;

872 + uint32_t cmd_intr;

873

874 trace_aspeed_i2c_bus_write(bus->id, offset, size, value);

875

@@ -868,6 +893,17 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,

893 handle_rx = SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_INTR_STS, RX_DONE)

894 && SHARED_FIELD_EX32(value, RX_DONE);

895 bus->regs[R_I2CD_INTR_STS] &= ~(value & 0x7FFF);

896 + /*

897 + * Re-apply interrupts lost due to synchronous command completion.

898 + * When a command completes instantly during an MMIO write, the new

899 + * interrupt status bits collide with already-pending bits. After

900 + * the ISR clears them, re-apply the saved bits so the ISR can

901 + * process the new completion.

902 + */

903 + if (bus->pending_intr_sts) {

904 + bus->regs[R_I2CD_INTR_STS] |= bus->pending_intr_sts;

905 + bus->pending_intr_sts = 0;

906 + }

907 if (!bus->regs[R_I2CD_INTR_STS]) {

908 bus->controller->intr_status &= ~(1 << bus->id);

909 qemu_irq_lower(aic->bus_get_irq(bus));

@@ -915,7 +951,17 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,

951 bus->regs[R_I2CD_CMD] &= ~0xFFFF;

952 bus->regs[R_I2CD_CMD] |= value & 0xFFFF;

953

954 + old_intr = bus->regs[R_I2CD_INTR_STS];

955 + bus->regs[R_I2CD_INTR_STS] = 0;

956 aspeed_i2c_bus_handle_cmd(bus, value);

957 + /*

958 + * cmd_intr has only the bits handle_cmd freshly set.

959 + * Overlap with old_intr means the same bit was re-fired

960 + * and would be lost when the ISR W1C-clears the old one.

961 + */

962 + cmd_intr = bus->regs[R_I2CD_INTR_STS];

963 + bus->regs[R_I2CD_INTR_STS] = cmd_intr | old_intr;

964 + bus->pending_intr_sts |= old_intr & cmd_intr;

965 aspeed_i2c_bus_raise_interrupt(bus);

966 break;

967 case A_I2CD_DMA_ADDR:

include/hw/i2c/aspeed_i2c.h

@@ -256,6 +256,7 @@ struct AspeedI2CBus {

256 qemu_irq irq;

257

258 uint32_t regs[ASPEED_I2C_NEW_NUM_REG];

259 + uint32_t pending_intr_sts;

260 uint8_t pool[ASPEED_I2C_BUS_POOL_SIZE];

261 uint64_t dma_dram_offset;

262 };