Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] i2c: iproc: fix race between client unreg and isr
@ 2020-07-18 23:39 Dhananjay Phadke
  2020-07-19  7:59 ` Rayagonda Kokatanur
  2020-07-20 17:49 ` Ray Jui
  0 siblings, 2 replies; 18+ messages in thread
From: Dhananjay Phadke @ 2020-07-18 23:39 UTC (permalink / raw)
  To: Rayagonda Kokatanur, linux-i2c, linux-kernel, Wolfram Sang
  Cc: Ray Jui, bcm-kernel-feedback-list, Dhananjay Phadke

When i2c client unregisters, synchronize irq before setting
iproc_i2c->slave to NULL.

Unable to handle kernel NULL pointer dereference at virtual address 0000000000000318

[  371.020421] pc : bcm_iproc_i2c_isr+0x530/0x11f0
[  371.025098] lr : __handle_irq_event_percpu+0x6c/0x170
[  371.030309] sp : ffff800010003e40
[  371.033727] x29: ffff800010003e40 x28: 0000000000000060
[  371.039206] x27: ffff800010ca9de0 x26: ffff800010f895df
[  371.044686] x25: ffff800010f18888 x24: ffff0008f7ff3600
[  371.050165] x23: 0000000000000003 x22: 0000000001600000
[  371.055645] x21: ffff800010f18888 x20: 0000000001600000
[  371.061124] x19: ffff0008f726f080 x18: 0000000000000000
[  371.066603] x17: 0000000000000000 x16: 0000000000000000
[  371.072082] x15: 0000000000000000 x14: 0000000000000000
[  371.077561] x13: 0000000000000000 x12: 0000000000000001
[  371.083040] x11: 0000000000000000 x10: 0000000000000040
[  371.088519] x9 : ffff800010f317c8 x8 : ffff800010f317c0
[  371.093999] x7 : ffff0008f805b3b0 x6 : 0000000000000000
[  371.099478] x5 : ffff0008f7ff36a4 x4 : ffff8008ee43d000
[  371.104957] x3 : 0000000000000000 x2 : ffff8000107d64c0
[  371.110436] x1 : 00000000c00000af x0 : 0000000000000000

[  371.115916] Call trace:
[  371.118439]  bcm_iproc_i2c_isr+0x530/0x11f0
[  371.122754]  __handle_irq_event_percpu+0x6c/0x170
[  371.127606]  handle_irq_event_percpu+0x34/0x88
[  371.132189]  handle_irq_event+0x40/0x120
[  371.136234]  handle_fasteoi_irq+0xcc/0x1a0
[  371.140459]  generic_handle_irq+0x24/0x38
[  371.144594]  __handle_domain_irq+0x60/0xb8
[  371.148820]  gic_handle_irq+0xc0/0x158
[  371.152687]  el1_irq+0xb8/0x140
[  371.155927]  arch_cpu_idle+0x10/0x18
[  371.159615]  do_idle+0x204/0x290
[  371.162943]  cpu_startup_entry+0x24/0x60
[  371.166990]  rest_init+0xb0/0xbc
[  371.170322]  arch_call_rest_init+0xc/0x14
[  371.174458]  start_kernel+0x404/0x430

Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
Signed-off-by: Dhananjay Phadke <dphadke@linux.microsoft.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index b58224b7b..37d2a79e7 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -1074,14 +1074,15 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
 	if (!iproc_i2c->slave)
 		return -EINVAL;
 
-	iproc_i2c->slave = NULL;
-
 	/* disable all slave interrupts */
 	tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
 	tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
 			IE_S_ALL_INTERRUPT_SHIFT);
 	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
 
+	synchronize_irq(iproc_i2c->irq);
+	iproc_i2c->slave = NULL;
+
 	/* Erase the slave address programmed */
 	tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
 	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-18 23:39 [PATCH] i2c: iproc: fix race between client unreg and isr Dhananjay Phadke
