On 25/05/16 16:46, Thierry Reding wrote: ... > How about this, which should be the most minimal to fix it (though it's > completely untested) and still update the internal cache (it just won't > signal an supply change, which wouldn't work at this point anyway). The > patch makes up for the supply change notification by doing that instead > of a full bq27xxx_battery_update() at the end of ->probe(). This should > take care of always sending out a uevent on successful probe, whereas a > bq27xxx_battery_update() at the end of ->probe() may not send one if it > is presented with the same data. > > Thierry > --- >8 --- > diff --git a/drivers/power/bq27xxx_battery.c b/drivers/power/bq27xxx_battery.c > index 45f6ebf88df6..df1b4cb2bbc2 100644 > --- a/drivers/power/bq27xxx_battery.c > +++ b/drivers/power/bq27xxx_battery.c > @@ -717,7 +717,13 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di) > di->charge_design_full = bq27xxx_battery_read_dcap(di); > } > > - if (di->cache.capacity != cache.capacity) > + /* > + * This function ends up being called while the power supply is being > + * registered, hence di->bat will be NULL on the first call, causing > + * power_supply_changed() to oops. Avoid that by checking if we have > + * been registered already or not. > + */ > + if (di->bat && di->cache.capacity != cache.capacity) > power_supply_changed(di->bat); > > if (memcmp(&di->cache, &cache, sizeof(cache)) != 0) > @@ -984,7 +990,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di) > > dev_info(di->dev, "support ver. %s enabled\n", DRIVER_VERSION); > > - bq27xxx_battery_update(di); > + power_supply_changed(di->bat); > > return 0; > } I think that would work too, my only concern is that this assumes that bq27xxx_battery_update() is called during the registration of the power supply. Looking at the backtrace from the panic we have ... [ 1.984150] [<ffff000008614984>] bq27xxx_battery_update+0x88/0x51c [ 1.990321] [<ffff000008615084>] bq27xxx_battery_poll+0x24/0x70 [ 1.996231] [<ffff000008615180>] bq27xxx_battery_get_property+0xb0/0x3b4 [ 2.002923] [<ffff0000086133d8>] power_supply_read_temp+0x2c/0x54 [ 2.009005] [<ffff000008616508>] thermal_zone_get_temp+0x5c/0x11c [ 2.015089] [<ffff0000086183b0>] thermal_zone_device_update+0x34/0xb4 [ 2.021518] [<ffff0000086193b4>] thermal_zone_device_register+0x87c/0x8cc [ 2.028295] [<ffff000008613b6c>] __power_supply_register+0x370/0x430 [ 2.034638] [<ffff000008613c54>] power_supply_register_no_ws+0x10/0x18 [ 2.041155] [<ffff000008614f1c>] bq27xxx_battery_setup+0x104/0x15c [ 2.047325] [<ffff000008615668>] bq27xxx_battery_i2c_probe+0xd0/0x1b0 Here bq27xxx_battery_update() is being called during the thermal zone registration and so as long as all bq27xxx devices have a POWER_SUPPLY_PROP_TEMP property then it *should* be ok. It would only break if there was a new bq27xxx with no temp support. May be that is a bit fragile and we are better off explicitly calling bq27xxx_battery_update()? Cheers Jon -- nvpublic -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
