On Thu, 19 Feb 2009 22:23:25 +0100, Clifford Wolf wrote: > Hi Jean, > > On Thu, Feb 19, 2009 at 05:26:05PM +0100, Jean Delvare wrote: > > > @@ -1000,7 +1000,8 @@ module_exit(i2c_exit); > > > */ > > > int i2c_transfer(struct i2c_adapter * adap, struct i2c_msg *msgs, int num) > > > { > > > - int ret; > > > + unsigned long orig_jiffies = jiffies; > > > > I think you should initialize orig_jiffies *after* you get the bus > > lock. Otherwise the behavior depends on how long you had to wait to get > > control of the bus. Or was is intended? > > phew! this is a good question... > > to be honest: I haven't thought about that one yet. > > I think both approaches (including the wait for the lock in the timeout on > the one hand and just counting the time spent after getting the lock on the > other hand) would be valid.. > > But I think it would be better to not include the wait-for-lock time and > move the initialization of orig_jiffies to after locking the mutex. I agree... Please send an updated patch. > In the case of including the wait-for-lock time there should be a timeout > handling added to the lock code so it only tries to get the lock for not > longer than the timeout... So my code is broken eighter way... ;-) That's a different issue. But I don't think we really need to handle this case. You should never wait long before you get access to the bus. If you do then there is a design problem which needs to be solved, and timing out if you don't get the bus lock in a given amount of time isn't going to solve it. If one really can't afford waiting for the bus then the i2c_transfer() function should return immediately (as it already does in the can't-sleep case.) > > This assumes that adap->timeout is expressed in jiffies. Which > > according to a comment in include/linux/i2c-dev.h, is the case, but > > this must be for historical reasons. This never made sense to me, as it > > means that the actual timeout value depends on the value of HZ. > > > > The key problem here is that the timeout value is exported to > > user-space and can be changed by user-space. And user-space normally > > doesn't know the value of HZ... So the timeout value should be > > expressed in a constant time unit. > > I haven't had a look at the code but would it be so hard to convert the > timeout value to ms or something alike when exporting the value to > user-space and convert in the other direction on reading from user-space? You are perfectly right. I came out to the same conclusion as I woke up this morning. Sleep over it... > imo jiffies is the best unit for time within the kernel and I would prefer > having one place for the convertion instead of duplicating it in every > single driver and multiple places in i2c-core.. Well you have to do the conversion in each driver either way, as hard-coding a number of jiffies is a bad idea (makes the timeout depend on the value of HZ). But I agree it is better to convert from ms to jiffies once at initialization rather than each time we need to use the timeout value. I'll send a patch to the list now, addressing the issue in i2c-dev. Your own work is unaffected by this. -- Jean Delvare -- 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