@ 2020-07-19  7:59 ` Rayagonda Kokatanur
  2020-07-20 17:40   ` Scott Branden
  2020-07-20 17:49 ` Ray Jui
  1 sibling, 1 reply; 18+ messages in thread
From: Rayagonda Kokatanur @ 2020-07-19  7:59 UTC (permalink / raw)
  To: Dhananjay Phadke
  Cc: linux-i2c, Linux Kernel Mailing List, Wolfram Sang, Ray Jui,
	BCM Kernel Feedback

On Sun, Jul 19, 2020 at 5:10 AM Dhananjay Phadke
<dphadke@linux.microsoft.com> wrote:
>
> When i2c client unregisters, synchronize irq before setting
> iproc_i2c->slave to NULL.
>
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000318
>
> [  371.020421] pc : bcm_iproc_i2c_isr+0x530/0x11f0
> [  371.025098] lr : __handle_irq_event_percpu+0x6c/0x170
> [  371.030309] sp : ffff800010003e40
> [  371.033727] x29: ffff800010003e40 x28: 0000000000000060
> [  371.039206] x27: ffff800010ca9de0 x26: ffff800010f895df
> [  371.044686] x25: ffff800010f18888 x24: ffff0008f7ff3600
> [  371.050165] x23: 0000000000000003 x22: 0000000001600000
> [  371.055645] x21: ffff800010f18888 x20: 0000000001600000
> [  371.061124] x19: ffff0008f726f080 x18: 0000000000000000
> [  371.066603] x17: 0000000000000000 x16: 0000000000000000
> [  371.072082] x15: 0000000000000000 x14: 0000000000000000
> [  371.077561] x13: 0000000000000000 x12: 0000000000000001
> [  371.083040] x11: 0000000000000000 x10: 0000000000000040
> [  371.088519] x9 : ffff800010f317c8 x8 : ffff800010f317c0
> [  371.093999] x7 : ffff0008f805b3b0 x6 : 0000000000000000
> [  371.099478] x5 : ffff0008f7ff36a4 x4 : ffff8008ee43d000
> [  371.104957] x3 : 0000000000000000 x2 : ffff8000107d64c0
> [  371.110436] x1 : 00000000c00000af x0 : 0000000000000000
>
> [  371.115916] Call trace:
> [  371.118439]  bcm_iproc_i2c_isr+0x530/0x11f0
> [  371.122754]  __handle_irq_event_percpu+0x6c/0x170
> [  371.127606]  handle_irq_event_percpu+0x34/0x88
> [  371.132189]  handle_irq_event+0x40/0x120
> [  371.136234]  handle_fasteoi_irq+0xcc/0x1a0
> [  371.140459]  generic_handle_irq+0x24/0x38
> [  371.144594]  __handle_domain_irq+0x60/0xb8
> [  371.148820]  gic_handle_irq+0xc0/0x158
> [  371.152687]  el1_irq+0xb8/0x140
> [  371.155927]  arch_cpu_idle+0x10/0x18
> [  371.159615]  do_idle+0x204/0x290
> [  371.162943]  cpu_startup_entry+0x24/0x60
> [  371.166990]  rest_init+0xb0/0xbc
> [  371.170322]  arch_call_rest_init+0xc/0x14
> [  371.174458]  start_kernel+0x404/0x430
>
> Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
> Signed-off-by: Dhananjay Phadke <dphadke@linux.microsoft.com>
> ---
>  drivers/i2c/busses/i2c-bcm-iproc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index b58224b7b..37d2a79e7 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -1074,14 +1074,15 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
>         if (!iproc_i2c->slave)
>                 return -EINVAL;
>
> -       iproc_i2c->slave = NULL;
> -
>         /* disable all slave interrupts */
>         tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
>         tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
>                         IE_S_ALL_INTERRUPT_SHIFT);
>         iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
>
> +       synchronize_irq(iproc_i2c->irq);
> +       iproc_i2c->slave = NULL;
> +
>         /* Erase the slave address programmed */
>         tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
>         tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);

Looks good to me. Thank you for the patch.
Reviewed-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>

Regards,
Rayagonda

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-19  7:59 ` Rayagonda Kokatanur
@ 2020-07-20 17:40   ` Scott Branden
  0 siblings, 0 replies; 18+ messages in thread
From: Scott Branden @ 2020-07-20 17:40 UTC (permalink / raw)
  To: Rayagonda Kokatanur, Dhananjay Phadke
  Cc: linux-i2c, Linux Kernel Mailing List, Wolfram Sang, Ray Jui,
	BCM Kernel Feedback

looks good.

On 2020-07-19 12:59 a.m., Rayagonda Kokatanur wrote:
> On Sun, Jul 19, 2020 at 5:10 AM Dhananjay Phadke
> <dphadke@linux.microsoft.com> wrote:
>> When i2c client unregisters, synchronize irq before setting
>> iproc_i2c->slave to NULL.
>>
>> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000318
>>
>> [  371.020421] pc : bcm_iproc_i2c_isr+0x530/0x11f0
>> [  371.025098] lr : __handle_irq_event_percpu+0x6c/0x170
>> [  371.030309] sp : ffff800010003e40
>> [  371.033727] x29: ffff800010003e40 x28: 0000000000000060
>> [  371.039206] x27: ffff800010ca9de0 x26: ffff800010f895df
>> [  371.044686] x25: ffff800010f18888 x24: ffff0008f7ff3600
>> [  371.050165] x23: 0000000000000003 x22: 0000000001600000
>> [  371.055645] x21: ffff800010f18888 x20: 0000000001600000
>> [  371.061124] x19: ffff0008f726f080 x18: 0000000000000000
>> [  371.066603] x17: 0000000000000000 x16: 0000000000000000
>> [  371.072082] x15: 0000000000000000 x14: 0000000000000000
>> [  371.077561] x13: 0000000000000000 x12: 0000000000000001
>> [  371.083040] x11: 0000000000000000 x10: 0000000000000040
>> [  371.088519] x9 : ffff800010f317c8 x8 : ffff800010f317c0
>> [  371.093999] x7 : ffff0008f805b3b0 x6 : 0000000000000000
>> [  371.099478] x5 : ffff0008f7ff36a4 x4 : ffff8008ee43d000
>> [  371.104957] x3 : 0000000000000000 x2 : ffff8000107d64c0
>> [  371.110436] x1 : 00000000c00000af x0 : 0000000000000000
>>
>> [  371.115916] Call trace:
>> [  371.118439]  bcm_iproc_i2c_isr+0x530/0x11f0
>> [  371.122754]  __handle_irq_event_percpu+0x6c/0x170
>> [  371.127606]  handle_irq_event_percpu+0x34/0x88
>> [  371.132189]  handle_irq_event+0x40/0x120
>> [  371.136234]  handle_fasteoi_irq+0xcc/0x1a0
>> [  371.140459]  generic_handle_irq+0x24/0x38
>> [  371.144594]  __handle_domain_irq+0x60/0xb8
>> [  371.148820]  gic_handle_irq+0xc0/0x158
>> [  371.152687]  el1_irq+0xb8/0x140
>> [  371.155927]  arch_cpu_idle+0x10/0x18
>> [  371.159615]  do_idle+0x204/0x290
>> [  371.162943]  cpu_startup_entry+0x24/0x60
>> [  371.166990]  rest_init+0xb0/0xbc
>> [  371.170322]  arch_call_rest_init+0xc/0x14
>> [  371.174458]  start_kernel+0x404/0x430
>>
>> Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
>> Signed-off-by: Dhananjay Phadke <dphadke@linux.microsoft.com>
Acked-by: Scott Branden <scott.branden@broadcom.com>
>> ---
>>   drivers/i2c/busses/i2c-bcm-iproc.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
>> index b58224b7b..37d2a79e7 100644
>> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
>> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
>> @@ -1074,14 +1074,15 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
>>          if (!iproc_i2c->slave)
>>                  return -EINVAL;
>>
>> -       iproc_i2c->slave = NULL;
>> -
>>          /* disable all slave interrupts */
>>          tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
>>          tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
>>                          IE_S_ALL_INTERRUPT_SHIFT);
>>          iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
>>
>> +       synchronize_irq(iproc_i2c->irq);
>> +       iproc_i2c->slave = NULL;
>> +
>>          /* Erase the slave address programmed */
>>          tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
>>          tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
> Looks good to me. Thank you for the patch.
> Reviewed-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
>
> Regards,
> Rayagonda


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-18 23:39 [PATCH] i2c: iproc: fix race between client unreg and isr Dhananjay Phadke
  2020-07-19  7:59 ` Rayagonda Kokatanur
