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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Jan 2021 11:41:22 +0800
From:   Chen-Yu Tsai <wens@...e.org>
To:     Samuel Holland <samuel@...lland.org>
Cc:     Sebastian Reichel <sre@...nel.org>,
        "open list:THERMAL" <linux-pm@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] power: supply: axp20x_usb_power: Init work before
 enabling IRQs

On Sun, Jan 24, 2021 at 11:24 PM Samuel Holland <samuel@...lland.org> 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@...lland.org>
> ---
>  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
>