[PATCH v5 0/3] More fixes for twl4030 charger

[PATCH v5 0/3] More fixes for twl4030 charger

[H. Nikolaus Schaller]

Changes V5:
* reworked max_current removal patch (comments by Sebastian Reichel)
* resubmit missing madc connection for AC power detection
* resubmit patch for irq allocation and -EPROBE_DEFER
* rebased on 4.12-rc1

Changes V4:
* resent commit (original one did contain material not upstream)

2017-04-14 21:29:39: 2017-04-14 20:26:00: Changes V3:
* worked in comments by Sebsatian Reichel
* clarifications of some commit messages
* rebased on v4.11rc-6

It took a while (18 months) until we propose an updated patch for upstream...

2015-11-02 12:27:39: Changes V2:
* worked in comments by Nishanth Menon <nm@ti.com>
* added another patch which solves a probing/boot stall problem (irq allocation vs. -EPROBE_DEFER)

V1:
4.3-rc1 introduced a new charger driver for the twl4030. This patch set fixes some
issues.

While making twl4030 changes from 4.3 operable we have found some issues
during testing on GTA04 and OpenPandora.

H. Nikolaus Schaller (2):
  drivers:power:twl4030-charger: remove nonstandard max_current sysfs
    attribute
  drivers:power:twl4030-charger: add deferred probing for phy and iio

Marek Belisko (1):
  ARM: dts: twl4030: Add missing madc reference for bci subnode

 .../ABI/testing/sysfs-class-power-twl4030          |  17 ----
 arch/arm/boot/dts/twl4030.dtsi                     |   2 +
 drivers/power/supply/twl4030_charger.c             | 100 +++++----------------
 3 files changed, 24 insertions(+), 95 deletions(-)

-- 
2.12.2

[PATCH v5 1/3] drivers:power:twl4030-charger: remove nonstandard max_current sysfs attribute

[H. Nikolaus Schaller]

Since we now support the standard 'input_current_limit' property by

commit 3fb319c2cdcd ("power: supply: twl4030-charger: add writable INPUT_CURRENT_LIMIT property")

we can now remove the nonstandard 'max_current' sysfs attribute.

See Documentation/power/power_supply_class.txt line 125

Both are functionally equivalent. From ABI point of view it is just a rename
of the property.

This also removes the entry in Documentation/ABI/testing/sysfs-class-power-twl4030

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 .../ABI/testing/sysfs-class-power-twl4030          | 17 ------
 drivers/power/supply/twl4030_charger.c             | 63 ----------------------
 2 files changed, 80 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-power-twl4030 b/Documentation/ABI/testing/sysfs-class-power-twl4030
index be26af0f1895..b4fd32d210c5 100644
--- a/Documentation/ABI/testing/sysfs-class-power-twl4030
+++ b/Documentation/ABI/testing/sysfs-class-power-twl4030
@@ -1,20 +1,3 @@
-What: /sys/class/power_supply/twl4030_ac/max_current
-      /sys/class/power_supply/twl4030_usb/max_current
-Description:
-	Read/Write limit on current which may
-	be drawn from the ac (Accessory Charger) or
-	USB port.
-
-	Value is in micro-Amps.
-
-	Value is set automatically to an appropriate
-	value when a cable is plugged or unplugged.
-
-	Value can the set by writing to the attribute.
-	The change will only persist until the next
-	plug event.  These event are reported via udev.
-
-
 What: /sys/class/power_supply/twl4030_usb/mode
 Description:
 	Changing mode for USB port.
diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
index 2f82d0e9ec1b..785a07bc4f39 100644
--- a/drivers/power/supply/twl4030_charger.c
+++ b/drivers/power/supply/twl4030_charger.c
@@ -624,63 +624,6 @@ static irqreturn_t twl4030_bci_interrupt(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
-/*
- * Provide "max_current" attribute in sysfs.
- */
-static ssize_t
-twl4030_bci_max_current_store(struct device *dev, struct device_attribute *attr,
-	const char *buf, size_t n)
-{
-	struct twl4030_bci *bci = dev_get_drvdata(dev->parent);
-	int cur = 0;
-	int status = 0;
-	status = kstrtoint(buf, 10, &cur);
-	if (status)
-		return status;
-	if (cur < 0)
-		return -EINVAL;
-	if (dev == &bci->ac->dev)
-		bci->ac_cur = cur;
-	else
-		bci->usb_cur_target = cur;
-
-	twl4030_charger_update_current(bci);
-	return n;
-}
-
-/*
- * sysfs max_current show
- */
-static ssize_t twl4030_bci_max_current_show(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	int status = 0;
-	int cur = -1;
-	u8 bcictl1;
-	struct twl4030_bci *bci = dev_get_drvdata(dev->parent);
-
-	if (dev == &bci->ac->dev) {
-		if (!bci->ac_is_active)
-			cur = bci->ac_cur;
-	} else {
-		if (bci->ac_is_active)
-			cur = bci->usb_cur_target;
-	}
-	if (cur < 0) {
-		cur = twl4030bci_read_adc_val(TWL4030_BCIIREF1);
-		if (cur < 0)
-			return cur;
-		status = twl4030_bci_read(TWL4030_BCICTL1, &bcictl1);
-		if (status < 0)
-			return status;
-		cur = regval2ua(cur, bcictl1 & TWL4030_CGAIN);
-	}
-	return scnprintf(buf, PAGE_SIZE, "%u\n", cur);
-}
-
-static DEVICE_ATTR(max_current, 0644, twl4030_bci_max_current_show,
-			twl4030_bci_max_current_store);
-
 static void twl4030_bci_usb_work(struct work_struct *data)
 {
 	struct twl4030_bci *bci = container_of(data, struct twl4030_bci, work);
@@ -1111,14 +1054,10 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 		dev_warn(&pdev->dev, "failed to unmask interrupts: %d\n", ret);
 
 	twl4030_charger_update_current(bci);
-	if (device_create_file(&bci->usb->dev, &dev_attr_max_current))
-		dev_warn(&pdev->dev, "could not create sysfs file\n");
 	if (device_create_file(&bci->usb->dev, &dev_attr_mode))
 		dev_warn(&pdev->dev, "could not create sysfs file\n");
 	if (device_create_file(&bci->ac->dev, &dev_attr_mode))
 		dev_warn(&pdev->dev, "could not create sysfs file\n");
-	if (device_create_file(&bci->ac->dev, &dev_attr_max_current))
-		dev_warn(&pdev->dev, "could not create sysfs file\n");
 
 	twl4030_charger_enable_ac(bci, true);
 	if (!IS_ERR_OR_NULL(bci->transceiver))
@@ -1150,9 +1089,7 @@ static int twl4030_bci_remove(struct platform_device *pdev)
 
 	iio_channel_release(bci->channel_vac);
 
-	device_remove_file(&bci->usb->dev, &dev_attr_max_current);
 	device_remove_file(&bci->usb->dev, &dev_attr_mode);
-	device_remove_file(&bci->ac->dev, &dev_attr_max_current);
 	device_remove_file(&bci->ac->dev, &dev_attr_mode);
 	/* mask interrupts */
 	twl_i2c_write_u8(TWL4030_MODULE_INTERRUPTS, 0xff,
-- 
2.12.2

Re: [PATCH v5 1/3] drivers:power:twl4030-charger: remove nonstandard max_current sysfs attribute

[Sebastian Reichel]

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

Hi,

On Sun, May 21, 2017 at 12:38:16PM +0200, H. Nikolaus Schaller wrote:
> Since we now support the standard 'input_current_limit' property by
> 
> commit 3fb319c2cdcd ("power: supply: twl4030-charger: add writable INPUT_CURRENT_LIMIT property")
> 
> we can now remove the nonstandard 'max_current' sysfs attribute.
> 
> See Documentation/power/power_supply_class.txt line 125
> 
> Both are functionally equivalent. From ABI point of view it is just a rename
> of the property.
> 
> This also removes the entry in Documentation/ABI/testing/sysfs-class-power-twl4030
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>

Thanks, queued.

-- Sebastian

> ---
>  .../ABI/testing/sysfs-class-power-twl4030          | 17 ------
>  drivers/power/supply/twl4030_charger.c             | 63 ----------------------
>  2 files changed, 80 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-power-twl4030 b/Documentation/ABI/testing/sysfs-class-power-twl4030
> index be26af0f1895..b4fd32d210c5 100644
> --- a/Documentation/ABI/testing/sysfs-class-power-twl4030
> +++ b/Documentation/ABI/testing/sysfs-class-power-twl4030
> @@ -1,20 +1,3 @@
> -What: /sys/class/power_supply/twl4030_ac/max_current
> -      /sys/class/power_supply/twl4030_usb/max_current
> -Description:
> -	Read/Write limit on current which may
> -	be drawn from the ac (Accessory Charger) or
> -	USB port.
> -
> -	Value is in micro-Amps.
> -
> -	Value is set automatically to an appropriate
> -	value when a cable is plugged or unplugged.
> -
> -	Value can the set by writing to the attribute.
> -	The change will only persist until the next
> -	plug event.  These event are reported via udev.
> -
> -
>  What: /sys/class/power_supply/twl4030_usb/mode
>  Description:
>  	Changing mode for USB port.
> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
> index 2f82d0e9ec1b..785a07bc4f39 100644
> --- a/drivers/power/supply/twl4030_charger.c
> +++ b/drivers/power/supply/twl4030_charger.c
> @@ -624,63 +624,6 @@ static irqreturn_t twl4030_bci_interrupt(int irq, void *arg)
>  	return IRQ_HANDLED;
>  }
>  
> -/*
> - * Provide "max_current" attribute in sysfs.
> - */
> -static ssize_t
> -twl4030_bci_max_current_store(struct device *dev, struct device_attribute *attr,
> -	const char *buf, size_t n)
> -{
> -	struct twl4030_bci *bci = dev_get_drvdata(dev->parent);
> -	int cur = 0;
> -	int status = 0;
> -	status = kstrtoint(buf, 10, &cur);
> -	if (status)
> -		return status;
> -	if (cur < 0)
> -		return -EINVAL;
> -	if (dev == &bci->ac->dev)
> -		bci->ac_cur = cur;
> -	else
> -		bci->usb_cur_target = cur;
> -
> -	twl4030_charger_update_current(bci);
> -	return n;
> -}
> -
> -/*
> - * sysfs max_current show
> - */
> -static ssize_t twl4030_bci_max_current_show(struct device *dev,
> -	struct device_attribute *attr, char *buf)
> -{
> -	int status = 0;
> -	int cur = -1;
> -	u8 bcictl1;
> -	struct twl4030_bci *bci = dev_get_drvdata(dev->parent);
> -
> -	if (dev == &bci->ac->dev) {
> -		if (!bci->ac_is_active)
> -			cur = bci->ac_cur;
> -	} else {
> -		if (bci->ac_is_active)
> -			cur = bci->usb_cur_target;
> -	}
> -	if (cur < 0) {
> -		cur = twl4030bci_read_adc_val(TWL4030_BCIIREF1);
> -		if (cur < 0)
> -			return cur;
> -		status = twl4030_bci_read(TWL4030_BCICTL1, &bcictl1);
> -		if (status < 0)
> -			return status;
> -		cur = regval2ua(cur, bcictl1 & TWL4030_CGAIN);
> -	}
> -	return scnprintf(buf, PAGE_SIZE, "%u\n", cur);
> -}
> -
> -static DEVICE_ATTR(max_current, 0644, twl4030_bci_max_current_show,
> -			twl4030_bci_max_current_store);
> -
>  static void twl4030_bci_usb_work(struct work_struct *data)
>  {
>  	struct twl4030_bci *bci = container_of(data, struct twl4030_bci, work);
> @@ -1111,14 +1054,10 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>  		dev_warn(&pdev->dev, "failed to unmask interrupts: %d\n", ret);
>  
>  	twl4030_charger_update_current(bci);
> -	if (device_create_file(&bci->usb->dev, &dev_attr_max_current))
> -		dev_warn(&pdev->dev, "could not create sysfs file\n");
>  	if (device_create_file(&bci->usb->dev, &dev_attr_mode))
>  		dev_warn(&pdev->dev, "could not create sysfs file\n");
>  	if (device_create_file(&bci->ac->dev, &dev_attr_mode))
>  		dev_warn(&pdev->dev, "could not create sysfs file\n");
> -	if (device_create_file(&bci->ac->dev, &dev_attr_max_current))
> -		dev_warn(&pdev->dev, "could not create sysfs file\n");
>  
>  	twl4030_charger_enable_ac(bci, true);
>  	if (!IS_ERR_OR_NULL(bci->transceiver))
> @@ -1150,9 +1089,7 @@ static int twl4030_bci_remove(struct platform_device *pdev)
>  
>  	iio_channel_release(bci->channel_vac);
>  
> -	device_remove_file(&bci->usb->dev, &dev_attr_max_current);
>  	device_remove_file(&bci->usb->dev, &dev_attr_mode);
> -	device_remove_file(&bci->ac->dev, &dev_attr_max_current);
>  	device_remove_file(&bci->ac->dev, &dev_attr_mode);
>  	/* mask interrupts */
>  	twl_i2c_write_u8(TWL4030_MODULE_INTERRUPTS, 0xff,
> -- 
> 2.12.2
> 

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

[PATCH v5 2/3] ARM: dts: twl4030: Add missing madc reference for bci subnode

[H. Nikolaus Schaller]

From: Marek Belisko <marek@goldelico.com>

The twl4030_charger driver expects an iio channel to detect the
presence of an AC charger by looking at VAC (madc channel 11).
This definition is missing in the device tree.

Signed-off-by: Marek Belisko <marek@goldelico.com>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 arch/arm/boot/dts/twl4030.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/twl4030.dtsi b/arch/arm/boot/dts/twl4030.dtsi
index 36ae9160b558..16533b62b0a2 100644
--- a/arch/arm/boot/dts/twl4030.dtsi
+++ b/arch/arm/boot/dts/twl4030.dtsi
@@ -23,6 +23,8 @@
 		compatible = "ti,twl4030-bci";
 		interrupts = <9>, <2>;
 		bci3v1-supply = <&vusb3v1>;
+		io-channels = <&twl_madc 11>;
+		io-channel-names = "vac";
 	};
 
 	watchdog {
-- 
2.12.2

Re: [PATCH v5 2/3] ARM: dts: twl4030: Add missing madc reference for bci subnode

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [170521 03:44]:
> From: Marek Belisko <marek@goldelico.com>
> 
> The twl4030_charger driver expects an iio channel to detect the
> presence of an AC charger by looking at VAC (madc channel 11).
> This definition is missing in the device tree.

I'm picking this patch into omap-for-v4.13/dt thanks.

Tony

Re: [PATCH v5 2/3] ARM: dts: twl4030: Add missing madc reference for bci subnode

[H. Nikolaus Schaller]

> Am 06.06.2017 um 09:23 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [170521 03:44]:
>> From: Marek Belisko <marek@goldelico.com>
>> 
>> The twl4030_charger driver expects an iio channel to detect the
>> presence of an AC charger by looking at VAC (madc channel 11).
>> This definition is missing in the device tree.
> 
> I'm picking this patch into omap-for-v4.13/dt thanks.

Thanks and BR!
Nikolaus

[PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio

[H. Nikolaus Schaller]

This fixes an issue if both this twl4030_charger driver and
phy-twl4030-usb are compiled as modules and loaded in random order.
It has been observed on GTA04 and OpenPandora devices that in worst
case the boot process hangs and in best case the AC detection fails
with a warning.

Therefore we add deferred probing checks for the usb_phy and the
iio channel for AC detection.

For implementing this we must reorder code because we can't safely
return -EPROBE_DEFER after allocating any devm managed interrupt
(it might already/still be enabled without working interrupt handler).

So the check for required resources that may abort probing by
returning -EPROBE_DEFER, must come first.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/power/supply/twl4030_charger.c | 37 ++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
index 785a07bc4f39..945eabdbbc89 100644
--- a/drivers/power/supply/twl4030_charger.c
+++ b/drivers/power/supply/twl4030_charger.c
@@ -984,6 +984,28 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, bci);
 
+	if (bci->dev->of_node) {
+		struct device_node *phynode;
+
+		phynode = of_find_compatible_node(bci->dev->of_node->parent,
+						  NULL, "ti,twl4030-usb");
+		if (phynode) {
+			bci->transceiver = devm_usb_get_phy_by_node(
+				bci->dev, phynode, &bci->usb_nb);
+			if (IS_ERR(bci->transceiver) &&
+			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;	/* PHY not ready */
+		}
+	}
+
+	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
+	if (IS_ERR(bci->channel_vac)) {
+		if (PTR_ERR(bci->channel_vac) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;	/* iio not ready */
+		dev_warn(&pdev->dev, "could not request vac iio channel");
+		bci->channel_vac = NULL;
+	}
+
 	bci->ac = devm_power_supply_register(&pdev->dev, &twl4030_bci_ac_desc,
 					     NULL);
 	if (IS_ERR(bci->ac)) {
@@ -1017,25 +1039,10 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
-	if (IS_ERR(bci->channel_vac)) {
-		bci->channel_vac = NULL;
-		dev_warn(&pdev->dev, "could not request vac iio channel");
-	}
-
 	INIT_WORK(&bci->work, twl4030_bci_usb_work);
 	INIT_DELAYED_WORK(&bci->current_worker, twl4030_current_worker);
 
 	bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
-	if (bci->dev->of_node) {
-		struct device_node *phynode;
-
-		phynode = of_find_compatible_node(bci->dev->of_node->parent,
-						  NULL, "ti,twl4030-usb");
-		if (phynode)
-			bci->transceiver = devm_usb_get_phy_by_node(
-				bci->dev, phynode, &bci->usb_nb);
-	}
 
 	/* Enable interrupts now. */
 	reg = ~(u32)(TWL4030_ICHGLOW | TWL4030_ICHGEOC | TWL4030_TBATOR2 |
-- 
2.12.2

Re: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio

[Sebastian Reichel]

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

Hi,

On Sun, May 21, 2017 at 12:38:18PM +0200, H. Nikolaus Schaller wrote:
> This fixes an issue if both this twl4030_charger driver and
> phy-twl4030-usb are compiled as modules and loaded in random order.
> It has been observed on GTA04 and OpenPandora devices that in worst
> case the boot process hangs and in best case the AC detection fails
> with a warning.
>
> Therefore we add deferred probing checks for the usb_phy and the
> iio channel for AC detection.
> 
> For implementing this we must reorder code because we can't safely
> return -EPROBE_DEFER after allocating any devm managed interrupt
> (it might already/still be enabled without working interrupt handler).
>
> So the check for required resources that may abort probing by
> returning -EPROBE_DEFER, must come first.

This sounds fishy. EPROBE_DEFER should not be different from
other error codes in this regard and devm_ requested resources
should be free'd on any error. Why is irq handler not working?

> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/power/supply/twl4030_charger.c | 37 ++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
> index 785a07bc4f39..945eabdbbc89 100644
> --- a/drivers/power/supply/twl4030_charger.c
> +++ b/drivers/power/supply/twl4030_charger.c
> @@ -984,6 +984,28 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, bci);
>  
> +	if (bci->dev->of_node) {
> +		struct device_node *phynode;
> +
> +		phynode = of_find_compatible_node(bci->dev->of_node->parent,
> +						  NULL, "ti,twl4030-usb");
> +		if (phynode) {
> +			bci->transceiver = devm_usb_get_phy_by_node(
> +				bci->dev, phynode, &bci->usb_nb);
> +			if (IS_ERR(bci->transceiver) &&
> +			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;	/* PHY not ready */
> +		}
> +	}
> +
> +	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
> +	if (IS_ERR(bci->channel_vac)) {
> +		if (PTR_ERR(bci->channel_vac) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;	/* iio not ready */
> +		dev_warn(&pdev->dev, "could not request vac iio channel");
> +		bci->channel_vac = NULL;
> +	}
> +

You should not request non-devm managed iio_channel before
devm-managed power-supply. This could be fixed by switching to
devm_iio_channel_get(), which also cleans up some code.

I suspect, that this is also your IRQ problem, since iio_channel
is currently free'd before irqs are free'd, but its used in irq
code.

>  	bci->ac = devm_power_supply_register(&pdev->dev, &twl4030_bci_ac_desc,
>  					     NULL);
>  	if (IS_ERR(bci->ac)) {
> @@ -1017,25 +1039,10 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
> -	if (IS_ERR(bci->channel_vac)) {
> -		bci->channel_vac = NULL;
> -		dev_warn(&pdev->dev, "could not request vac iio channel");
> -	}
> -
>  	INIT_WORK(&bci->work, twl4030_bci_usb_work);
>  	INIT_DELAYED_WORK(&bci->current_worker, twl4030_current_worker);
>  
>  	bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
> -	if (bci->dev->of_node) {
> -		struct device_node *phynode;
> -
> -		phynode = of_find_compatible_node(bci->dev->of_node->parent,
> -						  NULL, "ti,twl4030-usb");
> -		if (phynode)
> -			bci->transceiver = devm_usb_get_phy_by_node(
> -				bci->dev, phynode, &bci->usb_nb);
> -	}
>  
>  	/* Enable interrupts now. */
>  	reg = ~(u32)(TWL4030_ICHGLOW | TWL4030_ICHGEOC | TWL4030_TBATOR2 |

-- Sebastian

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

Re: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio

[H. Nikolaus Schaller]

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

Hi,

> Am 07.06.2017 um 22:44 schrieb Sebastian Reichel <sre@kernel.org>:
> 
> Hi,
> 
> On Sun, May 21, 2017 at 12:38:18PM +0200, H. Nikolaus Schaller wrote:
>> This fixes an issue if both this twl4030_charger driver and
>> phy-twl4030-usb are compiled as modules and loaded in random order.
>> It has been observed on GTA04 and OpenPandora devices that in worst
>> case the boot process hangs and in best case the AC detection fails
>> with a warning.
>> 
>> Therefore we add deferred probing checks for the usb_phy and the
>> iio channel for AC detection.
>> 
>> For implementing this we must reorder code because we can't safely
>> return -EPROBE_DEFER after allocating any devm managed interrupt
>> (it might already/still be enabled without working interrupt handler).
>> 
>> So the check for required resources that may abort probing by
>> returning -EPROBE_DEFER, must come first.
> 
> This sounds fishy. EPROBE_DEFER should not be different from
> other error codes in this regard and devm_ requested resources
> should be free'd on any error. Why is irq handler not working?

1. there is no other error code involved, before we try to convert the driver to handle EPROBE_DEFER.
So it is not that EPROBE_DEFER is special but that we add an error return path for it.

2. I don't know why it is not working - I just know that the handler seems to be triggered before
all resources are available (if probe() is aborted with EPROBE_DEFER error paths) which ends up in kernel panics.

Therefore just to be safe, I have reordered things a little (without changing the function):

1. check for resources (with some EPROBE_DEFER)
2. allocate non-devm (with optional EPROBE_DEFER)
3. allocate devm

So this should be safe in any case.

Please also compare a discussion

https://lkml.org/lkml/2013/2/22/65

> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/power/supply/twl4030_charger.c | 37 ++++++++++++++++++++--------------
>> 1 file changed, 22 insertions(+), 15 deletions(-)
>> 
>> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
>> index 785a07bc4f39..945eabdbbc89 100644
>> --- a/drivers/power/supply/twl4030_charger.c
>> +++ b/drivers/power/supply/twl4030_charger.c
>> @@ -984,6 +984,28 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>> 
>> 	platform_set_drvdata(pdev, bci);
>> 
>> +	if (bci->dev->of_node) {
>> +		struct device_node *phynode;
>> +
>> +		phynode = of_find_compatible_node(bci->dev->of_node->parent,
>> +						  NULL, "ti,twl4030-usb");
>> +		if (phynode) {
>> +			bci->transceiver = devm_usb_get_phy_by_node(
>> +				bci->dev, phynode, &bci->usb_nb);
>> +			if (IS_ERR(bci->transceiver) &&
>> +			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
>> +				return -EPROBE_DEFER;	/* PHY not ready */
>> +		}
>> +	}
>> +
>> +	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
>> +	if (IS_ERR(bci->channel_vac)) {
>> +		if (PTR_ERR(bci->channel_vac) == -EPROBE_DEFER)
>> +			return -EPROBE_DEFER;	/* iio not ready */
>> +		dev_warn(&pdev->dev, "could not request vac iio channel");
>> +		bci->channel_vac = NULL;
>> +	}
>> +
> 
> You should not request non-devm managed iio_channel before
> devm-managed power-supply.

Well, it is not *me* doing that.

It is the unpatched driver which already does. See:

	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/twl4030_charger.c?h=v4.12-rc4#n1069

I have just moved this line to a different location to be able to add a proper EPROBE_DEFER return path.

> This could be fixed by switching to
> devm_iio_channel_get(), which also cleans up some code.

Yes, this could be an alternative solution.
We still need EPROBE_DEFER handling.

> 
> I suspect, that this is also your IRQ problem, since iio_channel
> is currently free'd before irqs are free'd, but its used in irq
> code.

Hm. No.

In upstream code it is never freed on probe failure because there is only
an error return (goto fail) after allocating irqs in case the
twl_i2c_write_u8 fails. Which usually doesn't.

The EPROBE_DEFER returned by iio_channel_get not being ready simply prints
a warning and turns off AC detection (bci->channel_vac = NULL) forever.

> 
>> 	bci->ac = devm_power_supply_register(&pdev->dev, &twl4030_bci_ac_desc,
>> 					     NULL);
>> 	if (IS_ERR(bci->ac)) {
>> @@ -1017,25 +1039,10 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>> 		return ret;
>> 	}
>> 
>> -	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
>> -	if (IS_ERR(bci->channel_vac)) {

My first attempt to fix EPROBE_DEFER was to add this

+		if (PTR_ERR(bci->channel_vac) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;	/* iio not ready */

>> -		bci->channel_vac = NULL;
>> -		dev_warn(&pdev->dev, "could not request vac iio channel");
>> -	}
>> -
>> 	INIT_WORK(&bci->work, twl4030_bci_usb_work);
>> 	INIT_DELAYED_WORK(&bci->current_worker, twl4030_current_worker);
>> 
>> 	bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
>> -	if (bci->dev->of_node) {
>> -		struct device_node *phynode;
>> -
>> -		phynode = of_find_compatible_node(bci->dev->of_node->parent,
>> -						  NULL, "ti,twl4030-usb");
>> -		if (phynode)
>> -			bci->transceiver = devm_usb_get_phy_by_node(
>> -				bci->dev, phynode, &bci->usb_nb);

and this

+			if (IS_ERR(bci->transceiver) &&
+			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER) {
+				ret = -EPROBE_DEFER;	/* PHY not ready */
+				goto fail;

This did run (and potentially return) after installing devm_request_threaded_irq leading
to observation of severe kernel panics by interrupts.

Moving the iio channel allocation before devm_request_threaded_irq made them go away.

>> -	}
>> 
>> 	/* Enable interrupts now. */
>> 	reg = ~(u32)(TWL4030_ICHGLOW | TWL4030_ICHGEOC | TWL4030_TBATOR2 |
> 
> -- Sebastian

How important is it to keep the current sequence of devm_request_threaded_irq() + (devm_)iio_channel_get() untouched?

The reason I ask is that it is more likely for (devm_)iio_channel_get() to fail than devm_request_threaded_irq().

Hence the iio channel should imho be always allocated first, so that we skip allocating+freeing unused irq handlers in case
of iio not being ready.

Therefore, I suggest to keep the proposed reordering even if we think devm-conversion of iio_channel_get() alone
would solve it equally well.

Next: should there be two separate patches? One for converting the non-devm iio_channel_get() and cleaning up error path.
And one for adding EPROBE_DEFER error path.

Or keep it in a single patch because they only make sense if done together (you can't add/remove deferred probing
without converting and/or moving iio_channel_get())?

So please advise how to proceed.

BR and thanks,
Nikolaus


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio

[Grygorii Strashko]

On 06/09/2017 01:05 AM, H. Nikolaus Schaller wrote:
> Hi,
> 
>> Am 07.06.2017 um 22:44 schrieb Sebastian Reichel <sre@kernel.org>:
>>
>> Hi,
>>
>> On Sun, May 21, 2017 at 12:38:18PM +0200, H. Nikolaus Schaller wrote:
>>> This fixes an issue if both this twl4030_charger driver and
>>> phy-twl4030-usb are compiled as modules and loaded in random order.
>>> It has been observed on GTA04 and OpenPandora devices that in worst
>>> case the boot process hangs and in best case the AC detection fails
>>> with a warning.
>>>
>>> Therefore we add deferred probing checks for the usb_phy and the
>>> iio channel for AC detection.
>>>
>>> For implementing this we must reorder code because we can't safely
>>> return -EPROBE_DEFER after allocating any devm managed interrupt
>>> (it might already/still be enabled without working interrupt handler).
>>>
>>> So the check for required resources that may abort probing by
>>> returning -EPROBE_DEFER, must come first.
>>
>> This sounds fishy. EPROBE_DEFER should not be different from
>> other error codes in this regard and devm_ requested resources
>> should be free'd on any error. Why is irq handler not working?
> 
> 1. there is no other error code involved, before we try to convert the driver to handle EPROBE_DEFER.
> So it is not that EPROBE_DEFER is special but that we add an error return path for it.
> 
> 2. I don't know why it is not working - I just know that the handler seems to be triggered before
> all resources are available (if probe() is aborted with EPROBE_DEFER error paths) which ends up in kernel panics.
> 
> Therefore just to be safe, I have reordered things a little (without changing the function):
> 
> 1. check for resources (with some EPROBE_DEFER)
> 2. allocate non-devm (with optional EPROBE_DEFER)
> 3. allocate devm
> 
> So this should be safe in any case.
> 
> Please also compare a discussion
> 
> https://lkml.org/lkml/2013/2/22/65
> 
>>
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>> drivers/power/supply/twl4030_charger.c | 37 ++++++++++++++++++++--------------
>>> 1 file changed, 22 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
>>> index 785a07bc4f39..945eabdbbc89 100644
>>> --- a/drivers/power/supply/twl4030_charger.c
>>> +++ b/drivers/power/supply/twl4030_charger.c
>>> @@ -984,6 +984,28 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>>>
>>> 	platform_set_drvdata(pdev, bci);
>>>
>>> +	if (bci->dev->of_node) {
>>> +		struct device_node *phynode;
>>> +
>>> +		phynode = of_find_compatible_node(bci->dev->of_node->parent,
>>> +						  NULL, "ti,twl4030-usb");
>>> +		if (phynode) {
>>> +			bci->transceiver = devm_usb_get_phy_by_node(
>>> +				bci->dev, phynode, &bci->usb_nb);
>>> +			if (IS_ERR(bci->transceiver) &&
>>> +			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
>>> +				return -EPROBE_DEFER;	/* PHY not ready */
>>> +		}
>>> +	}
>>> +
>>> +	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
>>> +	if (IS_ERR(bci->channel_vac)) {
>>> +		if (PTR_ERR(bci->channel_vac) == -EPROBE_DEFER)
>>> +			return -EPROBE_DEFER;	/* iio not ready */
>>> +		dev_warn(&pdev->dev, "could not request vac iio channel");
>>> +		bci->channel_vac = NULL;
>>> +	}
>>> +
>>
>> You should not request non-devm managed iio_channel before
>> devm-managed power-supply.
> 
> Well, it is not *me* doing that.
> 
> It is the unpatched driver which already does. See:
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/twl4030_charger.c?h=v4.12-rc4#n1069
> 
> I have just moved this line to a different location to be able to add a proper EPROBE_DEFER return path.
> 
>> This could be fixed by switching to
>> devm_iio_channel_get(), which also cleans up some code.
> 
> Yes, this could be an alternative solution.
> We still need EPROBE_DEFER handling.
> 
>>
>> I suspect, that this is also your IRQ problem, since iio_channel
>> is currently free'd before irqs are free'd, but its used in irq
>> code.
> 
> Hm. No.
> 
> In upstream code it is never freed on probe failure because there is only
> an error return (goto fail) after allocating irqs in case the
> twl_i2c_write_u8 fails. Which usually doesn't.
> 
> The EPROBE_DEFER returned by iio_channel_get not being ready simply prints
> a warning and turns off AC detection (bci->channel_vac = NULL) forever.
> 
>>
>>> 	bci->ac = devm_power_supply_register(&pdev->dev, &twl4030_bci_ac_desc,
>>> 					     NULL);
>>> 	if (IS_ERR(bci->ac)) {
>>> @@ -1017,25 +1039,10 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>>> 		return ret;
>>> 	}
>>>
>>> -	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
>>> -	if (IS_ERR(bci->channel_vac)) {
> 
> My first attempt to fix EPROBE_DEFER was to add this
> 
> +		if (PTR_ERR(bci->channel_vac) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;	/* iio not ready */
> 
>>> -		bci->channel_vac = NULL;
>>> -		dev_warn(&pdev->dev, "could not request vac iio channel");
>>> -	}
>>> -
>>> 	INIT_WORK(&bci->work, twl4030_bci_usb_work);
>>> 	INIT_DELAYED_WORK(&bci->current_worker, twl4030_current_worker);
>>>
>>> 	bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
>>> -	if (bci->dev->of_node) {
>>> -		struct device_node *phynode;
>>> -
>>> -		phynode = of_find_compatible_node(bci->dev->of_node->parent,
>>> -						  NULL, "ti,twl4030-usb");
>>> -		if (phynode)
>>> -			bci->transceiver = devm_usb_get_phy_by_node(
>>> -				bci->dev, phynode, &bci->usb_nb);
> 
> and this
> 
> +			if (IS_ERR(bci->transceiver) &&
> +			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER) {
> +				ret = -EPROBE_DEFER;	/* PHY not ready */
> +				goto fail;
> 
> This did run (and potentially return) after installing devm_request_threaded_irq leading
> to observation of severe kernel panics by interrupts.
> 
> Moving the iio channel allocation before devm_request_threaded_irq made them go away.
> 
>>> -	}
>>>
>>> 	/* Enable interrupts now. */
>>> 	reg = ~(u32)(TWL4030_ICHGLOW | TWL4030_ICHGEOC | TWL4030_TBATOR2 |
>>
>> -- Sebastian
> 
> How important is it to keep the current sequence of devm_request_threaded_irq() + (devm_)iio_channel_get() untouched?
> 
> The reason I ask is that it is more likely for (devm_)iio_channel_get() to fail than devm_request_threaded_irq().
> 
> Hence the iio channel should imho be always allocated first, so that we skip allocating+freeing unused irq handlers in case
> of iio not being ready.
> 
> Therefore, I suggest to keep the proposed reordering even if we think devm-conversion of iio_channel_get() alone
> would solve it equally well.
> 
> Next: should there be two separate patches? One for converting the non-devm iio_channel_get() and cleaning up error path.
> And one for adding EPROBE_DEFER error path.
> 
> Or keep it in a single patch because they only make sense if done together (you can't add/remove deferred probing
> without converting and/or moving iio_channel_get())?
> 
> So please advise how to proceed.
> 

You should request irq as late as possible in probe - it's safest way to go always.
You see crushes simply because request_irq enables IRQ and IRQ can trigger immediately, so
IRQ handler will run and all required resources should be ready and initialized.

In this case:
twl4030_bci_interrupt() -> twl4030_charger_update_current() -> ac_available() -> iio_read_channel_processed()
OOOPS.

So, first thing to do is to reorder probe to ensure that sequence is safe.
Then other stuff - devm, EPROBE_DEFER ...


-- 
regards,
-grygorii

Re: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio

[H. Nikolaus Schaller]

Hi Grygorii,

> Am 09.06.2017 um 18:25 schrieb Grygorii Strashko <grygorii.strashko@ti.com>:
> 
> 
> 
> On 06/09/2017 01:05 AM, H. Nikolaus Schaller wrote:
>> Hi,
>> 
>>> Am 07.06.2017 um 22:44 schrieb Sebastian Reichel <sre@kernel.org>:
>>> 
>>> Hi,
>>> 
>>> On Sun, May 21, 2017 at 12:38:18PM +0200, H. Nikolaus Schaller wrote:
>>>> This fixes an issue if both this twl4030_charger driver and
>>>> phy-twl4030-usb are compiled as modules and loaded in random order.
>>>> It has been observed on GTA04 and OpenPandora devices that in worst
>>>> case the boot process hangs and in best case the AC detection fails
>>>> with a warning.
>>>> 
>>>> Therefore we add deferred probing checks for the usb_phy and the
>>>> iio channel for AC detection.
>>>> 
>>>> For implementing this we must reorder code because we can't safely
>>>> return -EPROBE_DEFER after allocating any devm managed interrupt
>>>> (it might already/still be enabled without working interrupt handler).
>>>> 
>>>> So the check for required resources that may abort probing by
>>>> returning -EPROBE_DEFER, must come first.
>>> 
>>> This sounds fishy. EPROBE_DEFER should not be different from
>>> other error codes in this regard and devm_ requested resources
>>> should be free'd on any error. Why is irq handler not working?
>> 
>> 1. there is no other error code involved, before we try to convert the driver to handle EPROBE_DEFER.
>> So it is not that EPROBE_DEFER is special but that we add an error return path for it.
>> 
>> 2. I don't know why it is not working - I just know that the handler seems to be triggered before
>> all resources are available (if probe() is aborted with EPROBE_DEFER error paths) which ends up in kernel panics.
>> 
>> Therefore just to be safe, I have reordered things a little (without changing the function):
>> 
>> 1. check for resources (with some EPROBE_DEFER)
>> 2. allocate non-devm (with optional EPROBE_DEFER)
>> 3. allocate devm
>> 
>> So this should be safe in any case.
>> 
>> Please also compare a discussion
>> 
>> https://lkml.org/lkml/2013/2/22/65
>> 
>>> 
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>> drivers/power/supply/twl4030_charger.c | 37 ++++++++++++++++++++--------------
>>>> 1 file changed, 22 insertions(+), 15 deletions(-)
>>>> 
>>>> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
>>>> index 785a07bc4f39..945eabdbbc89 100644
>>>> --- a/drivers/power/supply/twl4030_charger.c
>>>> +++ b/drivers/power/supply/twl4030_charger.c
>>>> @@ -984,6 +984,28 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>>>> 
>>>> 	platform_set_drvdata(pdev, bci);
>>>> 
>>>> +	if (bci->dev->of_node) {
>>>> +		struct device_node *phynode;
>>>> +
>>>> +		phynode = of_find_compatible_node(bci->dev->of_node->parent,
>>>> +						  NULL, "ti,twl4030-usb");
>>>> +		if (phynode) {
>>>> +			bci->transceiver = devm_usb_get_phy_by_node(
>>>> +				bci->dev, phynode, &bci->usb_nb);
>>>> +			if (IS_ERR(bci->transceiver) &&
>>>> +			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
>>>> +				return -EPROBE_DEFER;	/* PHY not ready */
>>>> +		}
>>>> +	}
>>>> +
>>>> +	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
>>>> +	if (IS_ERR(bci->channel_vac)) {
>>>> +		if (PTR_ERR(bci->channel_vac) == -EPROBE_DEFER)
>>>> +			return -EPROBE_DEFER;	/* iio not ready */
>>>> +		dev_warn(&pdev->dev, "could not request vac iio channel");
>>>> +		bci->channel_vac = NULL;
>>>> +	}
>>>> +
>>> 
>>> You should not request non-devm managed iio_channel before
>>> devm-managed power-supply.
>> 
>> Well, it is not *me* doing that.
>> 
>> It is the unpatched driver which already does. See:
>> 
>> 	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/twl4030_charger.c?h=v4.12-rc4#n1069
>> 
>> I have just moved this line to a different location to be able to add a proper EPROBE_DEFER return path.
>> 
>>> This could be fixed by switching to
>>> devm_iio_channel_get(), which also cleans up some code.
>> 
>> Yes, this could be an alternative solution.
>> We still need EPROBE_DEFER handling.
>> 
>>> 
>>> I suspect, that this is also your IRQ problem, since iio_channel
>>> is currently free'd before irqs are free'd, but its used in irq
>>> code.
>> 
>> Hm. No.
>> 
>> In upstream code it is never freed on probe failure because there is only
>> an error return (goto fail) after allocating irqs in case the
>> twl_i2c_write_u8 fails. Which usually doesn't.
>> 
>> The EPROBE_DEFER returned by iio_channel_get not being ready simply prints
>> a warning and turns off AC detection (bci->channel_vac = NULL) forever.
>> 
>>> 
>>>> 	bci->ac = devm_power_supply_register(&pdev->dev, &twl4030_bci_ac_desc,
>>>> 					     NULL);
>>>> 	if (IS_ERR(bci->ac)) {
>>>> @@ -1017,25 +1039,10 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>>>> 		return ret;
>>>> 	}
>>>> 
>>>> -	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
>>>> -	if (IS_ERR(bci->channel_vac)) {
>> 
>> My first attempt to fix EPROBE_DEFER was to add this
>> 
>> +		if (PTR_ERR(bci->channel_vac) == -EPROBE_DEFER)
>> +			return -EPROBE_DEFER;	/* iio not ready */
>> 
>>>> -		bci->channel_vac = NULL;
>>>> -		dev_warn(&pdev->dev, "could not request vac iio channel");
>>>> -	}
>>>> -
>>>> 	INIT_WORK(&bci->work, twl4030_bci_usb_work);
>>>> 	INIT_DELAYED_WORK(&bci->current_worker, twl4030_current_worker);
>>>> 
>>>> 	bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
>>>> -	if (bci->dev->of_node) {
>>>> -		struct device_node *phynode;
>>>> -
>>>> -		phynode = of_find_compatible_node(bci->dev->of_node->parent,
>>>> -						  NULL, "ti,twl4030-usb");
>>>> -		if (phynode)
>>>> -			bci->transceiver = devm_usb_get_phy_by_node(
>>>> -				bci->dev, phynode, &bci->usb_nb);
>> 
>> and this
>> 
>> +			if (IS_ERR(bci->transceiver) &&
>> +			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER) {
>> +				ret = -EPROBE_DEFER;	/* PHY not ready */
>> +				goto fail;
>> 
>> This did run (and potentially return) after installing devm_request_threaded_irq leading
>> to observation of severe kernel panics by interrupts.
>> 
>> Moving the iio channel allocation before devm_request_threaded_irq made them go away.
>> 
>>>> -	}
>>>> 
>>>> 	/* Enable interrupts now. */
>>>> 	reg = ~(u32)(TWL4030_ICHGLOW | TWL4030_ICHGEOC | TWL4030_TBATOR2 |
>>> 
>>> -- Sebastian
>> 
>> How important is it to keep the current sequence of devm_request_threaded_irq() + (devm_)iio_channel_get() untouched?
>> 
>> The reason I ask is that it is more likely for (devm_)iio_channel_get() to fail than devm_request_threaded_irq().
>> 
>> Hence the iio channel should imho be always allocated first, so that we skip allocating+freeing unused irq handlers in case
>> of iio not being ready.
>> 
>> Therefore, I suggest to keep the proposed reordering even if we think devm-conversion of iio_channel_get() alone
>> would solve it equally well.
>> 
>> Next: should there be two separate patches? One for converting the non-devm iio_channel_get() and cleaning up error path.
>> And one for adding EPROBE_DEFER error path.
>> 
>> Or keep it in a single patch because they only make sense if done together (you can't add/remove deferred probing
>> without converting and/or moving iio_channel_get())?
>> 
>> So please advise how to proceed.
>> 
> 
> You should request irq as late as possible in probe - it's safest way to go always.
> You see crushes simply because request_irq enables IRQ and IRQ can trigger immediately, so
> IRQ handler will run and all required resources should be ready and initialized.
> 
> In this case:
> twl4030_bci_interrupt() -> twl4030_charger_update_current() -> ac_available() -> iio_read_channel_processed()
> OOOPS.
> 
> So, first thing to do is to reorder probe to ensure that sequence is safe.

That is exactly what I guessed when proposing the reordering patch.

> Then other stuff - devm, EPROBE_DEFER ...

IMHO, reordering alone doesn't make much sense as a single patch. Or does it?

BR and thanks,
Nikolaus

Re: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio

[Grygorii Strashko]

On 06/09/2017 11:59 PM, H. Nikolaus Schaller wrote:
> Hi Grygorii,
> 
>> Am 09.06.2017 um 18:25 schrieb Grygorii Strashko <grygorii.strashko@ti.com>:
>>
>>
>>
>> On 06/09/2017 01:05 AM, H. Nikolaus Schaller wrote:
>>> Hi,
>>>
>>>> Am 07.06.2017 um 22:44 schrieb Sebastian Reichel <sre@kernel.org>:
>>>>
>>>> Hi,
>>>>
>>>> On Sun, May 21, 2017 at 12:38:18PM +0200, H. Nikolaus Schaller wrote:
>>>>> This fixes an issue if both this twl4030_charger driver and
>>>>> phy-twl4030-usb are compiled as modules and loaded in random order.
>>>>> It has been observed on GTA04 and OpenPandora devices that in worst
>>>>> case the boot process hangs and in best case the AC detection fails
>>>>> with a warning.
>>>>>
>>>>> Therefore we add deferred probing checks for the usb_phy and the
>>>>> iio channel for AC detection.
>>>>>
>>>>> For implementing this we must reorder code because we can't safely
>>>>> return -EPROBE_DEFER after allocating any devm managed interrupt
>>>>> (it might already/still be enabled without working interrupt handler).
>>>>>
>>>>> So the check for required resources that may abort probing by
>>>>> returning -EPROBE_DEFER, must come first.
>>>>
>>>> This sounds fishy. EPROBE_DEFER should not be different from
>>>> other error codes in this regard and devm_ requested resources
>>>> should be free'd on any error. Why is irq handler not working?
>>>
>>> 1. there is no other error code involved, before we try to convert the driver to handle EPROBE_DEFER.
>>> So it is not that EPROBE_DEFER is special but that we add an error return path for it.
>>>
>>> 2. I don't know why it is not working - I just know that the handler seems to be triggered before
>>> all resources are available (if probe() is aborted with EPROBE_DEFER error paths) which ends up in kernel panics.
>>>
>>> Therefore just to be safe, I have reordered things a little (without changing the function):
>>>
>>> 1. check for resources (with some EPROBE_DEFER)
>>> 2. allocate non-devm (with optional EPROBE_DEFER)
>>> 3. allocate devm
>>>
>>> So this should be safe in any case.
>>>
>>> Please also compare a discussion
>>>
>>> https://lkml.org/lkml/2013/2/22/65
>>>
>>>>
>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>>> ---
>>>>> drivers/power/supply/twl4030_charger.c | 37 ++++++++++++++++++++--------------
>>>>> 1 file changed, 22 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
>>>>> index 785a07bc4f39..945eabdbbc89 100644
>>>>> --- a/drivers/power/supply/twl4030_charger.c
>>>>> +++ b/drivers/power/supply/twl4030_charger.c
>>>>> @@ -984,6 +984,28 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>>>>>
>>>>> 	platform_set_drvdata(pdev, bci);
>>>>>
>>>>> +	if (bci->dev->of_node) {
>>>>> +		struct device_node *phynode;
>>>>> +
>>>>> +		phynode = of_find_compatible_node(bci->dev->of_node->parent,
>>>>> +						  NULL, "ti,twl4030-usb");
>>>>> +		if (phynode) {
>>>>> +			bci->transceiver = devm_usb_get_phy_by_node(
>>>>> +				bci->dev, phynode, &bci->usb_nb);
>>>>> +			if (IS_ERR(bci->transceiver) &&
>>>>> +			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
>>>>> +				return -EPROBE_DEFER;	/* PHY not ready */
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
>>>>> +	if (IS_ERR(bci->channel_vac)) {
>>>>> +		if (PTR_ERR(bci->channel_vac) == -EPROBE_DEFER)
>>>>> +			return -EPROBE_DEFER;	/* iio not ready */
>>>>> +		dev_warn(&pdev->dev, "could not request vac iio channel");
>>>>> +		bci->channel_vac = NULL;
>>>>> +	}
>>>>> +
>>>>
>>>> You should not request non-devm managed iio_channel before
>>>> devm-managed power-supply.
>>>
>>> Well, it is not *me* doing that.
>>>
>>> It is the unpatched driver which already does. See:
>>>
>>> 	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/twl4030_charger.c?h=v4.12-rc4#n1069
>>>
>>> I have just moved this line to a different location to be able to add a proper EPROBE_DEFER return path.
>>>
>>>> This could be fixed by switching to
>>>> devm_iio_channel_get(), which also cleans up some code.
>>>
>>> Yes, this could be an alternative solution.
>>> We still need EPROBE_DEFER handling.
>>>
>>>>
>>>> I suspect, that this is also your IRQ problem, since iio_channel
>>>> is currently free'd before irqs are free'd, but its used in irq
>>>> code.
>>>
>>> Hm. No.
>>>
>>> In upstream code it is never freed on probe failure because there is only
>>> an error return (goto fail) after allocating irqs in case the
>>> twl_i2c_write_u8 fails. Which usually doesn't.
>>>
>>> The EPROBE_DEFER returned by iio_channel_get not being ready simply prints
>>> a warning and turns off AC detection (bci->channel_vac = NULL) forever.
>>>
>>>>
>>>>> 	bci->ac = devm_power_supply_register(&pdev->dev, &twl4030_bci_ac_desc,
>>>>> 					     NULL);
>>>>> 	if (IS_ERR(bci->ac)) {
>>>>> @@ -1017,25 +1039,10 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>>>>> 		return ret;
>>>>> 	}
>>>>>
>>>>> -	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
>>>>> -	if (IS_ERR(bci->channel_vac)) {
>>>
>>> My first attempt to fix EPROBE_DEFER was to add this
>>>
>>> +		if (PTR_ERR(bci->channel_vac) == -EPROBE_DEFER)
>>> +			return -EPROBE_DEFER;	/* iio not ready */
>>>
>>>>> -		bci->channel_vac = NULL;
>>>>> -		dev_warn(&pdev->dev, "could not request vac iio channel");
>>>>> -	}
>>>>> -
>>>>> 	INIT_WORK(&bci->work, twl4030_bci_usb_work);
>>>>> 	INIT_DELAYED_WORK(&bci->current_worker, twl4030_current_worker);
>>>>>
>>>>> 	bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
>>>>> -	if (bci->dev->of_node) {
>>>>> -		struct device_node *phynode;
>>>>> -
>>>>> -		phynode = of_find_compatible_node(bci->dev->of_node->parent,
>>>>> -						  NULL, "ti,twl4030-usb");
>>>>> -		if (phynode)
>>>>> -			bci->transceiver = devm_usb_get_phy_by_node(
>>>>> -				bci->dev, phynode, &bci->usb_nb);
>>>
>>> and this
>>>
>>> +			if (IS_ERR(bci->transceiver) &&
>>> +			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER) {
>>> +				ret = -EPROBE_DEFER;	/* PHY not ready */
>>> +				goto fail;
>>>
>>> This did run (and potentially return) after installing devm_request_threaded_irq leading
>>> to observation of severe kernel panics by interrupts.
>>>
>>> Moving the iio channel allocation before devm_request_threaded_irq made them go away.
>>>
>>>>> -	}
>>>>>
>>>>> 	/* Enable interrupts now. */
>>>>> 	reg = ~(u32)(TWL4030_ICHGLOW | TWL4030_ICHGEOC | TWL4030_TBATOR2 |
>>>>
>>>> -- Sebastian
>>>
>>> How important is it to keep the current sequence of devm_request_threaded_irq() + (devm_)iio_channel_get() untouched?
>>>
>>> The reason I ask is that it is more likely for (devm_)iio_channel_get() to fail than devm_request_threaded_irq().
>>>
>>> Hence the iio channel should imho be always allocated first, so that we skip allocating+freeing unused irq handlers in case
>>> of iio not being ready.
>>>
>>> Therefore, I suggest to keep the proposed reordering even if we think devm-conversion of iio_channel_get() alone
>>> would solve it equally well.
>>>
>>> Next: should there be two separate patches? One for converting the non-devm iio_channel_get() and cleaning up error path.
>>> And one for adding EPROBE_DEFER error path.
>>>
>>> Or keep it in a single patch because they only make sense if done together (you can't add/remove deferred probing
>>> without converting and/or moving iio_channel_get())?
>>>
>>> So please advise how to proceed.
>>>
>>
>> You should request irq as late as possible in probe - it's safest way to go always.
>> You see crushes simply because request_irq enables IRQ and IRQ can trigger immediately, so
>> IRQ handler will run and all required resources should be ready and initialized.
>>
>> In this case:
>> twl4030_bci_interrupt() -> twl4030_charger_update_current() -> ac_available() -> iio_read_channel_processed()
>> OOOPS.
>>
>> So, first thing to do is to reorder probe to ensure that sequence is safe.
> 
> That is exactly what I guessed when proposing the reordering patch.
> 
>> Then other stuff - devm, EPROBE_DEFER ...
> 
> IMHO, reordering alone doesn't make much sense as a single patch. Or does it?
> 

The question is - is there bug in driver or not? As per current code seems yes.
You can easily test it by directly calling twl4030_charger_interrupt() right after
IRQ is requested. there is a bug if it will crush.
As for me it more reasonable to move forward using smaller steps.


Thanks for you work and patches.

-- 
regards,
-grygorii

Re: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio

[H. Nikolaus Schaller]

Hi Grygorii,

> Am 12.06.2017 um 18:24 schrieb Grygorii Strashko <grygorii.strashko@ti.com>:
> 
> 
> 
> On 06/09/2017 11:59 PM, H. Nikolaus Schaller wrote:
>> Hi Grygorii,
>> 
>>> Am 09.06.2017 um 18:25 schrieb Grygorii Strashko <grygorii.strashko@ti.com>:
>>> 
>>> 
>>>> 
>>>> So please advise how to proceed.
>>>> 
>>> 
>>> You should request irq as late as possible in probe - it's safest way to go always.
>>> You see crushes simply because request_irq enables IRQ and IRQ can trigger immediately, so
>>> IRQ handler will run and all required resources should be ready and initialized.
>>> 
>>> In this case:
>>> twl4030_bci_interrupt() -> twl4030_charger_update_current() -> ac_available() -> iio_read_channel_processed()
>>> OOOPS.
>>> 
>>> So, first thing to do is to reorder probe to ensure that sequence is safe.
>> 
>> That is exactly what I guessed when proposing the reordering patch.
>> 
>>> Then other stuff - devm, EPROBE_DEFER ...
>> 
>> IMHO, reordering alone doesn't make much sense as a single patch. Or does it?
>> 
> 
> The question is - is there bug in driver or not? As per current code seems yes.

If we all agree, do we need another confirmation?

> You can easily test it by directly calling twl4030_charger_interrupt() right after
> IRQ is requested. there is a bug if it will crush.

In the variant without EPROBE_DEFER you won't see it since ac_available either
has a valid iio channel or NULL.

The problem is only if we add an EPROBE_DEFER return if getting the iio channel
fails. This seems to make trouble if we don't take care of it. There are certainly
several options to work around but IMHO reordering is the best one (and even works
w/o devm for iio - which should be added in a separate step).

And there is a strong argument for reordering to have the most likely failure
first (iio fails more likely due to DEFER than allocating an irq). But only if
we process the iio failure at all.

So there are one a little doubtful argument for reordering (driver bug) and
a strong one.

> As for me it more reasonable to move forward using smaller steps.

Well, I wonder what it does help to fix a bug which does not even become visible
if we don't add EPROBE_DEFER?

So I would count two steps:
a) add EPROBE_DEFER and reorder code to make it work
b) convert to devm for iio

Ok?

BR and thanks,
Nikolaus

Re: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio

[Grygorii Strashko]

On 06/12/2017 12:02 PM, H. Nikolaus Schaller wrote:
> Hi Grygorii,
> 
>> Am 12.06.2017 um 18:24 schrieb Grygorii Strashko <grygorii.strashko@ti.com>:
>>
>>
>>
>> On 06/09/2017 11:59 PM, H. Nikolaus Schaller wrote:
>>> Hi Grygorii,
>>>
>>>> Am 09.06.2017 um 18:25 schrieb Grygorii Strashko <grygorii.strashko@ti.com>:
>>>>
>>>>
>>>>>
>>>>> So please advise how to proceed.
>>>>>
>>>>
>>>> You should request irq as late as possible in probe - it's safest way to go always.
>>>> You see crushes simply because request_irq enables IRQ and IRQ can trigger immediately, so
>>>> IRQ handler will run and all required resources should be ready and initialized.
>>>>
>>>> In this case:
>>>> twl4030_bci_interrupt() -> twl4030_charger_update_current() -> ac_available() -> iio_read_channel_processed()
>>>> OOOPS.
>>>>
>>>> So, first thing to do is to reorder probe to ensure that sequence is safe.
>>>
>>> That is exactly what I guessed when proposing the reordering patch.
>>>
>>>> Then other stuff - devm, EPROBE_DEFER ...
>>>
>>> IMHO, reordering alone doesn't make much sense as a single patch. Or does it?
>>>
>>
>> The question is - is there bug in driver or not? As per current code seems yes.
> 
> If we all agree, do we need another confirmation?
> 
>> You can easily test it by directly calling twl4030_charger_interrupt() right after
>> IRQ is requested. there is a bug if it will crush.
> 
> In the variant without EPROBE_DEFER you won't see it since ac_available either
> has a valid iio channel or NULL.
> 
> The problem is only if we add an EPROBE_DEFER return if getting the iio channel
> fails. This seems to make trouble if we don't take care of it. There are certainly
> several options to work around but IMHO reordering is the best one (and even works
> w/o devm for iio - which should be added in a separate step).
> 
> And there is a strong argument for reordering to have the most likely failure
> first (iio fails more likely due to DEFER than allocating an irq). But only if
> we process the iio failure at all.
> 
> So there are one a little doubtful argument for reordering (driver bug) and
> a strong one.
> 
>> As for me it more reasonable to move forward using smaller steps.
> 
> Well, I wonder what it does help to fix a bug which does not even become visible
> if we don't add EPROBE_DEFER?

Sry, but have you tried test I've proposed (I can't try it by myself now, sry)? 
There is not only iio channel can be a problem - for example "current_worker"
 initialized after IRQ request, but can be scheduled from IRQ handler.

> 
> So I would count two steps:
> a) add EPROBE_DEFER and reorder code to make it work
> b) convert to devm for iio
> 

Few words regarding devm_request_irq() - it's useful, from one side, and
dangerous form another :(, as below init sequence is proved to be unsafe
and so it has to be used very carefully:

probe() {
 <do some initialization>
 <create some object:> objX = create_objX() -- no devm
 devm_request_irq(IrqY) -- IrqY handler using objX

 <init step failed>
	goto err;
...

err:
 [a]
 release_objX() - note IRQ is still enabled here
}
dd_core if err:
 devres_release_all() - IRQ disabled and freed only here

remove() {
 [b]
 release_objX() -- note IRQ is still enabled here
}
dd_core:
 devres_release_all() - IRQ disabled and freed only here

IRQ has to be explicitly disabled at points [a] & [b] 

Sequence to move forward can be up to you in general,
personally I'd try to add devm_iio and move  devm_request_irq()
down right before "/* Enable interrupts now. */" line first.

-- 
regards,
-grygorii

Re: [PATCH v5 3/3] drivers:power:twl4030-charger: add deferred probing for phy and iio

[H. Nikolaus Schaller]

Hi,
>> 
>>> As for me it more reasonable to move forward using smaller steps.
>> 
>> Well, I wonder what it does help to fix a bug which does not even become visible
>> if we don't add EPROBE_DEFER?
> 
> Sry, but have you tried test I've proposed (I can't try it by myself now, sry)? 

No because I can't easily try it at the moment.

And I think it does not show what you expect.

Usually the iio_channel_get() fails and we have bci->channel_vac = NULL
which does not make problems in the irq handler and is catched in ac_available().

To make iio_channel_get() not fail we have to implement deferred probe
handling.

> There is not only iio channel can be a problem - for example "current_worker"
> initialized after IRQ request, but can be scheduled from IRQ handler.

Ah, yet another bug.

Looks as if there are some race conditions and we are just lucky that it
rarely happens.

> 
>> 
>> So I would count two steps:
>> a) add EPROBE_DEFER and reorder code to make it work
>> b) convert to devm for iio
>> 
> 
> Few words regarding devm_request_irq() - it's useful, from one side, and
> dangerous form another :(, as below init sequence is proved to be unsafe
> and so it has to be used very carefully:
> 
> probe() {
> <do some initialization>
> <create some object:> objX = create_objX() -- no devm
> devm_request_irq(IrqY) -- IrqY handler using objX
> 
> <init step failed>
> 	goto err;
> ...
> 
> err:
> [a]
> release_objX() - note IRQ is still enabled here
> }
> dd_core if err:
> devres_release_all() - IRQ disabled and freed only here
> 
> remove() {
> [b]
> release_objX() -- note IRQ is still enabled here
> }
> dd_core:
> devres_release_all() - IRQ disabled and freed only here
> 
> IRQ has to be explicitly disabled at points [a] & [b] 

Or everything done devm so that irqs are released first.

> 
> Sequence to move forward can be up to you in general,
> personally I'd try to add devm_iio and move  devm_request_irq()
> down right before "/* Enable interrupts now. */" line first.

Yes, that is what I almost proposed as well.

Except that you propose to move lines from INIT_WORK() up to

if (bci->dev->of_node) {
...
}

as well.

Looks good to me.

So if Sebastian or others have no more comments, I will prepare a patch v6 asap.

BR and thanks,
Nikolaus

Re: [PATCH v5 0/3] More fixes for twl4030 charger

[H. Nikolaus Schaller]

No comments for this series? Everything fine? Has it been merged somewhere?

> Am 21.05.2017 um 12:38 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Changes V5:
> * reworked max_current removal patch (comments by Sebastian Reichel)
> * resubmit missing madc connection for AC power detection
> * resubmit patch for irq allocation and -EPROBE_DEFER
> * rebased on 4.12-rc1
> 
> Changes V4:
> * resent commit (original one did contain material not upstream)
> 
> 2017-04-14 21:29:39: 2017-04-14 20:26:00: Changes V3:
> * worked in comments by Sebsatian Reichel
> * clarifications of some commit messages
> * rebased on v4.11rc-6
> 
> It took a while (18 months) until we propose an updated patch for upstream...
> 
> 2015-11-02 12:27:39: Changes V2:
> * worked in comments by Nishanth Menon <nm@ti.com>
> * added another patch which solves a probing/boot stall problem (irq allocation vs. -EPROBE_DEFER)
> 
> V1:
> 4.3-rc1 introduced a new charger driver for the twl4030. This patch set fixes some
> issues.
> 
> While making twl4030 changes from 4.3 operable we have found some issues
> during testing on GTA04 and OpenPandora.
> 
> H. Nikolaus Schaller (2):
>  drivers:power:twl4030-charger: remove nonstandard max_current sysfs
>    attribute
>  drivers:power:twl4030-charger: add deferred probing for phy and iio
> 
> Marek Belisko (1):
>  ARM: dts: twl4030: Add missing madc reference for bci subnode
> 
> .../ABI/testing/sysfs-class-power-twl4030          |  17 ----
> arch/arm/boot/dts/twl4030.dtsi                     |   2 +
> drivers/power/supply/twl4030_charger.c             | 100 +++++----------------
> 3 files changed, 24 insertions(+), 95 deletions(-)
> 
> -- 
> 2.12.2
>