@ 2020-07-20 17:49 ` Ray Jui
  2020-07-22 10:41   ` Wolfram Sang
  1 sibling, 1 reply; 18+ messages in thread
From: Ray Jui @ 2020-07-20 17:49 UTC (permalink / raw)
  To: Dhananjay Phadke, Rayagonda Kokatanur, linux-i2c, linux-kernel,
	Wolfram Sang
  Cc: Ray Jui, bcm-kernel-feedback-list



On 7/18/2020 4:39 PM, Dhananjay Phadke wrote:
> When i2c client unregisters, synchronize irq before setting
> iproc_i2c->slave to NULL.
> 
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000318
> 
> [  371.020421] pc : bcm_iproc_i2c_isr+0x530/0x11f0
> [  371.025098] lr : __handle_irq_event_percpu+0x6c/0x170
> [  371.030309] sp : ffff800010003e40
> [  371.033727] x29: ffff800010003e40 x28: 0000000000000060
> [  371.039206] x27: ffff800010ca9de0 x26: ffff800010f895df
> [  371.044686] x25: ffff800010f18888 x24: ffff0008f7ff3600
> [  371.050165] x23: 0000000000000003 x22: 0000000001600000
> [  371.055645] x21: ffff800010f18888 x20: 0000000001600000
> [  371.061124] x19: ffff0008f726f080 x18: 0000000000000000
> [  371.066603] x17: 0000000000000000 x16: 0000000000000000
> [  371.072082] x15: 0000000000000000 x14: 0000000000000000
> [  371.077561] x13: 0000000000000000 x12: 0000000000000001
> [  371.083040] x11: 0000000000000000 x10: 0000000000000040
> [  371.088519] x9 : ffff800010f317c8 x8 : ffff800010f317c0
> [  371.093999] x7 : ffff0008f805b3b0 x6 : 0000000000000000
> [  371.099478] x5 : ffff0008f7ff36a4 x4 : ffff8008ee43d000
> [  371.104957] x3 : 0000000000000000 x2 : ffff8000107d64c0
> [  371.110436] x1 : 00000000c00000af x0 : 0000000000000000
> 
> [  371.115916] Call trace:
> [  371.118439]  bcm_iproc_i2c_isr+0x530/0x11f0
> [  371.122754]  __handle_irq_event_percpu+0x6c/0x170
> [  371.127606]  handle_irq_event_percpu+0x34/0x88
> [  371.132189]  handle_irq_event+0x40/0x120
> [  371.136234]  handle_fasteoi_irq+0xcc/0x1a0
> [  371.140459]  generic_handle_irq+0x24/0x38
> [  371.144594]  __handle_domain_irq+0x60/0xb8
> [  371.148820]  gic_handle_irq+0xc0/0x158
> [  371.152687]  el1_irq+0xb8/0x140
> [  371.155927]  arch_cpu_idle+0x10/0x18
> [  371.159615]  do_idle+0x204/0x290
> [  371.162943]  cpu_startup_entry+0x24/0x60
> [  371.166990]  rest_init+0xb0/0xbc
> [  371.170322]  arch_call_rest_init+0xc/0x14
> [  371.174458]  start_kernel+0x404/0x430
> 
> Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
> Signed-off-by: Dhananjay Phadke <dphadke@linux.microsoft.com>
> ---
>  drivers/i2c/busses/i2c-bcm-iproc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index b58224b7b..37d2a79e7 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -1074,14 +1074,15 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
>  	if (!iproc_i2c->slave)
>  		return -EINVAL;
>  
> -	iproc_i2c->slave = NULL;
> -
>  	/* disable all slave interrupts */
>  	tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
>  	tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
>  			IE_S_ALL_INTERRUPT_SHIFT);
>  	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
>  
> +	synchronize_irq(iproc_i2c->irq);

If one takes a look at the I2C slave ISR routine, there are places where
IRQ can be re-enabled in the ISR itself. What happens after we mask all
slave interrupt and when 'synchronize_irq' is called, which I suppose is
meant to wait for inflight interrupt to finish where there's a chance
the interrupt can be re-enable again? How is one supposed to deal with that?

Thanks,

Ray

