diff mbox series

i2c: bcm-iproc: Fix bcm_iproc_i2c_isr deadlock issue

Message ID TYCP286MB1188708B1CE64D95FBACF9E78A20A@TYCP286MB1188.JPNP286.PROD.OUTLOOK.COM
State New
Headers show
Series i2c: bcm-iproc: Fix bcm_iproc_i2c_isr deadlock issue | expand

Commit Message

YE Chengfeng June 24, 2023, 7:36 p.m. UTC 
iproc_i2c_rd_reg and iproc_i2c_wr_reg are called from both
interrupt context (e.g. bcm_iproc_i2c_isr) and process context
(e.g. bcm_iproc_i2c_suspend). Therefore, interrupts should be
disabled to avoid potential deadlock. To prevent this deadlock,
use spin_lock_irqsave.

Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
2.17.1

Comments

Ray Jui June 26, 2023, 4:42 p.m. UTC | #1 
Hi Chengfeng,

On 6/24/2023 12:36 PM, YE Chengfeng wrote:
> iproc_i2c_rd_reg and iproc_i2c_wr_reg are called from both
> interrupt context (e.g. bcm_iproc_i2c_isr) and process context
> (e.g. bcm_iproc_i2c_suspend). Therefore, interrupts should be
> disabled to avoid potential deadlock. To prevent this deadlock,
> use spin_lock_irqsave.
> 
> Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk>
> ---
>  drivers/i2c/busses/i2c-bcm-iproc.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index 85d8a6b04885..d02245e4db8c 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -233,13 +233,14 @@ static inline u32 iproc_i2c_rd_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
>                                    u32 offset)
>  {
>         u32 val;
> +       unsigned long flags;
> 
>         if (iproc_i2c->idm_base) {
> -               spin_lock(&iproc_i2c->idm_lock);
> +               spin_lock_irqsave(&iproc_i2c->idm_lock, flags);
>                 writel(iproc_i2c->ape_addr_mask,
>                        iproc_i2c->idm_base + IDM_CTRL_DIRECT_OFFSET);
>                 val = readl(iproc_i2c->base + offset);
> -               spin_unlock(&iproc_i2c->idm_lock);
> +               spin_unlock_irqrestore(&iproc_i2c->idm_lock, flags);
>         } else {
>                 val = readl(iproc_i2c->base + offset);
>         }
> @@ -250,12 +251,13 @@ static inline u32 iproc_i2c_rd_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
>  static inline void iproc_i2c_wr_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
>                                     u32 offset, u32 val)
>  {
> +       unsigned long flags;
>         if (iproc_i2c->idm_base) {
> -               spin_lock(&iproc_i2c->idm_lock);
> +               spin_lock_irqsave(&iproc_i2c->idm_lock, flags);
>                 writel(iproc_i2c->ape_addr_mask,
>                        iproc_i2c->idm_base + IDM_CTRL_DIRECT_OFFSET);
>                 writel(val, iproc_i2c->base + offset);
> -               spin_unlock(&iproc_i2c->idm_lock);
> +               spin_unlock_irqrestore(&iproc_i2c->idm_lock, flags);
>         } else {
>                 writel(val, iproc_i2c->base + offset);
>         }
> -- 
> 2.17.1

This fix looks good to me. Thanks. Just curious, did you actually see a
race condition issue as a result of this, or the fix is done completely
based on the analysis of the code?

Acked-by: Ray Jui <ray.jui@broadcom.com>
YE Chengfeng June 26, 2023, 5:05 p.m. UTC | #2 
> This fix looks good to me. Thanks. Just curious, did you actually see a
> race condition issue as a result of this, or the fix is done completely
> based on the analysis of the code?

Thanks for the reply!

This bug is detected by a static analysis tool built by me, I just notice I should 
mention this in the commit message and sorry for not have made it clear. I noticed 
lockdep occasionally reported such similar deadlocks in other place thus built the 
static tool to locate such bugs.

Just feel free to let me know if anything in the patch should be improved.

Best Regards,
Chengfeng
Ray Jui June 26, 2023, 5:18 p.m. UTC | #3 
On 6/26/2023 10:05 AM, YE Chengfeng wrote:
>> This fix looks good to me. Thanks. Just curious, did you actually see a
>> race condition issue as a result of this, or the fix is done completely
>> based on the analysis of the code?
> 
> Thanks for the reply!
> 
> This bug is detected by a static analysis tool built by me, I just notice I should 
> mention this in the commit message and sorry for not have made it clear. I noticed 
> lockdep occasionally reported such similar deadlocks in other place thus built the 
> static tool to locate such bugs.
> 
> Just feel free to let me know if anything in the patch should be improved.
> 
> Best Regards,
> Chengfeng

This sounds good. Thanks again, Chengfeng!
Wolfram Sang July 6, 2023, 7:36 p.m. UTC | #4 
On Sat, Jun 24, 2023 at 07:36:02PM +0000, YE Chengfeng wrote:
> iproc_i2c_rd_reg and iproc_i2c_wr_reg are called from both
> interrupt context (e.g. bcm_iproc_i2c_isr) and process context
> (e.g. bcm_iproc_i2c_suspend). Therefore, interrupts should be
> disabled to avoid potential deadlock. To prevent this deadlock,
> use spin_lock_irqsave.
> 
> Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk>

I can't apply it, what version is this against? Also, if someone could
supply a proper Fixes-tag, this would be much appreciated.
Ray Jui July 6, 2023, 8:08 p.m. UTC | #5 
Hi Wolfram,

On 7/6/2023 12:36 PM, Wolfram Sang wrote:
> On Sat, Jun 24, 2023 at 07:36:02PM +0000, YE Chengfeng wrote:
>> iproc_i2c_rd_reg and iproc_i2c_wr_reg are called from both
>> interrupt context (e.g. bcm_iproc_i2c_isr) and process context
>> (e.g. bcm_iproc_i2c_suspend). Therefore, interrupts should be
>> disabled to avoid potential deadlock. To prevent this deadlock,
>> use spin_lock_irqsave.
>>
>> Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk>
> 
> I can't apply it, what version is this against? Also, if someone could
> supply a proper Fixes-tag, this would be much appreciated.
> 

I'll let Chengfeng check the version.

For the fixes tag, it is:
Fixes: 9a1038728037 ("i2c: iproc: add NIC I2C support")

Thanks,

Ray
YE Chengfeng July 6, 2023, 9:28 p.m. UTC | #6 
Thanks for the notice.

It is on 6.4-rc7.  
I just resend the patch, I think that one should be able to apply. 
 
Best Regards, 
Chengfeng
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 85d8a6b04885..d02245e4db8c 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -233,13 +233,14 @@  static inline u32 iproc_i2c_rd_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
                                   u32 offset)
 {
        u32 val;
+       unsigned long flags;

        if (iproc_i2c->idm_base) {
-               spin_lock(&iproc_i2c->idm_lock);
+               spin_lock_irqsave(&iproc_i2c->idm_lock, flags);
                writel(iproc_i2c->ape_addr_mask,
                       iproc_i2c->idm_base + IDM_CTRL_DIRECT_OFFSET);
                val = readl(iproc_i2c->base + offset);
-               spin_unlock(&iproc_i2c->idm_lock);
+               spin_unlock_irqrestore(&iproc_i2c->idm_lock, flags);
        } else {
                val = readl(iproc_i2c->base + offset);
        }
@@ -250,12 +251,13 @@  static inline u32 iproc_i2c_rd_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
 static inline void iproc_i2c_wr_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
                                    u32 offset, u32 val)
 {
+       unsigned long flags;
        if (iproc_i2c->idm_base) {
-               spin_lock(&iproc_i2c->idm_lock);
+               spin_lock_irqsave(&iproc_i2c->idm_lock, flags);
                writel(iproc_i2c->ape_addr_mask,
                       iproc_i2c->idm_base + IDM_CTRL_DIRECT_OFFSET);
                writel(val, iproc_i2c->base + offset);
-               spin_unlock(&iproc_i2c->idm_lock);
+               spin_unlock_irqrestore(&iproc_i2c->idm_lock, flags);
        } else {
                writel(val, iproc_i2c->base + offset);
        }