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.
I think .irq_set_type and .irq_get_irqchip_state callbacks should be
executed from process context without irq disabled by default. Thus 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.
The tentative patch fix the potential deadlock by spin_lock_irqsave().
Signed-off-by: Chengfeng Ye <[email protected]>
---
drivers/mfd/qcom-pm8xxx.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
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;
}
--
2.17.1
I'd like some additional eyes-on please.
This is very old code that you're changing and some of the people who
made the largest contributions are not on Cc.
On Wed, 28 Jun 2023, 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.
>
> I think .irq_set_type and .irq_get_irqchip_state callbacks should be
> executed from process context without irq disabled by default. Thus 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.
>
> The tentative patch fix the potential deadlock by spin_lock_irqsave().
>
> Signed-off-by: Chengfeng Ye <[email protected]>
> ---
> drivers/mfd/qcom-pm8xxx.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> 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;
> }
> --
> 2.17.1
>
--
Lee Jones [李琼斯]
> This is very old code that you're changing and some of the people who
> made the largest contributions are not on Cc.
Thanks for letting know. Will be patient while the patch is being reviewed.
Best,
Chengfeng
On Tue, 18 Jul 2023, Chengfeng Ye wrote:
> > This is very old code that you're changing and some of the people who
> > made the largest contributions are not on Cc.
>
> Thanks for letting know. Will be patient while the patch is being reviewed.
No, I need you to resubmit it with more historical people on Cc please.
`git log --full-diff` should help you trace back through name changes.
--
Lee Jones [李琼斯]
On Wed, Jun 28, 2023 at 07:28:40AM +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.
>
> I think .irq_set_type and .irq_get_irqchip_state callbacks should be
You are correct, so please drop "I think", and change "should be" to
"are generally".
> executed from process context without irq disabled by default. Thus 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.
>
> The tentative patch fix the potential deadlock by spin_lock_irqsave().
I don't think this is a "tentative patch fix", it is the patch to fix
the issue. I think you can omit this line, because you already described
your problem, and the solution above.
>
> Signed-off-by: Chengfeng Ye <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> You are correct, so please drop "I think", and change "should be" to
> "are generally"
> I don't think this is a "tentative patch fix", it is the patch to fix
> the issue. I think you can omit this line, because you already described
> your problem, and the solution above.
The V2 patch was just sent to address the problem.
> Reviewed-by: Bjorn Andersson <[email protected]>
Also thanks for your time in reviewing the patch :)
> No, I need you to resubmit it with more historical people on Cc please.
Thanks for the reminder, I Cc more people with large contributions to the
file in the V2 patch.
Best Regards,
Chengfeng