> +	iproc_i2c->slave = NULL;
> +
>  	/* Erase the slave address programmed */
>  	tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
>  	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-20 17:49 ` Ray Jui
@ 2020-07-22 10:41   ` Wolfram Sang
  2020-07-22 15:51     ` Ray Jui
  0 siblings, 1 reply; 18+ messages in thread
From: Wolfram Sang @ 2020-07-22 10:41 UTC (permalink / raw)
  To: Ray Jui
  Cc: Dhananjay Phadke, Rayagonda Kokatanur, linux-i2c, linux-kernel,
	Ray Jui, bcm-kernel-feedback-list


[-- Attachment #1: Type: text/plain, Size: 729 bytes --]


> > +	synchronize_irq(iproc_i2c->irq);
> 
> If one takes a look at the I2C slave ISR routine, there are places where
> IRQ can be re-enabled in the ISR itself. What happens after we mask all
> slave interrupt and when 'synchronize_irq' is called, which I suppose is
> meant to wait for inflight interrupt to finish where there's a chance
> the interrupt can be re-enable again? How is one supposed to deal with that?

I encountered the same problem with the i2c-rcar driver before I left
for my holidays.

> > +	iproc_i2c->slave = NULL;
> > +
> >  	/* Erase the slave address programmed */
> >  	tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
> >  	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
> > 

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-22 10:41   ` Wolfram Sang
@ 2020-07-22 15:51     ` Ray Jui
  2020-07-22 16:55       ` Ray Jui
                         ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Ray Jui @ 2020-07-22 15:51 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Dhananjay Phadke, Rayagonda Kokatanur, linux-i2c, linux-kernel,
	Ray Jui, bcm-kernel-feedback-list


On 7/22/2020 3:41 AM, Wolfram Sang wrote:
> 
>>> +	synchronize_irq(iproc_i2c->irq);
>>
>> If one takes a look at the I2C slave ISR routine, there are places where
>> IRQ can be re-enabled in the ISR itself. What happens after we mask all
>> slave interrupt and when 'synchronize_irq' is called, which I suppose is
>> meant to wait for inflight interrupt to finish where there's a chance
>> the interrupt can be re-enable again? How is one supposed to deal with that?
> 
> I encountered the same problem with the i2c-rcar driver before I left
> for my holidays.
> 

I think the following sequence needs to be implemented to make this
safe, i.e., after 'synchronize_irq', no further slave interrupt will be
fired.

In 'bcm_iproc_i2c_unreg_slave':

1. Set an atomic variable 'unreg_slave' (I'm bad in names so please come
up with a better name than this)

2. Disable all slave interrupts

3. synchronize_irq

4. Set slave to NULL

5. Erase slave addresses

In the ISR routine, it should always check against 'unreg_slave' before
enabling any slave interrupt. If 'unreg_slave' is set, no slave
interrupt should be re-enabled from within the ISR.

I think the above sequence can ensure no further slave interrupt after
'synchronize_irq'. I suggested using an atomic variable instead of
variable + spinlock due to the way how sync irq works, i.e., "If you use
this function while holding a resource the IRQ handler may need you will
deadlock.".

Thanks,

Ray

>>> +	iproc_i2c->slave = NULL;
>>> +
>>>  	/* Erase the slave address programmed */
>>>  	tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
>>>  	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
>>>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-22 15:51     ` Ray Jui
@ 2020-07-22 16:55       ` Ray Jui
  2020-07-23  0:58       ` Dhananjay Phadke
  2020-07-25 10:18       ` Wolfram Sang
  2 siblings, 0 replies; 18+ messages in thread
From: Ray Jui @ 2020-07-22 16:55 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Dhananjay Phadke, Rayagonda Kokatanur, linux-i2c, linux-kernel,
	Ray Jui, bcm-kernel-feedback-list



On 7/22/2020 8:51 AM, Ray Jui wrote:
> 
> On 7/22/2020 3:41 AM, Wolfram Sang wrote:
>>
>>>> +	synchronize_irq(iproc_i2c->irq);
>>>
>>> If one takes a look at the I2C slave ISR routine, there are places where
>>> IRQ can be re-enabled in the ISR itself. What happens after we mask all
>>> slave interrupt and when 'synchronize_irq' is called, which I suppose is
>>> meant to wait for inflight interrupt to finish where there's a chance
>>> the interrupt can be re-enable again? How is one supposed to deal with that?
>>
>> I encountered the same problem with the i2c-rcar driver before I left
>> for my holidays.
>>
> 
> I think the following sequence needs to be implemented to make this
> safe, i.e., after 'synchronize_irq', no further slave interrupt will be
> fired.
> 
> In 'bcm_iproc_i2c_unreg_slave':
> 
> 1. Set an atomic variable 'unreg_slave' (I'm bad in names so please come
> up with a better name than this)
> 
> 2. Disable all slave interrupts

Actually, thinking about it more, 1. and 2. here need to be an atomic
operation so it needs to be wrapped by a spin lock/unlock (and it is
safe to do so here before calling synchronize_irq below).

Same applies to the two read-modify-write sequneces to enable some of
the slave interrupts in the 'bcm_iproc_i2c_slave_isr' routine.

