[PATCH] PM / Domains: Replace -ENOSYS with -ENODEV

[PATCH] PM / Domains: Replace -ENOSYS with -ENODEV

[Lina Iyer]

ENOSYS is for syscalls that are not supported. ENODEV is a better return
value when the PM Domains are not available.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 include/linux/pm_domain.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 688dc57..8a63406 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -151,7 +151,7 @@ extern struct dev_power_governor pm_domain_always_on_gov;
 
 static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
 {
-	return ERR_PTR(-ENOSYS);
+	return ERR_PTR(-ENODEV);
 }
 static inline struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev)
 {
@@ -161,27 +161,27 @@ static inline int __pm_genpd_add_device(struct generic_pm_domain *genpd,
 					struct device *dev,
 					struct gpd_timing_data *td)
 {
-	return -ENOSYS;
+	return -ENODEV;
 }
 static inline int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 					 struct device *dev)
 {
-	return -ENOSYS;
+	return -ENODEV;
 }
 static inline int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 					 struct generic_pm_domain *new_sd)
 {
-	return -ENOSYS;
+	return -ENODEV;
 }
 static inline int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 					    struct generic_pm_domain *target)
 {
-	return -ENOSYS;
+	return -ENODEV;
 }
 static inline int pm_genpd_init(struct generic_pm_domain *genpd,
 				struct dev_power_governor *gov, bool is_off)
 {
-	return -ENOSYS;
+	return -ENODEV;
 }
 static inline int pm_genpd_of_parse_power_states(
 				struct generic_pm_domain *genpd)
-- 
2.7.4

Re: [PATCH] PM / Domains: Replace -ENOSYS with -ENODEV

[Lukas Wunner]

On Fri, Aug 05, 2016 at 11:06:56AM -0600, Lina Iyer wrote:
> ENOSYS is for syscalls that are not supported. ENODEV is a better return
> value when the PM Domains are not available.
> 
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>

I have no opinion on this, but would like to point out that other parts
in the PM core use -ENOSYS as well (e.g. see drivers/base/power/core.c).
For consistency, if pm_domain.h is changed, the rest should probably be
changed as well. It should also be kept in mind that functions might
check the return code and react specifically to the currently used -ENOSYS.
(I've written code like that just yesterday, for the (unmerged) thunderbolt
runtime pm.)

Thanks,

Lukas

> ---
>  include/linux/pm_domain.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 688dc57..8a63406 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -151,7 +151,7 @@ extern struct dev_power_governor pm_domain_always_on_gov;
>  
>  static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
>  {
> -	return ERR_PTR(-ENOSYS);
> +	return ERR_PTR(-ENODEV);
>  }
>  static inline struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev)
>  {
> @@ -161,27 +161,27 @@ static inline int __pm_genpd_add_device(struct generic_pm_domain *genpd,
>  					struct device *dev,
>  					struct gpd_timing_data *td)
>  {
> -	return -ENOSYS;
> +	return -ENODEV;
>  }
>  static inline int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>  					 struct device *dev)
>  {
> -	return -ENOSYS;
> +	return -ENODEV;
>  }
>  static inline int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>  					 struct generic_pm_domain *new_sd)
>  {
> -	return -ENOSYS;
> +	return -ENODEV;
>  }
>  static inline int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>  					    struct generic_pm_domain *target)
>  {
> -	return -ENOSYS;
> +	return -ENODEV;
>  }
>  static inline int pm_genpd_init(struct generic_pm_domain *genpd,
>  				struct dev_power_governor *gov, bool is_off)
>  {
> -	return -ENOSYS;
> +	return -ENODEV;
>  }
>  static inline int pm_genpd_of_parse_power_states(
>  				struct generic_pm_domain *genpd)
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PM / Domains: Replace -ENOSYS with -ENODEV

[Rafael J. Wysocki]

On Fri, Aug 5, 2016 at 9:43 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Fri, Aug 05, 2016 at 11:06:56AM -0600, Lina Iyer wrote:
>> ENOSYS is for syscalls that are not supported. ENODEV is a better return
>> value when the PM Domains are not available.
>>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>
> I have no opinion on this, but would like to point out that other parts
> in the PM core use -ENOSYS as well (e.g. see drivers/base/power/core.c).
> For consistency, if pm_domain.h is changed, the rest should probably be
> changed as well.

I agree here.  Changing one piece only is likely to result in confusion.

Also I'm not sure if ENODEV is a good replacement at all.  ENOTSUPP
was suggested instead last time a similar patch was discussed.

> It should also be kept in mind that functions might
> check the return code and react specifically to the currently used -ENOSYS.
> (I've written code like that just yesterday, for the (unmerged) thunderbolt
> runtime pm.)

But I wouldn't write code doing something like this.

