extcon: fix extcon_get_extcon_dev() error handling

PATCH Superseded Dan Carpenter 6 participants 17 messages Nov 23, 2021 – Feb 16, 2022

Subsystems (9)

USB Kernel janitors DesignWare USB3 DRD IP driver Power supply +5 more subsystems

lore

Share & tools

lore

Dan Carpenter

author

2021-11-23 08:43 UTC #0 lore raw

PATCH v2 1/1 extcon: fix extcon_get_extcon_dev() error handling

The extcon_get_extcon_dev() function returns error pointers on error, NULL when it's a -EPROBE_DEFER defer situation, and ERR_PTR(-ENODEV) when the CONFIG_EXTCON option is disabled. This is very complicated for the callers to handle and a number of them had bugs that would lead to an Oops.

In real life, there are two things which prevented crashes. First, error pointers would only be returned if there was bug in the caller where they passed a NULL "extcon_name" and none of them do that. Second, only two out of the eight drivers will build when CONFIG_EXTCON is disabled.

The normal way to write this would be to return -EPROBE_DEFER directly when appropriate and return NULL when CONFIG_EXTCON is disabled. Then the error handling is simple and just looks like:

	dev->edev = extcon_get_extcon_dev(acpi_dev_name(adev));
	if (IS_ERR(dev->edev))
		return PTR_ERR(dev->edev);

For the two drivers which can build with CONFIG_EXTCON disabled, then extcon_get_extcon_dev() will now return NULL which is not treated as an error and the probe will continue successfully. Those two drivers are "typec_fusb302" and "max8997-battery". In the original code, the typec_fusb302 driver had an 800ms hang in tcpm_get_current_limit() but now that function is a no-op. For the max8997-battery driver everything should continue working as is.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: return NULL when CONFIG_EXTCON is disabled

If we apply this patch, it might be a good idea to send it to -stable so that backported code that relies on handling error pointers does not break silently.

 drivers/extcon/extcon-axp288.c         |    4 ++--
 drivers/extcon/extcon.c                |    2 +-
 drivers/power/supply/axp288_charger.c  |   17 ++++++++++-------
 drivers/power/supply/charger-manager.c |    7 ++-----
 drivers/power/supply/max8997_charger.c |   10 +++++-----
 drivers/usb/dwc3/drd.c                 |    9 ++-------
 drivers/usb/phy/phy-omap-otg.c         |    4 ++--
 drivers/usb/typec/tcpm/fusb302.c       |    4 ++--
 include/linux/extcon.h                 |    2 +-
 9 files changed, 27 insertions(+), 32 deletions(-)

drivers/extcon/extcon.c +1 -1

index e7a9561a826d..a35e99928807 100644
@@ -876,7 +876,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;

include/linux/extcon.h +1 -1

index 0c19010da77f..685401d94d39 100644
@@ -296,7 +296,7 @@ static inline void devm_extcon_unregister_notifier_all(struct device *dev,
 
 static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {
-	return ERR_PTR(-ENODEV);
+	return NULL;
 }
 
 static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)

drivers/extcon/extcon-axp288.c +2 -2