> 
> 3. synchronize_irq
> 
> 4. Set slave to NULL
> 
> 5. Erase slave addresses
> 
> In the ISR routine, it should always check against 'unreg_slave' before
> enabling any slave interrupt. If 'unreg_slave' is set, no slave
> interrupt should be re-enabled from within the ISR.
> 
> I think the above sequence can ensure no further slave interrupt after
> 'synchronize_irq'. I suggested using an atomic variable instead of
> variable + spinlock due to the way how sync irq works, i.e., "If you use
> this function while holding a resource the IRQ handler may need you will
> deadlock.".
> 
> Thanks,
> 
> Ray
> 
>>>> +	iproc_i2c->slave = NULL;
>>>> +
>>>>  	/* Erase the slave address programmed */
>>>>  	tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
>>>>  	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
>>>>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-22 15:51     ` Ray Jui
  2020-07-22 16:55       ` Ray Jui
@ 2020-07-23  0:58       ` Dhananjay Phadke
  2020-07-25 10:18       ` Wolfram Sang
  2 siblings, 0 replies; 18+ messages in thread
From: Dhananjay Phadke @ 2020-07-23  0:58 UTC (permalink / raw)
  To: wsa, ray.jui
  Cc: rjui, rayagonda.kokatanur, linux-kernel, linux-i2c, dphadke,
	bcm-kernel-feedback-list

Ray Jui <ray.jui@broadcom.com> wrote:

>
> On 7/22/2020 3:41 AM, Wolfram Sang wrote:
> > 
> >>> +	synchronize_irq(iproc_i2c->irq);
> >>
> >> If one takes a look at the I2C slave ISR routine, there are places where
> >> IRQ can be re-enabled in the ISR itself. What happens after we mask all
> >> slave interrupt and when 'synchronize_irq' is called, which I suppose is
> >> meant to wait for inflight interrupt to finish where there's a chance
> >> the interrupt can be re-enable again? How is one supposed to deal with that?
> > 
> > I encountered the same problem with the i2c-rcar driver before I left
> > for my holidays.
> > 
>
> I think the following sequence needs to be implemented to make this
> safe, i.e., after 'synchronize_irq', no further slave interrupt will be
> fired.
>
> In 'bcm_iproc_i2c_unreg_slave':
>
> 1. Set an atomic variable 'unreg_slave' (I'm bad in names so please come
> up with a better name than this)
>
> 2. Disable all slave interrupts
>
> 3. synchronize_irq
>
> 4. Set slave to NULL
>
> 5. Erase slave addresses
>
> In the ISR routine, it should always check against 'unreg_slave' before
> enabling any slave interrupt. If 'unreg_slave' is set, no slave
> interrupt should be re-enabled from within the ISR.
>
> I think the above sequence can ensure no further slave interrupt after
> 'synchronize_irq'. I suggested using an atomic variable instead of
> variable + spinlock due to the way how sync irq works, i.e., "If you use
> this function while holding a resource the IRQ handler may need you will
> deadlock.".
>
> Thanks,
>
> Ray
>
> >>> +	iproc_i2c->slave = NULL;
> >>> +
> >>>  	/* Erase the slave address programmed */
> >>>  	tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
> >>>  	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
> >>>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-22 15:51     ` Ray Jui
  2020-07-22 16:55       ` Ray Jui
  2020-07-23  0:58       ` Dhananjay Phadke
@ 2020-07-25 10:18       ` Wolfram Sang
  2020-07-27  4:32         ` Rayagonda Kokatanur
  2020-07-27 15:42         ` Ray Jui
  2 siblings, 2 replies; 18+ messages in thread
From: Wolfram Sang @ 2020-07-25 10:18 UTC (permalink / raw)
  To: Ray Jui
  Cc: Dhananjay Phadke, Rayagonda Kokatanur, linux-i2c, linux-kernel,
	Ray Jui, bcm-kernel-feedback-list


