[PATCH v8 4/5] i2c: aspeed: added driver for Aspeed I2C
brendanhiggins at google.com
Fri Jun 2 19:29:39 AEST 2017
I addressed all of your comments in my next revision except the one below.
>> + time_left = wait_for_completion_timeout(&bus->cmd_complete,
>> + bus->adap.timeout);
>> + spin_lock_irqsave(&bus->lock, flags);
>> + bus->msgs = NULL;
>> + if (time_left == 0)
>> + ret = -ETIMEDOUT;
>> + else
>> + ret = bus->cmd_err;
>> + spin_unlock_irqrestore(&bus->lock, flags);
> I would make the interrupt handler self-clear msgs and I would copy
> cmd_err to a separate field (or ensure it's only set by the interrupt
> handler when msgs is non-NULL, by the completion code).
> That way you avoid the above lock which is racy, slave activity could
> get in there and trigger an error interrupt clobbering cmd_err for
> example no ? Or am I missing something...
The slave handler does not touch these fields, so that should be fine.
The only way I can think
that we could get in this state is if we had some sort of error
interrupt fire after we handled a
STOP or previous error; this should not happen, but I think it is
better to be safe than sorry.
Nevertheless, I do not think it is necessary to do more than what I
have already done because
it would mean the bus is in a pretty bad state anyway. Maybe I should
just drop the locks here.
If you disagree, could you elaborate on what you meant by putting
cmd_err in a separate field?
Did you just mean having one for xfer and one for everything else (like resets)?
>> + /* If nothing went wrong, return number of messages transferred. */
>> + if (ret >= 0)
>> + return bus->msgs_index + 1;
>> + else
>> + return ret;
More information about the openbmc