* Re: [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling
2021-11-23 8:43 ` [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling Dan Carpenter
@ 2021-11-23 14:14 ` Hans de Goede
2021-11-23 14:48 ` Guenter Roeck
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2021-11-23 14:14 UTC (permalink / raw)
To: Dan Carpenter, MyungJoo Ham, Guenter Roeck
Cc: Chanwoo Choi, Sebastian Reichel, Chen-Yu Tsai, Felipe Balbi,
Greg Kroah-Hartman, Heikki Krogerus, linux-kernel, linux-pm,
linux-usb, linux-omap, kernel-janitors
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");
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling
2021-11-23 8:43 ` [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling Dan Carpenter
2021-11-23 14:14 ` Hans de Goede
@ 2021-11-23 14:48 ` Guenter Roeck
2021-11-23 15:20 ` Heikki Krogerus
2021-12-16 6:39 ` Chanwoo Choi
3 siblings, 0 replies; 17+ messages in thread
From: Guenter Roeck @ 2021-11-23 14:48 UTC (permalink / raw)
To: Dan Carpenter, MyungJoo Ham
Cc: Chanwoo Choi, Sebastian Reichel, Chen-Yu Tsai, Hans de Goede,
Felipe Balbi, Greg Kroah-Hartman, Heikki Krogerus, linux-kernel,
linux-pm, linux-usb, linux-omap, kernel-janitors
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling
2021-11-23 8:43 ` [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling Dan Carpenter
2021-11-23 14:14 ` Hans de Goede
2021-11-23 14:48 ` Guenter Roeck
@ 2021-11-23 15:20 ` Heikki Krogerus
2021-12-16 6:39 ` Chanwoo Choi
3 siblings, 0 replies; 17+ messages in thread
From: Heikki Krogerus @ 2021-11-23 15:20 UTC (permalink / raw)
To: Dan Carpenter
Cc: MyungJoo Ham, Guenter Roeck, Chanwoo Choi, Sebastian Reichel,
Chen-Yu Tsai, Hans de Goede, Felipe Balbi, Greg Kroah-Hartman,
linux-kernel, linux-pm, linux-usb, linux-omap, kernel-janitors
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");
--
heikki
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling
2021-11-23 8:43 ` [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling Dan Carpenter
` (2 preceding siblings ...)
2021-11-23 15:20 ` Heikki Krogerus
@ 2021-12-16 6:39 ` Chanwoo Choi
2021-12-16 7:52 ` Dan Carpenter
3 siblings, 1 reply; 17+ messages in thread
From: Chanwoo Choi @ 2021-12-16 6:39 UTC (permalink / raw)
To: Dan Carpenter, MyungJoo Ham, Guenter Roeck
Cc: Sebastian Reichel, Chen-Yu Tsai, Hans de Goede, Felipe Balbi,
Greg Kroah-Hartman, Heikki Krogerus, linux-kernel, linux-pm,
linux-usb, linux-omap, kernel-janitors
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");
>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling
2021-12-16 6:39 ` Chanwoo Choi
@ 2021-12-16 7:52 ` Dan Carpenter
2021-12-16 8:24 ` Chanwoo Choi
0 siblings, 1 reply; 17+ messages in thread
From: Dan Carpenter @ 2021-12-16 7:52 UTC (permalink / raw)
To: Chanwoo Choi
Cc: MyungJoo Ham, Guenter Roeck, Sebastian Reichel, Chen-Yu Tsai,
Hans de Goede, Felipe Balbi, Greg Kroah-Hartman, Heikki Krogerus,
linux-kernel, linux-pm, linux-usb, linux-omap, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling
2021-12-16 7:52 ` Dan Carpenter
@ 2021-12-16 8:24 ` Chanwoo Choi
2021-12-16 8:05 ` Dan Carpenter
0 siblings, 1 reply; 17+ messages in thread
From: Chanwoo Choi @ 2021-12-16 8:24 UTC (permalink / raw)
To: Dan Carpenter
Cc: MyungJoo Ham, Guenter Roeck, Sebastian Reichel, Chen-Yu Tsai,
Hans de Goede, Felipe Balbi, Greg Kroah-Hartman, Heikki Krogerus,
linux-kernel, linux-pm, linux-usb, linux-omap, kernel-janitors
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.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling
2021-12-16 8:24 ` Chanwoo Choi
@ 2021-12-16 8:05 ` Dan Carpenter
2021-12-16 8:38 ` Chanwoo Choi
2021-12-16 9:08 ` Hans de Goede
0 siblings, 2 replies; 17+ messages in thread
From: Dan Carpenter @ 2021-12-16 8:05 UTC (permalink / raw)
To: Chanwoo Choi
Cc: MyungJoo Ham, Guenter Roeck, Sebastian Reichel, Chen-Yu Tsai,
Hans de Goede, Felipe Balbi, Greg Kroah-Hartman, Heikki Krogerus,
linux-kernel, linux-pm, linux-usb, linux-omap, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling
2021-12-16 8:05 ` Dan Carpenter
@ 2021-12-16 8:38 ` Chanwoo Choi
2021-12-16 15:59 ` Dan Carpenter
2021-12-16 9:08 ` Hans de Goede
1 sibling, 1 reply; 17+ messages in thread
From: Chanwoo Choi @ 2021-12-16 8:38 UTC (permalink / raw)
To: Dan Carpenter
Cc: MyungJoo Ham, Guenter Roeck, Sebastian Reichel, Chen-Yu Tsai,
Hans de Goede, Felipe Balbi, Greg Kroah-Hartman, Heikki Krogerus,
linux-kernel, linux-pm, linux-usb, linux-omap, kernel-janitors
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
>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling
2021-12-16 8:38 ` Chanwoo Choi
@ 2021-12-16 15:59 ` Dan Carpenter
2021-12-17 1:31 ` Chanwoo Choi
0 siblings, 1 reply; 17+ messages in thread
From: Dan Carpenter @ 2021-12-16 15:59 UTC (permalink / raw)
To: Chanwoo Choi
Cc: MyungJoo Ham, Guenter Roeck, Sebastian Reichel, Chen-Yu Tsai,
Hans de Goede, Felipe Balbi, Greg Kroah-Hartman, Heikki Krogerus,
linux-kernel, linux-pm, linux-usb, linux-omap, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]
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
[-- Attachment #2: smatch_kernel_probe.c --]
[-- Type: text/x-csrc, Size: 2518 bytes --]
/*
* 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);
}
[-- Attachment #3: check_EPROBE_DEFER.c --]
[-- Type: text/x-csrc, Size: 1619 bytes --]
/*
* 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
*/
#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);
}
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling
2021-12-16 15:59 ` Dan Carpenter
@ 2021-12-17 1:31 ` Chanwoo Choi
0 siblings, 0 replies; 17+ messages in thread
From: Chanwoo Choi @ 2021-12-17 1:31 UTC (permalink / raw)
To: Dan Carpenter
Cc: MyungJoo Ham, Guenter Roeck, Sebastian Reichel, Chen-Yu Tsai,
Hans de Goede, Felipe Balbi, Greg Kroah-Hartman, Heikki Krogerus,
linux-kernel, linux-pm, linux-usb, linux-omap, kernel-janitors
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.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] extcon: fix extcon_get_extcon_dev() error handling
2021-12-16 8:05 ` Dan Carpenter
2021-12-16 8:38 ` Chanwoo Choi
@ 2021-12-16 9:08 ` Hans de Goede
2021-12-17 6:28 ` [PATCH v3] " Dan Carpenter
1 sibling, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2021-12-16 9:08 UTC (permalink / raw)
To: Dan Carpenter, Chanwoo Choi
Cc: MyungJoo Ham, Guenter Roeck, Sebastian Reichel, Chen-Yu Tsai,
Felipe Balbi, Greg Kroah-Hartman, Heikki Krogerus, linux-kernel,
linux-pm, linux-usb, linux-omap, kernel-janitors
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3] extcon: fix extcon_get_extcon_dev() error handling
2021-12-16 9:08 ` Hans de Goede
@ 2021-12-17 6:28 ` Dan Carpenter
2021-12-20 1:20 ` Chanwoo Choi
2022-01-03 17:46 ` Sebastian Reichel
0 siblings, 2 replies; 17+ messages in thread
From: Dan Carpenter @ 2021-12-17 6:28 UTC (permalink / raw)
To: MyungJoo Ham, Guenter Roeck
Cc: Chanwoo Choi, Sebastian Reichel, Chen-Yu Tsai, Hans de Goede,
Felipe Balbi, Greg Kroah-Hartman, Heikki Krogerus, linux-kernel,
linux-pm, linux-usb, linux-omap, kernel-janitors
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
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(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index e7a9561a826d..9eb92997f3ae 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -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;
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");
--
2.20.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3] extcon: fix extcon_get_extcon_dev() error handling
2021-12-17 6:28 ` [PATCH v3] " Dan Carpenter
@ 2021-12-20 1:20 ` Chanwoo Choi
2022-02-03 5:24 ` Chanwoo Choi
2022-01-03 17:46 ` Sebastian Reichel
1 sibling, 1 reply; 17+ messages in thread
From: Chanwoo Choi @ 2021-12-20 1:20 UTC (permalink / raw)
To: Dan Carpenter, MyungJoo Ham, Guenter Roeck, Sebastian Reichel,
Felipe Balbi
Cc: Chen-Yu Tsai, Hans de Goede, Greg Kroah-Hartman, Heikki Krogerus,
linux-kernel, linux-pm, linux-usb, linux-omap, kernel-janitors
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
On 12/17/21 3:28 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
> 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(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index e7a9561a826d..9eb92997f3ae 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -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;
> 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");
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3] extcon: fix extcon_get_extcon_dev() error handling
2021-12-20 1:20 ` Chanwoo Choi
@ 2022-02-03 5:24 ` Chanwoo Choi
2022-02-16 1:12 ` Chanwoo Choi
0 siblings, 1 reply; 17+ messages in thread
From: Chanwoo Choi @ 2022-02-03 5:24 UTC (permalink / raw)
To: Felipe Balbi
Cc: Dan Carpenter, MyungJoo Ham, Guenter Roeck, Sebastian Reichel,
Chen-Yu Tsai, Hans de Goede, Greg Kroah-Hartman, Heikki Krogerus,
linux-kernel, linux-pm, linux-usb, linux-omap, kernel-janitors
Hi Felipe,
Gently Ping.
Best Regards,
Chanwoo Choi
On 12/20/21 10:20 AM, Chanwoo Choi wrote:
> 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
>
>
> On 12/17/21 3:28 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
>> 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(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index e7a9561a826d..9eb92997f3ae 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -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;
>> 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");
>>
>
>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3] extcon: fix extcon_get_extcon_dev() error handling
2022-02-03 5:24 ` Chanwoo Choi
@ 2022-02-16 1:12 ` Chanwoo Choi
0 siblings, 0 replies; 17+ messages in thread
From: Chanwoo Choi @ 2022-02-16 1:12 UTC (permalink / raw)
To: Felipe Balbi
Cc: Dan Carpenter, MyungJoo Ham, Guenter Roeck, Sebastian Reichel,
Chen-Yu Tsai, Hans de Goede, Greg Kroah-Hartman, Heikki Krogerus,
linux-kernel, linux-pm, linux-usb, linux-omap, kernel-janitors
Dear all,
Applied it. Thanks.
Best Regards,
Chanwoo Choi
On 2/3/22 2:24 PM, Chanwoo Choi wrote:
> Hi Felipe,
>
> Gently Ping.
>
> Best Regards,
> Chanwoo Choi
>
> On 12/20/21 10:20 AM, Chanwoo Choi wrote:
>> 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
>>
>>
>> On 12/17/21 3:28 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
>>> 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(-)
>>>
>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>> index e7a9561a826d..9eb92997f3ae 100644
>>> --- a/drivers/extcon/extcon.c
>>> +++ b/drivers/extcon/extcon.c
>>> @@ -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;
>>> 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");
>>>
>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3] extcon: fix extcon_get_extcon_dev() error handling
2021-12-17 6:28 ` [PATCH v3] " Dan Carpenter
2021-12-20 1:20 ` Chanwoo Choi
@ 2022-01-03 17:46 ` Sebastian Reichel
1 sibling, 0 replies; 17+ messages in thread
From: Sebastian Reichel @ 2022-01-03 17:46 UTC (permalink / raw)
To: Dan Carpenter
Cc: MyungJoo Ham, Guenter Roeck, Chanwoo Choi, Chen-Yu Tsai,
Hans de Goede, Felipe Balbi, Greg Kroah-Hartman, Heikki Krogerus,
linux-kernel, linux-pm, linux-usb, linux-omap, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 9682 bytes --]
Hi,
On Fri, Dec 17, 2021 at 09:28:46AM +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>
> ---
Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>
-- Sebastian
> 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(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index e7a9561a826d..9eb92997f3ae 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -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;
> 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");
> --
> 2.20.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread