| Message ID | 20221127180233.103678-3-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: > The pump_express_work which gets queued from an external_power_changed > callback might be pending / running on remove() (or on probe failure). > > Add a devm action cancelling the work, to ensure that it is cancelled. > > Note the devm action is added before devm_power_supply_register(), making > it run after devm unregisters the power_supply, so that the work cannot > be queued anymore (this is also why a devm action is used for this). > > Fixes: 48f45b094dbb ("power: supply: bq25890: Support higher charging voltages through Pump Express+ protocol") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> A comment in the code matching the last paragraph of this commit message would be helpful I think. Reviewed-by: Marek Vasut <marex@denx.de>
Hi, On Sun, Nov 27, 2022 at 07:02:25PM +0100, Hans de Goede wrote: > The pump_express_work which gets queued from an external_power_changed > callback might be pending / running on remove() (or on probe failure). > > Add a devm action cancelling the work, to ensure that it is cancelled. > > Note the devm action is added before devm_power_supply_register(), making > it run after devm unregisters the power_supply, so that the work cannot > be queued anymore (this is also why a devm action is used for this). > > Fixes: 48f45b094dbb ("power: supply: bq25890: Support higher charging voltages through Pump Express+ protocol") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/power/supply/bq25890_charger.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c > index 512c81662eea..30d77afab839 100644 > --- a/drivers/power/supply/bq25890_charger.c > +++ b/drivers/power/supply/bq25890_charger.c > @@ -1317,6 +1317,13 @@ static int bq25890_fw_probe(struct bq25890_device *bq) > return 0; > } > > +static void bq25890_non_devm_cleanup(void *data) > +{ > + struct bq25890_device *bq = data; > + > + cancel_delayed_work_sync(&bq->pump_express_work); > +} > + > static int bq25890_probe(struct i2c_client *client) > { > struct device *dev = &client->dev; > @@ -1372,6 +1379,10 @@ static int bq25890_probe(struct i2c_client *client) > /* OTG reporting */ > bq->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2); > > + ret = devm_add_action_or_reset(dev, bq25890_non_devm_cleanup, bq); > + if (ret) > + return ret; ret = devm_delayed_work_autocancel(dev, &bq->pump_express_work, bq25890_pump_express_work); if (ret) return ret; (+ removing the INIT_DELAYED_WORK) -- Sebastian > + > ret = bq25890_register_regulator(bq); > if (ret) > return ret; > -- > 2.38.1 >
Hi Sebastian, On 11/28/22 00:17, Sebastian Reichel wrote: > Hi, > > On Sun, Nov 27, 2022 at 07:02:25PM +0100, Hans de Goede wrote: >> The pump_express_work which gets queued from an external_power_changed >> callback might be pending / running on remove() (or on probe failure). >> >> Add a devm action cancelling the work, to ensure that it is cancelled. >> >> Note the devm action is added before devm_power_supply_register(), making >> it run after devm unregisters the power_supply, so that the work cannot >> be queued anymore (this is also why a devm action is used for this). >> >> Fixes: 48f45b094dbb ("power: supply: bq25890: Support higher charging voltages through Pump Express+ protocol") >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/power/supply/bq25890_charger.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c >> index 512c81662eea..30d77afab839 100644 >> --- a/drivers/power/supply/bq25890_charger.c >> +++ b/drivers/power/supply/bq25890_charger.c >> @@ -1317,6 +1317,13 @@ static int bq25890_fw_probe(struct bq25890_device *bq) >> return 0; >> } >> >> +static void bq25890_non_devm_cleanup(void *data) >> +{ >> + struct bq25890_device *bq = data; >> + >> + cancel_delayed_work_sync(&bq->pump_express_work); >> +} >> + >> static int bq25890_probe(struct i2c_client *client) >> { >> struct device *dev = &client->dev; >> @@ -1372,6 +1379,10 @@ static int bq25890_probe(struct i2c_client *client) >> /* OTG reporting */ >> bq->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2); >> >> + ret = devm_add_action_or_reset(dev, bq25890_non_devm_cleanup, bq); >> + if (ret) >> + return ret; > > ret = devm_delayed_work_autocancel(dev, &bq->pump_express_work, bq25890_pump_express_work); > if (ret) > return ret; > > (+ removing the INIT_DELAYED_WORK) That is a good point, but patch 8/10 builds on top of this and uses bq25890_non_devm_cleanup() to release the idr id needed when using multiple chargers on one board. And like cancelling the work this too can only be done after unregistering the psy device, and since the psy device is unregistered using devm this means the idr_remove() needs to use a devm callback too (to get the ordering right). I do plan to prepare a v2 for patches 2-10 addressing Marek's remarks this morning but given the need to have a devm action anyways (devm_delayed_work_autocancel() is just a convenience wrapper around devm_add_action) I plan to keep this as is. Otherwise we end up with 2 devm actions without any need for having 2 of them. Regards, Hans > > -- Sebastian > >> + >> ret = bq25890_register_regulator(bq); >> if (ret) >> return ret; >> -- >> 2.38.1 >>
diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c index 512c81662eea..30d77afab839 100644 --- a/drivers/power/supply/bq25890_charger.c +++ b/drivers/power/supply/bq25890_charger.c @@ -1317,6 +1317,13 @@ static int bq25890_fw_probe(struct bq25890_device *bq) return 0; } +static void bq25890_non_devm_cleanup(void *data) +{ + struct bq25890_device *bq = data; + + cancel_delayed_work_sync(&bq->pump_express_work); +} + static int bq25890_probe(struct i2c_client *client) { struct device *dev = &client->dev; @@ -1372,6 +1379,10 @@ static int bq25890_probe(struct i2c_client *client) /* OTG reporting */ bq->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2); + ret = devm_add_action_or_reset(dev, bq25890_non_devm_cleanup, bq); + if (ret) + return ret; + ret = bq25890_register_regulator(bq); if (ret) return ret;
The pump_express_work which gets queued from an external_power_changed callback might be pending / running on remove() (or on probe failure). Add a devm action cancelling the work, to ensure that it is cancelled. Note the devm action is added before devm_power_supply_register(), making it run after devm unregisters the power_supply, so that the work cannot be queued anymore (this is also why a devm action is used for this). Fixes: 48f45b094dbb ("power: supply: bq25890: Support higher charging voltages through Pump Express+ protocol") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/power/supply/bq25890_charger.c | 11 +++++++++++ 1 file changed, 11 insertions(+)