| Message ID | 20221127180233.103678-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | power: supply: bq25890: Fixes for 6.2 + further work for 6.3 | expand |
On 11/27/22 19:02, Hans de Goede wrote: > There are 2 races surrounding the usb-notifier: > > 1. The notifier, and thus usb_work, may run before the bq->charger > power_supply class device is registered. But usb_work may call > power_supply_changed() which relies on the psy device being registered. > > 2. usb_work may be pending/running at remove() time, so it needs to be > cancelled on remove after unregistering the usb-notifier. > > Fix 1. by moving usb-notifier registration to after the power_supply > registration. > > Fix 2. by adding a cancel_work_sync() call directly after the usb-notifier > unregistration. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/power/supply/bq25890_charger.c | 26 +++++++++++--------------- > 1 file changed, 11 insertions(+), 15 deletions(-) > > diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c > index 30d77afab839..032a10a3877b 100644 > --- a/drivers/power/supply/bq25890_charger.c > +++ b/drivers/power/supply/bq25890_charger.c > @@ -1387,16 +1387,10 @@ static int bq25890_probe(struct i2c_client *client) > if (ret) > return ret; > > - if (!IS_ERR_OR_NULL(bq->usb_phy)) { > - INIT_WORK(&bq->usb_work, bq25890_usb_work); > - bq->usb_nb.notifier_call = bq25890_usb_notifier; > - usb_register_notifier(bq->usb_phy, &bq->usb_nb); > - } > - > ret = bq25890_power_supply_init(bq); > if (ret < 0) { > dev_err(dev, "Failed to register power supply\n"); > - goto err_unregister_usb_notifier; > + return ret; You can even use 'return dev_err_probe()' above ^ to simplify that piece of code now. Reviewed-by: Marek Vasut <marex@denx.de>
diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c index 30d77afab839..032a10a3877b 100644 --- a/drivers/power/supply/bq25890_charger.c +++ b/drivers/power/supply/bq25890_charger.c @@ -1387,16 +1387,10 @@ static int bq25890_probe(struct i2c_client *client) if (ret) return ret; - if (!IS_ERR_OR_NULL(bq->usb_phy)) { - INIT_WORK(&bq->usb_work, bq25890_usb_work); - bq->usb_nb.notifier_call = bq25890_usb_notifier; - usb_register_notifier(bq->usb_phy, &bq->usb_nb); - } - ret = bq25890_power_supply_init(bq); if (ret < 0) { dev_err(dev, "Failed to register power supply\n"); - goto err_unregister_usb_notifier; + return ret; } ret = devm_request_threaded_irq(dev, client->irq, NULL, @@ -1404,23 +1398,25 @@ static int bq25890_probe(struct i2c_client *client) IRQF_TRIGGER_FALLING | IRQF_ONESHOT, BQ25890_IRQ_PIN, bq); if (ret) - goto err_unregister_usb_notifier; - - return 0; + return ret; -err_unregister_usb_notifier: - if (!IS_ERR_OR_NULL(bq->usb_phy)) - usb_unregister_notifier(bq->usb_phy, &bq->usb_nb); + if (!IS_ERR_OR_NULL(bq->usb_phy)) { + INIT_WORK(&bq->usb_work, bq25890_usb_work); + bq->usb_nb.notifier_call = bq25890_usb_notifier; + usb_register_notifier(bq->usb_phy, &bq->usb_nb); + } - return ret; + return 0; } static void bq25890_remove(struct i2c_client *client) { struct bq25890_device *bq = i2c_get_clientdata(client); - if (!IS_ERR_OR_NULL(bq->usb_phy)) + if (!IS_ERR_OR_NULL(bq->usb_phy)) { usb_unregister_notifier(bq->usb_phy, &bq->usb_nb); + cancel_work_sync(&bq->usb_work); + } if (!bq->skip_reset) { /* reset all registers to default values */
There are 2 races surrounding the usb-notifier: 1. The notifier, and thus usb_work, may run before the bq->charger power_supply class device is registered. But usb_work may call power_supply_changed() which relies on the psy device being registered. 2. usb_work may be pending/running at remove() time, so it needs to be cancelled on remove after unregistering the usb-notifier. Fix 1. by moving usb-notifier registration to after the power_supply registration. Fix 2. by adding a cancel_work_sync() call directly after the usb-notifier unregistration. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/power/supply/bq25890_charger.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)