New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 825102 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

ec: i2c_master.c: i2c_lock might call interrupt_disable/enable twice

Project Member Reported by drinkcat@chromium.org, Mar 23 2018

Issue description

i2c_lock does the following:

		interrupt_disable();

		i2c_port_active_count++;
		/* Ec cannot enter sleep if there's any i2c port active. */
		disable_sleep(SLEEP_MASK_I2C_MASTER);

		interrupt_enable();

disable_sleep() calls atomic_or()

This should be fine on cortex-m with proper atomic operations.

On cortex-m0 without proper atomic ops, this will call interrupt_disable() a second time, then interrupt_enable() twice in a row:

For example, on hammer:
armv7a-cros-linux-gnueabi-objdump -d build/hammer/RW/ec.RW.elf

080137ac <i2c_lock>:
 80137ac:       b508            push    {r3, lr}
 80137ae:       2800            cmp     r0, #0
 80137b0:       db18            blt.n   80137e4 <i2c_lock+0x38>
 80137b2:       2900            cmp     r1, #0
 80137b4:       d002            beq.n   80137bc <i2c_lock+0x10>
 80137b6:       f7ff ffdf       bl      8013778 <i2c_lock.part.0.lto_priv.102>
 80137ba:       e013            b.n     80137e4 <i2c_lock+0x38>
 80137bc:       b672            cpsid   i
 80137be:       4a0a            ldr     r2, [pc, #40]   ; (80137e8 <i2c_lock+0x3c>)
 80137c0:       6813            ldr     r3, [r2, #0]
 80137c2:       3b01            subs    r3, #1
 80137c4:       6013            str     r3, [r2, #0]
 80137c6:       2b00            cmp     r3, #0
 80137c8:       d106            bne.n   80137d8 <i2c_lock+0x2c>
 80137ca:       2204            movs    r2, #4
 80137cc:       4b07            ldr     r3, [pc, #28]   ; (80137ec <i2c_lock+0x40>)
 80137ce:       b672            cpsid   i
 80137d0:       6819            ldr     r1, [r3, #0]
 80137d2:       4391            bics    r1, r2
 80137d4:       6019            str     r1, [r3, #0]
 80137d6:       b662            cpsie   i
 80137d8:       b662            cpsie   i
 80137da:       4b05            ldr     r3, [pc, #20]   ; (80137f0 <i2c_lock+0x44>)
 80137dc:       00c0            lsls    r0, r0, #3
 80137de:       18c0            adds    r0, r0, r3
 80137e0:       f7ff fc44       bl      801306c <mutex_unlock>
 80137e4:       bd08            pop     {r3, pc}
 80137e6:       46c0            nop                     ; (mov r8, r8)
 80137e8:       20002b34        .word   0x20002b34
 80137ec:       20002c60        .word   0x20002c60
 80137f0:       20002b24        .word   0x20002b24

Probably harmless anyway, but we might still want to fix this.
 

Comment 1 by vpalatin@google.com, Mar 23 2018

Yes this construct is wrong, that why I'm very annoying about not using interrupt_disable() ;-)
AFAICT I agree this is totally harmless.
This why I think we need a flag save w.r.t. interrupt_disable(). That or keep a per task count for interrupt_disable()/interrupt_enable() nesting.
Yes.  It should be

uint32_t prev_state = interrupt_disable();

interrupt_restore(prev_state);


Sign in to add a comment