linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
@ 2018-11-10 18:10 Andy Shevchenko
  2018-11-10 18:10 ` [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found Andy Shevchenko
                   ` (6 more replies)
  0 siblings, 7 replies; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-10 18:10 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi, linux-usb, Felipe Balbi,
	Guenter Roeck, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, Sebastian Reichel, linux-omap, Darren Hart,
	platform-driver-x86, Greg Kroah-Hartman, linux-kernel,
	Chen-Yu Tsai, Hans de Goede
  Cc: Andy Shevchenko, Grant Likely, Peter Ujfalusi, Mark Brown, Andrzej Hajda

Consider the following scenario.

There are two independent devices coupled together by functional dependencies:
 - USB OTG (dwc3-pci)
 - extcon (tested with extcon-intel-mrfld, not yet in upstream)

Each of the driver services a corresponding device is built as a module. In the
Buildroot environment the modules are probed by alphabetical ordering of their
modaliases. The latter comes to the case when USB OTG driver will be probed
first followed by extcon one.

So, if the platform anticipates extcon device to be appeared, in the above case
we will get deferred probe of USB OTG, because of ordering.

Now, a cherry on top of the cake, the deferred probing list contains
the only two modules, i.e. USB OTG and extcon. Due to above circumstances,
values in the local_trigger_count and deferred_trigger_count are not the same,
and thus provokes deferred probe triggering again and again.

...
[   20.678332] platform dwc3.0.auto: Retrying from deferred list
[   20.694743] platform dwc3.0.auto: Driver dwc3 requests probe deferral
[   20.701254] platform dwc3.0.auto: Added to deferred list
[   20.706620] platform dwc3.0.auto: driver_deferred_probe_add_trigger 1 2
[   20.713732] platform dwc3.0.auto: Retrying from deferred list
[   20.730035] platform dwc3.0.auto: Driver dwc3 requests probe deferral
[   20.736540] platform dwc3.0.auto: Added to deferred list
[   20.741889] platform dwc3.0.auto: driver_deferred_probe_add_trigger 3 4
[   20.748991] platform dwc3.0.auto: Retrying from deferred list
[   20.765416] platform dwc3.0.auto: Driver dwc3 requests probe deferral
[   20.771914] platform dwc3.0.auto: Added to deferred list
[   20.777279] platform dwc3.0.auto: driver_deferred_probe_add_trigger 5 6
...

Deeper investigation shows the culprit commit 58b116bce136
("drivercore: deferral race condition fix") which was dedicated to fix some
other issue while bringing a regression.

This reverts commit 58b116bce13612e5aa6fcd49ecbd4cf8bb59e835 for good until
we will have better solution.

Cc: Grant Likely <grant.likely@linaro.org>
Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/dd.c | 27 ++-------------------------
 1 file changed, 2 insertions(+), 25 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 169412ee4ae8..9a966e45fda5 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -53,7 +53,6 @@
 static DEFINE_MUTEX(deferred_probe_mutex);
 static LIST_HEAD(deferred_probe_pending_list);
 static LIST_HEAD(deferred_probe_active_list);
-static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
 static struct dentry *deferred_devices;
 static bool initcalls_done;
 
@@ -143,17 +142,6 @@ static bool driver_deferred_probe_enable = false;
  * This functions moves all devices from the pending list to the active
  * list and schedules the deferred probe workqueue to process them.  It
  * should be called anytime a driver is successfully bound to a device.
