Re: [PATCH v8 4/5] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

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;
>> +}
>> +