+irqchip driver maintainers Quoting David Collins (2021-07-28 11:02:09) > The qpnpint_irq_set_type() callback function configures the type > (edge vs level) and polarity (high, low, or both) of a particular > PMIC interrupt within a given peripheral. To do this, it reads > the three consecutive IRQ configuration registers, modifies the > specified IRQ bit within the register values, and finally writes > the three modified register values back to the PMIC. While a > spinlock is used to provide mutual exclusion on the SPMI bus > during the register read and write calls, there is no locking > around the overall read, modify, write sequence. This opens up > the possibility of a race condition if two tasks set the type of > a PMIC IRQ within the same peripheral simultaneously. > > When the race condition is encountered, both tasks will read the > old value of the registers and IRQ bits set by one of the tasks > will be dropped upon the register write of the other task. This > then leads to PMIC IRQs being enabled with an incorrect type and > polarity configured. Such misconfiguration can lead to an IRQ > storm that overwhelms the system and causes it to crash. > > This race condition and IRQ storm have been observed when using > a pair of pm8941-pwrkey devices to handle PMK8350 pwrkey and > resin interrupts. The independent devices probe asynchronously > in parallel and can simultaneously request and configure PMIC > IRQs in the same PMIC peripheral. > > For a good case, the IRQ configuration calls end up serialized > due to timing deltas and the register read/write sequence looks > like this: > > 1. pwrkey probe: SPMI read(0x1311): 0x00, 0x00, 0x00 > 2. pwrkey probe: SPMI write(0x1311): 0x80, 0x80, 0x80 > 3. resin probe: SPMI read(0x1311): 0x80, 0x80, 0x80 > 4. resin probe: SPMI write(0x1311): 0xC0, 0xC0, 0xC0 > > The final register states after both devices have requested and > enabled their respective IRQs is thus: > > 0x1311: 0xC0 > 0x1312: 0xC0 > 0x1313: 0xC0 > 0x1314: 0x00 > 0x1315: 0xC0 > > For a bad case, the IRQ configuration calls end up occurring > simultaneously and the race condition is encountered. The > register read/write sequence then looks like this: > > 1. pwrkey probe: SPMI read(0x1311): 0x00, 0x00, 0x00 > 2. resin probe: SPMI read(0x1311): 0x00, 0x00, 0x00 > 3. pwrkey probe: SPMI write(0x1311): 0x80, 0x80, 0x80 > 4. resin probe: SPMI write(0x1311): 0x40, 0x40, 0x40 > > In this case, the final register states after both devices have > requested and enabled their respective IRQs is thus: > > 0x1311: 0x40 > 0x1312: 0x40 > 0x1313: 0x40 > 0x1314: 0x00 > 0x1315: 0xC0 > > This corresponds to the resin IRQ being configured for both > rising and falling edges, as expected. However, the pwrkey IRQ > is misconfigured as level type with both polarity high and low > set to disabled. The PMIC IRQ triggering hardware treats this > particular register configuration as if level low triggering is > enabled. > > The raw pwrkey IRQ signal is low when the power key is not being > pressed. Thus, the pwrkey IRQ begins firing continuously in an > IRQ storm. > > Fix the race condition by locking a spinlock for the duration of > the read, modify, write sequence in the qpnpint_irq_set_type() > function. > > Fixes: 67b563f1f258 ("spmi: pmic_arb: add support for interrupt handling") > Signed-off-by: David Collins <collinsd@xxxxxxxxxxxxxx> > --- > drivers/spmi/spmi-pmic-arb.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c > index bbbd311eda03..379ad6c1c14a 100644 > --- a/drivers/spmi/spmi-pmic-arb.c > +++ b/drivers/spmi/spmi-pmic-arb.c > @@ -127,6 +127,7 @@ struct apid_data { > * @intr: address of the SPMI interrupt control registers. > * @cnfg: address of the PMIC Arbiter configuration registers. > * @lock: lock to synchronize accesses. > + * @irq_lock: lock to ensure mutual exclusion for IRQ type setting > * @channel: execution environment channel to use for accesses. > * @irq: PMIC ARB interrupt. > * @ee: the current Execution Environment > @@ -146,6 +147,7 @@ struct spmi_pmic_arb { > void __iomem *core; > resource_size_t core_size; > raw_spinlock_t lock; > + raw_spinlock_t irq_lock; Maybe a better name is type_lock given that it's about the irq type setting rmw sequence. > u8 channel; > int irq; > u8 ee; > @@ -600,10 +602,13 @@ static void qpnpint_irq_unmask(struct irq_data *d) > > static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type) > { > + struct spmi_pmic_arb *pmic_arb = irq_data_get_irq_chip_data(d); > struct spmi_pmic_arb_qpnpint_type type; > irq_flow_handler_t flow_handler; > u8 irq = hwirq_to_irq(d->hwirq); > + unsigned long flags; > > + raw_spin_lock_irqsave(&pmic_arb->irq_lock, flags); > qpnpint_spmi_read(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type)); > > if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) { > @@ -616,8 +621,10 @@ static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type) > flow_handler = handle_edge_irq; > } else { > if ((flow_type & (IRQF_TRIGGER_HIGH)) && > - (flow_type & (IRQF_TRIGGER_LOW))) > + (flow_type & (IRQF_TRIGGER_LOW))) { > + raw_spin_unlock_irqrestore(&pmic_arb->irq_lock, flags); > return -EINVAL; > + } > > type.type &= ~BIT(irq); /* level trig */ > if (flow_type & IRQF_TRIGGER_HIGH) > @@ -629,6 +636,8 @@ static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type) > } > > qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type)); > + raw_spin_unlock_irqrestore(&pmic_arb->irq_lock, flags); Could we have a qpnpint_spmi_set_bit/clear_bit() API that takes the bit we want to touch as an argument and then does it all under the originial pmic_arb->lock? Then we don't need a different lock, we can avoid that drop the lock under the else if condition above, and the area for the lock will be contained within the set/clear function instead of here. > + > irq_set_handler_locked(d, flow_handler); > > return 0; > @@ -1285,6 +1294,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, ctrl); > raw_spin_lock_init(&pmic_arb->lock); > + raw_spin_lock_init(&pmic_arb->irq_lock); > > ctrl->cmd = pmic_arb_cmd; > ctrl->read_cmd = pmic_arb_read_cmd;