| Message ID | 20211123083925.GA3277@kili (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2] extcon: fix extcon_get_extcon_dev() error handling | expand |
Hi, On 11/23/21 09:43, Dan Carpenter wrote: > 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 <dan.carpenter@oracle.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > 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(-) > > diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c > index e7a9561a826d..a35e99928807 100644 > --- a/drivers/extcon/extcon.c > +++ b/drivers/extcon/extcon.c > @@ -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; > diff --git a/include/linux/extcon.h b/include/linux/extcon.h > index 0c19010da77f..685401d94d39 100644 > --- a/include/linux/extcon.h > +++ b/include/linux/extcon.h > @@ -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) > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > index 7c6d5857ff25..180be768c215 100644 > --- a/drivers/extcon/extcon-axp288.c > +++ b/drivers/extcon/extcon-axp288.c > @@ -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 { > diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c > index ec41f6cd3f93..4acfeb52a44e 100644 > --- a/drivers/power/supply/axp288_charger.c > +++ b/drivers/power/supply/axp288_charger.c > @@ -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"); > } > diff --git a/drivers/power/supply/charger-manager.c b/drivers/power/supply/charger-manager.c > index d67edb760c94..92db79400a6a 100644 > --- a/drivers/power/supply/charger-manager.c > +++ b/drivers/power/supply/charger-manager.c > @@ -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++) { > diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c > index 25207fe2aa68..634658adf313 100644 > --- a/drivers/power/supply/max8997_charger.c > +++ b/drivers/power/supply/max8997_charger.c > @@ -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) { > diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c > index d7f76835137f..a490f79131c1 100644 > --- a/drivers/usb/dwc3/drd.c > +++ b/drivers/usb/dwc3/drd.c > @@ -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" > diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c > index ee0863c6553e..6e6ef8c0bc7e 100644 > --- a/drivers/usb/phy/phy-omap-otg.c > +++ b/drivers/usb/phy/phy-omap-otg.c > @@ -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) > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index 72f9001b0792..96c55eaf3f80 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -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"); >
On 11/23/21 12:43 AM, Dan Carpenter wrote: > 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 <dan.carpenter@oracle.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
On Tue, Nov 23, 2021 at 11:43:06AM +0300, Dan Carpenter wrote: > 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 <dan.carpenter@oracle.com> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > 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(-) > > diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c > index e7a9561a826d..a35e99928807 100644 > --- a/drivers/extcon/extcon.c > +++ b/drivers/extcon/extcon.c > @@ -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; > diff --git a/include/linux/extcon.h b/include/linux/extcon.h > index 0c19010da77f..685401d94d39 100644 > --- a/include/linux/extcon.h > +++ b/include/linux/extcon.h > @@ -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) > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > index 7c6d5857ff25..180be768c215 100644 > --- a/drivers/extcon/extcon-axp288.c > +++ b/drivers/extcon/extcon-axp288.c > @@ -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 { > diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c > index ec41f6cd3f93..4acfeb52a44e 100644 > --- a/drivers/power/supply/axp288_charger.c > +++ b/drivers/power/supply/axp288_charger.c > @@ -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"); > } > diff --git a/drivers/power/supply/charger-manager.c b/drivers/power/supply/charger-manager.c > index d67edb760c94..92db79400a6a 100644 > --- a/drivers/power/supply/charger-manager.c > +++ b/drivers/power/supply/charger-manager.c > @@ -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++) { > diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c > index 25207fe2aa68..634658adf313 100644 > --- a/drivers/power/supply/max8997_charger.c > +++ b/drivers/power/supply/max8997_charger.c > @@ -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) { > diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c > index d7f76835137f..a490f79131c1 100644 > --- a/drivers/usb/dwc3/drd.c > +++ b/drivers/usb/dwc3/drd.c > @@ -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" > diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c > index ee0863c6553e..6e6ef8c0bc7e 100644 > --- a/drivers/usb/phy/phy-omap-otg.c > +++ b/drivers/usb/phy/phy-omap-otg.c > @@ -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) > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index 72f9001b0792..96c55eaf3f80 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -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");
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/5BEB63C3.1020504@samsung.com/ 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. On 11/23/21 5:43 PM, Dan Carpenter wrote: > 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 <dan.carpenter@oracle.com> > --- > 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(-) > > diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c > index e7a9561a826d..a35e99928807 100644 > --- a/drivers/extcon/extcon.c > +++ b/drivers/extcon/extcon.c > @@ -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; > diff --git a/include/linux/extcon.h b/include/linux/extcon.h > index 0c19010da77f..685401d94d39 100644 > --- a/include/linux/extcon.h > +++ b/include/linux/extcon.h > @@ -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) > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > index 7c6d5857ff25..180be768c215 100644 > --- a/drivers/extcon/extcon-axp288.c > +++ b/drivers/extcon/extcon-axp288.c > @@ -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 { > diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c > index ec41f6cd3f93..4acfeb52a44e 100644 > --- a/drivers/power/supply/axp288_charger.c > +++ b/drivers/power/supply/axp288_charger.c > @@ -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"); > } > diff --git a/drivers/power/supply/charger-manager.c b/drivers/power/supply/charger-manager.c > index d67edb760c94..92db79400a6a 100644 > --- a/drivers/power/supply/charger-manager.c > +++ b/drivers/power/supply/charger-manager.c > @@ -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++) { > diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c > index 25207fe2aa68..634658adf313 100644 > --- a/drivers/power/supply/max8997_charger.c > +++ b/drivers/power/supply/max8997_charger.c > @@ -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) { > diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c > index d7f76835137f..a490f79131c1 100644 > --- a/drivers/usb/dwc3/drd.c > +++ b/drivers/usb/dwc3/drd.c > @@ -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" > diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c > index ee0863c6553e..6e6ef8c0bc7e 100644 > --- a/drivers/usb/phy/phy-omap-otg.c > +++ b/drivers/usb/phy/phy-omap-otg.c > @@ -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) > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index 72f9001b0792..96c55eaf3f80 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -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"); > > >
On Thu, Dec 16, 2021 at 03:39:46PM +0900, Chanwoo Choi wrote: > 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/5BEB63C3.1020504@samsung.com/ > > 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
On Thu, Dec 16, 2021 at 05:24:30PM +0900, Chanwoo Choi wrote: > On 12/16/21 4:52 PM, Dan Carpenter wrote: > > On Thu, Dec 16, 2021 at 03:39:46PM +0900, Chanwoo Choi wrote: > >> 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/5BEB63C3.1020504@samsung.com/ > >> > >> 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. > > 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
On 12/16/21 4:52 PM, Dan Carpenter wrote: > On Thu, Dec 16, 2021 at 03:39:46PM +0900, Chanwoo Choi wrote: >> 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/5BEB63C3.1020504@samsung.com/ >> >> 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. 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. > > It's impossible to know what future programmers will want, but I feel > like this change probably makes it easier for them.
On 12/16/21 5:05 PM, Dan Carpenter wrote: > On Thu, Dec 16, 2021 at 05:24:30PM +0900, Chanwoo Choi wrote: >> On 12/16/21 4:52 PM, Dan Carpenter wrote: >>> On Thu, Dec 16, 2021 at 03:39:46PM +0900, Chanwoo Choi wrote: >>>> 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/5BEB63C3.1020504@samsung.com/ >>>> >>>> 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. >> >> 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()? 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. > > regards, > dan carpenter > > >
Hi, On 12/16/21 09:05, Dan Carpenter wrote: > On Thu, Dec 16, 2021 at 05:24:30PM +0900, Chanwoo Choi wrote: >> On 12/16/21 4:52 PM, Dan Carpenter wrote: >>> On Thu, Dec 16, 2021 at 03:39:46PM +0900, Chanwoo Choi wrote: >>>> 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/5BEB63C3.1020504@samsung.com/ >>>> >>>> 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. >> >> 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()? 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 > > regards, > dan carpenter >
On Thu, Dec 16, 2021 at 05:38:04PM +0900, Chanwoo Choi wrote: > > > > 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
On 12/17/21 12:59 AM, Dan Carpenter wrote: > On Thu, Dec 16, 2021 at 05:38:04PM +0900, Chanwoo Choi wrote: >>> >>> 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. 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.
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c index e7a9561a826d..a35e99928807 100644 --- a/drivers/extcon/extcon.c +++ b/drivers/extcon/extcon.c @@ -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; diff --git a/include/linux/extcon.h b/include/linux/extcon.h index 0c19010da77f..685401d94d39 100644 --- a/include/linux/extcon.h +++ b/include/linux/extcon.h @@ -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) diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c index 7c6d5857ff25..180be768c215 100644 --- a/drivers/extcon/extcon-axp288.c +++ b/drivers/extcon/extcon-axp288.c @@ -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 { diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c index ec41f6cd3f93..4acfeb52a44e 100644 --- a/drivers/power/supply/axp288_charger.c +++ b/drivers/power/supply/axp288_charger.c @@ -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"); } diff --git a/drivers/power/supply/charger-manager.c b/drivers/power/supply/charger-manager.c index d67edb760c94..92db79400a6a 100644 --- a/drivers/power/supply/charger-manager.c +++ b/drivers/power/supply/charger-manager.c @@ -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++) { diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c index 25207fe2aa68..634658adf313 100644 --- a/drivers/power/supply/max8997_charger.c +++ b/drivers/power/supply/max8997_charger.c @@ -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) { diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c index d7f76835137f..a490f79131c1 100644 --- a/drivers/usb/dwc3/drd.c +++ b/drivers/usb/dwc3/drd.c @@ -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" diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c index ee0863c6553e..6e6ef8c0bc7e 100644 --- a/drivers/usb/phy/phy-omap-otg.c +++ b/drivers/usb/phy/phy-omap-otg.c @@ -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) diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c index 72f9001b0792..96c55eaf3f80 100644 --- a/drivers/usb/typec/tcpm/fusb302.c +++ b/drivers/usb/typec/tcpm/fusb302.c @@ -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");
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 <dan.carpenter@oracle.com> --- 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(-)