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 <[email protected]>
---
?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
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 <[email protected]>
> ---
> 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 <[email protected]>
> 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
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!
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 <[email protected]>
I can't apply it, what version is this against? Also, if someone could
supply a proper Fixes-tag, this would be much appreciated.
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 <[email protected]>
>
> 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
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?