[-- Attachment #1: Type: text/plain, Size: 841 bytes --]


> I think the following sequence needs to be implemented to make this
> safe, i.e., after 'synchronize_irq', no further slave interrupt will be
> fired.
> 
> In 'bcm_iproc_i2c_unreg_slave':
> 
> 1. Set an atomic variable 'unreg_slave' (I'm bad in names so please come
> up with a better name than this)
> 
> 2. Disable all slave interrupts
> 
> 3. synchronize_irq
> 
> 4. Set slave to NULL
> 
> 5. Erase slave addresses

What about this in unreg_slave?

1. disable_irq()
	This includes synchronize_irq() and avoids the race. Because irq
	will be masked at interrupt controller level, interrupts coming
	in at the I2C IP core level should still be pending once we
	reenable the irq.

2. disable all slave interrupts

3. enable_irq()

4. clean up the rest (pointer, address)

Or am I overlooking something?


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-25 10:18       ` Wolfram Sang
@ 2020-07-27  4:32         ` Rayagonda Kokatanur
  2020-07-27 15:42         ` Ray Jui
  1 sibling, 0 replies; 18+ messages in thread
From: Rayagonda Kokatanur @ 2020-07-27  4:32 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Ray Jui, Dhananjay Phadke, linux-i2c, Linux Kernel Mailing List,
	Ray Jui, BCM Kernel Feedback

On Sat, Jul 25, 2020 at 3:48 PM Wolfram Sang <wsa@kernel.org> wrote:
>
>
> > I think the following sequence needs to be implemented to make this
> > safe, i.e., after 'synchronize_irq', no further slave interrupt will be
> > fired.
> >
> > In 'bcm_iproc_i2c_unreg_slave':
> >
> > 1. Set an atomic variable 'unreg_slave' (I'm bad in names so please come
> > up with a better name than this)
> >
> > 2. Disable all slave interrupts
> >
> > 3. synchronize_irq
> >
> > 4. Set slave to NULL
> >
> > 5. Erase slave addresses
>
> What about this in unreg_slave?
>
> 1. disable_irq()
>         This includes synchronize_irq() and avoids the race. Because irq
>         will be masked at interrupt controller level, interrupts coming
>         in at the I2C IP core level should still be pending once we
>         reenable the irq.
>
> 2. disable all slave interrupts
>
> 3. enable_irq()
>
> 4. clean up the rest (pointer, address)
>
> Or am I overlooking something?

This sequence will take care of all cases.

@Dhananjay Phadke is it possible to verify this from your side once.

Best regards,
Raaygonda

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-25 10:18       ` Wolfram Sang
  2020-07-27  4:32         ` Rayagonda Kokatanur
@ 2020-07-27 15:42         ` Ray Jui
  2020-07-27 17:38           ` Dhananjay Phadke
  2020-07-27 18:13           ` Wolfram Sang
  1 sibling, 2 replies; 18+ messages in thread
From: Ray Jui @ 2020-07-27 15:42 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Dhananjay Phadke, Rayagonda Kokatanur, linux-i2c, linux-kernel,
	Ray Jui, bcm-kernel-feedback-list

Hi Wolfram,

On 7/25/2020 3:18 AM, Wolfram Sang wrote:
> 
>> I think the following sequence needs to be implemented to make this
>> safe, i.e., after 'synchronize_irq', no further slave interrupt will be
>> fired.
>>
>> In 'bcm_iproc_i2c_unreg_slave':
>>
>> 1. Set an atomic variable 'unreg_slave' (I'm bad in names so please come
>> up with a better name than this)
>>
>> 2. Disable all slave interrupts
>>
>> 3. synchronize_irq
>>
>> 4. Set slave to NULL
>>
>> 5. Erase slave addresses
> 
> What about this in unreg_slave?
> 
> 1. disable_irq()
> 	This includes synchronize_irq() and avoids the race. Because irq
> 	will be masked at interrupt controller level, interrupts coming
> 	in at the I2C IP core level should still be pending once we
> 	reenable the irq.
> 

Can you confirm that even if we have irq pending at the i2c IP core
level, as long as we execute Step 2. below (to disable/mask all slave
interrupts), after 'enable_irq' is called, we still will not receive any
further i2c slave interrupt?

Basically I'm asking if interrupts will be "cached" at the GIC
controller level after 'disable_irq' is called. As long as that is not
the case, then I think we are good.

The goal of course is to ensure there's no further slave interrupts
after 'enable_irq' in Step 3 below.

Thanks!

> 2. disable all slave interrupts
> 
> 3. enable_irq()
> 
> 4. clean up the rest (pointer, address)
> 
> Or am I overlooking something?
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-27 15:42         ` Ray Jui
@ 2020-07-27 17:38           ` Dhananjay Phadke
  2020-07-27 18:13           ` Wolfram Sang
  1 sibling, 0 replies; 18+ messages in thread
From: Dhananjay Phadke @ 2020-07-27 17:38 UTC (permalink / raw)
  To: ray.jui
  Cc: bcm-kernel-feedback-list, dphadke, linux-i2c, linux-kernel,
	rayagonda.kokatanur, rjui, wsa

Ray Jui <ray.jui@broadcom.com> wrote:
>>> I think the following sequence needs to be implemented to make this
>>> safe, i.e., after 'synchronize_irq', no further slave interrupt will be
>>> fired.
>>>
>>> In 'bcm_iproc_i2c_unreg_slave':
>>>
>>> 1. Set an atomic variable 'unreg_slave' (I'm bad in names so please come
>>> up with a better name than this)
>>>
>>> 2. Disable all slave interrupts
>>>
>>> 3. synchronize_irq
>>>
>>> 4. Set slave to NULL
>>>
>>> 5. Erase slave addresses
>> 
>> What about this in unreg_slave?
>> 
>> 1. disable_irq()
>> 	This includes synchronize_irq() and avoids the race. Because irq
>> 	will be masked at interrupt controller level, interrupts coming
>> 	in at the I2C IP core level should still be pending once we
>> 	reenable the irq.
>> 
> 
> Can you confirm that even if we have irq pending at the i2c IP core
> level, as long as we execute Step 2. below (to disable/mask all slave
> interrupts), after 'enable_irq' is called, we still will not receive any
> further i2c slave interrupt?
> 
> Basically I'm asking if interrupts will be "cached" at the GIC
> controller level after 'disable_irq' is called. As long as that is not
> the case, then I think we are good.
> 
> The goal of course is to ensure there's no further slave interrupts
> after 'enable_irq' in Step 3 below.

That was my question as well, the best would be if the i2c controller itself
has a bit for masking all interrupts overriding individual event enables
set by the ISR.

Also with regards to the original sequence, I think slave address should
be erased before enable_irq(), besides draining rx and tx FIFOs.

I'll send reworked patch.

@Rayagonda will validate new sequence with the test that hit the race condition.

- Dhananjay


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-27 15:42         ` Ray Jui
  2020-07-27 17:38           ` Dhananjay Phadke
@ 2020-07-27 18:13           ` Wolfram Sang
  2020-07-27 20:26             ` Wolfram Sang
  1 sibling, 1 reply; 18+ messages in thread
From: Wolfram Sang @ 2020-07-27 18:13 UTC (permalink / raw)
  To: Ray Jui
  Cc: Dhananjay Phadke, Rayagonda Kokatanur, linux-i2c, linux-kernel,
	Ray Jui, bcm-kernel-feedback-list


[-- Attachment #1: Type: text/plain, Size: 513 bytes --]


> Can you confirm that even if we have irq pending at the i2c IP core
> level, as long as we execute Step 2. below (to disable/mask all slave
> interrupts), after 'enable_irq' is called, we still will not receive any
> further i2c slave interrupt?

This is HW dependant. From my tests with Renesas HW, this is not the
case. But the actual error case was impossible to trigger for me, so
far. I might try again later. But even in the worst case, I would only
get a "spurious interrupt" and not an NULL-ptr OOPS.


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-27 18:13           ` Wolfram Sang
@ 2020-07-27 20:26             ` Wolfram Sang
  2020-07-27 20:43               ` Ray Jui
  0 siblings, 1 reply; 18+ messages in thread
From: Wolfram Sang @ 2020-07-27 20:26 UTC (permalink / raw)
  To: Ray Jui
  Cc: Dhananjay Phadke, Rayagonda Kokatanur, linux-i2c, linux-kernel,
	Ray Jui, bcm-kernel-feedback-list


[-- Attachment #1: Type: text/plain, Size: 1211 bytes --]

On Mon, Jul 27, 2020 at 08:13:46PM +0200, Wolfram Sang wrote:
> 
> > Can you confirm that even if we have irq pending at the i2c IP core
> > level, as long as we execute Step 2. below (to disable/mask all slave
> > interrupts), after 'enable_irq' is called, we still will not receive any
> > further i2c slave interrupt?
> 
> This is HW dependant. From my tests with Renesas HW, this is not the
> case. But the actual error case was impossible to trigger for me, so
> far. I might try again later. But even in the worst case, I would only
> get a "spurious interrupt" and not an NULL-ptr OOPS.

Let me explain how I verified this:

0) add a debug print whenever the slave irq part is called

1) Put a 2 second delay after disable_irq() and before clearing
interrupt enable register