- *
- * Note, there is a race condition in multi-threaded probe. In the case where
- * more than one device is probing at the same time, it is possible for one
- * probe to complete successfully while another is about to defer. If the second
- * depends on the first, then it will get put on the pending list after the
- * trigger event has already occurred and will be stuck there.
- *
- * The atomic 'deferred_trigger_count' is used to determine if a successful
- * trigger has occurred in the midst of probing a driver. If the trigger count
- * changes in the midst of a probe, then deferred processing should be triggered
- * again.
  */
 static void driver_deferred_probe_trigger(void)
 {
@@ -166,7 +154,6 @@ static void driver_deferred_probe_trigger(void)
 	 * into the active list so they can be retried by the workqueue
 	 */
 	mutex_lock(&deferred_probe_mutex);
-	atomic_inc(&deferred_trigger_count);
 	list_splice_tail_init(&deferred_probe_pending_list,
 			      &deferred_probe_active_list);
 	mutex_unlock(&deferred_probe_mutex);
@@ -434,19 +421,9 @@ EXPORT_SYMBOL_GPL(device_bind_driver);
 static atomic_t probe_count = ATOMIC_INIT(0);
 static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
 
-static void driver_deferred_probe_add_trigger(struct device *dev,
-					      int local_trigger_count)
-{
-	driver_deferred_probe_add(dev);
-	/* Did a trigger occur while probing? Need to re-trigger if yes */
-	if (local_trigger_count != atomic_read(&deferred_trigger_count))
-		driver_deferred_probe_trigger();
-}
-
 static int really_probe(struct device *dev, struct device_driver *drv)
 {
 	int ret = -EPROBE_DEFER;
-	int local_trigger_count = atomic_read(&deferred_trigger_count);
 	bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
 			   !drv->suppress_bind_attrs;
 
@@ -463,7 +440,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 
 	ret = device_links_check_suppliers(dev);
 	if (ret == -EPROBE_DEFER)
-		driver_deferred_probe_add_trigger(dev, local_trigger_count);
+		driver_deferred_probe_add(dev);
 	if (ret)
 		return ret;
 
@@ -559,7 +536,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	case -EPROBE_DEFER:
 		/* Driver requested deferred probing */
 		dev_dbg(dev, "Driver %s requests probe deferral\n", drv->name);
-		driver_deferred_probe_add_trigger(dev, local_trigger_count);
+		driver_deferred_probe_add(dev);
 		break;
 	case -ENODEV:
 	case -ENXIO:
-- 
2.19.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-10 18:10 [PATCH v1 1/5] drivercore: Revert "deferral race condition fix" Andy Shevchenko
@ 2018-11-10 18:10 ` Andy Shevchenko
  2018-11-10 18:39   ` Guenter Roeck
                     ` (3 more replies)
  2018-11-10 18:10 ` [PATCH v1 3/5] staging: typec: fusb302: Rename fcs,extcon-name to linux,extcon-name Andy Shevchenko
                   ` (5 subsequent siblings)
  6 siblings, 4 replies; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-10 18:10 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi, linux-usb, Felipe Balbi,
	Guenter Roeck, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, Sebastian Reichel, linux-omap, Darren Hart,
	platform-driver-x86, Greg Kroah-Hartman, linux-kernel,
	Chen-Yu Tsai, Hans de Goede
  Cc: Andy Shevchenko

All current users of extcon_get_extcon_dev() API considers
an extcon device a mandatory to appear. Thus, they all convert
NULL pointer to -EPROBE_DEFER error code.

There is one more caller anticipated with the same requirements.

To decrease a code duplication and a burden to the callers,
return -EPROBE_DEFER directly from extcon_get_extcon_dev().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/extcon/extcon-axp288.c        | 4 ++--
 drivers/extcon/extcon.c               | 2 +-
 drivers/power/supply/axp288_charger.c | 8 ++++----
 drivers/usb/phy/phy-omap-otg.c        | 6 +++---
 drivers/usb/typec/tcpm/fusb302.c      | 4 ++--
 5 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
index a983708b77a6..3472d3b756ed 100644
--- a/drivers/extcon/extcon-axp288.c
+++ b/drivers/extcon/extcon-axp288.c
@@ -360,8 +360,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		name = acpi_dev_get_first_match_name("INT3496", NULL, -1);
 		if (name) {
 			info->id_extcon = extcon_get_extcon_dev(name);
-			if (!info->id_extcon)
-				return -EPROBE_DEFER;
+			if (IS_ERR(info->id_extcon))
+				return PTR_ERR(info->id_extcon);
 
 			dev_info(dev, "controlling USB role\n");
 		} else {
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 5ab0498be652..2bd0f2f33f05 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -884,7 +884,7 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 		if (!strcmp(sd->name, extcon_name))
 			goto out;
 	}
-	sd = NULL;
+	sd = ERR_PTR(-EPROBE_DEFER);
 out:
 	mutex_unlock(&extcon_dev_list_lock);
 	return sd;
diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 735658ee1c60..8558577fccf5 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -768,17 +768,17 @@ static int axp288_charger_probe(struct platform_device *pdev)
 	info->regmap_irqc = axp20x->regmap_irqc;
 
 	info->cable.edev = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME);
-	if (info->cable.edev == NULL) {
+	if (IS_ERR(info->cable.edev)) {
 		dev_dbg(&pdev->dev, "%s is not ready, probe deferred\n",
 			AXP288_EXTCON_DEV_NAME);
-		return -EPROBE_DEFER;
+		return PTR_ERR(info->cable.edev);
 	}
 
 	if (acpi_dev_present(USB_HOST_EXTCON_HID, NULL, -1)) {
 		info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_NAME);
-		if (info->otg.cable == NULL) {
+		if (IS_ERR(info->otg.cable)) {
 			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
-			return -EPROBE_DEFER;
+			return PTR_ERR(info->otg.cable);
 		}
 		dev_info(&pdev->dev,
 			 "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c
index ee0863c6553e..605314ddcd3d 100644
--- a/drivers/usb/phy/phy-omap-otg.c
+++ b/drivers/usb/phy/phy-omap-otg.c
@@ -91,12 +91,12 @@ static int omap_otg_probe(struct platform_device *pdev)
 	int ret;
 	u32 rev;
 
-	if (!config || !config->extcon)
+	if (!config)
 		return -ENODEV;
 
 	extcon = extcon_get_extcon_dev(config->extcon);
-	if (!extcon)
-		return -EPROBE_DEFER;
+	if (IS_ERR(extcon))
+		return PTR_ERR(extcon);
 
 	otg_dev = devm_kzalloc(&pdev->dev, sizeof(*otg_dev), GFP_KERNEL);
 	if (!otg_dev)
diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index 43b64d9309d0..6d332066202b 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -1767,8 +1767,8 @@ static int fusb302_probe(struct i2c_client *client,
 	 */
 	if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) {
 		chip->extcon = extcon_get_extcon_dev(name);
-		if (!chip->extcon)
-			return -EPROBE_DEFER;
+		if (IS_ERR(chip->extcon))
+			return PTR_ERR(chip->extcon);
 	}
 
 	chip->vbus = devm_regulator_get(chip->dev, "vbus");
-- 
2.19.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v1 3/5] staging: typec: fusb302: Rename fcs,extcon-name to linux,extcon-name
  2018-11-10 18:10 [PATCH v1 1/5] drivercore: Revert "deferral race condition fix" Andy Shevchenko
  2018-11-10 18:10 ` [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found Andy Shevchenko
@ 2018-11-10 18:10 ` Andy Shevchenko
  2018-11-10 18:40   ` Guenter Roeck
  2018-11-10 18:11 ` [PATCH v1 4/5] usb: dwc3: drd: Switch to device property for 'extcon' handling Andy Shevchenko
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-10 18:10 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi, linux-usb, Felipe Balbi,
	Guenter Roeck, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, Sebastian Reichel, linux-omap, Darren Hart,
	platform-driver-x86, Greg Kroah-Hartman, linux-kernel,
	Chen-Yu Tsai, Hans de Goede
  Cc: Andy Shevchenko

Since we are going to use the same in Designware USB 3 driver,
rename the property to be consistent across the drivers.

No functional change intended.

Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 2 +-
 drivers/usb/typec/tcpm/fusb302.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 464fe93657b5..87cbf18d0305 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -79,7 +79,7 @@ static const struct property_entry max17047_props[] = {
 };
 
 static const struct property_entry fusb302_props[] = {
-	PROPERTY_ENTRY_STRING("fcs,extcon-name", "cht_wcove_pwrsrc"),
+	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
 	PROPERTY_ENTRY_U32("fcs,max-sink-microvolt", 12000000),
 	PROPERTY_ENTRY_U32("fcs,max-sink-microamp",   3000000),
 	PROPERTY_ENTRY_U32("fcs,max-sink-microwatt", 36000000),
diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index 6d332066202b..62816ac571a0 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -1765,7 +1765,7 @@ static int fusb302_probe(struct i2c_client *client,
 	 * to be set by the platform code which also registers the i2c client
 	 * for the fusb302.
 	 */
-	if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) {
+	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0) {
 		chip->extcon = extcon_get_extcon_dev(name);
 		if (IS_ERR(chip->extcon))
 			return PTR_ERR(chip->extcon);
-- 
2.19.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v1 4/5] usb: dwc3: drd: Switch to device property for 'extcon' handling
  2018-11-10 18:10 [PATCH v1 1/5] drivercore: Revert "deferral race condition fix" Andy Shevchenko
  2018-11-10 18:10 ` [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found Andy Shevchenko
  2018-11-10 18:10 ` [PATCH v1 3/5] staging: typec: fusb302: Rename fcs,extcon-name to linux,extcon-name Andy Shevchenko
@ 2018-11-10 18:11 ` Andy Shevchenko
  2018-11-12 11:47   ` Felipe Balbi
  2018-11-10 18:11 ` [PATCH v1 5/5] usb: dwc3: drd: Add support for DR detection through extcon Andy Shevchenko
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-10 18:11 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi, linux-usb, Felipe Balbi,
	Guenter Roeck, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, Sebastian Reichel, linux-omap, Darren Hart,
	platform-driver-x86, Greg Kroah-Hartman, linux-kernel,
	Chen-Yu Tsai, Hans de Goede
  Cc: Andy Shevchenko

Switch to device property for 'extcon' handling.
No functional change intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/usb/dwc3/drd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index e7e421521a34..5dc4cddd5b68 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -10,6 +10,7 @@
 #include <linux/extcon.h>
 #include <linux/of_graph.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 
 #include "debug.h"
 #include "core.h"
@@ -485,8 +486,8 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	struct device_node *np_phy, *np_conn;
 	struct extcon_dev *edev;
 
-	if (of_property_read_bool(dev->of_node, "extcon"))
-		return extcon_get_edev_by_phandle(dwc->dev, 0);
+	if (device_property_read_bool(dev, "extcon"))
+		return extcon_get_edev_by_phandle(dev, 0);
 
 	np_phy = of_parse_phandle(dev->of_node, "phys", 0);
 	np_conn = of_graph_get_remote_node(np_phy, -1, -1);
-- 
2.19.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v1 5/5] usb: dwc3: drd: Add support for DR detection through extcon
  2018-11-10 18:10 [PATCH v1 1/5] drivercore: Revert "deferral race condition fix" Andy Shevchenko
                   ` (2 preceding siblings ...)
  2018-11-10 18:11 ` [PATCH v1 4/5] usb: dwc3: drd: Switch to device property for 'extcon' handling Andy Shevchenko
@ 2018-11-10 18:11 ` Andy Shevchenko
  2018-11-10 18:26 ` [PATCH v1 1/5] drivercore: Revert "deferral race condition fix" Greg Kroah-Hartman
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-10 18:11 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi, linux-usb, Felipe Balbi,
	Guenter Roeck, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, Sebastian Reichel, linux-omap, Darren Hart,
	platform-driver-x86, Greg Kroah-Hartman, linux-kernel,
	Chen-Yu Tsai, Hans de Goede
  Cc: Andy Shevchenko

Allow extcon device, found by name, to provide DR status for USB.
This is needed, for example, in case of Intel Merrifield platform,
where the Intel Basin Cove PMIC provides an extcon device to communicate
the detected role.

Note, that the "linux,extcon-name" property name is only for kernel
internal use by X86/ACPI platform code and as such is not documented
in the device tree bindings.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/usb/dwc3/drd.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 5dc4cddd5b68..d6b47829fdca 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -485,10 +485,20 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	struct device *dev = dwc->dev;
 	struct device_node *np_phy, *np_conn;
 	struct extcon_dev *edev;
+	const char *name;
 
 	if (device_property_read_bool(dev, "extcon"))
 		return extcon_get_edev_by_phandle(dev, 0);
 
+	/*
+	 * Device tree platforms should get extcon via phandle.
+	 * On ACPI platforms, we get the name from a device property.
+	 * This device property is for kernel internal use only and
+	 * is expected to be set by the glue code.
+	 */
+	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0)
+		return extcon_get_extcon_dev(name);
+
 	np_phy = of_parse_phandle(dev->of_node, "phys", 0);
 	np_conn = of_graph_get_remote_node(np_phy, -1, -1);
 
-- 
2.19.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
  2018-11-10 18:10 [PATCH v1 1/5] drivercore: Revert "deferral race condition fix" Andy Shevchenko
                   ` (3 preceding siblings ...)
  2018-11-10 18:11 ` [PATCH v1 5/5] usb: dwc3: drd: Add support for DR detection through extcon Andy Shevchenko
@ 2018-11-10 18:26 ` Greg Kroah-Hartman
  2018-11-10 18:36   ` Andy Shevchenko
  2018-11-11 13:04 ` Andy Shevchenko
  2018-11-12 16:11 ` Peter Ujfalusi
  6 siblings, 1 reply; 32+ messages in thread
From: Greg Kroah-Hartman @ 2018-11-10 18:26 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: MyungJoo Ham, Chanwoo Choi, linux-usb, Felipe Balbi,
	Guenter Roeck, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, Sebastian Reichel, linux-omap, Darren Hart,
	platform-driver-x86, linux-kernel, Chen-Yu Tsai, Hans de Goede,
	Grant Likely, Peter Ujfalusi, Mark Brown, Andrzej Hajda

On Sat, Nov 10, 2018 at 08:10:57PM +0200, Andy Shevchenko wrote:
> Consider the following scenario.
> 
> There are two independent devices coupled together by functional dependencies:
>  - USB OTG (dwc3-pci)
>  - extcon (tested with extcon-intel-mrfld, not yet in upstream)
> 
> Each of the driver services a corresponding device is built as a module. In the
> Buildroot environment the modules are probed by alphabetical ordering of their
> modaliases. The latter comes to the case when USB OTG driver will be probed
> first followed by extcon one.
> 
> So, if the platform anticipates extcon device to be appeared, in the above case
> we will get deferred probe of USB OTG, because of ordering.
> 
> Now, a cherry on top of the cake, the deferred probing list contains
> the only two modules, i.e. USB OTG and extcon. Due to above circumstances,
> values in the local_trigger_count and deferred_trigger_count are not the same,
> and thus provokes deferred probe triggering again and again.
> 
> ...
> [   20.678332] platform dwc3.0.auto: Retrying from deferred list
> [   20.694743] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> [   20.701254] platform dwc3.0.auto: Added to deferred list
> [   20.706620] platform dwc3.0.auto: driver_deferred_probe_add_trigger 1 2
> [   20.713732] platform dwc3.0.auto: Retrying from deferred list
> [   20.730035] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> [   20.736540] platform dwc3.0.auto: Added to deferred list
> [   20.741889] platform dwc3.0.auto: driver_deferred_probe_add_trigger 3 4
> [   20.748991] platform dwc3.0.auto: Retrying from deferred list
> [   20.765416] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> [   20.771914] platform dwc3.0.auto: Added to deferred list
> [   20.777279] platform dwc3.0.auto: driver_deferred_probe_add_trigger 5 6
> ...
> 
> Deeper investigation shows the culprit commit 58b116bce136
> ("drivercore: deferral race condition fix") which was dedicated to fix some
> other issue while bringing a regression.
> 
> This reverts commit 58b116bce13612e5aa6fcd49ecbd4cf8bb59e835 for good until
> we will have better solution.
> 
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/base/dd.c | 27 ++-------------------------
>  1 file changed, 2 insertions(+), 25 deletions(-)

Shouldn't there be a "Fixes:" line and cc: stable here?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
  2018-11-10 18:26 ` [PATCH v1 1/5] drivercore: Revert "deferral race condition fix" Greg Kroah-Hartman
@ 2018-11-10 18:36   ` Andy Shevchenko
  2018-11-11 11:45     ` Hans de Goede
  0 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-10 18:36 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: MyungJoo Ham, Chanwoo Choi, linux-usb, Felipe Balbi,
	Guenter Roeck, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, Sebastian Reichel, linux-omap, Darren Hart,
	platform-driver-x86, linux-kernel, Chen-Yu Tsai, Hans de Goede,
	Grant Likely, Peter Ujfalusi, Mark Brown, Andrzej Hajda

On Sat, Nov 10, 2018 at 10:26:22AM -0800, Greg Kroah-Hartman wrote:
> On Sat, Nov 10, 2018 at 08:10:57PM +0200, Andy Shevchenko wrote:
> > Consider the following scenario.
> > 
> > There are two independent devices coupled together by functional dependencies:
> >  - USB OTG (dwc3-pci)
> >  - extcon (tested with extcon-intel-mrfld, not yet in upstream)
> > 
> > Each of the driver services a corresponding device is built as a module. In the
> > Buildroot environment the modules are probed by alphabetical ordering of their
> > modaliases. The latter comes to the case when USB OTG driver will be probed
> > first followed by extcon one.
> > 
> > So, if the platform anticipates extcon device to be appeared, in the above case
> > we will get deferred probe of USB OTG, because of ordering.
> > 
> > Now, a cherry on top of the cake, the deferred probing list contains
> > the only two modules, i.e. USB OTG and extcon. Due to above circumstances,
> > values in the local_trigger_count and deferred_trigger_count are not the same,
> > and thus provokes deferred probe triggering again and again.
> > 
> > ...
> > [   20.678332] platform dwc3.0.auto: Retrying from deferred list
> > [   20.694743] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> > [   20.701254] platform dwc3.0.auto: Added to deferred list
> > [   20.706620] platform dwc3.0.auto: driver_deferred_probe_add_trigger 1 2
> > [   20.713732] platform dwc3.0.auto: Retrying from deferred list
> > [   20.730035] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> > [   20.736540] platform dwc3.0.auto: Added to deferred list
> > [   20.741889] platform dwc3.0.auto: driver_deferred_probe_add_trigger 3 4
> > [   20.748991] platform dwc3.0.auto: Retrying from deferred list
> > [   20.765416] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> > [   20.771914] platform dwc3.0.auto: Added to deferred list
> > [   20.777279] platform dwc3.0.auto: driver_deferred_probe_add_trigger 5 6
> > ...
> > 
> > Deeper investigation shows the culprit commit 58b116bce136
> > ("drivercore: deferral race condition fix") which was dedicated to fix some
> > other issue while bringing a regression.
> > 
> > This reverts commit 58b116bce13612e5aa6fcd49ecbd4cf8bb59e835 for good until
> > we will have better solution.
> > 
> > Cc: Grant Likely <grant.likely@linaro.org>
> > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Felipe Balbi <balbi@kernel.org>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/base/dd.c | 27 ++-------------------------
> >  1 file changed, 2 insertions(+), 25 deletions(-)
> 
> Shouldn't there be a "Fixes:" line and cc: stable here?

I'm not sure (yet). I would like to hear from people first, especially from
Grant (I spoke to him already for the matters at ELCE in Edinburg).

Perhaps, Hans can have a chance to test this and comment on.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-10 18:10 ` [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found Andy Shevchenko
@ 2018-11-10 18:39   ` Guenter Roeck
  2018-11-11  0:06   ` Sebastian Reichel
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 32+ messages in thread
From: Guenter Roeck @ 2018-11-10 18:39 UTC (permalink / raw)
  To: Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-usb,
	Felipe Balbi, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, Sebastian Reichel, linux-omap, Darren Hart,
	platform-driver-x86, Greg Kroah-Hartman, linux-kernel,
	Chen-Yu Tsai, Hans de Goede

On 11/10/18 10:10 AM, Andy Shevchenko wrote:
> All current users of extcon_get_extcon_dev() API considers
> an extcon device a mandatory to appear. Thus, they all convert
> NULL pointer to -EPROBE_DEFER error code.
> 
> There is one more caller anticipated with the same requirements.
> 
> To decrease a code duplication and a burden to the callers,
> return -EPROBE_DEFER directly from extcon_get_extcon_dev().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/extcon/extcon-axp288.c        | 4 ++--
>   drivers/extcon/extcon.c               | 2 +-
>   drivers/power/supply/axp288_charger.c | 8 ++++----
>   drivers/usb/phy/phy-omap-otg.c        | 6 +++---
>   drivers/usb/typec/tcpm/fusb302.c      | 4 ++--
>   5 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index a983708b77a6..3472d3b756ed 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -360,8 +360,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>   		name = acpi_dev_get_first_match_name("INT3496", NULL, -1);
>   		if (name) {
>   			info->id_extcon = extcon_get_extcon_dev(name);
> -			if (!info->id_extcon)
> -				return -EPROBE_DEFER;
> +			if (IS_ERR(info->id_extcon))
> +				return PTR_ERR(info->id_extcon);
>   
>   			dev_info(dev, "controlling USB role\n");
>   		} else {
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 5ab0498be652..2bd0f2f33f05 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -884,7 +884,7 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>   		if (!strcmp(sd->name, extcon_name))
>   			goto out;
>   	}
> -	sd = NULL;
> +	sd = ERR_PTR(-EPROBE_DEFER);
>   out:
>   	mutex_unlock(&extcon_dev_list_lock);
>   	return sd;
> diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
> index 735658ee1c60..8558577fccf5 100644
> --- a/drivers/power/supply/axp288_charger.c
> +++ b/drivers/power/supply/axp288_charger.c
> @@ -768,17 +768,17 @@ static int axp288_charger_probe(struct platform_device *pdev)
>   	info->regmap_irqc = axp20x->regmap_irqc;
>   
>   	info->cable.edev = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME);
> -	if (info->cable.edev == NULL) {
> +	if (IS_ERR(info->cable.edev)) {
>   		dev_dbg(&pdev->dev, "%s is not ready, probe deferred\n",
>   			AXP288_EXTCON_DEV_NAME);
> -		return -EPROBE_DEFER;
> +		return PTR_ERR(info->cable.edev);
>   	}
>   
>   	if (acpi_dev_present(USB_HOST_EXTCON_HID, NULL, -1)) {
>   		info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_NAME);
> -		if (info->otg.cable == NULL) {
> +		if (IS_ERR(info->otg.cable)) {
>   			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
> -			return -EPROBE_DEFER;
> +			return PTR_ERR(info->otg.cable);
>   		}
>   		dev_info(&pdev->dev,
>   			 "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
> diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c
> index ee0863c6553e..605314ddcd3d 100644
> --- a/drivers/usb/phy/phy-omap-otg.c
> +++ b/drivers/usb/phy/phy-omap-otg.c
> @@ -91,12 +91,12 @@ static int omap_otg_probe(struct platform_device *pdev)
>   	int ret;
>   	u32 rev;
>   
> -	if (!config || !config->extcon)
> +	if (!config)
>   		return -ENODEV;
>   
>   	extcon = extcon_get_extcon_dev(config->extcon);
> -	if (!extcon)
> -		return -EPROBE_DEFER;
> +	if (IS_ERR(extcon))
> +		return PTR_ERR(extcon);
>   
>   	otg_dev = devm_kzalloc(&pdev->dev, sizeof(*otg_dev), GFP_KERNEL);
>   	if (!otg_dev)
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 43b64d9309d0..6d332066202b 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -1767,8 +1767,8 @@ static int fusb302_probe(struct i2c_client *client,
>   	 */
>   	if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) {
>   		chip->extcon = extcon_get_extcon_dev(name);
> -		if (!chip->extcon)
> -			return -EPROBE_DEFER;
> +		if (IS_ERR(chip->extcon))
> +			return PTR_ERR(chip->extcon);
>   	}
>   
>   	chip->vbus = devm_regulator_get(chip->dev, "vbus");
> 


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 3/5] staging: typec: fusb302: Rename fcs,extcon-name to linux,extcon-name
  2018-11-10 18:10 ` [PATCH v1 3/5] staging: typec: fusb302: Rename fcs,extcon-name to linux,extcon-name Andy Shevchenko
@ 2018-11-10 18:40   ` Guenter Roeck
  0 siblings, 0 replies; 32+ messages in thread
From: Guenter Roeck @ 2018-11-10 18:40 UTC (permalink / raw)
  To: Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-usb,
	Felipe Balbi, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, Sebastian Reichel, linux-omap, Darren Hart,
	platform-driver-x86, Greg Kroah-Hartman, linux-kernel,
	Chen-Yu Tsai, Hans de Goede

On 11/10/18 10:10 AM, Andy Shevchenko wrote:
> Since we are going to use the same in Designware USB 3 driver,
> rename the property to be consistent across the drivers.
> 
> No functional change intended.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/platform/x86/intel_cht_int33fe.c | 2 +-
>   drivers/usb/typec/tcpm/fusb302.c         | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> index 464fe93657b5..87cbf18d0305 100644
> --- a/drivers/platform/x86/intel_cht_int33fe.c
> +++ b/drivers/platform/x86/intel_cht_int33fe.c
> @@ -79,7 +79,7 @@ static const struct property_entry max17047_props[] = {
>   };
>   
>   static const struct property_entry fusb302_props[] = {
> -	PROPERTY_ENTRY_STRING("fcs,extcon-name", "cht_wcove_pwrsrc"),
> +	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
>   	PROPERTY_ENTRY_U32("fcs,max-sink-microvolt", 12000000),
>   	PROPERTY_ENTRY_U32("fcs,max-sink-microamp",   3000000),
>   	PROPERTY_ENTRY_U32("fcs,max-sink-microwatt", 36000000),
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 6d332066202b..62816ac571a0 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -1765,7 +1765,7 @@ static int fusb302_probe(struct i2c_client *client,
>   	 * to be set by the platform code which also registers the i2c client
>   	 * for the fusb302.
>   	 */
> -	if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) {
> +	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0) {
>   		chip->extcon = extcon_get_extcon_dev(name);
>   		if (IS_ERR(chip->extcon))
>   			return PTR_ERR(chip->extcon);
> 


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-10 18:10 ` [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found Andy Shevchenko
  2018-11-10 18:39   ` Guenter Roeck
@ 2018-11-11  0:06   ` Sebastian Reichel
  2018-11-12  0:24   ` Chanwoo Choi
  2018-11-12 11:47   ` Heikki Krogerus
  3 siblings, 0 replies; 32+ messages in thread
From: Sebastian Reichel @ 2018-11-11  0:06 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: MyungJoo Ham, Chanwoo Choi, linux-usb, Felipe Balbi,
	Guenter Roeck, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, linux-omap, Darren Hart, platform-driver-x86,
	Greg Kroah-Hartman, linux-kernel, Chen-Yu Tsai, Hans de Goede

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

Hi,

On Sat, Nov 10, 2018 at 08:10:58PM +0200, Andy Shevchenko wrote:
> All current users of extcon_get_extcon_dev() API considers
> an extcon device a mandatory to appear. Thus, they all convert
> NULL pointer to -EPROBE_DEFER error code.
> 
> There is one more caller anticipated with the same requirements.
> 
> To decrease a code duplication and a burden to the callers,
> return -EPROBE_DEFER directly from extcon_get_extcon_dev().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/extcon/extcon-axp288.c        | 4 ++--
>  drivers/extcon/extcon.c               | 2 +-
>  drivers/power/supply/axp288_charger.c | 8 ++++----
>  drivers/usb/phy/phy-omap-otg.c        | 6 +++---
>  drivers/usb/typec/tcpm/fusb302.c      | 4 ++--
>  5 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index a983708b77a6..3472d3b756ed 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -360,8 +360,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>  		name = acpi_dev_get_first_match_name("INT3496", NULL, -1);
>  		if (name) {
>  			info->id_extcon = extcon_get_extcon_dev(name);
> -			if (!info->id_extcon)
> -				return -EPROBE_DEFER;
> +			if (IS_ERR(info->id_extcon))
> +				return PTR_ERR(info->id_extcon);
>  
>  			dev_info(dev, "controlling USB role\n");
>  		} else {
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 5ab0498be652..2bd0f2f33f05 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -884,7 +884,7 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>  		if (!strcmp(sd->name, extcon_name))
>  			goto out;
>  	}
> -	sd = NULL;
> +	sd = ERR_PTR(-EPROBE_DEFER);
>  out:
>  	mutex_unlock(&extcon_dev_list_lock);
>  	return sd;
> diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
> index 735658ee1c60..8558577fccf5 100644
> --- a/drivers/power/supply/axp288_charger.c
> +++ b/drivers/power/supply/axp288_charger.c
> @@ -768,17 +768,17 @@ static int axp288_charger_probe(struct platform_device *pdev)
>  	info->regmap_irqc = axp20x->regmap_irqc;
>  
>  	info->cable.edev = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME);
> -	if (info->cable.edev == NULL) {
> +	if (IS_ERR(info->cable.edev)) {
>  		dev_dbg(&pdev->dev, "%s is not ready, probe deferred\n",
>  			AXP288_EXTCON_DEV_NAME);
> -		return -EPROBE_DEFER;
> +		return PTR_ERR(info->cable.edev);
>  	}
>  
>  	if (acpi_dev_present(USB_HOST_EXTCON_HID, NULL, -1)) {
>  		info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_NAME);
> -		if (info->otg.cable == NULL) {
> +		if (IS_ERR(info->otg.cable)) {
>  			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
> -			return -EPROBE_DEFER;
> +			return PTR_ERR(info->otg.cable);
>  		}
>  		dev_info(&pdev->dev,
>  			 "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
> diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c
> index ee0863c6553e..605314ddcd3d 100644
> --- a/drivers/usb/phy/phy-omap-otg.c
> +++ b/drivers/usb/phy/phy-omap-otg.c
> @@ -91,12 +91,12 @@ static int omap_otg_probe(struct platform_device *pdev)
>  	int ret;
>  	u32 rev;
>  
> -	if (!config || !config->extcon)
> +	if (!config)
>  		return -ENODEV;
>  
>  	extcon = extcon_get_extcon_dev(config->extcon);
> -	if (!extcon)
> -		return -EPROBE_DEFER;
> +	if (IS_ERR(extcon))
> +		return PTR_ERR(extcon);
>  
>  	otg_dev = devm_kzalloc(&pdev->dev, sizeof(*otg_dev), GFP_KERNEL);
>  	if (!otg_dev)
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 43b64d9309d0..6d332066202b 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -1767,8 +1767,8 @@ static int fusb302_probe(struct i2c_client *client,
>  	 */
>  	if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) {
>  		chip->extcon = extcon_get_extcon_dev(name);
> -		if (!chip->extcon)
> -			return -EPROBE_DEFER;
> +		if (IS_ERR(chip->extcon))
> +			return PTR_ERR(chip->extcon);
>  	}
>  
>  	chip->vbus = devm_regulator_get(chip->dev, "vbus");
> -- 
> 2.19.1
> 

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
  2018-11-10 18:36   ` Andy Shevchenko
@ 2018-11-11 11:45     ` Hans de Goede
  0 siblings, 0 replies; 32+ messages in thread
From: Hans de Goede @ 2018-11-11 11:45 UTC (permalink / raw)
  To: Andy Shevchenko, Greg Kroah-Hartman
  Cc: MyungJoo Ham, Chanwoo Choi, linux-usb, Felipe Balbi,
	Guenter Roeck, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, Sebastian Reichel, linux-omap, Darren Hart,
	platform-driver-x86, linux-kernel, Chen-Yu Tsai, Grant Likely,
	Peter Ujfalusi, Mark Brown, Andrzej Hajda

Hi,

On 11/10/18 7:36 PM, Andy Shevchenko wrote:
> On Sat, Nov 10, 2018 at 10:26:22AM -0800, Greg Kroah-Hartman wrote:
>> On Sat, Nov 10, 2018 at 08:10:57PM +0200, Andy Shevchenko wrote:
>>> Consider the following scenario.
>>>
>>> There are two independent devices coupled together by functional dependencies:
>>>   - USB OTG (dwc3-pci)
>>>   - extcon (tested with extcon-intel-mrfld, not yet in upstream)
>>>
>>> Each of the driver services a corresponding device is built as a module. In the
>>> Buildroot environment the modules are probed by alphabetical ordering of their
>>> modaliases. The latter comes to the case when USB OTG driver will be probed
>>> first followed by extcon one.
>>>
>>> So, if the platform anticipates extcon device to be appeared, in the above case
>>> we will get deferred probe of USB OTG, because of ordering.
>>>
>>> Now, a cherry on top of the cake, the deferred probing list contains
>>> the only two modules, i.e. USB OTG and extcon. Due to above circumstances,
>>> values in the local_trigger_count and deferred_trigger_count are not the same,
>>> and thus provokes deferred probe triggering again and again.
>>>
>>> ...
>>> [   20.678332] platform dwc3.0.auto: Retrying from deferred list
>>> [   20.694743] platform dwc3.0.auto: Driver dwc3 requests probe deferral
>>> [   20.701254] platform dwc3.0.auto: Added to deferred list
>>> [   20.706620] platform dwc3.0.auto: driver_deferred_probe_add_trigger 1 2
>>> [   20.713732] platform dwc3.0.auto: Retrying from deferred list
>>> [   20.730035] platform dwc3.0.auto: Driver dwc3 requests probe deferral
>>> [   20.736540] platform dwc3.0.auto: Added to deferred list
>>> [   20.741889] platform dwc3.0.auto: driver_deferred_probe_add_trigger 3 4
>>> [   20.748991] platform dwc3.0.auto: Retrying from deferred list
>>> [   20.765416] platform dwc3.0.auto: Driver dwc3 requests probe deferral
>>> [   20.771914] platform dwc3.0.auto: Added to deferred list
>>> [   20.777279] platform dwc3.0.auto: driver_deferred_probe_add_trigger 5 6
>>> ...
>>>
>>> Deeper investigation shows the culprit commit 58b116bce136
>>> ("drivercore: deferral race condition fix") which was dedicated to fix some
>>> other issue while bringing a regression.
>>>
>>> This reverts commit 58b116bce13612e5aa6fcd49ecbd4cf8bb59e835 for good until
>>> we will have better solution.
>>>
>>> Cc: Grant Likely <grant.likely@linaro.org>
>>> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Mark Brown <broonie@kernel.org>
>>> Cc: Felipe Balbi <balbi@kernel.org>
>>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> ---
>>>   drivers/base/dd.c | 27 ++-------------------------
>>>   1 file changed, 2 insertions(+), 25 deletions(-)
>>
>> Shouldn't there be a "Fixes:" line and cc: stable here?
> 
> I'm not sure (yet). I would like to hear from people first, especially from
> Grant (I spoke to him already for the matters at ELCE in Edinburg).
> 
> Perhaps, Hans can have a chance to test this and comment on.

I'm currently hitting a regression in 4.20-rc1 which causes it to not
boot on my Cherry Trail test devices (sdhci driver times out so it
cannot find its root filesystem).

I'm currently debugging this (it looks like I need todo a full bisect to
find the cause) once I've that working, if I hit something which look
likes you've described I will give this patch a test. But currently I
cannot reproduce the problem you describe.

As for patches 2-55, they look good to me:

Ackeed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
  2018-11-10 18:10 [PATCH v1 1/5] drivercore: Revert "deferral race condition fix" Andy Shevchenko
                   ` (4 preceding siblings ...)
  2018-11-10 18:26 ` [PATCH v1 1/5] drivercore: Revert "deferral race condition fix" Greg Kroah-Hartman
@ 2018-11-11 13:04 ` Andy Shevchenko
  2018-11-11 19:26   ` Rob Herring
  2018-11-12 16:11 ` Peter Ujfalusi
  6 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-11 13:04 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Linux Kernel Mailing List,
	Hans de Goede, Grant Likely, Peter Ujfalusi, Andrzej Hajda

I seems Grant's mail delivery bounces messages. I delibirately reduced
the Cc list for sake of ping Grant in case it would pass.

On Sat, Nov 10, 2018 at 8:12 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Consider the following scenario.
>
> There are two independent devices coupled together by functional dependencies:
>  - USB OTG (dwc3-pci)
>  - extcon (tested with extcon-intel-mrfld, not yet in upstream)
>
> Each of the driver services a corresponding device is built as a module. In the
> Buildroot environment the modules are probed by alphabetical ordering of their
> modaliases. The latter comes to the case when USB OTG driver will be probed
> first followed by extcon one.
>
> So, if the platform anticipates extcon device to be appeared, in the above case
> we will get deferred probe of USB OTG, because of ordering.
>
> Now, a cherry on top of the cake, the deferred probing list contains
> the only two modules, i.e. USB OTG and extcon. Due to above circumstances,
> values in the local_trigger_count and deferred_trigger_count are not the same,
> and thus provokes deferred probe triggering again and again.
>
> ...
> [   20.678332] platform dwc3.0.auto: Retrying from deferred list
> [   20.694743] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> [   20.701254] platform dwc3.0.auto: Added to deferred list
> [   20.706620] platform dwc3.0.auto: driver_deferred_probe_add_trigger 1 2
> [   20.713732] platform dwc3.0.auto: Retrying from deferred list
> [   20.730035] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> [   20.736540] platform dwc3.0.auto: Added to deferred list
> [   20.741889] platform dwc3.0.auto: driver_deferred_probe_add_trigger 3 4
> [   20.748991] platform dwc3.0.auto: Retrying from deferred list
> [   20.765416] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> [   20.771914] platform dwc3.0.auto: Added to deferred list
> [   20.777279] platform dwc3.0.auto: driver_deferred_probe_add_trigger 5 6
> ...
>
> Deeper investigation shows the culprit commit 58b116bce136
> ("drivercore: deferral race condition fix") which was dedicated to fix some
> other issue while bringing a regression.
>
> This reverts commit 58b116bce13612e5aa6fcd49ecbd4cf8bb59e835 for good until
> we will have better solution.
>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/base/dd.c | 27 ++-------------------------
>  1 file changed, 2 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 169412ee4ae8..9a966e45fda5 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -53,7 +53,6 @@
>  static DEFINE_MUTEX(deferred_probe_mutex);
>  static LIST_HEAD(deferred_probe_pending_list);
>  static LIST_HEAD(deferred_probe_active_list);
> -static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
>  static struct dentry *deferred_devices;
>  static bool initcalls_done;
>
> @@ -143,17 +142,6 @@ static bool driver_deferred_probe_enable = false;
>   * This functions moves all devices from the pending list to the active
>   * list and schedules the deferred probe workqueue to process them.  It
>   * should be called anytime a driver is successfully bound to a device.
> - *
> - * Note, there is a race condition in multi-threaded probe. In the case where
> - * more than one device is probing at the same time, it is possible for one
> - * probe to complete successfully while another is about to defer. If the second
> - * depends on the first, then it will get put on the pending list after the
> - * trigger event has already occurred and will be stuck there.
> - *
> - * The atomic 'deferred_trigger_count' is used to determine if a successful
> - * trigger has occurred in the midst of probing a driver. If the trigger count
> - * changes in the midst of a probe, then deferred processing should be triggered
> - * again.
>   */
>  static void driver_deferred_probe_trigger(void)
>  {
> @@ -166,7 +154,6 @@ static void driver_deferred_probe_trigger(void)
>          * into the active list so they can be retried by the workqueue
>          */
>         mutex_lock(&deferred_probe_mutex);
> -       atomic_inc(&deferred_trigger_count);
>         list_splice_tail_init(&deferred_probe_pending_list,
>                               &deferred_probe_active_list);
>         mutex_unlock(&deferred_probe_mutex);
> @@ -434,19 +421,9 @@ EXPORT_SYMBOL_GPL(device_bind_driver);
>  static atomic_t probe_count = ATOMIC_INIT(0);
>  static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
>
> -static void driver_deferred_probe_add_trigger(struct device *dev,
> -                                             int local_trigger_count)
> -{
> -       driver_deferred_probe_add(dev);
> -       /* Did a trigger occur while probing? Need to re-trigger if yes */
> -       if (local_trigger_count != atomic_read(&deferred_trigger_count))
> -               driver_deferred_probe_trigger();
> -}
> -
>  static int really_probe(struct device *dev, struct device_driver *drv)
>  {
>         int ret = -EPROBE_DEFER;
> -       int local_trigger_count = atomic_read(&deferred_trigger_count);
>         bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
>                            !drv->suppress_bind_attrs;
>
> @@ -463,7 +440,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>
>         ret = device_links_check_suppliers(dev);
>         if (ret == -EPROBE_DEFER)
> -               driver_deferred_probe_add_trigger(dev, local_trigger_count);
> +               driver_deferred_probe_add(dev);
>         if (ret)
>                 return ret;
>
> @@ -559,7 +536,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>         case -EPROBE_DEFER:
>                 /* Driver requested deferred probing */
>                 dev_dbg(dev, "Driver %s requests probe deferral\n", drv->name);
> -               driver_deferred_probe_add_trigger(dev, local_trigger_count);
> +               driver_deferred_probe_add(dev);
>                 break;
>         case -ENODEV:
>         case -ENXIO:
> --
> 2.19.1
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
  2018-11-11 13:04 ` Andy Shevchenko
@ 2018-11-11 19:26   ` Rob Herring
  2018-11-11 23:53     ` Andy Shevchenko
  2018-11-14 23:25     ` Grant Likely
  0 siblings, 2 replies; 32+ messages in thread
From: Rob Herring @ 2018-11-11 19:26 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: andriy.shevchenko, Rafael J. Wysocki, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Hans de Goede, peter.ujfalusi,
	a.hajda, Grant Likely

On Sun, Nov 11, 2018 at 7:04 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> I seems Grant's mail delivery bounces messages. I delibirately reduced
> the Cc list for sake of ping Grant in case it would pass.

That would be because he is not at Linaro anymore. Added his ARM account.

> On Sat, Nov 10, 2018 at 8:12 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Consider the following scenario.
> >
> > There are two independent devices coupled together by functional dependencies:
> >  - USB OTG (dwc3-pci)
> >  - extcon (tested with extcon-intel-mrfld, not yet in upstream)
> >
> > Each of the driver services a corresponding device is built as a module. In the
> > Buildroot environment the modules are probed by alphabetical ordering of their
> > modaliases. The latter comes to the case when USB OTG driver will be probed
> > first followed by extcon one.
> >
> > So, if the platform anticipates extcon device to be appeared, in the above case
> > we will get deferred probe of USB OTG, because of ordering.
> >
> > Now, a cherry on top of the cake, the deferred probing list contains
> > the only two modules, i.e. USB OTG and extcon. Due to above circumstances,
> > values in the local_trigger_count and deferred_trigger_count are not the same,
> > and thus provokes deferred probe triggering again and again.
> >
> > ...
> > [   20.678332] platform dwc3.0.auto: Retrying from deferred list
> > [   20.694743] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> > [   20.701254] platform dwc3.0.auto: Added to deferred list
> > [   20.706620] platform dwc3.0.auto: driver_deferred_probe_add_trigger 1 2
> > [   20.713732] platform dwc3.0.auto: Retrying from deferred list
> > [   20.730035] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> > [   20.736540] platform dwc3.0.auto: Added to deferred list
> > [   20.741889] platform dwc3.0.auto: driver_deferred_probe_add_trigger 3 4
> > [   20.748991] platform dwc3.0.auto: Retrying from deferred list
> > [   20.765416] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> > [   20.771914] platform dwc3.0.auto: Added to deferred list
> > [   20.777279] platform dwc3.0.auto: driver_deferred_probe_add_trigger 5 6
> > ...
> >
> > Deeper investigation shows the culprit commit 58b116bce136
> > ("drivercore: deferral race condition fix") which was dedicated to fix some
> > other issue while bringing a regression.
> >
> > This reverts commit 58b116bce13612e5aa6fcd49ecbd4cf8bb59e835 for good until
> > we will have better solution.

How is behavior that's been there for 4.5 years a regression? Aren't
we going to break Davinci that this commit was supposed to fix? What
else could be relying on current behavior?

Rob

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
  2018-11-11 19:26   ` Rob Herring
@ 2018-11-11 23:53     ` Andy Shevchenko
  2018-11-14 23:25     ` Grant Likely
  1 sibling, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-11 23:53 UTC (permalink / raw)
  To: Rob Herring
  Cc: Andy Shevchenko, Rafael J. Wysocki, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Hans de Goede, Peter Ujfalusi,
	Andrzej Hajda, grant.likely

On Sun, Nov 11, 2018 at 9:26 PM Rob Herring <robh@kernel.org> wrote:
> On Sun, Nov 11, 2018 at 7:04 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

> > I seems Grant's mail delivery bounces messages. I delibirately reduced
> > the Cc list for sake of ping Grant in case it would pass.
>
> That would be because he is not at Linaro anymore. Added his ARM account.

Thanks!

> > On Sat, Nov 10, 2018 at 8:12 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > Consider the following scenario.
> > >
> > > There are two independent devices coupled together by functional dependencies:
> > >  - USB OTG (dwc3-pci)
> > >  - extcon (tested with extcon-intel-mrfld, not yet in upstream)
> > >
> > > Each of the driver services a corresponding device is built as a module. In the
> > > Buildroot environment the modules are probed by alphabetical ordering of their
> > > modaliases. The latter comes to the case when USB OTG driver will be probed
> > > first followed by extcon one.
> > >
> > > So, if the platform anticipates extcon device to be appeared, in the above case
> > > we will get deferred probe of USB OTG, because of ordering.
> > >
> > > Now, a cherry on top of the cake, the deferred probing list contains
> > > the only two modules, i.e. USB OTG and extcon. Due to above circumstances,
> > > values in the local_trigger_count and deferred_trigger_count are not the same,
> > > and thus provokes deferred probe triggering again and again.
> > >
> > > ...
> > > [   20.678332] platform dwc3.0.auto: Retrying from deferred list
> > > [   20.694743] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> > > [   20.701254] platform dwc3.0.auto: Added to deferred list
> > > [   20.706620] platform dwc3.0.auto: driver_deferred_probe_add_trigger 1 2
> > > [   20.713732] platform dwc3.0.auto: Retrying from deferred list
> > > [   20.730035] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> > > [   20.736540] platform dwc3.0.auto: Added to deferred list
> > > [   20.741889] platform dwc3.0.auto: driver_deferred_probe_add_trigger 3 4
> > > [   20.748991] platform dwc3.0.auto: Retrying from deferred list
> > > [   20.765416] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> > > [   20.771914] platform dwc3.0.auto: Added to deferred list
> > > [   20.777279] platform dwc3.0.auto: driver_deferred_probe_add_trigger 5 6
> > > ...
> > >
> > > Deeper investigation shows the culprit commit 58b116bce136
> > > ("drivercore: deferral race condition fix") which was dedicated to fix some
> > > other issue while bringing a regression.
> > >
> > > This reverts commit 58b116bce13612e5aa6fcd49ecbd4cf8bb59e835 for good until
> > > we will have better solution.
>
> How is behavior that's been there for 4.5 years a regression? Aren't
> we going to break Davinci that this commit was supposed to fix? What
> else could be relying on current behavior?

I guess we are purerly on a side of luck. I don't remember people
build usb and extcon as modules
and using Buildroot, or anything that probes modules let's say,in
"arbitrary" way.
(That is one of the reason why I didn't dare to put Fixes tag there).
Nevertheless, I have actual issue. I'm all ears to know about another
possibility
how to fix the issue on my side.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-10 18:10 ` [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found Andy Shevchenko
  2018-11-10 18:39   ` Guenter Roeck
  2018-11-11  0:06   ` Sebastian Reichel
@ 2018-11-12  0:24   ` Chanwoo Choi
  2018-11-13 23:52     ` Chanwoo Choi
  2018-11-12 11:47   ` Heikki Krogerus
  3 siblings, 1 reply; 32+ messages in thread
From: Chanwoo Choi @ 2018-11-12  0:24 UTC (permalink / raw)
  To: Andy Shevchenko, MyungJoo Ham, linux-usb, Felipe Balbi,
	Guenter Roeck, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, Sebastian Reichel, linux-omap, Darren Hart,
	platform-driver-x86, Greg Kroah-Hartman, linux-kernel,
	Chen-Yu Tsai, Hans de Goede

Hi Andy,

On 2018년 11월 11일 03:10, Andy Shevchenko wrote:
> All current users of extcon_get_extcon_dev() API considers
> an extcon device a mandatory to appear. Thus, they all convert
> NULL pointer to -EPROBE_DEFER error code.
> 
> There is one more caller anticipated with the same requirements.
> 
> To decrease a code duplication and a burden to the callers,
> return -EPROBE_DEFER directly from extcon_get_extcon_dev().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/extcon/extcon-axp288.c        | 4 ++--
>  drivers/extcon/extcon.c               | 2 +-
>  drivers/power/supply/axp288_charger.c | 8 ++++----
>  drivers/usb/phy/phy-omap-otg.c        | 6 +++---
>  drivers/usb/typec/tcpm/fusb302.c      | 4 ++--
>  5 files changed, 12 insertions(+), 12 deletions(-)

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

Best Regards,
Chanwoo Choi

> 
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index a983708b77a6..3472d3b756ed 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -360,8 +360,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>  		name = acpi_dev_get_first_match_name("INT3496", NULL, -1);
>  		if (name) {
>  			info->id_extcon = extcon_get_extcon_dev(name);
> -			if (!info->id_extcon)
> -				return -EPROBE_DEFER;
> +			if (IS_ERR(info->id_extcon))
> +				return PTR_ERR(info->id_extcon);
>  
>  			dev_info(dev, "controlling USB role\n");
>  		} else {
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 5ab0498be652..2bd0f2f33f05 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -884,7 +884,7 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>  		if (!strcmp(sd->name, extcon_name))
>  			goto out;
>  	}
> -	sd = NULL;
> +	sd = ERR_PTR(-EPROBE_DEFER);
>  out:
>  	mutex_unlock(&extcon_dev_list_lock);
>  	return sd;
> diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
> index 735658ee1c60..8558577fccf5 100644
> --- a/drivers/power/supply/axp288_charger.c
> +++ b/drivers/power/supply/axp288_charger.c
> @@ -768,17 +768,17 @@ static int axp288_charger_probe(struct platform_device *pdev)
>  	info->regmap_irqc = axp20x->regmap_irqc;
>  
>  	info->cable.edev = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME);
> -	if (info->cable.edev == NULL) {
> +	if (IS_ERR(info->cable.edev)) {
>  		dev_dbg(&pdev->dev, "%s is not ready, probe deferred\n",
>  			AXP288_EXTCON_DEV_NAME);
> -		return -EPROBE_DEFER;
> +		return PTR_ERR(info->cable.edev);
>  	}
>  
>  	if (acpi_dev_present(USB_HOST_EXTCON_HID, NULL, -1)) {
>  		info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_NAME);
> -		if (info->otg.cable == NULL) {
> +		if (IS_ERR(info->otg.cable)) {
>  			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
> -			return -EPROBE_DEFER;
> +			return PTR_ERR(info->otg.cable);
>  		}
>  		dev_info(&pdev->dev,
>  			 "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
> diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c
> index ee0863c6553e..605314ddcd3d 100644
> --- a/drivers/usb/phy/phy-omap-otg.c
> +++ b/drivers/usb/phy/phy-omap-otg.c
> @@ -91,12 +91,12 @@ static int omap_otg_probe(struct platform_device *pdev)
>  	int ret;
>  	u32 rev;
>  
> -	if (!config || !config->extcon)
> +	if (!config)
>  		return -ENODEV;
>  
>  	extcon = extcon_get_extcon_dev(config->extcon);
> -	if (!extcon)
> -		return -EPROBE_DEFER;
> +	if (IS_ERR(extcon))
> +		return PTR_ERR(extcon);
>  
>  	otg_dev = devm_kzalloc(&pdev->dev, sizeof(*otg_dev), GFP_KERNEL);
>  	if (!otg_dev)
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 43b64d9309d0..6d332066202b 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -1767,8 +1767,8 @@ static int fusb302_probe(struct i2c_client *client,
>  	 */
>  	if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) {
>  		chip->extcon = extcon_get_extcon_dev(name);
> -		if (!chip->extcon)
> -			return -EPROBE_DEFER;
> +		if (IS_ERR(chip->extcon))
> +			return PTR_ERR(chip->extcon);
>  	}
>  
>  	chip->vbus = devm_regulator_get(chip->dev, "vbus");
> 


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 4/5] usb: dwc3: drd: Switch to device property for 'extcon' handling
  2018-11-10 18:11 ` [PATCH v1 4/5] usb: dwc3: drd: Switch to device property for 'extcon' handling Andy Shevchenko
@ 2018-11-12 11:47   ` Felipe Balbi
  0 siblings, 0 replies; 32+ messages in thread
From: Felipe Balbi @ 2018-11-12 11:47 UTC (permalink / raw)
  To: Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-usb,
	Guenter Roeck, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, Sebastian Reichel, linux-omap, Darren Hart,
	platform-driver-x86, Greg Kroah-Hartman, linux-kernel,
	Chen-Yu Tsai, Hans de Goede
  Cc: Andy Shevchenko

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


Hi,

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> Switch to device property for 'extcon' handling.
> No functional change intended.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/usb/dwc3/drd.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index e7e421521a34..5dc4cddd5b68 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -10,6 +10,7 @@
>  #include <linux/extcon.h>
>  #include <linux/of_graph.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  
>  #include "debug.h"
>  #include "core.h"
> @@ -485,8 +486,8 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
>  	struct device_node *np_phy, *np_conn;
>  	struct extcon_dev *edev;
>  
> -	if (of_property_read_bool(dev->of_node, "extcon"))
> -		return extcon_get_edev_by_phandle(dwc->dev, 0);
> +	if (device_property_read_bool(dev, "extcon"))
> +		return extcon_get_edev_by_phandle(dev, 0);

No reservations against any of the patches. What is the idea for getting
these merged upstream? I don't mind taking 4-5 through my tree, but what
are the other folks considering?

-- 
balbi

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-10 18:10 ` [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found Andy Shevchenko
                     ` (2 preceding siblings ...)
  2018-11-12  0:24   ` Chanwoo Choi
@ 2018-11-12 11:47   ` Heikki Krogerus
  3 siblings, 0 replies; 32+ messages in thread
From: Heikki Krogerus @ 2018-11-12 11:47 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: MyungJoo Ham, Chanwoo Choi, linux-usb, Felipe Balbi,
	Guenter Roeck, Roger Quadros, linux-pm, Rafael J. Wysocki,
	Sebastian Reichel, linux-omap, Darren Hart, platform-driver-x86,
	Greg Kroah-Hartman, linux-kernel, Chen-Yu Tsai, Hans de Goede

On Sat, Nov 10, 2018 at 08:10:58PM +0200, Andy Shevchenko wrote:
> All current users of extcon_get_extcon_dev() API considers
> an extcon device a mandatory to appear. Thus, they all convert
> NULL pointer to -EPROBE_DEFER error code.
> 
> There is one more caller anticipated with the same requirements.
> 
> To decrease a code duplication and a burden to the callers,
> return -EPROBE_DEFER directly from extcon_get_extcon_dev().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/extcon/extcon-axp288.c        | 4 ++--
>  drivers/extcon/extcon.c               | 2 +-
>  drivers/power/supply/axp288_charger.c | 8 ++++----
>  drivers/usb/phy/phy-omap-otg.c        | 6 +++---
>  drivers/usb/typec/tcpm/fusb302.c      | 4 ++--
>  5 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index a983708b77a6..3472d3b756ed 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -360,8 +360,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>  		name = acpi_dev_get_first_match_name("INT3496", NULL, -1);
>  		if (name) {
>  			info->id_extcon = extcon_get_extcon_dev(name);
> -			if (!info->id_extcon)
> -				return -EPROBE_DEFER;
> +			if (IS_ERR(info->id_extcon))
> +				return PTR_ERR(info->id_extcon);
>  
>  			dev_info(dev, "controlling USB role\n");
>  		} else {
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 5ab0498be652..2bd0f2f33f05 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -884,7 +884,7 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>  		if (!strcmp(sd->name, extcon_name))
>  			goto out;
>  	}
> -	sd = NULL;
> +	sd = ERR_PTR(-EPROBE_DEFER);
>  out:
>  	mutex_unlock(&extcon_dev_list_lock);
>  	return sd;
> diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
> index 735658ee1c60..8558577fccf5 100644
> --- a/drivers/power/supply/axp288_charger.c
> +++ b/drivers/power/supply/axp288_charger.c
> @@ -768,17 +768,17 @@ static int axp288_charger_probe(struct platform_device *pdev)
>  	info->regmap_irqc = axp20x->regmap_irqc;
>  
>  	info->cable.edev = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME);
> -	if (info->cable.edev == NULL) {
> +	if (IS_ERR(info->cable.edev)) {
>  		dev_dbg(&pdev->dev, "%s is not ready, probe deferred\n",
>  			AXP288_EXTCON_DEV_NAME);
> -		return -EPROBE_DEFER;
> +		return PTR_ERR(info->cable.edev);
>  	}
>  
>  	if (acpi_dev_present(USB_HOST_EXTCON_HID, NULL, -1)) {
>  		info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_NAME);
> -		if (info->otg.cable == NULL) {
> +		if (IS_ERR(info->otg.cable)) {
>  			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
> -			return -EPROBE_DEFER;
> +			return PTR_ERR(info->otg.cable);
>  		}
>  		dev_info(&pdev->dev,
>  			 "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
> diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c
> index ee0863c6553e..605314ddcd3d 100644
> --- a/drivers/usb/phy/phy-omap-otg.c
> +++ b/drivers/usb/phy/phy-omap-otg.c
> @@ -91,12 +91,12 @@ static int omap_otg_probe(struct platform_device *pdev)
>  	int ret;
>  	u32 rev;
>  
> -	if (!config || !config->extcon)
> +	if (!config)
>  		return -ENODEV;
>  
>  	extcon = extcon_get_extcon_dev(config->extcon);
> -	if (!extcon)
> -		return -EPROBE_DEFER;
> +	if (IS_ERR(extcon))
> +		return PTR_ERR(extcon);
>  
>  	otg_dev = devm_kzalloc(&pdev->dev, sizeof(*otg_dev), GFP_KERNEL);
>  	if (!otg_dev)
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 43b64d9309d0..6d332066202b 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -1767,8 +1767,8 @@ static int fusb302_probe(struct i2c_client *client,
>  	 */
>  	if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) {
>  		chip->extcon = extcon_get_extcon_dev(name);
> -		if (!chip->extcon)
> -			return -EPROBE_DEFER;
> +		if (IS_ERR(chip->extcon))
> +			return PTR_ERR(chip->extcon);
>  	}
>  
>  	chip->vbus = devm_regulator_get(chip->dev, "vbus");
> -- 
> 2.19.1

-- 
heikki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
  2018-11-10 18:10 [PATCH v1 1/5] drivercore: Revert "deferral race condition fix" Andy Shevchenko
                   ` (5 preceding siblings ...)
  2018-11-11 13:04 ` Andy Shevchenko
@ 2018-11-12 16:11 ` Peter Ujfalusi
  2018-11-14  0:33   ` Mark Brown
  6 siblings, 1 reply; 32+ messages in thread
From: Peter Ujfalusi @ 2018-11-12 16:11 UTC (permalink / raw)
  To: Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-usb,
	Felipe Balbi, Guenter Roeck, Heikki Krogerus, Roger Quadros,
	linux-pm, Rafael J. Wysocki, Sebastian Reichel, linux-omap,
	Darren Hart, platform-driver-x86, Greg Kroah-Hartman,
	linux-kernel, Chen-Yu Tsai, Hans de Goede
  Cc: Grant Likely, Mark Brown, Andrzej Hajda

Hi Andy,

On 2018-11-10 20:10, Andy Shevchenko wrote:
> Consider the following scenario.
> 
> There are two independent devices coupled together by functional dependencies:
>  - USB OTG (dwc3-pci)
>  - extcon (tested with extcon-intel-mrfld, not yet in upstream)
> 
> Each of the driver services a corresponding device is built as a module. In the
> Buildroot environment the modules are probed by alphabetical ordering of their
> modaliases. The latter comes to the case when USB OTG driver will be probed
> first followed by extcon one.
> 
> So, if the platform anticipates extcon device to be appeared, in the above case
> we will get deferred probe of USB OTG, because of ordering.
> 
> Now, a cherry on top of the cake, the deferred probing list contains
> the only two modules, i.e. USB OTG and extcon. Due to above circumstances,
> values in the local_trigger_count and deferred_trigger_count are not the same,
> and thus provokes deferred probe triggering again and again.
> 
> ...
> [   20.678332] platform dwc3.0.auto: Retrying from deferred list
> [   20.694743] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> [   20.701254] platform dwc3.0.auto: Added to deferred list
> [   20.706620] platform dwc3.0.auto: driver_deferred_probe_add_trigger 1 2
> [   20.713732] platform dwc3.0.auto: Retrying from deferred list
> [   20.730035] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> [   20.736540] platform dwc3.0.auto: Added to deferred list
> [   20.741889] platform dwc3.0.auto: driver_deferred_probe_add_trigger 3 4
> [   20.748991] platform dwc3.0.auto: Retrying from deferred list
> [   20.765416] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> [   20.771914] platform dwc3.0.auto: Added to deferred list
> [   20.777279] platform dwc3.0.auto: driver_deferred_probe_add_trigger 5 6
> ...
> 
> Deeper investigation shows the culprit commit 58b116bce136
> ("drivercore: deferral race condition fix") which was dedicated to fix some
> other issue while bringing a regression.
> 
> This reverts commit 58b116bce13612e5aa6fcd49ecbd4cf8bb59e835 for good until
> we will have better solution.

if we revert the commit then the original issue will re-surfaces. afaik
it was not only audio which hit the 'last driver to be probed from the
deferred list would never probe, unless we provoke the kernel to load
additional module, or remove/reload the module' issue.

Do I understand correctly that in your case you have two modules
(dwc3-pci and extcon-intel-mrfld) in a deferred probe loop, iow both of
the drivers returns -EPROBE_DEFER and they just spin?

If both is deferring, how this supposed to work?

If we revert 58b116bce13612e5aa6fcd49ecbd4cf8bb59e835, then you might be
hitting the very same issue as described by the commit:
s/davinci_evm sound.3/dwc3-pci
s/davinci-mcasp 4803c000.mcasp/extcon-intel-mrfld

Am I missing something?

> 
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/base/dd.c | 27 ++-------------------------
>  1 file changed, 2 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 169412ee4ae8..9a966e45fda5 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -53,7 +53,6 @@
>  static DEFINE_MUTEX(deferred_probe_mutex);
>  static LIST_HEAD(deferred_probe_pending_list);
>  static LIST_HEAD(deferred_probe_active_list);
> -static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
>  static struct dentry *deferred_devices;
>  static bool initcalls_done;
>  
> @@ -143,17 +142,6 @@ static bool driver_deferred_probe_enable = false;
>   * This functions moves all devices from the pending list to the active
>   * list and schedules the deferred probe workqueue to process them.  It
>   * should be called anytime a driver is successfully bound to a device.
> - *
> - * Note, there is a race condition in multi-threaded probe. In the case where
> - * more than one device is probing at the same time, it is possible for one
> - * probe to complete successfully while another is about to defer. If the second
> - * depends on the first, then it will get put on the pending list after the
> - * trigger event has already occurred and will be stuck there.
> - *
> - * The atomic 'deferred_trigger_count' is used to determine if a successful
> - * trigger has occurred in the midst of probing a driver. If the trigger count
> - * changes in the midst of a probe, then deferred processing should be triggered
> - * again.
>   */
>  static void driver_deferred_probe_trigger(void)
>  {
> @@ -166,7 +154,6 @@ static void driver_deferred_probe_trigger(void)
>  	 * into the active list so they can be retried by the workqueue
>  	 */
>  	mutex_lock(&deferred_probe_mutex);
> -	atomic_inc(&deferred_trigger_count);
>  	list_splice_tail_init(&deferred_probe_pending_list,
>  			      &deferred_probe_active_list);
>  	mutex_unlock(&deferred_probe_mutex);
> @@ -434,19 +421,9 @@ EXPORT_SYMBOL_GPL(device_bind_driver);
>  static atomic_t probe_count = ATOMIC_INIT(0);
>  static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
>  
> -static void driver_deferred_probe_add_trigger(struct device *dev,
> -					      int local_trigger_count)
> -{
> -	driver_deferred_probe_add(dev);
> -	/* Did a trigger occur while probing? Need to re-trigger if yes */
> -	if (local_trigger_count != atomic_read(&deferred_trigger_count))
> -		driver_deferred_probe_trigger();
> -}
> -
>  static int really_probe(struct device *dev, struct device_driver *drv)
>  {
>  	int ret = -EPROBE_DEFER;
> -	int local_trigger_count = atomic_read(&deferred_trigger_count);
>  	bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
>  			   !drv->suppress_bind_attrs;
>  
> @@ -463,7 +440,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  
>  	ret = device_links_check_suppliers(dev);
>  	if (ret == -EPROBE_DEFER)
> -		driver_deferred_probe_add_trigger(dev, local_trigger_count);
> +		driver_deferred_probe_add(dev);
>  	if (ret)
>  		return ret;
>  
> @@ -559,7 +536,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  	case -EPROBE_DEFER:
>  		/* Driver requested deferred probing */
>  		dev_dbg(dev, "Driver %s requests probe deferral\n", drv->name);
> -		driver_deferred_probe_add_trigger(dev, local_trigger_count);
> +		driver_deferred_probe_add(dev);
>  		break;
>  	case -ENODEV:
>  	case -ENXIO:
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-12  0:24   ` Chanwoo Choi
@ 2018-11-13 23:52     ` Chanwoo Choi
  2018-11-14  8:35       ` Andy Shevchenko
  0 siblings, 1 reply; 32+ messages in thread
From: Chanwoo Choi @ 2018-11-13 23:52 UTC (permalink / raw)
  To: Andy Shevchenko, MyungJoo Ham, linux-usb, Felipe Balbi,
	Guenter Roeck, Heikki Krogerus, Roger Quadros, linux-pm,
	Rafael J. Wysocki, Sebastian Reichel, linux-omap, Darren Hart,
	platform-driver-x86, Greg Kroah-Hartman, linux-kernel,
	Chen-Yu Tsai, Hans de Goede

Hi Andy,

I was thinking about again to change from NULL to EPROBE_DEFER.

extcon_get_extcon_dev() function was almost called in the probe function.
But, this function might be called on other position instead of probe.

ENODEV is more correct error instead of EPROBE_DEFER.

Sorry. I'll withdraw my opinion related acked-by tag until we are clarifying it.

On 2018년 11월 12일 09:24, Chanwoo Choi wrote:
> Hi Andy,
> 
> On 2018년 11월 11일 03:10, Andy Shevchenko wrote:
>> All current users of extcon_get_extcon_dev() API considers
>> an extcon device a mandatory to appear. Thus, they all convert
>> NULL pointer to -EPROBE_DEFER error code.
>>
>> There is one more caller anticipated with the same requirements.
>>
>> To decrease a code duplication and a burden to the callers,
>> return -EPROBE_DEFER directly from extcon_get_extcon_dev().
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>  drivers/extcon/extcon-axp288.c        | 4 ++--
>>  drivers/extcon/extcon.c               | 2 +-
>>  drivers/power/supply/axp288_charger.c | 8 ++++----
>>  drivers/usb/phy/phy-omap-otg.c        | 6 +++---
>>  drivers/usb/typec/tcpm/fusb302.c      | 4 ++--
>>  5 files changed, 12 insertions(+), 12 deletions(-)
> 
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
> Best Regards,
> Chanwoo Choi
> 
>>
>> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
>> index a983708b77a6..3472d3b756ed 100644
>> --- a/drivers/extcon/extcon-axp288.c
>> +++ b/drivers/extcon/extcon-axp288.c
>> @@ -360,8 +360,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>>  		name = acpi_dev_get_first_match_name("INT3496", NULL, -1);
>>  		if (name) {
>>  			info->id_extcon = extcon_get_extcon_dev(name);
>> -			if (!info->id_extcon)
>> -				return -EPROBE_DEFER;
>> +			if (IS_ERR(info->id_extcon))
>> +				return PTR_ERR(info->id_extcon);
>>  
>>  			dev_info(dev, "controlling USB role\n");
>>  		} else {
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 5ab0498be652..2bd0f2f33f05 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -884,7 +884,7 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>>  		if (!strcmp(sd->name, extcon_name))
>>  			goto out;
>>  	}
>> -	sd = NULL;
>> +	sd = ERR_PTR(-EPROBE_DEFER);


(snip)

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
  2018-11-12 16:11 ` Peter Ujfalusi
@ 2018-11-14  0:33   ` Mark Brown
  2018-11-14  8:45     ` Andy Shevchenko
  0 siblings, 1 reply; 32+ messages in thread
From: Mark Brown @ 2018-11-14  0:33 UTC (permalink / raw)
  To: Peter Ujfalusi
  Cc: Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-usb,
	Felipe Balbi, Guenter Roeck, Heikki Krogerus, Roger Quadros,
	linux-pm, Rafael J. Wysocki, Sebastian Reichel, linux-omap,
	Darren Hart, platform-driver-x86, Greg Kroah-Hartman,
	linux-kernel, Chen-Yu Tsai, Hans de Goede, Grant Likely,
	Andrzej Hajda

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

On Mon, Nov 12, 2018 at 06:11:26PM +0200, Peter Ujfalusi wrote:

> if we revert the commit then the original issue will re-surfaces. afaik
> it was not only audio which hit the 'last driver to be probed from the
> deferred list would never probe, unless we provoke the kernel to load
> additional module, or remove/reload the module' issue.

Right, aren't we just going to be swapping one bug for another?

> Do I understand correctly that in your case you have two modules
> (dwc3-pci and extcon-intel-mrfld) in a deferred probe loop, iow both of
> the drivers returns -EPROBE_DEFER and they just spin?

> If both is deferring, how this supposed to work?

I'm struggling to follow the original explanation too :(

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-13 23:52     ` Chanwoo Choi
@ 2018-11-14  8:35       ` Andy Shevchenko
  2018-11-14  9:13         ` Chanwoo Choi
  0 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-14  8:35 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: Andy Shevchenko, MyungJoo Ham, USB, Felipe Balbi, Guenter Roeck,
	Krogerus, Heikki, rogerq, Linux PM, Rafael J. Wysocki,
	Sebastian Reichel, Linux OMAP Mailing List, Darren Hart,
	Platform Driver, Greg Kroah-Hartman, Linux Kernel Mailing List,
	Chen-Yu Tsai, Hans de Goede

On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:

> I was thinking about again to change from NULL to EPROBE_DEFER.
>
> extcon_get_extcon_dev() function was almost called in the probe function.
> But, this function might be called on other position instead of probe.

*Might be* sounds like a theoretical thing, care to share what is in you mind?
Current users and more important the new coming one are *all* doing the same.

> ENODEV is more correct error instead of EPROBE_DEFER.

So, you are proposing to continue duplicating conversion from ENODEV
to EPROBE_DEFER in *each* caller?

> Sorry. I'll withdraw my opinion related acked-by tag until we are clarifying it.

I honestly don't know what to clarify here.

When we would have a real case we can change API correspondingly.
For now, the score is 5:0 with use cases in practice.

> On 2018년 11월 12일 09:24, Chanwoo Choi wrote:
> > On 2018년 11월 11일 03:10, Andy Shevchenko wrote:
> >> All current users of extcon_get_extcon_dev() API considers
> >> an extcon device a mandatory to appear. Thus, they all convert
> >> NULL pointer to -EPROBE_DEFER error code.
> >>
> >> There is one more caller anticipated with the same requirements.
> >>
> >> To decrease a code duplication and a burden to the callers,
> >> return -EPROBE_DEFER directly from extcon_get_extcon_dev().

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
  2018-11-14  0:33   ` Mark Brown
@ 2018-11-14  8:45     ` Andy Shevchenko
  2018-11-14 10:19       ` Peter Ujfalusi
  0 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-14  8:45 UTC (permalink / raw)
  To: Mark Brown
  Cc: Peter Ujfalusi, Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, USB,
	Felipe Balbi, Guenter Roeck, Krogerus, Heikki, rogerq, Linux PM,
	Rafael J. Wysocki, Sebastian Reichel, Linux OMAP Mailing List,
	Darren Hart, Platform Driver, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Chen-Yu Tsai, Hans de Goede,
	Grant Likely, Andrzej Hajda

On Wed, Nov 14, 2018 at 2:34 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Nov 12, 2018 at 06:11:26PM +0200, Peter Ujfalusi wrote:
>
> > if we revert the commit then the original issue will re-surfaces. afaik
> > it was not only audio which hit the 'last driver to be probed from the
> > deferred list would never probe, unless we provoke the kernel to load
> > additional module, or remove/reload the module' issue.
>
> Right, aren't we just going to be swapping one bug for another?

Have anyone in possession of Davinchi tested most recent kernel with
this revert?

> > Do I understand correctly that in your case you have two modules
> > (dwc3-pci and extcon-intel-mrfld) in a deferred probe loop, iow both of
> > the drivers returns -EPROBE_DEFER and they just spin?
>
> > If both is deferring, how this supposed to work?
>
> I'm struggling to follow the original explanation too :(

Sorry, guys, I confused a nit myself. The bug is there, but
exxplanation is not fully corrent, indeed. I'll come back with more
details later.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-14  8:35       ` Andy Shevchenko
@ 2018-11-14  9:13         ` Chanwoo Choi
  2018-11-14  9:36           ` Andy Shevchenko
  0 siblings, 1 reply; 32+ messages in thread
From: Chanwoo Choi @ 2018-11-14  9:13 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, MyungJoo Ham, USB, Felipe Balbi, Guenter Roeck,
	Krogerus, Heikki, rogerq, Linux PM, Rafael J. Wysocki,
	Sebastian Reichel, Linux OMAP Mailing List, Darren Hart,
	Platform Driver, Greg Kroah-Hartman, Linux Kernel Mailing List,
	Chen-Yu Tsai, Hans de Goede

On 2018년 11월 14일 17:35, Andy Shevchenko wrote:
> On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
> 
>> I was thinking about again to change from NULL to EPROBE_DEFER.
>>
>> extcon_get_extcon_dev() function was almost called in the probe function.
>> But, this function might be called on other position instead of probe.
> 
> *Might be* sounds like a theoretical thing, care to share what is in you mind?
> Current users and more important the new coming one are *all* doing the same.
> 
>> ENODEV is more correct error instead of EPROBE_DEFER.
> 
> So, you are proposing to continue duplicating conversion from ENODEV
> to EPROBE_DEFER in *each* caller?

The extcon core don't know the caller situation is in either probe() or other position
in the caller driver. The caller driver should decide the kind of error value
by using the return value of extcon_get_extcon_dev().

extcon_get_extcon_dev() function cannot be modified for only one case.
If some device driver call extcon_get_extcon_dev() out of probe() fuction,
EPROBE_DEFER is not always correct.

> 
>> Sorry. I'll withdraw my opinion related acked-by tag until we are clarifying it.
> 
> I honestly don't know what to clarify here.
> 
> When we would have a real case we can change API correspondingly.
> For now, the score is 5:0 with use cases in practice.
> 
>> On 2018년 11월 12일 09:24, Chanwoo Choi wrote:
>>> On 2018년 11월 11일 03:10, Andy Shevchenko wrote:
>>>> All current users of extcon_get_extcon_dev() API considers
>>>> an extcon device a mandatory to appear. Thus, they all convert
>>>> NULL pointer to -EPROBE_DEFER error code.
>>>>
>>>> There is one more caller anticipated with the same requirements.
>>>>
>>>> To decrease a code duplication and a burden to the callers,
>>>> return -EPROBE_DEFER directly from extcon_get_extcon_dev().
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-14  9:13         ` Chanwoo Choi
@ 2018-11-14  9:36           ` Andy Shevchenko
  2018-11-14  9:48             ` Chanwoo Choi
  0 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-14  9:36 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: MyungJoo Ham, USB, Felipe Balbi, Guenter Roeck, Krogerus, Heikki,
	rogerq, Linux PM, Rafael J. Wysocki, Sebastian Reichel,
	Linux OMAP Mailing List, Darren Hart, Platform Driver,
	Greg Kroah-Hartman, Linux Kernel Mailing List, Chen-Yu Tsai,
	Hans de Goede

On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote:
> On 2018년 11월 14일 17:35, Andy Shevchenko wrote:
> > On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
> > 
> >> I was thinking about again to change from NULL to EPROBE_DEFER.
> >>
> >> extcon_get_extcon_dev() function was almost called in the probe function.
> >> But, this function might be called on other position instead of probe.
> > 
> > *Might be* sounds like a theoretical thing, care to share what is in you mind?
> > Current users and more important the new coming one are *all* doing the same.
> > 
> >> ENODEV is more correct error instead of EPROBE_DEFER.
> > 
> > So, you are proposing to continue duplicating conversion from ENODEV
> > to EPROBE_DEFER in *each* caller?
> 
> The extcon core don't know the caller situation is in either probe() or other position
> in the caller driver. The caller driver should decide the kind of error value
> by using the return value of extcon_get_extcon_dev().
> 
> extcon_get_extcon_dev() function cannot be modified for only one case.
> If some device driver call extcon_get_extcon_dev() out of probe() fuction,
> EPROBE_DEFER is not always correct.

I agree with this, but look at the current state of affairs. All users do the same.
If we need to have another case we may consider this later.

API inside the kernel are not carved in the stone.


-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-14  9:36           ` Andy Shevchenko
@ 2018-11-14  9:48             ` Chanwoo Choi
  2018-11-14 10:20               ` Andy Shevchenko
  0 siblings, 1 reply; 32+ messages in thread
From: Chanwoo Choi @ 2018-11-14  9:48 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: MyungJoo Ham, USB, Felipe Balbi, Guenter Roeck, Krogerus, Heikki,
	rogerq, Linux PM, Rafael J. Wysocki, Sebastian Reichel,
	Linux OMAP Mailing List, Darren Hart, Platform Driver,
	Greg Kroah-Hartman, Linux Kernel Mailing List, Chen-Yu Tsai,
	Hans de Goede

On 2018년 11월 14일 18:36, Andy Shevchenko wrote:
> On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote:
>> On 2018년 11월 14일 17:35, Andy Shevchenko wrote:
>>> On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>
>>>> I was thinking about again to change from NULL to EPROBE_DEFER.
>>>>
>>>> extcon_get_extcon_dev() function was almost called in the probe function.
>>>> But, this function might be called on other position instead of probe.
>>>
>>> *Might be* sounds like a theoretical thing, care to share what is in you mind?
>>> Current users and more important the new coming one are *all* doing the same.
>>>
>>>> ENODEV is more correct error instead of EPROBE_DEFER.
>>>
>>> So, you are proposing to continue duplicating conversion from ENODEV
>>> to EPROBE_DEFER in *each* caller?
>>
>> The extcon core don't know the caller situation is in either probe() or other position
>> in the caller driver. The caller driver should decide the kind of error value
>> by using the return value of extcon_get_extcon_dev().
>>
>> extcon_get_extcon_dev() function cannot be modified for only one case.
>> If some device driver call extcon_get_extcon_dev() out of probe() fuction,
>> EPROBE_DEFER is not always correct.
> 
> I agree with this, but look at the current state of affairs. All users do the same.
> If we need to have another case we may consider this later.

Because we know the potential wrong case of this change, I can't agree this change.
If extcon_get_extcon_dev() returns ENODEV instead of EPROBE_DEFER,
it is clear and then there are no problem on both current and future.

> 
> API inside the kernel are not carved in the stone.
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
  2018-11-14  8:45     ` Andy Shevchenko
@ 2018-11-14 10:19       ` Peter Ujfalusi
  0 siblings, 0 replies; 32+ messages in thread
From: Peter Ujfalusi @ 2018-11-14 10:19 UTC (permalink / raw)
  To: Andy Shevchenko, Mark Brown
  Cc: Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, USB, Felipe Balbi,
	Guenter Roeck, Krogerus, Heikki, rogerq, Linux PM,
	Rafael J. Wysocki, Sebastian Reichel, Linux OMAP Mailing List,
	Darren Hart, Platform Driver, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Chen-Yu Tsai, Hans de Goede,
	grant.likely, Andrzej Hajda

Andy,

On 2018-11-14 10:45, Andy Shevchenko wrote:
> On Wed, Nov 14, 2018 at 2:34 AM Mark Brown <broonie@kernel.org> wrote:
>>
>> On Mon, Nov 12, 2018 at 06:11:26PM +0200, Peter Ujfalusi wrote:
>>
>>> if we revert the commit then the original issue will re-surfaces. afaik
>>> it was not only audio which hit the 'last driver to be probed from the
>>> deferred list would never probe, unless we provoke the kernel to load
>>> additional module, or remove/reload the module' issue.
>>
>> Right, aren't we just going to be swapping one bug for another?
> 
> Have anyone in possession of Davinchi tested most recent kernel with
> this revert?

I don't think it is related to daVinci, in fact we have seen the very
same issue with OMAP4.
Fwiw this was my initial patch
http://lkml.iu.edu/hypermail/linux/kernel/1404.0/01603.html
Grant based his change on this.

Note the part in the commit message:
"The issue has been observed on embedded systems with CONFIG_PREEMPT
enabled, audio support built as modules and using nfsroot for root
filesystem."

as I recall I was only able to reproduce the stall with nfsroot and
buildroot fs. The timings were different compared to MMC rootfs.

Anyways the patch fixes a real race condition which is generic:
We have driverA and driverB as modules. driverB needs driverA to be
registered to a framework (no direct, hard dependency).

1. driverA starts to probe
2. driverB starts to probe
3. driverB can not continue as driverA is not registered itself to the
framework -> driverB will defer
4. driverA registers to the framework
5. driverA probes successfully
6. driver core will prepare the deferred probe list (note: driverB is
still in it's probe, not in the deferred list)
7. driverB returns from it's probe with -EPROBE_DEFER

and here we are, driverB is alone in the deferred list and the list is
not going to be tried as driverA and driverB were the last ones to
probe.

So, driverB is in the deferred list and stays there until other driver
probes successfully as the deferred driver list only tried when a driver
loads successfully (to see if that satisfy any of the deferred driver).

We have evidence that this has happened, it is a generic issue given
that now days we have more frameworks drivers are relying on.

>>> Do I understand correctly that in your case you have two modules
>>> (dwc3-pci and extcon-intel-mrfld) in a deferred probe loop, iow both of
>>> the drivers returns -EPROBE_DEFER and they just spin?
>>
>>> If both is deferring, how this supposed to work?
>>
>> I'm struggling to follow the original explanation too :(
> 
> Sorry, guys, I confused a nit myself. The bug is there, but
> exxplanation is not fully corrent, indeed. I'll come back with more
> details later.
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-14  9:48             ` Chanwoo Choi
@ 2018-11-14 10:20               ` Andy Shevchenko
  2018-11-14 11:05                 ` Chanwoo Choi
  0 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-14 10:20 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: MyungJoo Ham, USB, Felipe Balbi, Guenter Roeck, Krogerus, Heikki,
	rogerq, Linux PM, Rafael J. Wysocki, Sebastian Reichel,
	Linux OMAP Mailing List, Darren Hart, Platform Driver,
	Greg Kroah-Hartman, Linux Kernel Mailing List, Chen-Yu Tsai,
	Hans de Goede

On Wed, Nov 14, 2018 at 11:48 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>
> On 2018년 11월 14일 18:36, Andy Shevchenko wrote:
> > On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote:
> >> On 2018년 11월 14일 17:35, Andy Shevchenko wrote:
> >>> On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
> >>>
> >>>> I was thinking about again to change from NULL to EPROBE_DEFER.
> >>>>
> >>>> extcon_get_extcon_dev() function was almost called in the probe function.
> >>>> But, this function might be called on other position instead of probe.
> >>>
> >>> *Might be* sounds like a theoretical thing, care to share what is in you mind?
> >>> Current users and more important the new coming one are *all* doing the same.
> >>>
> >>>> ENODEV is more correct error instead of EPROBE_DEFER.
> >>>
> >>> So, you are proposing to continue duplicating conversion from ENODEV
> >>> to EPROBE_DEFER in *each* caller?
> >>
> >> The extcon core don't know the caller situation is in either probe() or other position
> >> in the caller driver. The caller driver should decide the kind of error value
> >> by using the return value of extcon_get_extcon_dev().
> >>
> >> extcon_get_extcon_dev() function cannot be modified for only one case.
> >> If some device driver call extcon_get_extcon_dev() out of probe() fuction,
> >> EPROBE_DEFER is not always correct.
> >
> > I agree with this, but look at the current state of affairs. All users do the same.
> > If we need to have another case we may consider this later.
>
> Because we know the potential wrong case of this change, I can't agree this change.
> If extcon_get_extcon_dev() returns ENODEV instead of EPROBE_DEFER,
> it is clear and then there are no problem on both current and future.

Changing NULL to -ENODEV is a trading bad to worse.
I would not go that way, so, it's your call.

> > API inside the kernel are not carved in the stone.

Only can repeat myself (see above). While I see *theoretical*
rationale on your side, mine has *practical* proofs.
So, I'm giving up on this and will duplicate same what it's done in 4
current callers.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-14 10:20               ` Andy Shevchenko
@ 2018-11-14 11:05                 ` Chanwoo Choi
  2018-11-14 11:17                   ` Andy Shevchenko
  0 siblings, 1 reply; 32+ messages in thread
From: Chanwoo Choi @ 2018-11-14 11:05 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: MyungJoo Ham, USB, Felipe Balbi, Guenter Roeck, Krogerus, Heikki,
	rogerq, Linux PM, Rafael J. Wysocki, Sebastian Reichel,
	Linux OMAP Mailing List, Darren Hart, Platform Driver,
	Greg Kroah-Hartman, Linux Kernel Mailing List, Chen-Yu Tsai,
	Hans de Goede

On 2018년 11월 14일 19:20, Andy Shevchenko wrote:
> On Wed, Nov 14, 2018 at 11:48 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>
>> On 2018년 11월 14일 18:36, Andy Shevchenko wrote:
>>> On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote:
>>>> On 2018년 11월 14일 17:35, Andy Shevchenko wrote:
>>>>> On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>>>
>>>>>> I was thinking about again to change from NULL to EPROBE_DEFER.
>>>>>>
>>>>>> extcon_get_extcon_dev() function was almost called in the probe function.
>>>>>> But, this function might be called on other position instead of probe.
>>>>>
>>>>> *Might be* sounds like a theoretical thing, care to share what is in you mind?
>>>>> Current users and more important the new coming one are *all* doing the same.
>>>>>
>>>>>> ENODEV is more correct error instead of EPROBE_DEFER.
>>>>>
>>>>> So, you are proposing to continue duplicating conversion from ENODEV
>>>>> to EPROBE_DEFER in *each* caller?
>>>>
>>>> The extcon core don't know the caller situation is in either probe() or other position
>>>> in the caller driver. The caller driver should decide the kind of error value
>>>> by using the return value of extcon_get_extcon_dev().
>>>>
>>>> extcon_get_extcon_dev() function cannot be modified for only one case.
>>>> If some device driver call extcon_get_extcon_dev() out of probe() fuction,
>>>> EPROBE_DEFER is not always correct.
>>>
>>> I agree with this, but look at the current state of affairs. All users do the same.
>>> If we need to have another case we may consider this later.
>>
>> Because we know the potential wrong case of this change, I can't agree this change.
>> If extcon_get_extcon_dev() returns ENODEV instead of EPROBE_DEFER,
>> it is clear and then there are no problem on both current and future.
> 
> Changing NULL to -ENODEV is a trading bad to worse.
> I would not go that way, so, it's your call.

If you think that this change is not necessary, just keep the current code
without the modification. Please drop this patch on next version.

> 
>>> API inside the kernel are not carved in the stone.
> 
> Only can repeat myself (see above). While I see *theoretical*
> rationale on your side, mine has *practical* proofs.
> So, I'm giving up on this and will duplicate same what it's done in 4
> current callers.
> 

I think that it is more important for removing the potential wrong case
instead of removing the duplicate code. The many device drivers already
decided the proper error value by using the return value of function of 
kernel framework.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-14 11:05                 ` Chanwoo Choi
@ 2018-11-14 11:17                   ` Andy Shevchenko
  2018-11-14 14:04                     ` Andy Shevchenko
  0 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-14 11:17 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: MyungJoo Ham, USB, Felipe Balbi, Guenter Roeck, Krogerus, Heikki,
	rogerq, Linux PM, Rafael J. Wysocki, Sebastian Reichel,
	Linux OMAP Mailing List, Darren Hart, Platform Driver,
	Greg Kroah-Hartman, Linux Kernel Mailing List, Chen-Yu Tsai,
	Hans de Goede

On Wed, Nov 14, 2018 at 1:05 PM Chanwoo Choi <cw00.choi@samsung.com> wrote:
> On 2018년 11월 14일 19:20, Andy Shevchenko wrote:
> > On Wed, Nov 14, 2018 at 11:48 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
> >> On 2018년 11월 14일 18:36, Andy Shevchenko wrote:
> >>> On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote:
> >>>> On 2018년 11월 14일 17:35, Andy Shevchenko wrote:
> >>>>> On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
> >>>>>
> >>>>>> I was thinking about again to change from NULL to EPROBE_DEFER.
> >>>>>>
> >>>>>> extcon_get_extcon_dev() function was almost called in the probe function.
> >>>>>> But, this function might be called on other position instead of probe.
> >>>>>
> >>>>> *Might be* sounds like a theoretical thing, care to share what is in you mind?
> >>>>> Current users and more important the new coming one are *all* doing the same.
> >>>>>
> >>>>>> ENODEV is more correct error instead of EPROBE_DEFER.
> >>>>>
> >>>>> So, you are proposing to continue duplicating conversion from ENODEV
> >>>>> to EPROBE_DEFER in *each* caller?
> >>>>
> >>>> The extcon core don't know the caller situation is in either probe() or other position
> >>>> in the caller driver. The caller driver should decide the kind of error value
> >>>> by using the return value of extcon_get_extcon_dev().
> >>>>
> >>>> extcon_get_extcon_dev() function cannot be modified for only one case.
> >>>> If some device driver call extcon_get_extcon_dev() out of probe() fuction,
> >>>> EPROBE_DEFER is not always correct.
> >>>
> >>> I agree with this, but look at the current state of affairs. All users do the same.
> >>> If we need to have another case we may consider this later.
> >>
> >> Because we know the potential wrong case of this change, I can't agree this change.
> >> If extcon_get_extcon_dev() returns ENODEV instead of EPROBE_DEFER,
> >> it is clear and then there are no problem on both current and future.
> >
> > Changing NULL to -ENODEV is a trading bad to worse.
> > I would not go that way, so, it's your call.
>
> If you think that this change is not necessary, just keep the current code
> without the modification.

Not only this, the useless churn for no benefit to anyone, except some
*theoretical* cases no one saw.

> Please drop this patch on next version.

I will.

> >>> API inside the kernel are not carved in the stone.
> >
> > Only can repeat myself (see above). While I see *theoretical*
> > rationale on your side, mine has *practical* proofs.
> > So, I'm giving up on this and will duplicate same what it's done in 4
> > current callers.
> >
>
> I think that it is more important for removing the potential wrong case
> instead of removing the duplicate code. The many device drivers already
> decided the proper error value by using the return value of function of
> kernel framework.

The API has been introduced back in 2012.

commit 74c5d09bd562edc220d6e076b8f1e118819c178f
Author: Donggeun Kim <dg77.kim@samsung.com>
Date:   Fri Apr 20 14:16:24 2012 +0900

So, you are insisting that 6.5 years of use in a way people are using
it is wrong?

I don't know what might change your mind, but for me it's a clear
win-win to switch to deferred probe error code based on the
*practical* evidence.
But as I said, I gave up now.

P.S. I still disagree with your arguments in relation to de facto use of an API.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-14 11:17                   ` Andy Shevchenko
@ 2018-11-14 14:04                     ` Andy Shevchenko
  2018-11-15  1:16                       ` Chanwoo Choi
  0 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2018-11-14 14:04 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: MyungJoo Ham, USB, Felipe Balbi, Guenter Roeck, Krogerus, Heikki,
	rogerq, Linux PM, Rafael J. Wysocki, Sebastian Reichel,
	Linux OMAP Mailing List, Darren Hart, Platform Driver,
	Greg Kroah-Hartman, Linux Kernel Mailing List, Chen-Yu Tsai,
	Hans de Goede

On Wed, Nov 14, 2018 at 1:17 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Nov 14, 2018 at 1:05 PM Chanwoo Choi <cw00.choi@samsung.com> wrote:

> > > Changing NULL to -ENODEV is a trading bad to worse.

> P.S. I still disagree with your arguments in relation to de facto use of an API.

I spoke to colleagues of mine and they are agree that semantically
-EPROBE_DEFER is a wrong error code from API that matches against some
list.
On the other hand, they agree with me that changing NULL to -ENODEV is
a trading bad to worse.

So, I withdraw mine complaints against API.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
  2018-11-11 19:26   ` Rob Herring
  2018-11-11 23:53     ` Andy Shevchenko
@ 2018-11-14 23:25     ` Grant Likely
  1 sibling, 0 replies; 32+ messages in thread
From: Grant Likely @ 2018-11-14 23:25 UTC (permalink / raw)
  To: Rob Herring, Andy Shevchenko
  Cc: andriy.shevchenko, Rafael J. Wysocki, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Hans de Goede, peter.ujfalusi,
	a.hajda



On 11/11/2018 19:26, Rob Herring wrote:
> On Sun, Nov 11, 2018 at 7:04 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>>
>> I seems Grant's mail delivery bounces messages. I delibirately reduced
>> the Cc list for sake of ping Grant in case it would pass.
>
> That would be because he is not at Linaro anymore. Added his ARM account.
>
>> On Sat, Nov 10, 2018 at 8:12 PM Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>>
>>> Consider the following scenario.
>>>
>>> There are two independent devices coupled together by functional dependencies:
>>>   - USB OTG (dwc3-pci)
>>>   - extcon (tested with extcon-intel-mrfld, not yet in upstream)
>>>
>>> Each of the driver services a corresponding device is built as a module. In the
>>> Buildroot environment the modules are probed by alphabetical ordering of their
>>> modaliases. The latter comes to the case when USB OTG driver will be probed
>>> first followed by extcon one.
>>>
>>> So, if the platform anticipates extcon device to be appeared, in the above case
>>> we will get deferred probe of USB OTG, because of ordering.
>>>
>>> Now, a cherry on top of the cake, the deferred probing list contains
>>> the only two modules, i.e. USB OTG and extcon. Due to above circumstances,
>>> values in the local_trigger_count and deferred_trigger_count are not the same,
>>> and thus provokes deferred probe triggering again and again.
>>>
>>> ...
>>> [   20.678332] platform dwc3.0.auto: Retrying from deferred list
>>> [   20.694743] platform dwc3.0.auto: Driver dwc3 requests probe deferral
>>> [   20.701254] platform dwc3.0.auto: Added to deferred list
>>> [   20.706620] platform dwc3.0.auto: driver_deferred_probe_add_trigger 1 2
>>> [   20.713732] platform dwc3.0.auto: Retrying from deferred list
>>> [   20.730035] platform dwc3.0.auto: Driver dwc3 requests probe deferral
>>> [   20.736540] platform dwc3.0.auto: Added to deferred list
>>> [   20.741889] platform dwc3.0.auto: driver_deferred_probe_add_trigger 3 4
>>> [   20.748991] platform dwc3.0.auto: Retrying from deferred list
>>> [   20.765416] platform dwc3.0.auto: Driver dwc3 requests probe deferral
>>> [   20.771914] platform dwc3.0.auto: Added to deferred list
>>> [   20.777279] platform dwc3.0.auto: driver_deferred_probe_add_trigger 5 6
>>> ...
>>>
>>> Deeper investigation shows the culprit commit 58b116bce136
>>> ("drivercore: deferral race condition fix") which was dedicated to fix some
>>> other issue while bringing a regression.
>>>
>>> This reverts commit 58b116bce13612e5aa6fcd49ecbd4cf8bb59e835 for good until
>>> we will have better solution.
>
> How is behavior that's been there for 4.5 years a regression? Aren't
> we going to break Davinci that this commit was supposed to fix? What
> else could be relying on current behavior?

It has been a long time since I looked at this code, but so this may be
totally wrong. Something is causing driver_deferred_probe_trigger to get
called. I suspect all of this is happening within the single probe
event. This driver creates nested devices, which are bound to there own
driver. If one of those child devices probes successfully, and then the
parent probe fails with -EPROBE_DEFER, does it cause the whole structure
to be torn down again? If so, then that's the cause. The successful
probe will cause the probe loop.

You should investigate what driver probe/remove events happen in the
failure case. That will tell you exactly what is happening.

g.


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
  2018-11-14 14:04                     ` Andy Shevchenko
@ 2018-11-15  1:16                       ` Chanwoo Choi
  0 siblings, 0 replies; 32+ messages in thread
From: Chanwoo Choi @ 2018-11-15  1:16 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: MyungJoo Ham, USB, Felipe Balbi, Guenter Roeck, Krogerus, Heikki,
	rogerq, Linux PM, Rafael J. Wysocki, Sebastian Reichel,
	Linux OMAP Mailing List, Darren Hart, Platform Driver,
	Greg Kroah-Hartman, Linux Kernel Mailing List, Chen-Yu Tsai,
	Hans de Goede

On 2018년 11월 14일 23:04, Andy Shevchenko wrote:
> On Wed, Nov 14, 2018 at 1:17 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Wed, Nov 14, 2018 at 1:05 PM Chanwoo Choi <cw00.choi@samsung.com> wrote:
> 
>>>> Changing NULL to -ENODEV is a trading bad to worse.
> 
>> P.S. I still disagree with your arguments in relation to de facto use of an API.
> 
> I spoke to colleagues of mine and they are agree that semantically
> -EPROBE_DEFER is a wrong error code from API that matches against some
> list.
> On the other hand, they agree with me that changing NULL to -ENODEV is
> a trading bad to worse.
> 
> So, I withdraw mine complaints against API.
> 

OK. Thanks.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2018-11-15  1:17 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-10 18:10 [PATCH v1 1/5] drivercore: Revert "deferral race condition fix" Andy Shevchenko
2018-11-10 18:10 ` [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found Andy Shevchenko
2018-11-10 18:39   ` Guenter Roeck
2018-11-11  0:06   ` Sebastian Reichel
2018-11-12  0:24   ` Chanwoo Choi
2018-11-13 23:52     ` Chanwoo Choi
2018-11-14  8:35       ` Andy Shevchenko
2018-11-14  9:13         ` Chanwoo Choi
2018-11-14  9:36           ` Andy Shevchenko
2018-11-14  9:48             ` Chanwoo Choi
2018-11-14 10:20               ` Andy Shevchenko
2018-11-14 11:05                 ` Chanwoo Choi
2018-11-14 11:17                   ` Andy Shevchenko
2018-11-14 14:04                     ` Andy Shevchenko
2018-11-15  1:16                       ` Chanwoo Choi
2018-11-12 11:47   ` Heikki Krogerus
2018-11-10 18:10 ` [PATCH v1 3/5] staging: typec: fusb302: Rename fcs,extcon-name to linux,extcon-name Andy Shevchenko
2018-11-10 18:40   ` Guenter Roeck
2018-11-10 18:11 ` [PATCH v1 4/5] usb: dwc3: drd: Switch to device property for 'extcon' handling Andy Shevchenko
2018-11-12 11:47   ` Felipe Balbi
2018-11-10 18:11 ` [PATCH v1 5/5] usb: dwc3: drd: Add support for DR detection through extcon Andy Shevchenko
2018-11-10 18:26 ` [PATCH v1 1/5] drivercore: Revert "deferral race condition fix" Greg Kroah-Hartman
2018-11-10 18:36   ` Andy Shevchenko
2018-11-11 11:45     ` Hans de Goede
2018-11-11 13:04 ` Andy Shevchenko
2018-11-11 19:26   ` Rob Herring
2018-11-11 23:53     ` Andy Shevchenko
2018-11-14 23:25     ` Grant Likely
2018-11-12 16:11 ` Peter Ujfalusi
2018-11-14  0:33   ` Mark Brown
2018-11-14  8:45     ` Andy Shevchenko
2018-11-14 10:19       ` Peter Ujfalusi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).