Re: [PATCH] power: supply: axp20x_usb_power: Init work before enabling IRQs

[Chen-Yu Tsai]

On Sun, Jan 24, 2021 at 11:24 PM Samuel Holland <samuel@xxxxxxxxxxxx> wrote:
>
> The IRQ handler calls mod_delayed_work() on power->vbus_detect. However,
> that work item is not initialized until after the IRQs are enabled. If
> an IRQ is already pending when the driver is probed, the driver calls
> mod_delayed_work() on an uninitialized work item, which causes an oops.
>
> Fixes: bcfb7ae3f50b ("power: supply: axp20x_usb_power: Only poll while offline")
> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
> ---
>  drivers/power/supply/axp20x_usb_power.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> index 20817a49110b..02aba3da271a 100644
> --- a/drivers/power/supply/axp20x_usb_power.c
> +++ b/drivers/power/supply/axp20x_usb_power.c
> @@ -711,20 +711,21 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>                              struct_size(power, irqs, axp_data->num_irq_names),
>                              GFP_KERNEL);
>         if (!power)
>                 return -ENOMEM;
>
>         platform_set_drvdata(pdev, power);
>
>         power->axp20x_id = axp_data->axp20x_id;
>         power->regmap = axp20x->regmap;
>         power->num_irqs = axp_data->num_irq_names;
> +       INIT_DELAYED_WORK(&power->vbus_detect, axp20x_usb_power_poll_vbus);

Nit:
Since axp20x_usb_power_poll_vbus() calls power_supply_changed() on
power->supply, it would make more sense to have INIT_DELAYED_WORK()
after devm_power_supply_register(), just so things are ordered correctly.
In practice this has no effect on the end result, since by the time the
interrupts are enabled the power supply would have been registered.

ChenYu

>         if (power->axp20x_id == AXP202_ID) {
>                 /* Enable vbus valid checking */
>                 ret = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
>                                          AXP20X_VBUS_MON_VBUS_VALID,
>                                          AXP20X_VBUS_MON_VBUS_VALID);
>                 if (ret)
>                         return ret;
>
>                 if (IS_ENABLED(CONFIG_AXP20X_ADC))
> @@ -763,21 +764,20 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>                 ret = devm_request_any_context_irq(&pdev->dev, power->irqs[i],
>                                                    axp20x_usb_power_irq, 0,
>                                                    DRVNAME, power);
>                 if (ret < 0) {
>                         dev_err(&pdev->dev, "Error requesting %s IRQ: %d\n",
>                                 axp_data->irq_names[i], ret);
>                         return ret;
>                 }
>         }
>
> -       INIT_DELAYED_WORK(&power->vbus_detect, axp20x_usb_power_poll_vbus);
>         if (axp20x_usb_vbus_needs_polling(power))
>                 queue_delayed_work(system_power_efficient_wq, &power->vbus_detect, 0);
>
>         return 0;
>  }
>
>  static int axp20x_usb_power_remove(struct platform_device *pdev)
>  {
>         struct axp20x_usb_power *power = platform_get_drvdata(pdev);
>
> --
> 2.26.2
>