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; >> +} >> + -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