2) unbind the slave driver in the background, triggering the 2s delay

3) during the delay, try to read from the to-be-unbound slave in the
   foreground

4) ensure there is no prinout from the slave irq

Worked fine for me with the Renesas R-Car I2C IP interface. As mentioned
before, I couldn't trigger a bad case with my setup. So, I hope this new
fix will work for Rayagonda's test case, too!


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-27 20:26             ` Wolfram Sang
@ 2020-07-27 20:43               ` Ray Jui
  2020-08-05  9:17                 ` Wolfram Sang
  0 siblings, 1 reply; 18+ messages in thread
From: Ray Jui @ 2020-07-27 20:43 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Dhananjay Phadke, Rayagonda Kokatanur, linux-i2c, linux-kernel,
	Ray Jui, bcm-kernel-feedback-list



On 7/27/2020 1:26 PM, Wolfram Sang wrote:
> On Mon, Jul 27, 2020 at 08:13:46PM +0200, Wolfram Sang wrote:
>>
>>> Can you confirm that even if we have irq pending at the i2c IP core
>>> level, as long as we execute Step 2. below (to disable/mask all slave
>>> interrupts), after 'enable_irq' is called, we still will not receive any
>>> further i2c slave interrupt?
>>
>> This is HW dependant. From my tests with Renesas HW, this is not the
>> case. But the actual error case was impossible to trigger for me, so
>> far. I might try again later. But even in the worst case, I would only
>> get a "spurious interrupt" and not an NULL-ptr OOPS.
> 
> Let me explain how I verified this:
> 
> 0) add a debug print whenever the slave irq part is called
> 
> 1) Put a 2 second delay after disable_irq() and before clearing
> interrupt enable register
> 
> 2) unbind the slave driver in the background, triggering the 2s delay
> 
> 3) during the delay, try to read from the to-be-unbound slave in the
>    foreground
> 
> 4) ensure there is no prinout from the slave irq
> 
> Worked fine for me with the Renesas R-Car I2C IP interface. As mentioned
> before, I couldn't trigger a bad case with my setup. So, I hope this new
> fix will work for Rayagonda's test case, too!
> 

Sure. I suggest Dhananjay gives the sequence you proposed here a try
(with delay added during the testing to widen the window to cover corner
cases). If it works, we can just go with your proposed sequence here.

Thanks!

Ray

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-07-27 20:43               ` Ray Jui
@ 2020-08-05  9:17                 ` Wolfram Sang
  2020-08-07 16:40                   ` Ray Jui
  0 siblings, 1 reply; 18+ messages in thread
From: Wolfram Sang @ 2020-08-05  9:17 UTC (permalink / raw)
  To: Ray Jui
  Cc: Dhananjay Phadke, Rayagonda Kokatanur, linux-i2c, linux-kernel,
	Ray Jui, bcm-kernel-feedback-list


[-- Attachment #1: Type: text/plain, Size: 1674 bytes --]

On Mon, Jul 27, 2020 at 01:43:40PM -0700, Ray Jui wrote:
> 
> 
> On 7/27/2020 1:26 PM, Wolfram Sang wrote:
> > On Mon, Jul 27, 2020 at 08:13:46PM +0200, Wolfram Sang wrote:
> >>
> >>> Can you confirm that even if we have irq pending at the i2c IP core
> >>> level, as long as we execute Step 2. below (to disable/mask all slave
> >>> interrupts), after 'enable_irq' is called, we still will not receive any
> >>> further i2c slave interrupt?
> >>
> >> This is HW dependant. From my tests with Renesas HW, this is not the
> >> case. But the actual error case was impossible to trigger for me, so
> >> far. I might try again later. But even in the worst case, I would only
> >> get a "spurious interrupt" and not an NULL-ptr OOPS.
> > 
> > Let me explain how I verified this:
> > 
> > 0) add a debug print whenever the slave irq part is called
> > 
> > 1) Put a 2 second delay after disable_irq() and before clearing
> > interrupt enable register
> > 
> > 2) unbind the slave driver in the background, triggering the 2s delay
> > 
> > 3) during the delay, try to read from the to-be-unbound slave in the
> >    foreground
> > 
> > 4) ensure there is no prinout from the slave irq
> > 
> > Worked fine for me with the Renesas R-Car I2C IP interface. As mentioned
> > before, I couldn't trigger a bad case with my setup. So, I hope this new
> > fix will work for Rayagonda's test case, too!
> > 
> 
> Sure. I suggest Dhananjay gives the sequence you proposed here a try
> (with delay added during the testing to widen the window to cover corner
> cases). If it works, we can just go with your proposed sequence here.

