* [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 related [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, other threads:[~2020-08-07 17:38 UTC | newest] 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
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.