index 7c6d5857ff25..180be768c215 100644
@@ -394,8 +394,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		if (adev) {
 			info->id_extcon = extcon_get_extcon_dev(acpi_dev_name(adev));
 			put_device(&adev->dev);
-			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 {

drivers/power/supply/axp288_charger.c +10 -7

index ec41f6cd3f93..4acfeb52a44e 100644
@@ -848,17 +848,20 @@ 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) {
-		dev_dbg(dev, "%s is not ready, probe deferred\n",
-			AXP288_EXTCON_DEV_NAME);
-		return -EPROBE_DEFER;
+	if (IS_ERR(info->cable.edev)) {
+		dev_err_probe(dev, PTR_ERR(info->cable.edev),
+			      "extcon_get_extcon_dev(%s) failed\n",
+			      AXP288_EXTCON_DEV_NAME);
+		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) {
-			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
-			return -EPROBE_DEFER;
+		if (IS_ERR(info->otg.cable)) {
+			dev_err_probe(dev, PTR_ERR(info->otg.cable),
+				      "extcon_get_extcon_dev(%s) failed\n",
+				      USB_HOST_EXTCON_NAME);
+			return PTR_ERR(info->otg.cable);
 		}
 		dev_info(dev, "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
 	}

drivers/power/supply/charger-manager.c +2 -5

index d67edb760c94..92db79400a6a 100644
@@ -985,13 +985,10 @@ static int charger_extcon_init(struct charger_manager *cm,
 	cable->nb.notifier_call = charger_extcon_notifier;
 
 	cable->extcon_dev = extcon_get_extcon_dev(cable->extcon_name);
-	if (IS_ERR_OR_NULL(cable->extcon_dev)) {
+	if (IS_ERR(cable->extcon_dev)) {
 		pr_err("Cannot find extcon_dev for %s (cable: %s)\n",
 			cable->extcon_name, cable->name);
-		if (cable->extcon_dev == NULL)
-			return -EPROBE_DEFER;
-		else
-			return PTR_ERR(cable->extcon_dev);
+		return PTR_ERR(cable->extcon_dev);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(extcon_mapping); i++) {

drivers/power/supply/max8997_charger.c +5 -5

index 25207fe2aa68..634658adf313 100644
@@ -248,13 +248,13 @@ static int max8997_battery_probe(struct platform_device *pdev)
 		dev_info(&pdev->dev, "couldn't get charger regulator\n");
 	}
 	charger->edev = extcon_get_extcon_dev("max8997-muic");
-	if (IS_ERR_OR_NULL(charger->edev)) {
-		if (!charger->edev)
-			return -EPROBE_DEFER;
-		dev_info(charger->dev, "couldn't get extcon device\n");
+	if (IS_ERR(charger->edev)) {
+		dev_err_probe(charger->dev, PTR_ERR(charger->edev),
+			      "couldn't get extcon device: max8997-muic\n");
+		return PTR_ERR(charger->edev);
 	}
 
-	if (!IS_ERR(charger->reg) && !IS_ERR_OR_NULL(charger->edev)) {
+	if (!IS_ERR(charger->reg)) {
 		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
 		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
 		if (ret) {

drivers/usb/dwc3/drd.c +2 -7

index d7f76835137f..a490f79131c1 100644
@@ -454,13 +454,8 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	 * 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) {
-		edev = extcon_get_extcon_dev(name);
-		if (!edev)
-			return ERR_PTR(-EPROBE_DEFER);
-
-		return edev;
-	}
+	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0)
+		return extcon_get_extcon_dev(name);
 
 	/*
 	 * Try to get an extcon device from the USB PHY controller's "port"

drivers/usb/phy/phy-omap-otg.c +2 -2

index ee0863c6553e..6e6ef8c0bc7e 100644
@@ -95,8 +95,8 @@ static int omap_otg_probe(struct platform_device *pdev)
 		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)

drivers/usb/typec/tcpm/fusb302.c +2 -2

index 72f9001b0792..96c55eaf3f80 100644
@@ -1708,8 +1708,8 @@ static int fusb302_probe(struct i2c_client *client,
 	 */
 	if (device_property_read_string(dev, "linux,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");

Hans de Goede

Reviewed-by on v2 1/1

2021-11-23 14:14 UTC #1 lore raw

Hi,

Quoted 28 lines · Dan Carpenter

The extcon_get_extcon_dev() function returns error pointers on error, NULL when it's a -EPROBE_DEFER defer situation, and ERR_PTR(-ENODEV) when the CONFIG_EXTCON option is disabled. This is very complicated for the callers to handle and a number of them had bugs that would lead to an Oops.
In real life, there are two things which prevented crashes. First, error pointers would only be returned if there was bug in the caller where they passed a NULL "extcon_name" and none of them do that. Second, only two out of the eight drivers will build when CONFIG_EXTCON is disabled.
The normal way to write this would be to return -EPROBE_DEFER directly when appropriate and return NULL when CONFIG_EXTCON is disabled. Then the error handling is simple and just looks like:
	dev->edev = extcon_get_extcon_dev(acpi_dev_name(adev));
	if (IS_ERR(dev->edev))
		return PTR_ERR(dev->edev);
For the two drivers which can build with CONFIG_EXTCON disabled, then extcon_get_extcon_dev() will now return NULL which is not treated as an error and the probe will continue successfully. Those two drivers are "typec_fusb302" and "max8997-battery". In the original code, the typec_fusb302 driver had an 800ms hang in tcpm_get_current_limit() but now that function is a no-op. For the max8997-battery driver everything should continue working as is.

Quoted 1 line

Signed-off-by Dan Carpenter <[email protected]>

Thanks, patch looks good to me:

Reviewed-by Hans de Goede <[email protected]>

Regards,

Hans

Quoted 185 lines

---
v2: return NULL when CONFIG_EXTCON is disabled

If we apply this patch, it might be a good idea to send it to -stable so that backported code that relies on handling error pointers does not break silently.

 drivers/extcon/extcon-axp288.c         |    4 ++--
 drivers/extcon/extcon.c                |    2 +-
 drivers/power/supply/axp288_charger.c  |   17 ++++++++++-------
 drivers/power/supply/charger-manager.c |    7 ++-----
 drivers/power/supply/max8997_charger.c |   10 +++++-----
 drivers/usb/dwc3/drd.c                 |    9 ++-------
 drivers/usb/phy/phy-omap-otg.c         |    4 ++--
 drivers/usb/typec/tcpm/fusb302.c       |    4 ++--
 include/linux/extcon.h                 |    2 +-
 9 files changed, 27 insertions(+), 32 deletions(-)

drivers/extcon/extcon.c +1 -1

index e7a9561a826d..a35e99928807 100644
@@ -876,7 +876,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;

include/linux/extcon.h +1 -1

index 0c19010da77f..685401d94d39 100644
@@ -296,7 +296,7 @@ static inline void devm_extcon_unregister_notifier_all(struct device *dev,
 
 static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {
-	return ERR_PTR(-ENODEV);
+	return NULL;
 }
 
 static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)

drivers/extcon/extcon-axp288.c +2 -2

index 7c6d5857ff25..180be768c215 100644
@@ -394,8 +394,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		if (adev) {
 			info->id_extcon = extcon_get_extcon_dev(acpi_dev_name(adev));
 			put_device(&adev->dev);
-			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 {

drivers/power/supply/axp288_charger.c +10 -7

index ec41f6cd3f93..4acfeb52a44e 100644
@@ -848,17 +848,20 @@ 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) {
-		dev_dbg(dev, "%s is not ready, probe deferred\n",
-			AXP288_EXTCON_DEV_NAME);
-		return -EPROBE_DEFER;
+	if (IS_ERR(info->cable.edev)) {
+		dev_err_probe(dev, PTR_ERR(info->cable.edev),
+			      "extcon_get_extcon_dev(%s) failed\n",
+			      AXP288_EXTCON_DEV_NAME);
+		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) {
-			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
-			return -EPROBE_DEFER;
+		if (IS_ERR(info->otg.cable)) {
+			dev_err_probe(dev, PTR_ERR(info->otg.cable),
+				      "extcon_get_extcon_dev(%s) failed\n",
+				      USB_HOST_EXTCON_NAME);
+			return PTR_ERR(info->otg.cable);
 		}
 		dev_info(dev, "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
 	}

drivers/power/supply/charger-manager.c +2 -5

index d67edb760c94..92db79400a6a 100644
@@ -985,13 +985,10 @@ static int charger_extcon_init(struct charger_manager *cm,
 	cable->nb.notifier_call = charger_extcon_notifier;
 
 	cable->extcon_dev = extcon_get_extcon_dev(cable->extcon_name);
-	if (IS_ERR_OR_NULL(cable->extcon_dev)) {
+	if (IS_ERR(cable->extcon_dev)) {
 		pr_err("Cannot find extcon_dev for %s (cable: %s)\n",
 			cable->extcon_name, cable->name);
-		if (cable->extcon_dev == NULL)
-			return -EPROBE_DEFER;
-		else
-			return PTR_ERR(cable->extcon_dev);
+		return PTR_ERR(cable->extcon_dev);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(extcon_mapping); i++) {

drivers/power/supply/max8997_charger.c +5 -5

index 25207fe2aa68..634658adf313 100644
@@ -248,13 +248,13 @@ static int max8997_battery_probe(struct platform_device *pdev)
 		dev_info(&pdev->dev, "couldn't get charger regulator\n");
 	}
 	charger->edev = extcon_get_extcon_dev("max8997-muic");
-	if (IS_ERR_OR_NULL(charger->edev)) {
-		if (!charger->edev)
-			return -EPROBE_DEFER;
-		dev_info(charger->dev, "couldn't get extcon device\n");
+	if (IS_ERR(charger->edev)) {
+		dev_err_probe(charger->dev, PTR_ERR(charger->edev),
+			      "couldn't get extcon device: max8997-muic\n");
+		return PTR_ERR(charger->edev);
 	}
 
-	if (!IS_ERR(charger->reg) && !IS_ERR_OR_NULL(charger->edev)) {
+	if (!IS_ERR(charger->reg)) {
 		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
 		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
 		if (ret) {

drivers/usb/dwc3/drd.c +2 -7

index d7f76835137f..a490f79131c1 100644
@@ -454,13 +454,8 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	 * 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) {
-		edev = extcon_get_extcon_dev(name);
-		if (!edev)
-			return ERR_PTR(-EPROBE_DEFER);
-
-		return edev;
-	}
+	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0)
+		return extcon_get_extcon_dev(name);
 
 	/*
 	 * Try to get an extcon device from the USB PHY controller's "port"

drivers/usb/phy/phy-omap-otg.c +2 -2

index ee0863c6553e..6e6ef8c0bc7e 100644
@@ -95,8 +95,8 @@ static int omap_otg_probe(struct platform_device *pdev)
 		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)

drivers/usb/typec/tcpm/fusb302.c +2 -2

index 72f9001b0792..96c55eaf3f80 100644
@@ -1708,8 +1708,8 @@ static int fusb302_probe(struct i2c_client *client,
 	 */
 	if (device_property_read_string(dev, "linux,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");

Guenter Roeck

Reviewed-by on v2 1/1

2021-11-23 14:48 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 28 lines · Dan Carpenter

The extcon_get_extcon_dev() function returns error pointers on error, NULL when it's a -EPROBE_DEFER defer situation, and ERR_PTR(-ENODEV) when the CONFIG_EXTCON option is disabled. This is very complicated for the callers to handle and a number of them had bugs that would lead to an Oops.
In real life, there are two things which prevented crashes. First, error pointers would only be returned if there was bug in the caller where they passed a NULL "extcon_name" and none of them do that. Second, only two out of the eight drivers will build when CONFIG_EXTCON is disabled.
The normal way to write this would be to return -EPROBE_DEFER directly when appropriate and return NULL when CONFIG_EXTCON is disabled. Then the error handling is simple and just looks like:
	dev->edev = extcon_get_extcon_dev(acpi_dev_name(adev));
	if (IS_ERR(dev->edev))
		return PTR_ERR(dev->edev);
For the two drivers which can build with CONFIG_EXTCON disabled, then extcon_get_extcon_dev() will now return NULL which is not treated as an error and the probe will continue successfully. Those two drivers are "typec_fusb302" and "max8997-battery". In the original code, the typec_fusb302 driver had an 800ms hang in tcpm_get_current_limit() but now that function is a no-op. For the max8997-battery driver everything should continue working as is.

Quoted 1 line

Signed-off-by Dan Carpenter <[email protected]>

Reviewed-by Guenter Roeck <[email protected]>

Heikki Krogerus

maintainer Reviewed-by on v2 1/1

2021-11-23 15:20 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 28 lines · Dan Carpenter

The extcon_get_extcon_dev() function returns error pointers on error, NULL when it's a -EPROBE_DEFER defer situation, and ERR_PTR(-ENODEV) when the CONFIG_EXTCON option is disabled. This is very complicated for the callers to handle and a number of them had bugs that would lead to an Oops.
In real life, there are two things which prevented crashes. First, error pointers would only be returned if there was bug in the caller where they passed a NULL "extcon_name" and none of them do that. Second, only two out of the eight drivers will build when CONFIG_EXTCON is disabled.
The normal way to write this would be to return -EPROBE_DEFER directly when appropriate and return NULL when CONFIG_EXTCON is disabled. Then the error handling is simple and just looks like:
	dev->edev = extcon_get_extcon_dev(acpi_dev_name(adev));
	if (IS_ERR(dev->edev))
		return PTR_ERR(dev->edev);
For the two drivers which can build with CONFIG_EXTCON disabled, then extcon_get_extcon_dev() will now return NULL which is not treated as an error and the probe will continue successfully. Those two drivers are "typec_fusb302" and "max8997-battery". In the original code, the typec_fusb302 driver had an 800ms hang in tcpm_get_current_limit() but now that function is a no-op. For the max8997-battery driver everything should continue working as is.

Quoted 1 line

Signed-off-by Dan Carpenter <[email protected]>

Reviewed-by Heikki Krogerus <[email protected]>

Quoted 184 lines

---
v2: return NULL when CONFIG_EXTCON is disabled

If we apply this patch, it might be a good idea to send it to -stable so that backported code that relies on handling error pointers does not break silently.

 drivers/extcon/extcon-axp288.c         |    4 ++--
 drivers/extcon/extcon.c                |    2 +-
 drivers/power/supply/axp288_charger.c  |   17 ++++++++++-------
 drivers/power/supply/charger-manager.c |    7 ++-----
 drivers/power/supply/max8997_charger.c |   10 +++++-----
 drivers/usb/dwc3/drd.c                 |    9 ++-------
 drivers/usb/phy/phy-omap-otg.c         |    4 ++--
 drivers/usb/typec/tcpm/fusb302.c       |    4 ++--
 include/linux/extcon.h                 |    2 +-
 9 files changed, 27 insertions(+), 32 deletions(-)

drivers/extcon/extcon.c +1 -1

index e7a9561a826d..a35e99928807 100644
@@ -876,7 +876,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;

include/linux/extcon.h +1 -1

index 0c19010da77f..685401d94d39 100644
@@ -296,7 +296,7 @@ static inline void devm_extcon_unregister_notifier_all(struct device *dev,
 
 static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {
-	return ERR_PTR(-ENODEV);
+	return NULL;
 }
 
 static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)

drivers/extcon/extcon-axp288.c +2 -2

index 7c6d5857ff25..180be768c215 100644
@@ -394,8 +394,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		if (adev) {
 			info->id_extcon = extcon_get_extcon_dev(acpi_dev_name(adev));
 			put_device(&adev->dev);
-			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 {

drivers/power/supply/axp288_charger.c +10 -7

index ec41f6cd3f93..4acfeb52a44e 100644
@@ -848,17 +848,20 @@ 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) {
-		dev_dbg(dev, "%s is not ready, probe deferred\n",
-			AXP288_EXTCON_DEV_NAME);
-		return -EPROBE_DEFER;
+	if (IS_ERR(info->cable.edev)) {
+		dev_err_probe(dev, PTR_ERR(info->cable.edev),
+			      "extcon_get_extcon_dev(%s) failed\n",
+			      AXP288_EXTCON_DEV_NAME);
+		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) {
-			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
-			return -EPROBE_DEFER;
+		if (IS_ERR(info->otg.cable)) {
+			dev_err_probe(dev, PTR_ERR(info->otg.cable),
+				      "extcon_get_extcon_dev(%s) failed\n",
+				      USB_HOST_EXTCON_NAME);
+			return PTR_ERR(info->otg.cable);
 		}
 		dev_info(dev, "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
 	}

drivers/power/supply/charger-manager.c +2 -5

index d67edb760c94..92db79400a6a 100644
@@ -985,13 +985,10 @@ static int charger_extcon_init(struct charger_manager *cm,
 	cable->nb.notifier_call = charger_extcon_notifier;
 
 	cable->extcon_dev = extcon_get_extcon_dev(cable->extcon_name);
-	if (IS_ERR_OR_NULL(cable->extcon_dev)) {
+	if (IS_ERR(cable->extcon_dev)) {
 		pr_err("Cannot find extcon_dev for %s (cable: %s)\n",
 			cable->extcon_name, cable->name);
-		if (cable->extcon_dev == NULL)
-			return -EPROBE_DEFER;
-		else
-			return PTR_ERR(cable->extcon_dev);
+		return PTR_ERR(cable->extcon_dev);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(extcon_mapping); i++) {

drivers/power/supply/max8997_charger.c +5 -5

index 25207fe2aa68..634658adf313 100644
@@ -248,13 +248,13 @@ static int max8997_battery_probe(struct platform_device *pdev)
 		dev_info(&pdev->dev, "couldn't get charger regulator\n");
 	}
 	charger->edev = extcon_get_extcon_dev("max8997-muic");
-	if (IS_ERR_OR_NULL(charger->edev)) {
-		if (!charger->edev)
-			return -EPROBE_DEFER;
-		dev_info(charger->dev, "couldn't get extcon device\n");
+	if (IS_ERR(charger->edev)) {
+		dev_err_probe(charger->dev, PTR_ERR(charger->edev),
+			      "couldn't get extcon device: max8997-muic\n");
+		return PTR_ERR(charger->edev);
 	}
 
-	if (!IS_ERR(charger->reg) && !IS_ERR_OR_NULL(charger->edev)) {
+	if (!IS_ERR(charger->reg)) {
 		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
 		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
 		if (ret) {

drivers/usb/dwc3/drd.c +2 -7

index d7f76835137f..a490f79131c1 100644
@@ -454,13 +454,8 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	 * 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) {
-		edev = extcon_get_extcon_dev(name);
-		if (!edev)
-			return ERR_PTR(-EPROBE_DEFER);
-
-		return edev;
-	}
+	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0)
+		return extcon_get_extcon_dev(name);
 
 	/*
 	 * Try to get an extcon device from the USB PHY controller's "port"

drivers/usb/phy/phy-omap-otg.c +2 -2

index ee0863c6553e..6e6ef8c0bc7e 100644
@@ -95,8 +95,8 @@ static int omap_otg_probe(struct platform_device *pdev)
 		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)

drivers/usb/typec/tcpm/fusb302.c +2 -2

index 72f9001b0792..96c55eaf3f80 100644
@@ -1708,8 +1708,8 @@ static int fusb302_probe(struct i2c_client *client,
 	 */
 	if (device_property_read_string(dev, "linux,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");

Signature · 1 line

heikki

Chanwoo Choi

maintainer

2021-12-16 06:17 UTC #4 lore raw

In reply to Dan Carpenter

Hi Dan,

First of all, sorry for late reply.

There is one issue. About this issue, I already discussed on patch[1]
[1] https://lore.kernel.org/lkml/[email protected]/

extcon_get_extcon_dev() is used for anywhere except for probe step. But EPROBE_DEFER is only used on probe step.

So that it is not clear to return EPROBE_DEFER from extcon_get_extcon_dev() because extcon_get_extcon_dev() never know either call it on probe function or not.

Quoted 216 lines · Dan Carpenter

The normal way to write this would be to return -EPROBE_DEFER directly when appropriate and return NULL when CONFIG_EXTCON is disabled. Then the error handling is simple and just looks like:

	dev->edev = extcon_get_extcon_dev(acpi_dev_name(adev));
	if (IS_ERR(dev->edev))
		return PTR_ERR(dev->edev);

Signed-off-by Dan Carpenter <[email protected]>

---
v2: return NULL when CONFIG_EXTCON is disabled

If we apply this patch, it might be a good idea to send it to -stable so that backported code that relies on handling error pointers does not break silently.

 drivers/extcon/extcon-axp288.c         |    4 ++--
 drivers/extcon/extcon.c                |    2 +-
 drivers/power/supply/axp288_charger.c  |   17 ++++++++++-------
 drivers/power/supply/charger-manager.c |    7 ++-----
 drivers/power/supply/max8997_charger.c |   10 +++++-----
 drivers/usb/dwc3/drd.c                 |    9 ++-------
 drivers/usb/phy/phy-omap-otg.c         |    4 ++--
 drivers/usb/typec/tcpm/fusb302.c       |    4 ++--
 include/linux/extcon.h                 |    2 +-
 9 files changed, 27 insertions(+), 32 deletions(-)

drivers/extcon/extcon.c +1 -1

index e7a9561a826d..a35e99928807 100644
@@ -876,7 +876,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;

include/linux/extcon.h +1 -1

index 0c19010da77f..685401d94d39 100644
@@ -296,7 +296,7 @@ static inline void devm_extcon_unregister_notifier_all(struct device *dev,
 
 static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {
-	return ERR_PTR(-ENODEV);
+	return NULL;
 }
 
 static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)

drivers/extcon/extcon-axp288.c +2 -2

index 7c6d5857ff25..180be768c215 100644
@@ -394,8 +394,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		if (adev) {
 			info->id_extcon = extcon_get_extcon_dev(acpi_dev_name(adev));
 			put_device(&adev->dev);
-			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 {

drivers/power/supply/axp288_charger.c +10 -7

index ec41f6cd3f93..4acfeb52a44e 100644
@@ -848,17 +848,20 @@ 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) {
-		dev_dbg(dev, "%s is not ready, probe deferred\n",
-			AXP288_EXTCON_DEV_NAME);
-		return -EPROBE_DEFER;
+	if (IS_ERR(info->cable.edev)) {
+		dev_err_probe(dev, PTR_ERR(info->cable.edev),
+			      "extcon_get_extcon_dev(%s) failed\n",
+			      AXP288_EXTCON_DEV_NAME);
+		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) {
-			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
-			return -EPROBE_DEFER;
+		if (IS_ERR(info->otg.cable)) {
+			dev_err_probe(dev, PTR_ERR(info->otg.cable),
+				      "extcon_get_extcon_dev(%s) failed\n",
+				      USB_HOST_EXTCON_NAME);
+			return PTR_ERR(info->otg.cable);
 		}
 		dev_info(dev, "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
 	}

drivers/power/supply/charger-manager.c +2 -5

index d67edb760c94..92db79400a6a 100644
@@ -985,13 +985,10 @@ static int charger_extcon_init(struct charger_manager *cm,
 	cable->nb.notifier_call = charger_extcon_notifier;
 
 	cable->extcon_dev = extcon_get_extcon_dev(cable->extcon_name);
-	if (IS_ERR_OR_NULL(cable->extcon_dev)) {
+	if (IS_ERR(cable->extcon_dev)) {
 		pr_err("Cannot find extcon_dev for %s (cable: %s)\n",
 			cable->extcon_name, cable->name);
-		if (cable->extcon_dev == NULL)
-			return -EPROBE_DEFER;
-		else
-			return PTR_ERR(cable->extcon_dev);
+		return PTR_ERR(cable->extcon_dev);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(extcon_mapping); i++) {

drivers/power/supply/max8997_charger.c +5 -5

index 25207fe2aa68..634658adf313 100644
@@ -248,13 +248,13 @@ static int max8997_battery_probe(struct platform_device *pdev)
 		dev_info(&pdev->dev, "couldn't get charger regulator\n");
 	}
 	charger->edev = extcon_get_extcon_dev("max8997-muic");
-	if (IS_ERR_OR_NULL(charger->edev)) {
-		if (!charger->edev)
-			return -EPROBE_DEFER;
-		dev_info(charger->dev, "couldn't get extcon device\n");
+	if (IS_ERR(charger->edev)) {
+		dev_err_probe(charger->dev, PTR_ERR(charger->edev),
+			      "couldn't get extcon device: max8997-muic\n");
+		return PTR_ERR(charger->edev);
 	}
 
-	if (!IS_ERR(charger->reg) && !IS_ERR_OR_NULL(charger->edev)) {
+	if (!IS_ERR(charger->reg)) {
 		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
 		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
 		if (ret) {

drivers/usb/dwc3/drd.c +2 -7

index d7f76835137f..a490f79131c1 100644
@@ -454,13 +454,8 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	 * 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) {
-		edev = extcon_get_extcon_dev(name);
-		if (!edev)
-			return ERR_PTR(-EPROBE_DEFER);
-
-		return edev;
-	}
+	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0)
+		return extcon_get_extcon_dev(name);
 
 	/*
 	 * Try to get an extcon device from the USB PHY controller's "port"

drivers/usb/phy/phy-omap-otg.c +2 -2

index ee0863c6553e..6e6ef8c0bc7e 100644
@@ -95,8 +95,8 @@ static int omap_otg_probe(struct platform_device *pdev)
 		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)

drivers/usb/typec/tcpm/fusb302.c +2 -2

index 72f9001b0792..96c55eaf3f80 100644
@@ -1708,8 +1708,8 @@ static int fusb302_probe(struct i2c_client *client,
 	 */
 	if (device_property_read_string(dev, "linux,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");

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dan Carpenter

author

2021-12-16 07:53 UTC #5 lore raw

Quoted 7 lines · Chanwoo Choi

Hi Dan,
First of all, sorry for late reply.
There is one issue. About this issue, I already discussed on patch[1]
[1] https://lore.kernel.org/lkml/[email protected]/

Quoted 6 lines

extcon_get_extcon_dev() is used for anywhere except for probe step. But EPROBE_DEFER is only used on probe step.
So that it is not clear to return EPROBE_DEFER from extcon_get_extcon_dev() because extcon_get_extcon_dev() never know either call it on probe function or not.

Currently extcon_get_extcon_dev() is only called from probe so it's not an issue.

It's impossible to know what future programmers will want, but I feel like this change probably makes it easier for them.

regards,
dan carpenter

Chanwoo Choi

maintainer

2021-12-16 08:01 UTC #6 lore raw

Quoted 11 lines · Dan Carpenter

Quoted 10 lines · Chanwoo Choi
Hi Dan,
First of all, sorry for late reply.
There is one issue. About this issue, I already discussed on patch[1]
[1] https://lore.kernel.org/lkml/[email protected]/
extcon_get_extcon_dev() is used for anywhere except for probe step. But EPROBE_DEFER is only used on probe step.

Quoted 6 lines

Quoted 3 lines
So that it is not clear to return EPROBE_DEFER from extcon_get_extcon_dev() because extcon_get_extcon_dev() never know either call it on probe function or not.
Currently extcon_get_extcon_dev() is only called from probe so it's not an issue.

Even if extcon_get_extcon_dev() is used on probe until now, it is possible to use on anywhere as I commented.

It is difficult to agree this approach without any other solution.

Basically, the subsystem core never know either probe time or not. It means that this issue should be handled in each device driver.

Quoted 3 lines

It's impossible to know what future programmers will want, but I feel like this change probably makes it easier for them.

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dan Carpenter

author

2021-12-16 08:06 UTC #7 lore raw

Quoted 19 lines · Chanwoo Choi

Quoted 11 lines · Dan Carpenter
Quoted 10 lines · Chanwoo Choi
Hi Dan,
First of all, sorry for late reply.
There is one issue. About this issue, I already discussed on patch[1]
[1] https://lore.kernel.org/lkml/[email protected]/
extcon_get_extcon_dev() is used for anywhere except for probe step. But EPROBE_DEFER is only used on probe step.
Quoted 6 lines
Quoted 3 lines
So that it is not clear to return EPROBE_DEFER from extcon_get_extcon_dev() because extcon_get_extcon_dev() never know either call it on probe function or not.
Currently extcon_get_extcon_dev() is only called from probe so it's not an issue.

Quoted 8 lines

Even if extcon_get_extcon_dev() is used on probe until now, it is possible to use on anywhere as I commented.
It is difficult to agree this approach without any other solution.
Basically, the subsystem core never know either probe time or not. It means that this issue should be handled in each device driver.

To be honest, I'm not sure how this differs from other functions which return -EPROBE_DEFER. How do other functions guarantee they will only be called from probe()?

regards,
dan carpenter

Chanwoo Choi

maintainer

2021-12-16 08:15 UTC #8 lore raw

Quoted 25 lines · Dan Carpenter

Quoted 24 lines · Chanwoo Choi
Quoted 11 lines · Dan Carpenter
Quoted 10 lines · Chanwoo Choi
Hi Dan,
First of all, sorry for late reply.
There is one issue. About this issue, I already discussed on patch[1]
[1] https://lore.kernel.org/lkml/[email protected]/
extcon_get_extcon_dev() is used for anywhere except for probe step. But EPROBE_DEFER is only used on probe step.
Quoted 6 lines
Quoted 3 lines
So that it is not clear to return EPROBE_DEFER from extcon_get_extcon_dev() because extcon_get_extcon_dev() never know either call it on probe function or not.
Currently extcon_get_extcon_dev() is only called from probe so it's not an issue.
Even if extcon_get_extcon_dev() is used on probe until now, it is possible to use on anywhere as I commented.
It is difficult to agree this approach without any other solution.

Quoted 7 lines

Quoted 3 lines
Basically, the subsystem core never know either probe time or not. It means that this issue should be handled in each device driver.
To be honest, I'm not sure how this differs from other functions which return -EPROBE_DEFER. How do other functions guarantee they will only be called from probe()?

If it is possible to know extcon_get_extcon_dev() will be only callled on probe, it is no problem. But, it is not able to guarantee that extcon_get_extcon_dev() is called on probe. Because of this reason, this issue should be handled in each device driver.

-EPROBE_DEFER is only for probe step. If return -EPROBE_DEFER except for probe, it is wrong return value.

Quoted 6 lines

regards,
dan carpenter

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dan Carpenter

author

2021-12-16 16:00 UTC #9 lore raw

Quoted 11 lines · Chanwoo Choi

Quoted 4 lines
To be honest, I'm not sure how this differs from other functions which return -EPROBE_DEFER. How do other functions guarantee they will only be called from probe()?
If it is possible to know extcon_get_extcon_dev() will be only callled on probe, it is no problem. But, it is not able to guarantee that extcon_get_extcon_dev() is called on probe. Because of this reason, this issue should be handled in each device driver.
-EPROBE_DEFER is only for probe step. If return -EPROBE_DEFER except for probe, it is wrong return value.

The future is vast and unknowable. We can't really future proof code and we should never try do that if it makes the code more complicated right now.

When Andy submitted basically the same patch as me three years ago we worried about future developers so we didn't merge his patch. But three years later no non-probe() were introduced. Meanwhile the bad API created bugs in the kernel for current users.

To some extent, we have to trust future developers to do sane things.

I have also created a static checker test for people who call EPROBE_DEFER outside of probe functions. I don't know that this test will work. It will take a few days for the call tree to be built. Another option would be to change the warning from "is this called from something besides probe()" to "is this called from an ioctl". That would generate fewer false positives.

Or potentially, I could save a most recent function pointer in the call tree. I'll play around with that in the coming months.

Anyway, I've attached my first draft just to show you my thinking on this.

regards,
dan carpenter

/*
* Copyright (C) 2021 Oracle.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* as published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
*/

/*
* This module answers the question:
* is_probe_function()
* called_from_probe() which returns 1 for yes, -1 for maybe and 0 for no. *
*/

#include "smatch.h"

static int my_id;

STATE(from_probe);
STATE(maybe);

static unsigned long is_probe_fn;

bool is_probe_function(void)

{
	return is_probe_fn;
}

int called_from_probe(void)

{
	struct smatch_state *state;

	if (is_probe_function())
		return 1;

	state = get_state(my_id, "from_probe", NULL);
	if (state == &from_probe)
		return 1;
	if (state == &maybe)
		return -1;

	return 0;
}

static struct smatch_state *merge_func(struct smatch_state *s1, struct smatch_state *s2)

{
	return &maybe;
}

static void match_probe_call(struct expression *expr)

{
	char *name;

	name = get_member_name(expr);
	if (!name)
		return;
	if (strstr(name, "->probe"))
		sql_insert_caller_info(expr, PROBE_FN, -1, "", "");
	free_string(name);
}

static void select_probe_fn(const char *name, struct symbol *sym, char *key, char *value)

{
	is_probe_fn = 1;
}

static void match_call_info(struct expression *expr)

{
	int call;

	call = called_from_probe();
	if (!call)
		return;

	sql_insert_caller_info(expr, CALLED_FROM_PROBE, -1, "", (call == 1) ? "y" : "m");
}

static void select_from_probe(const char *name, struct symbol *sym, char *key, char *value)

{
	if (strcmp(value, "y") == 0)
		set_state(my_id, "from_probe", NULL, &from_probe);
	else
		set_state(my_id, "from_probe", NULL, &maybe);
}

void register_kernel_probe(int id)

{
	my_id = id;

	if (option_project != PROJ_KERNEL)
		return;

	add_merge_hook(my_id, &merge_func);

	add_function_data(&is_probe_fn);
	add_hook(&match_probe_call, FUNCTION_CALL_HOOK);
	select_caller_info_hook(&select_probe_fn, PROBE_FN);

	add_hook(&match_call_info, FUNCTION_CALL_HOOK);
	select_caller_info_hook(&select_from_probe, CALLED_FROM_PROBE);
}

#include "smatch.h"
#include "smatch_slist.h"

static int my_id;

static void check_for_EPROBE_DEFER(struct expression *expr)

{
	sval_t sval;
	char *macro;

	if (!expr || expr->type != EXPR_PREOP || expr->op != '-')
		return;

	expr = expr->unop;
	if (!get_value(expr, &sval) || sval.value != 517)
		return;

	macro = get_macro_name(expr->pos);
	if (!macro || strcmp(macro, "EPROBE_DEFER") != 0)
		return;
	sm_warning("returning EPROBE_DEFER from non probe() function");
}

static void match_assign(struct expression *expr)

{
	/* "ret = ERR_PTR(-EPROBE_DEFER)" generates a fake assignment so
	 * that is covered here.
	 */
	check_for_EPROBE_DEFER(expr->right);
}

static void match_return(struct expression *expr)

{
	check_for_EPROBE_DEFER(expr);
}

void check_EPROBE_DEFER(int id)

{
	my_id = id;

	if (option_project != PROJ_KERNEL)
		return;

	add_hook(&match_assign, ASSIGNMENT_HOOK);
	add_hook(&match_return, RETURN_HOOK);
}

Chanwoo Choi

maintainer

2021-12-17 01:09 UTC #10 lore raw

Quoted 13 lines · Dan Carpenter

Quoted 11 lines · Chanwoo Choi
Quoted 4 lines
To be honest, I'm not sure how this differs from other functions which return -EPROBE_DEFER. How do other functions guarantee they will only be called from probe()?
If it is possible to know extcon_get_extcon_dev() will be only callled on probe, it is no problem. But, it is not able to guarantee that extcon_get_extcon_dev() is called on probe. Because of this reason, this issue should be handled in each device driver.
-EPROBE_DEFER is only for probe step. If return -EPROBE_DEFER except for probe, it is wrong return value.

Quoted 8 lines

The future is vast and unknowable. We can't really future proof code and we should never try do that if it makes the code more complicated right now.
When Andy submitted basically the same patch as me three years ago we worried about future developers so we didn't merge his patch. But three years later no non-probe() were introduced. Meanwhile the bad API created bugs in the kernel for current users.

As you mentioned, there were no use case except for probe step. OK. I agree this approach.

For merging this patch, need to get ack from power-supply and usb maintainer. After getting the ack, I'll merge it. Thanks.

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Hans de Goede

2021-12-16 09:08 UTC #11 lore raw

In reply to Dan Carpenter

Hi,

Quoted 25 lines · Dan Carpenter

Quoted 24 lines · Chanwoo Choi
Quoted 11 lines · Dan Carpenter
Quoted 10 lines · Chanwoo Choi
Hi Dan,
First of all, sorry for late reply.
There is one issue. About this issue, I already discussed on patch[1]
[1] https://lore.kernel.org/lkml/[email protected]/
extcon_get_extcon_dev() is used for anywhere except for probe step. But EPROBE_DEFER is only used on probe step.
Quoted 6 lines
Quoted 3 lines
So that it is not clear to return EPROBE_DEFER from extcon_get_extcon_dev() because extcon_get_extcon_dev() never know either call it on probe function or not.
Currently extcon_get_extcon_dev() is only called from probe so it's not an issue.
Even if extcon_get_extcon_dev() is used on probe until now, it is possible to use on anywhere as I commented.
It is difficult to agree this approach without any other solution.

Quoted 7 lines

Quoted 3 lines
Basically, the subsystem core never know either probe time or not. It means that this issue should be handled in each device driver.
To be honest, I'm not sure how this differs from other functions which return -EPROBE_DEFER. How do other functions guarantee they will only be called from probe()?

Right I have to agree with Dan here, all "get" functons for resources like gpios, clks, regulators, phys, pwms, etc. may return -EPROBE_DEFER and since these functions may return -EPROBE_DEFER they *must* only be called from a driver's probe() function.

So I believe that the solution here is to simply add a kernel-doc comment on extcon_get_extcon_dev() which documents that it may return -EPROBE_DEFER and that it thus *must* only be called from a driver's probe() function.

Regards,

Hans

Quoted 4 lines

regards,
dan carpenter

Dan Carpenter

author

2021-12-17 06:29 UTC #12 lore raw

PATCH v3 1/1 extcon: fix extcon_get_extcon_dev() error handling

The normal way to write this would be to return -EPROBE_DEFER directly when appropriate and return NULL when CONFIG_EXTCON is disabled. Then the error handling is simple and just looks like:

	dev->edev = extcon_get_extcon_dev(acpi_dev_name(adev));
	if (IS_ERR(dev->edev))
		return PTR_ERR(dev->edev);

Signed-off-by Dan Carpenter <[email protected]>

---
v2: return NULL when CONFIG_EXTCON is disabled
v3: Add a note to extcon_get_extcon_dev() to say that it may only be
called from probe().

 drivers/extcon/extcon.c                |  4 +++-
 include/linux/extcon.h                 |  2 +-
 drivers/extcon/extcon-axp288.c         |  4 ++--
 drivers/power/supply/axp288_charger.c  | 17 ++++++++++-------
 drivers/power/supply/charger-manager.c |  7 ++-----
 drivers/power/supply/max8997_charger.c | 10 +++++-----
 drivers/usb/dwc3/drd.c                 |  9 ++-------
 drivers/usb/phy/phy-omap-otg.c         |  4 ++--
 drivers/usb/typec/tcpm/fusb302.c       |  4 ++--
 9 files changed, 29 insertions(+), 32 deletions(-)

drivers/extcon/extcon.c +3 -1

index e7a9561a826d..9eb92997f3ae 100644
@@ -863,6 +863,8 @@ EXPORT_SYMBOL_GPL(extcon_set_property_capability);
  * @extcon_name:	the extcon name provided with extcon_dev_register()
  *
  * Return the pointer of extcon device if success or ERR_PTR(err) if fail.
+ * NOTE: This function returns -EPROBE_DEFER so it may only be called from
+ * probe() functions.
  */
 struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {

@@ -876,7 +878,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;

include/linux/extcon.h +1 -1

index 0c19010da77f..685401d94d39 100644
@@ -296,7 +296,7 @@ static inline void devm_extcon_unregister_notifier_all(struct device *dev,
 
 static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {
-	return ERR_PTR(-ENODEV);
+	return NULL;
 }
 
 static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)

drivers/extcon/extcon-axp288.c +2 -2

index 7c6d5857ff25..180be768c215 100644
@@ -394,8 +394,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		if (adev) {
 			info->id_extcon = extcon_get_extcon_dev(acpi_dev_name(adev));
 			put_device(&adev->dev);
-			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 {

drivers/power/supply/axp288_charger.c +10 -7

index ec41f6cd3f93..4acfeb52a44e 100644
@@ -848,17 +848,20 @@ 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) {
-		dev_dbg(dev, "%s is not ready, probe deferred\n",
-			AXP288_EXTCON_DEV_NAME);
-		return -EPROBE_DEFER;
+	if (IS_ERR(info->cable.edev)) {
+		dev_err_probe(dev, PTR_ERR(info->cable.edev),
+			      "extcon_get_extcon_dev(%s) failed\n",
+			      AXP288_EXTCON_DEV_NAME);
+		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) {
-			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
-			return -EPROBE_DEFER;
+		if (IS_ERR(info->otg.cable)) {
+			dev_err_probe(dev, PTR_ERR(info->otg.cable),
+				      "extcon_get_extcon_dev(%s) failed\n",
+				      USB_HOST_EXTCON_NAME);
+			return PTR_ERR(info->otg.cable);
 		}
 		dev_info(dev, "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
 	}

drivers/power/supply/charger-manager.c +2 -5

index d67edb760c94..92db79400a6a 100644
@@ -985,13 +985,10 @@ static int charger_extcon_init(struct charger_manager *cm,
 	cable->nb.notifier_call = charger_extcon_notifier;
 
 	cable->extcon_dev = extcon_get_extcon_dev(cable->extcon_name);
-	if (IS_ERR_OR_NULL(cable->extcon_dev)) {
+	if (IS_ERR(cable->extcon_dev)) {
 		pr_err("Cannot find extcon_dev for %s (cable: %s)\n",
 			cable->extcon_name, cable->name);
-		if (cable->extcon_dev == NULL)
-			return -EPROBE_DEFER;
-		else
-			return PTR_ERR(cable->extcon_dev);
+		return PTR_ERR(cable->extcon_dev);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(extcon_mapping); i++) {

drivers/power/supply/max8997_charger.c +5 -5

index 25207fe2aa68..634658adf313 100644
@@ -248,13 +248,13 @@ static int max8997_battery_probe(struct platform_device *pdev)
 		dev_info(&pdev->dev, "couldn't get charger regulator\n");
 	}
 	charger->edev = extcon_get_extcon_dev("max8997-muic");
-	if (IS_ERR_OR_NULL(charger->edev)) {
-		if (!charger->edev)
-			return -EPROBE_DEFER;
-		dev_info(charger->dev, "couldn't get extcon device\n");
+	if (IS_ERR(charger->edev)) {
+		dev_err_probe(charger->dev, PTR_ERR(charger->edev),
+			      "couldn't get extcon device: max8997-muic\n");
+		return PTR_ERR(charger->edev);
 	}
 
-	if (!IS_ERR(charger->reg) && !IS_ERR_OR_NULL(charger->edev)) {
+	if (!IS_ERR(charger->reg)) {
 		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
 		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
 		if (ret) {

drivers/usb/dwc3/drd.c +2 -7

index d7f76835137f..a490f79131c1 100644
@@ -454,13 +454,8 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	 * 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) {
-		edev = extcon_get_extcon_dev(name);
-		if (!edev)
-			return ERR_PTR(-EPROBE_DEFER);
-
-		return edev;
-	}
+	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0)
+		return extcon_get_extcon_dev(name);
 
 	/*
 	 * Try to get an extcon device from the USB PHY controller's "port"

drivers/usb/phy/phy-omap-otg.c +2 -2

index ee0863c6553e..6e6ef8c0bc7e 100644
@@ -95,8 +95,8 @@ static int omap_otg_probe(struct platform_device *pdev)
 		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)

drivers/usb/typec/tcpm/fusb302.c +2 -2

index 72f9001b0792..96c55eaf3f80 100644
@@ -1708,8 +1708,8 @@ static int fusb302_probe(struct i2c_client *client,
 	 */
 	if (device_property_read_string(dev, "linux,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");

Signature · 1 line

2.20.1

Chanwoo Choi

maintainer

2021-12-20 00:58 UTC #13 lore raw

Hi Sebastian and Felipe,

If Sebastian and Felipe don't have any additional opinion, could you please reply the review for this patch?

And if Sebastian and Felipe agree, I'll merge it to extcon tree.

Best Regards,
Chanwoo Choi

Quoted 221 lines · Dan Carpenter

The normal way to write this would be to return -EPROBE_DEFER directly when appropriate and return NULL when CONFIG_EXTCON is disabled. Then the error handling is simple and just looks like:

	dev->edev = extcon_get_extcon_dev(acpi_dev_name(adev));
	if (IS_ERR(dev->edev))
		return PTR_ERR(dev->edev);

Signed-off-by Dan Carpenter <[email protected]>

---
v2: return NULL when CONFIG_EXTCON is disabled
v3: Add a note to extcon_get_extcon_dev() to say that it may only be
called from probe().

 drivers/extcon/extcon.c                |  4 +++-
 include/linux/extcon.h                 |  2 +-
 drivers/extcon/extcon-axp288.c         |  4 ++--
 drivers/power/supply/axp288_charger.c  | 17 ++++++++++-------
 drivers/power/supply/charger-manager.c |  7 ++-----
 drivers/power/supply/max8997_charger.c | 10 +++++-----
 drivers/usb/dwc3/drd.c                 |  9 ++-------
 drivers/usb/phy/phy-omap-otg.c         |  4 ++--
 drivers/usb/typec/tcpm/fusb302.c       |  4 ++--
 9 files changed, 29 insertions(+), 32 deletions(-)

drivers/extcon/extcon.c +3 -1

index e7a9561a826d..9eb92997f3ae 100644
@@ -863,6 +863,8 @@ EXPORT_SYMBOL_GPL(extcon_set_property_capability);
  * @extcon_name:	the extcon name provided with extcon_dev_register()
  *
  * Return the pointer of extcon device if success or ERR_PTR(err) if fail.
+ * NOTE: This function returns -EPROBE_DEFER so it may only be called from
+ * probe() functions.
  */
 struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {

@@ -876,7 +878,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;

include/linux/extcon.h +1 -1

index 0c19010da77f..685401d94d39 100644
@@ -296,7 +296,7 @@ static inline void devm_extcon_unregister_notifier_all(struct device *dev,
 
 static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {
-	return ERR_PTR(-ENODEV);
+	return NULL;
 }
 
 static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)

drivers/extcon/extcon-axp288.c +2 -2

index 7c6d5857ff25..180be768c215 100644
@@ -394,8 +394,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		if (adev) {
 			info->id_extcon = extcon_get_extcon_dev(acpi_dev_name(adev));
 			put_device(&adev->dev);
-			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 {

drivers/power/supply/axp288_charger.c +10 -7

index ec41f6cd3f93..4acfeb52a44e 100644
@@ -848,17 +848,20 @@ 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) {
-		dev_dbg(dev, "%s is not ready, probe deferred\n",
-			AXP288_EXTCON_DEV_NAME);
-		return -EPROBE_DEFER;
+	if (IS_ERR(info->cable.edev)) {
+		dev_err_probe(dev, PTR_ERR(info->cable.edev),
+			      "extcon_get_extcon_dev(%s) failed\n",
+			      AXP288_EXTCON_DEV_NAME);
+		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) {
-			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
-			return -EPROBE_DEFER;
+		if (IS_ERR(info->otg.cable)) {
+			dev_err_probe(dev, PTR_ERR(info->otg.cable),
+				      "extcon_get_extcon_dev(%s) failed\n",
+				      USB_HOST_EXTCON_NAME);
+			return PTR_ERR(info->otg.cable);
 		}
 		dev_info(dev, "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
 	}

drivers/power/supply/charger-manager.c +2 -5

index d67edb760c94..92db79400a6a 100644
@@ -985,13 +985,10 @@ static int charger_extcon_init(struct charger_manager *cm,
 	cable->nb.notifier_call = charger_extcon_notifier;
 
 	cable->extcon_dev = extcon_get_extcon_dev(cable->extcon_name);
-	if (IS_ERR_OR_NULL(cable->extcon_dev)) {
+	if (IS_ERR(cable->extcon_dev)) {
 		pr_err("Cannot find extcon_dev for %s (cable: %s)\n",
 			cable->extcon_name, cable->name);
-		if (cable->extcon_dev == NULL)
-			return -EPROBE_DEFER;
-		else
-			return PTR_ERR(cable->extcon_dev);
+		return PTR_ERR(cable->extcon_dev);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(extcon_mapping); i++) {

drivers/power/supply/max8997_charger.c +5 -5

index 25207fe2aa68..634658adf313 100644
@@ -248,13 +248,13 @@ static int max8997_battery_probe(struct platform_device *pdev)
 		dev_info(&pdev->dev, "couldn't get charger regulator\n");
 	}
 	charger->edev = extcon_get_extcon_dev("max8997-muic");
-	if (IS_ERR_OR_NULL(charger->edev)) {
-		if (!charger->edev)
-			return -EPROBE_DEFER;
-		dev_info(charger->dev, "couldn't get extcon device\n");
+	if (IS_ERR(charger->edev)) {
+		dev_err_probe(charger->dev, PTR_ERR(charger->edev),
+			      "couldn't get extcon device: max8997-muic\n");
+		return PTR_ERR(charger->edev);
 	}
 
-	if (!IS_ERR(charger->reg) && !IS_ERR_OR_NULL(charger->edev)) {
+	if (!IS_ERR(charger->reg)) {
 		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
 		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
 		if (ret) {

drivers/usb/dwc3/drd.c +2 -7

index d7f76835137f..a490f79131c1 100644
@@ -454,13 +454,8 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	 * 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) {
-		edev = extcon_get_extcon_dev(name);
-		if (!edev)
-			return ERR_PTR(-EPROBE_DEFER);
-
-		return edev;
-	}
+	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0)
+		return extcon_get_extcon_dev(name);
 
 	/*
 	 * Try to get an extcon device from the USB PHY controller's "port"

drivers/usb/phy/phy-omap-otg.c +2 -2

index ee0863c6553e..6e6ef8c0bc7e 100644
@@ -95,8 +95,8 @@ static int omap_otg_probe(struct platform_device *pdev)
 		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)

drivers/usb/typec/tcpm/fusb302.c +2 -2

index 72f9001b0792..96c55eaf3f80 100644
@@ -1708,8 +1708,8 @@ static int fusb302_probe(struct i2c_client *client,
 	 */
 	if (device_property_read_string(dev, "linux,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");

Chanwoo Choi

maintainer

2022-02-03 05:01 UTC #14 lore raw

Hi Felipe,

Gently Ping.

Best Regards,
Chanwoo Choi

Quoted 238 lines · Chanwoo Choi

Hi Sebastian and Felipe,

If Sebastian and Felipe don't have any additional opinion, could you please reply the review for this patch?

And if Sebastian and Felipe agree, I'll merge it to extcon tree.

Best Regards,
Chanwoo Choi

Quoted 221 lines · Dan Carpenter

The normal way to write this would be to return -EPROBE_DEFER directly when appropriate and return NULL when CONFIG_EXTCON is disabled. Then the error handling is simple and just looks like:

	dev->edev = extcon_get_extcon_dev(acpi_dev_name(adev));
	if (IS_ERR(dev->edev))
		return PTR_ERR(dev->edev);

Signed-off-by Dan Carpenter <[email protected]>

---
v2: return NULL when CONFIG_EXTCON is disabled
v3: Add a note to extcon_get_extcon_dev() to say that it may only be
called from probe().

 drivers/extcon/extcon.c                |  4 +++-
 include/linux/extcon.h                 |  2 +-
 drivers/extcon/extcon-axp288.c         |  4 ++--
 drivers/power/supply/axp288_charger.c  | 17 ++++++++++-------
 drivers/power/supply/charger-manager.c |  7 ++-----
 drivers/power/supply/max8997_charger.c | 10 +++++-----
 drivers/usb/dwc3/drd.c                 |  9 ++-------
 drivers/usb/phy/phy-omap-otg.c         |  4 ++--
 drivers/usb/typec/tcpm/fusb302.c       |  4 ++--
 9 files changed, 29 insertions(+), 32 deletions(-)

drivers/extcon/extcon.c +3 -1

index e7a9561a826d..9eb92997f3ae 100644
@@ -863,6 +863,8 @@ EXPORT_SYMBOL_GPL(extcon_set_property_capability);
  * @extcon_name:	the extcon name provided with extcon_dev_register()
  *
  * Return the pointer of extcon device if success or ERR_PTR(err) if fail.
+ * NOTE: This function returns -EPROBE_DEFER so it may only be called from
+ * probe() functions.
  */
 struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {

@@ -876,7 +878,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;

include/linux/extcon.h +1 -1

index 0c19010da77f..685401d94d39 100644
@@ -296,7 +296,7 @@ static inline void devm_extcon_unregister_notifier_all(struct device *dev,
 
 static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {
-	return ERR_PTR(-ENODEV);
+	return NULL;
 }
 
 static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)

drivers/extcon/extcon-axp288.c +2 -2

index 7c6d5857ff25..180be768c215 100644
@@ -394,8 +394,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		if (adev) {
 			info->id_extcon = extcon_get_extcon_dev(acpi_dev_name(adev));
 			put_device(&adev->dev);
-			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 {

drivers/power/supply/axp288_charger.c +10 -7

index ec41f6cd3f93..4acfeb52a44e 100644
@@ -848,17 +848,20 @@ 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) {
-		dev_dbg(dev, "%s is not ready, probe deferred\n",
-			AXP288_EXTCON_DEV_NAME);
-		return -EPROBE_DEFER;
+	if (IS_ERR(info->cable.edev)) {
+		dev_err_probe(dev, PTR_ERR(info->cable.edev),
+			      "extcon_get_extcon_dev(%s) failed\n",
+			      AXP288_EXTCON_DEV_NAME);
+		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) {
-			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
-			return -EPROBE_DEFER;
+		if (IS_ERR(info->otg.cable)) {
+			dev_err_probe(dev, PTR_ERR(info->otg.cable),
+				      "extcon_get_extcon_dev(%s) failed\n",
+				      USB_HOST_EXTCON_NAME);
+			return PTR_ERR(info->otg.cable);
 		}
 		dev_info(dev, "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
 	}

drivers/power/supply/charger-manager.c +2 -5

index d67edb760c94..92db79400a6a 100644
@@ -985,13 +985,10 @@ static int charger_extcon_init(struct charger_manager *cm,
 	cable->nb.notifier_call = charger_extcon_notifier;
 
 	cable->extcon_dev = extcon_get_extcon_dev(cable->extcon_name);
-	if (IS_ERR_OR_NULL(cable->extcon_dev)) {
+	if (IS_ERR(cable->extcon_dev)) {
 		pr_err("Cannot find extcon_dev for %s (cable: %s)\n",
 			cable->extcon_name, cable->name);
-		if (cable->extcon_dev == NULL)
-			return -EPROBE_DEFER;
-		else
-			return PTR_ERR(cable->extcon_dev);
+		return PTR_ERR(cable->extcon_dev);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(extcon_mapping); i++) {

drivers/power/supply/max8997_charger.c +5 -5

index 25207fe2aa68..634658adf313 100644
@@ -248,13 +248,13 @@ static int max8997_battery_probe(struct platform_device *pdev)
 		dev_info(&pdev->dev, "couldn't get charger regulator\n");
 	}
 	charger->edev = extcon_get_extcon_dev("max8997-muic");
-	if (IS_ERR_OR_NULL(charger->edev)) {
-		if (!charger->edev)
-			return -EPROBE_DEFER;
-		dev_info(charger->dev, "couldn't get extcon device\n");
+	if (IS_ERR(charger->edev)) {
+		dev_err_probe(charger->dev, PTR_ERR(charger->edev),
+			      "couldn't get extcon device: max8997-muic\n");
+		return PTR_ERR(charger->edev);
 	}
 
-	if (!IS_ERR(charger->reg) && !IS_ERR_OR_NULL(charger->edev)) {
+	if (!IS_ERR(charger->reg)) {
 		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
 		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
 		if (ret) {

drivers/usb/dwc3/drd.c +2 -7

index d7f76835137f..a490f79131c1 100644
@@ -454,13 +454,8 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	 * 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) {
-		edev = extcon_get_extcon_dev(name);
-		if (!edev)
-			return ERR_PTR(-EPROBE_DEFER);
-
-		return edev;
-	}
+	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0)
+		return extcon_get_extcon_dev(name);
 
 	/*
 	 * Try to get an extcon device from the USB PHY controller's "port"

drivers/usb/phy/phy-omap-otg.c +2 -2

index ee0863c6553e..6e6ef8c0bc7e 100644
@@ -95,8 +95,8 @@ static int omap_otg_probe(struct platform_device *pdev)
 		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)

drivers/usb/typec/tcpm/fusb302.c +2 -2

index 72f9001b0792..96c55eaf3f80 100644
@@ -1708,8 +1708,8 @@ static int fusb302_probe(struct i2c_client *client,
 	 */
 	if (device_property_read_string(dev, "linux,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");

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Chanwoo Choi

maintainer Applied

2022-02-16 00:48 UTC #15 lore raw

Dear all,

Applied it. Thanks.

Best Regards,
Chanwoo Choi

Quoted 248 lines · Chanwoo Choi

Hi Felipe,

Gently Ping.

Best Regards,
Chanwoo Choi

Quoted 238 lines · Chanwoo Choi

Hi Sebastian and Felipe,

If Sebastian and Felipe don't have any additional opinion, could you please reply the review for this patch?

And if Sebastian and Felipe agree, I'll merge it to extcon tree.

Best Regards,
Chanwoo Choi

Quoted 221 lines · Dan Carpenter

The normal way to write this would be to return -EPROBE_DEFER directly when appropriate and return NULL when CONFIG_EXTCON is disabled. Then the error handling is simple and just looks like:

	dev->edev = extcon_get_extcon_dev(acpi_dev_name(adev));
	if (IS_ERR(dev->edev))
		return PTR_ERR(dev->edev);

Signed-off-by Dan Carpenter <[email protected]>

---
v2: return NULL when CONFIG_EXTCON is disabled
v3: Add a note to extcon_get_extcon_dev() to say that it may only be
called from probe().

 drivers/extcon/extcon.c                |  4 +++-
 include/linux/extcon.h                 |  2 +-
 drivers/extcon/extcon-axp288.c         |  4 ++--
 drivers/power/supply/axp288_charger.c  | 17 ++++++++++-------
 drivers/power/supply/charger-manager.c |  7 ++-----
 drivers/power/supply/max8997_charger.c | 10 +++++-----
 drivers/usb/dwc3/drd.c                 |  9 ++-------
 drivers/usb/phy/phy-omap-otg.c         |  4 ++--
 drivers/usb/typec/tcpm/fusb302.c       |  4 ++--
 9 files changed, 29 insertions(+), 32 deletions(-)

drivers/extcon/extcon.c +3 -1

index e7a9561a826d..9eb92997f3ae 100644
@@ -863,6 +863,8 @@ EXPORT_SYMBOL_GPL(extcon_set_property_capability);
  * @extcon_name:	the extcon name provided with extcon_dev_register()
  *
  * Return the pointer of extcon device if success or ERR_PTR(err) if fail.
+ * NOTE: This function returns -EPROBE_DEFER so it may only be called from
+ * probe() functions.
  */
 struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {

@@ -876,7 +878,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;

include/linux/extcon.h +1 -1

index 0c19010da77f..685401d94d39 100644
@@ -296,7 +296,7 @@ static inline void devm_extcon_unregister_notifier_all(struct device *dev,
 
 static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {
-	return ERR_PTR(-ENODEV);
+	return NULL;
 }
 
 static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)

drivers/extcon/extcon-axp288.c +2 -2

index 7c6d5857ff25..180be768c215 100644
@@ -394,8 +394,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		if (adev) {
 			info->id_extcon = extcon_get_extcon_dev(acpi_dev_name(adev));
 			put_device(&adev->dev);
-			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 {

drivers/power/supply/axp288_charger.c +10 -7

index ec41f6cd3f93..4acfeb52a44e 100644
@@ -848,17 +848,20 @@ 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) {
-		dev_dbg(dev, "%s is not ready, probe deferred\n",
-			AXP288_EXTCON_DEV_NAME);
-		return -EPROBE_DEFER;
+	if (IS_ERR(info->cable.edev)) {
+		dev_err_probe(dev, PTR_ERR(info->cable.edev),
+			      "extcon_get_extcon_dev(%s) failed\n",
+			      AXP288_EXTCON_DEV_NAME);
+		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) {
-			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
-			return -EPROBE_DEFER;
+		if (IS_ERR(info->otg.cable)) {
+			dev_err_probe(dev, PTR_ERR(info->otg.cable),
+				      "extcon_get_extcon_dev(%s) failed\n",
+				      USB_HOST_EXTCON_NAME);
+			return PTR_ERR(info->otg.cable);
 		}
 		dev_info(dev, "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
 	}

drivers/power/supply/charger-manager.c +2 -5

index d67edb760c94..92db79400a6a 100644
@@ -985,13 +985,10 @@ static int charger_extcon_init(struct charger_manager *cm,
 	cable->nb.notifier_call = charger_extcon_notifier;
 
 	cable->extcon_dev = extcon_get_extcon_dev(cable->extcon_name);
-	if (IS_ERR_OR_NULL(cable->extcon_dev)) {
+	if (IS_ERR(cable->extcon_dev)) {
 		pr_err("Cannot find extcon_dev for %s (cable: %s)\n",
 			cable->extcon_name, cable->name);
-		if (cable->extcon_dev == NULL)
-			return -EPROBE_DEFER;
-		else
-			return PTR_ERR(cable->extcon_dev);
+		return PTR_ERR(cable->extcon_dev);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(extcon_mapping); i++) {

drivers/power/supply/max8997_charger.c +5 -5

index 25207fe2aa68..634658adf313 100644
@@ -248,13 +248,13 @@ static int max8997_battery_probe(struct platform_device *pdev)
 		dev_info(&pdev->dev, "couldn't get charger regulator\n");
 	}
 	charger->edev = extcon_get_extcon_dev("max8997-muic");
-	if (IS_ERR_OR_NULL(charger->edev)) {
-		if (!charger->edev)
-			return -EPROBE_DEFER;
-		dev_info(charger->dev, "couldn't get extcon device\n");
+	if (IS_ERR(charger->edev)) {
+		dev_err_probe(charger->dev, PTR_ERR(charger->edev),
+			      "couldn't get extcon device: max8997-muic\n");
+		return PTR_ERR(charger->edev);
 	}
 
-	if (!IS_ERR(charger->reg) && !IS_ERR_OR_NULL(charger->edev)) {
+	if (!IS_ERR(charger->reg)) {
 		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
 		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
 		if (ret) {

drivers/usb/dwc3/drd.c +2 -7

index d7f76835137f..a490f79131c1 100644
@@ -454,13 +454,8 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	 * 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) {
-		edev = extcon_get_extcon_dev(name);
-		if (!edev)
-			return ERR_PTR(-EPROBE_DEFER);
-
-		return edev;
-	}
+	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0)
+		return extcon_get_extcon_dev(name);
 
 	/*
 	 * Try to get an extcon device from the USB PHY controller's "port"

drivers/usb/phy/phy-omap-otg.c +2 -2

index ee0863c6553e..6e6ef8c0bc7e 100644
@@ -95,8 +95,8 @@ static int omap_otg_probe(struct platform_device *pdev)
 		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)

drivers/usb/typec/tcpm/fusb302.c +2 -2

index 72f9001b0792..96c55eaf3f80 100644
@@ -1708,8 +1708,8 @@ static int fusb302_probe(struct i2c_client *client,
 	 */
 	if (device_property_read_string(dev, "linux,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");

Sebastian Reichel

Acked-by on v3 1/1

2022-01-03 17:46 UTC #16 lore raw

In reply to Dan Carpenter

Hi,

Quoted 28 lines · Dan Carpenter

The extcon_get_extcon_dev() function returns error pointers on error, NULL when it's a -EPROBE_DEFER defer situation, and ERR_PTR(-ENODEV) when the CONFIG_EXTCON option is disabled. This is very complicated for the callers to handle and a number of them had bugs that would lead to an Oops.
In real life, there are two things which prevented crashes. First, error pointers would only be returned if there was bug in the caller where they passed a NULL "extcon_name" and none of them do that. Second, only two out of the eight drivers will build when CONFIG_EXTCON is disabled.
The normal way to write this would be to return -EPROBE_DEFER directly when appropriate and return NULL when CONFIG_EXTCON is disabled. Then the error handling is simple and just looks like:
	dev->edev = extcon_get_extcon_dev(acpi_dev_name(adev));
	if (IS_ERR(dev->edev))
		return PTR_ERR(dev->edev);
For the two drivers which can build with CONFIG_EXTCON disabled, then extcon_get_extcon_dev() will now return NULL which is not treated as an error and the probe will continue successfully. Those two drivers are "typec_fusb302" and "max8997-battery". In the original code, the typec_fusb302 driver had an 800ms hang in tcpm_get_current_limit() but now that function is a no-op. For the max8997-battery driver everything should continue working as is.

Quoted 2 lines

Signed-off-by Dan Carpenter <[email protected]>
---

Acked-by Sebastian Reichel <[email protected]>

-- Sebastian

Quoted 193 lines

v2: return NULL when CONFIG_EXTCON is disabled
v3: Add a note to extcon_get_extcon_dev() to say that it may only be
called from probe().

 drivers/extcon/extcon.c                |  4 +++-
 include/linux/extcon.h                 |  2 +-
 drivers/extcon/extcon-axp288.c         |  4 ++--
 drivers/power/supply/axp288_charger.c  | 17 ++++++++++-------
 drivers/power/supply/charger-manager.c |  7 ++-----
 drivers/power/supply/max8997_charger.c | 10 +++++-----
 drivers/usb/dwc3/drd.c                 |  9 ++-------
 drivers/usb/phy/phy-omap-otg.c         |  4 ++--
 drivers/usb/typec/tcpm/fusb302.c       |  4 ++--
 9 files changed, 29 insertions(+), 32 deletions(-)

drivers/extcon/extcon.c +3 -1

index e7a9561a826d..9eb92997f3ae 100644
@@ -863,6 +863,8 @@ EXPORT_SYMBOL_GPL(extcon_set_property_capability);
  * @extcon_name:	the extcon name provided with extcon_dev_register()
  *
  * Return the pointer of extcon device if success or ERR_PTR(err) if fail.
+ * NOTE: This function returns -EPROBE_DEFER so it may only be called from
+ * probe() functions.
  */
 struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {

@@ -876,7 +878,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;

include/linux/extcon.h +1 -1

index 0c19010da77f..685401d94d39 100644
@@ -296,7 +296,7 @@ static inline void devm_extcon_unregister_notifier_all(struct device *dev,
 
 static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {
-	return ERR_PTR(-ENODEV);
+	return NULL;
 }
 
 static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)

drivers/extcon/extcon-axp288.c +2 -2

index 7c6d5857ff25..180be768c215 100644
@@ -394,8 +394,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		if (adev) {
 			info->id_extcon = extcon_get_extcon_dev(acpi_dev_name(adev));
 			put_device(&adev->dev);
-			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 {

drivers/power/supply/axp288_charger.c +10 -7

index ec41f6cd3f93..4acfeb52a44e 100644
@@ -848,17 +848,20 @@ 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) {
-		dev_dbg(dev, "%s is not ready, probe deferred\n",
-			AXP288_EXTCON_DEV_NAME);
-		return -EPROBE_DEFER;
+	if (IS_ERR(info->cable.edev)) {
+		dev_err_probe(dev, PTR_ERR(info->cable.edev),
+			      "extcon_get_extcon_dev(%s) failed\n",
+			      AXP288_EXTCON_DEV_NAME);
+		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) {
-			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
-			return -EPROBE_DEFER;
+		if (IS_ERR(info->otg.cable)) {
+			dev_err_probe(dev, PTR_ERR(info->otg.cable),
+				      "extcon_get_extcon_dev(%s) failed\n",
+				      USB_HOST_EXTCON_NAME);
+			return PTR_ERR(info->otg.cable);
 		}
 		dev_info(dev, "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
 	}

drivers/power/supply/charger-manager.c +2 -5

index d67edb760c94..92db79400a6a 100644
@@ -985,13 +985,10 @@ static int charger_extcon_init(struct charger_manager *cm,
 	cable->nb.notifier_call = charger_extcon_notifier;
 
 	cable->extcon_dev = extcon_get_extcon_dev(cable->extcon_name);
-	if (IS_ERR_OR_NULL(cable->extcon_dev)) {
+	if (IS_ERR(cable->extcon_dev)) {
 		pr_err("Cannot find extcon_dev for %s (cable: %s)\n",
 			cable->extcon_name, cable->name);
-		if (cable->extcon_dev == NULL)
-			return -EPROBE_DEFER;
-		else
-			return PTR_ERR(cable->extcon_dev);
+		return PTR_ERR(cable->extcon_dev);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(extcon_mapping); i++) {

drivers/power/supply/max8997_charger.c +5 -5

index 25207fe2aa68..634658adf313 100644
@@ -248,13 +248,13 @@ static int max8997_battery_probe(struct platform_device *pdev)
 		dev_info(&pdev->dev, "couldn't get charger regulator\n");
 	}
 	charger->edev = extcon_get_extcon_dev("max8997-muic");
-	if (IS_ERR_OR_NULL(charger->edev)) {
-		if (!charger->edev)
-			return -EPROBE_DEFER;
-		dev_info(charger->dev, "couldn't get extcon device\n");
+	if (IS_ERR(charger->edev)) {
+		dev_err_probe(charger->dev, PTR_ERR(charger->edev),
+			      "couldn't get extcon device: max8997-muic\n");
+		return PTR_ERR(charger->edev);
 	}
 
-	if (!IS_ERR(charger->reg) && !IS_ERR_OR_NULL(charger->edev)) {
+	if (!IS_ERR(charger->reg)) {
 		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
 		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
 		if (ret) {

drivers/usb/dwc3/drd.c +2 -7

index d7f76835137f..a490f79131c1 100644
@@ -454,13 +454,8 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	 * 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) {
-		edev = extcon_get_extcon_dev(name);
-		if (!edev)
-			return ERR_PTR(-EPROBE_DEFER);
-
-		return edev;
-	}
+	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0)
+		return extcon_get_extcon_dev(name);
 
 	/*
 	 * Try to get an extcon device from the USB PHY controller's "port"

drivers/usb/phy/phy-omap-otg.c +2 -2

index ee0863c6553e..6e6ef8c0bc7e 100644
@@ -95,8 +95,8 @@ static int omap_otg_probe(struct platform_device *pdev)
 		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)

drivers/usb/typec/tcpm/fusb302.c +2 -2

index 72f9001b0792..96c55eaf3f80 100644
@@ -1708,8 +1708,8 @@ static int fusb302_probe(struct i2c_client *client,
 	 */
 	if (device_property_read_string(dev, "linux,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");

Signature · 1 line

2.20.1