All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] power: supply: axp20x_usb_power: Init work before enabling IRQs
@ 2021-01-24 15:24 Samuel Holland
  2021-01-25  3:41 ` Chen-Yu Tsai
  2021-01-28  0:36 ` Sebastian Reichel
  0 siblings, 2 replies; 3+ messages in thread
From: Samuel Holland @ 2021-01-24 15:24 UTC (permalink / raw)
  To: Sebastian Reichel, Chen-Yu Tsai; +Cc: linux-pm, linux-kernel, Samuel Holland

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@sholland.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);
 
 	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


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] power: supply: axp20x_usb_power: Init work before enabling IRQs
  2021-01-24 15:24 [PATCH] power: supply: axp20x_usb_power: Init work before enabling IRQs Samuel Holland
@ 2021-01-25  3:41 ` Chen-Yu Tsai
  2021-01-28  0:36 ` Sebastian Reichel
  1 sibling, 0 replies; 3+ messages in thread
From: Chen-Yu Tsai @ 2021-01-25  3:41 UTC (permalink / raw)
  To: Samuel Holland; +Cc: Sebastian Reichel, open list:THERMAL, linux-kernel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] power: supply: axp20x_usb_power: Init work before enabling IRQs
  2021-01-24 15:24 [PATCH] power: supply: axp20x_usb_power: Init work before enabling IRQs Samuel Holland
  2021-01-25  3:41 ` Chen-Yu Tsai
@ 2021-01-28  0:36 ` Sebastian Reichel
  1 sibling, 0 replies; 3+ messages in thread
From: Sebastian Reichel @ 2021-01-28  0:36 UTC (permalink / raw)
  To: Samuel Holland; +Cc: Chen-Yu Tsai, linux-pm, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]

Hi,

On Sun, Jan 24, 2021 at 09:24:21AM -0600, Samuel Holland 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@sholland.org>
> ---

Thanks, queued.

-- Sebastian

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-28  0:38 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-24 15:24 [PATCH] power: supply: axp20x_usb_power: Init work before enabling IRQs Samuel Holland
2021-01-25  3:41 ` Chen-Yu Tsai
2021-01-28  0:36 ` Sebastian Reichel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.