[PATCH] i2c: bcm-iproc: Fix bcm_iproc_i2c_isr deadlock issue

[PATCH] i2c: bcm-iproc: Fix bcm_iproc_i2c_isr deadlock issue

[YE Chengfeng]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] i2c: bcm-iproc: Fix bcm_iproc_i2c_isr deadlock issue

[Ray Jui]

[-- Attachment #1.1: Type: text/plain, Size: 2728 bytes --]

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>


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4194 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] i2c: bcm-iproc: Fix bcm_iproc_i2c_isr deadlock issue

[YE Chengfeng]

> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] i2c: bcm-iproc: Fix bcm_iproc_i2c_isr deadlock issue

[Ray Jui]

[-- Attachment #1.1: Type: text/plain, Size: 711 bytes --]



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!

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4194 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] i2c: bcm-iproc: Fix bcm_iproc_i2c_isr deadlock issue

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 543 bytes --]

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.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] i2c: bcm-iproc: Fix bcm_iproc_i2c_isr deadlock issue

[Ray Jui]

[-- Attachment #1.1: Type: text/plain, Size: 738 bytes --]

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

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4194 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] i2c: bcm-iproc: Fix bcm_iproc_i2c_isr deadlock issue

[YE Chengfeng]

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 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel