Re: [PATCH 02/10] power: supply: bq25890: Ensure pump_express_work is cancelled on remove

Hans de Goede
2022-11-28T07:21:31Z

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
>>