| Message ID | 20181110181101.24557-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v1,1/5] drivercore: Revert "deferral race condition fix" | expand |
On 11/10/18 10:10 AM, Andy Shevchenko wrote: > All current users of extcon_get_extcon_dev() API considers > an extcon device a mandatory to appear. Thus, they all convert > NULL pointer to -EPROBE_DEFER error code. > > There is one more caller anticipated with the same requirements. > > To decrease a code duplication and a burden to the callers, > return -EPROBE_DEFER directly from extcon_get_extcon_dev(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Acked-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/extcon/extcon-axp288.c | 4 ++-- > drivers/extcon/extcon.c | 2 +- > drivers/power/supply/axp288_charger.c | 8 ++++---- > drivers/usb/phy/phy-omap-otg.c | 6 +++--- > drivers/usb/typec/tcpm/fusb302.c | 4 ++-- > 5 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > index a983708b77a6..3472d3b756ed 100644 > --- a/drivers/extcon/extcon-axp288.c > +++ b/drivers/extcon/extcon-axp288.c > @@ -360,8 +360,8 @@ static int axp288_extcon_probe(struct platform_device *pdev) > name = acpi_dev_get_first_match_name("INT3496", NULL, -1); > if (name) { > info->id_extcon = extcon_get_extcon_dev(name); > - if (!info->id_extcon) > - return -EPROBE_DEFER; > + if (IS_ERR(info->id_extcon)) > + return PTR_ERR(info->id_extcon); > > dev_info(dev, "controlling USB role\n"); > } else { > diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c > index 5ab0498be652..2bd0f2f33f05 100644 > --- a/drivers/extcon/extcon.c > +++ b/drivers/extcon/extcon.c > @@ -884,7 +884,7 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name) > if (!strcmp(sd->name, extcon_name)) > goto out; > } > - sd = NULL; > + sd = ERR_PTR(-EPROBE_DEFER); > out: > mutex_unlock(&extcon_dev_list_lock); > return sd; > diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c > index 735658ee1c60..8558577fccf5 100644 > --- a/drivers/power/supply/axp288_charger.c > +++ b/drivers/power/supply/axp288_charger.c > @@ -768,17 +768,17 @@ static int axp288_charger_probe(struct platform_device *pdev) > info->regmap_irqc = axp20x->regmap_irqc; > > info->cable.edev = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME); > - if (info->cable.edev == NULL) { > + if (IS_ERR(info->cable.edev)) { > dev_dbg(&pdev->dev, "%s is not ready, probe deferred\n", > AXP288_EXTCON_DEV_NAME); > - return -EPROBE_DEFER; > + return PTR_ERR(info->cable.edev); > } > > if (acpi_dev_present(USB_HOST_EXTCON_HID, NULL, -1)) { > info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_NAME); > - if (info->otg.cable == NULL) { > + if (IS_ERR(info->otg.cable)) { > dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n"); > - return -EPROBE_DEFER; > + return PTR_ERR(info->otg.cable); > } > dev_info(&pdev->dev, > "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n"); > diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c > index ee0863c6553e..605314ddcd3d 100644 > --- a/drivers/usb/phy/phy-omap-otg.c > +++ b/drivers/usb/phy/phy-omap-otg.c > @@ -91,12 +91,12 @@ static int omap_otg_probe(struct platform_device *pdev) > int ret; > u32 rev; > > - if (!config || !config->extcon) > + if (!config) > return -ENODEV; > > extcon = extcon_get_extcon_dev(config->extcon); > - if (!extcon) > - return -EPROBE_DEFER; > + if (IS_ERR(extcon)) > + return PTR_ERR(extcon); > > otg_dev = devm_kzalloc(&pdev->dev, sizeof(*otg_dev), GFP_KERNEL); > if (!otg_dev) > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index 43b64d9309d0..6d332066202b 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -1767,8 +1767,8 @@ static int fusb302_probe(struct i2c_client *client, > */ > if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) { > chip->extcon = extcon_get_extcon_dev(name); > - if (!chip->extcon) > - return -EPROBE_DEFER; > + if (IS_ERR(chip->extcon)) > + return PTR_ERR(chip->extcon); > } > > chip->vbus = devm_regulator_get(chip->dev, "vbus"); >
Hi, On Sat, Nov 10, 2018 at 08:10:58PM +0200, Andy Shevchenko wrote: > All current users of extcon_get_extcon_dev() API considers > an extcon device a mandatory to appear. Thus, they all convert > NULL pointer to -EPROBE_DEFER error code. > > There is one more caller anticipated with the same requirements. > > To decrease a code duplication and a burden to the callers, > return -EPROBE_DEFER directly from extcon_get_extcon_dev(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com> -- Sebastian > drivers/extcon/extcon-axp288.c | 4 ++-- > drivers/extcon/extcon.c | 2 +- > drivers/power/supply/axp288_charger.c | 8 ++++---- > drivers/usb/phy/phy-omap-otg.c | 6 +++--- > drivers/usb/typec/tcpm/fusb302.c | 4 ++-- > 5 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > index a983708b77a6..3472d3b756ed 100644 > --- a/drivers/extcon/extcon-axp288.c > +++ b/drivers/extcon/extcon-axp288.c > @@ -360,8 +360,8 @@ static int axp288_extcon_probe(struct platform_device *pdev) > name = acpi_dev_get_first_match_name("INT3496", NULL, -1); > if (name) { > info->id_extcon = extcon_get_extcon_dev(name); > - if (!info->id_extcon) > - return -EPROBE_DEFER; > + if (IS_ERR(info->id_extcon)) > + return PTR_ERR(info->id_extcon); > > dev_info(dev, "controlling USB role\n"); > } else { > diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c > index 5ab0498be652..2bd0f2f33f05 100644 > --- a/drivers/extcon/extcon.c > +++ b/drivers/extcon/extcon.c > @@ -884,7 +884,7 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name) > if (!strcmp(sd->name, extcon_name)) > goto out; > } > - sd = NULL; > + sd = ERR_PTR(-EPROBE_DEFER); > out: > mutex_unlock(&extcon_dev_list_lock); > return sd; > diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c > index 735658ee1c60..8558577fccf5 100644 > --- a/drivers/power/supply/axp288_charger.c > +++ b/drivers/power/supply/axp288_charger.c > @@ -768,17 +768,17 @@ static int axp288_charger_probe(struct platform_device *pdev) > info->regmap_irqc = axp20x->regmap_irqc; > > info->cable.edev = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME); > - if (info->cable.edev == NULL) { > + if (IS_ERR(info->cable.edev)) { > dev_dbg(&pdev->dev, "%s is not ready, probe deferred\n", > AXP288_EXTCON_DEV_NAME); > - return -EPROBE_DEFER; > + return PTR_ERR(info->cable.edev); > } > > if (acpi_dev_present(USB_HOST_EXTCON_HID, NULL, -1)) { > info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_NAME); > - if (info->otg.cable == NULL) { > + if (IS_ERR(info->otg.cable)) { > dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n"); > - return -EPROBE_DEFER; > + return PTR_ERR(info->otg.cable); > } > dev_info(&pdev->dev, > "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n"); > diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c > index ee0863c6553e..605314ddcd3d 100644 > --- a/drivers/usb/phy/phy-omap-otg.c > +++ b/drivers/usb/phy/phy-omap-otg.c > @@ -91,12 +91,12 @@ static int omap_otg_probe(struct platform_device *pdev) > int ret; > u32 rev; > > - if (!config || !config->extcon) > + if (!config) > return -ENODEV; > > extcon = extcon_get_extcon_dev(config->extcon); > - if (!extcon) > - return -EPROBE_DEFER; > + if (IS_ERR(extcon)) > + return PTR_ERR(extcon); > > otg_dev = devm_kzalloc(&pdev->dev, sizeof(*otg_dev), GFP_KERNEL); > if (!otg_dev) > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index 43b64d9309d0..6d332066202b 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -1767,8 +1767,8 @@ static int fusb302_probe(struct i2c_client *client, > */ > if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) { > chip->extcon = extcon_get_extcon_dev(name); > - if (!chip->extcon) > - return -EPROBE_DEFER; > + if (IS_ERR(chip->extcon)) > + return PTR_ERR(chip->extcon); > } > > chip->vbus = devm_regulator_get(chip->dev, "vbus"); > -- > 2.19.1 >
Hi Andy, On 2018년 11월 11일 03:10, Andy Shevchenko wrote: > All current users of extcon_get_extcon_dev() API considers > an extcon device a mandatory to appear. Thus, they all convert > NULL pointer to -EPROBE_DEFER error code. > > There is one more caller anticipated with the same requirements. > > To decrease a code duplication and a burden to the callers, > return -EPROBE_DEFER directly from extcon_get_extcon_dev(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/extcon/extcon-axp288.c | 4 ++-- > drivers/extcon/extcon.c | 2 +- > drivers/power/supply/axp288_charger.c | 8 ++++---- > drivers/usb/phy/phy-omap-otg.c | 6 +++--- > drivers/usb/typec/tcpm/fusb302.c | 4 ++-- > 5 files changed, 12 insertions(+), 12 deletions(-) Acked-by: Chanwoo Choi <cw00.choi@samsung.com> Best Regards, Chanwoo Choi > > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > index a983708b77a6..3472d3b756ed 100644 > --- a/drivers/extcon/extcon-axp288.c > +++ b/drivers/extcon/extcon-axp288.c > @@ -360,8 +360,8 @@ static int axp288_extcon_probe(struct platform_device *pdev) > name = acpi_dev_get_first_match_name("INT3496", NULL, -1); > if (name) { > info->id_extcon = extcon_get_extcon_dev(name); > - if (!info->id_extcon) > - return -EPROBE_DEFER; > + if (IS_ERR(info->id_extcon)) > + return PTR_ERR(info->id_extcon); > > dev_info(dev, "controlling USB role\n"); > } else { > diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c > index 5ab0498be652..2bd0f2f33f05 100644 > --- a/drivers/extcon/extcon.c > +++ b/drivers/extcon/extcon.c > @@ -884,7 +884,7 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name) > if (!strcmp(sd->name, extcon_name)) > goto out; > } > - sd = NULL; > + sd = ERR_PTR(-EPROBE_DEFER); > out: > mutex_unlock(&extcon_dev_list_lock); > return sd; > diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c > index 735658ee1c60..8558577fccf5 100644 > --- a/drivers/power/supply/axp288_charger.c > +++ b/drivers/power/supply/axp288_charger.c > @@ -768,17 +768,17 @@ static int axp288_charger_probe(struct platform_device *pdev) > info->regmap_irqc = axp20x->regmap_irqc; > > info->cable.edev = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME); > - if (info->cable.edev == NULL) { > + if (IS_ERR(info->cable.edev)) { > dev_dbg(&pdev->dev, "%s is not ready, probe deferred\n", > AXP288_EXTCON_DEV_NAME); > - return -EPROBE_DEFER; > + return PTR_ERR(info->cable.edev); > } > > if (acpi_dev_present(USB_HOST_EXTCON_HID, NULL, -1)) { > info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_NAME); > - if (info->otg.cable == NULL) { > + if (IS_ERR(info->otg.cable)) { > dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n"); > - return -EPROBE_DEFER; > + return PTR_ERR(info->otg.cable); > } > dev_info(&pdev->dev, > "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n"); > diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c > index ee0863c6553e..605314ddcd3d 100644 > --- a/drivers/usb/phy/phy-omap-otg.c > +++ b/drivers/usb/phy/phy-omap-otg.c > @@ -91,12 +91,12 @@ static int omap_otg_probe(struct platform_device *pdev) > int ret; > u32 rev; > > - if (!config || !config->extcon) > + if (!config) > return -ENODEV; > > extcon = extcon_get_extcon_dev(config->extcon); > - if (!extcon) > - return -EPROBE_DEFER; > + if (IS_ERR(extcon)) > + return PTR_ERR(extcon); > > otg_dev = devm_kzalloc(&pdev->dev, sizeof(*otg_dev), GFP_KERNEL); > if (!otg_dev) > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index 43b64d9309d0..6d332066202b 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -1767,8 +1767,8 @@ static int fusb302_probe(struct i2c_client *client, > */ > if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) { > chip->extcon = extcon_get_extcon_dev(name); > - if (!chip->extcon) > - return -EPROBE_DEFER; > + if (IS_ERR(chip->extcon)) > + return PTR_ERR(chip->extcon); > } > > chip->vbus = devm_regulator_get(chip->dev, "vbus"); >
On Sat, Nov 10, 2018 at 08:10:58PM +0200, Andy Shevchenko wrote: > All current users of extcon_get_extcon_dev() API considers > an extcon device a mandatory to appear. Thus, they all convert > NULL pointer to -EPROBE_DEFER error code. > > There is one more caller anticipated with the same requirements. > > To decrease a code duplication and a burden to the callers, > return -EPROBE_DEFER directly from extcon_get_extcon_dev(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/extcon/extcon-axp288.c | 4 ++-- > drivers/extcon/extcon.c | 2 +- > drivers/power/supply/axp288_charger.c | 8 ++++---- > drivers/usb/phy/phy-omap-otg.c | 6 +++--- > drivers/usb/typec/tcpm/fusb302.c | 4 ++-- > 5 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > index a983708b77a6..3472d3b756ed 100644 > --- a/drivers/extcon/extcon-axp288.c > +++ b/drivers/extcon/extcon-axp288.c > @@ -360,8 +360,8 @@ static int axp288_extcon_probe(struct platform_device *pdev) > name = acpi_dev_get_first_match_name("INT3496", NULL, -1); > if (name) { > info->id_extcon = extcon_get_extcon_dev(name); > - if (!info->id_extcon) > - return -EPROBE_DEFER; > + if (IS_ERR(info->id_extcon)) > + return PTR_ERR(info->id_extcon); > > dev_info(dev, "controlling USB role\n"); > } else { > diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c > index 5ab0498be652..2bd0f2f33f05 100644 > --- a/drivers/extcon/extcon.c > +++ b/drivers/extcon/extcon.c > @@ -884,7 +884,7 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name) > if (!strcmp(sd->name, extcon_name)) > goto out; > } > - sd = NULL; > + sd = ERR_PTR(-EPROBE_DEFER); > out: > mutex_unlock(&extcon_dev_list_lock); > return sd; > diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c > index 735658ee1c60..8558577fccf5 100644 > --- a/drivers/power/supply/axp288_charger.c > +++ b/drivers/power/supply/axp288_charger.c > @@ -768,17 +768,17 @@ static int axp288_charger_probe(struct platform_device *pdev) > info->regmap_irqc = axp20x->regmap_irqc; > > info->cable.edev = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME); > - if (info->cable.edev == NULL) { > + if (IS_ERR(info->cable.edev)) { > dev_dbg(&pdev->dev, "%s is not ready, probe deferred\n", > AXP288_EXTCON_DEV_NAME); > - return -EPROBE_DEFER; > + return PTR_ERR(info->cable.edev); > } > > if (acpi_dev_present(USB_HOST_EXTCON_HID, NULL, -1)) { > info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_NAME); > - if (info->otg.cable == NULL) { > + if (IS_ERR(info->otg.cable)) { > dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n"); > - return -EPROBE_DEFER; > + return PTR_ERR(info->otg.cable); > } > dev_info(&pdev->dev, > "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n"); > diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c > index ee0863c6553e..605314ddcd3d 100644 > --- a/drivers/usb/phy/phy-omap-otg.c > +++ b/drivers/usb/phy/phy-omap-otg.c > @@ -91,12 +91,12 @@ static int omap_otg_probe(struct platform_device *pdev) > int ret; > u32 rev; > > - if (!config || !config->extcon) > + if (!config) > return -ENODEV; > > extcon = extcon_get_extcon_dev(config->extcon); > - if (!extcon) > - return -EPROBE_DEFER; > + if (IS_ERR(extcon)) > + return PTR_ERR(extcon); > > otg_dev = devm_kzalloc(&pdev->dev, sizeof(*otg_dev), GFP_KERNEL); > if (!otg_dev) > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index 43b64d9309d0..6d332066202b 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -1767,8 +1767,8 @@ static int fusb302_probe(struct i2c_client *client, > */ > if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) { > chip->extcon = extcon_get_extcon_dev(name); > - if (!chip->extcon) > - return -EPROBE_DEFER; > + if (IS_ERR(chip->extcon)) > + return PTR_ERR(chip->extcon); > } > > chip->vbus = devm_regulator_get(chip->dev, "vbus"); > -- > 2.19.1
Hi Andy, I was thinking about again to change from NULL to EPROBE_DEFER. extcon_get_extcon_dev() function was almost called in the probe function. But, this function might be called on other position instead of probe. ENODEV is more correct error instead of EPROBE_DEFER. Sorry. I'll withdraw my opinion related acked-by tag until we are clarifying it. On 2018년 11월 12일 09:24, Chanwoo Choi wrote: > Hi Andy, > > On 2018년 11월 11일 03:10, Andy Shevchenko wrote: >> All current users of extcon_get_extcon_dev() API considers >> an extcon device a mandatory to appear. Thus, they all convert >> NULL pointer to -EPROBE_DEFER error code. >> >> There is one more caller anticipated with the same requirements. >> >> To decrease a code duplication and a burden to the callers, >> return -EPROBE_DEFER directly from extcon_get_extcon_dev(). >> >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> drivers/extcon/extcon-axp288.c | 4 ++-- >> drivers/extcon/extcon.c | 2 +- >> drivers/power/supply/axp288_charger.c | 8 ++++---- >> drivers/usb/phy/phy-omap-otg.c | 6 +++--- >> drivers/usb/typec/tcpm/fusb302.c | 4 ++-- >> 5 files changed, 12 insertions(+), 12 deletions(-) > > Acked-by: Chanwoo Choi <cw00.choi@samsung.com> > > Best Regards, > Chanwoo Choi > >> >> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c >> index a983708b77a6..3472d3b756ed 100644 >> --- a/drivers/extcon/extcon-axp288.c >> +++ b/drivers/extcon/extcon-axp288.c >> @@ -360,8 +360,8 @@ static int axp288_extcon_probe(struct platform_device *pdev) >> name = acpi_dev_get_first_match_name("INT3496", NULL, -1); >> if (name) { >> info->id_extcon = extcon_get_extcon_dev(name); >> - if (!info->id_extcon) >> - return -EPROBE_DEFER; >> + if (IS_ERR(info->id_extcon)) >> + return PTR_ERR(info->id_extcon); >> >> dev_info(dev, "controlling USB role\n"); >> } else { >> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c >> index 5ab0498be652..2bd0f2f33f05 100644 >> --- a/drivers/extcon/extcon.c >> +++ b/drivers/extcon/extcon.c >> @@ -884,7 +884,7 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name) >> if (!strcmp(sd->name, extcon_name)) >> goto out; >> } >> - sd = NULL; >> + sd = ERR_PTR(-EPROBE_DEFER); (snip)
On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote: > I was thinking about again to change from NULL to EPROBE_DEFER. > > extcon_get_extcon_dev() function was almost called in the probe function. > But, this function might be called on other position instead of probe. *Might be* sounds like a theoretical thing, care to share what is in you mind? Current users and more important the new coming one are *all* doing the same. > ENODEV is more correct error instead of EPROBE_DEFER. So, you are proposing to continue duplicating conversion from ENODEV to EPROBE_DEFER in *each* caller? > Sorry. I'll withdraw my opinion related acked-by tag until we are clarifying it. I honestly don't know what to clarify here. When we would have a real case we can change API correspondingly. For now, the score is 5:0 with use cases in practice. > On 2018년 11월 12일 09:24, Chanwoo Choi wrote: > > On 2018년 11월 11일 03:10, Andy Shevchenko wrote: > >> All current users of extcon_get_extcon_dev() API considers > >> an extcon device a mandatory to appear. Thus, they all convert > >> NULL pointer to -EPROBE_DEFER error code. > >> > >> There is one more caller anticipated with the same requirements. > >> > >> To decrease a code duplication and a burden to the callers, > >> return -EPROBE_DEFER directly from extcon_get_extcon_dev().
On 2018년 11월 14일 17:35, Andy Shevchenko wrote: > On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote: > >> I was thinking about again to change from NULL to EPROBE_DEFER. >> >> extcon_get_extcon_dev() function was almost called in the probe function. >> But, this function might be called on other position instead of probe. > > *Might be* sounds like a theoretical thing, care to share what is in you mind? > Current users and more important the new coming one are *all* doing the same. > >> ENODEV is more correct error instead of EPROBE_DEFER. > > So, you are proposing to continue duplicating conversion from ENODEV > to EPROBE_DEFER in *each* caller? The extcon core don't know the caller situation is in either probe() or other position in the caller driver. The caller driver should decide the kind of error value by using the return value of extcon_get_extcon_dev(). extcon_get_extcon_dev() function cannot be modified for only one case. If some device driver call extcon_get_extcon_dev() out of probe() fuction, EPROBE_DEFER is not always correct. > >> Sorry. I'll withdraw my opinion related acked-by tag until we are clarifying it. > > I honestly don't know what to clarify here. > > When we would have a real case we can change API correspondingly. > For now, the score is 5:0 with use cases in practice. > >> On 2018년 11월 12일 09:24, Chanwoo Choi wrote: >>> On 2018년 11월 11일 03:10, Andy Shevchenko wrote: >>>> All current users of extcon_get_extcon_dev() API considers >>>> an extcon device a mandatory to appear. Thus, they all convert >>>> NULL pointer to -EPROBE_DEFER error code. >>>> >>>> There is one more caller anticipated with the same requirements. >>>> >>>> To decrease a code duplication and a burden to the callers, >>>> return -EPROBE_DEFER directly from extcon_get_extcon_dev(). >
On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote: > On 2018년 11월 14일 17:35, Andy Shevchenko wrote: > > On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote: > > > >> I was thinking about again to change from NULL to EPROBE_DEFER. > >> > >> extcon_get_extcon_dev() function was almost called in the probe function. > >> But, this function might be called on other position instead of probe. > > > > *Might be* sounds like a theoretical thing, care to share what is in you mind? > > Current users and more important the new coming one are *all* doing the same. > > > >> ENODEV is more correct error instead of EPROBE_DEFER. > > > > So, you are proposing to continue duplicating conversion from ENODEV > > to EPROBE_DEFER in *each* caller? > > The extcon core don't know the caller situation is in either probe() or other position > in the caller driver. The caller driver should decide the kind of error value > by using the return value of extcon_get_extcon_dev(). > > extcon_get_extcon_dev() function cannot be modified for only one case. > If some device driver call extcon_get_extcon_dev() out of probe() fuction, > EPROBE_DEFER is not always correct. I agree with this, but look at the current state of affairs. All users do the same. If we need to have another case we may consider this later. API inside the kernel are not carved in the stone.
On 2018년 11월 14일 18:36, Andy Shevchenko wrote: > On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote: >> On 2018년 11월 14일 17:35, Andy Shevchenko wrote: >>> On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote: >>> >>>> I was thinking about again to change from NULL to EPROBE_DEFER. >>>> >>>> extcon_get_extcon_dev() function was almost called in the probe function. >>>> But, this function might be called on other position instead of probe. >>> >>> *Might be* sounds like a theoretical thing, care to share what is in you mind? >>> Current users and more important the new coming one are *all* doing the same. >>> >>>> ENODEV is more correct error instead of EPROBE_DEFER. >>> >>> So, you are proposing to continue duplicating conversion from ENODEV >>> to EPROBE_DEFER in *each* caller? >> >> The extcon core don't know the caller situation is in either probe() or other position >> in the caller driver. The caller driver should decide the kind of error value >> by using the return value of extcon_get_extcon_dev(). >> >> extcon_get_extcon_dev() function cannot be modified for only one case. >> If some device driver call extcon_get_extcon_dev() out of probe() fuction, >> EPROBE_DEFER is not always correct. > > I agree with this, but look at the current state of affairs. All users do the same. > If we need to have another case we may consider this later. Because we know the potential wrong case of this change, I can't agree this change. If extcon_get_extcon_dev() returns ENODEV instead of EPROBE_DEFER, it is clear and then there are no problem on both current and future. > > API inside the kernel are not carved in the stone. > >
On Wed, Nov 14, 2018 at 11:48 AM Chanwoo Choi <cw00.choi@samsung.com> wrote: > > On 2018년 11월 14일 18:36, Andy Shevchenko wrote: > > On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote: > >> On 2018년 11월 14일 17:35, Andy Shevchenko wrote: > >>> On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote: > >>> > >>>> I was thinking about again to change from NULL to EPROBE_DEFER. > >>>> > >>>> extcon_get_extcon_dev() function was almost called in the probe function. > >>>> But, this function might be called on other position instead of probe. > >>> > >>> *Might be* sounds like a theoretical thing, care to share what is in you mind? > >>> Current users and more important the new coming one are *all* doing the same. > >>> > >>>> ENODEV is more correct error instead of EPROBE_DEFER. > >>> > >>> So, you are proposing to continue duplicating conversion from ENODEV > >>> to EPROBE_DEFER in *each* caller? > >> > >> The extcon core don't know the caller situation is in either probe() or other position > >> in the caller driver. The caller driver should decide the kind of error value > >> by using the return value of extcon_get_extcon_dev(). > >> > >> extcon_get_extcon_dev() function cannot be modified for only one case. > >> If some device driver call extcon_get_extcon_dev() out of probe() fuction, > >> EPROBE_DEFER is not always correct. > > > > I agree with this, but look at the current state of affairs. All users do the same. > > If we need to have another case we may consider this later. > > Because we know the potential wrong case of this change, I can't agree this change. > If extcon_get_extcon_dev() returns ENODEV instead of EPROBE_DEFER, > it is clear and then there are no problem on both current and future. Changing NULL to -ENODEV is a trading bad to worse. I would not go that way, so, it's your call. > > API inside the kernel are not carved in the stone. Only can repeat myself (see above). While I see *theoretical* rationale on your side, mine has *practical* proofs. So, I'm giving up on this and will duplicate same what it's done in 4 current callers.
On 2018년 11월 14일 19:20, Andy Shevchenko wrote: > On Wed, Nov 14, 2018 at 11:48 AM Chanwoo Choi <cw00.choi@samsung.com> wrote: >> >> On 2018년 11월 14일 18:36, Andy Shevchenko wrote: >>> On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote: >>>> On 2018년 11월 14일 17:35, Andy Shevchenko wrote: >>>>> On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote: >>>>> >>>>>> I was thinking about again to change from NULL to EPROBE_DEFER. >>>>>> >>>>>> extcon_get_extcon_dev() function was almost called in the probe function. >>>>>> But, this function might be called on other position instead of probe. >>>>> >>>>> *Might be* sounds like a theoretical thing, care to share what is in you mind? >>>>> Current users and more important the new coming one are *all* doing the same. >>>>> >>>>>> ENODEV is more correct error instead of EPROBE_DEFER. >>>>> >>>>> So, you are proposing to continue duplicating conversion from ENODEV >>>>> to EPROBE_DEFER in *each* caller? >>>> >>>> The extcon core don't know the caller situation is in either probe() or other position >>>> in the caller driver. The caller driver should decide the kind of error value >>>> by using the return value of extcon_get_extcon_dev(). >>>> >>>> extcon_get_extcon_dev() function cannot be modified for only one case. >>>> If some device driver call extcon_get_extcon_dev() out of probe() fuction, >>>> EPROBE_DEFER is not always correct. >>> >>> I agree with this, but look at the current state of affairs. All users do the same. >>> If we need to have another case we may consider this later. >> >> Because we know the potential wrong case of this change, I can't agree this change. >> If extcon_get_extcon_dev() returns ENODEV instead of EPROBE_DEFER, >> it is clear and then there are no problem on both current and future. > > Changing NULL to -ENODEV is a trading bad to worse. > I would not go that way, so, it's your call. If you think that this change is not necessary, just keep the current code without the modification. Please drop this patch on next version. > >>> API inside the kernel are not carved in the stone. > > Only can repeat myself (see above). While I see *theoretical* > rationale on your side, mine has *practical* proofs. > So, I'm giving up on this and will duplicate same what it's done in 4 > current callers. > I think that it is more important for removing the potential wrong case instead of removing the duplicate code. The many device drivers already decided the proper error value by using the return value of function of kernel framework.
On Wed, Nov 14, 2018 at 1:05 PM Chanwoo Choi <cw00.choi@samsung.com> wrote: > On 2018년 11월 14일 19:20, Andy Shevchenko wrote: > > On Wed, Nov 14, 2018 at 11:48 AM Chanwoo Choi <cw00.choi@samsung.com> wrote: > >> On 2018년 11월 14일 18:36, Andy Shevchenko wrote: > >>> On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote: > >>>> On 2018년 11월 14일 17:35, Andy Shevchenko wrote: > >>>>> On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi <cw00.choi@samsung.com> wrote: > >>>>> > >>>>>> I was thinking about again to change from NULL to EPROBE_DEFER. > >>>>>> > >>>>>> extcon_get_extcon_dev() function was almost called in the probe function. > >>>>>> But, this function might be called on other position instead of probe. > >>>>> > >>>>> *Might be* sounds like a theoretical thing, care to share what is in you mind? > >>>>> Current users and more important the new coming one are *all* doing the same. > >>>>> > >>>>>> ENODEV is more correct error instead of EPROBE_DEFER. > >>>>> > >>>>> So, you are proposing to continue duplicating conversion from ENODEV > >>>>> to EPROBE_DEFER in *each* caller? > >>>> > >>>> The extcon core don't know the caller situation is in either probe() or other position > >>>> in the caller driver. The caller driver should decide the kind of error value > >>>> by using the return value of extcon_get_extcon_dev(). > >>>> > >>>> extcon_get_extcon_dev() function cannot be modified for only one case. > >>>> If some device driver call extcon_get_extcon_dev() out of probe() fuction, > >>>> EPROBE_DEFER is not always correct. > >>> > >>> I agree with this, but look at the current state of affairs. All users do the same. > >>> If we need to have another case we may consider this later. > >> > >> Because we know the potential wrong case of this change, I can't agree this change. > >> If extcon_get_extcon_dev() returns ENODEV instead of EPROBE_DEFER, > >> it is clear and then there are no problem on both current and future. > > > > Changing NULL to -ENODEV is a trading bad to worse. > > I would not go that way, so, it's your call. > > If you think that this change is not necessary, just keep the current code > without the modification. Not only this, the useless churn for no benefit to anyone, except some *theoretical* cases no one saw. > Please drop this patch on next version. I will. > >>> API inside the kernel are not carved in the stone. > > > > Only can repeat myself (see above). While I see *theoretical* > > rationale on your side, mine has *practical* proofs. > > So, I'm giving up on this and will duplicate same what it's done in 4 > > current callers. > > > > I think that it is more important for removing the potential wrong case > instead of removing the duplicate code. The many device drivers already > decided the proper error value by using the return value of function of > kernel framework. The API has been introduced back in 2012. commit 74c5d09bd562edc220d6e076b8f1e118819c178f Author: Donggeun Kim <dg77.kim@samsung.com> Date: Fri Apr 20 14:16:24 2012 +0900 So, you are insisting that 6.5 years of use in a way people are using it is wrong? I don't know what might change your mind, but for me it's a clear win-win to switch to deferred probe error code based on the *practical* evidence. But as I said, I gave up now. P.S. I still disagree with your arguments in relation to de facto use of an API.
On Wed, Nov 14, 2018 at 1:17 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Nov 14, 2018 at 1:05 PM Chanwoo Choi <cw00.choi@samsung.com> wrote: > > > Changing NULL to -ENODEV is a trading bad to worse. > P.S. I still disagree with your arguments in relation to de facto use of an API. I spoke to colleagues of mine and they are agree that semantically -EPROBE_DEFER is a wrong error code from API that matches against some list. On the other hand, they agree with me that changing NULL to -ENODEV is a trading bad to worse. So, I withdraw mine complaints against API.
On 2018년 11월 14일 23:04, Andy Shevchenko wrote: > On Wed, Nov 14, 2018 at 1:17 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Wed, Nov 14, 2018 at 1:05 PM Chanwoo Choi <cw00.choi@samsung.com> wrote: > >>>> Changing NULL to -ENODEV is a trading bad to worse. > >> P.S. I still disagree with your arguments in relation to de facto use of an API. > > I spoke to colleagues of mine and they are agree that semantically > -EPROBE_DEFER is a wrong error code from API that matches against some > list. > On the other hand, they agree with me that changing NULL to -ENODEV is > a trading bad to worse. > > So, I withdraw mine complaints against API. > OK. Thanks.
diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c index a983708b77a6..3472d3b756ed 100644 --- a/drivers/extcon/extcon-axp288.c +++ b/drivers/extcon/extcon-axp288.c @@ -360,8 +360,8 @@ static int axp288_extcon_probe(struct platform_device *pdev) name = acpi_dev_get_first_match_name("INT3496", NULL, -1); if (name) { info->id_extcon = extcon_get_extcon_dev(name); - if (!info->id_extcon) - return -EPROBE_DEFER; + if (IS_ERR(info->id_extcon)) + return PTR_ERR(info->id_extcon); dev_info(dev, "controlling USB role\n"); } else { diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c index 5ab0498be652..2bd0f2f33f05 100644 --- a/drivers/extcon/extcon.c +++ b/drivers/extcon/extcon.c @@ -884,7 +884,7 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name) if (!strcmp(sd->name, extcon_name)) goto out; } - sd = NULL; + sd = ERR_PTR(-EPROBE_DEFER); out: mutex_unlock(&extcon_dev_list_lock); return sd; diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c index 735658ee1c60..8558577fccf5 100644 --- a/drivers/power/supply/axp288_charger.c +++ b/drivers/power/supply/axp288_charger.c @@ -768,17 +768,17 @@ static int axp288_charger_probe(struct platform_device *pdev) info->regmap_irqc = axp20x->regmap_irqc; info->cable.edev = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME); - if (info->cable.edev == NULL) { + if (IS_ERR(info->cable.edev)) { dev_dbg(&pdev->dev, "%s is not ready, probe deferred\n", AXP288_EXTCON_DEV_NAME); - return -EPROBE_DEFER; + return PTR_ERR(info->cable.edev); } if (acpi_dev_present(USB_HOST_EXTCON_HID, NULL, -1)) { info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_NAME); - if (info->otg.cable == NULL) { + if (IS_ERR(info->otg.cable)) { dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n"); - return -EPROBE_DEFER; + return PTR_ERR(info->otg.cable); } dev_info(&pdev->dev, "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n"); diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c index ee0863c6553e..605314ddcd3d 100644 --- a/drivers/usb/phy/phy-omap-otg.c +++ b/drivers/usb/phy/phy-omap-otg.c @@ -91,12 +91,12 @@ static int omap_otg_probe(struct platform_device *pdev) int ret; u32 rev; - if (!config || !config->extcon) + if (!config) return -ENODEV; extcon = extcon_get_extcon_dev(config->extcon); - if (!extcon) - return -EPROBE_DEFER; + if (IS_ERR(extcon)) + return PTR_ERR(extcon); otg_dev = devm_kzalloc(&pdev->dev, sizeof(*otg_dev), GFP_KERNEL); if (!otg_dev) diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c index 43b64d9309d0..6d332066202b 100644 --- a/drivers/usb/typec/tcpm/fusb302.c +++ b/drivers/usb/typec/tcpm/fusb302.c @@ -1767,8 +1767,8 @@ static int fusb302_probe(struct i2c_client *client, */ if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) { chip->extcon = extcon_get_extcon_dev(name); - if (!chip->extcon) - return -EPROBE_DEFER; + if (IS_ERR(chip->extcon)) + return PTR_ERR(chip->extcon); } chip->vbus = devm_regulator_get(chip->dev, "vbus");
All current users of extcon_get_extcon_dev() API considers an extcon device a mandatory to appear. Thus, they all convert NULL pointer to -EPROBE_DEFER error code. There is one more caller anticipated with the same requirements. To decrease a code duplication and a burden to the callers, return -EPROBE_DEFER directly from extcon_get_extcon_dev(). Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/extcon/extcon-axp288.c | 4 ++-- drivers/extcon/extcon.c | 2 +- drivers/power/supply/axp288_charger.c | 8 ++++---- drivers/usb/phy/phy-omap-otg.c | 6 +++--- drivers/usb/typec/tcpm/fusb302.c | 4 ++-- 5 files changed, 12 insertions(+), 12 deletions(-)