| Message ID | 20230720071330.50382-1-dg573847474@gmail.com |
|---|---|
| State | Accepted |
| Commit | f1a63db65e456263fa10d8a52aed652f392f22ce |
| Headers | show |
| Series | [v2] mfd: qcom-pm8xxx: Fix potential deadlock on &chip->pm_irq_lock | expand |
On Thu, Jul 20, 2023 at 07:13:30AM +0000, Chengfeng Ye wrote: > As &chip->pm_irq_lock is acquired by pm8xxx_irq_handler() under irq > context, other process context code should disable irq before acquiring > the lock. > > Since .irq_set_type and .irq_get_irqchip_state callbacks are generally > executed from process context without irq disabled by default, the same > lock acquision should disable irq. > > Possible deadlock scenario > pm8xxx_irq_set_type() > -> pm8xxx_config_irq() > -> spin_lock(&chip->pm_irq_lock) > <irq interrupt> > -> pm8xxx_irq_handler() > -> pm8xxx_irq_master_handler() > -> pm8xxx_irq_block_handler() > -> pm8xxx_read_block_irq() > -> spin_lock(&chip->pm_irq_lock) (deadlock here) > > This flaw was found using an experimental static analysis tool we are > developing for irq-related deadlock. > > Fix the potential deadlock by spin_lock_irqsave(). > > Signed-off-by: Chengfeng Ye <dg573847474@gmail.com> Reviewed-by: Bjorn Andersson <quic_bjorande@quicinc.com> Regards, Bjorn
On Thu, 20 Jul 2023 07:13:30 +0000, Chengfeng Ye wrote: > As &chip->pm_irq_lock is acquired by pm8xxx_irq_handler() under irq > context, other process context code should disable irq before acquiring > the lock. > > Since .irq_set_type and .irq_get_irqchip_state callbacks are generally > executed from process context without irq disabled by default, the same > lock acquision should disable irq. > > [...] Applied, thanks! [1/1] mfd: qcom-pm8xxx: Fix potential deadlock on &chip->pm_irq_lock commit: 213f9a4eb2f712866d471d18ab1bf664c71183e6 -- Lee Jones [李琼斯]
diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c index 9a948df8c28d..07c531bd1236 100644 --- a/drivers/mfd/qcom-pm8xxx.c +++ b/drivers/mfd/qcom-pm8xxx.c @@ -103,8 +103,9 @@ static int pm8xxx_config_irq(struct pm_irq_chip *chip, unsigned int bp, unsigned int cp) { int rc; + unsigned long flags; - spin_lock(&chip->pm_irq_lock); + spin_lock_irqsave(&chip->pm_irq_lock, flags); rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, bp); if (rc) { pr_err("Failed Selecting Block %d rc=%d\n", bp, rc); @@ -116,7 +117,7 @@ pm8xxx_config_irq(struct pm_irq_chip *chip, unsigned int bp, unsigned int cp) if (rc) pr_err("Failed Configuring IRQ rc=%d\n", rc); bail: - spin_unlock(&chip->pm_irq_lock); + spin_unlock_irqrestore(&chip->pm_irq_lock, flags); return rc; } @@ -321,6 +322,7 @@ static int pm8xxx_irq_get_irqchip_state(struct irq_data *d, struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d); unsigned int pmirq = irqd_to_hwirq(d); unsigned int bits; + unsigned long flags; int irq_bit; u8 block; int rc; @@ -331,7 +333,7 @@ static int pm8xxx_irq_get_irqchip_state(struct irq_data *d, block = pmirq / 8; irq_bit = pmirq % 8; - spin_lock(&chip->pm_irq_lock); + spin_lock_irqsave(&chip->pm_irq_lock, flags); rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block); if (rc) { pr_err("Failed Selecting Block %d rc=%d\n", block, rc); @@ -346,7 +348,7 @@ static int pm8xxx_irq_get_irqchip_state(struct irq_data *d, *state = !!(bits & BIT(irq_bit)); bail: - spin_unlock(&chip->pm_irq_lock); + spin_unlock_irqrestore(&chip->pm_irq_lock, flags); return rc; }
As &chip->pm_irq_lock is acquired by pm8xxx_irq_handler() under irq context, other process context code should disable irq before acquiring the lock. Since .irq_set_type and .irq_get_irqchip_state callbacks are generally executed from process context without irq disabled by default, the same lock acquision should disable irq. Possible deadlock scenario pm8xxx_irq_set_type() -> pm8xxx_config_irq() -> spin_lock(&chip->pm_irq_lock) <irq interrupt> -> pm8xxx_irq_handler() -> pm8xxx_irq_master_handler() -> pm8xxx_irq_block_handler() -> pm8xxx_read_block_irq() -> spin_lock(&chip->pm_irq_lock) (deadlock here) This flaw was found using an experimental static analysis tool we are developing for irq-related deadlock. Fix the potential deadlock by spin_lock_irqsave(). Signed-off-by: Chengfeng Ye <dg573847474@gmail.com> --- Changes in v2: - Modified word usage in commit message as suggested by Bjorn Andersson --- drivers/mfd/qcom-pm8xxx.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)