| Message ID | 1360179649-22465-38-git-send-email-tj@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Feb 06, 2013 at 11:40:09AM -0800, Tejun Heo wrote: > Convert to the much saner new idr interface. > > Only compile tested. This broke I2C for me in -next today, I saw a spinlock bad magic error calling pm_runtime_enable(). -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Thu, Feb 07, 2013 at 03:28:31PM +0000, Mark Brown wrote: > On Wed, Feb 06, 2013 at 11:40:09AM -0800, Tejun Heo wrote: > > Convert to the much saner new idr interface. > > > > Only compile tested. > > This broke I2C for me in -next today, I saw a spinlock bad magic error > calling pm_runtime_enable(). Hmmm... weird, can't see where the difference in behavior would come from. The only material difference would be if id < 0 && id != -1 in i2c_add_numbered_adapter(), which now would trigger WARN_ON_ONCE() inside idr_alloc() instead of silently returning -EINVAL. Can you please elaborate the failure? I can't see how the idr conversion would lead to spinlock bad magic error. Does reverting this patch make the problem go away? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 07, 2013 at 08:32:47AM -0800, Tejun Heo wrote: > On Thu, Feb 07, 2013 at 03:28:31PM +0000, Mark Brown wrote: > > On Wed, Feb 06, 2013 at 11:40:09AM -0800, Tejun Heo wrote: > > > Convert to the much saner new idr interface. > > > Only compile tested. > > This broke I2C for me in -next today, I saw a spinlock bad magic error > > calling pm_runtime_enable(). > Hmmm... weird, can't see where the difference in behavior would come > from. The only material difference would be if id < 0 && id != -1 in > i2c_add_numbered_adapter(), which now would trigger WARN_ON_ONCE() > inside idr_alloc() instead of silently returning -EINVAL. > Can you please elaborate the failure? I can't see how the idr > conversion would lead to spinlock bad magic error. Does reverting > this patch make the problem go away? Yes, reverting the patch made the issue vanish. I've no more diagnostics I'm afraid, just a 10s timeout then bad magic - it looks like memory corruption. I'll try to find time to dig in more but not sure when that'll happen.
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 66a30f7..795c916 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -935,25 +935,16 @@ out_list: */ int i2c_add_adapter(struct i2c_adapter *adapter) { - int id, res = 0; - -retry: - if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0) - return -ENOMEM; + int res; mutex_lock(&core_lock); - /* "above" here means "above or equal to", sigh */ - res = idr_get_new_above(&i2c_adapter_idr, adapter, - __i2c_first_dynamic_bus_num, &id); + res = idr_alloc(&i2c_adapter_idr, adapter, + __i2c_first_dynamic_bus_num, 0, GFP_KERNEL); mutex_unlock(&core_lock); - - if (res < 0) { - if (res == -EAGAIN) - goto retry; + if (res < 0) return res; - } - adapter->nr = id; + adapter->nr = res; return i2c_register_adapter(adapter); } EXPORT_SYMBOL(i2c_add_adapter); @@ -984,33 +975,17 @@ EXPORT_SYMBOL(i2c_add_adapter); int i2c_add_numbered_adapter(struct i2c_adapter *adap) { int id; - int status; if (adap->nr == -1) /* -1 means dynamically assign bus id */ return i2c_add_adapter(adap); - if (adap->nr & ~MAX_IDR_MASK) - return -EINVAL; - -retry: - if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0) - return -ENOMEM; mutex_lock(&core_lock); - /* "above" here means "above or equal to", sigh; - * we need the "equal to" result to force the result - */ - status = idr_get_new_above(&i2c_adapter_idr, adap, adap->nr, &id); - if (status == 0 && id != adap->nr) { - status = -EBUSY; - idr_remove(&i2c_adapter_idr, id); - } + id = idr_alloc(&i2c_adapter_idr, adap, adap->nr, adap->nr + 1, + GFP_KERNEL); mutex_unlock(&core_lock); - if (status == -EAGAIN) - goto retry; - - if (status == 0) - status = i2c_register_adapter(adap); - return status; + if (id < 0) + return id == -ENOSPC ? -EBUSY : id; + return 0; } EXPORT_SYMBOL_GPL(i2c_add_numbered_adapter);
Convert to the much saner new idr interface. Only compile tested. Signed-off-by: Tejun Heo <tj@kernel.org> Cc: Jean Delvare <khali@linux-fr.org> Cc: linux-i2c@vger.kernel.org --- drivers/i2c/i2c-core.c | 45 ++++++++++----------------------------------- 1 file changed, 10 insertions(+), 35 deletions(-)