Re: [PATCH 9/9] I2C: mv64xxx: fix race between FSM/interrupt and process context
On Fri, May 17, 2013 at 11:00:16AM +0100, Russell King - ARM Linux wrote: > On Fri, May 17, 2013 at 11:51:51AM +0200, Wolfram Sang wrote: > > > { > > > switch(drv_data->action) { > > > case MV64XXX_I2C_ACTION_SEND_RESTART: > > > + /* We should only get here if we have further messages */ > > > + BUG_ON(drv_data->num_msgs == 0); > > > + > > > > ... > > > > > @@ -453,16 +463,20 @@ static int > > > mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int > > > num) > > > { > > > struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap); > > > - int i, rc; > > > + int rc, ret = num; > > > > > > - for (i = 0; i < num; i++) { > > > - rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[i], > > > - i == 0, i + 1 == num); > > > - if (rc < 0) > > > - return rc; > > > - } > > > + BUG_ON(drv_data->msgs != NULL); > > > > Can't we handle this more gracefully than to halt the kernel? Like > > WARN_ON and resetting the controller or disabling the bus or... > > Well, the latter really is something which should never ever happen, > and if it does happen it can only really be because something's > screwed up in the locking in the I2C layer. I'd think we should trust the layer here. > The former is more probable, and I thought about that, but I don't > have any good alternative solution. Given the problems I've seen, > I don't think resetting the controller is really an option, because > that'll likely cause the bus to lock (that's the original problem > which I'm trying to solve in this patch.) The thing really does > have to work according to the I2C protocol otherwise stuff goes > irrecoverably wrong to the point of needing an entire system reset. Fine with me for now. If somebody later has a setup where I2C slaves can be reset (e.g. via GPIO), so a complete bus reset is possible, we might need another solution, then. Thanks, Wolfram -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 9/9] I2C: mv64xxx: fix race between FSM/interrupt and process context
On Fri, May 17, 2013 at 11:51:51AM +0200, Wolfram Sang wrote: > > { > > switch(drv_data->action) { > > case MV64XXX_I2C_ACTION_SEND_RESTART: > > + /* We should only get here if we have further messages */ > > + BUG_ON(drv_data->num_msgs == 0); > > + > > ... > > > @@ -453,16 +463,20 @@ static int > > mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > > { > > struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap); > > - int i, rc; > > + int rc, ret = num; > > > > - for (i = 0; i < num; i++) { > > - rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[i], > > - i == 0, i + 1 == num); > > - if (rc < 0) > > - return rc; > > - } > > + BUG_ON(drv_data->msgs != NULL); > > Can't we handle this more gracefully than to halt the kernel? Like > WARN_ON and resetting the controller or disabling the bus or... Well, the latter really is something which should never ever happen, and if it does happen it can only really be because something's screwed up in the locking in the I2C layer. The former is more probable, and I thought about that, but I don't have any good alternative solution. Given the problems I've seen, I don't think resetting the controller is really an option, because that'll likely cause the bus to lock (that's the original problem which I'm trying to solve in this patch.) The thing really does have to work according to the I2C protocol otherwise stuff goes irrecoverably wrong to the point of needing an entire system reset. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 9/9] I2C: mv64xxx: fix race between FSM/interrupt and process context
> @@ -271,12 +273,25 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) > { > switch(drv_data->action) { > case MV64XXX_I2C_ACTION_SEND_RESTART: > + /* We should only get here if we have further messages */ > + BUG_ON(drv_data->num_msgs == 0); > + ... > @@ -453,16 +463,20 @@ static int > mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > { > struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap); > - int i, rc; > + int rc, ret = num; > > - for (i = 0; i < num; i++) { > - rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[i], > - i == 0, i + 1 == num); > - if (rc < 0) > - return rc; > - } > + BUG_ON(drv_data->msgs != NULL); Can't we handle this more gracefully than to halt the kernel? Like WARN_ON and resetting the controller or disabling the bus or... -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html