Any progress yet?


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-08-05  9:17                 ` Wolfram Sang
@ 2020-08-07 16:40                   ` Ray Jui
  2020-08-07 17:38                     ` Dhananjay Phadke
  0 siblings, 1 reply; 18+ messages in thread
From: Ray Jui @ 2020-08-07 16:40 UTC (permalink / raw)
  To: Wolfram Sang, Dhananjay Phadke, Rayagonda Kokatanur, linux-i2c,
	linux-kernel, Ray Jui, bcm-kernel-feedback-list

Hi Rayagonda/Dhananjay,

On 8/5/2020 2:17 AM, Wolfram Sang wrote:
> On Mon, Jul 27, 2020 at 01:43:40PM -0700, Ray Jui wrote:
>>
>>
>> On 7/27/2020 1:26 PM, Wolfram Sang wrote:
>>> On Mon, Jul 27, 2020 at 08:13:46PM +0200, Wolfram Sang wrote:
>>>>
>>>>> Can you confirm that even if we have irq pending at the i2c IP core
>>>>> level, as long as we execute Step 2. below (to disable/mask all slave
>>>>> interrupts), after 'enable_irq' is called, we still will not receive any
>>>>> further i2c slave interrupt?
>>>>
>>>> This is HW dependant. From my tests with Renesas HW, this is not the
>>>> case. But the actual error case was impossible to trigger for me, so
>>>> far. I might try again later. But even in the worst case, I would only
>>>> get a "spurious interrupt" and not an NULL-ptr OOPS.
>>>
>>> Let me explain how I verified this:
>>>
>>> 0) add a debug print whenever the slave irq part is called
>>>
>>> 1) Put a 2 second delay after disable_irq() and before clearing
>>> interrupt enable register
>>>
>>> 2) unbind the slave driver in the background, triggering the 2s delay
>>>
>>> 3) during the delay, try to read from the to-be-unbound slave in the
>>>    foreground
>>>
>>> 4) ensure there is no prinout from the slave irq
>>>
>>> Worked fine for me with the Renesas R-Car I2C IP interface. As mentioned
>>> before, I couldn't trigger a bad case with my setup. So, I hope this new
>>> fix will work for Rayagonda's test case, too!
>>>
>>
>> Sure. I suggest Dhananjay gives the sequence you proposed here a try
>> (with delay added during the testing to widen the window to cover corner
>> cases). If it works, we can just go with your proposed sequence here.
> 
> Any progress yet?
> 

I don't know if Dhananjay is actively working on this or not.

Rayagonda, given that you have the I2C slave setup already, do you think
you can help to to test and above sequence from Wolfram (by using the
widened delay window as instructed)?

Thanks,

Ray

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] i2c: iproc: fix race between client unreg and isr
  2020-08-07 16:40                   ` Ray Jui
@ 2020-08-07 17:38                     ` Dhananjay Phadke
  0 siblings, 0 replies; 18+ messages in thread
From: Dhananjay Phadke @ 2020-08-07 17:38 UTC (permalink / raw)
  To: ray.jui
  Cc: bcm-kernel-feedback-list, dphadke, linux-i2c, linux-kernel,
	rayagonda.kokatanur, rjui, wsa

Ray Jui wrote:
> 
> Any progress yet?
> 

> I don't know if Dhananjay is actively working on this or not.
> 
> Rayagonda, given that you have the I2C slave setup already, do you think
> you can help to to test and above sequence from Wolfram (by using the
> widened delay window as instructed)?
> 
> Thanks,
> 
> Ray

Sorry I was out for a while, I've tested the fix with delay and original
i2c client test, will send v2 patch shortly.

Thanks,
Dhananjay

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, back to index

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-18 23:39 [PATCH] i2c: iproc: fix race between client unreg and isr Dhananjay Phadke
2020-07-19  7:59 ` Rayagonda Kokatanur
2020-07-20 17:40   ` Scott Branden
2020-07-20 17:49 ` Ray Jui
2020-07-22 10:41   ` Wolfram Sang
2020-07-22 15:51     ` Ray Jui
2020-07-22 16:55       ` Ray Jui
2020-07-23  0:58       ` Dhananjay Phadke
2020-07-25 10:18       ` Wolfram Sang
2020-07-27  4:32         ` Rayagonda Kokatanur
2020-07-27 15:42         ` Ray Jui
2020-07-27 17:38           ` Dhananjay Phadke
2020-07-27 18:13           ` Wolfram Sang
2020-07-27 20:26             ` Wolfram Sang
2020-07-27 20:43               ` Ray Jui
2020-08-05  9:17                 ` Wolfram Sang
2020-08-07 16:40                   ` Ray Jui
2020-08-07 17:38                     ` Dhananjay Phadke

Linux-i2c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i2c/0 linux-i2c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i2c linux-i2c/ https://lore.kernel.org/linux-i2c \
		linux-i2c@vger.kernel.org
	public-inbox-index linux-i2c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-i2c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git