More fixes for twl4030 charger

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 <[email protected]>

---
 .../ABI/testing/sysfs-class-power-twl4030          | 17 ------
 drivers/power/supply/twl4030_charger.c             | 63 ----------------------
 2 files changed, 80 deletions(-)

Documentation/ABI/testing/sysfs-class-power-twl4030 +0 -17

index be26af0f1895..b4fd32d210c5 100644
@@ -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.

drivers/power/supply/twl4030_charger.c +0 -63

index 2f82d0e9ec1b..785a07bc4f39 100644
@@ -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,

Signature · 1 line

2.12.2

From: Marek Belisko <[email protected]>

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.

Am 06.06.2017 um 09:23 schrieb Tony Lindgren <[email protected]>:

Quoted 5 lines · H. Nikolaus Schaller

From: Marek Belisko <[email protected]>

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.

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 <[email protected]>

---
 drivers/power/supply/twl4030_charger.c | 37 ++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 15 deletions(-)

drivers/power/supply/twl4030_charger.c +12 -0

index 785a07bc4f39..945eabdbbc89 100644
@@ -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 */
+		}

drivers/power/supply/twl4030_charger.c +0 -8

 	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;

Am 07.06.2017 um 22:44 schrieb Sebastian Reichel <[email protected]>:

Hi,

Quoted 13 lines · 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).

Quoted 2 lines

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?

Quoted 31 lines

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/power/supply/twl4030_charger.c | 37 ++++++++++++++++++++--------------

1 file changed, 22 insertions(+), 15 deletions(-)

drivers/power/supply/twl4030_charger.c +17 -0

index 785a07bc4f39..945eabdbbc89 100644
@@ -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)

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1008

Quoted 5 lines

drivers/power/supply/twl4030_charger.c +5 -0

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

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1021

Quoted 10 lines

drivers/power/supply/twl4030_charger.c +0 -2

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

Quoted 7 lines

drivers/power/supply/twl4030_charger.c +0 -4

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

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1037

Quoted 9 lines

drivers/power/supply/twl4030_charger.c +0 -8

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

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1048

Quoted 4 lines

drivers/power/supply/twl4030_charger.c +0 -1

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

-- Sebastian

Hi,

Quoted 18 lines

Am 07.06.2017 um 22:44 schrieb Sebastian Reichel <[email protected]>:

Hi,

Quoted 13 lines · 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).

Quoted 6 lines

Quoted 2 lines

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

Quoted 31 lines

Quoted 31 lines

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/power/supply/twl4030_charger.c | 37 ++++++++++++++++++++--------------

1 file changed, 22 insertions(+), 15 deletions(-)

drivers/power/supply/twl4030_charger.c +17 -0

index 785a07bc4f39..945eabdbbc89 100644
@@ -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)

Quoted 8 lines

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1008

Quoted 5 lines

drivers/power/supply/twl4030_charger.c +5 -0

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

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 10 lines

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1021

Quoted 10 lines

drivers/power/supply/twl4030_charger.c +0 -2

 	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

drivers/power/supply/twl4030_charger.c +2 -0

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

Quoted 8 lines

Quoted 8 lines

drivers/power/supply/twl4030_charger.c +0 -4

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

Quoted 9 lines

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1037

Quoted 9 lines

drivers/power/supply/twl4030_charger.c +0 -8

 	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

(quoted diff) +4 -0

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

Quoted 6 lines

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1048

Quoted 4 lines

drivers/power/supply/twl4030_charger.c +0 -1