Thanks,
Rafael

Re: [PATCH] PM / Domains: Replace -ENOSYS with -ENODEV

[Lukas Wunner]

On Sat, Aug 06, 2016 at 12:17:58AM +0200, Rafael J. Wysocki wrote:
> On Fri, Aug 5, 2016 at 9:43 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Fri, Aug 05, 2016 at 11:06:56AM -0600, Lina Iyer wrote:
> >> ENOSYS is for syscalls that are not supported. ENODEV is a better return
> >> value when the PM Domains are not available.
> >>
> >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> >
> > I have no opinion on this, but would like to point out that other parts
> > in the PM core use -ENOSYS as well (e.g. see drivers/base/power/core.c).

I meant drivers/base/power/runtime.c and include/linux/pm_runtime.h,
apologies.

> > For consistency, if pm_domain.h is changed, the rest should probably be
> > changed as well.
> 
> I agree here.  Changing one piece only is likely to result in confusion.
> 
> Also I'm not sure if ENODEV is a good replacement at all.  ENOTSUPP
> was suggested instead last time a similar patch was discussed.
> 
> > It should also be kept in mind that functions might
> > check the return code and react specifically to the currently used -ENOSYS.
> > (I've written code like that just yesterday, for the (unmerged) thunderbolt
> > runtime pm.)
> 
> But I wouldn't write code doing something like this.

Understood, I just removed it from my tree. :-)

Thanks,

Lukas

Re: [PATCH] PM / Domains: Replace -ENOSYS with -ENODEV

[Geert Uytterhoeven]

Hi Rafael,

On Sat, Aug 6, 2016 at 12:17 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Aug 5, 2016 at 9:43 PM, Lukas Wunner <lukas@wunner.de> wrote:
>> On Fri, Aug 05, 2016 at 11:06:56AM -0600, Lina Iyer wrote:
>>> ENOSYS is for syscalls that are not supported. ENODEV is a better return
>>> value when the PM Domains are not available.
>>>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>
>> I have no opinion on this, but would like to point out that other parts
>> in the PM core use -ENOSYS as well (e.g. see drivers/base/power/core.c).
>> For consistency, if pm_domain.h is changed, the rest should probably be
>> changed as well.

ENOSYS has been used to indicate "functionality not supported" all over the
place...

> I agree here.  Changing one piece only is likely to result in confusion.
>
> Also I'm not sure if ENODEV is a good replacement at all.  ENOTSUPP
> was suggested instead last time a similar patch was discussed.
>
>> It should also be kept in mind that functions might
>> check the return code and react specifically to the currently used -ENOSYS.
>> (I've written code like that just yesterday, for the (unmerged) thunderbolt
>> runtime pm.)
>
> But I wouldn't write code doing something like this.

(in the general case, not limited to PM) What to do instead, if you know a
dummy function returns -ENOSYS and you need to react on that?

In these cases the exact error code is a contract between caller and
callee.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] PM / Domains: Replace -ENOSYS with -ENODEV

[Rafael J. Wysocki]

On Monday, August 08, 2016 11:11:11 AM Geert Uytterhoeven wrote:
> Hi Rafael,
> 
> On Sat, Aug 6, 2016 at 12:17 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Fri, Aug 5, 2016 at 9:43 PM, Lukas Wunner <lukas@wunner.de> wrote:
> >> On Fri, Aug 05, 2016 at 11:06:56AM -0600, Lina Iyer wrote:
> >>> ENOSYS is for syscalls that are not supported. ENODEV is a better return
> >>> value when the PM Domains are not available.
> >>>
> >>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> >>
> >> I have no opinion on this, but would like to point out that other parts
> >> in the PM core use -ENOSYS as well (e.g. see drivers/base/power/core.c).
> >> For consistency, if pm_domain.h is changed, the rest should probably be
> >> changed as well.
> 
> ENOSYS has been used to indicate "functionality not supported" all over the
> place...
> 
> > I agree here.  Changing one piece only is likely to result in confusion.
> >
> > Also I'm not sure if ENODEV is a good replacement at all.  ENOTSUPP
> > was suggested instead last time a similar patch was discussed.
> >
> >> It should also be kept in mind that functions might
> >> check the return code and react specifically to the currently used -ENOSYS.
> >> (I've written code like that just yesterday, for the (unmerged) thunderbolt
> >> runtime pm.)
> >
> > But I wouldn't write code doing something like this.
> 
> (in the general case, not limited to PM) What to do instead, if you know a
> dummy function returns -ENOSYS and you need to react on that?

Use IS_ENABLED()?

That at least will make the intention clear to an occasional reader of the code.

> In these cases the exact error code is a contract between caller and
> callee.

That's fair enough.

Thanks,
Rafael