| Message ID | 20161107200921.30284-2-paul.burton@imgtec.com |
|---|---|
| State | Deferred |
| Headers | show |
On Mon, Nov 07, 2016 at 08:09:21PM +0000, Paul Burton wrote: > Commit 1bb1ff3e7c74 ("i2c: octeon: Improve performance if interrupt is > early") modified octeon_i2c_wait() & octeon_i2c_hlc_wait() to attempt to > check for a valid bit being clear & if not to sleep for a while then try > again before waiting on a waitqueue which may time out. However it does > so by sleeping within a function called as the condition provided to > wait_event_timeout() which seems to cause strange behaviour, with the > system hanging during boot with the condition being checked constantly & > the timeout not seeming to have any effect. > > Fix this by instead checking for the valid bit being clear in the > octeon_i2c(_hlc)_wait() functions & sleeping there if that condition is > not met, then calling the wait_event_timeout with a condition that does > not sleep. > > Tested on a Rhino Labs UTM-8 with Octeon CN7130. This patch breaks ipmi on ThunderX (CN88xx). We also have not seen the problems you mention, although I must admit that I don't like the complicated nested waits. --Jan > Signed-off-by: Paul Burton <paul.burton@imgtec.com> > Cc: David Daney <david.daney@cavium.com> > Cc: Jan Glauber <jglauber@cavium.com> > Cc: Peter Swain <pswain@cavium.com> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-i2c@vger.kernel.org > --- > > drivers/i2c/busses/i2c-octeon-core.c | 58 ++++++++++-------------------------- > 1 file changed, 15 insertions(+), 43 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c > index 419b54b..2e270a1 100644 > --- a/drivers/i2c/busses/i2c-octeon-core.c > +++ b/drivers/i2c/busses/i2c-octeon-core.c > @@ -36,24 +36,6 @@ static bool octeon_i2c_test_iflg(struct octeon_i2c *i2c) > return (octeon_i2c_ctl_read(i2c) & TWSI_CTL_IFLG); > } > > -static bool octeon_i2c_test_ready(struct octeon_i2c *i2c, bool *first) > -{ > - if (octeon_i2c_test_iflg(i2c)) > - return true; > - > - if (*first) { > - *first = false; > - return false; > - } > - > - /* > - * IRQ has signaled an event but IFLG hasn't changed. > - * Sleep and retry once. > - */ > - usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT); > - return octeon_i2c_test_iflg(i2c); > -} > - > /** > * octeon_i2c_wait - wait for the IFLG to be set > * @i2c: The struct octeon_i2c > @@ -80,8 +62,13 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c) > } > > i2c->int_enable(i2c); > - time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_ready(i2c, &first), > - i2c->adap.timeout); > + time_left = i2c->adap.timeout; > + if (!octeon_i2c_test_iflg(i2c)) { > + usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT); > + time_left = wait_event_timeout(i2c->queue, > + octeon_i2c_test_iflg(i2c), > + time_left); > + } > i2c->int_disable(i2c); > > if (i2c->broken_irq_check && !time_left && > @@ -99,26 +86,8 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c) > > static bool octeon_i2c_hlc_test_valid(struct octeon_i2c *i2c) > { > - return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0; > -} > - > -static bool octeon_i2c_hlc_test_ready(struct octeon_i2c *i2c, bool *first) > -{ > /* check if valid bit is cleared */ > - if (octeon_i2c_hlc_test_valid(i2c)) > - return true; > - > - if (*first) { > - *first = false; > - return false; > - } > - > - /* > - * IRQ has signaled an event but valid bit isn't cleared. > - * Sleep and retry once. > - */ > - usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT); > - return octeon_i2c_hlc_test_valid(i2c); > + return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0; > } > > static void octeon_i2c_hlc_int_clear(struct octeon_i2c *i2c) > @@ -176,7 +145,6 @@ static void octeon_i2c_hlc_disable(struct octeon_i2c *i2c) > */ > static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c) > { > - bool first = true; > int time_left; > > /* > @@ -194,9 +162,13 @@ static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c) > } > > i2c->hlc_int_enable(i2c); > - time_left = wait_event_timeout(i2c->queue, > - octeon_i2c_hlc_test_ready(i2c, &first), > - i2c->adap.timeout); > + time_left = i2c->adap.timeout; > + if (!octeon_i2c_hlc_test_valid(i2c)) { > + usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT); > + time_left = wait_event_timeout(i2c->queue, > + octeon_i2c_hlc_test_valid(i2c), > + time_left); > + } > i2c->hlc_int_disable(i2c); > if (!time_left) > octeon_i2c_hlc_int_clear(i2c); > -- > 2.10.2 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Paul, I think we should revert commit "70121f7 i2c: octeon: thunderx: Limit register access retries". With debugging enabled I'm getting: [ 78.871568] ipmi_ssif: Trying hotmod-specified SSIF interface at i2c address 0x12, adapter Cavium ThunderX i2c adapter at 0000:01:09.4, slave address 0x0 [ 78.886107] do not call blocking ops when !TASK_RUNNING; state=2 set at [<fffffc00080e0088>] prepare_to_wait_event+0x58/0x10c [ 78.897436] ------------[ cut here ]------------ [ 78.902050] WARNING: CPU: 6 PID: 2235 at kernel/sched/core.c:7718 __might_sleep+0x80/0x88 [ 78.910218] Modules linked in: ipmi_ssif i2c_thunderx i2c_smbus nicvf nicpf thunder_bgx thunder_xcv mdio_thunder [ 78.921916] CPU: 6 PID: 2235 Comm: bash Tainted: G W 4.9.0-rc3-jang+ #17 [ 78.929737] Hardware name: www.cavium.com ThunderX CRB-1S/ThunderX CRB-1S, BIOS 0.3 Aug 24 2016 [ 78.938426] task: fffffe1fdd554500 task.stack: fffffe1fe384c000 [ 78.944338] PC is at __might_sleep+0x80/0x88 [ 78.948601] LR is at __might_sleep+0x80/0x88 [ 78.952863] pc : [<fffffc00080c3aac>] lr : [<fffffc00080c3aac>] pstate: 80000145 [ 78.960250] sp : fffffe1fe384f600 [ 78.963557] x29: fffffe1fe384f600 x28: fffffe1fe384f860 [ 78.968875] x27: fffffe1fd07fa018 x26: fffffe1fe384f968 [ 78.974193] x25: fffffc0009a2b000 x24: 00000000ffff26d6 [ 78.979510] x23: fffffe1fe384f860 x22: fffffe1fe384f860 [ 78.984827] x21: 0000000000000000 x20: 00000000000000b1 [ 78.990144] x19: fffffc0000e330b8 x18: 0000000000005bb0 [ 78.995461] x17: fffffc0009669ca8 x16: 0000000000000000 [ 79.000779] x15: 0000000000000539 x14: 66663c5b20746120 [ 79.006097] x13: 74657320323d6574 x12: 617473203b474e49 [ 79.011415] x11: 4e4e55525f4b5341 x10: 5421206e65687720 [ 79.016732] x9 : 73706f20676e696b x8 : 0000000000000000 [ 79.022049] x7 : fffffc00080f5740 x6 : fffffc00080f5740 [ 79.027367] x5 : ffffffffffffff80 x4 : 0000000000000060 [ 79.032684] x3 : 0000000000000000 x2 : 0000000000000001 [ 79.038001] x1 : 0000000000000000 x0 : 0000000000000071 [ 79.044803] ---[ end trace d8af6005f683d444 ]--- [ 79.049413] Call trace: [ 79.051853] Exception stack(0xfffffe1fe384f420 to 0xfffffe1fe384f550) [ 79.058287] f420: fffffc0000e330b8 0000040000000000 fffffe1fe384f600 fffffc00080c3aac [ 79.066109] f440: 0000000080000145 000000000000003d 0000000000000000 fffffc0008853920 [ 79.073931] f460: 0000040000000000 0000000100000001 fffffe1fe384f520 fffffc00080f60d8 [ 79.081752] f480: fffffc0000e330b8 00000000000000b1 0000000000000000 fffffe1fe384f860 [ 79.089574] f4a0: fffffe1fe384f860 00000000ffff26d6 fffffc0009a2b000 fffffe1fe384f968 [ 79.097396] f4c0: fffffe1fd07fa018 fffffe1fe384f860 0000000000000071 0000000000000000 [ 79.105218] f4e0: 0000000000000001 0000000000000000 0000000000000060 ffffffffffffff80 [ 79.113040] f500: fffffc00080f5740 fffffc00080f5740 0000000000000000 73706f20676e696b [ 79.120861] f520: 5421206e65687720 4e4e55525f4b5341 617473203b474e49 74657320323d6574 [ 79.128683] f540: 66663c5b20746120 0000000000000539 [ 79.133553] [<fffffc00080c3aac>] __might_sleep+0x80/0x88 [ 79.138862] [<fffffc0000e30138>] octeon_i2c_test_iflg+0x4c/0xbc [i2c_thunderx] [ 79.146077] [<fffffc0000e30958>] octeon_i2c_test_ready+0x18/0x70 [i2c_thunderx] [ 79.153379] [<fffffc0000e30b04>] octeon_i2c_wait+0x154/0x1a4 [i2c_thunderx] [ 79.160334] [<fffffc0000e310bc>] octeon_i2c_xfer+0xf4/0xf60 [i2c_thunderx] This is not caused by the usleep inside the wait_event but by readq_poll_timeout(). Could you try if it works for you if you only revert this patch? Thanks, Jan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, 9 November 2016 14:41:03 GMT Jan Glauber wrote: > Hi Paul, > > I think we should revert commit "70121f7 i2c: octeon: thunderx: Limit > register access retries". With debugging enabled I'm getting: > > <snip> > > This is not caused by the usleep inside the wait_event but by > readq_poll_timeout(). Could you try if it works for you if you only revert > this patch? > > Thanks, > Jan Hi Jan, If I drop both my patches & just revert 70121f7f3725 ("i2c: octeon: thunderx: Limit register access retries") sadly it doesn't fix my system. A boot of a cavium_octeon_defconfig kernel with initcall_debug ends with: calling octeon_mgmt_mod_init+0x0/0x28 @ 1 initcall octeon_mgmt_mod_init+0x0/0x28 returned 0 after 67 usecs calling ds1307_driver_init+0x0/0x10 @ 1 initcall ds1307_driver_init+0x0/0x10 returned 0 after 19 usecs calling octeon_i2c_driver_init+0x0/0x10 @ 1 ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) ata1.00: ATA-9: SanDisk SDSSDA240G, Z22000RL, max UDMA/133 ata1.00: 468862128 sectors, multi 1: LBA48 NCQ (depth 31/32) ata1.00: configured for UDMA/133 scsi 0:0:0:0: Direct-Access ATA SanDisk SDSSDA24 00RL PQ: 0 ANSI: 5 sd 0:0:0:0: [sda] 468862128 512-byte logical blocks: (240 GB/224 GiB) sd 0:0:0:0: [sda] Write Protect is off sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA sda: sda1 sda2 sda3 sda4 sd 0:0:0:0: [sda] Attached SCSI disk ata2: SATA link down (SStatus 0 SControl 300) random: crng init done As you can see octeon_i2c_driver_init never returns. Are you able to test on one of your MIPS-based systems? Thanks, Paul
On Wed, Nov 09, 2016 at 02:07:58PM +0000, Paul Burton wrote: > On Wednesday, 9 November 2016 14:41:03 GMT Jan Glauber wrote: > > Hi Paul, > > > > I think we should revert commit "70121f7 i2c: octeon: thunderx: Limit > > register access retries". With debugging enabled I'm getting: > > > > <snip> > > > > This is not caused by the usleep inside the wait_event but by > > readq_poll_timeout(). Could you try if it works for you if you only revert > > this patch? > > > > Thanks, > > Jan > > Hi Jan, > > If I drop both my patches & just revert 70121f7f3725 ("i2c: octeon: thunderx: > Limit register access retries") sadly it doesn't fix my system. A boot of a > cavium_octeon_defconfig kernel with initcall_debug ends with: > > calling octeon_mgmt_mod_init+0x0/0x28 @ 1 > initcall octeon_mgmt_mod_init+0x0/0x28 returned 0 after 67 usecs > calling ds1307_driver_init+0x0/0x10 @ 1 > initcall ds1307_driver_init+0x0/0x10 returned 0 after 19 usecs > calling octeon_i2c_driver_init+0x0/0x10 @ 1 > ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) > ata1.00: ATA-9: SanDisk SDSSDA240G, Z22000RL, max UDMA/133 > ata1.00: 468862128 sectors, multi 1: LBA48 NCQ (depth 31/32) > ata1.00: configured for UDMA/133 > scsi 0:0:0:0: Direct-Access ATA SanDisk SDSSDA24 00RL PQ: 0 ANSI: 5 > sd 0:0:0:0: [sda] 468862128 512-byte logical blocks: (240 GB/224 GiB) > sd 0:0:0:0: [sda] Write Protect is off > sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 > sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support > DPO or FUA > sda: sda1 sda2 sda3 sda4 > sd 0:0:0:0: [sda] Attached SCSI disk > ata2: SATA link down (SStatus 0 SControl 300) > random: crng init done > > As you can see octeon_i2c_driver_init never returns. Are you able to test on > one of your MIPS-based systems? CC'ing Steven who might be able to test on MIPS. > Thanks, > Paul -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c index 419b54b..2e270a1 100644 --- a/drivers/i2c/busses/i2c-octeon-core.c +++ b/drivers/i2c/busses/i2c-octeon-core.c @@ -36,24 +36,6 @@ static bool octeon_i2c_test_iflg(struct octeon_i2c *i2c) return (octeon_i2c_ctl_read(i2c) & TWSI_CTL_IFLG); } -static bool octeon_i2c_test_ready(struct octeon_i2c *i2c, bool *first) -{ - if (octeon_i2c_test_iflg(i2c)) - return true; - - if (*first) { - *first = false; - return false; - } - - /* - * IRQ has signaled an event but IFLG hasn't changed. - * Sleep and retry once. - */ - usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT); - return octeon_i2c_test_iflg(i2c); -} - /** * octeon_i2c_wait - wait for the IFLG to be set * @i2c: The struct octeon_i2c @@ -80,8 +62,13 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c) } i2c->int_enable(i2c); - time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_ready(i2c, &first), - i2c->adap.timeout); + time_left = i2c->adap.timeout; + if (!octeon_i2c_test_iflg(i2c)) { + usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT); + time_left = wait_event_timeout(i2c->queue, + octeon_i2c_test_iflg(i2c), + time_left); + } i2c->int_disable(i2c); if (i2c->broken_irq_check && !time_left && @@ -99,26 +86,8 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c) static bool octeon_i2c_hlc_test_valid(struct octeon_i2c *i2c) { - return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0; -} - -static bool octeon_i2c_hlc_test_ready(struct octeon_i2c *i2c, bool *first) -{ /* check if valid bit is cleared */ - if (octeon_i2c_hlc_test_valid(i2c)) - return true; - - if (*first) { - *first = false; - return false; - } - - /* - * IRQ has signaled an event but valid bit isn't cleared. - * Sleep and retry once. - */ - usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT); - return octeon_i2c_hlc_test_valid(i2c); + return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0; } static void octeon_i2c_hlc_int_clear(struct octeon_i2c *i2c) @@ -176,7 +145,6 @@ static void octeon_i2c_hlc_disable(struct octeon_i2c *i2c) */ static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c) { - bool first = true; int time_left; /* @@ -194,9 +162,13 @@ static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c) } i2c->hlc_int_enable(i2c); - time_left = wait_event_timeout(i2c->queue, - octeon_i2c_hlc_test_ready(i2c, &first), - i2c->adap.timeout); + time_left = i2c->adap.timeout; + if (!octeon_i2c_hlc_test_valid(i2c)) { + usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT); + time_left = wait_event_timeout(i2c->queue, + octeon_i2c_hlc_test_valid(i2c), + time_left); + } i2c->hlc_int_disable(i2c); if (!time_left) octeon_i2c_hlc_int_clear(i2c);
Commit 1bb1ff3e7c74 ("i2c: octeon: Improve performance if interrupt is early") modified octeon_i2c_wait() & octeon_i2c_hlc_wait() to attempt to check for a valid bit being clear & if not to sleep for a while then try again before waiting on a waitqueue which may time out. However it does so by sleeping within a function called as the condition provided to wait_event_timeout() which seems to cause strange behaviour, with the system hanging during boot with the condition being checked constantly & the timeout not seeming to have any effect. Fix this by instead checking for the valid bit being clear in the octeon_i2c(_hlc)_wait() functions & sleeping there if that condition is not met, then calling the wait_event_timeout with a condition that does not sleep. Tested on a Rhino Labs UTM-8 with Octeon CN7130. Signed-off-by: Paul Burton <paul.burton@imgtec.com> Cc: David Daney <david.daney@cavium.com> Cc: Jan Glauber <jglauber@cavium.com> Cc: Peter Swain <pswain@cavium.com> Cc: Wolfram Sang <wsa@the-dreams.de> Cc: linux-i2c@vger.kernel.org --- drivers/i2c/busses/i2c-octeon-core.c | 58 ++++++++++-------------------------- 1 file changed, 15 insertions(+), 43 deletions(-)