-	}
 
 	/* 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.

Am 09.06.2017 um 18:25 schrieb Grygorii Strashko <[email protected]>:

Quoted 175 lines · H. Nikolaus Schaller

Hi,

Quoted 18 lines

Am 07.06.2017 um 22:44 schrieb Sebastian Reichel <[email protected]>:

Hi,

Quoted 13 lines · 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).

Quoted 6 lines

Quoted 2 lines

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

Quoted 31 lines

Quoted 31 lines

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/power/supply/twl4030_charger.c | 37 ++++++++++++++++++++--------------

1 file changed, 22 insertions(+), 15 deletions(-)

drivers/power/supply/twl4030_charger.c +17 -0

index 785a07bc4f39..945eabdbbc89 100644
@@ -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)

Quoted 8 lines

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1008

Quoted 5 lines

drivers/power/supply/twl4030_charger.c +5 -0

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

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 10 lines

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1021

Quoted 10 lines

drivers/power/supply/twl4030_charger.c +0 -2

 	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

drivers/power/supply/twl4030_charger.c +2 -0

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

Quoted 8 lines

Quoted 8 lines

drivers/power/supply/twl4030_charger.c +0 -4

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

Quoted 9 lines

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1037

Quoted 9 lines

drivers/power/supply/twl4030_charger.c +0 -8

 	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

(quoted diff) +4 -0

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

Quoted 6 lines

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1048

Quoted 4 lines

drivers/power/supply/twl4030_charger.c +0 -1

-	}
 
 	/* 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.

Quoted 8 lines

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

Hi Grygorii,

Quoted 197 lines

Am 09.06.2017 um 18:25 schrieb Grygorii Strashko <[email protected]>:

Quoted 175 lines · H. Nikolaus Schaller

Hi,

Quoted 18 lines

Am 07.06.2017 um 22:44 schrieb Sebastian Reichel <[email protected]>:

Hi,

Quoted 13 lines · 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).

Quoted 6 lines

Quoted 2 lines

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

Quoted 31 lines

Quoted 31 lines

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/power/supply/twl4030_charger.c | 37 ++++++++++++++++++++--------------

1 file changed, 22 insertions(+), 15 deletions(-)

drivers/power/supply/twl4030_charger.c +17 -0

index 785a07bc4f39..945eabdbbc89 100644
@@ -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)

Quoted 8 lines

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1008

Quoted 5 lines

drivers/power/supply/twl4030_charger.c +5 -0

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

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 10 lines

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1021

Quoted 10 lines

drivers/power/supply/twl4030_charger.c +0 -2

 	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

drivers/power/supply/twl4030_charger.c +2 -0

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

Quoted 8 lines

Quoted 8 lines

drivers/power/supply/twl4030_charger.c +0 -4

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

Quoted 9 lines

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1037

Quoted 9 lines

drivers/power/supply/twl4030_charger.c +0 -8

 	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

(quoted diff) +4 -0

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

Quoted 6 lines

drivers/power/supply/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 1048

Quoted 4 lines

drivers/power/supply/twl4030_charger.c +0 -1

-	}
 
 	/* 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.

Quoted 8 lines

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.

Quoted 1 line

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.

Quoted 1 line

Then other stuff - devm, EPROBE_DEFER ...

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

Am 12.06.2017 um 18:24 schrieb Grygorii Strashko <[email protected]>:

Quoted 19 lines · H. Nikolaus Schaller

Hi Grygorii,

Quoted 16 lines

Am 09.06.2017 um 18:25 schrieb Grygorii Strashko <[email protected]>:

Quoted 3 lines

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.

Quoted 6 lines

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

Quoted 1 line

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.

Hi Grygorii,

Quoted 24 lines

Am 12.06.2017 um 18:24 schrieb Grygorii Strashko <[email protected]>:

Quoted 19 lines · H. Nikolaus Schaller

Hi Grygorii,

Quoted 16 lines

Am 09.06.2017 um 18:25 schrieb Grygorii Strashko <[email protected]>:

Quoted 3 lines

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.

Quoted 8 lines

Quoted 6 lines

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

Quoted 1 line

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?

Quoted 2 lines

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.

Quoted 1 line

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

Am 21.05.2017 um 12:38 schrieb H. Nikolaus Schaller <[email protected]>:

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 <[email protected]>
* 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(-)

Signature · 1 line

2.12.2